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. > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html