capabilities: do not audit log BPRM_FCAPS on set*id

---
 security/commoncap.c |   63 +++++++++++++++++++++++++++----------------------
 1 files changed, 35 insertions(+), 28 deletions(-)

security/commoncap.c +35 ~~-28~~

index d8e26fb..927fe93 100644
@@ -472,6 +472,39 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
 	return rc;
 }
 
+static void handle_privileged_root(struct linux_binprm *bprm, bool has_cap,
+				   bool *effective, kuid_t root_uid)
+{
+	const struct cred *old = current_cred();
+	struct cred *new = bprm->cred;
+
+	if (issecure(SECURE_NOROOT))
+		return;
+	/*
+	 * If the legacy file capability is set, then don't set privs
+	 * for a setuid root binary run by a non-root user.  Do set it
+	 * for a root user just to cause least surprise to an admin.
+	 */
+	if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) {
+		warn_setuid_and_fcaps_mixed(bprm->filename);
+		return;
+	}
+	/*
+	 * To support inheritance of root-permissions and suid-root
+	 * executables under compatibility mode, we override the
+	 * capability sets for the file.
+	 *
+	 * If only the real uid is 0, we do not set the effective bit.
+	 */
+	if (uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid)) {
+		/* pP' = (cap_bset & ~0) | (pI & ~0) */
+		new->cap_permitted = cap_combine(old->cap_bset,
+						 old->cap_inheritable);
+	}
+	if (uid_eq(new->euid, root_uid))
+		*effective = true;
+}
+
 /**
  * cap_bprm_set_creds - Set up the proposed credentials for execve().
  * @bprm: The execution parameters, including the proposed creds

@@ -484,46 +517,20 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 {
 	const struct cred *old = current_cred();
 	struct cred *new = bprm->cred;
-	bool effective, has_cap = false, is_setid;
+	bool effective = false, has_cap = false, is_setid;
 	int ret;
 	kuid_t root_uid;
 
 	if (WARN_ON(!cap_ambient_invariant_ok(old)))
 		return -EPERM;
 
-	effective = false;
 	ret = get_file_caps(bprm, &effective, &has_cap);
 	if (ret < 0)
 		return ret;
 
 	root_uid = make_kuid(new->user_ns, 0);
 
-	if (!issecure(SECURE_NOROOT)) {
-		/*
-		 * If the legacy file capability is set, then don't set privs
-		 * for a setuid root binary run by a non-root user.  Do set it
-		 * for a root user just to cause least surprise to an admin.
-		 */
-		if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) {
-			warn_setuid_and_fcaps_mixed(bprm->filename);
-			goto skip;
-		}
-		/*
-		 * To support inheritance of root-permissions and suid-root
-		 * executables under compatibility mode, we override the
-		 * capability sets for the file.
-		 *
-		 * If only the real uid is 0, we do not set the effective bit.
-		 */
-		if (uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid)) {
-			/* pP' = (cap_bset & ~0) | (pI & ~0) */
-			new->cap_permitted = cap_combine(old->cap_bset,
-							 old->cap_inheritable);
-		}
-		if (uid_eq(new->euid, root_uid))
-			effective = true;
-	}
-skip:
+	handle_privileged_root(bprm, has_cap, &effective, root_uid);
 
 	/* if we have fs caps, clear dangerous personality flags */
 	if (!cap_issubset(new->cap_permitted, old->cap_permitted))

Signature · 1 line

1.7.1

James Morris

maintainer Acked-by on v4 1/10

2017-09-06 06:05 UTC #2 lore raw

Re: [PATCH V4 01/10] capabilities: factor out cap_bprm_set_creds privileged root

Quoted 9 lines · Richard Guy Briggs

Factor out the case of privileged root from the function cap_bprm_set_creds() to make the latter easier to read and analyse.
Suggested-by Serge Hallyn <[email protected]>
Signed-off-by Richard Guy Briggs <[email protected]>
Reviewed-by Serge Hallyn <[email protected]>
---
 security/commoncap.c |   63 +++++++++++++++++++++++++++----------------------
 1 files changed, 35 insertions(+), 28 deletions(-)

Signature · 2 lines

James Morris
<[email protected]>

Kees Cook

Acked-by on v4 1/10

2017-09-07 19:42 UTC #3 lore raw

Re: [PATCH V4 01/10] capabilities: factor out cap_bprm_set_creds privileged root

Quoted 6 lines · Richard Guy Briggs

Factor out the case of privileged root from the function cap_bprm_set_creds() to make the latter easier to read and analyse.
Suggested-by Serge Hallyn <[email protected]>
Signed-off-by Richard Guy Briggs <[email protected]>
Reviewed-by Serge Hallyn <[email protected]>

Note below...

Quoted 9 lines

---
 security/commoncap.c |   63 +++++++++++++++++++++++++++----------------------
 1 files changed, 35 insertions(+), 28 deletions(-)

security/commoncap.c +0 -0

index d8e26fb..927fe93 100644

security/commoncap.c static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c · line 476

Quoted 5 lines

security/commoncap.c +2 -0

@@ -472,6 +472,39 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
        return rc;
 }
 
+static void handle_privileged_root(struct linux_binprm *bprm, bool has_cap,
+                                  bool *effective, kuid_t root_uid)

If you do a v5 of this, I think it'd be nice to add a comment here describing what is being checked and the side-effects (i.e. cap_permitted changes, when effective is set, etc).

-Kees

Quoted 86 lines

security/commoncap.c +33 ~~-29~~

+{
+       const struct cred *old = current_cred();
+       struct cred *new = bprm->cred;
+
+       if (issecure(SECURE_NOROOT))
+               return;
+       /*
+        * If the legacy file capability is set, then don't set privs
+        * for a setuid root binary run by a non-root user.  Do set it
+        * for a root user just to cause least surprise to an admin.
+        */
+       if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) {
+               warn_setuid_and_fcaps_mixed(bprm->filename);
+               return;
+       }
+       /*
+        * To support inheritance of root-permissions and suid-root
+        * executables under compatibility mode, we override the
+        * capability sets for the file.
+        *
+        * If only the real uid is 0, we do not set the effective bit.
+        */
+       if (uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid)) {
+               /* pP' = (cap_bset & ~0) | (pI & ~0) */
+               new->cap_permitted = cap_combine(old->cap_bset,
+                                                old->cap_inheritable);
+       }
+       if (uid_eq(new->euid, root_uid))
+               *effective = true;
+}
+
 /**
  * cap_bprm_set_creds - Set up the proposed credentials for execve().
  * @bprm: The execution parameters, including the proposed creds

@@ -484,46 +517,20 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 {
        const struct cred *old = current_cred();
        struct cred *new = bprm->cred;
-       bool effective, has_cap = false, is_setid;
+       bool effective = false, has_cap = false, is_setid;
        int ret;
        kuid_t root_uid;
 
        if (WARN_ON(!cap_ambient_invariant_ok(old)))
                return -EPERM;
 
-       effective = false;
        ret = get_file_caps(bprm, &effective, &has_cap);
        if (ret < 0)
                return ret;
 
        root_uid = make_kuid(new->user_ns, 0);
 
-       if (!issecure(SECURE_NOROOT)) {
-               /*
-                * If the legacy file capability is set, then don't set privs
-                * for a setuid root binary run by a non-root user.  Do set it
-                * for a root user just to cause least surprise to an admin.
-                */
-               if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) {
-                       warn_setuid_and_fcaps_mixed(bprm->filename);
-                       goto skip;
-               }
-               /*
-                * To support inheritance of root-permissions and suid-root
-                * executables under compatibility mode, we override the
-                * capability sets for the file.
-                *
-                * If only the real uid is 0, we do not set the effective bit.
-                */
-               if (uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid)) {
-                       /* pP' = (cap_bset & ~0) | (pI & ~0) */
-                       new->cap_permitted = cap_combine(old->cap_bset,
-                                                        old->cap_inheritable);
-               }
-               if (uid_eq(new->euid, root_uid))
-                       effective = true;
-       }
-skip:
+       handle_privileged_root(bprm, has_cap, &effective, root_uid);
 
        /* if we have fs caps, clear dangerous personality flags */
        if (!cap_issubset(new->cap_permitted, old->cap_permitted))
--

1.7.1

