capabilities: do not audit log BPRM_FCAPS on set*id

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]>

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

security/commoncap.c +34 -28

index 78b3783..b7fbf77 100644
@@ -481,6 +481,38 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
 	return rc;
 }
 
+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

@@ -493,46 +525,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

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]>

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

security/commoncap.c +0 -0

index 78b3783..b7fbf77 100644

Quoted 17 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]>

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

security/commoncap.c +0 -0

index 78b3783..b7fbf77 100644
@@ -481,6 +481,38 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
 	return rc;
 }
 

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

Quoted 1 line

security/commoncap.c +1 -0

+void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effective, kuid_t root_uid)

Can this be static?

James Morris

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 |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

security/commoncap.c +0 -0

index b7fbf77..6f05ec0 100644

security/commoncap.c +10 -6

+#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

@@ -541,10 +547,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.
 	 *

@@ -552,8 +557,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 */

@@ -605,8 +609,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);

Signature · 1 line

1.7.1

Quoted 8 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]>

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

security/commoncap.c void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec · line 515

Quoted 8 lines

security/commoncap.c +0 -0

index b7fbf77..6f05ec0 100644
@@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec
 		*effective = true;
 }
 

It's subjective and so might be just me, but I think I'd find it easier to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target)

This looks correct though, so either way

Reviewed-by Serge Hallyn <[email protected]>

Quoted 36 lines

security/commoncap.c +8 -5

+#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

@@ -541,10 +547,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.
 	 *

@@ -552,8 +557,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 */

@@ -605,8 +609,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)) {

Quoted 8 lines

security/commoncap.c +2 -1

-		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);

Signature · 1 line

1.7.1

Quoted 9 lines · Serge E. Hallyn

Quoted 8 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]>

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

Quoted 11 lines

security/commoncap.c void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec · line 515

Quoted 8 lines

security/commoncap.c +0 -0

index b7fbf77..6f05ec0 100644
@@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec
 		*effective = true;
 }
 

It's subjective and so might be just me, but I think I'd find it easier to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target)

In more than one place, I wanted to put the parameter that I was trying to read aloud closest to the function name to make reading it flow better, leaving the parameters less critical to comprehension towards the end.

Quoted 3 lines

This looks correct though, so either way

Reviewed-by Serge Hallyn <[email protected]>

Thanks. Did you want to put this through, or send it through Paul's audit tree?

Quoted 22 lines · Richard Guy Briggs

Quoted 9 lines · Serge E. Hallyn

Quoted 8 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]>

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

Quoted 11 lines

security/commoncap.c void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec · line 515

Quoted 8 lines

security/commoncap.c +0 -0

index b7fbf77..6f05ec0 100644
@@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec
          *effective = true;
 }
 

It's subjective and so might be just me, but I think I'd find it easier to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target)

Quoted 4 lines

In more than one place, I wanted to put the parameter that I was trying to read aloud closest to the function name to make reading it flow better, leaving the parameters less critical to comprehension towards the end.

And I see that in the final patch it looks nicer the way you have it.

Quoted 6 lines

Quoted 3 lines

This looks correct though, so either way

Reviewed-by Serge Hallyn <[email protected]>

Thanks. Did you want to put this through, or send it through Paul's audit tree?

If Paul's around I'm happy to have it go through his tree.

Quoted 30 lines · Serge E. Hallyn

Quoted 22 lines · Richard Guy Briggs

Quoted 9 lines · Serge E. Hallyn

Quoted 8 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]>

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

Quoted 11 lines

security/commoncap.c void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec · line 515

Quoted 8 lines

security/commoncap.c +0 -0

index b7fbf77..6f05ec0 100644
@@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec
          *effective = true;
 }
 

It's subjective and so might be just me, but I think I'd find it easier to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target)

Quoted 4 lines

In more than one place, I wanted to put the parameter that I was trying to read aloud closest to the function name to make reading it flow better, leaving the parameters less critical to comprehension towards the end.

And I see that in the final patch it looks nicer the way you have it.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

This looks correct though, so either way

Reviewed-by Serge Hallyn <[email protected]>

Thanks. Did you want to put this through, or send it through Paul's audit tree?

If Paul's around I'm happy to have it go through his tree.

Is this series based against -next with the changes that touch commoncap.c?

Also, did you validate this with the existing LTP tests and selftests?

https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/secureexec&id=ee67ae7ef6ff499137292ac8a9dfe86096796283

Quoted 32 lines · Serge E. Hallyn

Quoted 22 lines · Richard Guy Briggs

Quoted 9 lines · Serge E. Hallyn

Quoted 8 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]>

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

Quoted 11 lines

security/commoncap.c void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec · line 515

Quoted 8 lines

security/commoncap.c +0 -0

