| Message ID | 1318974898-21431-10-git-send-email-serge@hallyn.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 2011-10-18 at 21:54 +0000, Serge Hallyn wrote: > From: "Serge E. Hallyn" <serge.hallyn@canonical.com> Hi Serge. Just some trivial style notes. > Currently uids are compared without regard for the user namespace. > Fix that to prevent tasks in a different user namespace from > wrongly matching on SCM_CREDENTIALS. [] > diff --git a/net/core/scm.c b/net/core/scm.c > -static __inline__ int scm_check_creds(struct ucred *creds) > +static __inline__ bool uidequiv(const struct cred *src, struct ucred *tgt, > + struct user_namespace *ns) Perhaps inline is better than __inline__ and do these functions really need to be marked inline at all? > +{ > + if (src->user_ns != ns) > + goto check_capable; > + if (src->uid == tgt->uid || src->euid == tgt->uid || > + src->suid == tgt->uid) Perhaps this is less prone to typo errors and are a bit more readable as: if (tgt->uid == src->uid || tgt->uid == src->euid || tgt->uid == src->suid) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Joe Perches (joe@perches.com): > On Tue, 2011-10-18 at 21:54 +0000, Serge Hallyn wrote: > > From: "Serge E. Hallyn" <serge.hallyn@canonical.com> > > Hi Serge. > > Just some trivial style notes. > > > Currently uids are compared without regard for the user namespace. > > Fix that to prevent tasks in a different user namespace from > > wrongly matching on SCM_CREDENTIALS. > [] > > diff --git a/net/core/scm.c b/net/core/scm.c > > > -static __inline__ int scm_check_creds(struct ucred *creds) > > +static __inline__ bool uidequiv(const struct cred *src, struct ucred *tgt, > > + struct user_namespace *ns) > > Perhaps inline is better than __inline__ and do these > functions really need to be marked inline at all? Dunno, I was just sticking with the current style. > > +{ > > + if (src->user_ns != ns) > > + goto check_capable; > > + if (src->uid == tgt->uid || src->euid == tgt->uid || > > + src->suid == tgt->uid) > > Perhaps this is less prone to typo errors and are a bit > more readable as: > > if (tgt->uid == src->uid || > tgt->uid == src->euid || > tgt->uid == src->suid) I do like that better. thanks, -serge -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Serge Hallyn <serge@hallyn.com> writes: > From: "Serge E. Hallyn" <serge.hallyn@canonical.com> > > Currently uids are compared without regard for the user namespace. > Fix that to prevent tasks in a different user namespace from > wrongly matching on SCM_CREDENTIALS. > > In the past, either your uids had to match, or you had to have > CAP_SETXID. In a namespaced world, you must either (both be in the > same user namespace and have your uids match), or you must have > CAP_SETXID targeted at the other user namespace. The latter can > happen for instance if uid 500 created a new user namespace and > now interacts with uid 0 in it. Serge this approach is wrong. Because we pass the cred and the pid through the socket socket itself is just a conduit and should be ignored in this context. The only interesting test should be are you allowed to impersonate other users in your current userk namespace. So it should be possible to simplify the entire patch to just: static __inline__ int scm_check_creds(struct ucred *creds) { const struct cred *cred = current_cred(); + struct user_namespace *ns = cred->user_ns; - if ((creds->pid == task_tgid_vnr(current) || capable(CAP_SYS_ADMIN)) && - ((creds->uid == cred->uid || creds->uid == cred->euid || - creds->uid == cred->suid) || capable(CAP_SETUID)) && - ((creds->gid == cred->gid || creds->gid == cred->egid || - creds->gid == cred->sgid) || capable(CAP_SETGID))) { + if ((creds->pid == task_tgid_vnr(current) || ns_capable(ns, CAP_SYS_ADMIN)) && + ((creds->uid == cred->uid || creds->uid == cred->euid || + creds->uid == cred->suid) || ns_capable(ns, CAP_SETUID)) && + ((creds->gid == cred->gid || creds->gid == cred->egid || + creds->gid == cred->sgid) || ns_capable(ns, CAP_SETGID))) { return 0; } return -EPERM; } -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Eric W. Biederman (ebiederm@xmission.com): > Serge Hallyn <serge@hallyn.com> writes: > > > From: "Serge E. Hallyn" <serge.hallyn@canonical.com> > > > > Currently uids are compared without regard for the user namespace. > > Fix that to prevent tasks in a different user namespace from > > wrongly matching on SCM_CREDENTIALS. > > > > In the past, either your uids had to match, or you had to have > > CAP_SETXID. In a namespaced world, you must either (both be in the > > same user namespace and have your uids match), or you must have > > CAP_SETXID targeted at the other user namespace. The latter can > > happen for instance if uid 500 created a new user namespace and > > now interacts with uid 0 in it. > > Serge this approach is wrong. Thanks for looking, Eric. > Because we pass the cred and the pid through the socket socket itself > is just a conduit and should be ignored in this context. Ok, that makes sense, but > The only interesting test should be are you allowed to impersonate other > users in your current userk namespace. Why in your current user namespace? Shouldn't it be in the target user ns? I understand it could be wrong to tie the user ns owning the socket to the target userns (though I still kind of like it), but just because I have CAP_SETUID in my own user_ns doesn't mean I should be able to pose as another uid in your user_ns. (Now I also see that cred_to_ucred() translates to the current user_ns, so that should have been a hint to me before about your intent, but I'm not convinced I agree with your intent). And you do the same with the pid. Why is that a valid assumption? (I've got that feeling that I'll feel like a dunce once you explain :) > So it should be possible to simplify the entire patch to just: > static __inline__ int scm_check_creds(struct ucred *creds) > { > const struct cred *cred = current_cred(); > + struct user_namespace *ns = cred->user_ns; > > - if ((creds->pid == task_tgid_vnr(current) || capable(CAP_SYS_ADMIN)) && > - ((creds->uid == cred->uid || creds->uid == cred->euid || > - creds->uid == cred->suid) || capable(CAP_SETUID)) && > - ((creds->gid == cred->gid || creds->gid == cred->egid || > - creds->gid == cred->sgid) || capable(CAP_SETGID))) { > + if ((creds->pid == task_tgid_vnr(current) || ns_capable(ns, CAP_SYS_ADMIN)) && > + ((creds->uid == cred->uid || creds->uid == cred->euid || > + creds->uid == cred->suid) || ns_capable(ns, CAP_SETUID)) && > + ((creds->gid == cred->gid || creds->gid == cred->egid || > + creds->gid == cred->sgid) || ns_capable(ns, CAP_SETGID))) { > return 0; > } > return -EPERM; > } -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
"Serge E. Hallyn" <serge@hallyn.com> writes: > Quoting Eric W. Biederman (ebiederm@xmission.com): >> Serge Hallyn <serge@hallyn.com> writes: >> >> > From: "Serge E. Hallyn" <serge.hallyn@canonical.com> >> > >> > Currently uids are compared without regard for the user namespace. >> > Fix that to prevent tasks in a different user namespace from >> > wrongly matching on SCM_CREDENTIALS. >> > >> > In the past, either your uids had to match, or you had to have >> > CAP_SETXID. In a namespaced world, you must either (both be in the >> > same user namespace and have your uids match), or you must have >> > CAP_SETXID targeted at the other user namespace. The latter can >> > happen for instance if uid 500 created a new user namespace and >> > now interacts with uid 0 in it. >> >> Serge this approach is wrong. > > Thanks for looking, Eric. > >> Because we pass the cred and the pid through the socket socket itself >> is just a conduit and should be ignored in this context. > > Ok, that makes sense, but > >> The only interesting test should be are you allowed to impersonate other >> users in your current userk namespace. > > Why in your current user namespace? Shouldn't it be in the > target user ns? I understand it could be wrong to tie the > user ns owning the socket to the target userns (though I still > kind of like it), but just because I have CAP_SETUID in my > own user_ns doesn't mean I should be able to pose as another > uid in your user_ns. First and foremost it is important that you be able if you have the capability to impersonate other users in your current user namespace. That is what the capability actually controls. None of this allows you to impersonate any user in any other user namespace. The translation between users prevents that. > (Now I also see that cred_to_ucred() translates to the current > user_ns, so that should have been a hint to me before about > your intent, but I'm not convinced I agree with your intent). > > And you do the same with the pid. Why is that a valid assumption? Yes. Basically all the code is allow you to impersonate people you would have been able to impersonate before. If your target is in another namespace you can not fool them. With pids the logic should be a lot clearer. Pretend to be a pid you can see in your current pid namespace. Lookup and convert to struct pid aka the namespace agnostic object. On output return the pid value that the target process will know you as. Ultimately I think we need a ns_capable for the current user namespace instead of a global one. But I don't see any rush to introduce ns_capable here. Eric -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Eric W. Biederman (ebiederm@xmission.com): > "Serge E. Hallyn" <serge@hallyn.com> writes: > > > Quoting Eric W. Biederman (ebiederm@xmission.com): > >> Serge Hallyn <serge@hallyn.com> writes: > >> > >> > From: "Serge E. Hallyn" <serge.hallyn@canonical.com> > >> > > >> > Currently uids are compared without regard for the user namespace. > >> > Fix that to prevent tasks in a different user namespace from > >> > wrongly matching on SCM_CREDENTIALS. > >> > > >> > In the past, either your uids had to match, or you had to have > >> > CAP_SETXID. In a namespaced world, you must either (both be in the > >> > same user namespace and have your uids match), or you must have > >> > CAP_SETXID targeted at the other user namespace. The latter can > >> > happen for instance if uid 500 created a new user namespace and > >> > now interacts with uid 0 in it. > >> > >> Serge this approach is wrong. > > > > Thanks for looking, Eric. > > > >> Because we pass the cred and the pid through the socket socket itself > >> is just a conduit and should be ignored in this context. > > > > Ok, that makes sense, but > > > >> The only interesting test should be are you allowed to impersonate other > >> users in your current userk namespace. > > > > Why in your current user namespace? Shouldn't it be in the > > target user ns? I understand it could be wrong to tie the > > user ns owning the socket to the target userns (though I still > > kind of like it), but just because I have CAP_SETUID in my > > own user_ns doesn't mean I should be able to pose as another > > uid in your user_ns. > > First and foremost it is important that you be able if you have the > capability to impersonate other users in your current user namespace. > That is what the capability actually controls. > > None of this allows you to impersonate any user in any other user > namespace. The translation between users prevents that. > > > (Now I also see that cred_to_ucred() translates to the current > > user_ns, so that should have been a hint to me before about > > your intent, but I'm not convinced I agree with your intent). > > > > And you do the same with the pid. Why is that a valid assumption? > > Yes. Basically all the code is allow you to impersonate people you > would have been able to impersonate before. If your target is in > another namespace you can not fool them. > > With pids the logic should be a lot clearer. Pretend to be a pid you can > see in your current pid namespace. Lookup and convert to struct pid aka > the namespace agnostic object. On output return the pid value that No. That conversion is happending before the user-specified pid is set. > the target process will know you as. > > Ultimately I think we need a ns_capable for the current user namespace > instead of a global one. But I don't see any rush to introduce > ns_capable here. > > Eric > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Eric W. Biederman (ebiederm@xmission.com): > "Serge E. Hallyn" <serge@hallyn.com> writes: > > > Quoting Eric W. Biederman (ebiederm@xmission.com): > >> Serge Hallyn <serge@hallyn.com> writes: > >> > >> > From: "Serge E. Hallyn" <serge.hallyn@canonical.com> > >> > > >> > Currently uids are compared without regard for the user namespace. > >> > Fix that to prevent tasks in a different user namespace from > >> > wrongly matching on SCM_CREDENTIALS. > >> > > >> > In the past, either your uids had to match, or you had to have > >> > CAP_SETXID. In a namespaced world, you must either (both be in the > >> > same user namespace and have your uids match), or you must have > >> > CAP_SETXID targeted at the other user namespace. The latter can > >> > happen for instance if uid 500 created a new user namespace and > >> > now interacts with uid 0 in it. > >> > >> Serge this approach is wrong. > > > > Thanks for looking, Eric. > > > >> Because we pass the cred and the pid through the socket socket itself > >> is just a conduit and should be ignored in this context. > > > > Ok, that makes sense, but > > > >> The only interesting test should be are you allowed to impersonate other > >> users in your current userk namespace. > > > > Why in your current user namespace? Shouldn't it be in the > > target user ns? I understand it could be wrong to tie the > > user ns owning the socket to the target userns (though I still > > kind of like it), but just because I have CAP_SETUID in my > > own user_ns doesn't mean I should be able to pose as another > > uid in your user_ns. > > First and foremost it is important that you be able if you have the > capability to impersonate other users in your current user namespace. > That is what the capability actually controls. > > None of this allows you to impersonate any user in any other user > namespace. The translation between users prevents that. > > > (Now I also see that cred_to_ucred() translates to the current > > user_ns, so that should have been a hint to me before about > > your intent, but I'm not convinced I agree with your intent). > > > > And you do the same with the pid. Why is that a valid assumption? > > Yes. Basically all the code is allow you to impersonate people you > would have been able to impersonate before. If your target is in > another namespace you can not fool them. > > With pids the logic should be a lot clearer. Pretend to be a pid you can > see in your current pid namespace. Lookup and convert to struct pid aka > the namespace agnostic object. On output return the pid value that > the target process will know you as. > > Ultimately I think we need a ns_capable for the current user namespace > instead of a global one. But I don't see any rush to introduce > ns_capable here. I think I agree - I was mistakenly thinking that without this patch there is an opportunity for a less privileged task in child user ns to impersonate, but that's not possible, so let's drop this patch for now! thanks, -serge -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Serge E. Hallyn (serge.hallyn@canonical.com): > Quoting Eric W. Biederman (ebiederm@xmission.com): > > "Serge E. Hallyn" <serge@hallyn.com> writes: > > > > > Quoting Eric W. Biederman (ebiederm@xmission.com): > > >> Serge Hallyn <serge@hallyn.com> writes: > > >> > > >> > From: "Serge E. Hallyn" <serge.hallyn@canonical.com> > > >> > > > >> > Currently uids are compared without regard for the user namespace. > > >> > Fix that to prevent tasks in a different user namespace from > > >> > wrongly matching on SCM_CREDENTIALS. > > >> > > > >> > In the past, either your uids had to match, or you had to have > > >> > CAP_SETXID. In a namespaced world, you must either (both be in the > > >> > same user namespace and have your uids match), or you must have > > >> > CAP_SETXID targeted at the other user namespace. The latter can > > >> > happen for instance if uid 500 created a new user namespace and > > >> > now interacts with uid 0 in it. > > >> > > >> Serge this approach is wrong. > > > > > > Thanks for looking, Eric. > > > > > >> Because we pass the cred and the pid through the socket socket itself > > >> is just a conduit and should be ignored in this context. > > > > > > Ok, that makes sense, but > > > > > >> The only interesting test should be are you allowed to impersonate other > > >> users in your current userk namespace. > > > > > > Why in your current user namespace? Shouldn't it be in the > > > target user ns? I understand it could be wrong to tie the > > > user ns owning the socket to the target userns (though I still > > > kind of like it), but just because I have CAP_SETUID in my > > > own user_ns doesn't mean I should be able to pose as another > > > uid in your user_ns. > > > > First and foremost it is important that you be able if you have the > > capability to impersonate other users in your current user namespace. > > That is what the capability actually controls. > > > > None of this allows you to impersonate any user in any other user > > namespace. The translation between users prevents that. > > > > > (Now I also see that cred_to_ucred() translates to the current > > > user_ns, so that should have been a hint to me before about > > > your intent, but I'm not convinced I agree with your intent). > > > > > > And you do the same with the pid. Why is that a valid assumption? > > > > Yes. Basically all the code is allow you to impersonate people you > > would have been able to impersonate before. If your target is in > > another namespace you can not fool them. > > > > With pids the logic should be a lot clearer. Pretend to be a pid you can > > see in your current pid namespace. Lookup and convert to struct pid aka > > the namespace agnostic object. On output return the pid value that > > No. That conversion is happending before the user-specified pid is > set. Never mind, it all gets a little convoluted, but I see how it works, and - when the time comes - how to do it right for userns. :) Sorry about that. thanks, -serge -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
"Serge E. Hallyn" <serge@hallyn.com> writes: > Never mind, it all gets a little convoluted, but I see how it works, > and - when the time comes - how to do it right for userns. :) Sorry > about that. No problem. Eric -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/scm.c b/net/core/scm.c index 811b53f..4f376bf 100644 --- a/net/core/scm.c +++ b/net/core/scm.c @@ -43,17 +43,44 @@ * setu(g)id. */ -static __inline__ int scm_check_creds(struct ucred *creds) +static __inline__ bool uidequiv(const struct cred *src, struct ucred *tgt, + struct user_namespace *ns) +{ + if (src->user_ns != ns) + goto check_capable; + if (src->uid == tgt->uid || src->euid == tgt->uid || + src->suid == tgt->uid) + return true; +check_capable: + if (ns_capable(ns, CAP_SETUID)) + return true; + return false; +} + +static __inline__ bool gidequiv(const struct cred *src, struct ucred *tgt, + struct user_namespace *ns) +{ + if (src->user_ns != ns) + goto check_capable; + if (src->gid == tgt->gid || src->egid == tgt->gid || + src->sgid == tgt->gid) + return true; +check_capable: + if (ns_capable(ns, CAP_SETGID)) + return true; + return false; +} + +static __inline__ int scm_check_creds(struct ucred *creds, struct socket *sock) { const struct cred *cred = current_cred(); + struct user_namespace *ns = sock_net(sock->sk)->user_ns; - if ((creds->pid == task_tgid_vnr(current) || capable(CAP_SYS_ADMIN)) && - ((creds->uid == cred->uid || creds->uid == cred->euid || - creds->uid == cred->suid) || capable(CAP_SETUID)) && - ((creds->gid == cred->gid || creds->gid == cred->egid || - creds->gid == cred->sgid) || capable(CAP_SETGID))) { + if ((creds->pid == task_tgid_vnr(current) || ns_capable(ns, CAP_SYS_ADMIN)) && + uidequiv(cred, creds, ns) && gidequiv(cred, creds, ns)) { return 0; } + return -EPERM; } @@ -169,7 +196,7 @@ int __scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *p) if (cmsg->cmsg_len != CMSG_LEN(sizeof(struct ucred))) goto error; memcpy(&p->creds, CMSG_DATA(cmsg), sizeof(struct ucred)); - err = scm_check_creds(&p->creds); + err = scm_check_creds(&p->creds, sock); if (err) goto error;