From: serge@hallyn.com (Serge E. Hallyn) To: linux-security-module@vger.kernel.org Subject: [RFC PATCH V2 1/4] capabilities: use macros to make the logic easier to follow and verify Date: Fri, 12 May 2017 00:35:16 -0500 [thread overview] Message-ID: <20170512053516.GA29944@mail.hallyn.com> (raw) In-Reply-To: <f676394915d5a3b3feb3ffc2f1587575bc13deaa.1494527628.git.rgb@redhat.com> 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@redhat.com> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING: multiple messages have this Message-ID (diff)
From: "Serge E. Hallyn" <serge@hallyn.com> To: Richard Guy Briggs <rgb@redhat.com> Cc: linux-security-module@vger.kernel.org, linux-audit@redhat.com, Andy Lutomirski <luto@kernel.org>, "Serge E. Hallyn" <serge.hallyn@ubuntu.com>, Kees Cook <keescook@chromium.org>, James Morris <james.l.morris@oracle.com>, Eric Paris <eparis@redhat.com>, Paul Moore <pmoore@redhat.com>, Steve Grubb <sgrubb@redhat.com> Subject: Re: [RFC PATCH V2 1/4] capabilities: use macros to make the logic easier to follow and verify Date: Fri, 12 May 2017 00:35:16 -0500 [thread overview] Message-ID: <20170512053516.GA29944@mail.hallyn.com> (raw) In-Reply-To: <f676394915d5a3b3feb3ffc2f1587575bc13deaa.1494527628.git.rgb@redhat.com> 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@redhat.com> > --- > 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
next prev parent reply other threads:[~2017-05-12 5:35 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-05-11 20:42 [RFC PATCH V2 0/4] capabilities: do not audit log BPRM_FCAPS on set*id Richard Guy Briggs 2017-05-11 20:42 ` Richard Guy Briggs 2017-05-11 20:42 ` [RFC PATCH V2 1/4] capabilities: use macros to make the logic easier to follow and verify Richard Guy Briggs 2017-05-11 20:42 ` Richard Guy Briggs 2017-05-12 5:35 ` Serge E. Hallyn [this message] 2017-05-12 5:35 ` Serge E. Hallyn 2017-05-12 11:37 ` Richard Guy Briggs 2017-05-12 11:37 ` Richard Guy Briggs 2017-05-12 13:50 ` Serge E. Hallyn 2017-05-12 13:50 ` Serge E. Hallyn 2017-05-11 20:42 ` [RFC PATCH V2 2/4] capabilities: invert logic for clarity Richard Guy Briggs 2017-05-11 20:42 ` Richard Guy Briggs 2017-05-11 20:42 ` [RFC PATCH V2 3/4] capabilities: fix logic for effective root or real root Richard Guy Briggs 2017-05-11 20:42 ` Richard Guy Briggs 2017-05-11 20:42 ` [RFC PATCH V2 4/4] capabilities: auit log other surprising conditions Richard Guy Briggs 2017-05-11 20:42 ` Richard Guy Briggs 2017-06-02 15:19 ` [RFC PATCH V2 0/4] capabilities: do not audit log BPRM_FCAPS on set*id Paul Moore 2017-06-02 15:19 ` Paul Moore 2017-06-02 18:03 ` Richard Guy Briggs 2017-06-02 18:03 ` Richard Guy Briggs 2017-06-02 19:30 ` Paul Moore 2017-06-02 19:30 ` Paul Moore
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20170512053516.GA29944@mail.hallyn.com \ --to=serge@hallyn.com \ --cc=linux-security-module@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.