* [PATCH 7/9] user namespace: make each net (net_ns) belong to a user_ns [not found] <1318974898-21431-1-git-send-email-serge@hallyn.com> @ 2011-10-18 21:54 ` Serge Hallyn 2011-10-18 21:54 ` [PATCH 8/9] protect cap_netlink_recv from user namespaces Serge Hallyn 2011-10-18 21:54 ` [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware Serge Hallyn 2 siblings, 0 replies; 13+ messages in thread From: Serge Hallyn @ 2011-10-18 21:54 UTC (permalink / raw) To: linux-kernel Cc: ebiederm, akpm, oleg, richard, mikevs, segoon, gregkh, dhowells, eparis, Serge E. Hallyn, netdev From: "Serge E. Hallyn" <serge.hallyn@canonical.com> The user namespace which creates a new network namespace owns that namespace and all resources created in it. This way we can target capability checks for privileged operations against network resources to the user_ns which created the network namespace in which the resource lives. Privilege to the user namespace which owns the network namespace, or any parent user namespace thereof, provides the same privilege to the network resource. Changelog: jul 8: nsproxy: don't assign netns->userns if not cloning. Signed-off-by: Serge E. Hallyn <serge.hallyn@canonical.com> Cc: Eric W. Biederman <ebiederm@xmission.com> Cc: netdev@vger.kernel.org --- include/net/net_namespace.h | 2 ++ kernel/nsproxy.c | 2 ++ net/core/net_namespace.c | 3 +++ 3 files changed, 7 insertions(+), 0 deletions(-) diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h index 3bb6fa0..d91fe5f 100644 --- a/include/net/net_namespace.h +++ b/include/net/net_namespace.h @@ -29,6 +29,7 @@ struct ctl_table_header; struct net_generic; struct sock; struct netns_ipvs; +struct user_namespace; #define NETDEV_HASHBITS 8 @@ -101,6 +102,7 @@ struct net { struct netns_xfrm xfrm; #endif struct netns_ipvs *ipvs; + struct user_namespace *user_ns; }; diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c index 9aeab4b..0d5bf8d 100644 --- a/kernel/nsproxy.c +++ b/kernel/nsproxy.c @@ -95,6 +95,8 @@ static struct nsproxy *create_new_namespaces(unsigned long flags, err = PTR_ERR(new_nsp->net_ns); goto out_net; } + if (flags & CLONE_NEWNET) + new_nsp->net_ns->user_ns = get_user_ns(task_cred_xxx(tsk, user_ns)); return new_nsp; diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c index 5bbdbf0..791c19c 100644 --- a/net/core/net_namespace.c +++ b/net/core/net_namespace.c @@ -10,6 +10,7 @@ #include <linux/nsproxy.h> #include <linux/proc_fs.h> #include <linux/file.h> +#include <linux/user_namespace.h> #include <net/net_namespace.h> #include <net/netns/generic.h> @@ -209,6 +210,7 @@ static void net_free(struct net *net) } #endif kfree(net->gen); + put_user_ns(net->user_ns); kmem_cache_free(net_cachep, net); } @@ -389,6 +391,7 @@ static int __init net_ns_init(void) rcu_assign_pointer(init_net.gen, ng); mutex_lock(&net_mutex); + init_net.user_ns = &init_user_ns; if (setup_net(&init_net)) panic("Could not setup the initial network namespace"); -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 8/9] protect cap_netlink_recv from user namespaces [not found] <1318974898-21431-1-git-send-email-serge@hallyn.com> 2011-10-18 21:54 ` [PATCH 7/9] user namespace: make each net (net_ns) belong to a user_ns Serge Hallyn @ 2011-10-18 21:54 ` Serge Hallyn 2011-10-18 21:54 ` [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware Serge Hallyn 2 siblings, 0 replies; 13+ messages in thread From: Serge Hallyn @ 2011-10-18 21:54 UTC (permalink / raw) To: linux-kernel Cc: ebiederm, akpm, oleg, richard, mikevs, segoon, gregkh, dhowells, eparis, Serge E. Hallyn, netdev From: "Serge E. Hallyn" <serge.hallyn@canonical.com> cap_netlink_recv() was granting privilege if a capability is in current_cap(), regardless of the user namespace. Fix that by targeting the capability check against the user namespace which owns the skb. Caller passes the user ns down because sock_net is static inline defined in net/sock.h, which we'd rather not #include at the cap_netlink_recv function. Signed-off-by: Serge E. Hallyn <serge.hallyn@canonical.com> Cc: Eric W. Biederman <ebiederm@xmission.com> Cc: netdev@vger.kernel.org --- drivers/scsi/scsi_netlink.c | 3 ++- include/linux/security.h | 14 +++++++++----- kernel/audit.c | 6 ++++-- net/core/rtnetlink.c | 3 ++- net/decnet/netfilter/dn_rtmsg.c | 3 ++- net/ipv4/netfilter/ip_queue.c | 3 ++- net/ipv6/netfilter/ip6_queue.c | 3 ++- net/netfilter/nfnetlink.c | 2 +- net/netlink/genetlink.c | 2 +- net/xfrm/xfrm_user.c | 2 +- security/commoncap.c | 6 ++---- security/security.c | 4 ++-- security/selinux/hooks.c | 5 +++-- 13 files changed, 33 insertions(+), 23 deletions(-) diff --git a/drivers/scsi/scsi_netlink.c b/drivers/scsi/scsi_netlink.c index 26a8a45..0aa2e57 100644 --- a/drivers/scsi/scsi_netlink.c +++ b/drivers/scsi/scsi_netlink.c @@ -111,7 +111,8 @@ scsi_nl_rcv_msg(struct sk_buff *skb) goto next_msg; } - if (security_netlink_recv(skb, CAP_SYS_ADMIN)) { + if (security_netlink_recv(skb, CAP_SYS_ADMIN, + sock_net(skb->sk)->user_ns)) { err = -EPERM; goto next_msg; } diff --git a/include/linux/security.h b/include/linux/security.h index ebd2a53..cfa1f47 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -95,7 +95,8 @@ struct xfrm_user_sec_ctx; struct seq_file; extern int cap_netlink_send(struct sock *sk, struct sk_buff *skb); -extern int cap_netlink_recv(struct sk_buff *skb, int cap); +extern int cap_netlink_recv(struct sk_buff *skb, int cap, + struct user_namespace *ns); void reset_security_ops(void); @@ -797,6 +798,7 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts) * @skb. * @skb contains the sk_buff structure for the netlink message. * @cap indicates the capability required + * @ns is the user namespace which owns skb * Return 0 if permission is granted. * * Security hooks for Unix domain networking. @@ -1557,7 +1559,8 @@ struct security_operations { struct sembuf *sops, unsigned nsops, int alter); int (*netlink_send) (struct sock *sk, struct sk_buff *skb); - int (*netlink_recv) (struct sk_buff *skb, int cap); + int (*netlink_recv) (struct sk_buff *skb, int cap, + struct user_namespace *ns); void (*d_instantiate) (struct dentry *dentry, struct inode *inode); @@ -1806,7 +1809,7 @@ void security_d_instantiate(struct dentry *dentry, struct inode *inode); int security_getprocattr(struct task_struct *p, char *name, char **value); int security_setprocattr(struct task_struct *p, char *name, void *value, size_t size); int security_netlink_send(struct sock *sk, struct sk_buff *skb); -int security_netlink_recv(struct sk_buff *skb, int cap); +int security_netlink_recv(struct sk_buff *skb, int cap, struct user_namespace *ns); int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen); int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid); void security_release_secctx(char *secdata, u32 seclen); @@ -2498,9 +2501,10 @@ static inline int security_netlink_send(struct sock *sk, struct sk_buff *skb) return cap_netlink_send(sk, skb); } -static inline int security_netlink_recv(struct sk_buff *skb, int cap) +static inline int security_netlink_recv(struct sk_buff *skb, int cap, + struct user_namespace *ns) { - return cap_netlink_recv(skb, cap); + return cap_netlink_recv(skb, cap, ns); } static inline int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen) diff --git a/kernel/audit.c b/kernel/audit.c index 0a1355c..48144c4 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -601,13 +601,15 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type) case AUDIT_TTY_SET: case AUDIT_TRIM: case AUDIT_MAKE_EQUIV: - if (security_netlink_recv(skb, CAP_AUDIT_CONTROL)) + if (security_netlink_recv(skb, CAP_AUDIT_CONTROL, + sock_net(skb->sk)->user_ns)) err = -EPERM; break; case AUDIT_USER: case AUDIT_FIRST_USER_MSG ... AUDIT_LAST_USER_MSG: case AUDIT_FIRST_USER_MSG2 ... AUDIT_LAST_USER_MSG2: - if (security_netlink_recv(skb, CAP_AUDIT_WRITE)) + if (security_netlink_recv(skb, CAP_AUDIT_WRITE, + sock_net(skb->sk)->user_ns)) err = -EPERM; break; default: /* bad msg */ diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 99d9e95..4a444de 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -1931,7 +1931,8 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) sz_idx = type>>2; kind = type&3; - if (kind != 2 && security_netlink_recv(skb, CAP_NET_ADMIN)) + if (kind != 2 && security_netlink_recv(skb, CAP_NET_ADMIN, + net->user_ns)) return -EPERM; if (kind == 2 && nlh->nlmsg_flags&NLM_F_DUMP) { diff --git a/net/decnet/netfilter/dn_rtmsg.c b/net/decnet/netfilter/dn_rtmsg.c index 69975e0..2d052ab 100644 --- a/net/decnet/netfilter/dn_rtmsg.c +++ b/net/decnet/netfilter/dn_rtmsg.c @@ -108,7 +108,8 @@ static inline void dnrmg_receive_user_skb(struct sk_buff *skb) if (nlh->nlmsg_len < sizeof(*nlh) || skb->len < nlh->nlmsg_len) return; - if (security_netlink_recv(skb, CAP_NET_ADMIN)) + if (security_netlink_recv(skb, CAP_NET_ADMIN, + sock_net(skb->sk)->user_ns)) RCV_SKB_FAIL(-EPERM); /* Eventually we might send routing messages too */ diff --git a/net/ipv4/netfilter/ip_queue.c b/net/ipv4/netfilter/ip_queue.c index e59aabd..d20bede 100644 --- a/net/ipv4/netfilter/ip_queue.c +++ b/net/ipv4/netfilter/ip_queue.c @@ -430,7 +430,8 @@ __ipq_rcv_skb(struct sk_buff *skb) if (type <= IPQM_BASE) return; - if (security_netlink_recv(skb, CAP_NET_ADMIN)) + if (security_netlink_recv(skb, CAP_NET_ADMIN, + sock_net(skb->sk)->user_ns)) RCV_SKB_FAIL(-EPERM); spin_lock_bh(&queue_lock); diff --git a/net/ipv6/netfilter/ip6_queue.c b/net/ipv6/netfilter/ip6_queue.c index e63c397..09db01c 100644 --- a/net/ipv6/netfilter/ip6_queue.c +++ b/net/ipv6/netfilter/ip6_queue.c @@ -431,7 +431,8 @@ __ipq_rcv_skb(struct sk_buff *skb) if (type <= IPQM_BASE) return; - if (security_netlink_recv(skb, CAP_NET_ADMIN)) + if (security_netlink_recv(skb, CAP_NET_ADMIN, + sock_net(skb->sk)->user_ns)) RCV_SKB_FAIL(-EPERM); spin_lock_bh(&queue_lock); diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c index 1905976..bcaff9d 100644 --- a/net/netfilter/nfnetlink.c +++ b/net/netfilter/nfnetlink.c @@ -130,7 +130,7 @@ static int nfnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) const struct nfnetlink_subsystem *ss; int type, err; - if (security_netlink_recv(skb, CAP_NET_ADMIN)) + if (security_netlink_recv(skb, CAP_NET_ADMIN, net->user_ns)) return -EPERM; /* All the messages must at least contain nfgenmsg */ diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c index 482fa57..00a101c 100644 --- a/net/netlink/genetlink.c +++ b/net/netlink/genetlink.c @@ -516,7 +516,7 @@ static int genl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) return -EOPNOTSUPP; if ((ops->flags & GENL_ADMIN_PERM) && - security_netlink_recv(skb, CAP_NET_ADMIN)) + security_netlink_recv(skb, CAP_NET_ADMIN, net->user_ns)) return -EPERM; if (nlh->nlmsg_flags & NLM_F_DUMP) { diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index 0256b8a..1808e1e 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -2290,7 +2290,7 @@ static int xfrm_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) link = &xfrm_dispatch[type]; /* All operations require privileges, even GET */ - if (security_netlink_recv(skb, CAP_NET_ADMIN)) + if (security_netlink_recv(skb, CAP_NET_ADMIN, net->user_ns)) return -EPERM; if ((type == (XFRM_MSG_GETSA - XFRM_MSG_BASE) || diff --git a/security/commoncap.c b/security/commoncap.c index a93b3b7..1e48e6a 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -56,11 +56,9 @@ int cap_netlink_send(struct sock *sk, struct sk_buff *skb) return 0; } -int cap_netlink_recv(struct sk_buff *skb, int cap) +int cap_netlink_recv(struct sk_buff *skb, int cap, struct user_namespace *ns) { - if (!cap_raised(current_cap(), cap)) - return -EPERM; - return 0; + return security_capable(ns, current_cred(), cap); } EXPORT_SYMBOL(cap_netlink_recv); diff --git a/security/security.c b/security/security.c index 0e4fccf..0a1453e 100644 --- a/security/security.c +++ b/security/security.c @@ -941,9 +941,9 @@ int security_netlink_send(struct sock *sk, struct sk_buff *skb) return security_ops->netlink_send(sk, skb); } -int security_netlink_recv(struct sk_buff *skb, int cap) +int security_netlink_recv(struct sk_buff *skb, int cap, struct user_namespace *ns) { - return security_ops->netlink_recv(skb, cap); + return security_ops->netlink_recv(skb, cap, ns); } EXPORT_SYMBOL(security_netlink_recv); diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 266a229..fe290bb 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4723,13 +4723,14 @@ static int selinux_netlink_send(struct sock *sk, struct sk_buff *skb) return selinux_nlmsg_perm(sk, skb); } -static int selinux_netlink_recv(struct sk_buff *skb, int capability) +static int selinux_netlink_recv(struct sk_buff *skb, int capability, + struct user_namespace *ns) { int err; struct common_audit_data ad; u32 sid; - err = cap_netlink_recv(skb, capability); + err = cap_netlink_recv(skb, capability, ns); if (err) return err; -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware [not found] <1318974898-21431-1-git-send-email-serge@hallyn.com> 2011-10-18 21:54 ` [PATCH 7/9] user namespace: make each net (net_ns) belong to a user_ns Serge Hallyn 2011-10-18 21:54 ` [PATCH 8/9] protect cap_netlink_recv from user namespaces Serge Hallyn @ 2011-10-18 21:54 ` Serge Hallyn 2011-10-18 22:14 ` Joe Perches 2011-10-19 13:52 ` [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware Eric W. Biederman 2 siblings, 2 replies; 13+ messages in thread From: Serge Hallyn @ 2011-10-18 21:54 UTC (permalink / raw) To: linux-kernel Cc: ebiederm, akpm, oleg, richard, mikevs, segoon, gregkh, dhowells, eparis, Serge E. Hallyn, netdev 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. Signed-off-by: Serge E. Hallyn <serge.hallyn@canonical.com> Cc: Eric W. Biederman <ebiederm@xmission.com> Cc: netdev@vger.kernel.org --- net/core/scm.c | 41 ++++++++++++++++++++++++++++++++++------- 1 files changed, 34 insertions(+), 7 deletions(-) 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; -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware 2011-10-18 21:54 ` [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware Serge Hallyn @ 2011-10-18 22:14 ` Joe Perches 2011-10-18 23:22 ` Serge E. Hallyn 2011-10-19 13:52 ` [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware Eric W. Biederman 1 sibling, 1 reply; 13+ messages in thread From: Joe Perches @ 2011-10-18 22:14 UTC (permalink / raw) To: Serge Hallyn Cc: linux-kernel, ebiederm, akpm, oleg, richard, mikevs, segoon, gregkh, dhowells, eparis, Serge E. Hallyn, netdev 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) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware 2011-10-18 22:14 ` Joe Perches @ 2011-10-18 23:22 ` Serge E. Hallyn 2011-10-19 2:25 ` [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware (v2) Serge E. Hallyn 0 siblings, 1 reply; 13+ messages in thread From: Serge E. Hallyn @ 2011-10-18 23:22 UTC (permalink / raw) To: Joe Perches Cc: linux-kernel, ebiederm, akpm, oleg, richard, mikevs, segoon, gregkh, dhowells, eparis, Serge E. Hallyn, netdev 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware (v2) 2011-10-18 23:22 ` Serge E. Hallyn @ 2011-10-19 2:25 ` Serge E. Hallyn 0 siblings, 0 replies; 13+ messages in thread From: Serge E. Hallyn @ 2011-10-19 2:25 UTC (permalink / raw) To: Serge E. Hallyn Cc: Joe Perches, linux-kernel, ebiederm, akpm, oleg, richard, mikevs, segoon, gregkh, dhowells, eparis, netdev (Thanks for the suggestions, Joe.) 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. Changelog: Oct 18: Per Joe Perches: don't mark uidequiv and gidequiv fns inline (let the compiler do that if appropriate), and change the flow of id comparisons to make it clearer. Signed-off-by: Serge E. Hallyn <serge.hallyn@canonical.com> Cc: Eric W. Biederman <ebiederm@xmission.com> Cc: Joe Perches <joe@perches.com> --- net/core/scm.c | 43 ++++++++++++++++++++++++++++++++++++------- 1 files changed, 36 insertions(+), 7 deletions(-) diff --git a/net/core/scm.c b/net/core/scm.c index 811b53f..2261607 100644 --- a/net/core/scm.c +++ b/net/core/scm.c @@ -43,17 +43,46 @@ * setu(g)id. */ -static __inline__ int scm_check_creds(struct ucred *creds) +static bool uidequiv(const struct cred *src, struct ucred *tgt, + struct user_namespace *ns) +{ + if (src->user_ns != ns) + goto check_capable; + if (tgt->uid == src->uid || + tgt->uid == src->euid || + tgt->uid == src->suid) + return true; +check_capable: + if (ns_capable(ns, CAP_SETUID)) + return true; + return false; +} + +static bool gidequiv(const struct cred *src, struct ucred *tgt, + struct user_namespace *ns) +{ + if (src->user_ns != ns) + goto check_capable; + if (tgt->gid == src->gid || + tgt->gid == src->egid || + tgt->gid == src->sgid) + return true; +check_capable: + if (ns_capable(ns, CAP_SETGID)) + return true; + return false; +} + +static 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 +198,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; -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware 2011-10-18 21:54 ` [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware Serge Hallyn 2011-10-18 22:14 ` Joe Perches @ 2011-10-19 13:52 ` Eric W. Biederman 2011-10-20 12:58 ` Serge E. Hallyn 1 sibling, 1 reply; 13+ messages in thread From: Eric W. Biederman @ 2011-10-19 13:52 UTC (permalink / raw) To: Serge Hallyn Cc: linux-kernel, akpm, oleg, richard, mikevs, segoon, gregkh, dhowells, eparis, Serge E. Hallyn, netdev 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; } ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware 2011-10-19 13:52 ` [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware Eric W. Biederman @ 2011-10-20 12:58 ` Serge E. Hallyn 2011-10-20 13:35 ` Eric W. Biederman 0 siblings, 1 reply; 13+ messages in thread From: Serge E. Hallyn @ 2011-10-20 12:58 UTC (permalink / raw) To: Eric W. Biederman Cc: linux-kernel, akpm, oleg, richard, mikevs, segoon, gregkh, dhowells, eparis, Serge E. Hallyn, netdev 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; > } ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware 2011-10-20 12:58 ` Serge E. Hallyn @ 2011-10-20 13:35 ` Eric W. Biederman 2011-10-20 14:14 ` Serge E. Hallyn 2011-10-20 14:24 ` Serge E. Hallyn 0 siblings, 2 replies; 13+ messages in thread From: Eric W. Biederman @ 2011-10-20 13:35 UTC (permalink / raw) To: Serge E. Hallyn Cc: linux-kernel, akpm, oleg, richard, mikevs, segoon, gregkh, dhowells, eparis, Serge E. Hallyn, netdev "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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware 2011-10-20 13:35 ` Eric W. Biederman @ 2011-10-20 14:14 ` Serge E. Hallyn 2011-10-24 4:15 ` Serge E. Hallyn 2011-10-20 14:24 ` Serge E. Hallyn 1 sibling, 1 reply; 13+ messages in thread From: Serge E. Hallyn @ 2011-10-20 14:14 UTC (permalink / raw) To: Eric W. Biederman Cc: Serge E. Hallyn, linux-kernel, akpm, oleg, richard, mikevs, segoon, gregkh, dhowells, eparis, netdev 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware 2011-10-20 14:14 ` Serge E. Hallyn @ 2011-10-24 4:15 ` Serge E. Hallyn 2011-10-24 4:27 ` Eric W. Biederman 0 siblings, 1 reply; 13+ messages in thread From: Serge E. Hallyn @ 2011-10-24 4:15 UTC (permalink / raw) To: Serge E. Hallyn Cc: Eric W. Biederman, linux-kernel, akpm, oleg, richard, mikevs, segoon, gregkh, dhowells, eparis, netdev 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware 2011-10-24 4:15 ` Serge E. Hallyn @ 2011-10-24 4:27 ` Eric W. Biederman 0 siblings, 0 replies; 13+ messages in thread From: Eric W. Biederman @ 2011-10-24 4:27 UTC (permalink / raw) To: Serge E. Hallyn Cc: Serge E. Hallyn, linux-kernel, akpm, oleg, richard, mikevs, segoon, gregkh, dhowells, eparis, netdev "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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware 2011-10-20 13:35 ` Eric W. Biederman 2011-10-20 14:14 ` Serge E. Hallyn @ 2011-10-20 14:24 ` Serge E. Hallyn 1 sibling, 0 replies; 13+ messages in thread From: Serge E. Hallyn @ 2011-10-20 14:24 UTC (permalink / raw) To: Eric W. Biederman Cc: linux-kernel, akpm, oleg, richard, mikevs, segoon, gregkh, dhowells, eparis, Serge E. Hallyn, netdev 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2011-10-24 4:27 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1318974898-21431-1-git-send-email-serge@hallyn.com>
2011-10-18 21:54 ` [PATCH 7/9] user namespace: make each net (net_ns) belong to a user_ns Serge Hallyn
2011-10-18 21:54 ` [PATCH 8/9] protect cap_netlink_recv from user namespaces Serge Hallyn
2011-10-18 21:54 ` [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware Serge Hallyn
2011-10-18 22:14 ` Joe Perches
2011-10-18 23:22 ` Serge E. Hallyn
2011-10-19 2:25 ` [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware (v2) Serge E. Hallyn
2011-10-19 13:52 ` [PATCH 9/9] make net/core/scm.c uid comparisons user namespace aware Eric W. Biederman
2011-10-20 12:58 ` Serge E. Hallyn
2011-10-20 13:35 ` Eric W. Biederman
2011-10-20 14:14 ` Serge E. Hallyn
2011-10-24 4:15 ` Serge E. Hallyn
2011-10-24 4:27 ` Eric W. Biederman
2011-10-20 14:24 ` Serge E. Hallyn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).