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

Len Sorensen lennartsorensen at ruggedcom.com
Mon Oct 18 17:26:05 UTC 2010


On Fri, Sep 24, 2010 at 06:52:32PM -0400, Roger Leigh wrote:
> 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.

Perhaps I was a bit too aggresive.  I had a cold, it wasn't the first time
this year schroot had broken things that previously worked without even
a hint in the changelog that such breakage was being introduced.  In noen
of the cases does there seem to be any good reason for breaking things.
To me these breakages are so severe that a NEWS entry should have popped
up warning the user that this was happening.

> 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..

Changing the restrictions on the allowed chroot names should not be done
lightly, and it should certainly be in the changelogs and probably even
more visible than that.  Of course it really ought to not be done at all
because you break existing users for what appears to be no good reason.

> 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.

schroot NOW imposses such restrictions.  For years it has not done so
and was working fine.

> 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.

Sure, but . is valid in filenames.  A simple but dumb regex that forbids
the first character from being a . solves all filename issues as long
as you also don't permit /.  Simple, and vastly better than the current
setup and very unlikely to break any existing users.

> 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.

I don't think breaking things this close to a release with the intent
of fixing it someday later is acceptable.  Why should configs from Lenny
stop working when people move to Squeeze?  Are you going to put that in
the release notes?  Are you going to have debconf pop up a warning to
users that their configs are now potentially broken?

> 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

Obviously, but it took a lot of digging to find out about it once it
broke things.

> • it was made to prevent session breakage and security exploits

Well it broke things very well changing it.

> • 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

So when is that going to happen?  If it isn't before squeeze is finallized
then I think this change has to be reverted or at least vastly relaxed.
It breaks too much as it is.

> • you are free to modify the source to make this change yourself,
>   at entirely your own risk

Well I had too, given I can't use it anymore without changing it.
It has become useless to me as shipped upstream.

Certainly the filenames permitted by run-parts is way too restrictive
and not a good source of a regex to use.

I will happily write up a regex if I knew everything it needs to consider.

Also, it seems wrong for schroot to refuse to use ANY chroot if there
is any chroot in any config with an "illegal" name.  So you can't even
use the valid ones when any invalid ones are defined.

-- 
Len Sorensen





More information about the Buildd-tools-devel mailing list