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

[Andy Lutomirski]

On Wed, Oct 23, 2013 at 10:59 PM, Eric W. Biederman
<ebiederm@xxxxxxxxxxxx> wrote:
> Andy Lutomirski <luto@xxxxxxxxxxxxxx> writes:
>
>> On 10/16/2013 08:52 PM, Eric W. Biederman wrote:
>>> Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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.

Here's the attack:

Suppose there's a daemon that uses this in conjunction with
SCM_RIGHTS.  The daemon is effectively root (under the current
proposal, it has to be).  So a client connects, sends a credential fd,
and the daemon impersonates those credentials.

Now a malicious user obtains *any* fd opened by root.  It sends that
fd to the daemon.  The daemon then impersonates root.  We lose.  (It
can't possibly be hard to obtain an fd with highly privileged f_cred
-- I bet that most console programs have stdin like that, for example.
 There are probably many setuid programs that will happily open
/dev/null for you, too.)

>
>> 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.

IMO, there are two reasonable models that involve fds carrying some
kind of credential.

1. The fd actually carries the ability to use the credentials.  You
need to be very careful whom you send these to.  The security
implications are obvious (which is good) and the receiver doesn't need
privilege (which is good, as long as the receiver is careful).

2. The fd is for identification only.  But this means that the fd
carries the ability to identify as a user.  So you *still* have to be
very careful about whom you send it to.  What you need is an operation
that allows you to identify using the fd without transitively granting
the recipient the same ability.  On networks, this is done by signing
some kind of challenge.  The kernel could work the same way, or there
could be a new CMSG_IDENTITY that you need an identity fd to send but
that does not copy that fd to the recipient.

>
>> 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.

If the daemon impersonates and execs, we still lose.


--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/