Re: [Virtio-fs] virtiofsd: doesn't garant write access at users allowed by group permission

* Re: [Virtio-fs] virtiofsd: doesn't garant write access at users allowed by group permission
@ 2021-06-18 23:28 Ameer Ghani
  2021-06-22 13:45 ` Vivek Goyal
  0 siblings, 1 reply; 9+ messages in thread
From: Ameer Ghani @ 2021-06-18 23:28 UTC (permalink / raw)
  To: virtio-fs; +Cc: vgoyal

[-- Attachment #1: Type: text/plain, Size: 5550 bytes --]

Hello,

We were experiencing the same issue w/r/t group permissions not being respected
on directories.

I did a quick-and-dirty hack to add the SECBIT_NO_SETUID_FIXUP securebit, as
suggested by Chirantan and Vivek, and that seems to have done the trick. Looks
like if we set the securebits before we setup sandbox we can avoid leaving the
process with CAP_SETPCAP.

Seems a better approach would just to expose the ability to set securebits at
the CLI like what is done with caps. Then anybody affected by this bug can set
SECBIT_NO_SETUID_FIXUP themselves. I'd prefer this if the maintainers agree.

Would be happy to clean up my patch and submit it--just want to check if this
is an approach y'all are content with.

Current patch is included if anybody wants to try it out. Note that the
securebits header is lifted from kernel 5.4.0 /usr/include/linux/securebits.h

Thanks,
Ameer Ghani

---
include/standard-headers/linux/securebits.h | 61 +++++++++++++++++++++
tools/virtiofsd/passthrough_ll.c            | 15 +++++
2 files changed, 76 insertions(+)
create mode 100644 include/standard-headers/linux/securebits.h

diff --git a/include/standard-headers/linux/securebits.h b/include/standard-headers/linux/securebits.h
new file mode 100644
index 0000000000..69c11a49c5
--- /dev/null
+++ b/include/standard-headers/linux/securebits.h
@@ -0,0 +1,61 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _LINUX_SECUREBITS_H
+#define _LINUX_SECUREBITS_H
+
+/* Each securesetting is implemented using two bits. One bit specifies
+   whether the setting is on or off. The other bit specify whether the
+   setting is locked or not. A setting which is locked cannot be
+   changed from user-level. */
+#define issecure_mask(X)  (1 << (X))
+
+#define SECUREBITS_DEFAULT 0x00000000
+
+/* When set UID 0 has no special privileges. When unset, we support
+   inheritance of root-permissions and suid-root executable under
+   compatibility mode. We raise the effective and inheritable bitmasks
+   *of the executable file* if the effective uid of the new process is
+   0. If the real uid is 0, we raise the effective (legacy) bit of the
+   executable file. */
+#define SECURE_NOROOT                0
+#define SECURE_NOROOT_LOCKED         1  /* make bit-0 immutable */
+
+#define SECBIT_NOROOT          (issecure_mask(SECURE_NOROOT))
+#define SECBIT_NOROOT_LOCKED    (issecure_mask(SECURE_NOROOT_LOCKED))
+
+/* When set, setuid to/from uid 0 does not trigger capability-"fixup".
+   When unset, to provide compatiblility with old programs relying on
+   set*uid to gain/lose privilege, transitions to/from uid 0 cause
+   capabilities to be gained/lost. */
+#define SECURE_NO_SETUID_FIXUP       2
+#define SECURE_NO_SETUID_FIXUP_LOCKED 3  /* make bit-2 immutable */
+
+#define SECBIT_NO_SETUID_FIXUP  (issecure_mask(SECURE_NO_SETUID_FIXUP))
+#define SECBIT_NO_SETUID_FIXUP_LOCKED \
+               (issecure_mask(SECURE_NO_SETUID_FIXUP_LOCKED))
+
+/* When set, a process can retain its capabilities even after
+   transitioning to a non-root user (the set-uid fixup suppressed by
+   bit 2). Bit-4 is cleared when a process calls exec(); setting both
+   bit 4 and 5 will create a barrier through exec that no exec()'d
+   child can use this feature again. */
+#define SECURE_KEEP_CAPS        4
+#define SECURE_KEEP_CAPS_LOCKED      5  /* make bit-4 immutable */
+
+#define SECBIT_KEEP_CAPS  (issecure_mask(SECURE_KEEP_CAPS))
+#define SECBIT_KEEP_CAPS_LOCKED (issecure_mask(SECURE_KEEP_CAPS_LOCKED))
+
+/* When set, a process cannot add new capabilities to its ambient set. */
+#define SECURE_NO_CAP_AMBIENT_RAISE       6
+#define SECURE_NO_CAP_AMBIENT_RAISE_LOCKED 7  /* make bit-6 immutable */
+
+#define SECBIT_NO_CAP_AMBIENT_RAISE (issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE))
+#define SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED \
+               (issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE_LOCKED))
+
+#define SECURE_ALL_BITS         (issecure_mask(SECURE_NOROOT) | \
+                    issecure_mask(SECURE_NO_SETUID_FIXUP) | \
+                    issecure_mask(SECURE_KEEP_CAPS) | \
+                    issecure_mask(SECURE_NO_CAP_AMBIENT_RAISE))
+#define SECURE_ALL_LOCKS  (SECURE_ALL_BITS << 1)
+
+#endif /* _LINUX_SECUREBITS_H */
diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 49c21fd855..afec9a6567 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -43,6 +43,7 @@
#include "fuse_log.h"
#include "fuse_lowlevel.h"
#include "standard-headers/linux/fuse.h"
+#include "standard-headers/linux/securebits.h"
#include <cap-ng.h>
#include <dirent.h>
#include <pthread.h>
@@ -3515,6 +3516,17 @@ static void setup_chroot(struct lo_data *lo)
     }
}
+static void setup_securebits(void)
+{
+    int status;
+
+    status = prctl(PR_SET_SECUREBITS, SECBIT_NO_SETUID_FIXUP);
+    if (status == -1) {
+        fuse_log(FUSE_LOG_ERR, "prctl(PR_SET_SECUREBITS): %m\n");
+        exit(1);
+    }
+}
+
/*
  * Lock down this process to prevent access to other processes or files outside
  * source directory.  This reduces the impact of arbitrary code execution bugs.
@@ -3869,6 +3881,9 @@ int main(int argc, char *argv[])
     setup_nofile_rlimit(opts.rlimit_nofile);
+    /* Set securebits before we drop CAP_SETPCAP */
+    setup_securebits();
+
     /* Must be before sandbox since it wants /proc */
     setup_capng();
--
2.25.1


[-- Attachment #2: Type: text/html, Size: 21041 bytes --]

^ permalink raw reply related	[flat|nested] 9+ messages in thread