[buildd-tools-devel] Bug#597778: Bug#597778: schroot: Stupid name restrictions is breaking existing setups for no good reason.

[Roger Leigh]

severity 597778 important
thanks

On Wed, Sep 22, 2010 at 05:21:15PM -0400, Len Sorensen wrote:
> Package: schroot
> Version: 1.4.12-1
> Severity: grave
> 
> I just upgraded schroot to the current version in testing, and now I
> can't use it anymore because someone got the bright (not) idea that only
> alphanumeric, dashes and underscores should be allowed in chroot names.
> It was annoying enough when a while ago the config files started having
> that (undocumented unless you read the source code and without useful
> error message) restriction.
> 
> Put it back to sanity please.
> 
> If I want to name a chroot rr2.1.0 because that's what is in it, then
> that's my choice.
> 
> While you are at it, let me choose the names of my config files too.
> 
> Breaking existing users is NOT acceptable so stop doing that.

While this change is clearly an annoyance to yourself, the severity
is certainly not grave, so I am changing it accordingly.  If the
change was causing irretrievable data loss, then the grave severity
would have been appropriate.

Your comments are also rather aggressive.  Please keep to the facts,
rather than using unnecessarily inflammatory language.  It's not
helpful, and is most certainly *not* appreciated.  Bug reports should
have the goal of improving the software, not abusing its developers.
Please bear that in mind, both in any replies to this email, and in
any future bug reports.


Regarding anything undocumented: if something is undocumented that
you need to know about, that's a bug, and if there's an error
message that's not clear, that's also a bug.  Please do file
(separate) bug reports about any such incidence and I'll be happy to
improve the documentation.  Remember that programs such as schroot
are documented by their authors and we already know every aspect
of the program inside and out.  Quality documentation is of course a
goal we have, but it's an unfortunate reality that it's easy to
inadvertently miss out details, especially when you are writing for
someone who isn't intimately familiar with the program. Making
documentation that is easily accessible to newcomers that contains
exactly what they need to know is a rare skill, and while I have done
a lot of work on the manual pages, I am fully aware that the
documentation could be much better.  But what I really need to do that
is *feedback* about what wasn't there that was needed, and what was
there that was unnecessary, etc..


Now, to the main point of your report.  schroot imposes certain
restrictions upon what it considers to be a permitted filename
when reading files under /etc/schroot/chroot.d and also what
is permitted to be a valid chroot and/or session name.

Since the chroot name is used to create session files bearing
the same name, the rules used to validate filenames therefore
may also have a concomitant restriction upon session names.
Before adding stricter validation of chroot names, it was possible
to create names which could lead to sessions which were not
possible to end, or which might potentially lead to an exploit
(naming your chroot "../../../../etc/xxx/xxx could have some
interesting consequences, as a simple example), so the primary
motivation behind this change was to improve security.  Additional
restrictions are also placed by other components we depend upon
(for example, if LVM logical volumes and Btrfs subvolumes and
snapshots have their own naming restrictions).  The imposed
restrictions do actually prevent breakage and/or security issues in
a number of scenarios.

The second motivation behind the change was to provide a single
validation mechanism for all names, again with the goal of improving
security and reliability.  You can find this function in
sbuild/sbuild-util.cc, called is_valid_filename().  Having made this
change, we have now added a second function called
is_valid_sessionname() which is used in places which only need to
validate names, which has relaxed requirements compared with the
filename checks.

  static regex file_namespace("^[a-zA-Z0-9][a-zA-Z0-9_-]*$");

Now, the intention has always been to further relax this requirement
and allow a wider variety of names.  I am well aware that this is
in some respects a "regression".  However, you must understand that
schroot is a suid root executable and the *primary* concern is and
always has been security.  If you wish to "fix" the naming
restrictions, all you need to do is alter that regular expression.
I will make that change once I have *fully evaluated the security
implications of doing so*, and not before.

It may be that "^[^/]$" is sufficient, but we need to be sure of
that.  The chroot/session name is used in quite a lot of places
and for many different purposes, and each of those needs careful
consideration.


So to summarise:

• this change was entirely intentional
• it was made to prevent session breakage and security exploits
• we have put the infrastructure in place to separately validate
  filenames and sesssion names
• we will allow more permissive chroot naming once the security
  implications have been throughly evaluated, and the code
  audited
• you are free to modify the source to make this change yourself,
  at entirely your own risk


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/20100924/6034c0e8/attachment.pgp>