minijail: Untangle redundant SECUREBITS logic
The existing code first decides whether to set SECBIT_KEEP_CAPS
individually via PR_SET_KEEPCAPS, then updates it again via
PR_SET_SECUREBITS. This change untangles that logic into a single
function.
Bug: None
TEST=Builds and passes tests.
Change-Id: I78bb0d78ade8deabffdaddf71f01edce67b222bb
diff --git a/libminijail.c b/libminijail.c
index af2ede2..1dfcc27 100644
--- a/libminijail.c
+++ b/libminijail.c
@@ -2157,23 +2157,19 @@
}
/*
- * POSIX capabilities are a bit tricky. If we drop our capability to
- * change uids, our attempt to use drop_ugid() below will fail. Hang on
- * to root caps across drop_ugid(), then lock securebits.
+ * POSIX capabilities are a bit tricky. We must set SECBIT_KEEP_CAPS
+ * before drop_ugid() below as the latter would otherwise drop all
+ * capabilities.
*/
if (j->flags.use_caps) {
/*
- * Using ambient capabilities takes care of most of the cases
- * where PR_SET_KEEPCAPS would be needed, but still try to set
- * them unless it is locked (maybe due to running minijail
- * within an already-minijailed process).
+ * When using ambient capabilities, CAP_SET{GID,UID} can be
+ * inherited across execve(2), so SECBIT_KEEP_CAPS is not
+ * strictly needed.
*/
- if (!j->flags.set_ambient_caps || !secure_keep_caps_locked()) {
- if (prctl(PR_SET_KEEPCAPS, 1))
- pdie("prctl(PR_SET_KEEPCAPS) failed");
- }
-
- if (lock_securebits(j->securebits_skip_mask) < 0) {
+ bool require_keep_caps = !j->flags.set_ambient_caps;
+ if (lock_securebits(j->securebits_skip_mask,
+ require_keep_caps) < 0) {
pdie("locking securebits failed");
}
}
diff --git a/system.c b/system.c
index 7527653..2dcf152 100644
--- a/system.c
+++ b/system.c
@@ -48,31 +48,49 @@
_Static_assert(SECURE_ALL_BITS == 0x55, "SECURE_ALL_BITS == 0x55.");
#endif
-int secure_keep_caps_locked(void)
-{
- int bits = prctl(PR_GET_SECUREBITS);
- if (bits < 0)
- return 0;
- return bits & SECBIT_KEEP_CAPS_LOCKED;
-}
-
int secure_noroot_set_and_locked(uint64_t mask)
{
return (mask & (SECBIT_NOROOT | SECBIT_NOROOT_LOCKED)) ==
(SECBIT_NOROOT | SECBIT_NOROOT_LOCKED);
}
-int lock_securebits(uint64_t skip_mask)
+int lock_securebits(uint64_t skip_mask, bool require_keep_caps)
{
+ /* The general idea is to set all bits, subject to exceptions below. */
+ unsigned long securebits = SECURE_ALL_BITS | SECURE_ALL_LOCKS;
+
+ /*
+ * SECBIT_KEEP_CAPS is special in that it is automatically cleared on
+ * execve(2). This implies that attempts to set SECBIT_KEEP_CAPS (as is
+ * the default) in processes that have it locked already (such as nested
+ * minijail usage) would fail. Thus, unless the caller requires it,
+ * allow it to remain off if it is already locked.
+ */
+ if (!require_keep_caps) {
+ int current_securebits = prctl(PR_GET_SECUREBITS);
+ if (current_securebits < 0) {
+ pwarn("prctl(PR_GET_SECUREBITS) failed");
+ return -1;
+ }
+
+ if ((current_securebits & SECBIT_KEEP_CAPS_LOCKED) != 0 &&
+ (current_securebits & SECBIT_KEEP_CAPS) == 0) {
+ securebits &= ~SECBIT_KEEP_CAPS;
+ }
+ }
+
/*
* Ambient capabilities can only be raised if they're already present
* in the permitted *and* inheritable set. Therefore, we don't really
* need to lock the NO_CAP_AMBIENT_RAISE securebit, since we are already
* configuring the permitted and inheritable set.
*/
- unsigned long securebits =
- (SECBIT_NO_CAP_AMBIENT_RAISE | SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED) &
- ~skip_mask;
+ securebits &=
+ ~(SECBIT_NO_CAP_AMBIENT_RAISE | SECBIT_NO_CAP_AMBIENT_RAISE_LOCKED);
+
+ /* Don't set any bits that the user requested not to be touched. */
+ securebits &= ~skip_mask;
+
if (!securebits) {
warn("not locking any securebits");
return 0;
diff --git a/system.h b/system.h
index 5007981..cd1a98a 100644
--- a/system.h
+++ b/system.h
@@ -38,9 +38,8 @@
#define PR_CAP_AMBIENT_CLEAR_ALL 4
#endif
-int secure_keep_caps_locked(void);
int secure_noroot_set_and_locked(uint64_t mask);
-int lock_securebits(uint64_t skip_mask);
+int lock_securebits(uint64_t skip_mask, bool require_keep_caps);
unsigned int get_last_valid_cap(void);
int cap_ambient_supported(void);
