[MERGE] ChrootTransportDecorator

[John Arbash Meinel]

Andrew Bennetts wrote:
> 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.
> 

Actually, I don't think it is quite as secure as you think it is. It
*does* audit whether the path which is supplied is valid (relpath is
actually a relative path).

It *doesn't* test that the function itself combines that with self.base
in the right manner.

What I think is a much better audit, is to have a test that ensures that
all transport functions fail if requested to work on a path that is not
a sub-path. This is easy to write as part of
test_transport_implementations and means that all implementations will
be tested, rather than just one combination of 'chroot+' and a single
transport.

I don't even think it is terribly hard to change the code for all of this.

I'm not 100% sure how to break out the functionality. Mostly because
sftp actually has a need for a different remote path than most of the
other transports. It needs a real remote path, rather than just the
absolute url. FTP is probably the same way.

So I'm thinking that maybe we should have 3 functions in Transport, one
to give the normalized url, another to give the remote path, and a third
which allows creating a Transport pointing to the parent directory. And
we should deprecate clone() accepting '..', and in the short term, just
thunk it over to whatever parent function.

> [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 )

^- Looking forward too???

> 
> 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 ;)

The primary reason we didn't want to do this, is because we get paths
from a lot of places. And certainly write a lot of them in the test
suite. And having to wrap every call into a URLRelpath('.') makes the
API very unfriendly.

So stuff like
root_transport = transport
transport = transport.clone('.bzr')

would change to

transport = transport.clone(urlutils.URLRelpath('.bzr'))

And the test suite suddenly is filled with
make_branch_and_tree(URLRelpath('tree'))

It is possible that some of the helper functions don't require a URL
object, but then you start having this huge split between function that
take a URL and ones that take strings because it is easier for us humans.

There are probably also some performance implications of creating lots
of custom objects like that, but I don't really know what it would be,
so I'm not going to say it would be large. (But we might be creating
tens of thousands of them, so it isn't like the number is small).

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

At this point I'm okay with merging the decorator. I think the real fix
needs to be deeper, but even then we would need 'chroot+' because you'll
always have some version of clone() which allows you to move around the
remote file system.

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

Its interesting how a useful abstraction can help with stuff like that.
Because we have the abstraction as to whether things are local or
remote, we already needed to have a layer that everything goes through.
But it does indeed make it easier to add security at that layer.

Now, if as we start adding smarts to the smart server, I think we are
going to find that it starts stepping outside of the Transport (for
example, WorkingTree uses local operations, not Transport ones). So it
isn't like this is the only fix we will need to do. In general, stuff
inside .bzr/* is accessed through Transport, but stuff outside of that
is accessed directly.

So I think we'll neeed to revisit a lot of this security, in case
someone could create a fake revision which says it modifies
'../../../etc/password', and then ask to have that applied to the server
branch. Now, ATM our inventories are incapable of stating this, because
they only say what parent directory they are in, never giving the
absolute path or even relative path. But as we move forward, we may
change how we serialize inventories. And I'm really curious what would
happen in git or hg if you added '../../foo' to their manifest. Most
likely they would just complain about '..', but I'm still curious.

So you have my approval to merge. I would *like* to see a thorough audit
of Transport. And I'm even willing to contribute to it.

John
=:->


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20061110/69c0927c/attachment.pgp