[MERGE] ChrootTransportDecorator

[Andrew Bennetts]

John Arbash Meinel wrote:
> Andrew Bennetts wrote:
> > This bundle adds a ChrootTransportDecorator.  I use this in the latest revision
[...]
> 
> Sorry about the delay. This patch does what it claims to do. Adding a
> 'chroot+' decorator on urls, and preventing you from accessing files
> outside of that domain.
> 
> I feel like it is a bit of a band-aid, and not really the way things
> should work. Because you suddenly have to override every command of
> Transport, and really Transport was never intended to allow paths
> outside of 'base', other than at 'clone()' time.
> 
> So what I would prefer from an internals POV, is to have Transport
> classes audited or refactored, so that whatever function they use for
> generating the 'real' path from a relative path does not support '..',
> and then only 'clone()' functions are allowed to support '..'.

I don't entirely agree, although I haven't totally made up my mind.

I agree that in a purely technical sense, this could be done in Transport
itself, and that the existing structure wouldn't be very hard to massage into
shape to do that.  However, I'm not convinced that's a good idea, because this
is so security critical.  A seperate decorator is much easier to audit than the
large Transport base class, let alone worrying about all the implementations.

Also, the decorator will fail securely if a new method or parameter is added to
the Transport interface without consideration of the chroot security: it will
cause an error but not open a hole, and when a developer goes to update
chroot.py for the new interface, it will be obvious to both them and their
reviewer that the new code must take chrooting into account.  If we rely on
making Transport responsible for chrooting there's a much greater risk that a
hole will be unwittingly introduced.

[Hmm, actually, ChrootTransportDecorator, by inheriting from TransportDecorator,
does have a significant risk of having new, insecure methods added to it by
accident atm -- I should change this.]

I think it's quite attractive to not add this requirement to Transport; it's
already quite large and complex, and requiring a new subtlety of behaviour makes
it that little bit harder to maintain, and is one more thing a developer can get
wrong when changing it.  The decorator takes this responsibility, and takes care
of it seperately, keeping both things simpler.

If we do decide we want to put this wholly in Transport, we need to make the
choke point for enforcement as narrow as possible, so that it's as auditable as
possible, and so the risk of mistakes is minimised.

> This would do a few things:
> 
> 1) We would have to change tests that do transport.get('/foo') or
> .has('/') . But those shouldn't be using absolute paths anyway, since
> they aren't 'relpath', aka url fragments.

This is probably a good thing to do anyway.

> 2) ChrootDecorator then only has to trap 'def clone()', rather than
> trapping every function and adding all of the associated overhead.

This is at least a narrow chokepoint.

> 3) We should add transport implementation tests that assert for all
> functions that they don't allow an absolute path or a relative path with
> '..' in it. There is no real need to support path/../foo, so I would
> rather not try to support it.

path/../foo is fairly straightforward URL path algebra; we could in principle
always normalise paths in URLs before doing anything with them.  The chroot
decorator does this (by calling abspath).  (If there's a chink in
ChrootTransportDecorator's armour, this is it: it relies on the abspath
implementation of the underlying transport.  I am definitely looking forward to )

Sometimes I wish we used URL and URLRelpath objects, rather than strings... I
think I have a strong typing fascist in me trying to get out.  Or perhaps a
Hungarian ;)

> 4) If you want, we could actually deprecate clone('..'), in favor of
> using Transport.parent_dir_transport(), and then
> Transport.child_dir_transport(relpath).
> 
> I was never settled on clone(), it was just the closest I could get. The
> only time we need clone('..') is when we are looking for a containing
> object, and we should only be using it as clone('..'), never
> clone('../path/../other/path/../')  or anything like that.
> 
> 
> So if this is a strict requirement to getting the smart server fully
> operational over http, we can merge it. I'd rather we clean up the
> Transport implementations rather than wrapping another band-aid layer on
> top of them, though.

I'd rather we merge the decorator.  If we decide that it's the wrong approach,
then it's a trivial thing to deprecate -- it just becomes an utterly simple
no-op decorator that happens to emit a DeprecationWarning.  In the meantime,
it's already written, tested, and working -- and it's easy to see that it does
the right thing, whereas it's harder to see what the right thing would be if we
change Transport.  We have the decorator now, and the cost of switching later
rather than now is virtually the same, so in my mind it's an easy choice. :)

I'm definitely against merging the WSGI application without solving this --
releasing insecure software is not going to us any favours.

Whatever we do, so long as there's a clear chokepoint that we can rigorously
test with all possible edge cases, so we can be confident that we're water-tight
and will stay that way, I'll happy.

By the way, even though we haven't previously had functionality that exposed
bzrlib's transport layer to possibly hostile remote users, it's pretty neat that
we don't have to throughly audit the whole of bzrlib to be confident we can do
it safely.  Our existing design already makes this problem quite manageable.  I
can easily imagine alternative universes where writing a secure server for
previously non-server VCS would be much, much harder that it is for us.  I think
people deserve a pat on the back!

-Andrew.