[buildd-tools-devel] Bug#605939: Bug#605939: Bug#605939: Bug#605939: Regression: Chroots with periods in the name no longer work.

[Roger Leigh]

On Sat, Dec 04, 2010 at 09:16:39PM -0500, Andres Mejia wrote:
> On Saturday 04 December 2010 17:36:02 Roger Leigh wrote:
> > severity 601043 important
> > merge 601043 605939
> > thanks
> > 
> > On Sat, Dec 04, 2010 at 03:25:00PM -0500, Nelson Elhage wrote:
> > > As of schroot commit 8c1c93708397bc08519a9415da96fbdd9e26315e
> > > (released with version 1.4.9), chroots with periods in their name no
> > > longer work.
> > > 
> > > I personally find chroot names with dots useful, since I keep chroots
> > > around as build/test environments for different versions of various
> > > pieces of software, and I name them after the software version
> > > (x.y.z).
> > > 
> > > I've attached a patch which adds '.' back in to is_valid_sessionname.
> > 
> > Thanks for the patch.  I am planning to relax the restriction shortly,
> > but it does need some checking of other parts of the codebase to
> > ensure we aren't opening up a security hole (which is why we restricted
> > the allowed characters).
> > 
> > A leading '.' is particularly troublesome since it would allow one
> > to overwrite files on the host system with a session name containing
> > "../../" etc.  For this reason, we would need to use
> > 
> > static regex file_namespace("^[a-zA-Z0-9][a-zA-Z0-9_.-]*$");
> > 
> > in place of:
> > 
> > static regex file_namespace("^[a-zA-Z0-9.][a-zA-Z0-9_.-]*$");
> > 
> > We already restrict the use of '/', so this one isn't too likely at
> > present, but there were some other cases I wasn't so sure about.
> > Once I've checked, I'll relax the restriction.
> > 
> > 
> > Regards,
> > Roger
> 
> As I understand this, a valid chroot name should be a string in a language of
> all strings generated by a regular expression [a-zA-Z0-9_.-]+ which excludes
> the strings '.' and '..'.
> 
> Perhaps the regular expression used should be:
> static regex file_namespace("^([.]{2}[a-zA-Z0-9_.-]+|[.]?[a-zA-Z0-9_-][a-zA-Z0-9_.-]*)$");
> 
> I've attached a patch that can be used to test this regular expression as well
> as describe the DFA used to construct this regular expression.

Thanks.  I've looked at where all the chroot/session and filename
validation occurs in schroot:

is_valid_sessionname:
  schroot::options_base (validate user input)
   schroot::main_base (actual use)
   → session::set_session_id
   → chroot::clone_session
   → chroot_facet_session_clonable::clone_session_setup
   → chroot::set_name
  sbuild::chroot (set_name)
  sbuild::chroot (set_aliases [missing, added by me today])
  sbuild::chroot_config::add_config_directory (validate filename)

is_valid_filename
  sbuild::run_parts (not used for session names at all)

So the only function of is_valid_sessionname is to ensure we have a
"safe" session name when writing out a session file and/or making
use of that session name in setup scripts.  We're not skipping needed
validation anywhere that I can see.  set_name is the main point of
checking; everything else is just aborting earlier when a good
diagnostic can be issued (e.g. validating options and filenames),
but they would hit the set_name check ultimately if the extra
checks weren't present.

Requirements:
 - no leading dot to allow writing in parent directories
 - no slashes to allow writing in subdirectories
 - no colons (not a security issue, but LVM snapshot names can't
   contain a : or else lvcreate errors out)
 - no commas (we use comma-separated lists in the config file, so
   alias names can't contain a comma)

So, we could get away with a really simple regex:

  static regex file_namespace("^[^:/,.][^:/,]*$");
  static regex debian_dpkg_conffile_cruft("dpkg-(old|dist|new|tmp)$");

  if (regex_search(name, file_namespace) &&
      !regex_search(name, debian_dpkg_conffile_cruft)) {
    match = true;
  }

I've added the dpkg checks from is_valid_filename, because these are
also needed for avoiding conffile cruft under /etc/schroot/chroot.d
(previously, the existing restrictions prevented this anyway).  So
dots are allowed anywhere except the first position, and ':', '/' and
',' are not permitted anywhere.

Does this look OK?  It's probably about as permissive as we can be.

Can anyone see any downside from being this permissive, or any
security implication I've not seen?  (I'm only looking at pathname-
based security exploits here--is there anything else we need to
worry about?)


Regards,
Roger

-- 
  .''`.  Roger Leigh
 : :' :  Debian GNU/Linux             http://people.debian.org/~rleigh/
 `. `'   Printing on GNU/Linux?       http://gutenprint.sourceforge.net/
   `-    GPG Public Key: 0x25BFB848   Please GPG sign your mail.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.alioth.debian.org/pipermail/buildd-tools-devel/attachments/20101205/8ff508e8/attachment.pgp>