Re: [patch] sys_setpriority() euid semantics fix

[Peter Williams <pwil3058@bigpond.net.au>]

Ingo Molnar wrote:
> * Peter Williams <pwil3058@bigpond.net.au> wrote:
> 
> 
>>I've just noticed what might be a bug in the original code.  Shouldn't
>>the following:
>>
>>       if ((current->euid != p->euid) && (current->euid != p->uid) &&
>>               !capable(CAP_SYS_NICE))
>>
>>be:
>>
>>       if ((current->uid != p->uid) && (current->euid != p->uid) &&
>>               !capable(CAP_SYS_NICE))
>>
>>I.e. if the real or effective uid of the task doing the setting is not
>>the same as the uid of the target task it is not allowed to change the
>>target task's policy unless it is specially privileged.
> 
> 
> no. The original code is quite logical: when doing something to others,
> only the euid is taken into account. When others do something to you,
> both the uid and the euid is checked ('others' might have no idea about
> this task temporarily changing the euid to a less/more privileged uid). 
> So sys_setscheduler() [and sys_setaffinity(), which does the same] is
> fine.

I disagree.  Logically, the only privileges that should be relevant are 
those of the task making the change i.e. those of current.  The 
effective uid of the target task is irrelevant.  If only the effective 
uid counts for determining what a task can do then the statement can be 
simplified to:

        if ((current->euid != p->uid) && !capable(CAP_SYS_NICE))

but I think that this is wrong as having an effective uid that is 
different to the real uid doesn't cancel the privileges assoctaied with 
the real uid.

The way uid and effective uid effect a task's operations on files are a 
guide to their logical application in cases like this.

Since this operation is usually performed by a task on itself it 
probably doesn't matter.  But when task A is trying to change the policy 
of task B then it is the privileges of the task A that count and all 
that is relevant about task B is who its real owner is.

For example, if a user A ran a program that was setuid to an 
unprivileged user B then A would be unable (from within that program) to 
change the policy of any RT tasks that she had running to SCHED_NORMAL 
unless those tasks were also setuid to B.  I agree that this is a highly 
unlikely circumstance but it does serve to illustrate the point w.r.t. 
what is logical.

> 
> what _is_ inconsistent is kernel/sys.c's setpriority()/set_one_prio(). 
> 
> It checks current->euid|uid against p->uid, which makes little sense,

I think that this is correct.

> but is how we've been doing it ever since. It's a Linux quirk documented
> in the manpage. To make things funnier, SuS requires current->euid|uid
> match against p->euid.

I've just read the man page and I think that the description of what 
Linux does (or is supposed to do) is correct/logical and that the other 
behaviors described are (very) illogical.  In any case, the change that 
I suggested will make the behaviour match what the man page says.

> 
> The patch below fixes it (and brings the logic in line with what
> setscheduler()/setaffinity() does), but if we do it then it should be
> done only in 2.6.12 or later, after good exposure in -mm.
> 
> (Worst-case this could break an application but i highly doubt it: it at
> most could deny renicing another task to positive (or in very rare
> cases, to negative) nice values, which no application should crash on
> something like that, normally.)

Yes, it's fairly benign.  Especially as most common use of this would be 
tasks calling it on themselves.  I'm not going to lose any sleep over it :-)

Peter
-- 
Peter Williams                                   pwil3058@bigpond.net.au

"Learning, n. The kind of ignorance distinguishing the studious."
  -- Ambrose Bierce