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

[luto@amacapital.net (Andy Lutomirski)]

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 at amacapital.net):
>> >
>> > --Andy
>> > > On Aug 25, 2017, at 11:51 AM, Serge E. Hallyn <serge@hallyn.com> wrote:
>> > >
>> > > Quoting Andy Lutomirski (luto at 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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html