[PATCH V4 00/10] capabilities: do not audit log BPRM_FCAPS on set*id

STALE3281d

Revision v4 of 4 in this series.

30 messages, 4 authors, 2017-09-20 · open the first message on its own page

[PATCH V4 00/10] capabilities: do not audit log BPRM_FCAPS on set*id

From: Richard Guy Briggs <hidden>
Date: 2017-09-05 06:46:00

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 first to eighth just massage the logic to make it easier to
understand.  Some of them could be squashed together.

The patch that resolves this issue is the ninth.  

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.

Passes: (ltp 20170516)
	./runltp -f syscalls -s cap
	./runltp -f securebits
	./runltp -f cap_bounds
	./runltp -f filecaps
	make TARGETS=capabilities kselftest (when run locally, fails over nfs)

v4
  rebase on kees' 4.13 commoncap changes
  minor local func renames

v3
  refactor into several sub-functions
  convert most macros to inline funcs

v2
  use macros to clarify intent of calculations
  fix original logic error
  address additional audit logging conditions

Richard Guy Briggs (10):
  capabilities: factor out cap_bprm_set_creds privileged root
  capabilities: intuitive names for cap gain status
  capabilities: rename has_cap to has_fcap
  capabilities: use root_priveleged inline to clarify logic
  capabilities: use intuitive names for id changes
  capabilities: move audit log decision to function
  capabilities: remove a layer of conditional logic
  capabilities: invert logic for clarity
  capabilities: fix logic for effective root or real root
  capabilities: audit log other surprising conditions

 security/commoncap.c |  179 ++++++++++++++++++++++++++++++++------------------
 1 files changed, 114 insertions(+), 65 deletions(-)

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

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

From: Richard Guy Briggs <hidden>
Date: 2017-09-05 06:46:01

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 <serge@hallyn.com>
Signed-off-by: Richard Guy Briggs <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
---
 security/commoncap.c |   63 +++++++++++++++++++++++++++----------------------
 1 files changed, 35 insertions(+), 28 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index d8e26fb..927fe93 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

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

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

-- 
1.7.1

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

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

From: Richard Guy Briggs <hidden>
Date: 2017-09-05 06:46:02

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 <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>
---
 security/commoncap.c |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index 927fe93..cf6e2b0 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

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

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

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

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

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

-- 
1.7.1

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

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

From: Richard Guy Briggs <hidden>
Date: 2017-09-05 06:46:03

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 <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>
---
 security/commoncap.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index cf6e2b0..623f251 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

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

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

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

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

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

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

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

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

-- 
1.7.1

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

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

From: Richard Guy Briggs <hidden>
Date: 2017-09-05 06:46:04

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

Suggested-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Richard Guy Briggs <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>
---
 security/commoncap.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index 623f251..1904f49 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

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

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

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

-- 
1.7.1

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

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

From: Richard Guy Briggs <hidden>
Date: 2017-09-05 06:46:05

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 <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>
---
 security/commoncap.c |   28 ++++++++++++++++++++++------
 1 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index 1904f49..d37ebec 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

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

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

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

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

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

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

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

-- 
1.7.1

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

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

From: Richard Guy Briggs <hidden>
Date: 2017-09-05 06:46:06

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

Suggested-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Richard Guy Briggs <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>
---
 security/commoncap.c |   50 ++++++++++++++++++++++++++++++--------------------
 1 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index d37ebec..eae7431 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

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

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

-- 
1.7.1

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

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

From: Richard Guy Briggs <hidden>
Date: 2017-09-05 06:46:07

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

Signed-off-by: Richard Guy Briggs <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>
---
 security/commoncap.c |   23 ++++++++++-------------
 1 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index eae7431..cf95d73 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

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

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

-- 
1.7.1

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

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

From: Richard Guy Briggs <hidden>
Date: 2017-09-05 06:46:08

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 <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>
---
 security/commoncap.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index cf95d73..7e8041d 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

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

-- 
1.7.1

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

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

From: Richard Guy Briggs <hidden>
Date: 2017-09-05 06:46:09

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

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

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

