On Tue, Sep 5, 2017 at 2:46 AM, Richard Guy Briggs <[email protected]> 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 <[email protected]> > Reviewed-by: Serge Hallyn <[email protected]> > Acked-by: James Morris <[email protected]> > --- > 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 <[email protected]> > 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 [email protected] > More majordomo info at http://vger.kernel.org/majordomo-info.html -- paul moore www.paul-moore.com -- Linux-audit mailing list [email protected] https://www.redhat.com/mailman/listinfo/linux-audit
