Re: [PATCH v4 1/9] capability: introduce new capable flag NODENYAUDIT

[Serge E. Hallyn]

On Wed, May 31, 2023 at 09:07:34AM -0500, Serge E. Hallyn wrote:
> On Thu, May 11, 2023 at 04:25:24PM +0200, Christian Göttsche wrote:
> > Introduce a new capable flag, CAP_OPT_NODENYAUDIT, to not generate
> > an audit event if the requested capability is not granted.  This will be
> > used in a new capable_any() functionality to reduce the number of
> > necessary capable calls.
> > 
> > Handle the flag accordingly in AppArmor and SELinux.
> > 
> > Suggested-by: Paul Moore <paul@xxxxxxxxxxxxxx>
> > Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> 
> Reviewed-by: Serge Hallyn <serge@xxxxxxxxxx>

Sorry, obviously I should have removed this, until the comment below was
answered :)

> > ---
> >  include/linux/security.h       |  2 ++
> >  security/apparmor/capability.c |  8 +++++---
> >  security/selinux/hooks.c       | 14 ++++++++------
> >  3 files changed, 15 insertions(+), 9 deletions(-)
> > 
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index e2734e9e44d5..629c775ec297 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -67,6 +67,8 @@ struct watch_notification;
> >  #define CAP_OPT_NOAUDIT BIT(1)
> >  /* If capable is being called by a setid function */
> >  #define CAP_OPT_INSETID BIT(2)
> > +/* If capable should audit the security request for authorized requests only */
> > +#define CAP_OPT_NODENYAUDIT BIT(3)
> >  
> >  /* LSM Agnostic defines for security_sb_set_mnt_opts() flags */
> >  #define SECURITY_LSM_NATIVE_LABELS	1
> > diff --git a/security/apparmor/capability.c b/security/apparmor/capability.c
> > index 326a51838ef2..98120dd62ca7 100644
> > --- a/security/apparmor/capability.c
> > +++ b/security/apparmor/capability.c
> > @@ -108,7 +108,8 @@ static int audit_caps(struct common_audit_data *sa, struct aa_profile *profile,
> >   * profile_capable - test if profile allows use of capability @cap
> >   * @profile: profile being enforced    (NOT NULL, NOT unconfined)
> >   * @cap: capability to test if allowed
> > - * @opts: CAP_OPT_NOAUDIT bit determines whether audit record is generated
> > + * @opts: CAP_OPT_NOAUDIT/CAP_OPT_NODENYAUDIT bit determines whether audit
> > + *	record is generated
> >   * @sa: audit data (MAY BE NULL indicating no auditing)
> >   *
> >   * Returns: 0 if allowed else -EPERM
> > @@ -126,7 +127,7 @@ static int profile_capable(struct aa_profile *profile, int cap,
> >  	else
> >  		error = -EPERM;
> >  
> > -	if (opts & CAP_OPT_NOAUDIT) {
> > +	if ((opts & CAP_OPT_NOAUDIT) || ((opts & CAP_OPT_NODENYAUDIT) && error)) {
> >  		if (!COMPLAIN_MODE(profile))
> >  			return error;
> >  		/* audit the cap request in complain mode but note that it
> > @@ -142,7 +143,8 @@ static int profile_capable(struct aa_profile *profile, int cap,
> >   * aa_capable - test permission to use capability
> >   * @label: label being tested for capability (NOT NULL)
> >   * @cap: capability to be tested
> > - * @opts: CAP_OPT_NOAUDIT bit determines whether audit record is generated
> > + * @opts: CAP_OPT_NOAUDIT/CAP_OPT_NODENYAUDIT bit determines whether audit
> > + *	record is generated
> >   *
> >   * Look up capability in profile capability set.
> >   *
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 79b4890e9936..0730edf2f5f1 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -1571,7 +1571,7 @@ static int cred_has_capability(const struct cred *cred,
> >  	u16 sclass;
> >  	u32 sid = cred_sid(cred);
> >  	u32 av = CAP_TO_MASK(cap);
> > -	int rc;
> > +	int rc, rc2;
> >  
> >  	ad.type = LSM_AUDIT_DATA_CAP;
> >  	ad.u.cap = cap;
> > @@ -1590,11 +1590,13 @@ static int cred_has_capability(const struct cred *cred,
> >  	}
> >  
> >  	rc = avc_has_perm_noaudit(sid, sid, sclass, av, 0, &avd);
> > -	if (!(opts & CAP_OPT_NOAUDIT)) {
> > -		int rc2 = avc_audit(sid, sid, sclass, av, &avd, rc, &ad);
> > -		if (rc2)
> > -			return rc2;
> > -	}
> > +	if ((opts & CAP_OPT_NOAUDIT) || ((opts & CAP_OPT_NODENYAUDIT) && rc))
> > +		return rc;
> 
> Hm, if the caller passes only CAP_OPT_NODENYAUDIT, and rc == 0, then
> you will audit the allow.  Is that what you want, or did you want, or
> did you want CAP_OPT_NODENYAUDIT to imply CAP_OPT_NOAUDIT?
> 
> > +
> > +	rc2 = avc_audit(sid, sid, sclass, av, &avd, rc, &ad);
> > +	if (rc2)
> > +		return rc2;
> > +
> >  	return rc;
> >  }
> >  
> > -- 
> > 2.40.1