| Message ID | 1360ed3437f87ac0b9e076ff5ea05c67ee8a7ed8.1503459890.git.rgb@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Richard Guy Briggs (rgb@redhat.com): > Introduce a number of inlines to make the use of the negation of > uid_eq() easier to read and analyse. > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> Reviewed-by: Serge Hallyn <serge@hallyn.com> > --- > security/commoncap.c | 26 +++++++++++++++++++++----- > 1 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/security/commoncap.c b/security/commoncap.c > index 36c38a1..1af7dec 100644 > --- a/security/commoncap.c > +++ b/security/commoncap.c > @@ -483,6 +483,15 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f > > static inline bool root_privileged(void) { return !issecure(SECURE_NOROOT); } > > +static inline bool is_real(kuid_t uid, struct cred *cred) > +{ return uid_eq(cred->uid, uid); } > + > +static inline bool is_eff(kuid_t uid, struct cred *cred) > +{ return uid_eq(cred->euid, uid); } > + > +static inline bool is_suid(kuid_t uid, struct cred *cred) > +{ return !is_real(uid, cred) && is_eff(uid, cred); } > + > void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effective, kuid_t root_uid) > { > const struct cred *old = current_cred(); > @@ -493,7 +502,7 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe > * 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_fcap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) { > + if (has_fcap && is_suid(root_uid, new)) { > warn_setuid_and_fcaps_mixed(bprm->filename); > return; > } > @@ -504,12 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe > * > * 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 (is_eff(root_uid, new) || is_real(root_uid, new)) { > /* 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 (is_eff(root_uid, new)) > *effective = true; > } > > @@ -519,6 +528,13 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe > !cap_issubset(cred->cap_##target, cred->cap_##source) > #define cap_full(field, cred) \ > cap_issubset(CAP_FULL_SET, cred->cap_##field) > + > +static inline bool is_setuid(struct cred *new, const struct cred *old) > +{ return !uid_eq(new->euid, old->uid); } > + > +static inline bool is_setgid(struct cred *new, const struct cred *old) > +{ return !gid_eq(new->egid, old->gid); } > + > /** > * cap_bprm_set_creds - Set up the proposed credentials for execve(). > * @bprm: The execution parameters, including the proposed creds > @@ -556,7 +572,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) > * > * 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); > + is_setid = is_setuid(new, old) || is_setgid(new, old); > > if ((is_setid || cap_gained(permitted, new, old)) && > ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) || > @@ -612,7 +628,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) > */ > if (cap_grew(effective, ambient, new)) { > if (!cap_full(effective, new) || > - !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) || > + !is_eff(root_uid, new) || !is_real(root_uid, new) || > !root_privileged()) { > ret = audit_log_bprm_fcaps(bprm, new, old); > if (ret < 0) > -- > 1.7.1 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 23 Aug 2017, Richard Guy Briggs wrote: > Introduce a number of inlines to make the use of the negation of > uid_eq() easier to read and analyse. > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > security/commoncap.c | 26 +++++++++++++++++++++----- > 1 files changed, 21 insertions(+), 5 deletions(-) Acked-by: James Morris <james.l.morris@oracle.com>
On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <rgb@redhat.com> wrote: > Introduce a number of inlines to make the use of the negation of > uid_eq() easier to read and analyse. > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > security/commoncap.c | 26 +++++++++++++++++++++----- > 1 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/security/commoncap.c b/security/commoncap.c > index 36c38a1..1af7dec 100644 > --- a/security/commoncap.c > +++ b/security/commoncap.c > @@ -483,6 +483,15 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f > > static inline bool root_privileged(void) { return !issecure(SECURE_NOROOT); } > > +static inline bool is_real(kuid_t uid, struct cred *cred) > +{ return uid_eq(cred->uid, uid); } OK I guess, but this just seems like a way to obfuscate the code a bit and save typing "->uid". > + > +static inline bool is_eff(kuid_t uid, struct cred *cred) > +{ return uid_eq(cred->euid, uid); } Ditto. > + > +static inline bool is_suid(kuid_t uid, struct cred *cred) > +{ return !is_real(uid, cred) && is_eff(uid, cred); } Please no. This is IMO insane. You're hiding really weird, nonintuitive logic in an oddly named helper. Also, this is going to cause massive confusion and severe bugs: given the same, the only remotely sensible guess as to what this function does is uid_eq(cred->suid, uid). So NAK to this. > + > void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effective, kuid_t root_uid) > { > const struct cred *old = current_cred(); > @@ -493,7 +502,7 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe > * 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_fcap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) { > + if (has_fcap && is_suid(root_uid, new)) { e.g. this. The logic used to be obviously slightly dicey. Now it looks sane but doesn't do what you'd naively expect it to do, which is far worse. > @@ -519,6 +528,13 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe > !cap_issubset(cred->cap_##target, cred->cap_##source) > #define cap_full(field, cred) \ > cap_issubset(CAP_FULL_SET, cred->cap_##field) > + > +static inline bool is_setuid(struct cred *new, const struct cred *old) > +{ return !uid_eq(new->euid, old->uid); } > + > +static inline bool is_setgid(struct cred *new, const struct cred *old) > +{ return !gid_eq(new->egid, old->gid); } Please don't. This logic is fragile, and these helpers are pretending it's not fragile even though it's still fragile. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Andy Lutomirski (luto@kernel.org): > On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <rgb@redhat.com> wrote: > > Introduce a number of inlines to make the use of the negation of > > uid_eq() easier to read and analyse. > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > --- > > security/commoncap.c | 26 +++++++++++++++++++++----- > > 1 files changed, 21 insertions(+), 5 deletions(-) > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > index 36c38a1..1af7dec 100644 > > --- a/security/commoncap.c > > +++ b/security/commoncap.c > > @@ -483,6 +483,15 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f > > > > static inline bool root_privileged(void) { return !issecure(SECURE_NOROOT); } > > > > +static inline bool is_real(kuid_t uid, struct cred *cred) > > +{ return uid_eq(cred->uid, uid); } > > OK I guess, but this just seems like a way to obfuscate the code a bit > and save typing "->uid". Personally I find the new to be far more readable. In the old, the distinction between uid and euid is one character hidden in the middle of the expression. > > + > > +static inline bool is_eff(kuid_t uid, struct cred *cred) > > +{ return uid_eq(cred->euid, uid); } > > Ditto. > > > + > > +static inline bool is_suid(kuid_t uid, struct cred *cred) > > +{ return !is_real(uid, cred) && is_eff(uid, cred); } > > Please no. This is IMO insane. You're hiding really weird, > nonintuitive logic in an oddly named helper. How is it nonintuitive? It's very precisely checking that a nonroot user is executing something that results in euid=0. That's what it's checking for, the name of the new helper makes that clear, and the code becomes clearer because we only see the thing we care about checking for rather than the intent being hidden. > Also, this is going to cause massive confusion and severe bugs: given > the same, the only remotely sensible guess as to what this function > does is uid_eq(cred->suid, uid). So NAK to this. > > > + > > void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effective, kuid_t root_uid) > > { > > const struct cred *old = current_cred(); > > @@ -493,7 +502,7 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe > > * 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_fcap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) { > > + if (has_fcap && is_suid(root_uid, new)) { > > e.g. this. The logic used to be obviously slightly dicey. Now it > looks sane but doesn't do what you'd naively expect it to do, which is > far worse. In what way does not do what you'd expect? -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--Andy > On Aug 25, 2017, at 11:51 AM, Serge E. Hallyn <serge@hallyn.com> wrote: > > Quoting Andy Lutomirski (luto@kernel.org): >>> On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <rgb@redhat.com> wrote: >>> Introduce a number of inlines to make the use of the negation of >>> uid_eq() easier to read and analyse. >>> >>> Signed-off-by: Richard Guy Briggs <rgb@redhat.com> >>> --- >>> security/commoncap.c | 26 +++++++++++++++++++++----- >>> 1 files changed, 21 insertions(+), 5 deletions(-) >>> >>> diff --git a/security/commoncap.c b/security/commoncap.c >>> index 36c38a1..1af7dec 100644 >>> --- a/security/commoncap.c >>> +++ b/security/commoncap.c >>> @@ -483,6 +483,15 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f >>> >>> static inline bool root_privileged(void) { return !issecure(SECURE_NOROOT); } >>> >>> +static inline bool is_real(kuid_t uid, struct cred *cred) >>> +{ return uid_eq(cred->uid, uid); } >> >> OK I guess, but this just seems like a way to obfuscate the code a bit >> and save typing "->uid". > > Personally I find the new to be far more readable. In the old, the > distinction between uid and euid is one character hidden in the middle > of the expression. Would real_uid_eq be better? > >>> + >>> +static inline bool is_eff(kuid_t uid, struct cred *cred) >>> +{ return uid_eq(cred->euid, uid); } >> >> Ditto. >> >>> + >>> +static inline bool is_suid(kuid_t uid, struct cred *cred) >>> +{ return !is_real(uid, cred) && is_eff(uid, cred); } >> >> Please no. This is IMO insane. You're hiding really weird, >> nonintuitive logic in an oddly named helper. > > How is it nonintuitive? It's very precisely checking that a > nonroot user is executing something that results in euid=0. I can think of several sensible predicated: 1. Are we execing a setuid-root program, where the setuod bit wasn't suppressed by nnp, mount options, trace, etc? 2. Same as 1, but also require that we weren't root. 3. Is the new euid 0 and old uid != 0? 4. Does suid == 0? This helper checks something equivalent to 3, but only once were far enough through exec and before user code starts. This is probably equivalent to 2 as well. This is quite subtle and deserves an open-coded check, a much more carefully named helper, or, better yet, something that looks at binprm instead of cred. is_suid sounds like #4. > That's what it's checking for, the name of the new helper makes > that clear, and the code becomes clearer because we only see the > thing we care about checking for rather than the intent being > hidden. > >> Also, this is going to cause massive confusion and severe bugs: given >> the same, the only remotely sensible guess as to what this function >> does is uid_eq(cred->suid, uid). So NAK to this. >> >>> + >>> void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effective, kuid_t root_uid) >>> { >>> const struct cred *old = current_cred(); >>> @@ -493,7 +502,7 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe >>> * 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_fcap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) { >>> + if (has_fcap && is_suid(root_uid, new)) { >> >> e.g. this. The logic used to be obviously slightly dicey. Now it >> looks sane but doesn't do what you'd naively expect it to do, which is >> far worse. > > In what way does not do what you'd expect? It doesn't look at cred->suid.-- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Andy Lutomirski (luto@amacapital.net): > > > > --Andy > > On Aug 25, 2017, at 11:51 AM, Serge E. Hallyn <serge@hallyn.com> wrote: > > > > Quoting Andy Lutomirski (luto@kernel.org): > >>> On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <rgb@redhat.com> wrote: > >>> Introduce a number of inlines to make the use of the negation of > >>> uid_eq() easier to read and analyse. > >>> > >>> Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > >>> --- > >>> security/commoncap.c | 26 +++++++++++++++++++++----- > >>> 1 files changed, 21 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/security/commoncap.c b/security/commoncap.c > >>> index 36c38a1..1af7dec 100644 > >>> --- a/security/commoncap.c > >>> +++ b/security/commoncap.c > >>> @@ -483,6 +483,15 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f > >>> > >>> static inline bool root_privileged(void) { return !issecure(SECURE_NOROOT); } > >>> > >>> +static inline bool is_real(kuid_t uid, struct cred *cred) > >>> +{ return uid_eq(cred->uid, uid); } > >> > >> OK I guess, but this just seems like a way to obfuscate the code a bit > >> and save typing "->uid". > > > > Personally I find the new to be far more readable. In the old, the > > distinction between uid and euid is one character hidden in the middle > > of the expression. > > Would real_uid_eq be better? Replacing is_real() with real_uid_eq() would be good. I still think that a is_setuid() is worthwhile. > >>> +static inline bool is_eff(kuid_t uid, struct cred *cred) > >>> +{ return uid_eq(cred->euid, uid); } > >> > >> Ditto. > >> > >>> + > >>> +static inline bool is_suid(kuid_t uid, struct cred *cred) > >>> +{ return !is_real(uid, cred) && is_eff(uid, cred); } > >> > >> Please no. This is IMO insane. You're hiding really weird, > >> nonintuitive logic in an oddly named helper. > > > > How is it nonintuitive? It's very precisely checking that a > > nonroot user is executing something that results in euid=0. > > I can think of several sensible predicated: > > 1. Are we execing a setuid-root program, where the setuod bit wasn't suppressed by nnp, mount options, trace, etc? > > 2. Same as 1, but also require that we weren't root. > > 3. Is the new euid 0 and old uid != 0? > > 4. Does suid == 0? > > This helper checks something equivalent to 3, but only once were far enough through exec and before user code starts. This is probably equivalent to 2 as well. This is quite subtle and deserves an open-coded check, a much more carefully named helper, or, better yet, something that looks at binprm instead of cred. Part of the motivation here is that the things we are checking for are some rather baroque combinations of conditions, so having each piece of those be as simple and clear as possible helps to better reason about what is going on (which helped Richard to find the bug he is fixing). These helpers are local (should all be static, as James pointed out). Making helpers to simplify the final checks is the right way to clarify code. I'm all for making sure they are as clear as possible, but I do think their existence is justified. > is_suid sounds like #4. [...] > >>> @@ -493,7 +502,7 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe > >>> * 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_fcap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) { > >>> + if (has_fcap && is_suid(root_uid, new)) { > >> > >> e.g. this. The logic used to be obviously slightly dicey. Now it > >> looks sane but doesn't do what you'd naively expect it to do, which is > >> far worse. > > > > In what way does not do what you'd expect? > > It doesn't look at cred->suid. Heh, good point. How about is_setuid()? -serge -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 25 Aug 2017, Serge E. Hallyn wrote: > Part of the motivation here is that the things we are checking for are some > rather baroque combinations of conditions, so having each piece of those be > as simple and clear as possible helps to better reason about what is going on > (which helped Richard to find the bug he is fixing). > > These helpers are local (should all be static, as James pointed out). Making > helpers to simplify the final checks is the right way to clarify code. I'm > all for making sure they are as clear as possible, but I do think their existence > is justified. Perhaps document them better and maybe prefix them with __ to emphasize that they're internal only?
On 2017-08-25 15:06, Serge E. Hallyn wrote: > Quoting Andy Lutomirski (luto@amacapital.net): > > > > --Andy > > > On Aug 25, 2017, at 11:51 AM, Serge E. Hallyn <serge@hallyn.com> wrote: > > > > > > Quoting Andy Lutomirski (luto@kernel.org): > > >>> On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <rgb@redhat.com> wrote: > > >>> Introduce a number of inlines to make the use of the negation of > > >>> uid_eq() easier to read and analyse. > > >>> > > >>> Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > >>> --- > > >>> security/commoncap.c | 26 +++++++++++++++++++++----- > > >>> 1 files changed, 21 insertions(+), 5 deletions(-) > > >>> > > >>> diff --git a/security/commoncap.c b/security/commoncap.c > > >>> index 36c38a1..1af7dec 100644 > > >>> --- a/security/commoncap.c > > >>> +++ b/security/commoncap.c > > >>> @@ -483,6 +483,15 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f > > >>> > > >>> static inline bool root_privileged(void) { return !issecure(SECURE_NOROOT); } > > >>> > > >>> +static inline bool is_real(kuid_t uid, struct cred *cred) > > >>> +{ return uid_eq(cred->uid, uid); } > > >> > > >> OK I guess, but this just seems like a way to obfuscate the code a bit > > >> and save typing "->uid". > > > > > > Personally I find the new to be far more readable. In the old, the > > > distinction between uid and euid is one character hidden in the middle > > > of the expression. > > > > Would real_uid_eq be better? > > Replacing is_real() with real_uid_eq() would be good. I still think that a > is_setuid() is worthwhile. I was trying to get away entirely from "uid_eq" because I didn't find it at all helpful in understanding what that function did, so I don't see real_uid_eq() as an improvement. (More below.) > > >>> +static inline bool is_eff(kuid_t uid, struct cred *cred) > > >>> +{ return uid_eq(cred->euid, uid); } > > >> > > >> Ditto. > > >> > > >>> + > > >>> +static inline bool is_suid(kuid_t uid, struct cred *cred) > > >>> +{ return !is_real(uid, cred) && is_eff(uid, cred); } > > >> > > >> Please no. This is IMO insane. You're hiding really weird, > > >> nonintuitive logic in an oddly named helper. > > > > > > How is it nonintuitive? It's very precisely checking that a > > > nonroot user is executing something that results in euid=0. > > > > I can think of several sensible predicated: > > > > 1. Are we execing a setuid-root program, where the setuod bit wasn't suppressed by nnp, mount options, trace, etc? > > > > 2. Same as 1, but also require that we weren't root. > > > > 3. Is the new euid 0 and old uid != 0? > > > > 4. Does suid == 0? > > > > This helper checks something equivalent to 3, but only once were far enough through exec and before user code starts. This is probably equivalent to 2 as well. This is quite subtle and deserves an open-coded check, a much more carefully named helper, or, better yet, something that looks at binprm instead of cred. > > Part of the motivation here is that the things we are checking for are some > rather baroque combinations of conditions, so having each piece of those be > as simple and clear as possible helps to better reason about what is going on > (which helped Richard to find the bug he is fixing). > > These helpers are local (should all be static, as James pointed out). Making > helpers to simplify the final checks is the right way to clarify code. I'm > all for making sure they are as clear as possible, but I do think their existence > is justified. > > > is_suid sounds like #4. > [...] > > >>> @@ -493,7 +502,7 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe > > >>> * 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_fcap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) { > > >>> + if (has_fcap && is_suid(root_uid, new)) { > > >> > > >> e.g. this. The logic used to be obviously slightly dicey. Now it > > >> looks sane but doesn't do what you'd naively expect it to do, which is > > >> far worse. > > > > > > In what way does not do what you'd expect? > > > > It doesn't look at cred->suid. > > Heh, good point. How about is_setuid()? Except that *someone* earlier had come up with the local variable is_setid(): 58319057b784 luto 2015-09-04 ("capabilities: ambient capabilities") So I'm finding this particular objection and renaming scheme a bit hard to swallow. I simply extended the convention and made the two conditions that feed it follow the same naming pattern but for the fact that it is a function name with the minimum necessary arguments in the order that made the most sense to me to read aloud to understand its usage. Making it a macro that hides the arguments was my original effort. I was trying to find another unique function name that got across the intent of what I needed to express. > -serge - RGB -- Richard Guy Briggs <rgb@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 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 28, 2017 at 2:12 AM, Richard Guy Briggs <rgb@redhat.com> wrote: > On 2017-08-25 15:06, Serge E. Hallyn wrote: >> Quoting Andy Lutomirski (luto@amacapital.net): >> > >> > --Andy >> > > On Aug 25, 2017, at 11:51 AM, Serge E. Hallyn <serge@hallyn.com> wrote: >> > > >> > > Quoting Andy Lutomirski (luto@kernel.org): >> > >>> On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <rgb@redhat.com> wrote: >> > >>> Introduce a number of inlines to make the use of the negation of >> > >>> uid_eq() easier to read and analyse. >> > >>> >> > >>> Signed-off-by: Richard Guy Briggs <rgb@redhat.com> >> > >>> --- >> > >>> security/commoncap.c | 26 +++++++++++++++++++++----- >> > >>> 1 files changed, 21 insertions(+), 5 deletions(-) >> > >>> >> > >>> diff --git a/security/commoncap.c b/security/commoncap.c >> > >>> index 36c38a1..1af7dec 100644 >> > >>> --- a/security/commoncap.c >> > >>> +++ b/security/commoncap.c >> > >>> @@ -483,6 +483,15 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f >> > >>> >> > >>> static inline bool root_privileged(void) { return !issecure(SECURE_NOROOT); } >> > >>> >> > >>> +static inline bool is_real(kuid_t uid, struct cred *cred) >> > >>> +{ return uid_eq(cred->uid, uid); } >> > >> >> > >> OK I guess, but this just seems like a way to obfuscate the code a bit >> > >> and save typing "->uid". >> > > >> > > Personally I find the new to be far more readable. In the old, the >> > > distinction between uid and euid is one character hidden in the middle >> > > of the expression. >> > >> > Would real_uid_eq be better? >> >> Replacing is_real() with real_uid_eq() would be good. I still think that a >> is_setuid() is worthwhile. > > I was trying to get away entirely from "uid_eq" because I didn't find it > at all helpful in understanding what that function did, so I don't see > real_uid_eq() as an improvement. (More below.) > >> > >>> +static inline bool is_eff(kuid_t uid, struct cred *cred) >> > >>> +{ return uid_eq(cred->euid, uid); } >> > >> >> > >> Ditto. >> > >> >> > >>> + >> > >>> +static inline bool is_suid(kuid_t uid, struct cred *cred) >> > >>> +{ return !is_real(uid, cred) && is_eff(uid, cred); } >> > >> >> > >> Please no. This is IMO insane. You're hiding really weird, >> > >> nonintuitive logic in an oddly named helper. >> > > >> > > How is it nonintuitive? It's very precisely checking that a >> > > nonroot user is executing something that results in euid=0. >> > >> > I can think of several sensible predicated: >> > >> > 1. Are we execing a setuid-root program, where the setuod bit wasn't suppressed by nnp, mount options, trace, etc? >> > >> > 2. Same as 1, but also require that we weren't root. >> > >> > 3. Is the new euid 0 and old uid != 0? >> > >> > 4. Does suid == 0? >> > >> > This helper checks something equivalent to 3, but only once were far enough through exec and before user code starts. This is probably equivalent to 2 as well. This is quite subtle and deserves an open-coded check, a much more carefully named helper, or, better yet, something that looks at binprm instead of cred. >> >> Part of the motivation here is that the things we are checking for are some >> rather baroque combinations of conditions, so having each piece of those be >> as simple and clear as possible helps to better reason about what is going on >> (which helped Richard to find the bug he is fixing). >> >> These helpers are local (should all be static, as James pointed out). Making >> helpers to simplify the final checks is the right way to clarify code. I'm >> all for making sure they are as clear as possible, but I do think their existence >> is justified. >> >> > is_suid sounds like #4. >> [...] >> > >>> @@ -493,7 +502,7 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe >> > >>> * 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_fcap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) { >> > >>> + if (has_fcap && is_suid(root_uid, new)) { >> > >> >> > >> e.g. this. The logic used to be obviously slightly dicey. Now it >> > >> looks sane but doesn't do what you'd naively expect it to do, which is >> > >> far worse. >> > > >> > > In what way does not do what you'd expect? >> > >> > It doesn't look at cred->suid. >> >> Heh, good point. How about is_setuid()? > > Except that *someone* earlier had come up with the local variable is_setid(): > > 58319057b784 luto 2015-09-04 ("capabilities: ambient capabilities") > > So I'm finding this particular objection and renaming scheme a bit hard > to swallow. At least that thing is a variable that's local to the function (and hence to the context in which that function is called). I'm not saying it's a work of art. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/security/commoncap.c b/security/commoncap.c index 36c38a1..1af7dec 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -483,6 +483,15 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f static inline bool root_privileged(void) { return !issecure(SECURE_NOROOT); } +static inline bool is_real(kuid_t uid, struct cred *cred) +{ return uid_eq(cred->uid, uid); } + +static inline bool is_eff(kuid_t uid, struct cred *cred) +{ return uid_eq(cred->euid, uid); } + +static inline bool is_suid(kuid_t uid, struct cred *cred) +{ return !is_real(uid, cred) && is_eff(uid, cred); } + void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effective, kuid_t root_uid) { const struct cred *old = current_cred(); @@ -493,7 +502,7 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe * 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_fcap && !uid_eq(new->uid, root_uid) && uid_eq(new->euid, root_uid)) { + if (has_fcap && is_suid(root_uid, new)) { warn_setuid_and_fcaps_mixed(bprm->filename); return; } @@ -504,12 +513,12 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe * * 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 (is_eff(root_uid, new) || is_real(root_uid, new)) { /* 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 (is_eff(root_uid, new)) *effective = true; } @@ -519,6 +528,13 @@ void handle_privileged_root(struct linux_binprm *bprm, bool has_fcap, bool *effe !cap_issubset(cred->cap_##target, cred->cap_##source) #define cap_full(field, cred) \ cap_issubset(CAP_FULL_SET, cred->cap_##field) + +static inline bool is_setuid(struct cred *new, const struct cred *old) +{ return !uid_eq(new->euid, old->uid); } + +static inline bool is_setgid(struct cred *new, const struct cred *old) +{ return !gid_eq(new->egid, old->gid); } + /** * cap_bprm_set_creds - Set up the proposed credentials for execve(). * @bprm: The execution parameters, including the proposed creds @@ -556,7 +572,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) * * 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); + is_setid = is_setuid(new, old) || is_setgid(new, old); if ((is_setid || cap_gained(permitted, new, old)) && ((bprm->unsafe & ~LSM_UNSAFE_PTRACE) || @@ -612,7 +628,7 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) */ if (cap_grew(effective, ambient, new)) { if (!cap_full(effective, new) || - !uid_eq(new->euid, root_uid) || !uid_eq(new->uid, root_uid) || + !is_eff(root_uid, new) || !is_real(root_uid, new) || !root_privileged()) { ret = audit_log_bprm_fcaps(bprm, new, old); if (ret < 0)
Introduce a number of inlines to make the use of the negation of uid_eq() easier to read and analyse. Signed-off-by: Richard Guy Briggs <rgb@redhat.com> --- security/commoncap.c | 26 +++++++++++++++++++++----- 1 files changed, 21 insertions(+), 5 deletions(-)