jail: fix capabilities
authorDaniel Golle <daniel@makrotopia.org>
Fri, 6 Nov 2020 18:42:25 +0000 (18:42 +0000)
committerDaniel Golle <daniel@makrotopia.org>
Sat, 7 Nov 2020 04:37:03 +0000 (04:37 +0000)
Allocate enough stack space for capget()/capset() which requires
2*sizeof(struct __user_cap_data_struct), each containing 32-bit fields,
where the 2nd struct contains the bits for high (>32) capabilities.
Failing to do that not only leads to those high capabilities being
inaccessible but also overwrote the stack resulting in ujail hanging
infinitely instead of returning from applyOCIcapabilities().
Also adapt debugging output to 64-bit format.
Apart from that, don't set SECBIT_NO_SETUID_FIXUP when not actually
modifying capabilities explicitely, as that would result in ALL
capabilities retained in the subsequent setuid() call instead of
having them all dropped.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
jail/capabilities.c
jail/jail.c

index b3627461b642bd27175f7ffd19c0dba7aae02603..434fc2f77e266bfc2b5e945a26a176b68c797039 100644 (file)
@@ -120,7 +120,7 @@ int parseOCIcapabilities(struct jail_capset *capset, struct blob_attr *msg)
 int applyOCIcapabilities(struct jail_capset ocicapset, uint64_t retain)
 {
        struct __user_cap_header_struct uh = {};
-       struct __user_cap_data_struct ud;
+       struct __user_cap_data_struct ud[2];
        int cap;
        int is_set;
 
@@ -153,25 +153,37 @@ int applyOCIcapabilities(struct jail_capset ocicapset, uint64_t retain)
        uh.version = _LINUX_CAPABILITY_VERSION_3;
        uh.pid = getpid();
 
-       if (capget(&uh, &ud)) {
+       if (capget(&uh, ud)) {
                ERROR("capget() failed\n");
                return -1;
        }
 
-       DEBUG("old capabilities: Pe=%08x Pp=%08x Pi=%08x\n", ud.effective, ud.permitted, ud.inheritable);
+       DEBUG("old capabilities: Pe=%016llx Pp=%016llx Pi=%016llx\n",
+               0LLU | ud[0].effective | (0LLU | ud[1].effective) << 32,
+               0LLU | ud[0].permitted | (0LLU | ud[1].permitted) << 32,
+               0LLU | ud[0].inheritable | (0LLU | ud[1].inheritable) << 32);
 
-       if (ocicapset.effective != JAIL_CAP_ALL)
-               ud.effective = ocicapset.effective | retain;
+       if (ocicapset.effective != JAIL_CAP_ALL) {
+               ud[0].effective = (ocicapset.effective | retain) & 0xFFFFFFFFU;
+               ud[1].effective = ((ocicapset.effective | retain) >> 32) & 0xFFFFFFFFU;
+       }
 
-       if (ocicapset.permitted != JAIL_CAP_ALL)
-               ud.permitted = ocicapset.permitted | retain;
+       if (ocicapset.permitted != JAIL_CAP_ALL) {
+               ud[0].permitted = (ocicapset.permitted | retain) & 0xFFFFFFFFU;
+               ud[1].permitted = ((ocicapset.permitted | retain) >> 32) & 0xFFFFFFFFU;
+       }
 
-       if (ocicapset.inheritable != JAIL_CAP_ALL)
-               ud.inheritable = ocicapset.inheritable;
+       if (ocicapset.inheritable != JAIL_CAP_ALL) {
+               ud[0].inheritable = (ocicapset.inheritable | retain) & 0xFFFFFFFFU;
+               ud[1].inheritable = ((ocicapset.inheritable | retain) >> 32) & 0xFFFFFFFFU;
+       }
 
-       DEBUG("new capabilities: Pe=%08x Pp=%08x Pi=%08x\n", ud.effective, ud.permitted, ud.inheritable);
+       DEBUG("new capabilities: Pe=%016llx Pp=%016llx Pi=%016llx\n",
+               0LLU | ud[0].effective | (0LLU | ud[1].effective) << 32,
+               0LLU | ud[0].permitted | (0LLU | ud[1].permitted) << 32,
+               0LLU | ud[0].inheritable | (0LLU | ud[1].inheritable) << 32);
 
-       if (capset(&uh, &ud)) {
+       if (capset(&uh, ud)) {
                ERROR("capset() failed\n");
                return -1;
        }
index 4b368ea74253df1ba940a7b60da767efc12ef155..f09e1a4d9915376d6102610d356480bb7a8e9b5e 100644 (file)
@@ -1178,9 +1178,15 @@ static void post_start_hook(void)
 {
        int pw_uid, pw_gid, gr_gid;
 
-       if (prctl(PR_SET_SECUREBITS, SECBIT_NO_SETUID_FIXUP)) {
-               ERROR("prctl(PR_SET_SECUREBITS) failed: %m\n");
-               exit(EXIT_FAILURE);
+       /*
+        * make sure setuid/setgid won't drop capabilities in case capabilities
+        * have been specified explicitely.
+        */
+       if (opts.capset.apply) {
+               if (prctl(PR_SET_SECUREBITS, SECBIT_NO_SETUID_FIXUP)) {
+                       ERROR("prctl(PR_SET_SECUREBITS) failed: %m\n");
+                       exit(EXIT_FAILURE);
+               }
        }
 
        /* drop capabilities, retain those still needed to further setup jail */
@@ -1200,9 +1206,12 @@ static void post_start_hook(void)
        if (opts.set_umask)
                umask(opts.umask);
 
-       if (prctl(PR_SET_SECUREBITS, 0)) {
-               ERROR("prctl(PR_SET_SECUREBITS) failed: %m\n");
-               exit(EXIT_FAILURE);
+       /* restore securebits back to normal */
+       if (opts.capset.apply) {
+               if (prctl(PR_SET_SECUREBITS, 0)) {
+                       ERROR("prctl(PR_SET_SECUREBITS) failed: %m\n");
+                       exit(EXIT_FAILURE);
+               }
        }
 
        /* drop remaining capabilities to end up with specified sets */