--Andy
> On Aug 25, 2017, at 11:51 AM, Serge E. Hallyn <se...@hallyn.com> wrote:
>
> Quoting Andy Lutomirski (l...@kernel.org):
>>> On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <r...@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 <r...@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.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
