Re: [PATCH 1/3] switch_creds: Syscall to switch creds for file server ops

[ebiederm@xmission.com (Eric W. Biederman)]

Andy Lutomirski <luto@amacapital.net> writes:

> On 10/16/2013 08:52 PM, Eric W. Biederman wrote:
>> Al Viro <viro@ZenIV.linux.org.uk> writes:
>> 
>>> On Wed, Oct 16, 2013 at 06:18:16PM -0700, Eric W. Biederman wrote:
>>>
>>>> That doesn't look bad but it does need capable(CAP_SETUID) &&
>>>> capable(CAP_SETGID) or possibly something a little more refined.
>>>
>>> D'oh
>>>
>>>> I don't think we want file descriptor passing to all of a sudden become
>>>> a grant of privilege, beyond what the passed fd can do.
>>>
>>> Definitely.  And an extra ) to make it compile wouldn't hurt either...
>> 
>> There also appears to need to be a check that we don't gain any
>> capabilities.
>> 
>> We also need a check so that you don't gain any capabilities, and
>> possibly a few other things.
>
> Why?  I like the user_ns part, but I'm not immediately seeing the issue
> with capabilities.

My reasoning was instead of making this syscall as generic as possible
start it out by only allowing the cases Jim cares about and working with
a model where you can't gain any permissions you couldn't gain
otherwise.

Although the fd -1 trick to revert to your other existing cred seems
reasonable.  

>> So I suspect we want a check something like:
>> 
>> if ((new_cred->securebits != current_cred->securebits)  ||
>>     (new_cred->cap_inheritable != current_cred->cap_inheritable) ||
>>     (new_cred->cap_permitted != current_cred->cap_permitted) ||
>>     (new_cred->cap_effective != current_cred->cap_effective) ||
>>     (new_cred->cap_bset != current_cred->cap_bset) ||
>>     (new_cred->jit_keyring != current_cred->jit_keyring) ||
>>     (new_cred->session_keyring != current_cred->session_keyring) ||
>>     (new_cred->process_keyring != current_cred->process_keyring) ||
>>     (new_cred->thread_keyring != current_cred->thread_keyring) ||
>>     (new_cred->request_keyring != current_cred->request_keyring) ||
>>     (new_cred->security != current_cred->security) ||
>>     (new_cred->user_ns != current_cred->user_ns)) {
>> 	return -EPERM;
>> }
>> 
>
> I *really* don't like the idea of being able to use any old file
> descriptor.  I barely care what rights the caller needs to have to
> invoke this -- if you're going to pass an fd that grants a capability
> (in the non-Linux sense of the work), please make sure that the sender
> actually wants that behavior.
>
> IOW, have a syscall to generate a special fd for this purpose.  It's
> only a couple lines of code, and I think we'll really regret it if we
> fsck this up.
>
> (I will take it as a personal challenge to find at least one exploitable
> privilege escalation in this if an arbitrary fd works.)

If you can't switch to a uid or a gid you couldn't switch to otherwise
then the worst that can happen is an information leak.  And information
leaks are rarely directly exploitable.

> Also... real_cred looks confusing.  AFAICS it is used *only* for knfsd
> and faccessat.  That is, current userspace can't see it.  But now you'll
> expose various oddities.  For example, AFAICS a capability-less process
> that's a userns owner can always use setuid.  This will *overwrite*
> real_cred.  Then you're screwed, especially if this happens by
> accident.

And doing in userland what faccessat, and knfsd do in the kernel is
exactly what is desired here.  But maybe there are issues with that.

> That being said, Windows has had functions like this for a long time.
> Processes have a primary token and possibly an impersonation token.  Any
> process can call ImpersonateLoggedOnUser (no privilege required) to
> impersonate the credentials of a token (which is special kind of fd).
> Similarly, any process can call RevertToSelf to undo it.
>
> Is there any actual problem with allowing completely unprivileged tasks
> to switch to one of these magic cred fds?  That would avoid needing a
> "revert" operation.

If the permission model is this switching of credentials doesn't get you
anything you couldn't get some other way.  That would seem to totally
rules out unprivileged processes switching these things.

> Another note: I think that there may be issues if the creator of a token
> has no_new_privs set and the user doesn't.  Imagine a daemon that
> accepts one of these fds, impersonates it, and calls exec.  This could
> be used to escape from no_new_privs land.

Which is why I was suggesting that we don't allow changing any field in
the cred except for uids and gids.

Eric