index b7fbf77..6f05ec0 100644
@@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec
          *effective = true;
 }
 

It's subjective and so might be just me, but I think I'd find it easier to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target)

Quoted 4 lines

In more than one place, I wanted to put the parameter that I was trying to read aloud closest to the function name to make reading it flow better, leaving the parameters less critical to comprehension towards the end.

And I see that in the final patch it looks nicer the way you have it.

Quoted 2 lines

Quoted 2 lines

This looks correct though, so either way

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Reviewed-by Serge Hallyn <[email protected]>

Thanks. Did you want to put this through, or send it through Paul's audit tree?

If Paul's around I'm happy to have it go through his tree.

Is this series based against -next with the changes that touch commoncap.c?

Also, did you validate this with the existing LTP tests and selftests?

https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/secureexec&id=ee67ae7ef6ff499137292ac8a9dfe86096796283

Kees Cook

Quoted 33 lines · Kees Cook

Quoted 32 lines · Serge E. Hallyn

Quoted 22 lines · Richard Guy Briggs

Quoted 9 lines · Serge E. Hallyn

Quoted 8 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]>

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

Quoted 11 lines

security/commoncap.c void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec · line 515

Quoted 8 lines

security/commoncap.c +0 -0

index b7fbf77..6f05ec0 100644
@@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec
          *effective = true;
 }
 

It's subjective and so might be just me, but I think I'd find it easier to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target)

Quoted 4 lines

In more than one place, I wanted to put the parameter that I was trying to read aloud closest to the function name to make reading it flow better, leaving the parameters less critical to comprehension towards the end.

And I see that in the final patch it looks nicer the way you have it.

Quoted 2 lines

Quoted 2 lines

This looks correct though, so either way

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Reviewed-by Serge Hallyn <[email protected]>

Thanks. Did you want to put this through, or send it through Paul's audit tree?

If Paul's around I'm happy to have it go through his tree.

Is this series based against -next with the changes that touch commoncap.c?

This series is against pcmoore's audit/next tree (I know I'm missing two commits but they pose no conflict.).

Which -next tree are you talking about? I might guess linux-security/next or linux-next/master (I have at least a dozen "next" in my git repo config.)

I did eventually find your patches in sfr's tree and in your for-next/kspp branch.

I'll have a look at the commoncap.c changes including the elimination of cap_effective.

Quoted 3 lines

Also, did you validate this with the existing LTP tests and selftests?

https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/secureexec&id=ee67ae7ef6ff499137292ac8a9dfe86096796283

No. I will look into doing that. Thanks for the suggestion.

I see that bprm->cap_effective has vanished, so that will affect at least one hunk.

Quoted 1 line

Kees Cook

- RGB

Quoted 52 lines · Richard Guy Briggs

Quoted 33 lines · Kees Cook

Quoted 32 lines · Serge E. Hallyn

Quoted 22 lines · Richard Guy Briggs

Quoted 9 lines · Serge E. Hallyn

Quoted 8 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]>

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

Quoted 11 lines

security/commoncap.c void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec · line 515

Quoted 8 lines

security/commoncap.c +0 -0

index b7fbf77..6f05ec0 100644
@@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec
          *effective = true;
 }
 

It's subjective and so might be just me, but I think I'd find it easier to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target)

Quoted 4 lines

In more than one place, I wanted to put the parameter that I was trying to read aloud closest to the function name to make reading it flow better, leaving the parameters less critical to comprehension towards the end.

And I see that in the final patch it looks nicer the way you have it.

Quoted 2 lines

Quoted 2 lines

This looks correct though, so either way

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Reviewed-by Serge Hallyn <[email protected]>

Thanks. Did you want to put this through, or send it through Paul's audit tree?

If Paul's around I'm happy to have it go through his tree.

Is this series based against -next with the changes that touch commoncap.c?

This series is against pcmoore's audit/next tree (I know I'm missing two commits but they pose no conflict.).

Which -next tree are you talking about? I might guess linux-security/next or linux-next/master (I have at least a dozen "next" in my git repo config.)

I did eventually find your patches in sfr's tree and in your for-next/kspp branch.

Quoted 7 lines

I'll have a look at the commoncap.c changes including the elimination of cap_effective.

Quoted 3 lines

Also, did you validate this with the existing LTP tests and selftests?

https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/secureexec&id=ee67ae7ef6ff499137292ac8a9dfe86096796283

No. I will look into doing that. Thanks for the suggestion.

Quoted 1 line

I see that bprm->cap_effective has vanished, so that will affect at least one hunk.

And I spoke too soon and didn't notice this was in the bprm struct and not the cred struct, so I think were're fine there. I'll go ahead and rebase on your commoncap.c changes as well as run the ltp and kernel self-test validation tests.