Signature · 2 lines

Kees Cook
Pixel Security

Richard Guy Briggs

author

2017-09-05 06:46 UTC #4 lore raw

PATCH v4 2/10 capabilities: intuitive names for cap gain status

Introduce macros cap_gained, cap_grew, cap_full to make the use of the negation of is_subset() easier to read and analyse.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 security/commoncap.c |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

security/commoncap.c +11 -7

index 927fe93..cf6e2b0 100644
@@ -505,6 +505,12 @@ static void handle_privileged_root(struct linux_binprm *bprm, bool has_cap,
 		*effective = true;
 }
 
+#define __cap_gained(field, target, source) \
+	!cap_issubset(target->cap_##field, source->cap_##field)
+#define __cap_grew(target, source, cred) \
+	!cap_issubset(cred->cap_##target, cred->cap_##source)
+#define __cap_full(field, cred) \
+	cap_issubset(CAP_FULL_SET, cred->cap_##field)
 /**
  * cap_bprm_set_creds - Set up the proposed credentials for execve().
  * @bprm: The execution parameters, including the proposed creds

@@ -533,10 +539,9 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 	handle_privileged_root(bprm, has_cap, &effective, root_uid);
 
 	/* if we have fs caps, clear dangerous personality flags */
-	if (!cap_issubset(new->cap_permitted, old->cap_permitted))
+	if (__cap_gained(permitted, new, old))
 		bprm->per_clear |= PER_CLEAR_ON_SETID;
 
-
 	/* Don't let someone trace a set[ug]id/setpcap binary with the revised
 	 * credentials unless they have the appropriate permit.
 	 *

@@ -544,8 +549,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 	 */
 	is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid);
 
-	if ((is_setid ||
-	     !cap_issubset(new->cap_permitted, old->cap_permitted)) &&
+	if ((is_setid || __cap_gained(permitted, new, old)) &&
 	    ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
 	     !ptracer_capable(current, new->user_ns))) {
 		/* downgrade; they get no more than they had, and maybe less */

@@ -595,8 +599,8 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 	 * Number 1 above might fail if you don't have a full bset, but I think
 	 * that is interesting information to audit.
 	 */
-	if (!cap_issubset(new->cap_effective, new->cap_ambient)) {
-		if (!cap_issubset(CAP_FULL_SET, new->cap_effective) ||
+	if (__cap_grew(effective, ambient, new)) {
+		if (!__cap_full(effective, new) ||
 		    !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) ||
 		    issecure(SECURE_NOROOT)) {
 			ret = audit_log_bprm_fcaps(bprm, new, old);

@@ -616,7 +620,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 		bprm->cap_elevated = 1;
 	} else if (!uid_eq(new->uid, root_uid)) {
 		if (effective ||
-		    !cap_issubset(new->cap_permitted, new->cap_ambient))
+		    __cap_grew(permitted, ambient, new))
 			bprm->cap_elevated = 1;
 	}

Signature · 1 line

1.7.1

Kees Cook

Acked-by on v4 2/10

2017-09-07 19:57 UTC #5 lore raw

Re: [PATCH V4 02/10] capabilities: intuitive names for cap gain status

Quoted 6 lines · Richard Guy Briggs

Introduce macros cap_gained, cap_grew, cap_full to make the use of the negation of is_subset() easier to read and analyse.
Signed-off-by Richard Guy Briggs <[email protected]>
Reviewed-by Serge Hallyn <[email protected]>
Acked-by James Morris <[email protected]>

I still find these hard to read, but it IS better than it was before. ;)

Quoted 67 lines

---
 security/commoncap.c |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

security/commoncap.c +6 -0

index 927fe93..cf6e2b0 100644
@@ -505,6 +505,12 @@ static void handle_privileged_root(struct linux_binprm *bprm, bool has_cap,
                *effective = true;
 }
 
+#define __cap_gained(field, target, source) \
+       !cap_issubset(target->cap_##field, source->cap_##field)
+#define __cap_grew(target, source, cred) \
+       !cap_issubset(cred->cap_##target, cred->cap_##source)
+#define __cap_full(field, cred) \
+       cap_issubset(CAP_FULL_SET, cred->cap_##field)
 
 /**
  * cap_bprm_set_creds - Set up the proposed credentials for execve().

* @bprm: The execution parameters, including the proposed creds

security/commoncap.c +5 -8

@@ -533,10 +539,9 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
        handle_privileged_root(bprm, has_cap, &effective, root_uid);
 
        /* if we have fs caps, clear dangerous personality flags */
-       if (!cap_issubset(new->cap_permitted, old->cap_permitted))
+       if (__cap_gained(permitted, new, old))
                bprm->per_clear |= PER_CLEAR_ON_SETID;
 
-
        /* Don't let someone trace a set[ug]id/setpcap binary with the revised
         * credentials unless they have the appropriate permit.
         *

@@ -544,8 +549,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
         */
        is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid);
 
-       if ((is_setid ||
-            !cap_issubset(new->cap_permitted, old->cap_permitted)) &&
+       if ((is_setid || __cap_gained(permitted, new, old)) &&
            ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
             !ptracer_capable(current, new->user_ns))) {
                /* downgrade; they get no more than they had, and maybe less */

@@ -595,8 +599,8 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
         * Number 1 above might fail if you don't have a full bset, but I think
         * that is interesting information to audit.
         */
-       if (!cap_issubset(new->cap_effective, new->cap_ambient)) {
-               if (!cap_issubset(CAP_FULL_SET, new->cap_effective) ||
+       if (__cap_grew(effective, ambient, new)) {
+               if (!__cap_full(effective, new) ||
                    !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) ||
                    issecure(SECURE_NOROOT)) {
                        ret = audit_log_bprm_fcaps(bprm, new, old);

@@ -616,7 +620,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
                bprm->cap_elevated = 1;
        } else if (!uid_eq(new->uid, root_uid)) {
                if (effective ||
-                   !cap_issubset(new->cap_permitted, new->cap_ambient))
+                   __cap_grew(permitted, ambient, new))
                        bprm->cap_elevated = 1;
        }
 
--

1.7.1

Signature · 2 lines

Kees Cook
Pixel Security

Richard Guy Briggs

author

2017-09-05 06:46 UTC #6 lore raw

PATCH v4 3/10 capabilities: rename has_cap to has_fcap

Rename has_cap to has_fcap to clarify it applies to file capabilities since the entire source file is about capabilities.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 security/commoncap.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

security/commoncap.c +10 ~~-10~~

index cf6e2b0..623f251 100644
@@ -330,7 +330,7 @@ int cap_inode_killpriv(struct dentry *dentry)
 static inline int bprm_caps_from_vfs_caps(struct cpu_vfs_cap_data *caps,
 					  struct linux_binprm *bprm,
 					  bool *effective,
-					  bool *has_cap)
+					  bool *has_fcap)
 {
 	struct cred *new = bprm->cred;
 	unsigned i;

@@ -340,7 +340,7 @@ static inline int bprm_caps_from_vfs_caps(struct cpu_vfs_cap_data *caps,
 		*effective = true;
 
 	if (caps->magic_etc & VFS_CAP_REVISION_MASK)
-		*has_cap = true;
+		*has_fcap = true;
 
 	CAP_FOR_EACH_U32(i) {
 		__u32 permitted = caps->permitted.cap[i];

@@ -429,7 +429,7 @@ int get_vfs_caps_from_disk(const struct dentry *dentry, struct cpu_vfs_cap_data 
  * its xattrs and, if present, apply them to the proposed credentials being
  * constructed by execve().
  */
-static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_cap)
+static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_fcap)
 {
 	int rc = 0;
 	struct cpu_vfs_cap_data vcaps;

@@ -460,7 +460,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
 		goto out;
 	}
 
-	rc = bprm_caps_from_vfs_caps(&vcaps, bprm, effective, has_cap);
+	rc = bprm_caps_from_vfs_caps(&vcaps, bprm, effective, has_fcap);
 	if (rc == -EINVAL)
 		printk(KERN_NOTICE "%s: cap_from_disk returned %d for %s\n",
 		       __func__, rc, bprm->filename);

@@ -472,7 +472,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
 	return rc;
 }
 
-static void handle_privileged_root(struct linux_binprm *bprm, bool has_cap,
+static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap,
 				   bool *effective, kuid_t root_uid)
 {
 	const struct cred *old = current_cred();

@@ -485,7 +485,7 @@ static void handle_privileged_root(struct linux_binprm *bprm, bool has_cap,
 	 * for a setuid root binary run by a non-root user.  Do set it
 	 * for a root user just to cause least surprise to an admin.
 	 */
-	if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) {
+	if (has_fcap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) {
 		warn_setuid_and_fcaps_mixed(bprm->filename);
 		return;
 	}

@@ -523,20 +523,20 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 {
 	const struct cred *old = current_cred();
 	struct cred *new = bprm->cred;
-	bool effective = false, has_cap = false, is_setid;
+	bool effective = false, has_fcap = false, is_setid;
 	int ret;
 	kuid_t root_uid;
 
 	if (WARN_ON(!cap_ambient_invariant_ok(old)))
 		return -EPERM;
 
-	ret = get_file_caps(bprm, &effective, &has_cap);
+	ret = get_file_caps(bprm, &effective, &has_fcap);
 	if (ret < 0)
 		return ret;
 
 	root_uid = make_kuid(new->user_ns, 0);
 
-	handle_privileged_root(bprm, has_cap, &effective, root_uid);
+	handle_privileged_root(bprm, has_fcap, &effective, root_uid);
 
 	/* if we have fs caps, clear dangerous personality flags */
 	if (__cap_gained(permitted, new, old))

@@ -566,7 +566,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 	new->sgid = new->fsgid = new->egid;
 
 	/* File caps or setid cancels ambient. */
-	if (has_cap || is_setid)
+	if (has_fcap || is_setid)
 		cap_clear(new->cap_ambient);
 
 	/*

Signature · 1 line

1.7.1

Kees Cook

Acked-by on v4 3/10

2017-09-08 18:15 UTC #7 lore raw

Re: [PATCH V4 03/10] capabilities: rename has_cap to has_fcap

Quoted 6 lines · Richard Guy Briggs

Rename has_cap to has_fcap to clarify it applies to file capabilities since the entire source file is about capabilities.
Signed-off-by Richard Guy Briggs <[email protected]>
Reviewed-by Serge Hallyn <[email protected]>
Acked-by James Morris <[email protected]>

-Kees

Quoted 98 lines

---
 security/commoncap.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

security/commoncap.c +10 ~~-11~~

index cf6e2b0..623f251 100644
@@ -330,7 +330,7 @@ int cap_inode_killpriv(struct dentry *dentry)
 static inline int bprm_caps_from_vfs_caps(struct cpu_vfs_cap_data *caps,
                                          struct linux_binprm *bprm,
                                          bool *effective,
-                                         bool *has_cap)
+                                         bool *has_fcap)
 {
        struct cred *new = bprm->cred;
        unsigned i;

@@ -340,7 +340,7 @@ static inline int bprm_caps_from_vfs_caps(struct cpu_vfs_cap_data *caps,
                *effective = true;
 
        if (caps->magic_etc & VFS_CAP_REVISION_MASK)
-               *has_cap = true;
+               *has_fcap = true;
 
        CAP_FOR_EACH_U32(i) {
                __u32 permitted = caps->permitted.cap[i];

@@ -429,7 +429,7 @@ int get_vfs_caps_from_disk(const struct dentry *dentry, struct cpu_vfs_cap_data
  * its xattrs and, if present, apply them to the proposed credentials being
  * constructed by execve().
  */
-static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_cap)
+static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_fcap)
 {
        int rc = 0;
        struct cpu_vfs_cap_data vcaps;

@@ -460,7 +460,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
                goto out;
        }
 
-       rc = bprm_caps_from_vfs_caps(&vcaps, bprm, effective, has_cap);
+       rc = bprm_caps_from_vfs_caps(&vcaps, bprm, effective, has_fcap);
        if (rc == -EINVAL)
                printk(KERN_NOTICE "%s: cap_from_disk returned %d for %s\n",
                       __func__, rc, bprm->filename);

@@ -472,7 +472,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
        return rc;
 }
 
-static void handle_privileged_root(struct linux_binprm *bprm, bool has_cap,
+static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap,
                                   bool *effective, kuid_t root_uid)
 {
        const struct cred *old = current_cred();

@@ -485,7 +485,7 @@ static void handle_privileged_root(struct linux_binprm *bprm, bool has_cap,
         * for a setuid root binary run by a non-root user.  Do set it
         * for a root user just to cause least surprise to an admin.
         */
-       if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) {
+       if (has_fcap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) {
                warn_setuid_and_fcaps_mixed(bprm->filename);
                return;
        }

@@ -523,20 +523,20 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 {
        const struct cred *old = current_cred();
        struct cred *new = bprm->cred;
-       bool effective = false, has_cap = false, is_setid;
+       bool effective = false, has_fcap = false, is_setid;
        int ret;
        kuid_t root_uid;
 
        if (WARN_ON(!cap_ambient_invariant_ok(old)))
                return -EPERM;
 
-       ret = get_file_caps(bprm, &effective, &has_cap);
+       ret = get_file_caps(bprm, &effective, &has_fcap);
        if (ret < 0)
                return ret;
 
        root_uid = make_kuid(new->user_ns, 0);
 
-       handle_privileged_root(bprm, has_cap, &effective, root_uid);
+       handle_privileged_root(bprm, has_fcap, &effective, root_uid);
 
        /* if we have fs caps, clear dangerous personality flags */
        if (__cap_gained(permitted, new, old))

@@ -566,7 +566,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
        new->sgid = new->fsgid = new->egid;
 
        /* File caps or setid cancels ambient. */
-       if (has_cap || is_setid)
+       if (has_fcap || is_setid)
                cap_clear(new->cap_ambient);
 
        /*
--

1.7.1

Signature · 2 lines

Kees Cook
Pixel Security

Richard Guy Briggs

author

2017-09-05 06:46 UTC #8 lore raw

PATCH v4 4/10 capabilities: use root_priveleged inline to clarify logic

Introduce inline root_privileged() to make use of SECURE_NONROOT easier to read.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 security/commoncap.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

security/commoncap.c +5 -4

index 623f251..1904f49 100644
@@ -472,14 +472,14 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f
 	return rc;
 }
 
+static inline bool root_privileged(void) { return !issecure(SECURE_NOROOT); }
+
 static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap,
 				   bool *effective, kuid_t root_uid)
 {
 	const struct cred *old = current_cred();
 	struct cred *new = bprm->cred;
 
-	if (issecure(SECURE_NOROOT))
-		return;
 	/*
 	 * If the legacy file capability is set, then don't set privs
 	 * for a setuid root binary run by a non-root user.  Do set it

@@ -536,7 +536,8 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 
 	root_uid = make_kuid(new->user_ns, 0);
 
-	handle_privileged_root(bprm, has_fcap, &effective, root_uid);
+	if (root_privileged())
+		handle_privileged_root(bprm, has_fcap, &effective, root_uid);
 
 	/* if we have fs caps, clear dangerous personality flags */
 	if (__cap_gained(permitted, new, old))

@@ -602,7 +603,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 	if (__cap_grew(effective, ambient, new)) {
 		if (!__cap_full(effective, new) ||
 		    !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) ||
-		    issecure(SECURE_NOROOT)) {
+		    !root_privileged()) {
 			ret = audit_log_bprm_fcaps(bprm, new, old);
 			if (ret < 0)
 				return ret;

Signature · 1 line

1.7.1

Kees Cook

Acked-by on v4 4/10

2017-09-08 18:18 UTC #9 lore raw

Re: [PATCH V4 04/10] capabilities: use root_priveleged inline to clarify logic

Quoted 19 lines · Richard Guy Briggs

Introduce inline root_privileged() to make use of SECURE_NONROOT easier to read.
Suggested-by Serge Hallyn <[email protected]>
Signed-off-by Richard Guy Briggs <[email protected]>
Reviewed-by Serge Hallyn <[email protected]>
Acked-by James Morris <[email protected]>
---
 security/commoncap.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)
security/commoncap.c +0 -0
index 623f251..1904f49 100644
@@ -472,14 +472,14 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f
        return rc;
 }
 

security/commoncap.c static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f · line 482

Quoted 10 lines

security/commoncap.c +2 -2

+static inline bool root_privileged(void) { return !issecure(SECURE_NOROOT); }
+
 static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap,
                                   bool *effective, kuid_t root_uid)
 {
        const struct cred *old = current_cred();
        struct cred *new = bprm->cred;
 
-       if (issecure(SECURE_NOROOT))
-               return;

Seems like it'd be better to just leave this check here (with the new inline name). But if there's no v5, then consider it:

-Kees

Quoted 25 lines

security/commoncap.c +3 -3

        /*
         * If the legacy file capability is set, then don't set privs
         * for a setuid root binary run by a non-root user.  Do set it

@@ -536,7 +536,8 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 
        root_uid = make_kuid(new->user_ns, 0);
 
-       handle_privileged_root(bprm, has_fcap, &effective, root_uid);
+       if (root_privileged())
+               handle_privileged_root(bprm, has_fcap, &effective, root_uid);
 
        /* if we have fs caps, clear dangerous personality flags */
        if (__cap_gained(permitted, new, old))

@@ -602,7 +603,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
        if (__cap_grew(effective, ambient, new)) {
                if (!__cap_full(effective, new) ||
                    !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) ||
-                   issecure(SECURE_NOROOT)) {
+                   !root_privileged()) {
                        ret = audit_log_bprm_fcaps(bprm, new, old);
                        if (ret < 0)
                                return ret;
--

1.7.1

Signature · 2 lines

Kees Cook
Pixel Security

Richard Guy Briggs

author

2017-09-05 06:46 UTC #10 lore raw

PATCH v4 5/10 capabilities: use intuitive names for id changes

Introduce a number of inlines to make the use of the negation of uid_eq() easier to read and analyse.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 security/commoncap.c |   28 ++++++++++++++++++++++------
 1 files changed, 22 insertions(+), 6 deletions(-)

security/commoncap.c +22 -6

index 1904f49..d37ebec 100644
@@ -474,6 +474,15 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f
 
 static inline bool root_privileged(void) { return !issecure(SECURE_NOROOT); }
 
+static inline bool __is_real(kuid_t uid, struct cred *cred)
+{ return uid_eq(cred->uid, uid); }
+
+static inline bool __is_eff(kuid_t uid, struct cred *cred)
+{ return uid_eq(cred->euid, uid); }
+
+static inline bool __is_suid(kuid_t uid, struct cred *cred)
+{ return !__is_real(uid, cred) && __is_eff(uid, cred); }
+
 static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap,
 				   bool *effective, kuid_t root_uid)
 {

@@ -485,7 +494,7 @@ static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap,
 	 * for a setuid root binary run by a non-root user.  Do set it
 	 * for a root user just to cause least surprise to an admin.
 	 */
-	if (has_fcap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) {
+	if (has_fcap && __is_suid(root_uid, new)) {
 		warn_setuid_and_fcaps_mixed(bprm->filename);
 		return;
 	}

@@ -496,12 +505,12 @@ static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap,
 	 *
 	 * If only the real uid is 0, we do not set the effective bit.
 	 */
-	if (uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid)) {
+	if (__is_eff(root_uid, new) || __is_real(root_uid, new)) {
 		/* pP' = (cap_bset & ~0) | (pI & ~0) */
 		new->cap_permitted = cap_combine(old->cap_bset,
 						 old->cap_inheritable);
 	}
-	if (uid_eq(new->euid, root_uid))
+	if (__is_eff(root_uid, new))
 		*effective = true;
 }

@@ -511,6 +520,13 @@ static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap,
 	!cap_issubset(cred->cap_##target, cred->cap_##source)
 #define __cap_full(field, cred) \
 	cap_issubset(CAP_FULL_SET, cred->cap_##field)
+
+static inline bool __is_setuid(struct cred *new, const struct cred *old)
+{ return !uid_eq(new->euid, old->uid); }
+
+static inline bool __is_setgid(struct cred *new, const struct cred *old)
+{ return !gid_eq(new->egid, old->gid); }
+
 /**
  * cap_bprm_set_creds - Set up the proposed credentials for execve().
  * @bprm: The execution parameters, including the proposed creds

@@ -548,7 +564,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 	 *
 	 * In addition, if NO_NEW_PRIVS, then ensure we get no new privs.
 	 */
-	is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid);
+	is_setid = __is_setuid(new, old) || __is_setgid(new, old);
 
 	if ((is_setid || __cap_gained(permitted, new, old)) &&
 	    ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||

@@ -602,7 +618,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 	 */
 	if (__cap_grew(effective, ambient, new)) {
 		if (!__cap_full(effective, new) ||
-		    !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) ||
+		    !__is_eff(root_uid, new) || !__is_real(root_uid, new) ||
 		    !root_privileged()) {
 			ret = audit_log_bprm_fcaps(bprm, new, old);
 			if (ret < 0)

@@ -619,7 +635,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 	bprm->cap_elevated = 0;
 	if (is_setid) {
 		bprm->cap_elevated = 1;
-	} else if (!uid_eq(new->uid, root_uid)) {
+	} else if (!__is_real(root_uid, new)) {
 		if (effective ||
 		    __cap_grew(permitted, ambient, new))
 			bprm->cap_elevated = 1;

Signature · 1 line

1.7.1

Kees Cook

Acked-by on v4 5/10

2017-09-08 18:22 UTC #11 lore raw

Re: [PATCH V4 05/10] capabilities: use intuitive names for id changes

Quoted 6 lines · Richard Guy Briggs

Introduce a number of inlines to make the use of the negation of uid_eq() easier to read and analyse.
Signed-off-by Richard Guy Briggs <[email protected]>
Reviewed-by Serge Hallyn <[email protected]>
Acked-by James Morris <[email protected]>

-Kees

Quoted 92 lines

---
 security/commoncap.c |   28 ++++++++++++++++++++++------
 1 files changed, 22 insertions(+), 6 deletions(-)

security/commoncap.c +22 -7

index 1904f49..d37ebec 100644
@@ -474,6 +474,15 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f
 
 static inline bool root_privileged(void) { return !issecure(SECURE_NOROOT); }
 
+static inline bool __is_real(kuid_t uid, struct cred *cred)
+{ return uid_eq(cred->uid, uid); }
+
+static inline bool __is_eff(kuid_t uid, struct cred *cred)
+{ return uid_eq(cred->euid, uid); }
+
+static inline bool __is_suid(kuid_t uid, struct cred *cred)
+{ return !__is_real(uid, cred) && __is_eff(uid, cred); }
+
 static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap,
                                   bool *effective, kuid_t root_uid)
 {

@@ -485,7 +494,7 @@ static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap,
         * for a setuid root binary run by a non-root user.  Do set it
         * for a root user just to cause least surprise to an admin.
         */
-       if (has_fcap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) {
+       if (has_fcap && __is_suid(root_uid, new)) {
                warn_setuid_and_fcaps_mixed(bprm->filename);
                return;
        }

@@ -496,12 +505,12 @@ static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap,
         *
         * If only the real uid is 0, we do not set the effective bit.
         */
-       if (uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid)) {
+       if (__is_eff(root_uid, new) || __is_real(root_uid, new)) {
                /* pP' = (cap_bset & ~0) | (pI & ~0) */
                new->cap_permitted = cap_combine(old->cap_bset,
                                                 old->cap_inheritable);
        }
-       if (uid_eq(new->euid, root_uid))
+       if (__is_eff(root_uid, new))
                *effective = true;
 }

@@ -511,6 +520,13 @@ static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap,
        !cap_issubset(cred->cap_##target, cred->cap_##source)
 #define __cap_full(field, cred) \
        cap_issubset(CAP_FULL_SET, cred->cap_##field)
+
+static inline bool __is_setuid(struct cred *new, const struct cred *old)
+{ return !uid_eq(new->euid, old->uid); }
+
+static inline bool __is_setgid(struct cred *new, const struct cred *old)
+{ return !gid_eq(new->egid, old->gid); }
+
 /**
  * cap_bprm_set_creds - Set up the proposed credentials for execve().
  * @bprm: The execution parameters, including the proposed creds

@@ -548,7 +564,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
         *
         * In addition, if NO_NEW_PRIVS, then ensure we get no new privs.
         */
-       is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid);
+       is_setid = __is_setuid(new, old) || __is_setgid(new, old);
 
        if ((is_setid || __cap_gained(permitted, new, old)) &&
            ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||

@@ -602,7 +618,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
         */
        if (__cap_grew(effective, ambient, new)) {
                if (!__cap_full(effective, new) ||
-                   !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) ||
+                   !__is_eff(root_uid, new) || !__is_real(root_uid, new) ||
                    !root_privileged()) {
                        ret = audit_log_bprm_fcaps(bprm, new, old);
                        if (ret < 0)

@@ -619,7 +635,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
        bprm->cap_elevated = 0;
        if (is_setid) {
                bprm->cap_elevated = 1;
-       } else if (!uid_eq(new->uid, root_uid)) {
+       } else if (!__is_real(root_uid, new)) {
                if (effective ||
                    __cap_grew(permitted, ambient, new))
                        bprm->cap_elevated = 1;
--

1.7.1

Signature · 6 lines

Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Richard Guy Briggs

author

2017-09-05 06:46 UTC #12 lore raw

PATCH v4 6/10 capabilities: move audit log decision to function

Move the audit log decision logic to its own function to isolate the complexity in one place.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 security/commoncap.c |   50 ++++++++++++++++++++++++++++++--------------------
 1 files changed, 30 insertions(+), 20 deletions(-)

security/commoncap.c +30 ~~-20~~

index d37ebec..eae7431 100644
@@ -527,6 +527,32 @@ static inline bool __is_setuid(struct cred *new, const struct cred *old)
 static inline bool __is_setgid(struct cred *new, const struct cred *old)
 { return !gid_eq(new->egid, old->gid); }
 
+/*
+ * Audit candidate if current->cap_effective is set
+ *
+ * We do not bother to audit if 3 things are true:
+ *   1) cap_effective has all caps
+ *   2) we are root
+ *   3) root is supposed to have all caps (SECURE_NOROOT)
+ * Since this is just a normal root execing a process.
+ *
+ * Number 1 above might fail if you don't have a full bset, but I think
+ * that is interesting information to audit.
+ */
+static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
+{
+	bool ret = false;
+
+	if (__cap_grew(effective, ambient, cred)) {
+		if (!__cap_full(effective, cred) ||
+		    !__is_eff(root, cred) || !__is_real(root, cred) ||
+		    !root_privileged()) {
+			ret = true;
+		}
+	}
+	return ret;
+}
+
 /**
  * cap_bprm_set_creds - Set up the proposed credentials for execve().
  * @bprm: The execution parameters, including the proposed creds

@@ -604,26 +630,10 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 	if (WARN_ON(!cap_ambient_invariant_ok(new)))
 		return -EPERM;
 
-	/*
-	 * Audit candidate if current->cap_effective is set
-	 *
-	 * We do not bother to audit if 3 things are true:
-	 *   1) cap_effective has all caps
-	 *   2) we are root
-	 *   3) root is supposed to have all caps (SECURE_NOROOT)
-	 * Since this is just a normal root execing a process.
-	 *
-	 * Number 1 above might fail if you don't have a full bset, but I think
-	 * that is interesting information to audit.
-	 */
-	if (__cap_grew(effective, ambient, new)) {
-		if (!__cap_full(effective, new) ||
-		    !__is_eff(root_uid, new) || !__is_real(root_uid, new) ||
-		    !root_privileged()) {
-			ret = audit_log_bprm_fcaps(bprm, new, old);
-			if (ret < 0)
-				return ret;
-		}
+	if (nonroot_raised_pE(new, root_uid)) {
+		ret = audit_log_bprm_fcaps(bprm, new, old);
+		if (ret < 0)
+			return ret;
 	}
 
 	new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);

Signature · 1 line

1.7.1

Kees Cook

Acked-by on v4 6/10

2017-09-08 18:23 UTC #13 lore raw

Re: [PATCH V4 06/10] capabilities: move audit log decision to function

Quoted 7 lines · Richard Guy Briggs

Move the audit log decision logic to its own function to isolate the complexity in one place.
Suggested-by Serge Hallyn <[email protected]>
Signed-off-by Richard Guy Briggs <[email protected]>
Reviewed-by Serge Hallyn <[email protected]>
Acked-by James Morris <[email protected]>

-Kees

Quoted 75 lines

---
 security/commoncap.c |   50 ++++++++++++++++++++++++++++++--------------------
 1 files changed, 30 insertions(+), 20 deletions(-)

security/commoncap.c +30 ~~-21~~

index d37ebec..eae7431 100644
@@ -527,6 +527,32 @@ static inline bool __is_setuid(struct cred *new, const struct cred *old)
 static inline bool __is_setgid(struct cred *new, const struct cred *old)
 { return !gid_eq(new->egid, old->gid); }
 
+/*
+ * Audit candidate if current->cap_effective is set
+ *
+ * We do not bother to audit if 3 things are true:
+ *   1) cap_effective has all caps
+ *   2) we are root
+ *   3) root is supposed to have all caps (SECURE_NOROOT)
+ * Since this is just a normal root execing a process.
+ *
+ * Number 1 above might fail if you don't have a full bset, but I think
+ * that is interesting information to audit.
+ */
+static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
+{
+       bool ret = false;
+
+       if (__cap_grew(effective, ambient, cred)) {
+               if (!__cap_full(effective, cred) ||
+                   !__is_eff(root, cred) || !__is_real(root, cred) ||
+                   !root_privileged()) {
+                       ret = true;
+               }
+       }
+       return ret;
+}
+
 /**
  * cap_bprm_set_creds - Set up the proposed credentials for execve().
  * @bprm: The execution parameters, including the proposed creds

@@ -604,26 +630,10 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
        if (WARN_ON(!cap_ambient_invariant_ok(new)))
                return -EPERM;
 
-       /*
-        * Audit candidate if current->cap_effective is set
-        *
-        * We do not bother to audit if 3 things are true:
-        *   1) cap_effective has all caps
-        *   2) we are root
-        *   3) root is supposed to have all caps (SECURE_NOROOT)
-        * Since this is just a normal root execing a process.
-        *
-        * Number 1 above might fail if you don't have a full bset, but I think
-        * that is interesting information to audit.
-        */
-       if (__cap_grew(effective, ambient, new)) {
-               if (!__cap_full(effective, new) ||
-                   !__is_eff(root_uid, new) || !__is_real(root_uid, new) ||
-                   !root_privileged()) {
-                       ret = audit_log_bprm_fcaps(bprm, new, old);
-                       if (ret < 0)
-                               return ret;
-               }
+       if (nonroot_raised_pE(new, root_uid)) {
+               ret = audit_log_bprm_fcaps(bprm, new, old);
+               if (ret < 0)
+                       return ret;
        }
 
        new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
--

1.7.1

Signature · 2 lines

Kees Cook
Pixel Security

Richard Guy Briggs

author

2017-09-05 06:46 UTC #14 lore raw

PATCH v4 7/10 capabilities: remove a layer of conditional logic

Remove a layer of conditional logic to make the use of conditions easier to read and analyse.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 security/commoncap.c |   23 ++++++++++-------------
 1 files changed, 10 insertions(+), 13 deletions(-)

security/commoncap.c +10 ~~-13~~

index eae7431..cf95d73 100644
@@ -543,13 +543,12 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
 {
 	bool ret = false;
 
-	if (__cap_grew(effective, ambient, cred)) {
-		if (!__cap_full(effective, cred) ||
-		    !__is_eff(root, cred) || !__is_real(root, cred) ||
-		    !root_privileged()) {
-			ret = true;
-		}
-	}
+	if (__cap_grew(effective, ambient, cred) &&
+	    (!__cap_full(effective, cred) ||
+	     !__is_eff(root, cred) ||
+	     !__is_real(root, cred) ||
+	     !root_privileged()))
+		ret = true;
 	return ret;
 }

@@ -643,13 +642,11 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 
 	/* Check for privilege-elevated exec. */
 	bprm->cap_elevated = 0;
-	if (is_setid) {
+	if (is_setid ||
+	    (!__is_real(root_uid, new) &&
+	     (effective ||
+	      __cap_grew(permitted, ambient, new))))
 		bprm->cap_elevated = 1;
-	} else if (!__is_real(root_uid, new)) {
-		if (effective ||
-		    __cap_grew(permitted, ambient, new))
-			bprm->cap_elevated = 1;
-	}
 
 	return 0;
 }

Signature · 1 line

1.7.1

Kees Cook

Acked-by on v4 7/10

2017-09-08 18:26 UTC #15 lore raw

Re: [PATCH V4 07/10] capabilities: remove a layer of conditional logic

Quoted 6 lines · Richard Guy Briggs

Remove a layer of conditional logic to make the use of conditions easier to read and analyse.
Signed-off-by Richard Guy Briggs <[email protected]>
Reviewed-by Serge Hallyn <[email protected]>
Acked-by James Morris <[email protected]>

-Kees

Quoted 49 lines

---
 security/commoncap.c |   23 ++++++++++-------------
 1 files changed, 10 insertions(+), 13 deletions(-)

security/commoncap.c +10 ~~-14~~

index eae7431..cf95d73 100644
@@ -543,13 +543,12 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
 {
        bool ret = false;
 
-       if (__cap_grew(effective, ambient, cred)) {
-               if (!__cap_full(effective, cred) ||
-                   !__is_eff(root, cred) || !__is_real(root, cred) ||
-                   !root_privileged()) {
-                       ret = true;
-               }
-       }
+       if (__cap_grew(effective, ambient, cred) &&
+           (!__cap_full(effective, cred) ||
+            !__is_eff(root, cred) ||
+            !__is_real(root, cred) ||
+            !root_privileged()))
+               ret = true;
        return ret;
 }

@@ -643,13 +642,11 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 
        /* Check for privilege-elevated exec. */
        bprm->cap_elevated = 0;
-       if (is_setid) {
+       if (is_setid ||
+           (!__is_real(root_uid, new) &&
+            (effective ||
+             __cap_grew(permitted, ambient, new))))
                bprm->cap_elevated = 1;
-       } else if (!__is_real(root_uid, new)) {
-               if (effective ||
-                   __cap_grew(permitted, ambient, new))
-                       bprm->cap_elevated = 1;
-       }
 
        return 0;
 }
--

1.7.1

Signature · 2 lines

Kees Cook
Pixel Security

Richard Guy Briggs

author

2017-09-05 06:46 UTC #16 lore raw

PATCH v4 8/10 capabilities: invert logic for clarity

The way the logic was presented, it was awkward to read and verify. Invert the logic using DeMorgan's Law to be more easily able to read and understand.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 security/commoncap.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

security/commoncap.c +4 -4

index cf95d73..7e8041d 100644
@@ -544,10 +544,10 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
 	bool ret = false;
 
 	if (__cap_grew(effective, ambient, cred) &&
-	    (!__cap_full(effective, cred) ||
-	     !__is_eff(root, cred) ||
-	     !__is_real(root, cred) ||
-	     !root_privileged()))
+	    !(__cap_full(effective, cred) &&
+	      __is_eff(root, cred) &&
+	      __is_real(root, cred) &&
+	      root_privileged()))
 		ret = true;
 	return ret;
 }

Signature · 1 line

1.7.1

Kees Cook

Acked-by on v4 8/10

2017-09-08 18:27 UTC #17 lore raw

Re: [PATCH V4 08/10] capabilities: invert logic for clarity

Quoted 7 lines · Richard Guy Briggs

The way the logic was presented, it was awkward to read and verify. Invert the logic using DeMorgan's Law to be more easily able to read and understand.
Signed-off-by Richard Guy Briggs <[email protected]>
Reviewed-by Serge Hallyn <[email protected]>
Acked-by James Morris <[email protected]>

-Kees

Quoted 26 lines

---
 security/commoncap.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

security/commoncap.c +4 -5

index cf95d73..7e8041d 100644
@@ -544,10 +544,10 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
        bool ret = false;
 
        if (__cap_grew(effective, ambient, cred) &&
-           (!__cap_full(effective, cred) ||
-            !__is_eff(root, cred) ||
-            !__is_real(root, cred) ||
-            !root_privileged()))
+           !(__cap_full(effective, cred) &&
+             __is_eff(root, cred) &&
+             __is_real(root, cred) &&
+             root_privileged()))
                ret = true;
        return ret;
 }
--

1.7.1

Signature · 2 lines

Kees Cook
Pixel Security

Richard Guy Briggs

author

2017-09-05 06:46 UTC #18 lore raw

PATCH v4 9/10 capabilities: fix logic for effective root or real root

Now that the logic is inverted, it is much easier to see that both real root and effective root conditions had to be met to avoid printing the BPRM_FCAPS record with audit syscalls. This meant that any setuid root applications would print a full BPRM_FCAPS record when it wasn't necessary, cluttering the event output, since the SYSCALL and PATH records indicated the presence of the setuid bit and effective root user id.

Require only one of effective root or real root to avoid printing the unnecessary record.

Ref: commit 3fc689e96c0c ("Add audit_log_bprm_fcaps/AUDIT_BPRM_FCAPS")
See: https://github.com/linux-audit/audit-kernel/issues/16

Signed-off-by Richard Guy Briggs <[email protected]>

---
 security/commoncap.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

security/commoncap.c +2 -3

index 7e8041d..759f3fa 100644
@@ -532,7 +532,7 @@ static inline bool __is_setgid(struct cred *new, const struct cred *old)
  *
  * We do not bother to audit if 3 things are true:
  *   1) cap_effective has all caps
- *   2) we are root
+ *   2) we became root *OR* are were already root
  *   3) root is supposed to have all caps (SECURE_NOROOT)
  * Since this is just a normal root execing a process.
  *

@@ -545,8 +545,7 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
 
 	if (__cap_grew(effective, ambient, cred) &&
 	    !(__cap_full(effective, cred) &&
-	      __is_eff(root, cred) &&
-	      __is_real(root, cred) &&
+	      (__is_eff(root, cred) || __is_real(root, cred)) &&
 	      root_privileged()))
 		ret = true;
 	return ret;

Signature · 1 line

1.7.1

Kees Cook

Acked-by on v4 9/10

2017-09-08 18:34 UTC #19 lore raw

Re: [PATCH V4 09/10] capabilities: fix logic for effective root or real root

Quoted 11 lines · Richard Guy Briggs

Now that the logic is inverted, it is much easier to see that both real root and effective root conditions had to be met to avoid printing the BPRM_FCAPS record with audit syscalls. This meant that any setuid root applications would print a full BPRM_FCAPS record when it wasn't necessary, cluttering the event output, since the SYSCALL and PATH records indicated the presence of the setuid bit and effective root user id.
Require only one of effective root or real root to avoid printing the unnecessary record.

Quoted 6 lines

Ref: commit 3fc689e96c0c ("Add audit_log_bprm_fcaps/AUDIT_BPRM_FCAPS")
See: https://github.com/linux-audit/audit-kernel/issues/16
Signed-off-by Richard Guy Briggs <[email protected]>
Reviewed-by Serge Hallyn <[email protected]>
Acked-by James Morris <[email protected]>

-Kees

Quoted 30 lines

---
 security/commoncap.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

security/commoncap.c +2 -4

index 7e8041d..759f3fa 100644
@@ -532,7 +532,7 @@ static inline bool __is_setgid(struct cred *new, const struct cred *old)
  *
  * We do not bother to audit if 3 things are true:
  *   1) cap_effective has all caps
- *   2) we are root
+ *   2) we became root *OR* are were already root
  *   3) root is supposed to have all caps (SECURE_NOROOT)
  * Since this is just a normal root execing a process.
  *

@@ -545,8 +545,7 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
 
        if (__cap_grew(effective, ambient, cred) &&
            !(__cap_full(effective, cred) &&
-             __is_eff(root, cred) &&
-             __is_real(root, cred) &&
+             (__is_eff(root, cred) || __is_real(root, cred)) &&
              root_privileged()))
                ret = true;
        return ret;
--

1.7.1

Signature · 2 lines

Kees Cook
Pixel Security

Paul Moore

maintainer Acked-by on v4 9/10

2017-09-20 22:11 UTC #20 lore raw

Re: [PATCH V4 09/10] capabilities: fix logic for effective root or real root

Quoted 14 lines · Richard Guy Briggs

Now that the logic is inverted, it is much easier to see that both real root and effective root conditions had to be met to avoid printing the BPRM_FCAPS record with audit syscalls. This meant that any setuid root applications would print a full BPRM_FCAPS record when it wasn't necessary, cluttering the event output, since the SYSCALL and PATH records indicated the presence of the setuid bit and effective root user id.
Require only one of effective root or real root to avoid printing the unnecessary record.
Ref: commit 3fc689e96c0c ("Add audit_log_bprm_fcaps/AUDIT_BPRM_FCAPS")
See: https://github.com/linux-audit/audit-kernel/issues/16

Quoted 6 lines

Signed-off-by Richard Guy Briggs <[email protected]>
Reviewed-by Serge Hallyn <[email protected]>
Acked-by James Morris <[email protected]>
---
 security/commoncap.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

Trying to sort this out, I've decided that I dislike the capabilities code as much as I dislike the audit code.

Acked-by Paul Moore <[email protected]>

Quoted 30 lines

security/commoncap.c +2 -4

index 7e8041d..759f3fa 100644
@@ -532,7 +532,7 @@ static inline bool __is_setgid(struct cred *new, const struct cred *old)
  *
  * We do not bother to audit if 3 things are true:
  *   1) cap_effective has all caps
- *   2) we are root
+ *   2) we became root *OR* are were already root
  *   3) root is supposed to have all caps (SECURE_NOROOT)
  * Since this is just a normal root execing a process.
  *

@@ -545,8 +545,7 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
 
        if (__cap_grew(effective, ambient, cred) &&
            !(__cap_full(effective, cred) &&
-             __is_eff(root, cred) &&
-             __is_real(root, cred) &&
+             (__is_eff(root, cred) || __is_real(root, cred)) &&
              root_privileged()))
                ret = true;
        return ret;
--

1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

paul moore
www.paul-moore.com

Kees Cook

2017-09-20 22:25 UTC #21 lore raw

Re: [PATCH V4 09/10] capabilities: fix logic for effective root or real root

Quoted 22 lines · Paul Moore

Quoted 14 lines · Richard Guy Briggs
Now that the logic is inverted, it is much easier to see that both real root and effective root conditions had to be met to avoid printing the BPRM_FCAPS record with audit syscalls. This meant that any setuid root applications would print a full BPRM_FCAPS record when it wasn't necessary, cluttering the event output, since the SYSCALL and PATH records indicated the presence of the setuid bit and effective root user id.
Require only one of effective root or real root to avoid printing the unnecessary record.
Ref: commit 3fc689e96c0c ("Add audit_log_bprm_fcaps/AUDIT_BPRM_FCAPS")
See: https://github.com/linux-audit/audit-kernel/issues/16
Quoted 6 lines
Signed-off-by Richard Guy Briggs <[email protected]>
Reviewed-by Serge Hallyn <[email protected]>
Acked-by James Morris <[email protected]>
---
 security/commoncap.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

Quoted 2 lines

Trying to sort this out, I've decided that I dislike the capabilities code as much as I dislike the audit code.

Read binfmt_elf.c and your journey towards the dark side will be complete!

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

Paul Moore

maintainer

2017-09-20 22:27 UTC #22 lore raw

Re: [PATCH V4 09/10] capabilities: fix logic for effective root or real root

Quoted 23 lines · Kees Cook

Quoted 22 lines · Paul Moore
Quoted 20 lines · Richard Guy Briggs
Now that the logic is inverted, it is much easier to see that both real root and effective root conditions had to be met to avoid printing the BPRM_FCAPS record with audit syscalls. This meant that any setuid root applications would print a full BPRM_FCAPS record when it wasn't necessary, cluttering the event output, since the SYSCALL and PATH records indicated the presence of the setuid bit and effective root user id.
Require only one of effective root or real root to avoid printing the unnecessary record.
Ref: commit 3fc689e96c0c ("Add audit_log_bprm_fcaps/AUDIT_BPRM_FCAPS")
See: https://github.com/linux-audit/audit-kernel/issues/16
Signed-off-by Richard Guy Briggs <[email protected]>
Reviewed-by Serge Hallyn <[email protected]>
Acked-by James Morris <[email protected]>
---
 security/commoncap.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

Quoted 4 lines

Quoted 2 lines
Trying to sort this out, I've decided that I dislike the capabilities code as much as I dislike the audit code.
Read binfmt_elf.c and your journey towards the dark side will be complete!

It's only Wednesday, I'm not sure want to inflict that much self-harm on myself by mid-week.

Signature · 2 lines

paul moore
www.paul-moore.com

Richard Guy Briggs

author

2017-09-05 06:46 UTC #23 lore raw

PATCH v4 10/10 capabilities: audit log other surprising conditions

The existing condition tested for process effective capabilities set by file attributes but intended to ignore the change if the result was unsurprisingly an effective full set in the case root is special with a setuid root executable file and we are root.

Stated again:
- When you execute a setuid root application, it is no surprise and expected that it got all capabilities, so we do not want capabilities recorded.

        if (pE_grew && !(pE_fullset && (eff_root || real_root) && root_priveleged) )

Now make sure we cover other cases:
- If something prevented a setuid root app getting all capabilities and it wound up with one capability only, then it is a surprise and should be logged. When it is a setuid root file, we only want capabilities when the process does not get full capabilities..

        root_priveleged && setuid_root && !pE_fullset

- Similarly if a non-setuid program does pick up capabilities due to file system based capabilities, then we want to know what capabilities were picked up. When it has file system based capabilities we want the capabilities.

        !is_setuid && (has_fcap && pP_gained)

- If it is a non-setuid file and it gets ambient capabilities, we want the capabilities.

        !is_setuid && pA_gained

- These last two are combined into one due to the common first parameter.

Signed-off-by Richard Guy Briggs <[email protected]>

---
 security/commoncap.c |   29 ++++++++++++++++++++++-------
 1 files changed, 22 insertions(+), 7 deletions(-)

security/commoncap.c +22 -7

index 759f3fa..afebfd3 100644
@@ -528,7 +528,7 @@ static inline bool __is_setgid(struct cred *new, const struct cred *old)
 { return !gid_eq(new->egid, old->gid); }
 
 /*
- * Audit candidate if current->cap_effective is set
+ * 1) Audit candidate if current->cap_effective is set
  *
  * We do not bother to audit if 3 things are true:
  *   1) cap_effective has all caps

@@ -538,16 +538,31 @@ static inline bool __is_setgid(struct cred *new, const struct cred *old)
  *
  * Number 1 above might fail if you don't have a full bset, but I think
  * that is interesting information to audit.
+ *
+ * A number of other conditions require logging:
+ * 2) something prevented setuid root getting all caps
+ * 3) non-setuid root gets fcaps
+ * 4) non-setuid root gets ambient
  */
-static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
+static inline bool nonroot_raised_pE(struct cred *new, const struct cred *old,
+				     kuid_t root, bool has_fcap)
 {
 	bool ret = false;
 
-	if (__cap_grew(effective, ambient, cred) &&
-	    !(__cap_full(effective, cred) &&
-	      (__is_eff(root, cred) || __is_real(root, cred)) &&
-	      root_privileged()))
+	if ((__cap_grew(effective, ambient, new) &&
+	     !(__cap_full(effective, new) &&
+	       (__is_eff(root, new) || __is_real(root, new)) &&
+	       root_privileged())) ||
+	    (root_privileged() &&
+	     __is_suid(root, new) &&
+	     !__cap_full(effective, new)) ||
+	    (!__is_setuid(new, old) &&
+	     ((has_fcap &&
+	       __cap_gained(permitted, new, old)) ||
+	      __cap_gained(ambient, new, old))))
+
 		ret = true;
+
 	return ret;
 }

@@ -628,7 +643,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 	if (WARN_ON(!cap_ambient_invariant_ok(new)))
 		return -EPERM;
 
-	if (nonroot_raised_pE(new, root_uid)) {
+	if (nonroot_raised_pE(new, old, root_uid, has_fcap)) {
 		ret = audit_log_bprm_fcaps(bprm, new, old);
 		if (ret < 0)
 			return ret;

Signature · 1 line

1.7.1

Kees Cook

Reviewed-by on v4 10/10

2017-09-08 18:36 UTC #24 lore raw

Re: [PATCH V4 10/10] capabilities: audit log other surprising conditions

Quoted 28 lines · Richard Guy Briggs

The existing condition tested for process effective capabilities set by file attributes but intended to ignore the change if the result was unsurprisingly an effective full set in the case root is special with a setuid root executable file and we are root.
Stated again:
- When you execute a setuid root application, it is no surprise and expected that it got all capabilities, so we do not want capabilities recorded.
        if (pE_grew && !(pE_fullset && (eff_root || real_root) && root_priveleged) )
Now make sure we cover other cases:
- If something prevented a setuid root app getting all capabilities and it wound up with one capability only, then it is a surprise and should be logged. When it is a setuid root file, we only want capabilities when the process does not get full capabilities..
        root_priveleged && setuid_root && !pE_fullset
- Similarly if a non-setuid program does pick up capabilities due to file system based capabilities, then we want to know what capabilities were picked up. When it has file system based capabilities we want the capabilities.
        !is_setuid && (has_fcap && pP_gained)
- If it is a non-setuid file and it gets ambient capabilities, we want the capabilities.
        !is_setuid && pA_gained

Quoted 7 lines

- These last two are combined into one due to the common first parameter.
Related: https://github.com/linux-audit/audit-kernel/issues/16
Signed-off-by Richard Guy Briggs <[email protected]>
Reviewed-by Serge Hallyn <[email protected]>
Acked-by James Morris <[email protected]>

Reviewed-by Kees Cook <[email protected]>

-Kees

Quoted 66 lines

---
 security/commoncap.c |   29 ++++++++++++++++++++++-------
 1 files changed, 22 insertions(+), 7 deletions(-)

security/commoncap.c +22 -8

index 759f3fa..afebfd3 100644
@@ -528,7 +528,7 @@ static inline bool __is_setgid(struct cred *new, const struct cred *old)
 { return !gid_eq(new->egid, old->gid); }
 
 /*
- * Audit candidate if current->cap_effective is set
+ * 1) Audit candidate if current->cap_effective is set
  *
  * We do not bother to audit if 3 things are true:
  *   1) cap_effective has all caps

@@ -538,16 +538,31 @@ static inline bool __is_setgid(struct cred *new, const struct cred *old)
  *
  * Number 1 above might fail if you don't have a full bset, but I think
  * that is interesting information to audit.
+ *
+ * A number of other conditions require logging:
+ * 2) something prevented setuid root getting all caps
+ * 3) non-setuid root gets fcaps
+ * 4) non-setuid root gets ambient
  */
-static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
+static inline bool nonroot_raised_pE(struct cred *new, const struct cred *old,
+                                    kuid_t root, bool has_fcap)
 {
        bool ret = false;
 
-       if (__cap_grew(effective, ambient, cred) &&
-           !(__cap_full(effective, cred) &&
-             (__is_eff(root, cred) || __is_real(root, cred)) &&
-             root_privileged()))
+       if ((__cap_grew(effective, ambient, new) &&
+            !(__cap_full(effective, new) &&
+              (__is_eff(root, new) || __is_real(root, new)) &&
+              root_privileged())) ||
+           (root_privileged() &&
+            __is_suid(root, new) &&
+            !__cap_full(effective, new)) ||
+           (!__is_setuid(new, old) &&
+            ((has_fcap &&
+              __cap_gained(permitted, new, old)) ||
+             __cap_gained(ambient, new, old))))
+
                ret = true;
+
        return ret;
 }

@@ -628,7 +643,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
        if (WARN_ON(!cap_ambient_invariant_ok(new)))
                return -EPERM;
 
-       if (nonroot_raised_pE(new, root_uid)) {
+       if (nonroot_raised_pE(new, old, root_uid, has_fcap)) {
                ret = audit_log_bprm_fcaps(bprm, new, old);
                if (ret < 0)
                        return ret;
--

1.7.1

Signature · 2 lines

Kees Cook
Pixel Security

Paul Moore

maintainer Acked-by on v4 10/10

2017-09-20 22:22 UTC #25 lore raw

Re: [PATCH V4 10/10] capabilities: audit log other surprising conditions

Quoted 30 lines · Richard Guy Briggs

The existing condition tested for process effective capabilities set by file attributes but intended to ignore the change if the result was unsurprisingly an effective full set in the case root is special with a setuid root executable file and we are root.
Stated again:
- When you execute a setuid root application, it is no surprise and expected that it got all capabilities, so we do not want capabilities recorded.
        if (pE_grew && !(pE_fullset && (eff_root || real_root) && root_priveleged) )
Now make sure we cover other cases:
- If something prevented a setuid root app getting all capabilities and it wound up with one capability only, then it is a surprise and should be logged. When it is a setuid root file, we only want capabilities when the process does not get full capabilities..
        root_priveleged && setuid_root && !pE_fullset
- Similarly if a non-setuid program does pick up capabilities due to file system based capabilities, then we want to know what capabilities were picked up. When it has file system based capabilities we want the capabilities.
        !is_setuid && (has_fcap && pP_gained)
- If it is a non-setuid file and it gets ambient capabilities, we want the capabilities.
        !is_setuid && pA_gained
- These last two are combined into one due to the common first parameter.

Quoted 8 lines

Related: https://github.com/linux-audit/audit-kernel/issues/16
Signed-off-by Richard Guy Briggs <[email protected]>
Reviewed-by Serge Hallyn <[email protected]>
Acked-by James Morris <[email protected]>
---
 security/commoncap.c |   29 ++++++++++++++++++++++-------
 1 files changed, 22 insertions(+), 7 deletions(-)

I really don't like how this patchset has gotten so large, I would have preferred to see a patch (or two, or three, ...) that made the minimal number of changes necessary to fix the audit problem and then a follow-on patchset to do the rework you felt was worthwhile. Reviewing this was a real pain and I have very little confidence in my conclusions, please try to not do this again.

Acked-by Paul Moore <[email protected]>

Quoted 66 lines

security/commoncap.c +22 -8

index 759f3fa..afebfd3 100644
@@ -528,7 +528,7 @@ static inline bool __is_setgid(struct cred *new, const struct cred *old)
 { return !gid_eq(new->egid, old->gid); }
 
 /*
- * Audit candidate if current->cap_effective is set
+ * 1) Audit candidate if current->cap_effective is set
  *
  * We do not bother to audit if 3 things are true:
  *   1) cap_effective has all caps

@@ -538,16 +538,31 @@ static inline bool __is_setgid(struct cred *new, const struct cred *old)
  *
  * Number 1 above might fail if you don't have a full bset, but I think
  * that is interesting information to audit.
+ *
+ * A number of other conditions require logging:
+ * 2) something prevented setuid root getting all caps
+ * 3) non-setuid root gets fcaps
+ * 4) non-setuid root gets ambient
  */
-static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
+static inline bool nonroot_raised_pE(struct cred *new, const struct cred *old,
+                                    kuid_t root, bool has_fcap)
 {
        bool ret = false;
 
-       if (__cap_grew(effective, ambient, cred) &&
-           !(__cap_full(effective, cred) &&
-             (__is_eff(root, cred) || __is_real(root, cred)) &&
-             root_privileged()))
+       if ((__cap_grew(effective, ambient, new) &&
+            !(__cap_full(effective, new) &&
+              (__is_eff(root, new) || __is_real(root, new)) &&
+              root_privileged())) ||
+           (root_privileged() &&
+            __is_suid(root, new) &&
+            !__cap_full(effective, new)) ||
+           (!__is_setuid(new, old) &&
+            ((has_fcap &&
+              __cap_gained(permitted, new, old)) ||
+             __cap_gained(ambient, new, old))))
+
                ret = true;
+
        return ret;
 }

@@ -628,7 +643,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
        if (WARN_ON(!cap_ambient_invariant_ok(new)))
                return -EPERM;
 
-       if (nonroot_raised_pE(new, root_uid)) {
+       if (nonroot_raised_pE(new, old, root_uid, has_fcap)) {
                ret = audit_log_bprm_fcaps(bprm, new, old);
                if (ret < 0)
                        return ret;
--

1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

paul moore
www.paul-moore.com

Paul Moore

maintainer

2017-09-08 17:02 UTC #26 lore raw