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

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(a)redhat.com&gt; > --- > 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