diff mbox

[V8,11/11] virtio-9p: Chroot environment for other functions

Message ID 1299690960-12007-12-git-send-email-mohan@in.ibm.com
State New
Headers show

Commit Message

Mohan Kumar M March 9, 2011, 5:16 p.m. UTC
Add chroot functionality for systemcalls that can operate on a file
using relative directory file descriptor.

Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
 hw/9pfs/virtio-9p-local.c |  156 ++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 139 insertions(+), 17 deletions(-)

Comments

Stefan Hajnoczi March 10, 2011, 12:29 p.m. UTC | #1
On Wed, Mar 9, 2011 at 5:16 PM, M. Mohan Kumar <mohan@in.ibm.com> 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 "..".

I'm not 100% sure of the semantics but I suspect that chmodat(dir_fd,
"..", ...) does not honor the chroot since your current task is not
inside the chroot.  If so, then you can manipulate the parent
directory of the chroot using some of the operations added in this
patch.

The safe solution is to perform all operations inside the chroot.
This will require extending the chroot socket protocol.

Stefan
jvrao March 11, 2011, 5:54 a.m. UTC | #2
On 3/10/2011 4:29 AM, Stefan Hajnoczi wrote:
> On Wed, Mar 9, 2011 at 5:16 PM, M. Mohan Kumar <mohan@in.ibm.com> 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.

Thanks,
JV

> I'm not 100% sure of the semantics but I suspect that chmodat(dir_fd,
> "..", ...) does not honor the chroot since your current task is not
> inside the chroot.  If so, then you can manipulate the parent
> directory of the chroot using some of the operations added in this
> patch.
> 
> The safe solution is to perform all operations inside the chroot.
> This will require extending the chroot socket protocol.
> 
> Stefan
>
Stefan Hajnoczi March 11, 2011, 6:30 a.m. UTC | #3
On Fri, Mar 11, 2011 at 5:54 AM, Venkateswararao Jujjuri (JV)
<jvrao@linux.vnet.ibm.com> wrote:
> On 3/10/2011 4:29 AM, Stefan Hajnoczi wrote:
>> On Wed, Mar 9, 2011 at 5:16 PM, M. Mohan Kumar <mohan@in.ibm.com> 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.

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'll take a look later today to make sure all operations safe traverse
paths outside the chroot.

Stefan
jvrao March 11, 2011, 3:23 p.m. UTC | #4
On 3/10/2011 10:30 PM, Stefan Hajnoczi wrote:
> On Fri, Mar 11, 2011 at 5:54 AM, Venkateswararao Jujjuri (JV)
> <jvrao@linux.vnet.ibm.com> wrote:
>> On 3/10/2011 4:29 AM, Stefan Hajnoczi wrote:
>>> On Wed, Mar 9, 2011 at 5:16 PM, M. Mohan Kumar <mohan@in.ibm.com> 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
>
diff mbox

Patch

diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c
index 1670e33..0157eb8 100644
--- a/hw/9pfs/virtio-9p-local.c
+++ b/hw/9pfs/virtio-9p-local.c
@@ -22,6 +22,7 @@ 
 #include <sys/socket.h>
 #include <sys/un.h>
 #include <attr/xattr.h>
+#include <libgen.h>
 
 /* Helper routine to fill V9fsFileObjectRequest structure */
 static int fill_fileobjectrequest(V9fsFileObjectRequest *request,
@@ -70,14 +71,41 @@  static int passthrough_request(FsContext *fs_ctx, const char *old_path,
     return retval;
 }
 
+/*
+ * Returns file descriptor of dirname(path)
+ * This fd can be used by *at functions
+ */
+static int get_dirfd(FsContext *fs_ctx, const char *path)
+{
+    V9fsFileObjectRequest request;
+    int fd;
+    char *dpath = qemu_strdup(path);
+
+    fd = fill_fileobjectrequest(&request, dirname(dpath), NULL);
+    if (fd < 0) {
+        qemu_free(dpath);
+        errno = -fd;
+        return -1;
+    }
+    request.data.type = T_OPEN;
+    fd = v9fs_request(fs_ctx, &request);
+    qemu_free(dpath);
+    if (fd < 0) {
+        errno = -fd;
+        fd = -1;
+    }
+    return fd;
+}
+
 static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
 {
     int err;
-    err =  lstat(rpath(fs_ctx, path), stbuf);
-    if (err) {
-        return err;
-    }
+
     if (fs_ctx->fs_sm == SM_MAPPED) {
+        err =  lstat(rpath(fs_ctx, path), stbuf);
+        if (err) {
+            return err;
+        }
         /* Actual credentials are part of extended attrs */
         uid_t tmp_uid;
         gid_t tmp_gid;
@@ -99,6 +127,27 @@  static int local_lstat(FsContext *fs_ctx, const char *path, struct stat *stbuf)
                         sizeof(dev_t)) > 0) {
                 stbuf->st_rdev = tmp_dev;
         }
+    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        int pfd, serrno = 0;
+        char *tmp_path;
+
+        pfd = get_dirfd(fs_ctx, path);
+        if (pfd < 0) {
+            return -1;
+        }
+        tmp_path = qemu_strdup(path);
+        err = fstatat(pfd, basename(tmp_path), stbuf, AT_SYMLINK_NOFOLLOW);
+        if (err < 0) {
+            serrno = errno;
+        }
+        close(pfd);
+        qemu_free(tmp_path);
+        errno = serrno;
+    } else {
+        err =  lstat(rpath(fs_ctx, path), stbuf);
+        if (err) {
+            return err;
+        }
     }
     return err;
 }
