capabilities: do not audit log BPRM_FCAPS on set*id

This change is intended to be logic-neutral and simply make the logic easier to read in natural language and verify without getting distracted by details.

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

---
 security/commoncap.c |   53 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 34 insertions(+), 19 deletions(-)

security/commoncap.c +0 -0

index 78b3783..9520f0a 100644

security/commoncap.c +16 -11

+#define SUID !uid_eq(new->euid, old->uid) /* set uid */
+#define SGID !gid_eq(new->egid, old->gid) /* set gid */
+#define pPADD !cap_issubset(new->cap_permitted, old->cap_permitted) /* process permitted capabilities have been added */
+#define pESET !cap_issubset(new->cap_effective, new->cap_ambient) /* process effective capabilities have been set */
+#define pEALL cap_issubset(CAP_FULL_SET, new->cap_effective) /* process effective capabilities are full set */
+#define pAADD !cap_issubset(new->cap_ambient, old->cap_ambient) /* process ambient capabilities have been added */
 	if (WARN_ON(!cap_ambient_invariant_ok(old)))
 		return -EPERM;
 

@@ -507,13 +517,13 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 
 	root_uid = make_kuid(new->user_ns, 0);
 
-	if (!issecure(SECURE_NOROOT)) {
+	if (SROOT) {
 		/*
 		 * 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)) {
+		if (has_cap && SETUIDROOT) {
 			warn_setuid_and_fcaps_mixed(bprm->filename);
 			goto skip;
 		}

@@ -521,33 +531,32 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 		 * 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)) {
+		if (EROOT || RROOT) {
 			/* 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 only the real uid is root, we do not set the effective bit.
+		 */
+		if (EROOT)
 			effective = true;
 	}
 skip:
 
 	/* if we have fs caps, clear dangerous personality flags */
-	if (!cap_issubset(new->cap_permitted, old->cap_permitted))
+	if (pPADD)
 		bprm->per_clear |= PER_CLEAR_ON_SETID;
 
+	is_setid = SUID || SGID;
 
 	/* Don't let someone trace a set[ug]id/setpcap binary with the revised
 	 * credentials unless they have the appropriate permit.
 	 *
 	 * 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);
-
-	if ((is_setid ||
-	     !cap_issubset(new->cap_permitted, old->cap_permitted)) &&
+	if ((is_setid || pPADD) &&
 	    ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
 	     !ptracer_capable(current, new->user_ns))) {
 		/* downgrade; they get no more than they had, and maybe less */

@@ -599,14 +608,10 @@ 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.

security/commoncap.c +13 -0

+		ret = audit_log_bprm_fcaps(bprm, new, old);
+		if (ret < 0)
+			return ret;
 	}
 
 	new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);

@@ -615,6 +620,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 		return -EPERM;
 
 	return 0;
+#undef SROOT
+#undef RROOT
+#undef EROOT
+#undef SETUIDROOT
+#undef SUID
+#undef SGID
+#undef pPADD
+#undef pESET
+#undef pEALL
+#undef pAADD
 }
 
 /**

Signature · 1 line

1.7.1

Quoted 8 lines · Richard Guy Briggs

This change is intended to be logic-neutral and simply make the logic easier to read in natural language and verify without getting distracted by details.

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

---
 security/commoncap.c |   53 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 34 insertions(+), 19 deletions(-)

security/commoncap.c int cap_bprm_set_creds(struct linux_binprm *bprm) · line 499

Quoted 8 lines

security/commoncap.c +0 -0

index 78b3783..9520f0a 100644
@@ -497,6 +497,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 	int ret;
 	kuid_t root_uid;
 

The #defines make me uncomfortable, especially the lack of parens around them. The way they are used seems fine, but they seem like potential future maintenance issues. I definately appreciate the way you broke the functionality down, though. And I'm not sure I can improve on it.

security/commoncap.c int cap_bprm_set_creds(struct linux_binprm *bprm) · line 500

Quoted 1 line

security/commoncap.c +1 -0

+#define SROOT !issecure(SECURE_NOROOT) /* root is special */

maybe

static inline bool root_privileged() { return !issecure(SECURE_NOROOT); }

security/commoncap.c int cap_bprm_set_creds(struct linux_binprm *bprm) · line 503

Quoted 3 lines

security/commoncap.c +3 -0

+#define RROOT uid_eq(new->uid, root_uid) /* real root */
+#define EROOT uid_eq(new->euid, root_uid) /* effective root */
+#define SETUIDROOT !RROOT && EROOT /* set uid root */

