Re: [PATCH V3 05/10] capabilities: use intuitive names for id changes

Quoting Andy Lutomirski (luto(a)amacapital.net): > > --Andy > > On Aug 25, 2017, at 11:51 AM, Serge E. Hallyn <serge(a)hallyn.com&gt; wrote: > > > > Quoting Andy Lutomirski (luto(a)kernel.org): > >>> On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; 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(a)redhat.com&gt; > >>> --- > >>> 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