[RFC PATCH V2 0/4] capabilities: do not audit log BPRM_FCAPS on set*id

[RFC PATCH V2 0/4] capabilities: do not audit log BPRM_FCAPS on set*id

[Richard Guy Briggs]

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

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

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

It would be possible to address the original issue with a change of
	"!uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid)"
to
	"!(uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid))"
but it took me long enough to understand this logic that I don't think I'd be
doing any favours by leaving it this difficult to understand.

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

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

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

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

[RFC PATCH V2 1/4] capabilities: use macros to make the logic easier to follow and verify

[Richard Guy Briggs]

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

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 security/commoncap.c |   53 ++++++++++++++++++++++++++++++++-----------------
 1 files changed, 34 insertions(+), 19 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index 78b3783..9520f0a 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -497,6 +497,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 	int ret;
 	kuid_t root_uid;
 
+#define SROOT !issecure(SECURE_NOROOT) /* root is special */
+#define RROOT uid_eq(new->uid, root_uid) /* real root */
+#define EROOT uid_eq(new->euid, root_uid) /* effective root */
+#define SETUIDROOT !RROOT && EROOT /* set uid root */
+#define SUID !uid_eq(new->euid, old->uid) /* set uid */
+#define SGID !gid_eq(new->egid, old->gid) /* set gid */
+#define pPADD !cap_issubset(new->cap_permitted, old->cap_permitted) /* process permitted capabilities have been added */
+#define pESET !cap_issubset(new->cap_effective, new->cap_ambient) /* process effective capabilities have been set */
+#define pEALL cap_issubset(CAP_FULL_SET, new->cap_effective) /* process effective capabilities are full set */
+#define pAADD !cap_issubset(new->cap_ambient, old->cap_ambient) /* process ambient capabilities have been added */
 	if (WARN_ON(!cap_ambient_invariant_ok(old)))
 		return -EPERM;
 
@@ -507,13 +517,13 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 
 	root_uid = make_kuid(new->user_ns, 0);
 