Yeah every time I start typing an alternative it doesn't look as good.

Quoted 70 lines

security/commoncap.c +16 -11

+#define SUID !uid_eq(new->euid, old->uid) /* set uid */
+#define SGID !gid_eq(new->egid, old->gid) /* set gid */
+#define pPADD !cap_issubset(new->cap_permitted, old->cap_permitted) /* process permitted capabilities have been added */
+#define pESET !cap_issubset(new->cap_effective, new->cap_ambient) /* process effective capabilities have been set */
+#define pEALL cap_issubset(CAP_FULL_SET, new->cap_effective) /* process effective capabilities are full set */
+#define pAADD !cap_issubset(new->cap_ambient, old->cap_ambient) /* process ambient capabilities have been added */
 	if (WARN_ON(!cap_ambient_invariant_ok(old)))
 		return -EPERM;
 

@@ -507,13 +517,13 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 
 	root_uid = make_kuid(new->user_ns, 0);
 
-	if (!issecure(SECURE_NOROOT)) {
+	if (SROOT) {
 		/*
 		 * 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)) {
+		if (has_cap && SETUIDROOT) {
 			warn_setuid_and_fcaps_mixed(bprm->filename);
 			goto skip;
 		}

@@ -521,33 +531,32 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 		 * 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)) {
+		if (EROOT || RROOT) {
 			/* 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 only the real uid is root, we do not set the effective bit.
+		 */
+		if (EROOT)
 			effective = true;
 	}
 skip:
 
 	/* if we have fs caps, clear dangerous personality flags */
-	if (!cap_issubset(new->cap_permitted, old->cap_permitted))
+	if (pPADD)
 		bprm->per_clear |= PER_CLEAR_ON_SETID;
 
+	is_setid = SUID || SGID;
 
 	/* Don't let someone trace a set[ug]id/setpcap binary with the revised
 	 * credentials unless they have the appropriate permit.
 	 *
 	 * 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);
-
-	if ((is_setid ||
-	     !cap_issubset(new->cap_permitted, old->cap_permitted)) &&
+	if ((is_setid || pPADD) &&
 	    ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
 	     !ptracer_capable(current, new->user_ns))) {
 		/* downgrade; they get no more than they had, and maybe less */

@@ -599,14 +608,10 @@ 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.

security/commoncap.c int cap_bprm_set_creds(struct linux_binprm *bprm) · line 611

Quoted 10 lines

security/commoncap.c +1 -8

 	 */
-	if (!cap_issubset(new->cap_effective, new->cap_ambient)) {
-		if (!cap_issubset(CAP_FULL_SET, new->cap_effective) ||
-		    !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) ||
-		    issecure(SECURE_NOROOT)) {
-			ret = audit_log_bprm_fcaps(bprm, new, old);
-			if (ret < 0)
-				return ret;
-		}
+	if (pESET && (!pEALL || !EROOT || !RROOT || !SROOT) ) {

This might be better served by a separate helper

	if (nonroot_raised_e(new, root_uid)) {
		ret = audit_log_bprm_fcaps(bprm, new, old);
		if (ret < 0)
			return ret;
	}

Quoted 22 lines

security/commoncap.c +13 -0

+		ret = audit_log_bprm_fcaps(bprm, new, old);
+		if (ret < 0)
+			return ret;
 	}
 
 	new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);

@@ -615,6 +620,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 		return -EPERM;
 
 	return 0;
+#undef SROOT
+#undef RROOT
+#undef EROOT
+#undef SETUIDROOT
+#undef SUID
+#undef SGID
+#undef pPADD
+#undef pESET
+#undef pEALL
+#undef pAADD
 }
 

Quoted 3 lines

security/commoncap.c +0 -0

 /**

Signature · 1 line

1.7.1

This change is intended to be logic-neutral and simply make the logic easier to read in natural language and verify without getting distracted by details.

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

---
 security/commoncap.c |   53 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 34 insertions(+), 19 deletions(-)

security/commoncap.c +2 -0

index 78b3783..9520f0a 100644
@@ -497,6 +497,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 	int ret;
 	kuid_t root_uid;
 
+#define SROOT !issecure(SECURE_NOROOT) /* root is special */
+#define RROOT uid_eq(new->uid, root_uid) /* real root */

