Libguestfs - Jun 2014 - [PATCH] fuse: UID 0 should override all permissions checks (RHBZ#1106548).

Previously if you were root, and you tried to change directory into a
directory which was not owned by you and not readable (eg. 0700
bin:bin), it would fail.

This doesn't fail on regular directories because when you are root the
kernel just ignores permissions.

Although libguestfs in general tries not to duplicate kernel code, in
the case where we emulate the FUSE access(2) system call,
unfortunately we have to do it by stat-ing the object and performing
some (half-arsed) heuristics.

This commit modifies the FUSE access(2) system call, so root is now
able to chdir to any directory.

It also adds some debugging so we can debug these complex permissions
checks in the field if some other problem arises in future.
---
 src/fuse.c | 51 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 36 insertions(+), 15 deletions(-)

diff --git a/src/fuse.c b/src/fuse.c
index dd63729..292ebee 100644
--- a/src/fuse.c
+++ b/src/fuse.c
@@ -301,21 +301,42 @@ mount_local_access (const char *path, int mask)
 
   fuse = fuse_get_context ();
 
-  if (mask & R_OK)
-    ok = ok &&
-      (  fuse->uid == statbuf.st_uid ? statbuf.st_mode & S_IRUSR
-       : fuse->gid == statbuf.st_gid ? statbuf.st_mode & S_IRGRP
-       : statbuf.st_mode & S_IROTH);
-  if (mask & W_OK)
-    ok = ok &&
-      (  fuse->uid == statbuf.st_uid ? statbuf.st_mode & S_IWUSR
-       : fuse->gid == statbuf.st_gid ? statbuf.st_mode & S_IWGRP
-       : statbuf.st_mode & S_IWOTH);
-  if (mask & X_OK)
-    ok = ok &&
-      (  fuse->uid == statbuf.st_uid ? statbuf.st_mode & S_IXUSR
-       : fuse->gid == statbuf.st_gid ? statbuf.st_mode & S_IXGRP
-       : statbuf.st_mode & S_IXOTH);
+  /* Root user should be able to access everything, so only bother
+   * with these fine-grained tests for non-root.  (RHBZ#1106548).
+   */
+  if (fuse->uid != 0) {
+    if (mask & R_OK)
+      ok = ok &&
+        (  fuse->uid == statbuf.st_uid ? statbuf.st_mode & S_IRUSR
+           : fuse->gid == statbuf.st_gid ? statbuf.st_mode & S_IRGRP
+           : statbuf.st_mode & S_IROTH);
+    if (mask & W_OK)
+      ok = ok &&
+        (  fuse->uid == statbuf.st_uid ? statbuf.st_mode & S_IWUSR
+           : fuse->gid == statbuf.st_gid ? statbuf.st_mode & S_IWGRP
+           : statbuf.st_mode & S_IWOTH);
+    if (mask & X_OK)
+      ok = ok &&
+        (  fuse->uid == statbuf.st_uid ? statbuf.st_mode & S_IXUSR
+           : fuse->gid == statbuf.st_gid ? statbuf.st_mode & S_IXGRP
+           : statbuf.st_mode & S_IXOTH);
+  }
+
+  debug (g, "%s: "
+         "testing access mask%s%s%s%s: "
+         "caller UID:GID = %d:%d, "
+         "file UID:GID = %d:%d, "
+         "file mode = %o, "
+         "result = %s",
+         path,
+         mask & R_OK ? " R_OK" : "",
+         mask & W_OK ? " W_OK" : "",
+         mask & X_OK ? " X_OK" : "",
+         mask == 0 ? " 0" : "",
+         fuse->uid, fuse->gid,
+         statbuf.st_uid, statbuf.st_gid,
+         statbuf.st_mode,
+         ok ? "OK" : "EACCESS");
 
   return ok ? 0 : -EACCES;
 }
-- 
1.9.0

On Thursday 12 June 2014 13:32:54 Richard W.M. Jones
wrote:
> Previously if you were root, and you tried to change directory into a
> directory which was not owned by you and not readable (eg. 0700
> bin:bin), it would fail.
> 
> This doesn't fail on regular directories because when you are root the
> kernel just ignores permissions.
> 
> Although libguestfs in general tries not to duplicate kernel code, in
> the case where we emulate the FUSE access(2) system call,
> unfortunately we have to do it by stat-ing the object and performing
> some (half-arsed) heuristics.
> 
> This commit modifies the FUSE access(2) system call, so root is now
> able to chdir to any directory.
I've taken a look at few non-trivial FUSE filesystems, and none of them 
seems to implement the access operation. I guess this means the kernel 
does all the job by itself based on the permissions.
On the other hand, removing the access operation makes test-fuse.sh fail 
in the chmod part, at:
  [ ! -x new ]
interestingly enough, the permissions of "new" at that point are fine 
(no -x), and strace'ing that test command gives
  access("new", X_OK)                     = 0
so I'm puzzled...

Interestingly enough, even trying the allow_root and allow_other FUSE 
options makes no difference.

So I'd say to commit this for now; just one note below.
> It also adds some debugging so we can debug these complex permissions
> checks in the field if some other problem arises in future.
> [...]
> +  debug (g, "%s: "
> +         "testing access mask%s%s%s%s: "
> +         "caller UID:GID = %d:%d, "
> +         "file UID:GID = %d:%d, "
> +         "file mode = %o, "
> +         "result = %s",
> +         path,
> +         mask & R_OK ? " R_OK" : "",
> +         mask & W_OK ? " W_OK" : "",
> +         mask & X_OK ? " X_OK" : "",
> +         mask == 0 ? " 0" : "",
> +         fuse->uid, fuse->gid,
> +         statbuf.st_uid, statbuf.st_gid,
> +         statbuf.st_mode,
> +         ok ? "OK" : "EACCESS");
Would it be possible to split most of this debug right after the 
mount_local_getattr invocation, so early returns have this debug as 
well?

Thanks,
-- 
Pino Toscano

On Fri, Jun 13, 2014 at 06:14:26PM +0200, Pino Toscano
wrote:
> Would it be possible to split most of this debug right after the 
> mount_local_getattr invocation, so early returns have this debug as 
> well?
I added some debug along that early return path, but I didn't see a
good way to share the common debug statement, so now there are two
debug stmts.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/