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

[Serge E. Hallyn]

Quoting Richard Guy Briggs (rgb at redhat.com):
> On 2017-08-24 11:29, Serge E. Hallyn wrote:
> > Quoting Richard Guy Briggs (rgb at 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 at redhat.com>
> > 
> > Reviewed-by: Serge Hallyn <serge at 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 at 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