-	if (!issecure(SECURE_NOROOT)) {
+	if (SROOT) {
 		/*
 		 * If the legacy file capability is set, then don't set privs
 		 * for a setuid root binary run by a non-root user.  Do set it
 		 * for a root user just to cause least surprise to an admin.
 		 */
-		if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) {
+		if (has_cap && SETUIDROOT) {
 			warn_setuid_and_fcaps_mixed(bprm->filename);
 			goto skip;
 		}
@@ -521,33 +531,32 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 		 * To support inheritance of root-permissions and suid-root
 		 * executables under compatibility mode, we override the
 		 * capability sets for the file.
-		 *
-		 * If only the real uid is 0, we do not set the effective bit.
 		 */
-		if (uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid)) {
+		if (EROOT || RROOT) {
 			/* pP' = (cap_bset & ~0) | (pI & ~0) */
 			new->cap_permitted = cap_combine(old->cap_bset,
 							 old->cap_inheritable);
 		}
-		if (uid_eq(new->euid, root_uid))
+		/*
+		 * If only the real uid is root, we do not set the effective bit.
+		 */
+		if (EROOT)
 			effective = true;
 	}
 skip:
 
 	/* if we have fs caps, clear dangerous personality flags */
-	if (!cap_issubset(new->cap_permitted, old->cap_permitted))
+	if (pPADD)
 		bprm->per_clear |= PER_CLEAR_ON_SETID;
 
+	is_setid = SUID || SGID;
 
 	/* Don't let someone trace a set[ug]id/setpcap binary with the revised
 	 * credentials unless they have the appropriate permit.
 	 *
 	 * In addition, if NO_NEW_PRIVS, then ensure we get no new privs.
 	 */
-	is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid);
-
-	if ((is_setid ||
-	     !cap_issubset(new->cap_permitted, old->cap_permitted)) &&
+	if ((is_setid || pPADD) &&
 	    ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
 	     !ptracer_capable(current, new->user_ns))) {
 		/* downgrade; they get no more than they had, and maybe less */
@@ -599,14 +608,10 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 	 * Number 1 above might fail if you don't have a full bset, but I think
 	 * that is interesting information to audit.
 	 */
-	if (!cap_issubset(new->cap_effective, new->cap_ambient)) {
-		if (!cap_issubset(CAP_FULL_SET, new->cap_effective) ||
-		    !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) ||
-		    issecure(SECURE_NOROOT)) {
-			ret = audit_log_bprm_fcaps(bprm, new, old);
-			if (ret < 0)
-				return ret;
-		}
+	if (pESET && (!pEALL || !EROOT || !RROOT || !SROOT) ) {
+		ret = audit_log_bprm_fcaps(bprm, new, old);
+		if (ret < 0)
+			return ret;
 	}
 
 	new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
@@ -615,6 +620,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
 		return -EPERM;
 
 	return 0;
+#undef SROOT
+#undef RROOT
+#undef EROOT
+#undef SETUIDROOT
+#undef SUID
+#undef SGID
+#undef pPADD
+#undef pESET
+#undef pEALL
+#undef pAADD
 }
 
 /**
-- 
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

[RFC PATCH V2 1/4] capabilities: use macros to make the logic easier to follow and verify

[Serge E. Hallyn]

On Thu, May 11, 2017 at 04:42:40PM -0400, Richard Guy Briggs wrote:
> This change is intended to be logic-neutral and simply make the logic easier to
> read in natural language and verify without getting distracted by details.
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  security/commoncap.c |   53 ++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 34 insertions(+), 19 deletions(-)
> 
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 78b3783..9520f0a 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -497,6 +497,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  	int ret;
>  	kuid_t root_uid;
>  

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

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

maybe

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

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

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

> +#define SUID !uid_eq(new->euid, old->uid) /* set uid */
> +#define SGID !gid_eq(new->egid, old->gid) /* set gid */
> +#define pPADD !cap_issubset(new->cap_permitted, old->cap_permitted) /* process permitted capabilities have been added */
> +#define pESET !cap_issubset(new->cap_effective, new->cap_ambient) /* process effective capabilities have been set */
> +#define pEALL cap_issubset(CAP_FULL_SET, new->cap_effective) /* process effective capabilities are full set */
> +#define pAADD !cap_issubset(new->cap_ambient, old->cap_ambient) /* process ambient capabilities have been added */
>  	if (WARN_ON(!cap_ambient_invariant_ok(old)))
>  		return -EPERM;
>  
> @@ -507,13 +517,13 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  
>  	root_uid = make_kuid(new->user_ns, 0);
>  
> -	if (!issecure(SECURE_NOROOT)) {
> +	if (SROOT) {
>  		/*
>  		 * If the legacy file capability is set, then don't set privs
>  		 * for a setuid root binary run by a non-root user.  Do set it
>  		 * for a root user just to cause least surprise to an admin.
>  		 */
> -		if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) {
> +		if (has_cap && SETUIDROOT) {
>  			warn_setuid_and_fcaps_mixed(bprm->filename);
>  			goto skip;
>  		}
> @@ -521,33 +531,32 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  		 * To support inheritance of root-permissions and suid-root
>  		 * executables under compatibility mode, we override the
>  		 * capability sets for the file.
> -		 *
> -		 * If only the real uid is 0, we do not set the effective bit.
>  		 */
> -		if (uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid)) {
> +		if (EROOT || RROOT) {
>  			/* pP' = (cap_bset & ~0) | (pI & ~0) */
>  			new->cap_permitted = cap_combine(old->cap_bset,
>  							 old->cap_inheritable);
>  		}
> -		if (uid_eq(new->euid, root_uid))
> +		/*
> +		 * If only the real uid is root, we do not set the effective bit.
> +		 */
> +		if (EROOT)
>  			effective = true;
>  	}
>  skip:
>  
>  	/* if we have fs caps, clear dangerous personality flags */
> -	if (!cap_issubset(new->cap_permitted, old->cap_permitted))
> +	if (pPADD)
>  		bprm->per_clear |= PER_CLEAR_ON_SETID;
>  
> +	is_setid = SUID || SGID;
>  
>  	/* Don't let someone trace a set[ug]id/setpcap binary with the revised
>  	 * credentials unless they have the appropriate permit.
>  	 *
>  	 * In addition, if NO_NEW_PRIVS, then ensure we get no new privs.
>  	 */
> -	is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid);
> -
> -	if ((is_setid ||
> -	     !cap_issubset(new->cap_permitted, old->cap_permitted)) &&
> +	if ((is_setid || pPADD) &&
>  	    ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
>  	     !ptracer_capable(current, new->user_ns))) {
>  		/* downgrade; they get no more than they had, and maybe less */
> @@ -599,14 +608,10 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  	 * Number 1 above might fail if you don't have a full bset, but I think
>  	 * that is interesting information to audit.
>  	 */
> -	if (!cap_issubset(new->cap_effective, new->cap_ambient)) {
> -		if (!cap_issubset(CAP_FULL_SET, new->cap_effective) ||
> -		    !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) ||
> -		    issecure(SECURE_NOROOT)) {
> -			ret = audit_log_bprm_fcaps(bprm, new, old);
> -			if (ret < 0)
> -				return ret;
> -		}
> +	if (pESET && (!pEALL || !EROOT || !RROOT || !SROOT) ) {

This might be better served by a separate helper 

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

> +		ret = audit_log_bprm_fcaps(bprm, new, old);
> +		if (ret < 0)
> +			return ret;
>  	}
>  
>  	new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
> @@ -615,6 +620,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  		return -EPERM;
>  
>  	return 0;
> +#undef SROOT
> +#undef RROOT
> +#undef EROOT
> +#undef SETUIDROOT
> +#undef SUID
> +#undef SGID
> +#undef pPADD
> +#undef pESET
> +#undef pEALL
> +#undef pAADD
>  }
>  
>  /**
> -- 
> 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

[RFC PATCH V2 1/4] capabilities: use macros to make the logic easier to follow and verify

[Richard Guy Briggs]

On 2017-05-12 00:35, Serge E. Hallyn wrote:
> On Thu, May 11, 2017 at 04:42:40PM -0400, Richard Guy Briggs wrote:
> > This change is intended to be logic-neutral and simply make the logic easier to
> > read in natural language and verify without getting distracted by details.
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  security/commoncap.c |   53 ++++++++++++++++++++++++++++++++-----------------
> >  1 files changed, 34 insertions(+), 19 deletions(-)
> > 
> > diff --git a/security/commoncap.c b/security/commoncap.c
> > index 78b3783..9520f0a 100644
> > --- a/security/commoncap.c
> > +++ b/security/commoncap.c
> > @@ -497,6 +497,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
> >  	int ret;
> >  	kuid_t root_uid;
> >  
> 
> The #defines make me uncomfortable, especially the lack of parens around
> them.  The way they are used seems fine, but they seem like potential
> future maintenance issues.  I definately appreciate the way you broke
> the functionality down, though.  And I'm not sure I can improve on it.

I wrestled with #defines vs functions and the #defines won because they
were cleaner without parameters in parens to cloud the reading of the
logic.  I could go with local variables to assign these a value, but
they would be evaluated at the time of setting the local variable (which
would be fine for most of the variables involved) but also use local
stack space, though the compiler may be smart enough to optimize it
away.  This was also why I quickly undefined them as soon as
practicable.

> > +#define SROOT !issecure(SECURE_NOROOT) /* root is special */
> 
> maybe
> 
> static inline bool root_privileged() { return !issecure(SECURE_NOROOT); }