Signed-off-by: Richard Guy Briggs <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>
---
 security/commoncap.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index 7e8041d..759f3fa 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

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

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

-- 
1.7.1

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

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

From: Richard Guy Briggs <hidden>
Date: 2017-09-05 06:46:10

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 <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>
---
 security/commoncap.c |   29 ++++++++++++++++++++++-------
 1 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index 759f3fa..afebfd3 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

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

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

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

-- 
1.7.1

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

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

From: jmorris@namei.org (James Morris)
Date: 2017-09-06 06:05:17

On Tue, 5 Sep 2017, Richard Guy Briggs wrote:

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 <serge@hallyn.com>
Signed-off-by: Richard Guy Briggs <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
---
 security/commoncap.c |   63 +++++++++++++++++++++++++++----------------------
 1 files changed, 35 insertions(+), 28 deletions(-)


Acked-by: James Morris <redacted>


-- 
James Morris
[off-list ref]

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

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

From: Kees Cook <hidden>
Date: 2017-09-07 19:42:11

On Mon, Sep 4, 2017 at 11:46 PM, Richard Guy Briggs [off-list ref] wrote:

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 <serge@hallyn.com>
Signed-off-by: Richard Guy Briggs <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>

Acked-by: Kees Cook <redacted>

Note below...

quoted hunk

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

diff --git a/security/commoncap.c b/security/commoncap.c
index d8e26fb..927fe93 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

@@ -472,6 +472,39 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
        return rc;
 }

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

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

-Kees

quoted hunk

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

