diff mbox

[V3,09/10] capabilities: fix logic for effective root or real root

Message ID 0d9646956d9b2d99e8699c009de21f14fa592e7a.1503459890.git.rgb@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Richard Guy Briggs Aug. 23, 2017, 10:13 a.m. UTC
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 |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

Comments

Serge E. Hallyn Aug. 24, 2017, 4:29 p.m. UTC | #1
Quoting Richard Guy Briggs (rgb@redhat.com):
> 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>

Reviewed-by: Serge Hallyn <serge@hallyn.com>

I wonder whether,

> ---
>  security/commoncap.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/security/commoncap.c b/security/commoncap.c
> index eb2da69..49cce06 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -540,7 +540,7 @@ static inline bool is_setgid(struct cred *new, const struct cred *old)
>   *
>   * We do not bother to audit if 3 things are true:
>   *   1) cap_effective has all caps
> - *   2) we are root
> + *   2) we became root *OR* are root

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

>   *   3) root is supposed to have all caps (SECURE_NOROOT)
>   * Since this is just a normal root execing a process.
>   *
> @@ -553,8 +553,8 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
>  
>  	if (cap_grew(effective, ambient, cred) &&
>  	    !(cap_full(effective, cred) &&
> -	      is_eff(root, cred) &&
> -	      is_real(root, cred) &&
> +	      (is_eff(root, cred) ||
> +	       is_real(root, cred)) &&
>  	      root_privileged()))
>  		ret = true;
>  	return ret;
> -- 
> 1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Richard Guy Briggs Aug. 24, 2017, 4:44 p.m. UTC | #2
On 2017-08-24 11:29, Serge E. Hallyn wrote:
> Quoting Richard Guy Briggs (rgb@redhat.com):
> > 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>
> 
> Reviewed-by: Serge Hallyn <serge@hallyn.com>
> 
> I wonder whether,
> 
> > ---
> >  security/commoncap.c |    6 +++---
> >  1 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/security/commoncap.c b/security/commoncap.c
> > index eb2da69..49cce06 100644
> > --- a/security/commoncap.c
> > +++ b/security/commoncap.c
> > @@ -540,7 +540,7 @@ static inline bool is_setgid(struct cred *new, const struct cred *old)
> >   *
> >   * We do not bother to audit if 3 things are true:
> >   *   1) cap_effective has all caps
> > - *   2) we are root
> > + *   2) we became root *OR* are root
> 
> For clarity, what do you think about adding "(because fcaps were not used)"?

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

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

> >   *   3) root is supposed to have all caps (SECURE_NOROOT)
> >   * Since this is just a normal root execing a process.
> >   *
> > @@ -553,8 +553,8 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
> >  
> >  	if (cap_grew(effective, ambient, cred) &&
> >  	    !(cap_full(effective, cred) &&
> > -	      is_eff(root, cred) &&
> > -	      is_real(root, cred) &&
> > +	      (is_eff(root, cred) ||
> > +	       is_real(root, cred)) &&
> >  	      root_privileged()))
> >  		ret = true;
> >  	return ret;
> > -- 
> > 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Serge E. Hallyn Aug. 24, 2017, 4:47 p.m. UTC | #3
Quoting Richard Guy Briggs (rgb@redhat.com):
> On 2017-08-24 11:29, Serge E. Hallyn wrote:
> > Quoting Richard Guy Briggs (rgb@redhat.com):
> > > 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>
> > 
> > Reviewed-by: Serge Hallyn <serge@hallyn.com>
> > 
> > I wonder whether,
> > 
> > > ---
> > >  security/commoncap.c |    6 +++---
> > >  1 files changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/security/commoncap.c b/security/commoncap.c
> > > index eb2da69..49cce06 100644
> > > --- a/security/commoncap.c
> > > +++ b/security/commoncap.c
> > > @@ -540,7 +540,7 @@ static inline bool is_setgid(struct cred *new, const struct cred *old)
> > >   *
> > >   * We do not bother to audit if 3 things are true:
> > >   *   1) cap_effective has all caps
> > > - *   2) we are root
> > > + *   2) we became root *OR* are root
> > 
> > For clarity, what do you think about adding "(because fcaps were not used)"?
> 
> Possibly.  Is it possible to become root without fcaps other than
> logging in on a console as root from the get-go?  But I see your point.
> Even if su or sudo were used to gain root, it would have been on a
> previous operation and not the immediate one being audited.
> 
> The intention behind the change in the comment wording was to emphasize
> that the original comment hand-waved a bit about effective root vs real
> root without being explicit that it could be one or the other rather
> than requiring both, which affected the logic used to express it.

Ok - let's leave it as is.  Thanks for the set!

> > >   *   3) root is supposed to have all caps (SECURE_NOROOT)
> > >   * Since this is just a normal root execing a process.
> > >   *
> > > @@ -553,8 +553,8 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
> > >  
> > >  	if (cap_grew(effective, ambient, cred) &&
> > >  	    !(cap_full(effective, cred) &&
> > > -	      is_eff(root, cred) &&
> > > -	      is_real(root, cred) &&
> > > +	      (is_eff(root, cred) ||
> > > +	       is_real(root, cred)) &&
> > >  	      root_privileged()))
> > >  		ret = true;
> > >  	return ret;
> > > -- 
> > > 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Morris Aug. 25, 2017, 5:48 a.m. UTC | #4
On Wed, 23 Aug 2017, Richard Guy Briggs 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: 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 |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)


Acked-by: James Morris <james.l.morris@oracle.com>


> 
> diff --git a/security/commoncap.c b/security/commoncap.c
> index eb2da69..49cce06 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -540,7 +540,7 @@ static inline bool is_setgid(struct cred *new, const struct cred *old)
>   *
>   * We do not bother to audit if 3 things are true:
>   *   1) cap_effective has all caps
> - *   2) we are root
> + *   2) we became root *OR* are root
>   *   3) root is supposed to have all caps (SECURE_NOROOT)
>   * Since this is just a normal root execing a process.
>   *
> @@ -553,8 +553,8 @@ static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
>  
>  	if (cap_grew(effective, ambient, cred) &&
>  	    !(cap_full(effective, cred) &&
> -	      is_eff(root, cred) &&
> -	      is_real(root, cred) &&
> +	      (is_eff(root, cred) ||
> +	       is_real(root, cred)) &&
>  	      root_privileged()))
>  		ret = true;
>  	return ret;
>
diff mbox

Patch

diff --git a/security/commoncap.c b/security/commoncap.c
index eb2da69..49cce06 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -540,7 +540,7 @@  static inline bool is_setgid(struct cred *new, const struct cred *old)
  *
  * We do not bother to audit if 3 things are true:
  *   1) cap_effective has all caps
- *   2) we are root
+ *   2) we became root *OR* are root
  *   3) root is supposed to have all caps (SECURE_NOROOT)
  * Since this is just a normal root execing a process.
  *
@@ -553,8 +553,8 @@  static inline bool nonroot_raised_pE(struct cred *cred, kuid_t root)
 
 	if (cap_grew(effective, ambient, cred) &&
 	    !(cap_full(effective, cred) &&
-	      is_eff(root, cred) &&
-	      is_real(root, cred) &&
+	      (is_eff(root, cred) ||
+	       is_real(root, cred)) &&
 	      root_privileged()))
 		ret = true;
 	return ret;