Quoted 3 lines

Quoted 1 line

Kees Cook

- RGB

- RGB

Quoted 53 lines · Richard Guy Briggs

Quoted 52 lines · Richard Guy Briggs

Quoted 33 lines · Kees Cook

Quoted 32 lines · Serge E. Hallyn

Quoted 22 lines · Richard Guy Briggs

Quoted 9 lines · Serge E. Hallyn

Quoted 8 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]>

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

Quoted 11 lines

security/commoncap.c void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec · line 515

Quoted 8 lines

security/commoncap.c +0 -0

index b7fbf77..6f05ec0 100644
@@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec
          *effective = true;
 }
 

It's subjective and so might be just me, but I think I'd find it easier to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target)

Quoted 4 lines

In more than one place, I wanted to put the parameter that I was trying to read aloud closest to the function name to make reading it flow better, leaving the parameters less critical to comprehension towards the end.

And I see that in the final patch it looks nicer the way you have it.

Quoted 2 lines

Quoted 2 lines

This looks correct though, so either way

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Reviewed-by Serge Hallyn <[email protected]>

Thanks. Did you want to put this through, or send it through Paul's audit tree?

If Paul's around I'm happy to have it go through his tree.

Is this series based against -next with the changes that touch commoncap.c?

This series is against pcmoore's audit/next tree (I know I'm missing two commits but they pose no conflict.).

Which -next tree are you talking about? I might guess linux-security/next or linux-next/master (I have at least a dozen "next" in my git repo config.)

I did eventually find your patches in sfr's tree and in your for-next/kspp branch.

Quoted 7 lines

Quoted 7 lines

I'll have a look at the commoncap.c changes including the elimination of cap_effective.

Quoted 3 lines

Also, did you validate this with the existing LTP tests and selftests?

https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/secureexec&id=ee67ae7ef6ff499137292ac8a9dfe86096796283

No. I will look into doing that. Thanks for the suggestion.

Ok, I'm running the kernel self-test

make TARGETS="capabilities" kselftest

and getting a good way through it and then hit this on an unmodified kernel:

	[RUN]   +++ Tests with uid != 0 +++
	[NOTE]  Using global UIDs for tests
	[OK]    Child succeeded

test_execve: chdir to private tmpfs: Permission denied

[FAIL] Child failed
selftests: test_execve [FAIL]

Is this a known limitation or have I got something weird in my runtime environment that is killing it at this part of the test?

Quoted 68 lines · Richard Guy Briggs

Quoted 53 lines · Richard Guy Briggs

Quoted 52 lines · Richard Guy Briggs

Quoted 33 lines · Kees Cook

Quoted 32 lines · Serge E. Hallyn

Quoted 22 lines · Richard Guy Briggs

Quoted 9 lines · Serge E. Hallyn

Quoted 8 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]>

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

Quoted 11 lines

security/commoncap.c void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec · line 515

Quoted 8 lines

security/commoncap.c +0 -0

index b7fbf77..6f05ec0 100644
@@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec
          *effective = true;
 }
 

It's subjective and so might be just me, but I think I'd find it easier to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target)

Quoted 4 lines

In more than one place, I wanted to put the parameter that I was trying to read aloud closest to the function name to make reading it flow better, leaving the parameters less critical to comprehension towards the end.

And I see that in the final patch it looks nicer the way you have it.

Quoted 2 lines

Quoted 2 lines

This looks correct though, so either way

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Reviewed-by Serge Hallyn <[email protected]>

Thanks. Did you want to put this through, or send it through Paul's audit tree?

If Paul's around I'm happy to have it go through his tree.

Is this series based against -next with the changes that touch commoncap.c?

This series is against pcmoore's audit/next tree (I know I'm missing two commits but they pose no conflict.).

Which -next tree are you talking about? I might guess linux-security/next or linux-next/master (I have at least a dozen "next" in my git repo config.)

I did eventually find your patches in sfr's tree and in your for-next/kspp branch.

Quoted 7 lines

Quoted 7 lines

I'll have a look at the commoncap.c changes including the elimination of cap_effective.

Quoted 3 lines

Also, did you validate this with the existing LTP tests and selftests?

https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/secureexec&id=ee67ae7ef6ff499137292ac8a9dfe86096796283

No. I will look into doing that. Thanks for the suggestion.

Ok, I'm running the kernel self-test

make TARGETS="capabilities" kselftest

and getting a good way through it and then hit this on an unmodified kernel:

Quoted 4 lines

	[RUN]   +++ Tests with uid != 0 +++
	[NOTE]  Using global UIDs for tests
	[OK]    Child succeeded

test_execve: chdir to private tmpfs: Permission denied

Hm, just a hunch, anything in syslog? The fact that you can mount the private tmp but not chdir to it just sounds like selinux contexts.