@@ -484,46 +517,20 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 {
        const struct cred *old = current_cred();
        struct cred *new = bprm->cred;
-       bool effective, has_cap = false, is_setid;
+       bool effective = false, has_cap = false, is_setid;
        int ret;
        kuid_t root_uid;

        if (WARN_ON(!cap_ambient_invariant_ok(old)))
                return -EPERM;

-       effective = false;
        ret = get_file_caps(bprm, &effective, &has_cap);
        if (ret < 0)
                return ret;

        root_uid = make_kuid(new->user_ns, 0);

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

        /* if we have fs caps, clear dangerous personality flags */
        if (!cap_issubset(new->cap_permitted, old->cap_permitted))
--

1.7.1



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

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

From: Kees Cook <hidden>
Date: 2017-09-07 19:57:30

On Mon, Sep 4, 2017 at 11:46 PM, Richard Guy Briggs [off-list ref] wrote:

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 <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>

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

Acked-by: Kees Cook <redacted>

quoted hunk

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

diff --git a/security/commoncap.c b/security/commoncap.c
index 927fe93..cf6e2b0 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

@@ -505,6 +505,12 @@ static void handle_privileged_root(struct linux_binprm *bprm, bool has_cap,
                *effective = true;
 }

+#define __cap_gained(field, target, source) \
+       !cap_issubset(target->cap_##field, source->cap_##field)
+#define __cap_grew(target, source, cred) \
+       !cap_issubset(cred->cap_##target, cred->cap_##source)
+#define __cap_full(field, cred) \
+       cap_issubset(CAP_FULL_SET, cred->cap_##field)

 /**
  * cap_bprm_set_creds - Set up the proposed credentials for execve().
  * @bprm: The execution parameters, including the proposed creds

@@ -533,10 +539,9 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
        handle_privileged_root(bprm, has_cap, &effective, root_uid);

        /* if we have fs caps, clear dangerous personality flags */
-       if (!cap_issubset(new->cap_permitted, old->cap_permitted))
+       if (__cap_gained(permitted, new, old))
                bprm->per_clear |= PER_CLEAR_ON_SETID;

-
        /* Don't let someone trace a set[ug]id/setpcap binary with the revised
         * credentials unless they have the appropriate permit.
         *

@@ -544,8 +549,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
         */
        is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid);

-       if ((is_setid ||
-            !cap_issubset(new->cap_permitted, old->cap_permitted)) &&
+       if ((is_setid || __cap_gained(permitted, new, old)) &&
            ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
             !ptracer_capable(current, new->user_ns))) {
                /* downgrade; they get no more than they had, and maybe less */

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

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

--

1.7.1



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

[PATCH V4 00/10] capabilities: do not audit log BPRM_FCAPS on set*id

From: paul@paul-moore.com (Paul Moore)
Date: 2017-09-08 17:02:56

On Tue, Sep 5, 2017 at 2:46 AM, Richard Guy Briggs [off-list ref] wrote:

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 first to eighth just massage the logic to make it easier to
understand.  Some of them could be squashed together.

The patch that resolves this issue is the ninth.

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.

Passes: (ltp 20170516)
        ./runltp -f syscalls -s cap
        ./runltp -f securebits
        ./runltp -f cap_bounds
        ./runltp -f filecaps
        make TARGETS=capabilities kselftest (when run locally, fails over nfs)

v4
  rebase on kees' 4.13 commoncap changes
  minor local func renames

v3
  refactor into several sub-functions
  convert most macros to inline funcs

v2
  use macros to clarify intent of calculations
  fix original logic error
  address additional audit logging conditions

Richard Guy Briggs (10):
  capabilities: factor out cap_bprm_set_creds privileged root
  capabilities: intuitive names for cap gain status
  capabilities: rename has_cap to has_fcap
  capabilities: use root_priveleged inline to clarify logic
  capabilities: use intuitive names for id changes
  capabilities: move audit log decision to function
  capabilities: remove a layer of conditional logic
  capabilities: invert logic for clarity
  capabilities: fix logic for effective root or real root
  capabilities: audit log other surprising conditions

 security/commoncap.c |  179 ++++++++++++++++++++++++++++++++------------------
 1 files changed, 114 insertions(+), 65 deletions(-)

I took a quick look at this latest revision and aside from some
disagreements on style/formatting it looks okayish to me.  However, I
am going to walk back my previous I-can-take-this-via-the-audit-tree
comments, I think this probably should go in via the capabilities
(Serge) and/or linux-security (James) tree.

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

From: Kees Cook <hidden>
Date: 2017-09-08 18:15:45

On Mon, Sep 4, 2017 at 11:46 PM, Richard Guy Briggs [off-list ref] wrote:

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 <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>

Acked-by: Kees Cook <redacted>

-Kees

quoted hunk

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

diff --git a/security/commoncap.c b/security/commoncap.c
index cf6e2b0..623f251 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

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

@@ -340,7 +340,7 @@ static inline int bprm_caps_from_vfs_caps(struct cpu_vfs_cap_data *caps,
                *effective = true;

        if (caps->magic_etc & VFS_CAP_REVISION_MASK)
-               *has_cap = true;
+               *has_fcap = true;

        CAP_FOR_EACH_U32(i) {
                __u32 permitted = caps->permitted.cap[i];

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

@@ -460,7 +460,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
                goto out;
        }

-       rc = bprm_caps_from_vfs_caps(&vcaps, bprm, effective, has_cap);
+       rc = bprm_caps_from_vfs_caps(&vcaps, bprm, effective, has_fcap);
        if (rc == -EINVAL)
                printk(KERN_NOTICE "%s: cap_from_disk returned %d for %s\n",
                       __func__, rc, bprm->filename);

@@ -472,7 +472,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c
        return rc;
 }

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

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

@@ -523,20 +523,20 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 {
        const struct cred *old = current_cred();
        struct cred *new = bprm->cred;
-       bool effective = false, has_cap = false, is_setid;
+       bool effective = false, has_fcap = false, is_setid;
        int ret;
        kuid_t root_uid;

        if (WARN_ON(!cap_ambient_invariant_ok(old)))
                return -EPERM;

-       ret = get_file_caps(bprm, &effective, &has_cap);
+       ret = get_file_caps(bprm, &effective, &has_fcap);
        if (ret < 0)
                return ret;

        root_uid = make_kuid(new->user_ns, 0);

-       handle_privileged_root(bprm, has_cap, &effective, root_uid);
+       handle_privileged_root(bprm, has_fcap, &effective, root_uid);

        /* if we have fs caps, clear dangerous personality flags */
        if (__cap_gained(permitted, new, old))

@@ -566,7 +566,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
        new->sgid = new->fsgid = new->egid;

        /* File caps or setid cancels ambient. */
-       if (has_cap || is_setid)
+       if (has_fcap || is_setid)
                cap_clear(new->cap_ambient);

        /*
--

1.7.1



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

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

From: Kees Cook <hidden>
Date: 2017-09-08 18:18:00

On Mon, Sep 4, 2017 at 11:46 PM, Richard Guy Briggs [off-list ref] wrote:

quoted hunk

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

Suggested-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Richard Guy Briggs <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>
---
 security/commoncap.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index 623f251..1904f49 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

@@ -472,14 +472,14 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f
        return rc;
 }

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

-       if (issecure(SECURE_NOROOT))
-               return;

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

Acked-by: Kees Cook <redacted>

-Kees

quoted hunk

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

@@ -536,7 +536,8 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)

        root_uid = make_kuid(new->user_ns, 0);

-       handle_privileged_root(bprm, has_fcap, &effective, root_uid);
+       if (root_privileged())
+               handle_privileged_root(bprm, has_fcap, &effective, root_uid);

        /* if we have fs caps, clear dangerous personality flags */
        if (__cap_gained(permitted, new, old))
@@ -602,7 +603,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
        if (__cap_grew(effective, ambient, new)) {
                if (!__cap_full(effective, new) ||
                    !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) ||
-                   issecure(SECURE_NOROOT)) {
+                   !root_privileged()) {
                        ret = audit_log_bprm_fcaps(bprm, new, old);
                        if (ret < 0)
                                return ret;
--

1.7.1



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

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

From: Kees Cook <hidden>
Date: 2017-09-08 18:22:18

On Mon, Sep 4, 2017 at 11:46 PM, Richard Guy Briggs [off-list ref] wrote:

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 <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>

Acked-by: Kees Cook <redacted>

-Kees

quoted hunk

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

diff --git a/security/commoncap.c b/security/commoncap.c
index 1904f49..d37ebec 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

@@ -474,6 +474,15 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f

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

+static inline bool __is_real(kuid_t uid, struct cred *cred)
+{ return uid_eq(cred->uid, uid); }
+
+static inline bool __is_eff(kuid_t uid, struct cred *cred)
+{ return uid_eq(cred->euid, uid); }
+
+static inline bool __is_suid(kuid_t uid, struct cred *cred)
+{ return !__is_real(uid, cred) && __is_eff(uid, cred); }
+
 static void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap,
                                   bool *effective, kuid_t root_uid)
 {

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

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

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

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

        if ((is_setid || __cap_gained(permitted, new, old)) &&
            ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||

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

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

1.7.1



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

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

From: Kees Cook <hidden>
Date: 2017-09-08 18:23:50

On Mon, Sep 4, 2017 at 11:46 PM, Richard Guy Briggs [off-list ref] wrote:

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

Suggested-by: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Richard Guy Briggs <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>

Acked-by: Kees Cook <redacted>

-Kees

quoted hunk

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

diff --git a/security/commoncap.c b/security/commoncap.c
index d37ebec..eae7431 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

@@ -527,6 +527,32 @@ static inline bool __is_setuid(struct cred *new, const struct cred *old)
 static inline bool __is_setgid(struct cred *new, const struct cred *old)
 { return !gid_eq(new->egid, old->gid); }

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

@@ -604,26 +630,10 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
        if (WARN_ON(!cap_ambient_invariant_ok(new)))
                return -EPERM;

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

        new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
--

1.7.1



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

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

From: Kees Cook <hidden>
Date: 2017-09-08 18:26:04

On Mon, Sep 4, 2017 at 11:46 PM, Richard Guy Briggs [off-list ref] wrote:

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

Signed-off-by: Richard Guy Briggs <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>

Acked-by: Kees Cook <redacted>

-Kees

quoted hunk

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

diff --git a/security/commoncap.c b/security/commoncap.c
index eae7431..cf95d73 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

@@ -543,13 +543,12 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
 {
        bool ret = false;

-       if (__cap_grew(effective, ambient, cred)) {
-               if (!__cap_full(effective, cred) ||
-                   !__is_eff(root, cred) || !__is_real(root, cred) ||
-                   !root_privileged()) {
-                       ret = true;
-               }
-       }
+       if (__cap_grew(effective, ambient, cred) &&
+           (!__cap_full(effective, cred) ||
+            !__is_eff(root, cred) ||
+            !__is_real(root, cred) ||
+            !root_privileged()))
+               ret = true;
        return ret;
 }

@@ -643,13 +642,11 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)

        /* Check for privilege-elevated exec. */
        bprm->cap_elevated = 0;
-       if (is_setid) {
+       if (is_setid ||
+           (!__is_real(root_uid, new) &&
+            (effective ||
+             __cap_grew(permitted, ambient, new))))
                bprm->cap_elevated = 1;
-       } else if (!__is_real(root_uid, new)) {
-               if (effective ||
-                   __cap_grew(permitted, ambient, new))
-                       bprm->cap_elevated = 1;
-       }

        return 0;
 }
--

1.7.1



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

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

From: Kees Cook <hidden>
Date: 2017-09-08 18:27:01

On Mon, Sep 4, 2017 at 11:46 PM, Richard Guy Briggs [off-list ref] wrote:

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 <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>

Acked-by: Kees Cook <redacted>

-Kees

quoted hunk

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

diff --git a/security/commoncap.c b/security/commoncap.c
index cf95d73..7e8041d 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

@@ -544,10 +544,10 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
        bool ret = false;

        if (__cap_grew(effective, ambient, cred) &&
-           (!__cap_full(effective, cred) ||
-            !__is_eff(root, cred) ||
-            !__is_real(root, cred) ||
-            !root_privileged()))
+           !(__cap_full(effective, cred) &&
+             __is_eff(root, cred) &&
+             __is_real(root, cred) &&
+             root_privileged()))
                ret = true;
        return ret;
 }
--

1.7.1



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

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

From: Kees Cook <hidden>
Date: 2017-09-08 18:34:27

On Mon, Sep 4, 2017 at 11:46 PM, Richard Guy Briggs [off-list ref] wrote:

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

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

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

Signed-off-by: Richard Guy Briggs <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>

Acked-by: Kees Cook <redacted>

-Kees

quoted hunk

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

diff --git a/security/commoncap.c b/security/commoncap.c
index 7e8041d..759f3fa 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

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

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

        if (__cap_grew(effective, ambient, cred) &&
            !(__cap_full(effective, cred) &&
-             __is_eff(root, cred) &&
-             __is_real(root, cred) &&
+             (__is_eff(root, cred) || __is_real(root, cred)) &&
              root_privileged()))
                ret = true;
        return ret;
--

1.7.1



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

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

From: Kees Cook <hidden>
Date: 2017-09-08 18:36:49

On Mon, Sep 4, 2017 at 11:46 PM, Richard Guy Briggs [off-list ref] wrote:

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 <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>

Reviewed-by: Kees Cook <redacted>

-Kees

quoted hunk

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

diff --git a/security/commoncap.c b/security/commoncap.c
index 759f3fa..afebfd3 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

@@ -528,7 +528,7 @@ static inline bool __is_setgid(struct cred *new, const struct cred *old)
 { return !gid_eq(new->egid, old->gid); }

 /*
- * Audit candidate if current->cap_effective is set
+ * 1) Audit candidate if current->cap_effective is set
  *
  * We do not bother to audit if 3 things are true:
  *   1) cap_effective has all caps

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

-       if (__cap_grew(effective, ambient, cred) &&
-           !(__cap_full(effective, cred) &&
-             (__is_eff(root, cred) || __is_real(root, cred)) &&
-             root_privileged()))
+       if ((__cap_grew(effective, ambient, new) &&
+            !(__cap_full(effective, new) &&
+              (__is_eff(root, new) || __is_real(root, new)) &&
+              root_privileged())) ||
+           (root_privileged() &&
+            __is_suid(root, new) &&
+            !__cap_full(effective, new)) ||
+           (!__is_setuid(new, old) &&
+            ((has_fcap &&
+              __cap_gained(permitted, new, old)) ||
+             __cap_gained(ambient, new, old))))
+
                ret = true;
+
        return ret;
 }

@@ -628,7 +643,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
        if (WARN_ON(!cap_ambient_invariant_ok(new)))
                return -EPERM;

-       if (nonroot_raised_pE(new, root_uid)) {
+       if (nonroot_raised_pE(new, old, root_uid, has_fcap)) {
                ret = audit_log_bprm_fcaps(bprm, new, old);
                if (ret < 0)
                        return ret;
--

1.7.1



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

[PATCH V4 00/10] capabilities: do not audit log BPRM_FCAPS on set*id

From: Richard Guy Briggs <hidden>
Date: 2017-09-14 05:54:05

On 2017-09-08 13:02, Paul Moore wrote:

On Tue, Sep 5, 2017 at 2:46 AM, Richard Guy Briggs [off-list ref] wrote:

quoted

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 first to eighth just massage the logic to make it easier to
understand.  Some of them could be squashed together.

The patch that resolves this issue is the ninth.

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.

Passes: (ltp 20170516)
        ./runltp -f syscalls -s cap
        ./runltp -f securebits
        ./runltp -f cap_bounds
        ./runltp -f filecaps
        make TARGETS=capabilities kselftest (when run locally, fails over nfs)

v4
  rebase on kees' 4.13 commoncap changes
  minor local func renames

v3
  refactor into several sub-functions
  convert most macros to inline funcs

v2
  use macros to clarify intent of calculations
  fix original logic error
  address additional audit logging conditions

Richard Guy Briggs (10):
  capabilities: factor out cap_bprm_set_creds privileged root
  capabilities: intuitive names for cap gain status
  capabilities: rename has_cap to has_fcap
  capabilities: use root_priveleged inline to clarify logic
  capabilities: use intuitive names for id changes
  capabilities: move audit log decision to function
  capabilities: remove a layer of conditional logic
  capabilities: invert logic for clarity
  capabilities: fix logic for effective root or real root
  capabilities: audit log other surprising conditions

 security/commoncap.c |  179 ++++++++++++++++++++++++++++++++------------------
 1 files changed, 114 insertions(+), 65 deletions(-)

I took a quick look at this latest revision and aside from some
disagreements on style/formatting it looks okayish to me.  However, I
am going to walk back my previous I-can-take-this-via-the-audit-tree
comments, I think this probably should go in via the capabilities
(Serge) and/or linux-security (James) tree.

"okayish"?  That sounds positive.  :-)  Can you offer a clear ack or reviewed-by?

paul moore

- RGB

--
Richard Guy Briggs [off-list ref]
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V4 00/10] capabilities: do not audit log BPRM_FCAPS on set*id

From: paul@paul-moore.com (Paul Moore)
Date: 2017-09-14 06:46:57

On Thu, Sep 14, 2017 at 1:54 AM, Richard Guy Briggs [off-list ref] wrote:

On 2017-09-08 13:02, Paul Moore wrote:

quoted

On Tue, Sep 5, 2017 at 2:46 AM, Richard Guy Briggs [off-list ref] wrote:

quoted

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 first to eighth just massage the logic to make it easier to
understand.  Some of them could be squashed together.

The patch that resolves this issue is the ninth.

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.

Passes: (ltp 20170516)
        ./runltp -f syscalls -s cap
        ./runltp -f securebits
        ./runltp -f cap_bounds
        ./runltp -f filecaps
        make TARGETS=capabilities kselftest (when run locally, fails over nfs)

v4
  rebase on kees' 4.13 commoncap changes
  minor local func renames

v3
  refactor into several sub-functions
  convert most macros to inline funcs

v2
  use macros to clarify intent of calculations
  fix original logic error
  address additional audit logging conditions

Richard Guy Briggs (10):
  capabilities: factor out cap_bprm_set_creds privileged root
  capabilities: intuitive names for cap gain status
  capabilities: rename has_cap to has_fcap
  capabilities: use root_priveleged inline to clarify logic
  capabilities: use intuitive names for id changes
  capabilities: move audit log decision to function
  capabilities: remove a layer of conditional logic
  capabilities: invert logic for clarity
  capabilities: fix logic for effective root or real root
  capabilities: audit log other surprising conditions

 security/commoncap.c |  179 ++++++++++++++++++++++++++++++++------------------
 1 files changed, 114 insertions(+), 65 deletions(-)

I took a quick look at this latest revision and aside from some
disagreements on style/formatting it looks okayish to me.  However, I
am going to walk back my previous I-can-take-this-via-the-audit-tree
comments, I think this probably should go in via the capabilities
(Serge) and/or linux-security (James) tree.

"okayish"?  That sounds positive.  :-)

It's intended to be positive~ish ;)

Parts of the patchset looked good, others I didn't really agree with,
but in general a lot of the changes seem to be subjective judgement
calls.  As long as Serge is happy with it I'm okay(ish) with it.

Can you offer a clear ack or reviewed-by?

I would need to look over the patchset again before I'm comfortable
providing either and due to LSS I'm not sure I'll have the opportunity
this week.  I'll add it to my todo list for next week.

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V4 00/10] capabilities: do not audit log BPRM_FCAPS on set*id

From: paul@paul-moore.com (Paul Moore)
Date: 2017-09-14 06:49:11

On Thu, Sep 14, 2017 at 2:46 AM, Paul Moore [off-list ref] wrote:

On Thu, Sep 14, 2017 at 1:54 AM, Richard Guy Briggs [off-list ref] wrote:

quoted

On 2017-09-08 13:02, Paul Moore wrote:

quoted

On Tue, Sep 5, 2017 at 2:46 AM, Richard Guy Briggs [off-list ref] wrote:

quoted

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 first to eighth just massage the logic to make it easier to
understand.  Some of them could be squashed together.

The patch that resolves this issue is the ninth.

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.

Passes: (ltp 20170516)
        ./runltp -f syscalls -s cap
        ./runltp -f securebits
        ./runltp -f cap_bounds
        ./runltp -f filecaps
        make TARGETS=capabilities kselftest (when run locally, fails over nfs)

v4
  rebase on kees' 4.13 commoncap changes
  minor local func renames

v3
  refactor into several sub-functions
  convert most macros to inline funcs

v2
  use macros to clarify intent of calculations
  fix original logic error
  address additional audit logging conditions

Richard Guy Briggs (10):
  capabilities: factor out cap_bprm_set_creds privileged root
  capabilities: intuitive names for cap gain status
  capabilities: rename has_cap to has_fcap
  capabilities: use root_priveleged inline to clarify logic
  capabilities: use intuitive names for id changes
  capabilities: move audit log decision to function
  capabilities: remove a layer of conditional logic
  capabilities: invert logic for clarity
  capabilities: fix logic for effective root or real root
  capabilities: audit log other surprising conditions

 security/commoncap.c |  179 ++++++++++++++++++++++++++++++++------------------
 1 files changed, 114 insertions(+), 65 deletions(-)

I took a quick look at this latest revision and aside from some
disagreements on style/formatting it looks okayish to me.  However, I
am going to walk back my previous I-can-take-this-via-the-audit-tree
comments, I think this probably should go in via the capabilities
(Serge) and/or linux-security (James) tree.

"okayish"?  That sounds positive.  :-)

It's intended to be positive~ish ;)

Parts of the patchset looked good, others I didn't really agree with,
but in general a lot of the changes seem to be subjective judgement
calls.  As long as Serge is happy with it I'm okay(ish) with it.

quoted

Can you offer a clear ack or reviewed-by?

I would need to look over the patchset again before I'm comfortable
providing either and due to LSS I'm not sure I'll have the opportunity
this week.  I'll add it to my todo list for next week.

Just to add, I'm "okay" with this going in - I don't remember anything
objectionable from my perspective - I just didn't look closely enough
to give it my Reviewed/Acked tag if that makes any sense.

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

From: paul@paul-moore.com (Paul Moore)
Date: 2017-09-20 22:11:40

On Tue, Sep 5, 2017 at 2:46 AM, Richard Guy Briggs [off-list ref] wrote:

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

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

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

Signed-off-by: Richard Guy Briggs <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>
---
 security/commoncap.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

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

Acked-by: Paul Moore <paul@paul-moore.com>

quoted hunk

diff --git a/security/commoncap.c b/security/commoncap.c
index 7e8041d..759f3fa 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

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

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

        if (__cap_grew(effective, ambient, cred) &&
            !(__cap_full(effective, cred) &&
-             __is_eff(root, cred) &&
-             __is_real(root, cred) &&
+             (__is_eff(root, cred) || __is_real(root, cred)) &&
              root_privileged()))
                ret = true;
        return ret;
--

1.7.1

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



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

From: paul@paul-moore.com (Paul Moore)
Date: 2017-09-20 22:22:30

On Tue, Sep 5, 2017 at 2:46 AM, Richard Guy Briggs [off-list ref] wrote:

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 <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>
---
 security/commoncap.c |   29 ++++++++++++++++++++++-------
 1 files changed, 22 insertions(+), 7 deletions(-)

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

Acked-by: Paul Moore <paul@paul-moore.com>

quoted hunk

diff --git a/security/commoncap.c b/security/commoncap.c
index 759f3fa..afebfd3 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

@@ -528,7 +528,7 @@ static inline bool __is_setgid(struct cred *new, const struct cred *old)
 { return !gid_eq(new->egid, old->gid); }

 /*
- * Audit candidate if current->cap_effective is set
+ * 1) Audit candidate if current->cap_effective is set
  *
  * We do not bother to audit if 3 things are true:
  *   1) cap_effective has all caps

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

-       if (__cap_grew(effective, ambient, cred) &&
-           !(__cap_full(effective, cred) &&
-             (__is_eff(root, cred) || __is_real(root, cred)) &&
-             root_privileged()))
+       if ((__cap_grew(effective, ambient, new) &&
+            !(__cap_full(effective, new) &&
+              (__is_eff(root, new) || __is_real(root, new)) &&
+              root_privileged())) ||
+           (root_privileged() &&
+            __is_suid(root, new) &&
+            !__cap_full(effective, new)) ||
+           (!__is_setuid(new, old) &&
+            ((has_fcap &&
+              __cap_gained(permitted, new, old)) ||
+             __cap_gained(ambient, new, old))))
+
                ret = true;
+
        return ret;
 }

@@ -628,7 +643,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
        if (WARN_ON(!cap_ambient_invariant_ok(new)))
                return -EPERM;

-       if (nonroot_raised_pE(new, root_uid)) {
+       if (nonroot_raised_pE(new, old, root_uid, has_fcap)) {
                ret = audit_log_bprm_fcaps(bprm, new, old);
                if (ret < 0)
                        return ret;
--

1.7.1

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



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

From: Kees Cook <hidden>
Date: 2017-09-20 22:25:15

On Wed, Sep 20, 2017 at 3:11 PM, Paul Moore [off-list ref] wrote:

On Tue, Sep 5, 2017 at 2:46 AM, Richard Guy Briggs [off-list ref] wrote:

quoted

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

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

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

Signed-off-by: Richard Guy Briggs <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>
---
 security/commoncap.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

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

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

-Kees

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

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

From: paul@paul-moore.com (Paul Moore)
Date: 2017-09-20 22:27:51

On Wed, Sep 20, 2017 at 6:25 PM, Kees Cook [off-list ref] wrote:

On Wed, Sep 20, 2017 at 3:11 PM, Paul Moore [off-list ref] wrote:

quoted

On Tue, Sep 5, 2017 at 2:46 AM, Richard Guy Briggs [off-list ref] wrote:

quoted

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

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

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

Signed-off-by: Richard Guy Briggs <redacted>
Reviewed-by: Serge Hallyn <serge@hallyn.com>
Acked-by: James Morris <redacted>
---
 security/commoncap.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

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

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

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

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help