That is one of the few clean ones that doesn't need any other parameters
(such as "new" and "old" and the recently computed "root_uid").

> > +#define RROOT uid_eq(new->uid, root_uid) /* real root */
> > +#define EROOT uid_eq(new->euid, root_uid) /* effective root */
> > +#define SETUIDROOT !RROOT && EROOT /* set uid root */
> 
> Yeah every time I start typing an alternative it doesn't look as good.

Yup, this one was originally SUIDROOT which would have conflicted with SUID.

> > +#define SUID !uid_eq(new->euid, old->uid) /* set uid */
> > +#define SGID !gid_eq(new->egid, old->gid) /* set gid */
> > +#define pPADD !cap_issubset(new->cap_permitted, old->cap_permitted) /* process permitted capabilities have been added */
> > +#define pESET !cap_issubset(new->cap_effective, new->cap_ambient) /* process effective capabilities have been set */
> > +#define pEALL cap_issubset(CAP_FULL_SET, new->cap_effective) /* process effective capabilities are full set */
> > +#define pAADD !cap_issubset(new->cap_ambient, old->cap_ambient) /* process ambient capabilities have been added */
> >  	if (WARN_ON(!cap_ambient_invariant_ok(old)))
> >  		return -EPERM;
> >  
> > @@ -507,13 +517,13 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
> >  
> >  	root_uid = make_kuid(new->user_ns, 0);
> >  
> > -	if (!issecure(SECURE_NOROOT)) {
> > +	if (SROOT) {
> >  		/*
> >  		 * If the legacy file capability is set, then don't set privs
> >  		 * for a setuid root binary run by a non-root user.  Do set it
> >  		 * for a root user just to cause least surprise to an admin.
> >  		 */
> > -		if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) {
> > +		if (has_cap && SETUIDROOT) {
> >  			warn_setuid_and_fcaps_mixed(bprm->filename);
> >  			goto skip;
> >  		}
> > @@ -521,33 +531,32 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
> >  		 * To support inheritance of root-permissions and suid-root
> >  		 * executables under compatibility mode, we override the
> >  		 * capability sets for the file.
> > -		 *
> > -		 * If only the real uid is 0, we do not set the effective bit.
> >  		 */
> > -		if (uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid)) {
> > +		if (EROOT || RROOT) {
> >  			/* pP' = (cap_bset & ~0) | (pI & ~0) */
> >  			new->cap_permitted = cap_combine(old->cap_bset,
> >  							 old->cap_inheritable);
> >  		}
> > -		if (uid_eq(new->euid, root_uid))
> > +		/*
> > +		 * If only the real uid is root, we do not set the effective bit.
> > +		 */
> > +		if (EROOT)
> >  			effective = true;
> >  	}
> >  skip:
> >  
> >  	/* if we have fs caps, clear dangerous personality flags */
> > -	if (!cap_issubset(new->cap_permitted, old->cap_permitted))
> > +	if (pPADD)
> >  		bprm->per_clear |= PER_CLEAR_ON_SETID;
> >  
> > +	is_setid = SUID || SGID;
> >  
> >  	/* Don't let someone trace a set[ug]id/setpcap binary with the revised
> >  	 * credentials unless they have the appropriate permit.
> >  	 *
> >  	 * In addition, if NO_NEW_PRIVS, then ensure we get no new privs.
> >  	 */
> > -	is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid);
> > -
> > -	if ((is_setid ||
> > -	     !cap_issubset(new->cap_permitted, old->cap_permitted)) &&
> > +	if ((is_setid || pPADD) &&
> >  	    ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
> >  	     !ptracer_capable(current, new->user_ns))) {
> >  		/* downgrade; they get no more than they had, and maybe less */
> > @@ -599,14 +608,10 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
> >  	 * Number 1 above might fail if you don't have a full bset, but I think
> >  	 * that is interesting information to audit.
> >  	 */
> > -	if (!cap_issubset(new->cap_effective, new->cap_ambient)) {
> > -		if (!cap_issubset(CAP_FULL_SET, new->cap_effective) ||
> > -		    !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) ||
> > -		    issecure(SECURE_NOROOT)) {
> > -			ret = audit_log_bprm_fcaps(bprm, new, old);
> > -			if (ret < 0)
> > -				return ret;
> > -		}
> > +	if (pESET && (!pEALL || !EROOT || !RROOT || !SROOT) ) {
> 
> This might be better served by a separate helper 
> 
> 	if (nonroot_raised_e(new, root_uid)) {
> 		ret = audit_log_bprm_fcaps(bprm, new, old);
> 		if (ret < 0)
> 			return ret;
> 	}
> 
> > +		ret = audit_log_bprm_fcaps(bprm, new, old);
> > +		if (ret < 0)
> > +			return ret;
> >  	}
> >  
> >  	new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
> > @@ -615,6 +620,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
> >  		return -EPERM;
> >  
> >  	return 0;
> > +#undef SROOT
> > +#undef RROOT
> > +#undef EROOT
> > +#undef SETUIDROOT
> > +#undef SUID
> > +#undef SGID
> > +#undef pPADD
> > +#undef pESET
> > +#undef pEALL
> > +#undef pAADD
> >  }
> >  
> >  /**
> > -- 
> > 1.7.1

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
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

[RFC PATCH V2 1/4] capabilities: use macros to make the logic easier to follow and verify

[Serge E. Hallyn]

Quoting Richard Guy Briggs (rgb at redhat.com):
> This change is intended to be logic-neutral and simply make the logic easier to
> read in natural language and verify without getting distracted by details.
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  security/commoncap.c |   53 ++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 34 insertions(+), 19 deletions(-)
> 
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 78b3783..9520f0a 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -497,6 +497,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  	int ret;
>  	kuid_t root_uid;
>  
> +#define SROOT !issecure(SECURE_NOROOT) /* root is special */
> +#define RROOT uid_eq(new->uid, root_uid) /* real root */
> +#define EROOT uid_eq(new->euid, root_uid) /* effective root */
> +#define SETUIDROOT !RROOT && EROOT /* set uid root */
> +#define SUID !uid_eq(new->euid, old->uid) /* set uid */
> +#define SGID !gid_eq(new->egid, old->gid) /* set gid */
> +#define pPADD !cap_issubset(new->cap_permitted, old->cap_permitted) /* process permitted capabilities have been added */
> +#define pESET !cap_issubset(new->cap_effective, new->cap_ambient) /* process effective capabilities have been set */
> +#define pEALL cap_issubset(CAP_FULL_SET, new->cap_effective) /* process effective capabilities are full set */
> +#define pAADD !cap_issubset(new->cap_ambient, old->cap_ambient) /* process ambient capabilities have been added */
>  	if (WARN_ON(!cap_ambient_invariant_ok(old)))
>  		return -EPERM;