security/commoncap.c +0 -0

 	    ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
 	     !ptracer_capable(current, new->user_ns))) {
 		/* downgrade; they get no more than they had, and maybe less */

@@ -599,14 +608,10 @@ 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.

security/commoncap.c +3 -0

+		ret = audit_log_bprm_fcaps(bprm, new, old);
+		if (ret < 0)
+			return ret;
 	}
 
 	new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);

@@ -615,6 +620,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 		return -EPERM;
 
 	return 0;

The audit subsystem is adding a BPRM_FCAPS record when auditing setuid application execution (SYSCALL execve). This is not expected as it was supposed to be limited to when the file system actually had capabilities in an extended attribute. It lists all capabilities making the event really ugly to parse what is happening. The PATH record correctly records the setuid bit and owner. Suppress the BPRM_FCAPS record on set*id.

See: https://github.com/linux-audit/audit-kernel/issues/16

The patch that resolves this issue is the third. The first and second just massage the logic to make it easier to understand.

It would be possible to address the original issue with a change of

        "!uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid)"
to
        "!(uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid))"

but it took me long enough to understand this logic that I don't think I'd be doing any favours by leaving it this difficult to understand.

The final patch attempts to address all the conditions that need logging based on mailing list conversations, recoginizing there is probably some duplication in the logic, which is why I'm posting this as an RFC for some feedback.

Richard Guy Briggs (4):
capabilities: use macros to make the logic easier to follow and
verify
capabilities: invert logic for clarity
capabilities: fix logic for effective root or real root
capabilities: auit log other surprising conditions

 security/commoncap.c |   55 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 36 insertions(+), 19 deletions(-)

Quoted 31 lines · Richard Guy Briggs

The audit subsystem is adding a BPRM_FCAPS record when auditing setuid application execution (SYSCALL execve). This is not expected as it was supposed to be limited to when the file system actually had capabilities in an extended attribute. It lists all capabilities making the event really ugly to parse what is happening. The PATH record correctly records the setuid bit and owner. Suppress the BPRM_FCAPS record on set*id.

See: https://github.com/linux-audit/audit-kernel/issues/16

The patch that resolves this issue is the third. The first and second just massage the logic to make it easier to understand.

It would be possible to address the original issue with a change of

        "!uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid)"
to
        "!(uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid))"

but it took me long enough to understand this logic that I don't think I'd be doing any favours by leaving it this difficult to understand.

The final patch attempts to address all the conditions that need logging based on mailing list conversations, recoginizing there is probably some duplication in the logic, which is why I'm posting this as an RFC for some feedback.

Richard Guy Briggs (4):
capabilities: use macros to make the logic easier to follow and
verify
capabilities: invert logic for clarity
capabilities: fix logic for effective root or real root
capabilities: auit log other surprising conditions

Quoted 2 lines

 security/commoncap.c |   55 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 36 insertions(+), 19 deletions(-)

Following up on this set of patches ... I see there was some discussion between you and Serge for one of the patches, but it isn't clear to me that there was any resolution reached; where do things stand at the moment?

paul moore

Quoted 35 lines · Paul Moore

Quoted 33 lines · Richard Guy Briggs

The audit subsystem is adding a BPRM_FCAPS record when auditing setuid application execution (SYSCALL execve). This is not expected as it was supposed to be limited to when the file system actually had capabilities in an extended attribute. It lists all capabilities making the event really ugly to parse what is happening. The PATH record correctly records the setuid bit and owner. Suppress the BPRM_FCAPS record on set*id.

See: https://github.com/linux-audit/audit-kernel/issues/16

The patch that resolves this issue is the third. The first and second just massage the logic to make it easier to understand.

It would be possible to address the original issue with a change of

        "!uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid)"
to
        "!(uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid))"

but it took me long enough to understand this logic that I don't think I'd be doing any favours by leaving it this difficult to understand.

The final patch attempts to address all the conditions that need logging based on mailing list conversations, recoginizing there is probably some duplication in the logic, which is why I'm posting this as an RFC for some feedback.

Richard Guy Briggs (4):
capabilities: use macros to make the logic easier to follow and
verify
capabilities: invert logic for clarity
capabilities: fix logic for effective root or real root
capabilities: auit log other surprising conditions

 security/commoncap.c |   55 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 36 insertions(+), 19 deletions(-)

Quoted 4 lines

Following up on this set of patches ... I see there was some discussion between you and Serge for one of the patches, but it isn't clear to me that there was any resolution reached; where do things stand at the moment?

Well, I was still waiting to hear from you before submitting another round of patches.

I'd like to replace the macros with local variables (rather than funcitons), expecting that the compiler would be smarter than me and figure out the best way to do things.