Might run it under strace...

Boy, that's an interesting testcase.

Quoted 5 lines

[FAIL] Child failed
selftests: test_execve [FAIL]

Is this a known limitation or have I got something weird in my runtime environment that is killing it at this part of the test?

Quoted 69 lines · Serge E. Hallyn

Quoted 68 lines · Richard Guy Briggs

Quoted 53 lines · Richard Guy Briggs

Quoted 52 lines · Richard Guy Briggs

Quoted 33 lines · Kees Cook

Quoted 32 lines · Serge E. Hallyn

Quoted 22 lines · Richard Guy Briggs

Quoted 9 lines · Serge E. Hallyn

Quoted 8 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]>

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

Quoted 11 lines

security/commoncap.c void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec · line 515

Quoted 8 lines

security/commoncap.c +0 -0

index b7fbf77..6f05ec0 100644
@@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec
          *effective = true;
 }
 

It's subjective and so might be just me, but I think I'd find it easier to read if it was cap_gained(source, target, field) and cap_grew(cred, source, target)

Quoted 4 lines

In more than one place, I wanted to put the parameter that I was trying to read aloud closest to the function name to make reading it flow better, leaving the parameters less critical to comprehension towards the end.

And I see that in the final patch it looks nicer the way you have it.

Quoted 2 lines

Quoted 2 lines

This looks correct though, so either way

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Reviewed-by Serge Hallyn <[email protected]>

Thanks. Did you want to put this through, or send it through Paul's audit tree?

If Paul's around I'm happy to have it go through his tree.

Is this series based against -next with the changes that touch commoncap.c?

This series is against pcmoore's audit/next tree (I know I'm missing two commits but they pose no conflict.).

Which -next tree are you talking about? I might guess linux-security/next or linux-next/master (I have at least a dozen "next" in my git repo config.)

I did eventually find your patches in sfr's tree and in your for-next/kspp branch.

Quoted 7 lines

Quoted 7 lines

I'll have a look at the commoncap.c changes including the elimination of cap_effective.

Quoted 3 lines

Also, did you validate this with the existing LTP tests and selftests?

https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=for-next/secureexec&id=ee67ae7ef6ff499137292ac8a9dfe86096796283

No. I will look into doing that. Thanks for the suggestion.

Ok, I'm running the kernel self-test

make TARGETS="capabilities" kselftest

and getting a good way through it and then hit this on an unmodified kernel:

Quoted 7 lines

Quoted 4 lines

	[RUN]   +++ Tests with uid != 0 +++
	[NOTE]  Using global UIDs for tests
	[OK]    Child succeeded

test_execve: chdir to private tmpfs: Permission denied

Hm, just a hunch, anything in syslog? The fact that you can mount the private tmp but not chdir to it just sounds like selinux contexts.

Nothing in journalctl -b, nothing in /var/log

Quoted 1 line

Might run it under strace...

strace reports: chdir("/root/rgb/git/linux-2.6/tools/testing/selftests/capabilities") = -1 EACCES (Permission denied)

An audit rule only reports the previous success.

Problem looks to be the nfs mount on which the test is run. Running it locally works fine.

Quoted 7 lines

Boy, that's an interesting testcase.

Quoted 5 lines

[FAIL] Child failed
selftests: test_execve [FAIL]

Is this a known limitation or have I got something weird in my runtime environment that is killing it at this part of the test?

Ok, cleared that up...

Next I'm trying to run the ltp and it hangs on several tests and eventually trashes my nfs mount and can't make further progress. Kees, what version of the LTP do you run and what subset of tests do you suggest to validate things? I'm running the upstream clone git.

- RGB

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 |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

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 |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

security/commoncap.c +0 -0

index b7fbf77..6f05ec0 100644

Quoted 16 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]>

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

security/commoncap.c +0 -0

index b7fbf77..6f05ec0 100644
@@ -513,6 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec
                *effective = true;
 }
 

security/commoncap.c void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec · line 521

Quoted 6 lines

security/commoncap.c +6 -0

+#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)

I read this several times and I can't figure out what "gain" means, what "grew" means, and why it's more intuitive that they signify weird

permutations of the parameters. I also don't know what "source" and "target" mean in this context.

Can you clearly articulate how this is better than the status quo?

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 6f05ec0..028d4e4 100644
@@ -339,7 +339,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;

@@ -349,7 +349,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];

@@ -438,7 +438,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;

@@ -469,7 +469,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);

@@ -481,7 +481,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
 	return rc;
 }
 
-void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effective, kuid_t root_uid)
+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;

@@ -493,7 +493,7 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_cap, bool *effec
 	 * 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;
 	}

@@ -531,20 +531,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))

@@ -574,7 +574,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

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(-)

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]>

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

