Re: [PATCH V3 05/10] capabilities: use intuitive names for id changes
--Andy
On Aug 25, 2017, at 11:51 AM, Serge E. Hallyn
<serge(a)hallyn.com> wrote:
Quoting Andy Lutomirski (luto(a)kernel.org):
>> On Wed, Aug 23, 2017 at 3:12 AM, Richard Guy Briggs <rgb(a)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(a)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.