@@ -163,9 +212,23 @@  static ssize_t local_readlink(FsContext *fs_ctx, const char *path,
         } while (tsize == -1 && errno == EINTR);
         close(fd);
         return tsize;
-    } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) ||
-               (fs_ctx->fs_sm == SM_NONE)) {
+    } else if (fs_ctx->fs_sm == SM_NONE) {
         tsize = readlink(rpath(fs_ctx, path), buf, bufsz);
+    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        int pfd, serrno = 0;
+        char *tmp_path;
+        pfd = get_dirfd(fs_ctx, path);
+        if (pfd < 0) {
+            return -1;
+        }
+        tmp_path = qemu_strdup(path);
+        tsize = readlinkat(pfd, basename(tmp_path), buf, bufsz);
+        if (tsize < 0) {
+            serrno = errno;
+        }
+        close(pfd);
+        qemu_free(tmp_path);
+        errno = serrno;
     }
     return tsize;
 }
@@ -257,8 +320,23 @@  static int local_chmod(FsContext *fs_ctx, const char *path, FsCred *credp)
 {
     if (fs_ctx->fs_sm == SM_MAPPED) {
         return local_set_xattr(rpath(fs_ctx, path), credp);
-    } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) ||
-               (fs_ctx->fs_sm == SM_NONE)) {
+    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        int pfd, err, serrno = 0;
+        char *tmp_path;
+        pfd = get_dirfd(fs_ctx, path);
+        if (pfd < 0) {
+            return -1;
+        }
+        tmp_path = qemu_strdup(path);
+        err = fchmodat(pfd, basename(tmp_path), credp->fc_mode, 0);
+        if (err == -1) {
+            serrno = errno;
+        }
+        qemu_free(tmp_path);
+        close(pfd);
+        errno = serrno;
+        return err;
+    } else if (fs_ctx->fs_sm == SM_NONE) {
         return chmod(rpath(fs_ctx, path), credp->fc_mode);
     }
     return -1;
@@ -506,7 +584,21 @@  static int local_link(FsContext *fs_ctx, const char *oldpath,
 
 static int local_truncate(FsContext *ctx, const char *path, off_t size)
 {
-    return truncate(rpath(ctx, path), size);
+    if (ctx->fs_sm == SM_PASSTHROUGH) {
+        int fd, retval, serrno;
+        fd = passthrough_request(ctx, NULL, path, O_RDWR, NULL, T_OPEN);
+        if (fd < 0) {
+            errno = -fd;
+            return -1;
+        }
+        retval = ftruncate(fd, size);
+        serrno = errno;
+        close(fd);
+        errno = serrno;
+        return retval;
+    } else {
+        return truncate(rpath(ctx, path), size);
+    }
 }
 
 static int local_rename(FsContext *ctx, const char *oldpath,
@@ -536,22 +628,52 @@  static int local_rename(FsContext *ctx, const char *oldpath,
 
 static int local_chown(FsContext *fs_ctx, const char *path, FsCred *credp)
 {
-    if ((credp->fc_uid == -1 && credp->fc_gid == -1) ||
-            (fs_ctx->fs_sm == SM_PASSTHROUGH)) {
+    if (fs_ctx->fs_sm != SM_PASSTHROUGH &&
+            (credp->fc_uid == -1 && credp->fc_gid == -1)) {
+        return lchown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid);
+    } else if (fs_ctx->fs_sm == SM_NONE) {
         return lchown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid);
     } else if (fs_ctx->fs_sm == SM_MAPPED) {
         return local_set_xattr(rpath(fs_ctx, path), credp);
-    } else if ((fs_ctx->fs_sm == SM_PASSTHROUGH) ||
-               (fs_ctx->fs_sm == SM_NONE)) {
-        return lchown(rpath(fs_ctx, path), credp->fc_uid, credp->fc_gid);
+    } else if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        int pfd, err, serrno = 0;
+        char *old_path;
+        pfd = get_dirfd(fs_ctx, path);
+        if (pfd < 0) {
+            return -1;
+        }
+        old_path = qemu_strdup(path);
+        err = fchownat(pfd, basename(old_path), credp->fc_uid, credp->fc_gid,
+                        AT_SYMLINK_NOFOLLOW);
+        if (err == -1) {
+            serrno = errno;
+        }
+        qemu_free(old_path);
+        close(pfd);
+        errno = serrno;
+        return err;
     }
     return -1;
 }
 
-static int local_utimensat(FsContext *s, const char *path,
-                           const struct timespec *buf)
+static int local_utimensat(FsContext *fs_ctx, const char *path,
+                const struct timespec *buf)
 {
-    return qemu_utimensat(AT_FDCWD, rpath(s, path), buf, AT_SYMLINK_NOFOLLOW);
+    if (fs_ctx->fs_sm == SM_PASSTHROUGH) {
+        int fd, retval;
+        fd = passthrough_request(fs_ctx, NULL, path,
+                        O_RDONLY | O_NONBLOCK | O_NOFOLLOW, NULL, T_OPEN);
+        if (fd < 0) {
+            errno = -fd;
+            return -1;
+        }
+        retval = futimens(fd, buf);
+        close(fd);
+        return retval;
+    } else {
+        return utimensat(AT_FDCWD, rpath(fs_ctx, path), buf,
+                        AT_SYMLINK_NOFOLLOW);
+    }
 }
 
 static int local_remove(FsContext *ctx, const char *path)