security/commoncap.c +5 -4

index 028d4e4..36c38a1 100644
@@ -481,13 +481,13 @@ 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); }
+
 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

@@ -544,7 +544,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))

@@ -612,7 +613,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

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]>

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

Quoted 3 lines · Richard Guy Briggs

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

Quoted 5 lines

Suggested-by Serge Hallyn <[email protected]>

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

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

Acked-by James Morris <[email protected]>

James Morris

Quoted 11 lines · James Morris

Quoted 8 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]>

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

Quoted 1 line

Acked-by James Morris <[email protected]>

Does anyone have the appetite to move this helper function to include/linux/securebits.h along with issecure() to make it more widely available?

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 |   26 +++++++++++++++++++++-----
 1 files changed, 21 insertions(+), 5 deletions(-)

security/commoncap.c +21 -5

index 36c38a1..1af7dec 100644
@@ -483,6 +483,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); }
+
 void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effective, kuid_t root_uid)
 {
 	const struct cred *old = current_cred();

@@ -493,7 +502,7 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe
 	 * 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;
 	}

@@ -504,12 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe
 	 *
 	 * 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;
 }
 

@@ -519,6 +528,13 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe
 	!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

@@ -556,7 +572,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) ||

@@ -612,7 +628,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)

Signature · 1 line

1.7.1

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 |   26 +++++++++++++++++++++-----
 1 files changed, 21 insertions(+), 5 deletions(-)

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 |   26 +++++++++++++++++++++-----
 1 files changed, 21 insertions(+), 5 deletions(-)

security/commoncap.c +0 -0

index 36c38a1..1af7dec 100644

Quoted 14 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]>

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

security/commoncap.c +0 -0

index 36c38a1..1af7dec 100644
@@ -483,6 +483,15 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f
 

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

Quoted 4 lines

security/commoncap.c +2 -0

 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); }

OK I guess, but this just seems like a way to obfuscate the code a bit and save typing "->uid".

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

Quoted 3 lines

security/commoncap.c +3 -0

+
+static inline bool is_eff(kuid_t uid, struct cred *cred)
+{ return uid_eq(cred->euid, uid); }

Ditto.

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

Quoted 3 lines

security/commoncap.c +3 -0

+
+static inline bool is_suid(kuid_t uid, struct cred *cred)
+{ return !is_real(uid, cred) && is_eff(uid, cred); }

Please no. This is IMO insane. You're hiding really weird, nonintuitive logic in an oddly named helper.

Also, this is going to cause massive confusion and severe bugs: given the same, the only remotely sensible guess as to what this function does is uid_eq(cred->suid, uid). So NAK to this.

security/commoncap.c void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe · line 505

Quoted 10 lines

security/commoncap.c +2 -1

+
 void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effective, kuid_t root_uid)
 {
        const struct cred *old = current_cred();

@@ -493,7 +502,7 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe
         * 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)) {

e.g. this. The logic used to be obviously slightly dicey. Now it looks sane but doesn't do what you'd naively expect it to do, which is far worse.

Quoted 15 lines · Andy Lutomirski

Quoted 15 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]>

---
security/commoncap.c | 26 +++++++++++++++++++++-----

1 files changed, 21 insertions(+), 5 deletions(-)

security/commoncap.c +0 -0

index 36c38a1..1af7dec 100644
@@ -483,6 +483,15 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f
 

Quoted 7 lines

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

Quoted 4 lines

security/commoncap.c +2 -0

 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); }

OK I guess, but this just seems like a way to obfuscate the code a bit and save typing "->uid".

Personally I find the new to be far more readable. In the old, the distinction between uid and euid is one character hidden in the middle of the expression.

Quoted 7 lines

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

Quoted 4 lines

security/commoncap.c +3 -0

+
+static inline bool is_eff(kuid_t uid, struct cred *cred)
+{ return uid_eq(cred->euid, uid); }

Ditto.

Quoted 6 lines

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

Quoted 3 lines

security/commoncap.c +3 -0

+
+static inline bool is_suid(kuid_t uid, struct cred *cred)
+{ return !is_real(uid, cred) && is_eff(uid, cred); }

Please no. This is IMO insane. You're hiding really weird, nonintuitive logic in an oddly named helper.

How is it nonintuitive? It's very precisely checking that a nonroot user is executing something that results in euid=0.

That's what it's checking for, the name of the new helper makes that clear, and the code becomes clearer because we only see the thing we care about checking for rather than the intent being hidden.

Quoted 4 lines

Also, this is going to cause massive confusion and severe bugs: given the same, the only remotely sensible guess as to what this function does is uid_eq(cred->suid, uid). So NAK to this.

Quoted 14 lines

security/commoncap.c void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe · line 505

Quoted 10 lines