Ok, I'm going to offer a few alternatives below.  Please feel free to
say you think yours is better, and I'll simply ack that.  I just feel
obliged to give it the old college try,

> @@ -507,13 +517,13 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  
>  	root_uid = make_kuid(new->user_ns, 0);
>  
> -	if (!issecure(SECURE_NOROOT)) {
> +	if (SROOT) {
>  		/*
>  		 * If the legacy file capability is set, then don't set privs
>  		 * for a setuid root binary run by a non-root user.  Do set it
>  		 * for a root user just to cause least surprise to an admin.
>  		 */
> -		if (has_cap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) {
> +		if (has_cap && SETUIDROOT) {

Thinking about this some more last night, there are two things which make these
checks harder to read than they should be.  One is the fact that the brain can
have a hard time with negation.  The second is that we have long conditionals
in which the pieces are expressed using the mechanism we need to express what
we want, rather than what we are really asking.    So sometimes just coming up
with a different name for a function can help clarify its use, i.e.
uid_changed(from, to) instead of !uid_eq.

So the above I think would be much easier as

		if (has_cap && did_setuid_to(new, root_uid)) {

Needing to predefine root_uid is unfortunate.  Perhaps giving it a shorter name
like 'root' would help in some of these lines.  So then

		if (has_cap && did_setuid_to(new, root)) {

>  			warn_setuid_and_fcaps_mixed(bprm->filename);
>  			goto skip;
>  		}
> @@ -521,33 +531,32 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  		 * To support inheritance of root-permissions and suid-root
>  		 * executables under compatibility mode, we override the
>  		 * capability sets for the file.
> -		 *
> -		 * If only the real uid is 0, we do not set the effective bit.
>  		 */
> -		if (uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid)) {
> +		if (EROOT || RROOT) {

		if (same_real_uid(new, root) || same_eff_uid(new, root))

>  			/* pP' = (cap_bset & ~0) | (pI & ~0) */
>  			new->cap_permitted = cap_combine(old->cap_bset,
>  							 old->cap_inheritable);
>  		}
> -		if (uid_eq(new->euid, root_uid))
> +		/*
> +		 * If only the real uid is root, we do not set the effective bit.
> +		 */
> +		if (EROOT)
>  			effective = true;

		if (same_eff_uid(new, root))

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

		if (p_caps_grew(old, new))

Not quite sure whether p_caps_grew, e_caps_grew, a_caps_grew() are
nicer, or caps_grew(old, new, field).  i like the latter, but when
seeing the final patch maybe the former would be clearer.  But so maybe

		if (caps_grew(old, new, permitted))

>  
> +	is_setid = SUID || SGID;

	is_setid = eff_uid_changed(old, new) || eff_gid_changed(old, new)

uid_changed() is a trivial change, but i think is a lot more helpful while
reading this.

>  
>  	/* Don't let someone trace a set[ug]id/setpcap binary with the revised
>  	 * credentials unless they have the appropriate permit.
>  	 *
>  	 * In addition, if NO_NEW_PRIVS, then ensure we get no new privs.
>  	 */
> -	is_setid = !uid_eq(new->euid, old->uid) || !gid_eq(new->egid, old->gid);
> -
> -	if ((is_setid ||
> -	     !cap_issubset(new->cap_permitted, old->cap_permitted)) &&
> +	if ((is_setid || pPADD) &&

	if ((is_setid || p_caps_grew(old, new)) &&

>  	    ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) ||
>  	     !ptracer_capable(current, new->user_ns))) {
>  		/* downgrade; they get no more than they had, and maybe less */
> @@ -599,14 +608,10 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  	 * Number 1 above might fail if you don't have a full bset, but I think
>  	 * that is interesting information to audit.
>  	 */
> -	if (!cap_issubset(new->cap_effective, new->cap_ambient)) {
> -		if (!cap_issubset(CAP_FULL_SET, new->cap_effective) ||
> -		    !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) ||
> -		    issecure(SECURE_NOROOT)) {
> -			ret = audit_log_bprm_fcaps(bprm, new, old);
> -			if (ret < 0)
> -				return ret;
> -		}
> +	if (pESET && (!pEALL || !EROOT || !RROOT || !SROOT) ) {

And this I still think this should be a separate function, though what
you have obviously helped you reason about its correctness.

> +		ret = audit_log_bprm_fcaps(bprm, new, old);
> +		if (ret < 0)
> +			return ret;
>  	}
>  
>  	new->securebits &= ~issecure_mask(SECURE_KEEP_CAPS);
> @@ -615,6 +620,16 @@ int cap_bprm_set_creds(struct linux_binprm *bprm)
>  		return -EPERM;
>  
>  	return 0;
> +#undef SROOT
> +#undef RROOT
> +#undef EROOT
> +#undef SETUIDROOT
> +#undef SUID
> +#undef SGID
> +#undef pPADD
> +#undef pESET
> +#undef pEALL
> +#undef pAADD

Lastly, the first !issecure(SECURE_NOROOT) block could stand to be a
separate function.

	handle_privileged_root(bprm, has_cap, &effective, root);
--
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

[RFC PATCH V2 2/4] capabilities: invert logic for clarity

[Richard Guy Briggs]

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

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 security/commoncap.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index 9520f0a..664d6a5 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -608,7 +608,7 @@ 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 (pESET && (!pEALL || !EROOT || !RROOT || !SROOT) ) {
+	if (pESET && !(pEALL && EROOT && RROOT && SROOT) ) {
 		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

[RFC PATCH V2 3/4] capabilities: fix logic for effective root or real root

[Richard Guy Briggs]

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

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

Ref: 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 <rgb@redhat.com>
---
 security/commoncap.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index 664d6a5..c0adee6 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -608,7 +608,7 @@ 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 (pESET && !(pEALL && EROOT && RROOT && SROOT) ) {
+	if (pESET && !(pEALL && (EROOT || RROOT) && SROOT) ) {
 		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

[RFC PATCH V2 4/4] capabilities: auit log other surprising conditions

[Richard Guy Briggs]

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

Stated again:
- When you execute a setuid root application, it is no surprise and expected
that it got all capabilities, so we do not want capabilities recorded.
        if (pESET && !(pEALL && (EROOT || RROOT) && SROOT) )

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..
        SROOT && SETUIDROOT && !pEALL

- 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.
        !SUID && FILECAP && pPADD

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

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

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 security/commoncap.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/security/commoncap.c b/security/commoncap.c
index c0adee6..6309e81 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -608,7 +608,9 @@ 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 (pESET && !(pEALL && (EROOT || RROOT) && SROOT) ) {
+	if ( (pESET && !(pEALL && (EROOT || RROOT) && SROOT) )
+	    || (SROOT && SETUIDROOT && !pEALL)
+	    ||  (!SUID && ( (has_cap && pPADD) || pAADD) )) {
 		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

[RFC PATCH V2 0/4] capabilities: do not audit log BPRM_FCAPS on set*id

[Paul Moore]

On Thu, May 11, 2017 at 4:42 PM, Richard Guy Briggs <rgb@redhat.com> 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 patch that resolves this issue is the third.  The first and second just
> massage the logic to make it easier to understand.
>
> It would be possible to address the original issue with a change of
>         "!uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid)"
> to
>         "!(uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid))"
> but it took me long enough to understand this logic that I don't think I'd be
> doing any favours by leaving it this difficult to understand.
>
> The final patch attempts to address all the conditions that need logging based
> on mailing list conversations, recoginizing there is probably some duplication
> in the logic, which is why I'm posting this as an RFC for some feedback.
>
> Richard Guy Briggs (4):
>   capabilities: use macros to make the logic easier to follow and
>     verify
>   capabilities: invert logic for clarity
>   capabilities: fix logic for effective root or real root
>   capabilities: auit log other surprising conditions
>
>  security/commoncap.c |   55 ++++++++++++++++++++++++++++++++-----------------
>  1 files changed, 36 insertions(+), 19 deletions(-)

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

-- 
paul moore
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

[RFC PATCH V2 0/4] capabilities: do not audit log BPRM_FCAPS on set*id

[Richard Guy Briggs]

On 2017-06-02 11:19, Paul Moore wrote:
> On Thu, May 11, 2017 at 4:42 PM, Richard Guy Briggs <rgb@redhat.com> 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 patch that resolves this issue is the third.  The first and second just
> > massage the logic to make it easier to understand.
> >
> > It would be possible to address the original issue with a change of
> >         "!uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid)"
> > to
> >         "!(uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid))"
> > but it took me long enough to understand this logic that I don't think I'd be
> > doing any favours by leaving it this difficult to understand.
> >
> > The final patch attempts to address all the conditions that need logging based
> > on mailing list conversations, recoginizing there is probably some duplication
> > in the logic, which is why I'm posting this as an RFC for some feedback.
> >
> > Richard Guy Briggs (4):
> >   capabilities: use macros to make the logic easier to follow and
> >     verify
> >   capabilities: invert logic for clarity
> >   capabilities: fix logic for effective root or real root
> >   capabilities: auit log other surprising conditions
> >
> >  security/commoncap.c |   55 ++++++++++++++++++++++++++++++++-----------------
> >  1 files changed, 36 insertions(+), 19 deletions(-)
> 
> Following up on this set of patches ... I see there was some
> discussion between you and Serge for one of the patches, but it isn't
> clear to me that there was any resolution reached; where do things
> stand at the moment?

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

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

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
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

[RFC PATCH V2 0/4] capabilities: do not audit log BPRM_FCAPS on set*id

[Paul Moore]

On Fri, Jun 2, 2017 at 2:03 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-06-02 11:19, Paul Moore wrote:
>> On Thu, May 11, 2017 at 4:42 PM, Richard Guy Briggs <rgb@redhat.com> 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 patch that resolves this issue is the third.  The first and second just
>> > massage the logic to make it easier to understand.
>> >
>> > It would be possible to address the original issue with a change of
>> >         "!uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid)"
>> > to
>> >         "!(uid_eq(new->euid, root_uid) || uid_eq(new->uid, root_uid))"
>> > but it took me long enough to understand this logic that I don't think I'd be
>> > doing any favours by leaving it this difficult to understand.
>> >
>> > The final patch attempts to address all the conditions that need logging based
>> > on mailing list conversations, recoginizing there is probably some duplication
>> > in the logic, which is why I'm posting this as an RFC for some feedback.
>> >
>> > Richard Guy Briggs (4):
>> >   capabilities: use macros to make the logic easier to follow and
>> >     verify
>> >   capabilities: invert logic for clarity
>> >   capabilities: fix logic for effective root or real root
>> >   capabilities: auit log other surprising conditions
>> >
>> >  security/commoncap.c |   55 ++++++++++++++++++++++++++++++++-----------------
>> >  1 files changed, 36 insertions(+), 19 deletions(-)
>>
>> Following up on this set of patches ... I see there was some
>> discussion between you and Serge for one of the patches, but it isn't
>> clear to me that there was any resolution reached; where do things
>> stand at the moment?
>
> Well, I was still waiting to hear from you before submitting another
> round of patches.

All of the changes in this patchset relate to the capabilities code
and not the audit code so I don't really have a strong opinion.  Serge
knows way more about the subtleties of Linux capabilities than I do; I
trust his judgement there.

The only area I might want to weigh in on would be if/when we generate
records, but considering the rather vague answer from Steve when he
was asked about certification requirements, I'm not going to worry
about it too much right now.  Your logic outlined in the description
seems reasonable.

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

Since that lives in the capabilities code and not the audit code that
is between you and Serge as far as I'm concerned.  I can send this up
via the audit tree if Serge would prefer, but I would want to see his
ACK.

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