Re: [Qemu-devel] Re: [V8 PATCH 11/11] virtio-9p: Chroot environment for other functions

[Venkateswararao Jujjuri (JV)]

On 3/10/2011 10:30 PM, Stefan Hajnoczi wrote:
> On Fri, Mar 11, 2011 at 5:54 AM, Venkateswararao Jujjuri (JV)
> <address@hidden> wrote:
>> On 3/10/2011 4:29 AM, Stefan Hajnoczi wrote:
>>> On Wed, Mar 9, 2011 at 5:16 PM, M. Mohan Kumar <address@hidden> wrote:
>>>> Add chroot functionality for systemcalls that can operate on a file
>>>> using relative directory file descriptor.
>>>
>>> I suspect the relative directory approach is broken and escapes the
>>> chroot.  Here's why:
>>>
>>> The request is local_chmod(fs_ctx, "/..", credp).  dirname("/..") is
>>> "/" and basename("..") is "..".
>>
>> We should never receive protocol operations with relative path.
>> Client should always resolve to full path and send the request.
>> If the client is malicious this scenario can be be possible.. but in that 
>> case
>> it is fine to fail the operation.
> 
> What I haven't audited yet is whether symlinks can be abused in any of
> these *at(2) operations.

Reading symlink sends only contents to the client. So a symlink can contain
anything.
But when the fully populated path comes we avoid the potential symlink issue by
opening
the entire dir in chrooted process.
> 
> The *at(2) approach seems like a shortcut to avoid implementing
> individual chroot protocol requests/responses for stat(2) and friends.
>  But it carries the risk that if we don't use NOFOLLOW then we can be
> tricked into escaping the "chroot" because we're performing the
> operation outside the chroot.

I would agree with your observation. With *at(2) we need the following:

1. The path should never have ".." May be we can check it early enough and fail.
2. Get the pfd from the chroot_thread
3. use NO_FOLLOW.

If we do all three then we are fool prof.
> 
> I'll take a look later today to make sure all operations safe traverse
> paths outside the chroot.

If we move all the operations to chroot, we are essentially serializing all
operations.
But the code looks lot cleaner though.

- JV


> 
> Stefan
>