security/commoncap.c +2 -1

+
 void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effective, kuid_t root_uid)
 {
       const struct cred *old = current_cred();

@@ -493,7 +502,7 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe
        * 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)) {

e.g. this. The logic used to be obviously slightly dicey. Now it looks sane but doesn't do what you'd naively expect it to do, which is far worse.

In what way does not do what you'd expect?

--Andy

Quoted 26 lines · Serge E. Hallyn

Quoted 22 lines · Andy Lutomirski

Quoted 14 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]>

---
security/commoncap.c | 26 +++++++++++++++++++++-----

1 files changed, 21 insertions(+), 5 deletions(-)

security/commoncap.c +0 -0

index 36c38a1..1af7dec 100644

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

Quoted 5 lines

security/commoncap.c +2 -0

@@ -483,6 +483,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); }

OK I guess, but this just seems like a way to obfuscate the code a bit and save typing "->uid".

Quoted 3 lines

Personally I find the new to be far more readable. In the old, the distinction between uid and euid is one character hidden in the middle of the expression.

Would real_uid_eq be better?

Quoted 12 lines

Quoted 11 lines

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

Quoted 2 lines

security/commoncap.c +2 -0

+static inline bool is_eff(kuid_t uid, struct cred *cred)
+{ return uid_eq(cred->euid, uid); }

Ditto.

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

Quoted 3 lines

security/commoncap.c +3 -0

+
+static inline bool is_suid(kuid_t uid, struct cred *cred)
+{ return !is_real(uid, cred) && is_eff(uid, cred); }

Please no. This is IMO insane. You're hiding really weird, nonintuitive logic in an oddly named helper.

Quoted 2 lines

How is it nonintuitive? It's very precisely checking that a nonroot user is executing something that results in euid=0.

I can think of several sensible predicated:

1. Are we execing a setuid-root program, where the setuod bit wasn't suppressed by nnp, mount options, trace, etc?

2. Same as 1, but also require that we weren't root.

3. Is the new euid 0 and old uid != 0?

4. Does suid == 0?

This helper checks something equivalent to 3, but only once were far enough through exec and before user code starts. This is probably equivalent to 2 as well. This is quite subtle and deserves an open-coded check, a much more carefully named helper, or, better yet, something that looks at binprm instead of cred.

is_suid sounds like #4.

Quoted 11 lines

Quoted 10 lines

security/commoncap.c void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe · line 505

Quoted 6 lines

security/commoncap.c +1 -1

@@ -493,7 +502,7 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe
        * 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)) {

e.g. this. The logic used to be obviously slightly dicey. Now it looks sane but doesn't do what you'd naively expect it to do, which is far worse.

Quoted 1 line

In what way does not do what you'd expect?

It doesn't look at cred->suid.

Part of the motivation here is that the things we are checking for are some rather baroque combinations of conditions, so having each piece of those be as simple and clear as possible helps to better reason about what is going on (which helped Richard to find the bug he is fixing).

These helpers are local (should all be static, as James pointed out). Making helpers to simplify the final checks is the right way to clarify code. I'm all for making sure they are as clear as possible, but I do think their existence is justified.

Quoted 28 lines · Andy Lutomirski

--Andy

Quoted 26 lines · Serge E. Hallyn

Quoted 22 lines · Andy Lutomirski

Quoted 14 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]>

---
security/commoncap.c | 26 +++++++++++++++++++++-----

1 files changed, 21 insertions(+), 5 deletions(-)

security/commoncap.c +0 -0

index 36c38a1..1af7dec 100644

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

Quoted 5 lines

security/commoncap.c +2 -0

@@ -483,6 +483,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); }

OK I guess, but this just seems like a way to obfuscate the code a bit and save typing "->uid".

Quoted 5 lines

Quoted 3 lines

Personally I find the new to be far more readable. In the old, the distinction between uid and euid is one character hidden in the middle of the expression.

Would real_uid_eq be better?

Replacing is_real() with real_uid_eq() would be good. I still think that a is_setuid() is worthwhile.

Quoted 19 lines

Quoted 12 lines

Quoted 11 lines

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

Quoted 2 lines

security/commoncap.c +2 -0

+static inline bool is_eff(kuid_t uid, struct cred *cred)
+{ return uid_eq(cred->euid, uid); }

Ditto.

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

Quoted 3 lines

security/commoncap.c +3 -0

+
+static inline bool is_suid(kuid_t uid, struct cred *cred)
+{ return !is_real(uid, cred) && is_eff(uid, cred); }

Please no. This is IMO insane. You're hiding really weird, nonintuitive logic in an oddly named helper.

Quoted 2 lines

How is it nonintuitive? It's very precisely checking that a nonroot user is executing something that results in euid=0.

I can think of several sensible predicated:

1. Are we execing a setuid-root program, where the setuod bit wasn't suppressed by nnp, mount options, trace, etc?

Quoted 7 lines

2. Same as 1, but also require that we weren't root.

3. Is the new euid 0 and old uid != 0?

4. Does suid == 0?

This helper checks something equivalent to 3, but only once were far enough through exec and before user code starts. This is probably equivalent to 2 as well. This is quite subtle and deserves an open-coded check, a much more carefully named helper, or, better yet, something that looks at binprm instead of cred.

Part of the motivation here is that the things we are checking for are some rather baroque combinations of conditions, so having each piece of those be as simple and clear as possible helps to better reason about what is going on (which helped Richard to find the bug he is fixing).

These helpers are local (should all be static, as James pointed out). Making helpers to simplify the final checks is the right way to clarify code. I'm all for making sure they are as clear as possible, but I do think their existence is justified.

Quoted 1 line

is_suid sounds like #4.

[...]

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

security/commoncap.c void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe · line 505

Quoted 6 lines

security/commoncap.c +1 -1

@@ -493,7 +502,7 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe
        * 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)) {

e.g. this. The logic used to be obviously slightly dicey. Now it looks sane but doesn't do what you'd naively expect it to do, which is far worse.

Quoted 3 lines

Quoted 1 line

In what way does not do what you'd expect?

It doesn't look at cred->suid.

Heh, good point. How about is_setuid()?

-serge

Quoted 29 lines · Serge E. Hallyn

Quoted 28 lines · Andy Lutomirski

--Andy

Quoted 26 lines · Serge E. Hallyn

Quoted 22 lines · Andy Lutomirski

Quoted 14 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]>

---
security/commoncap.c | 26 +++++++++++++++++++++-----

1 files changed, 21 insertions(+), 5 deletions(-)

security/commoncap.c +0 -0

index 36c38a1..1af7dec 100644

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

Quoted 5 lines

security/commoncap.c +2 -0

@@ -483,6 +483,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); }

OK I guess, but this just seems like a way to obfuscate the code a bit and save typing "->uid".

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

Personally I find the new to be far more readable. In the old, the distinction between uid and euid is one character hidden in the middle of the expression.

Would real_uid_eq be better?

Replacing is_real() with real_uid_eq() would be good. I still think that a is_setuid() is worthwhile.

I was trying to get away entirely from "uid_eq" because I didn't find it at all helpful in understanding what that function did, so I don't see real_uid_eq() as an improvement. (More below.)

Quoted 54 lines

Quoted 19 lines

Quoted 12 lines

Quoted 11 lines

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

Quoted 2 lines

security/commoncap.c +2 -0

+static inline bool is_eff(kuid_t uid, struct cred *cred)
+{ return uid_eq(cred->euid, uid); }

Ditto.

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

Quoted 3 lines

security/commoncap.c +3 -0

+
+static inline bool is_suid(kuid_t uid, struct cred *cred)
+{ return !is_real(uid, cred) && is_eff(uid, cred); }

Please no. This is IMO insane. You're hiding really weird, nonintuitive logic in an oddly named helper.

Quoted 2 lines

How is it nonintuitive? It's very precisely checking that a nonroot user is executing something that results in euid=0.

I can think of several sensible predicated:

1. Are we execing a setuid-root program, where the setuod bit wasn't suppressed by nnp, mount options, trace, etc?

Quoted 7 lines

2. Same as 1, but also require that we weren't root.

3. Is the new euid 0 and old uid != 0?

4. Does suid == 0?

This helper checks something equivalent to 3, but only once were far enough through exec and before user code starts. This is probably equivalent to 2 as well. This is quite subtle and deserves an open-coded check, a much more carefully named helper, or, better yet, something that looks at binprm instead of cred.

Part of the motivation here is that the things we are checking for are some rather baroque combinations of conditions, so having each piece of those be as simple and clear as possible helps to better reason about what is going on (which helped Richard to find the bug he is fixing).

These helpers are local (should all be static, as James pointed out). Making helpers to simplify the final checks is the right way to clarify code. I'm all for making sure they are as clear as possible, but I do think their existence is justified.

Quoted 1 line

is_suid sounds like #4.

[...]

Quoted 14 lines

Quoted 11 lines

Quoted 10 lines

security/commoncap.c void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe · line 505

Quoted 6 lines

security/commoncap.c +1 -1

@@ -493,7 +502,7 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe
        * 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)) {

e.g. this. The logic used to be obviously slightly dicey. Now it looks sane but doesn't do what you'd naively expect it to do, which is far worse.

Quoted 1 line

In what way does not do what you'd expect?

It doesn't look at cred->suid.

Quoted 1 line

Heh, good point. How about is_setuid()?

Except that *someone* earlier had come up with the local variable is_setid():

        58319057b784 luto 2015-09-04 ("capabilities: ambient capabilities")

So I'm finding this particular objection and renaming scheme a bit hard to swallow.

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]>

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

