Message ID | 06bbac8182aa1054ab8f6392f14e9148def0f561.1401472882.git.luto@amacapital.net |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
From: Andy Lutomirski <luto@amacapital.net> Date: Fri, 30 May 2014 11:04:00 -0700 > From: "Eric W. Biederman" <ebiederm@xmission.com> > > It was possible to get a setuid root or setcap executable to write to > it's stdout or stderr (which has been set made a netlink socket) and > inadvertently reconfigure the networking stack. > > To prevent this we check that both the creator of the socket and > the currentl applications has permission to reconfigure the network > stack. > > Unfortunately this breaks Zebra which always uses sendto/sendmsg > and creates it's socket without any privileges. > > To keep Zebra working don't bother checking if the creator of the > socket has privilege when a destination address is specified. Instead > rely exclusively on the privileges of the sender of the socket. > > Note from Andy: This is exactly Eric's code except for some comment > clarifications and formatting fixes. Neither I nor, I think, anyone > else is thrilled with this approach, but I'm hesitant to wait on a > better fix since 3.15 is almost here. > > Note to stable maintainers: This is a mess. An earlier series of > patches in 3.15 fix a rather serious security issue (CVE-2014-0181), > but they did so in a way that breaks Zebra. The offending series > includes: > > commit aa4cf9452f469f16cea8c96283b641b4576d4a7b > Author: Eric W. Biederman <ebiederm@xmission.com> > Date: Wed Apr 23 14:28:03 2014 -0700 > > net: Add variants of capable for use on netlink messages > > If a given kernel version is missing that series of fixes, it's > probably worth backporting it and this patch. if that series is > present, then this fix is critical if you care about Zebra. > > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > Signed-off-by: Andy Lutomirski <luto@amacapital.net> Applied, thanks Andy. -- 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
On Fri, 30 May 2014 11:04:00 -0700, Andy Lutomirski wrote: > --- a/net/netlink/af_netlink.c > +++ b/net/netlink/af_netlink.c > @@ -1373,7 +1373,9 @@ retry: > bool __netlink_ns_capable(const struct netlink_skb_parms *nsp, > struct user_namespace *user_ns, int cap) > { > - return sk_ns_capable(nsp->sk, user_ns, cap); > + return ((nsp->flags & NETLINK_SKB_DST) || > + file_ns_capable(nsp->sk->sk_socket->file, user_ns, cap)) && > + ns_capable(user_ns, cap); Sorry if I'm missing something. Is socket->file valid (non-NULL) in kernel->kernel netlink communication? I don't think it's assigned for sockets created by netlink_kernel_create, is it? Seems this would cause NULL ptr dereference. But then, I don't even know whether kernel->kernel netlink communication is allowed. (I'm aware that if this is really the case the NULL ptr deref is not caused by this patch but by the one this is fixing.) Thanks, Jiri
diff --git a/include/linux/netlink.h b/include/linux/netlink.h index f64b017..034cda7 100644 --- a/include/linux/netlink.h +++ b/include/linux/netlink.h @@ -16,9 +16,10 @@ static inline struct nlmsghdr *nlmsg_hdr(const struct sk_buff *skb) } enum netlink_skb_flags { - NETLINK_SKB_MMAPED = 0x1, /* Packet data is mmaped */ - NETLINK_SKB_TX = 0x2, /* Packet was sent by userspace */ - NETLINK_SKB_DELIVERED = 0x4, /* Packet was delivered */ + NETLINK_SKB_MMAPED = 0x1, /* Packet data is mmaped */ + NETLINK_SKB_TX = 0x2, /* Packet was sent by userspace */ + NETLINK_SKB_DELIVERED = 0x4, /* Packet was delivered */ + NETLINK_SKB_DST = 0x8, /* Dst set in sendto or sendmsg */ }; struct netlink_skb_parms { diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index 81dca96..f22757a 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -1373,7 +1373,9 @@ retry: bool __netlink_ns_capable(const struct netlink_skb_parms *nsp, struct user_namespace *user_ns, int cap) { - return sk_ns_capable(nsp->sk, user_ns, cap); + return ((nsp->flags & NETLINK_SKB_DST) || + file_ns_capable(nsp->sk->sk_socket->file, user_ns, cap)) && + ns_capable(user_ns, cap); } EXPORT_SYMBOL(__netlink_ns_capable); @@ -2293,6 +2295,7 @@ static int netlink_sendmsg(struct kiocb *kiocb, struct socket *sock, struct sk_buff *skb; int err; struct scm_cookie scm; + u32 netlink_skb_flags = 0; if (msg->msg_flags&MSG_OOB) return -EOPNOTSUPP; @@ -2314,6 +2317,7 @@ static int netlink_sendmsg(struct kiocb *kiocb, struct socket *sock, if ((dst_group || dst_portid) && !netlink_allowed(sock, NL_CFG_F_NONROOT_SEND)) goto out; + netlink_skb_flags |= NETLINK_SKB_DST; } else { dst_portid = nlk->dst_portid; dst_group = nlk->dst_group; @@ -2343,6 +2347,7 @@ static int netlink_sendmsg(struct kiocb *kiocb, struct socket *sock, NETLINK_CB(skb).portid = nlk->portid; NETLINK_CB(skb).dst_group = dst_group; NETLINK_CB(skb).creds = siocb->scm->creds; + NETLINK_CB(skb).flags = netlink_skb_flags; err = -EFAULT; if (memcpy_fromiovec(skb_put(skb, len), msg->msg_iov, len)) {