From mboxrd@z Thu Jan 1 00:00:00 1970 From: luto@amacapital.net (Andy Lutomirski) Date: Mon, 28 Aug 2017 13:12:21 -0700 Subject: [PATCH V3 05/10] capabilities: use intuitive names for id changes In-Reply-To: <20170828091255.GD24714@madcap2.tricolour.ca> References: <1360ed3437f87ac0b9e076ff5ea05c67ee8a7ed8.1503459890.git.rgb@redhat.com> <20170825185127.GB26620@mail.hallyn.com> <9465A086-A2C8-41E6-994E-34C7B1B9F0F9@amacapital.net> <20170825200646.GA27821@mail.hallyn.com> <20170828091255.GD24714@madcap2.tricolour.ca> Message-ID: To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org On Mon, Aug 28, 2017 at 2:12 AM, Richard Guy Briggs wrote: > On 2017-08-25 15:06, Serge E. Hallyn wrote: >> Quoting Andy Lutomirski (luto at amacapital.net): >> > >> > --Andy >> > > On Aug 25, 2017, at 11:51 AM, Serge E. Hallyn wrote: >> > > >> > > Quoting Andy Lutomirski (luto at kernel.org): >> > >>> On Wed, Aug 23, 2017 at 3:12 AM, 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 >> > >>> --- >> > >>> 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 at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Lutomirski Subject: Re: [PATCH V3 05/10] capabilities: use intuitive names for id changes Date: Mon, 28 Aug 2017 13:12:21 -0700 Message-ID: References: <1360ed3437f87ac0b9e076ff5ea05c67ee8a7ed8.1503459890.git.rgb@redhat.com> <20170825185127.GB26620@mail.hallyn.com> <9465A086-A2C8-41E6-994E-34C7B1B9F0F9@amacapital.net> <20170825200646.GA27821@mail.hallyn.com> <20170828091255.GD24714@madcap2.tricolour.ca> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <20170828091255.GD24714@madcap2.tricolour.ca> Sender: owner-linux-security-module@vger.kernel.org To: Richard Guy Briggs Cc: "Serge E. Hallyn" , Andy Lutomirski , LSM List , linux-audit@redhat.com, "Serge E. Hallyn" , Kees Cook , James Morris , Eric Paris , Paul Moore , Steve Grubb List-Id: linux-audit@redhat.com On Mon, Aug 28, 2017 at 2:12 AM, Richard Guy Briggs 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 wro= te: >> > > >> > > Quoting Andy Lutomirski (luto@kernel.org): >> > >>> On Wed, Aug 23, 2017 at 3:12 AM, 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 >> > >>> --- >> > >>> 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 midd= le >> > > of the expression. >> > >> > Would real_uid_eq be better? >> >> Replacing is_real() with real_uid_eq() would be good. I still think tha= t 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=3D0. >> > >> > I can think of several sensible predicated: >> > >> > 1. Are we execing a setuid-root program, where the setuod bit wasn't s= uppressed 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 !=3D 0? >> > >> > 4. Does suid =3D=3D 0? >> > >> > This helper checks something equivalent to 3, but only once were far e= nough through exec and before user code starts. This is probably equivalen= t to 2 as well. This is quite subtle and deserves an open-coded check, a m= uch more carefully named helper, or, better yet, something that looks at bi= nprm instead of cred. >> >> Part of the motivation here is that the things we are checking for are s= ome >> 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 goi= ng on >> (which helped Richard to find the bug he is fixing). >> >> These helpers are local (should all be static, as James pointed out). M= aking >> 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_binpr= m *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_seti= d(): > > 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.