security/commoncap.c +30 -20

index 1af7dec..5d81354 100644
@@ -535,6 +535,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

@@ -614,26 +640,10 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 
 	bprm->cap_effective = effective;
 
-	/*
-	 * 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

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]>

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

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 |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

security/commoncap.c +6 -7

index 5d81354..ffcaff0 100644
@@ -551,13 +551,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;
 }
 

Signature · 1 line

1.7.1

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 |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

security/commoncap.c +6 -7

index 5d81354..ffcaff0 100644
@@ -551,13 +551,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;
 }
 

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 |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

security/commoncap.c +0 -3

index 5d81354..ffcaff0 100644
@@ -551,13 +551,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) ||

security/commoncap.c +0 -1

        return ret;
 }
 
--

1.7.1

Quoted 19 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]>

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

security/commoncap.c +0 -3

index 5d81354..ffcaff0 100644
@@ -551,13 +551,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) ||

security/commoncap.c static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root) · line 559

Quoted 10 lines

security/commoncap.c +6 -4

-                   !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;

I'm trying to give this whole series the benefit of the doubt. Here's what happens when I try to read this:

Did effective grow ambient caps? No, makes no sense. Did ambient grow effective caps? No, not that either. Is effective more fully grown than ambient? That's probably it, but that sounds really weird.

The rest would IMO be clearer if you named the helpers ruid_eq and euid_eq instead of is_eff and is_real.

Quoted 6 lines

security/commoncap.c +0 -1

        return ret;
 }
 
--

1.7.1

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 ffcaff0..eb2da69 100644
@@ -552,10 +552,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

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 ffcaff0..eb2da69 100644
@@ -552,10 +552,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;
 }

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: 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 |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

security/commoncap.c +0 -0

index eb2da69..49cce06 100644

security/commoncap.c +2 -2

  *   3) root is supposed to have all caps (SECURE_NOROOT)
  * Since this is just a normal root execing a process.
  *

@@ -553,8 +553,8 @@ 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

Quoted 7 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.

Quoted 7 lines

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

Ref: 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]>

I wonder whether,

Quoted 4 lines

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

security/commoncap.c static inline bool is_setgid(struct cred *new, const struct cred *old) · line 543

Quoted 10 lines

security/commoncap.c +1 -1

index eb2da69..49cce06 100644
@@ -540,7 +540,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 root

For clarity, what do you think about adding "(because fcaps were not used)"?

Quoted 5 lines

security/commoncap.c +0 -0

  *   3) root is supposed to have all caps (SECURE_NOROOT)
  * Since this is just a normal root execing a process.
  *

@@ -553,8 +553,8 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
 

Quoted 11 lines

security/commoncap.c +2 -2

 	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

Quoted 24 lines · Serge E. Hallyn

Quoted 7 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.

Quoted 7 lines

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

Ref: 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]>

I wonder whether,

Quoted 4 lines

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

Quoted 12 lines

security/commoncap.c static inline bool is_setgid(struct cred *new, const struct cred *old) · line 543

Quoted 10 lines

security/commoncap.c +1 -1

index eb2da69..49cce06 100644
@@ -540,7 +540,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 root

For clarity, what do you think about adding "(because fcaps were not used)"?

Possibly. Is it possible to become root without fcaps other than logging in on a console as root from the get-go? But I see your point. Even if su or sudo were used to gain root, it would have been on a previous operation and not the immediate one being audited.

The intention behind the change in the comment wording was to emphasize that the original comment hand-waved a bit about effective root vs real root without being explicit that it could be one or the other rather than requiring both, which affected the logic used to express it.

Quoted 5 lines

Quoted 5 lines

security/commoncap.c +0 -0

  *   3) root is supposed to have all caps (SECURE_NOROOT)
  * Since this is just a normal root execing a process.
  *

@@ -553,8 +553,8 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
 

Quoted 11 lines

Quoted 11 lines

security/commoncap.c +2 -2

 	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

- RGB

--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

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: 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 |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

security/commoncap.c +3 -3

index eb2da69..49cce06 100644
@@ -540,7 +540,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 root
  *   3) root is supposed to have all caps (SECURE_NOROOT)
  * Since this is just a normal root execing a process.
  *

@@ -553,8 +553,8 @@ 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;

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.

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

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

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

security/commoncap.c +20 -8

index 49cce06..8da965c 100644
@@ -536,7 +536,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

@@ -546,16 +546,28 @@ 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;
 }

@@ -639,7 +651,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 
 	bprm->cap_effective = effective;
 
-	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

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.

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

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