* [PATCH 0/6]: Preventing abuse when passing file descriptors [not found] ` <87r44qrt8v.fsf_-_@x220.int.ebiederm.org> @ 2014-04-22 21:13 ` Eric W. Biederman 2014-04-22 21:14 ` [PATCH 1/6] netlink: Rename netlink_capable netlink_allowed Eric W. Biederman ` (6 more replies) 0 siblings, 7 replies; 46+ messages in thread From: Eric W. Biederman @ 2014-04-22 21:13 UTC (permalink / raw) To: David S. Miller Cc: Vivek Goyal, Simo Sorce, security, Andy Lutomirski, netdev, Serge E. Hallyn Andy Lutomirski when looking at the networking stack noticed that it is possible to trick privilged processes into calling write on a netlink socket and send netlink messages they did not intend. In particular from time to time there are suid applications that will write to stdout or stderr without checking exactly what kind of file descriptors those are and can be tricked into acting as a limited form of suid cat. In other conversations the magic string CVE-2014-0818 has been used to talk about this issue. This patchset cleans things up a bit, adds some clean abstractions that when used prevent this kind of problem and then finally changes all of the handlers of netlink messages that I could find that call capable to use netlink_ns_capable or an appropriate wrapper. The abstraction netlink_ns_capable verifies that the original creator of the netlink socket a message is sent from had the necessary capabilities as well as verifying that the current sender of a netlink packet has the necessary capabilities. The idea is to prevent file descriptor massing of any form from resulting in a file descriptor that can do more than it can for the creator of the file descriptor. Eric W. Biederman (6): netlink: Rename netlink_capable netlink_allowed net: Move the permission check in sock_diag_put_filterinfo to packet_diag_dump net: Fix ns_capable check in packet_diag_dump net: Add variants of capable for use on on sockets net: Add variants of capable for use on netlink messages net: Use netlink_ns_capable to verify the permisions of netlink messages crypto/crypto_user.c | 2 +- drivers/connector/cn_proc.c | 2 +- drivers/scsi/scsi_netlink.c | 2 +- include/linux/netlink.h | 7 ++++ include/linux/sock_diag.h | 2 +- include/net/sock.h | 5 +++ kernel/audit.c | 4 +-- net/can/gw.c | 4 +-- net/core/rtnetlink.c | 20 ++++++----- net/core/sock.c | 49 +++++++++++++++++++++++++++ net/core/sock_diag.c | 4 +-- net/dcb/dcbnl.c | 2 +- net/decnet/dn_dev.c | 4 +-- net/decnet/dn_fib.c | 4 +-- net/decnet/netfilter/dn_rtmsg.c | 2 +- net/netfilter/nfnetlink.c | 2 +- net/netlink/af_netlink.c | 75 ++++++++++++++++++++++++++++++++++++++--- net/netlink/genetlink.c | 2 +- net/packet/diag.c | 7 +++- net/phonet/pn_netlink.c | 8 ++--- net/sched/act_api.c | 2 +- net/sched/cls_api.c | 2 +- net/sched/sch_api.c | 6 ++-- net/tipc/netlink.c | 2 +- net/xfrm/xfrm_user.c | 2 +- 25 files changed, 177 insertions(+), 44 deletions(-) Eric ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 1/6] netlink: Rename netlink_capable netlink_allowed 2014-04-22 21:13 ` [PATCH 0/6]: Preventing abuse when passing file descriptors Eric W. Biederman @ 2014-04-22 21:14 ` Eric W. Biederman 2014-04-22 21:15 ` [PATCH 2/6] net: Move the permission check in sock_diag_put_filterinfo to packet_diag_dump Eric W. Biederman ` (5 subsequent siblings) 6 siblings, 0 replies; 46+ messages in thread From: Eric W. Biederman @ 2014-04-22 21:14 UTC (permalink / raw) To: David S. Miller Cc: Vivek Goyal, Simo Sorce, security, Andy Lutomirski, netdev, Serge E. Hallyn netlink_capable is a static internal function in af_netlink.c and we have better uses for the name netlink_capable. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- net/netlink/af_netlink.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index 894cda0206bb..7f931fe4d187 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -1360,7 +1360,7 @@ retry: return err; } -static inline int netlink_capable(const struct socket *sock, unsigned int flag) +static inline int netlink_allowed(const struct socket *sock, unsigned int flag) { return (nl_table[sock->sk->sk_protocol].flags & flag) || ns_capable(sock_net(sock->sk)->user_ns, CAP_NET_ADMIN); @@ -1428,7 +1428,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr, /* Only superuser is allowed to listen multicasts */ if (nladdr->nl_groups) { - if (!netlink_capable(sock, NL_CFG_F_NONROOT_RECV)) + if (!netlink_allowed(sock, NL_CFG_F_NONROOT_RECV)) return -EPERM; err = netlink_realloc_groups(sk); if (err) @@ -1490,7 +1490,7 @@ static int netlink_connect(struct socket *sock, struct sockaddr *addr, return -EINVAL; if ((nladdr->nl_groups || nladdr->nl_pid) && - !netlink_capable(sock, NL_CFG_F_NONROOT_SEND)) + !netlink_allowed(sock, NL_CFG_F_NONROOT_SEND)) return -EPERM; if (!nlk->portid) @@ -2096,7 +2096,7 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname, break; case NETLINK_ADD_MEMBERSHIP: case NETLINK_DROP_MEMBERSHIP: { - if (!netlink_capable(sock, NL_CFG_F_NONROOT_RECV)) + if (!netlink_allowed(sock, NL_CFG_F_NONROOT_RECV)) return -EPERM; err = netlink_realloc_groups(sk); if (err) @@ -2247,7 +2247,7 @@ static int netlink_sendmsg(struct kiocb *kiocb, struct socket *sock, dst_group = ffs(addr->nl_groups); err = -EPERM; if ((dst_group || dst_portid) && - !netlink_capable(sock, NL_CFG_F_NONROOT_SEND)) + !netlink_allowed(sock, NL_CFG_F_NONROOT_SEND)) goto out; } else { dst_portid = nlk->dst_portid; -- 1.9.1 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 2/6] net: Move the permission check in sock_diag_put_filterinfo to packet_diag_dump 2014-04-22 21:13 ` [PATCH 0/6]: Preventing abuse when passing file descriptors Eric W. Biederman 2014-04-22 21:14 ` [PATCH 1/6] netlink: Rename netlink_capable netlink_allowed Eric W. Biederman @ 2014-04-22 21:15 ` Eric W. Biederman 2014-04-22 21:15 ` [PATCH 3/6] net: Fix ns_capable check in packet_diag_dump Eric W. Biederman ` (4 subsequent siblings) 6 siblings, 0 replies; 46+ messages in thread From: Eric W. Biederman @ 2014-04-22 21:15 UTC (permalink / raw) To: David S. Miller Cc: Vivek Goyal, Simo Sorce, security, Andy Lutomirski, netdev, Serge E. Hallyn The permission check in sock_diag_put_filterinfo is wrong, and it is so removed from it's sources it is not clear why it is wrong. Move the computation into packet_diag_dump and pass a bool of the result into sock_diag_filterinfo. This does not yet correct the capability check but instead simply moves it to make it clear what is going on. Reported-by: Andy Lutomirski <luto@amacapital.net> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- include/linux/sock_diag.h | 2 +- net/core/sock_diag.c | 4 ++-- net/packet/diag.c | 8 +++++++- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/include/linux/sock_diag.h b/include/linux/sock_diag.h index 54f91d35e5fd..46cca4c06848 100644 --- a/include/linux/sock_diag.h +++ b/include/linux/sock_diag.h @@ -23,7 +23,7 @@ int sock_diag_check_cookie(void *sk, __u32 *cookie); void sock_diag_save_cookie(void *sk, __u32 *cookie); int sock_diag_put_meminfo(struct sock *sk, struct sk_buff *skb, int attr); -int sock_diag_put_filterinfo(struct user_namespace *user_ns, struct sock *sk, +int sock_diag_put_filterinfo(bool may_report_filterinfo, struct sock *sk, struct sk_buff *skb, int attrtype); #endif diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c index d7af18859322..a4216a4c9572 100644 --- a/net/core/sock_diag.c +++ b/net/core/sock_diag.c @@ -49,7 +49,7 @@ int sock_diag_put_meminfo(struct sock *sk, struct sk_buff *skb, int attrtype) } EXPORT_SYMBOL_GPL(sock_diag_put_meminfo); -int sock_diag_put_filterinfo(struct user_namespace *user_ns, struct sock *sk, +int sock_diag_put_filterinfo(bool may_report_filterinfo, struct sock *sk, struct sk_buff *skb, int attrtype) { struct sock_fprog_kern *fprog; @@ -58,7 +58,7 @@ int sock_diag_put_filterinfo(struct user_namespace *user_ns, struct sock *sk, unsigned int flen; int err = 0; - if (!ns_capable(user_ns, CAP_NET_ADMIN)) { + if (!may_report_filterinfo) { nla_reserve(skb, attrtype, 0); return 0; } diff --git a/net/packet/diag.c b/net/packet/diag.c index 533ce4ff108a..f5ad130ee3b8 100644 --- a/net/packet/diag.c +++ b/net/packet/diag.c @@ -128,6 +128,7 @@ static int pdiag_put_fanout(struct packet_sock *po, struct sk_buff *nlskb) static int sk_diag_fill(struct sock *sk, struct sk_buff *skb, struct packet_diag_req *req, + bool may_report_filterinfo, struct user_namespace *user_ns, u32 portid, u32 seq, u32 flags, int sk_ino) { @@ -172,7 +173,8 @@ static int sk_diag_fill(struct sock *sk, struct sk_buff *skb, goto out_nlmsg_trim; if ((req->pdiag_show & PACKET_SHOW_FILTER) && - sock_diag_put_filterinfo(user_ns, sk, skb, PACKET_DIAG_FILTER)) + sock_diag_put_filterinfo(may_report_filterinfo, sk, skb, + PACKET_DIAG_FILTER)) goto out_nlmsg_trim; return nlmsg_end(skb, nlh); @@ -188,9 +190,12 @@ static int packet_diag_dump(struct sk_buff *skb, struct netlink_callback *cb) struct packet_diag_req *req; struct net *net; struct sock *sk; + bool may_report_filterinfo; net = sock_net(skb->sk); req = nlmsg_data(cb->nlh); + may_report_filterinfo = + ns_capable(sk_user_ns(NETLINK_CB(cb->skb).sk), CAP_NET_ADMIN); mutex_lock(&net->packet.sklist_lock); sk_for_each(sk, &net->packet.sklist) { @@ -200,6 +205,7 @@ static int packet_diag_dump(struct sk_buff *skb, struct netlink_callback *cb) goto next; if (sk_diag_fill(sk, skb, req, + may_report_filterinfo, sk_user_ns(NETLINK_CB(cb->skb).sk), NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq, NLM_F_MULTI, -- 1.9.1 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 3/6] net: Fix ns_capable check in packet_diag_dump 2014-04-22 21:13 ` [PATCH 0/6]: Preventing abuse when passing file descriptors Eric W. Biederman 2014-04-22 21:14 ` [PATCH 1/6] netlink: Rename netlink_capable netlink_allowed Eric W. Biederman 2014-04-22 21:15 ` [PATCH 2/6] net: Move the permission check in sock_diag_put_filterinfo to packet_diag_dump Eric W. Biederman @ 2014-04-22 21:15 ` Eric W. Biederman 2014-04-22 21:16 ` [PATCH 4/6] net: Add variants of capable for use on on sockets Eric W. Biederman ` (3 subsequent siblings) 6 siblings, 0 replies; 46+ messages in thread From: Eric W. Biederman @ 2014-04-22 21:15 UTC (permalink / raw) To: David S. Miller Cc: Vivek Goyal, Simo Sorce, security, Andy Lutomirski, netdev, Serge E. Hallyn The caller needs capabilities on the namespace being queried, not on their own namespace. This is a security bug, although it likely has only a minor impact. Reported-by: Andy Lutomirski <luto@amacapital.net> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- net/packet/diag.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/net/packet/diag.c b/net/packet/diag.c index f5ad130ee3b8..b34d0de24091 100644 --- a/net/packet/diag.c +++ b/net/packet/diag.c @@ -194,8 +194,7 @@ static int packet_diag_dump(struct sk_buff *skb, struct netlink_callback *cb) net = sock_net(skb->sk); req = nlmsg_data(cb->nlh); - may_report_filterinfo = - ns_capable(sk_user_ns(NETLINK_CB(cb->skb).sk), CAP_NET_ADMIN); + may_report_filterinfo = ns_capable(net->user_ns, CAP_NET_ADMIN); mutex_lock(&net->packet.sklist_lock); sk_for_each(sk, &net->packet.sklist) { -- 1.9.1 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 4/6] net: Add variants of capable for use on on sockets 2014-04-22 21:13 ` [PATCH 0/6]: Preventing abuse when passing file descriptors Eric W. Biederman ` (2 preceding siblings ...) 2014-04-22 21:15 ` [PATCH 3/6] net: Fix ns_capable check in packet_diag_dump Eric W. Biederman @ 2014-04-22 21:16 ` Eric W. Biederman 2014-04-22 21:16 ` [PATCH 5/6] net: Add variants of capable for use on netlink messages Eric W. Biederman ` (2 subsequent siblings) 6 siblings, 0 replies; 46+ messages in thread From: Eric W. Biederman @ 2014-04-22 21:16 UTC (permalink / raw) To: David S. Miller Cc: Vivek Goyal, Simo Sorce, security, Andy Lutomirski, netdev, Serge E. Hallyn sk_net_capable - The common case, operations that are safe in a network namespace. sk_capable - Operations that are not known to be safe in a network namespace sk_ns_capable - The general case for special cases. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- include/net/sock.h | 5 +++++ net/core/sock.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/include/net/sock.h b/include/net/sock.h index 8338a14e4805..21569cf456ed 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -2255,6 +2255,11 @@ int sock_get_timestampns(struct sock *, struct timespec __user *); int sock_recv_errqueue(struct sock *sk, struct msghdr *msg, int len, int level, int type); +bool sk_ns_capable(const struct sock *sk, + struct user_namespace *user_ns, int cap); +bool sk_capable(const struct sock *sk, int cap); +bool sk_net_capable(const struct sock *sk, int cap); + /* * Enable debug/info messages */ diff --git a/net/core/sock.c b/net/core/sock.c index b4fff008136f..664ee4295b6f 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -145,6 +145,55 @@ static DEFINE_MUTEX(proto_list_mutex); static LIST_HEAD(proto_list); +/** + * sk_ns_capable - General socket capability test + * @sk: Socket to use a capability on or through + * @user_ns: The user namespace of the capability to use + * @cap: The capability to use + * + * Test to see if the opener of the socket had when the socket was + * created and the current process has the capability @cap in the user + * namespace @user_ns. + */ +bool sk_ns_capable(const struct sock *sk, + struct user_namespace *user_ns, int cap) +{ + return file_ns_capable(sk->sk_socket->file, user_ns, cap) && + ns_capable(user_ns, cap); +} +EXPORT_SYMBOL(sk_ns_capable); + +/** + * sk_capable - Socket global capability test + * @sk: Socket to use a capability on or through + * @cap: The global capbility to use + * + * Test to see if the opener of the socket had when the socket was + * created and the current process has the capability @cap in all user + * namespaces. + */ +bool sk_capable(const struct sock *sk, int cap) +{ + return sk_ns_capable(sk, &init_user_ns, cap); +} +EXPORT_SYMBOL(sk_capable); + +/** + * sk_net_capable - Network namespace socket capability test + * @sk: Socket to use a capability on or through + * @cap: The capability to use + * + * Test to see if the opener of the socket had when the socke was created + * and the current process has the capability @cap over the network namespace + * the socket is a member of. + */ +bool sk_net_capable(const struct sock *sk, int cap) +{ + return sk_ns_capable(sk, sock_net(sk)->user_ns, cap); +} +EXPORT_SYMBOL(sk_net_capable); + + #ifdef CONFIG_MEMCG_KMEM int mem_cgroup_sockets_init(struct mem_cgroup *memcg, struct cgroup_subsys *ss) { -- 1.9.1 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 5/6] net: Add variants of capable for use on netlink messages 2014-04-22 21:13 ` [PATCH 0/6]: Preventing abuse when passing file descriptors Eric W. Biederman ` (3 preceding siblings ...) 2014-04-22 21:16 ` [PATCH 4/6] net: Add variants of capable for use on on sockets Eric W. Biederman @ 2014-04-22 21:16 ` Eric W. Biederman 2014-04-22 21:17 ` [PATCH 6/6] net: Use netlink_ns_capable to verify the permisions of " Eric W. Biederman 2014-04-23 19:32 ` [PATCH 0/6]: Preventing abuse when passing file descriptors David Miller 6 siblings, 0 replies; 46+ messages in thread From: Eric W. Biederman @ 2014-04-22 21:16 UTC (permalink / raw) To: David S. Miller Cc: Vivek Goyal, Simo Sorce, security, Andy Lutomirski, netdev, Serge E. Hallyn netlink_net_capable - The common case use, for operations that are safe on a network namespace netlink_capable - For operations that are only known to be safe for the global root netlink_ns_capable - The general case of capable used to handle special cases __netlink_ns_capable - Same as netlink_ns_capable except taking a netlink_skb_parms instead of the skbuff of a netlink message. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- include/linux/netlink.h | 7 ++++++ net/netlink/af_netlink.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/include/linux/netlink.h b/include/linux/netlink.h index aad8eeaf416d..f64b01787ddc 100644 --- a/include/linux/netlink.h +++ b/include/linux/netlink.h @@ -169,4 +169,11 @@ struct netlink_tap { extern int netlink_add_tap(struct netlink_tap *nt); extern int netlink_remove_tap(struct netlink_tap *nt); +bool __netlink_ns_capable(const struct netlink_skb_parms *nsp, + struct user_namespace *ns, int cap); +bool netlink_ns_capable(const struct sk_buff *skb, + struct user_namespace *ns, int cap); +bool netlink_capable(const struct sk_buff *skb, int cap); +bool netlink_net_capable(const struct sk_buff *skb, int cap); + #endif /* __LINUX_NETLINK_H */ diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index 7f931fe4d187..81dca96d2be6 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -1360,6 +1360,71 @@ retry: return err; } +/** + * __netlink_ns_capable - General netlink message capability test + * @nsp: NETLINK_CB of the socket buffer holding a netlink command from userspace. + * @user_ns: The user namespace of the capability to use + * @cap: The capability to use + * + * Test to see if the opener of the socket we received the message + * from had when the netlink socket was created and the sender of the + * message has has the capability @cap in the user namespace @user_ns. + */ +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); +} +EXPORT_SYMBOL(__netlink_ns_capable); + +/** + * netlink_ns_capable - General netlink message capability test + * @skb: socket buffer holding a netlink command from userspace + * @user_ns: The user namespace of the capability to use + * @cap: The capability to use + * + * Test to see if the opener of the socket we received the message + * from had when the netlink socket was created and the sender of the + * message has has the capability @cap in the user namespace @user_ns. + */ +bool netlink_ns_capable(const struct sk_buff *skb, + struct user_namespace *user_ns, int cap) +{ + return __netlink_ns_capable(&NETLINK_CB(skb), user_ns, cap); +} +EXPORT_SYMBOL(netlink_ns_capable); + +/** + * netlink_capable - Netlink global message capability test + * @skb: socket buffer holding a netlink command from userspace + * @cap: The capability to use + * + * Test to see if the opener of the socket we received the message + * from had when the netlink socket was created and the sender of the + * message has has the capability @cap in all user namespaces. + */ +bool netlink_capable(const struct sk_buff *skb, int cap) +{ + return netlink_ns_capable(skb, &init_user_ns, cap); +} +EXPORT_SYMBOL(netlink_capable); + +/** + * netlink_net_capable - Netlink network namespace message capability test + * @skb: socket buffer holding a netlink command from userspace + * @cap: The capability to use + * + * Test to see if the opener of the socket we received the message + * from had when the netlink socket was created and the sender of the + * message has has the capability @cap over the network namespace of + * the socket we received the message from. + */ +bool netlink_net_capable(const struct sk_buff *skb, int cap) +{ + return netlink_ns_capable(skb, sock_net(skb->sk)->user_ns, cap); +} +EXPORT_SYMBOL(netlink_net_capable); + static inline int netlink_allowed(const struct socket *sock, unsigned int flag) { return (nl_table[sock->sk->sk_protocol].flags & flag) || -- 1.9.1 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 6/6] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-04-22 21:13 ` [PATCH 0/6]: Preventing abuse when passing file descriptors Eric W. Biederman ` (4 preceding siblings ...) 2014-04-22 21:16 ` [PATCH 5/6] net: Add variants of capable for use on netlink messages Eric W. Biederman @ 2014-04-22 21:17 ` Eric W. Biederman 2014-04-23 19:32 ` [PATCH 0/6]: Preventing abuse when passing file descriptors David Miller 6 siblings, 0 replies; 46+ messages in thread From: Eric W. Biederman @ 2014-04-22 21:17 UTC (permalink / raw) To: David S. Miller Cc: Vivek Goyal, Simo Sorce, security, Andy Lutomirski, netdev, Serge E. Hallyn It is possible by passing a netlink socket to a more privileged executable and then to fool that executable into writing to the socket data that happens to be valid netlink message to do something that privileged executable did not intend to do. To keep this from happening replace bare capable and ns_capable calls with netlink_capable, netlink_net_calls and netlink_ns_capable calls. Which act the same as the previous calls except they verify that the opener of the socket had the desired permissions as well. Reported-by: Andy Lutomirski <luto@amacapital.net> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- crypto/crypto_user.c | 2 +- drivers/connector/cn_proc.c | 2 +- drivers/scsi/scsi_netlink.c | 2 +- kernel/audit.c | 4 ++-- net/can/gw.c | 4 ++-- net/core/rtnetlink.c | 20 +++++++++++--------- net/dcb/dcbnl.c | 2 +- net/decnet/dn_dev.c | 4 ++-- net/decnet/dn_fib.c | 4 ++-- net/decnet/netfilter/dn_rtmsg.c | 2 +- net/netfilter/nfnetlink.c | 2 +- net/netlink/genetlink.c | 2 +- net/packet/diag.c | 2 +- net/phonet/pn_netlink.c | 8 ++++---- net/sched/act_api.c | 2 +- net/sched/cls_api.c | 2 +- net/sched/sch_api.c | 6 +++--- net/tipc/netlink.c | 2 +- net/xfrm/xfrm_user.c | 2 +- 19 files changed, 38 insertions(+), 36 deletions(-) diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c index 1512e41cd93d..43665d0d0905 100644 --- a/crypto/crypto_user.c +++ b/crypto/crypto_user.c @@ -466,7 +466,7 @@ static int crypto_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) type -= CRYPTO_MSG_BASE; link = &crypto_dispatch[type]; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if ((type == (CRYPTO_MSG_GETALG - CRYPTO_MSG_BASE) && diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c index 148d707a1d43..ccdd4c7e748b 100644 --- a/drivers/connector/cn_proc.c +++ b/drivers/connector/cn_proc.c @@ -369,7 +369,7 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg, return; /* Can only change if privileged. */ - if (!capable(CAP_NET_ADMIN)) { + if (!__netlink_ns_capable(nsp, &init_user_ns, CAP_NET_ADMIN)) { err = EPERM; goto out; } diff --git a/drivers/scsi/scsi_netlink.c b/drivers/scsi/scsi_netlink.c index fe30ea94ffe6..109802f776ed 100644 --- a/drivers/scsi/scsi_netlink.c +++ b/drivers/scsi/scsi_netlink.c @@ -77,7 +77,7 @@ scsi_nl_rcv_msg(struct sk_buff *skb) goto next_msg; } - if (!capable(CAP_SYS_ADMIN)) { + if (!netlink_capable(skb, CAP_SYS_ADMIN)) { err = -EPERM; goto next_msg; } diff --git a/kernel/audit.c b/kernel/audit.c index 7c2893602d06..47845c57eb19 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -643,13 +643,13 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type) if ((task_active_pid_ns(current) != &init_pid_ns)) return -EPERM; - if (!capable(CAP_AUDIT_CONTROL)) + if (!netlink_capable(skb, CAP_AUDIT_CONTROL)) 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 (!capable(CAP_AUDIT_WRITE)) + if (!netlink_capable(skb, CAP_AUDIT_WRITE)) err = -EPERM; break; default: /* bad msg */ diff --git a/net/can/gw.c b/net/can/gw.c index ac31891967da..050a2110d43f 100644 --- a/net/can/gw.c +++ b/net/can/gw.c @@ -804,7 +804,7 @@ static int cgw_create_job(struct sk_buff *skb, struct nlmsghdr *nlh) u8 limhops = 0; int err = 0; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (nlmsg_len(nlh) < sizeof(*r)) @@ -893,7 +893,7 @@ static int cgw_remove_job(struct sk_buff *skb, struct nlmsghdr *nlh) u8 limhops = 0; int err = 0; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (nlmsg_len(nlh) < sizeof(*r)) diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index d4ff41739b0f..64ad17d077ed 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -1395,7 +1395,8 @@ static int do_set_master(struct net_device *dev, int ifindex) return 0; } -static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm, +static int do_setlink(const struct sk_buff *skb, + struct net_device *dev, struct ifinfomsg *ifm, struct nlattr **tb, char *ifname, int modified) { const struct net_device_ops *ops = dev->netdev_ops; @@ -1407,7 +1408,7 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm, err = PTR_ERR(net); goto errout; } - if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) { + if (!netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN)) { err = -EPERM; goto errout; } @@ -1661,7 +1662,7 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh) if (err < 0) goto errout; - err = do_setlink(dev, ifm, tb, ifname, 0); + err = do_setlink(skb, dev, ifm, tb, ifname, 0); errout: return err; } @@ -1778,7 +1779,8 @@ err: } EXPORT_SYMBOL(rtnl_create_link); -static int rtnl_group_changelink(struct net *net, int group, +static int rtnl_group_changelink(const struct sk_buff *skb, + struct net *net, int group, struct ifinfomsg *ifm, struct nlattr **tb) { @@ -1787,7 +1789,7 @@ static int rtnl_group_changelink(struct net *net, int group, for_each_netdev(net, dev) { if (dev->group == group) { - err = do_setlink(dev, ifm, tb, NULL, 0); + err = do_setlink(skb, dev, ifm, tb, NULL, 0); if (err < 0) return err; } @@ -1929,12 +1931,12 @@ replay: modified = 1; } - return do_setlink(dev, ifm, tb, ifname, modified); + return do_setlink(skb, dev, ifm, tb, ifname, modified); } if (!(nlh->nlmsg_flags & NLM_F_CREATE)) { if (ifm->ifi_index == 0 && tb[IFLA_GROUP]) - return rtnl_group_changelink(net, + return rtnl_group_changelink(skb, net, nla_get_u32(tb[IFLA_GROUP]), ifm, tb); return -ENODEV; @@ -2321,7 +2323,7 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh) int err = -EINVAL; __u8 *addr; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX, NULL); @@ -2773,7 +2775,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) sz_idx = type>>2; kind = type&3; - if (kind != 2 && !ns_capable(net->user_ns, CAP_NET_ADMIN)) + if (kind != 2 && !netlink_net_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (kind == 2 && nlh->nlmsg_flags&NLM_F_DUMP) { diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c index 553644402670..f8b98d89c285 100644 --- a/net/dcb/dcbnl.c +++ b/net/dcb/dcbnl.c @@ -1669,7 +1669,7 @@ static int dcb_doit(struct sk_buff *skb, struct nlmsghdr *nlh) struct nlmsghdr *reply_nlh = NULL; const struct reply_func *fn; - if ((nlh->nlmsg_type == RTM_SETDCB) && !capable(CAP_NET_ADMIN)) + if ((nlh->nlmsg_type == RTM_SETDCB) && !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; ret = nlmsg_parse(nlh, sizeof(*dcb), tb, DCB_ATTR_MAX, diff --git a/net/decnet/dn_dev.c b/net/decnet/dn_dev.c index a603823a3e27..3b726f31c64c 100644 --- a/net/decnet/dn_dev.c +++ b/net/decnet/dn_dev.c @@ -574,7 +574,7 @@ static int dn_nl_deladdr(struct sk_buff *skb, struct nlmsghdr *nlh) struct dn_ifaddr __rcu **ifap; int err = -EINVAL; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (!net_eq(net, &init_net)) @@ -618,7 +618,7 @@ static int dn_nl_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh) struct dn_ifaddr *ifa; int err; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (!net_eq(net, &init_net)) diff --git a/net/decnet/dn_fib.c b/net/decnet/dn_fib.c index 57dc159245ec..d332aefb0846 100644 --- a/net/decnet/dn_fib.c +++ b/net/decnet/dn_fib.c @@ -505,7 +505,7 @@ static int dn_fib_rtm_delroute(struct sk_buff *skb, struct nlmsghdr *nlh) struct nlattr *attrs[RTA_MAX+1]; int err; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (!net_eq(net, &init_net)) @@ -530,7 +530,7 @@ static int dn_fib_rtm_newroute(struct sk_buff *skb, struct nlmsghdr *nlh) struct nlattr *attrs[RTA_MAX+1]; int err; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (!net_eq(net, &init_net)) diff --git a/net/decnet/netfilter/dn_rtmsg.c b/net/decnet/netfilter/dn_rtmsg.c index e83015cecfa7..e4d9560a910b 100644 --- a/net/decnet/netfilter/dn_rtmsg.c +++ b/net/decnet/netfilter/dn_rtmsg.c @@ -107,7 +107,7 @@ static inline void dnrmg_receive_user_skb(struct sk_buff *skb) if (nlh->nlmsg_len < sizeof(*nlh) || skb->len < nlh->nlmsg_len) return; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) RCV_SKB_FAIL(-EPERM); /* Eventually we might send routing messages too */ diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c index e8138da4c14f..84392f3237c1 100644 --- a/net/netfilter/nfnetlink.c +++ b/net/netfilter/nfnetlink.c @@ -375,7 +375,7 @@ static void nfnetlink_rcv(struct sk_buff *skb) skb->len < nlh->nlmsg_len) return; - if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) { + if (!netlink_net_capable(skb, CAP_NET_ADMIN)) { netlink_ack(skb, nlh, -EPERM); return; } diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c index b1dcdb932a86..a3ba3ca0ff92 100644 --- a/net/netlink/genetlink.c +++ b/net/netlink/genetlink.c @@ -561,7 +561,7 @@ static int genl_family_rcv_msg(struct genl_family *family, return -EOPNOTSUPP; if ((ops->flags & GENL_ADMIN_PERM) && - !capable(CAP_NET_ADMIN)) + !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) { diff --git a/net/packet/diag.c b/net/packet/diag.c index b34d0de24091..92f2c7107eec 100644 --- a/net/packet/diag.c +++ b/net/packet/diag.c @@ -194,7 +194,7 @@ static int packet_diag_dump(struct sk_buff *skb, struct netlink_callback *cb) net = sock_net(skb->sk); req = nlmsg_data(cb->nlh); - may_report_filterinfo = ns_capable(net->user_ns, CAP_NET_ADMIN); + may_report_filterinfo = netlink_net_capable(cb->skb, CAP_NET_ADMIN); mutex_lock(&net->packet.sklist_lock); sk_for_each(sk, &net->packet.sklist) { diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c index dc15f4300808..b64151ade6b3 100644 --- a/net/phonet/pn_netlink.c +++ b/net/phonet/pn_netlink.c @@ -70,10 +70,10 @@ static int addr_doit(struct sk_buff *skb, struct nlmsghdr *nlh) int err; u8 pnaddr; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; - if (!capable(CAP_SYS_ADMIN)) + if (!netlink_capable(skb, CAP_SYS_ADMIN)) return -EPERM; ASSERT_RTNL(); @@ -233,10 +233,10 @@ static int route_doit(struct sk_buff *skb, struct nlmsghdr *nlh) int err; u8 dst; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; - if (!capable(CAP_SYS_ADMIN)) + if (!netlink_capable(skb, CAP_SYS_ADMIN)) return -EPERM; ASSERT_RTNL(); diff --git a/net/sched/act_api.c b/net/sched/act_api.c index 8a5ba5add4bc..648778aef1a2 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -948,7 +948,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n) u32 portid = skb ? NETLINK_CB(skb).portid : 0; int ret = 0, ovr = 0; - if ((n->nlmsg_type != RTM_GETACTION) && !capable(CAP_NET_ADMIN)) + if ((n->nlmsg_type != RTM_GETACTION) && !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL); diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index 29a30a14c315..bdbdb1a7920a 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -134,7 +134,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n) int err; int tp_created = 0; - if ((n->nlmsg_type != RTM_GETTFILTER) && !capable(CAP_NET_ADMIN)) + if ((n->nlmsg_type != RTM_GETTFILTER) && !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; replay: diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index a0b84e0e22de..400769014bbd 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -1084,7 +1084,7 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n) struct Qdisc *p = NULL; int err; - if ((n->nlmsg_type != RTM_GETQDISC) && !capable(CAP_NET_ADMIN)) + if ((n->nlmsg_type != RTM_GETQDISC) && !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; err = nlmsg_parse(n, sizeof(*tcm), tca, TCA_MAX, NULL); @@ -1151,7 +1151,7 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n) struct Qdisc *q, *p; int err; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; replay: @@ -1490,7 +1490,7 @@ static int tc_ctl_tclass(struct sk_buff *skb, struct nlmsghdr *n) u32 qid; int err; - if ((n->nlmsg_type != RTM_GETTCLASS) && !capable(CAP_NET_ADMIN)) + if ((n->nlmsg_type != RTM_GETTCLASS) && !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; err = nlmsg_parse(n, sizeof(*tcm), tca, TCA_MAX, NULL); diff --git a/net/tipc/netlink.c b/net/tipc/netlink.c index 3aaf73de9e2d..ad844d365340 100644 --- a/net/tipc/netlink.c +++ b/net/tipc/netlink.c @@ -47,7 +47,7 @@ static int handle_cmd(struct sk_buff *skb, struct genl_info *info) int hdr_space = nlmsg_total_size(GENL_HDRLEN + TIPC_GENL_HDRLEN); u16 cmd; - if ((req_userhdr->cmd & 0xC000) && (!capable(CAP_NET_ADMIN))) + if ((req_userhdr->cmd & 0xC000) && (!netlink_capable(skb, CAP_NET_ADMIN))) cmd = TIPC_CMD_NOT_NET_ADMIN; else cmd = req_userhdr->cmd; diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index 8f131c10a6f3..51398ae6cda8 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -2377,7 +2377,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 (!ns_capable(net->user_ns, CAP_NET_ADMIN)) + if (!netlink_net_capable(skb, CAP_NET_ADMIN)) return -EPERM; if ((type == (XFRM_MSG_GETSA - XFRM_MSG_BASE) || -- 1.9.1 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 0/6]: Preventing abuse when passing file descriptors 2014-04-22 21:13 ` [PATCH 0/6]: Preventing abuse when passing file descriptors Eric W. Biederman ` (5 preceding siblings ...) 2014-04-22 21:17 ` [PATCH 6/6] net: Use netlink_ns_capable to verify the permisions of " Eric W. Biederman @ 2014-04-23 19:32 ` David Miller 2014-04-23 21:24 ` [PATCH 0/5]: " Eric W. Biederman 6 siblings, 1 reply; 46+ messages in thread From: David Miller @ 2014-04-23 19:32 UTC (permalink / raw) To: ebiederm; +Cc: vgoyal, ssorce, security, luto, netdev, serge From: ebiederm@xmission.com (Eric W. Biederman) Date: Tue, 22 Apr 2014 14:13:43 -0700 > Andy Lutomirski when looking at the networking stack noticed that it is > possible to trick privilged processes into calling write on a netlink > socket and send netlink messages they did not intend. > > In particular from time to time there are suid applications that will > write to stdout or stderr without checking exactly what kind of file > descriptors those are and can be tricked into acting as a limited form > of suid cat. In other conversations the magic string CVE-2014-0818 has > been used to talk about this issue. > > This patchset cleans things up a bit, adds some clean abstractions that > when used prevent this kind of problem and then finally changes all of > the handlers of netlink messages that I could find that call capable > to use netlink_ns_capable or an appropriate wrapper. > > The abstraction netlink_ns_capable verifies that the original creator > of the netlink socket a message is sent from had the necessary > capabilities as well as verifying that the current sender of a netlink > packet has the necessary capabilities. > > The idea is to prevent file descriptor massing of any form from > resulting in a file descriptor that can do more than it can for the > creator of the file descriptor. These patches were made against net-next, please rebase them against 'net' and resubmit, thank you. ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 0/5]: Preventing abuse when passing file descriptors 2014-04-23 19:32 ` [PATCH 0/6]: Preventing abuse when passing file descriptors David Miller @ 2014-04-23 21:24 ` Eric W. Biederman 2014-04-23 21:25 ` [PATCH 1/5] netlink: Rename netlink_capable netlink_allowed Eric W. Biederman ` (5 more replies) 0 siblings, 6 replies; 46+ messages in thread From: Eric W. Biederman @ 2014-04-23 21:24 UTC (permalink / raw) To: David Miller; +Cc: vgoyal, ssorce, security, luto, netdev, serge Andy Lutomirski when looking at the networking stack noticed that it is possible to trick privilged processes into calling write on a netlink socket and send netlink messages they did not intend. In particular from time to time there are suid applications that will write to stdout or stderr without checking exactly what kind of file descriptors those are and can be tricked into acting as a limited form of suid cat. In other conversations the magic string CVE-2014-0181 has been used to talk about this issue. This patchset cleans things up a bit, adds some clean abstractions that when used prevent this kind of problem and then finally changes all of the handlers of netlink messages that I could find that call capable to use netlink_ns_capable or an appropriate wrapper. The abstraction netlink_ns_capable verifies that the original creator of the netlink socket a message is sent from had the necessary capabilities as well as verifying that the current sender of a netlink packet has the necessary capabilities. The idea is to prevent file descriptor passing of any form from resulting in a file descriptor that can do more than it can for the creator of the file descriptor. Eric W. Biederman (5): netlink: Rename netlink_capable netlink_allowed net: Move the permission check in sock_diag_put_filterinfo to packet_diag_dump net: Add variants of capable for use on on sockets net: Add variants of capable for use on netlink messages net: Use netlink_ns_capable to verify the permisions of netlink messages crypto/crypto_user.c | 2 +- drivers/connector/cn_proc.c | 2 +- drivers/scsi/scsi_netlink.c | 2 +- include/linux/netlink.h | 7 ++++ include/linux/sock_diag.h | 2 +- include/net/sock.h | 5 +++ kernel/audit.c | 4 +-- net/can/gw.c | 4 +-- net/core/rtnetlink.c | 20 ++++++----- net/core/sock.c | 49 +++++++++++++++++++++++++++ net/core/sock_diag.c | 4 +-- net/dcb/dcbnl.c | 2 +- net/decnet/dn_dev.c | 4 +-- net/decnet/dn_fib.c | 4 +-- net/decnet/netfilter/dn_rtmsg.c | 2 +- net/netfilter/nfnetlink.c | 2 +- net/netlink/af_netlink.c | 75 ++++++++++++++++++++++++++++++++++++++--- net/netlink/genetlink.c | 2 +- net/packet/diag.c | 7 +++- net/phonet/pn_netlink.c | 8 ++--- net/sched/act_api.c | 2 +- net/sched/cls_api.c | 2 +- net/sched/sch_api.c | 6 ++-- net/tipc/netlink.c | 2 +- net/xfrm/xfrm_user.c | 2 +- 25 files changed, 177 insertions(+), 44 deletions(-) Eric ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 1/5] netlink: Rename netlink_capable netlink_allowed 2014-04-23 21:24 ` [PATCH 0/5]: " Eric W. Biederman @ 2014-04-23 21:25 ` Eric W. Biederman 2014-04-23 21:26 ` [PATCH 2/5] net: Move the permission check in sock_diag_put_filterinfo to packet_diag_dump Eric W. Biederman ` (4 subsequent siblings) 5 siblings, 0 replies; 46+ messages in thread From: Eric W. Biederman @ 2014-04-23 21:25 UTC (permalink / raw) To: David Miller; +Cc: vgoyal, ssorce, security, luto, netdev, serge netlink_capable is a static internal function in af_netlink.c and we have better uses for the name netlink_capable. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- net/netlink/af_netlink.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index 894cda0206bb..7f931fe4d187 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -1360,7 +1360,7 @@ retry: return err; } -static inline int netlink_capable(const struct socket *sock, unsigned int flag) +static inline int netlink_allowed(const struct socket *sock, unsigned int flag) { return (nl_table[sock->sk->sk_protocol].flags & flag) || ns_capable(sock_net(sock->sk)->user_ns, CAP_NET_ADMIN); @@ -1428,7 +1428,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr, /* Only superuser is allowed to listen multicasts */ if (nladdr->nl_groups) { - if (!netlink_capable(sock, NL_CFG_F_NONROOT_RECV)) + if (!netlink_allowed(sock, NL_CFG_F_NONROOT_RECV)) return -EPERM; err = netlink_realloc_groups(sk); if (err) @@ -1490,7 +1490,7 @@ static int netlink_connect(struct socket *sock, struct sockaddr *addr, return -EINVAL; if ((nladdr->nl_groups || nladdr->nl_pid) && - !netlink_capable(sock, NL_CFG_F_NONROOT_SEND)) + !netlink_allowed(sock, NL_CFG_F_NONROOT_SEND)) return -EPERM; if (!nlk->portid) @@ -2096,7 +2096,7 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname, break; case NETLINK_ADD_MEMBERSHIP: case NETLINK_DROP_MEMBERSHIP: { - if (!netlink_capable(sock, NL_CFG_F_NONROOT_RECV)) + if (!netlink_allowed(sock, NL_CFG_F_NONROOT_RECV)) return -EPERM; err = netlink_realloc_groups(sk); if (err) @@ -2247,7 +2247,7 @@ static int netlink_sendmsg(struct kiocb *kiocb, struct socket *sock, dst_group = ffs(addr->nl_groups); err = -EPERM; if ((dst_group || dst_portid) && - !netlink_capable(sock, NL_CFG_F_NONROOT_SEND)) + !netlink_allowed(sock, NL_CFG_F_NONROOT_SEND)) goto out; } else { dst_portid = nlk->dst_portid; -- 1.9.1 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 2/5] net: Move the permission check in sock_diag_put_filterinfo to packet_diag_dump 2014-04-23 21:24 ` [PATCH 0/5]: " Eric W. Biederman 2014-04-23 21:25 ` [PATCH 1/5] netlink: Rename netlink_capable netlink_allowed Eric W. Biederman @ 2014-04-23 21:26 ` Eric W. Biederman 2014-04-23 21:26 ` [PATCH 3/5] net: Add variants of capable for use on on sockets Eric W. Biederman ` (3 subsequent siblings) 5 siblings, 0 replies; 46+ messages in thread From: Eric W. Biederman @ 2014-04-23 21:26 UTC (permalink / raw) To: David Miller; +Cc: vgoyal, ssorce, security, luto, netdev, serge The permission check in sock_diag_put_filterinfo is wrong, and it is so removed from it's sources it is not clear why it is wrong. Move the computation into packet_diag_dump and pass a bool of the result into sock_diag_filterinfo. This does not yet correct the capability check but instead simply moves it to make it clear what is going on. Reported-by: Andy Lutomirski <luto@amacapital.net> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- include/linux/sock_diag.h | 2 +- net/core/sock_diag.c | 4 ++-- net/packet/diag.c | 7 ++++++- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/include/linux/sock_diag.h b/include/linux/sock_diag.h index 302ab805b0bb..46cca4c06848 100644 --- a/include/linux/sock_diag.h +++ b/include/linux/sock_diag.h @@ -23,7 +23,7 @@ int sock_diag_check_cookie(void *sk, __u32 *cookie); void sock_diag_save_cookie(void *sk, __u32 *cookie); int sock_diag_put_meminfo(struct sock *sk, struct sk_buff *skb, int attr); -int sock_diag_put_filterinfo(struct sock *sk, +int sock_diag_put_filterinfo(bool may_report_filterinfo, struct sock *sk, struct sk_buff *skb, int attrtype); #endif diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c index 9deb6abd6cf6..a4216a4c9572 100644 --- a/net/core/sock_diag.c +++ b/net/core/sock_diag.c @@ -49,7 +49,7 @@ int sock_diag_put_meminfo(struct sock *sk, struct sk_buff *skb, int attrtype) } EXPORT_SYMBOL_GPL(sock_diag_put_meminfo); -int sock_diag_put_filterinfo(struct sock *sk, +int sock_diag_put_filterinfo(bool may_report_filterinfo, struct sock *sk, struct sk_buff *skb, int attrtype) { struct sock_fprog_kern *fprog; @@ -58,7 +58,7 @@ int sock_diag_put_filterinfo(struct sock *sk, unsigned int flen; int err = 0; - if (!ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN)) { + if (!may_report_filterinfo) { nla_reserve(skb, attrtype, 0); return 0; } diff --git a/net/packet/diag.c b/net/packet/diag.c index 435ff99ba8c7..b34d0de24091 100644 --- a/net/packet/diag.c +++ b/net/packet/diag.c @@ -128,6 +128,7 @@ static int pdiag_put_fanout(struct packet_sock *po, struct sk_buff *nlskb) static int sk_diag_fill(struct sock *sk, struct sk_buff *skb, struct packet_diag_req *req, + bool may_report_filterinfo, struct user_namespace *user_ns, u32 portid, u32 seq, u32 flags, int sk_ino) { @@ -172,7 +173,8 @@ static int sk_diag_fill(struct sock *sk, struct sk_buff *skb, goto out_nlmsg_trim; if ((req->pdiag_show & PACKET_SHOW_FILTER) && - sock_diag_put_filterinfo(sk, skb, PACKET_DIAG_FILTER)) + sock_diag_put_filterinfo(may_report_filterinfo, sk, skb, + PACKET_DIAG_FILTER)) goto out_nlmsg_trim; return nlmsg_end(skb, nlh); @@ -188,9 +190,11 @@ static int packet_diag_dump(struct sk_buff *skb, struct netlink_callback *cb) struct packet_diag_req *req; struct net *net; struct sock *sk; + bool may_report_filterinfo; net = sock_net(skb->sk); req = nlmsg_data(cb->nlh); + may_report_filterinfo = ns_capable(net->user_ns, CAP_NET_ADMIN); mutex_lock(&net->packet.sklist_lock); sk_for_each(sk, &net->packet.sklist) { @@ -200,6 +204,7 @@ static int packet_diag_dump(struct sk_buff *skb, struct netlink_callback *cb) goto next; if (sk_diag_fill(sk, skb, req, + may_report_filterinfo, sk_user_ns(NETLINK_CB(cb->skb).sk), NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq, NLM_F_MULTI, -- 1.9.1 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 3/5] net: Add variants of capable for use on on sockets 2014-04-23 21:24 ` [PATCH 0/5]: " Eric W. Biederman 2014-04-23 21:25 ` [PATCH 1/5] netlink: Rename netlink_capable netlink_allowed Eric W. Biederman 2014-04-23 21:26 ` [PATCH 2/5] net: Move the permission check in sock_diag_put_filterinfo to packet_diag_dump Eric W. Biederman @ 2014-04-23 21:26 ` Eric W. Biederman 2014-04-23 21:28 ` [PATCH 4/5] net: Add variants of capable for use on netlink messages Eric W. Biederman ` (2 subsequent siblings) 5 siblings, 0 replies; 46+ messages in thread From: Eric W. Biederman @ 2014-04-23 21:26 UTC (permalink / raw) To: David Miller; +Cc: vgoyal, ssorce, security, luto, netdev, serge sk_net_capable - The common case, operations that are safe in a network namespace. sk_capable - Operations that are not known to be safe in a network namespace sk_ns_capable - The general case for special cases. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- include/net/sock.h | 5 +++++ net/core/sock.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/include/net/sock.h b/include/net/sock.h index 8338a14e4805..21569cf456ed 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -2255,6 +2255,11 @@ int sock_get_timestampns(struct sock *, struct timespec __user *); int sock_recv_errqueue(struct sock *sk, struct msghdr *msg, int len, int level, int type); +bool sk_ns_capable(const struct sock *sk, + struct user_namespace *user_ns, int cap); +bool sk_capable(const struct sock *sk, int cap); +bool sk_net_capable(const struct sock *sk, int cap); + /* * Enable debug/info messages */ diff --git a/net/core/sock.c b/net/core/sock.c index b4fff008136f..664ee4295b6f 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -145,6 +145,55 @@ static DEFINE_MUTEX(proto_list_mutex); static LIST_HEAD(proto_list); +/** + * sk_ns_capable - General socket capability test + * @sk: Socket to use a capability on or through + * @user_ns: The user namespace of the capability to use + * @cap: The capability to use + * + * Test to see if the opener of the socket had when the socket was + * created and the current process has the capability @cap in the user + * namespace @user_ns. + */ +bool sk_ns_capable(const struct sock *sk, + struct user_namespace *user_ns, int cap) +{ + return file_ns_capable(sk->sk_socket->file, user_ns, cap) && + ns_capable(user_ns, cap); +} +EXPORT_SYMBOL(sk_ns_capable); + +/** + * sk_capable - Socket global capability test + * @sk: Socket to use a capability on or through + * @cap: The global capbility to use + * + * Test to see if the opener of the socket had when the socket was + * created and the current process has the capability @cap in all user + * namespaces. + */ +bool sk_capable(const struct sock *sk, int cap) +{ + return sk_ns_capable(sk, &init_user_ns, cap); +} +EXPORT_SYMBOL(sk_capable); + +/** + * sk_net_capable - Network namespace socket capability test + * @sk: Socket to use a capability on or through + * @cap: The capability to use + * + * Test to see if the opener of the socket had when the socke was created + * and the current process has the capability @cap over the network namespace + * the socket is a member of. + */ +bool sk_net_capable(const struct sock *sk, int cap) +{ + return sk_ns_capable(sk, sock_net(sk)->user_ns, cap); +} +EXPORT_SYMBOL(sk_net_capable); + + #ifdef CONFIG_MEMCG_KMEM int mem_cgroup_sockets_init(struct mem_cgroup *memcg, struct cgroup_subsys *ss) { -- 1.9.1 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 4/5] net: Add variants of capable for use on netlink messages 2014-04-23 21:24 ` [PATCH 0/5]: " Eric W. Biederman ` (2 preceding siblings ...) 2014-04-23 21:26 ` [PATCH 3/5] net: Add variants of capable for use on on sockets Eric W. Biederman @ 2014-04-23 21:28 ` Eric W. Biederman 2014-04-23 21:29 ` [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of " Eric W. Biederman 2014-04-24 17:45 ` [PATCH 0/5]: Preventing abuse when passing file descriptors David Miller 5 siblings, 0 replies; 46+ messages in thread From: Eric W. Biederman @ 2014-04-23 21:28 UTC (permalink / raw) To: David Miller; +Cc: vgoyal, ssorce, security, luto, netdev, serge netlink_net_capable - The common case use, for operations that are safe on a network namespace netlink_capable - For operations that are only known to be safe for the global root netlink_ns_capable - The general case of capable used to handle special cases __netlink_ns_capable - Same as netlink_ns_capable except taking a netlink_skb_parms instead of the skbuff of a netlink message. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- include/linux/netlink.h | 7 ++++++ net/netlink/af_netlink.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/include/linux/netlink.h b/include/linux/netlink.h index aad8eeaf416d..f64b01787ddc 100644 --- a/include/linux/netlink.h +++ b/include/linux/netlink.h @@ -169,4 +169,11 @@ struct netlink_tap { extern int netlink_add_tap(struct netlink_tap *nt); extern int netlink_remove_tap(struct netlink_tap *nt); +bool __netlink_ns_capable(const struct netlink_skb_parms *nsp, + struct user_namespace *ns, int cap); +bool netlink_ns_capable(const struct sk_buff *skb, + struct user_namespace *ns, int cap); +bool netlink_capable(const struct sk_buff *skb, int cap); +bool netlink_net_capable(const struct sk_buff *skb, int cap); + #endif /* __LINUX_NETLINK_H */ diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index 7f931fe4d187..81dca96d2be6 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -1360,6 +1360,71 @@ retry: return err; } +/** + * __netlink_ns_capable - General netlink message capability test + * @nsp: NETLINK_CB of the socket buffer holding a netlink command from userspace. + * @user_ns: The user namespace of the capability to use + * @cap: The capability to use + * + * Test to see if the opener of the socket we received the message + * from had when the netlink socket was created and the sender of the + * message has has the capability @cap in the user namespace @user_ns. + */ +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); +} +EXPORT_SYMBOL(__netlink_ns_capable); + +/** + * netlink_ns_capable - General netlink message capability test + * @skb: socket buffer holding a netlink command from userspace + * @user_ns: The user namespace of the capability to use + * @cap: The capability to use + * + * Test to see if the opener of the socket we received the message + * from had when the netlink socket was created and the sender of the + * message has has the capability @cap in the user namespace @user_ns. + */ +bool netlink_ns_capable(const struct sk_buff *skb, + struct user_namespace *user_ns, int cap) +{ + return __netlink_ns_capable(&NETLINK_CB(skb), user_ns, cap); +} +EXPORT_SYMBOL(netlink_ns_capable); + +/** + * netlink_capable - Netlink global message capability test + * @skb: socket buffer holding a netlink command from userspace + * @cap: The capability to use + * + * Test to see if the opener of the socket we received the message + * from had when the netlink socket was created and the sender of the + * message has has the capability @cap in all user namespaces. + */ +bool netlink_capable(const struct sk_buff *skb, int cap) +{ + return netlink_ns_capable(skb, &init_user_ns, cap); +} +EXPORT_SYMBOL(netlink_capable); + +/** + * netlink_net_capable - Netlink network namespace message capability test + * @skb: socket buffer holding a netlink command from userspace + * @cap: The capability to use + * + * Test to see if the opener of the socket we received the message + * from had when the netlink socket was created and the sender of the + * message has has the capability @cap over the network namespace of + * the socket we received the message from. + */ +bool netlink_net_capable(const struct sk_buff *skb, int cap) +{ + return netlink_ns_capable(skb, sock_net(skb->sk)->user_ns, cap); +} +EXPORT_SYMBOL(netlink_net_capable); + static inline int netlink_allowed(const struct socket *sock, unsigned int flag) { return (nl_table[sock->sk->sk_protocol].flags & flag) || -- 1.9.1 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-04-23 21:24 ` [PATCH 0/5]: " Eric W. Biederman ` (3 preceding siblings ...) 2014-04-23 21:28 ` [PATCH 4/5] net: Add variants of capable for use on netlink messages Eric W. Biederman @ 2014-04-23 21:29 ` Eric W. Biederman 2014-05-07 22:18 ` Jorge Boncompte [DTI2] 2014-04-24 17:45 ` [PATCH 0/5]: Preventing abuse when passing file descriptors David Miller 5 siblings, 1 reply; 46+ messages in thread From: Eric W. Biederman @ 2014-04-23 21:29 UTC (permalink / raw) To: David Miller; +Cc: vgoyal, ssorce, security, luto, netdev, serge It is possible by passing a netlink socket to a more privileged executable and then to fool that executable into writing to the socket data that happens to be valid netlink message to do something that privileged executable did not intend to do. To keep this from happening replace bare capable and ns_capable calls with netlink_capable, netlink_net_calls and netlink_ns_capable calls. Which act the same as the previous calls except they verify that the opener of the socket had the desired permissions as well. Reported-by: Andy Lutomirski <luto@amacapital.net> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- crypto/crypto_user.c | 2 +- drivers/connector/cn_proc.c | 2 +- drivers/scsi/scsi_netlink.c | 2 +- kernel/audit.c | 4 ++-- net/can/gw.c | 4 ++-- net/core/rtnetlink.c | 20 +++++++++++--------- net/dcb/dcbnl.c | 2 +- net/decnet/dn_dev.c | 4 ++-- net/decnet/dn_fib.c | 4 ++-- net/decnet/netfilter/dn_rtmsg.c | 2 +- net/netfilter/nfnetlink.c | 2 +- net/netlink/genetlink.c | 2 +- net/packet/diag.c | 2 +- net/phonet/pn_netlink.c | 8 ++++---- net/sched/act_api.c | 2 +- net/sched/cls_api.c | 2 +- net/sched/sch_api.c | 6 +++--- net/tipc/netlink.c | 2 +- net/xfrm/xfrm_user.c | 2 +- 19 files changed, 38 insertions(+), 36 deletions(-) diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c index 1512e41cd93d..43665d0d0905 100644 --- a/crypto/crypto_user.c +++ b/crypto/crypto_user.c @@ -466,7 +466,7 @@ static int crypto_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) type -= CRYPTO_MSG_BASE; link = &crypto_dispatch[type]; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if ((type == (CRYPTO_MSG_GETALG - CRYPTO_MSG_BASE) && diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c index 148d707a1d43..ccdd4c7e748b 100644 --- a/drivers/connector/cn_proc.c +++ b/drivers/connector/cn_proc.c @@ -369,7 +369,7 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg, return; /* Can only change if privileged. */ - if (!capable(CAP_NET_ADMIN)) { + if (!__netlink_ns_capable(nsp, &init_user_ns, CAP_NET_ADMIN)) { err = EPERM; goto out; } diff --git a/drivers/scsi/scsi_netlink.c b/drivers/scsi/scsi_netlink.c index fe30ea94ffe6..109802f776ed 100644 --- a/drivers/scsi/scsi_netlink.c +++ b/drivers/scsi/scsi_netlink.c @@ -77,7 +77,7 @@ scsi_nl_rcv_msg(struct sk_buff *skb) goto next_msg; } - if (!capable(CAP_SYS_ADMIN)) { + if (!netlink_capable(skb, CAP_SYS_ADMIN)) { err = -EPERM; goto next_msg; } diff --git a/kernel/audit.c b/kernel/audit.c index 7c2893602d06..47845c57eb19 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -643,13 +643,13 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type) if ((task_active_pid_ns(current) != &init_pid_ns)) return -EPERM; - if (!capable(CAP_AUDIT_CONTROL)) + if (!netlink_capable(skb, CAP_AUDIT_CONTROL)) 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 (!capable(CAP_AUDIT_WRITE)) + if (!netlink_capable(skb, CAP_AUDIT_WRITE)) err = -EPERM; break; default: /* bad msg */ diff --git a/net/can/gw.c b/net/can/gw.c index ac31891967da..050a2110d43f 100644 --- a/net/can/gw.c +++ b/net/can/gw.c @@ -804,7 +804,7 @@ static int cgw_create_job(struct sk_buff *skb, struct nlmsghdr *nlh) u8 limhops = 0; int err = 0; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (nlmsg_len(nlh) < sizeof(*r)) @@ -893,7 +893,7 @@ static int cgw_remove_job(struct sk_buff *skb, struct nlmsghdr *nlh) u8 limhops = 0; int err = 0; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (nlmsg_len(nlh) < sizeof(*r)) diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index d4ff41739b0f..64ad17d077ed 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -1395,7 +1395,8 @@ static int do_set_master(struct net_device *dev, int ifindex) return 0; } -static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm, +static int do_setlink(const struct sk_buff *skb, + struct net_device *dev, struct ifinfomsg *ifm, struct nlattr **tb, char *ifname, int modified) { const struct net_device_ops *ops = dev->netdev_ops; @@ -1407,7 +1408,7 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm, err = PTR_ERR(net); goto errout; } - if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) { + if (!netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN)) { err = -EPERM; goto errout; } @@ -1661,7 +1662,7 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh) if (err < 0) goto errout; - err = do_setlink(dev, ifm, tb, ifname, 0); + err = do_setlink(skb, dev, ifm, tb, ifname, 0); errout: return err; } @@ -1778,7 +1779,8 @@ err: } EXPORT_SYMBOL(rtnl_create_link); -static int rtnl_group_changelink(struct net *net, int group, +static int rtnl_group_changelink(const struct sk_buff *skb, + struct net *net, int group, struct ifinfomsg *ifm, struct nlattr **tb) { @@ -1787,7 +1789,7 @@ static int rtnl_group_changelink(struct net *net, int group, for_each_netdev(net, dev) { if (dev->group == group) { - err = do_setlink(dev, ifm, tb, NULL, 0); + err = do_setlink(skb, dev, ifm, tb, NULL, 0); if (err < 0) return err; } @@ -1929,12 +1931,12 @@ replay: modified = 1; } - return do_setlink(dev, ifm, tb, ifname, modified); + return do_setlink(skb, dev, ifm, tb, ifname, modified); } if (!(nlh->nlmsg_flags & NLM_F_CREATE)) { if (ifm->ifi_index == 0 && tb[IFLA_GROUP]) - return rtnl_group_changelink(net, + return rtnl_group_changelink(skb, net, nla_get_u32(tb[IFLA_GROUP]), ifm, tb); return -ENODEV; @@ -2321,7 +2323,7 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh) int err = -EINVAL; __u8 *addr; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX, NULL); @@ -2773,7 +2775,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) sz_idx = type>>2; kind = type&3; - if (kind != 2 && !ns_capable(net->user_ns, CAP_NET_ADMIN)) + if (kind != 2 && !netlink_net_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (kind == 2 && nlh->nlmsg_flags&NLM_F_DUMP) { diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c index 553644402670..f8b98d89c285 100644 --- a/net/dcb/dcbnl.c +++ b/net/dcb/dcbnl.c @@ -1669,7 +1669,7 @@ static int dcb_doit(struct sk_buff *skb, struct nlmsghdr *nlh) struct nlmsghdr *reply_nlh = NULL; const struct reply_func *fn; - if ((nlh->nlmsg_type == RTM_SETDCB) && !capable(CAP_NET_ADMIN)) + if ((nlh->nlmsg_type == RTM_SETDCB) && !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; ret = nlmsg_parse(nlh, sizeof(*dcb), tb, DCB_ATTR_MAX, diff --git a/net/decnet/dn_dev.c b/net/decnet/dn_dev.c index a603823a3e27..3b726f31c64c 100644 --- a/net/decnet/dn_dev.c +++ b/net/decnet/dn_dev.c @@ -574,7 +574,7 @@ static int dn_nl_deladdr(struct sk_buff *skb, struct nlmsghdr *nlh) struct dn_ifaddr __rcu **ifap; int err = -EINVAL; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (!net_eq(net, &init_net)) @@ -618,7 +618,7 @@ static int dn_nl_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh) struct dn_ifaddr *ifa; int err; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (!net_eq(net, &init_net)) diff --git a/net/decnet/dn_fib.c b/net/decnet/dn_fib.c index 57dc159245ec..d332aefb0846 100644 --- a/net/decnet/dn_fib.c +++ b/net/decnet/dn_fib.c @@ -505,7 +505,7 @@ static int dn_fib_rtm_delroute(struct sk_buff *skb, struct nlmsghdr *nlh) struct nlattr *attrs[RTA_MAX+1]; int err; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (!net_eq(net, &init_net)) @@ -530,7 +530,7 @@ static int dn_fib_rtm_newroute(struct sk_buff *skb, struct nlmsghdr *nlh) struct nlattr *attrs[RTA_MAX+1]; int err; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (!net_eq(net, &init_net)) diff --git a/net/decnet/netfilter/dn_rtmsg.c b/net/decnet/netfilter/dn_rtmsg.c index e83015cecfa7..e4d9560a910b 100644 --- a/net/decnet/netfilter/dn_rtmsg.c +++ b/net/decnet/netfilter/dn_rtmsg.c @@ -107,7 +107,7 @@ static inline void dnrmg_receive_user_skb(struct sk_buff *skb) if (nlh->nlmsg_len < sizeof(*nlh) || skb->len < nlh->nlmsg_len) return; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) RCV_SKB_FAIL(-EPERM); /* Eventually we might send routing messages too */ diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c index e8138da4c14f..84392f3237c1 100644 --- a/net/netfilter/nfnetlink.c +++ b/net/netfilter/nfnetlink.c @@ -375,7 +375,7 @@ static void nfnetlink_rcv(struct sk_buff *skb) skb->len < nlh->nlmsg_len) return; - if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) { + if (!netlink_net_capable(skb, CAP_NET_ADMIN)) { netlink_ack(skb, nlh, -EPERM); return; } diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c index b1dcdb932a86..a3ba3ca0ff92 100644 --- a/net/netlink/genetlink.c +++ b/net/netlink/genetlink.c @@ -561,7 +561,7 @@ static int genl_family_rcv_msg(struct genl_family *family, return -EOPNOTSUPP; if ((ops->flags & GENL_ADMIN_PERM) && - !capable(CAP_NET_ADMIN)) + !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if ((nlh->nlmsg_flags & NLM_F_DUMP) == NLM_F_DUMP) { diff --git a/net/packet/diag.c b/net/packet/diag.c index b34d0de24091..92f2c7107eec 100644 --- a/net/packet/diag.c +++ b/net/packet/diag.c @@ -194,7 +194,7 @@ static int packet_diag_dump(struct sk_buff *skb, struct netlink_callback *cb) net = sock_net(skb->sk); req = nlmsg_data(cb->nlh); - may_report_filterinfo = ns_capable(net->user_ns, CAP_NET_ADMIN); + may_report_filterinfo = netlink_net_capable(cb->skb, CAP_NET_ADMIN); mutex_lock(&net->packet.sklist_lock); sk_for_each(sk, &net->packet.sklist) { diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c index dc15f4300808..b64151ade6b3 100644 --- a/net/phonet/pn_netlink.c +++ b/net/phonet/pn_netlink.c @@ -70,10 +70,10 @@ static int addr_doit(struct sk_buff *skb, struct nlmsghdr *nlh) int err; u8 pnaddr; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; - if (!capable(CAP_SYS_ADMIN)) + if (!netlink_capable(skb, CAP_SYS_ADMIN)) return -EPERM; ASSERT_RTNL(); @@ -233,10 +233,10 @@ static int route_doit(struct sk_buff *skb, struct nlmsghdr *nlh) int err; u8 dst; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; - if (!capable(CAP_SYS_ADMIN)) + if (!netlink_capable(skb, CAP_SYS_ADMIN)) return -EPERM; ASSERT_RTNL(); diff --git a/net/sched/act_api.c b/net/sched/act_api.c index 8a5ba5add4bc..648778aef1a2 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -948,7 +948,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n) u32 portid = skb ? NETLINK_CB(skb).portid : 0; int ret = 0, ovr = 0; - if ((n->nlmsg_type != RTM_GETACTION) && !capable(CAP_NET_ADMIN)) + if ((n->nlmsg_type != RTM_GETACTION) && !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL); diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index 29a30a14c315..bdbdb1a7920a 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -134,7 +134,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n) int err; int tp_created = 0; - if ((n->nlmsg_type != RTM_GETTFILTER) && !capable(CAP_NET_ADMIN)) + if ((n->nlmsg_type != RTM_GETTFILTER) && !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; replay: diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index a0b84e0e22de..400769014bbd 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -1084,7 +1084,7 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n) struct Qdisc *p = NULL; int err; - if ((n->nlmsg_type != RTM_GETQDISC) && !capable(CAP_NET_ADMIN)) + if ((n->nlmsg_type != RTM_GETQDISC) && !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; err = nlmsg_parse(n, sizeof(*tcm), tca, TCA_MAX, NULL); @@ -1151,7 +1151,7 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n) struct Qdisc *q, *p; int err; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; replay: @@ -1490,7 +1490,7 @@ static int tc_ctl_tclass(struct sk_buff *skb, struct nlmsghdr *n) u32 qid; int err; - if ((n->nlmsg_type != RTM_GETTCLASS) && !capable(CAP_NET_ADMIN)) + if ((n->nlmsg_type != RTM_GETTCLASS) && !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; err = nlmsg_parse(n, sizeof(*tcm), tca, TCA_MAX, NULL); diff --git a/net/tipc/netlink.c b/net/tipc/netlink.c index 3aaf73de9e2d..ad844d365340 100644 --- a/net/tipc/netlink.c +++ b/net/tipc/netlink.c @@ -47,7 +47,7 @@ static int handle_cmd(struct sk_buff *skb, struct genl_info *info) int hdr_space = nlmsg_total_size(GENL_HDRLEN + TIPC_GENL_HDRLEN); u16 cmd; - if ((req_userhdr->cmd & 0xC000) && (!capable(CAP_NET_ADMIN))) + if ((req_userhdr->cmd & 0xC000) && (!netlink_capable(skb, CAP_NET_ADMIN))) cmd = TIPC_CMD_NOT_NET_ADMIN; else cmd = req_userhdr->cmd; diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index 8f131c10a6f3..51398ae6cda8 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -2377,7 +2377,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 (!ns_capable(net->user_ns, CAP_NET_ADMIN)) + if (!netlink_net_capable(skb, CAP_NET_ADMIN)) return -EPERM; if ((type == (XFRM_MSG_GETSA - XFRM_MSG_BASE) || -- 1.9.1 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-04-23 21:29 ` [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of " Eric W. Biederman @ 2014-05-07 22:18 ` Jorge Boncompte [DTI2] 2014-05-07 22:26 ` Andy Lutomirski 0 siblings, 1 reply; 46+ messages in thread From: Jorge Boncompte [DTI2] @ 2014-05-07 22:18 UTC (permalink / raw) To: Eric W. Biederman, David Miller Cc: vgoyal, ssorce, security, luto, netdev, serge El 23/04/2014 23:29, Eric W. Biederman escribió: > > It is possible by passing a netlink socket to a more privileged > executable and then to fool that executable into writing to the > socket data that happens to be valid netlink message to do something > that privileged executable did not intend to do. > > To keep this from happening replace bare capable and ns_capable calls > with netlink_capable, netlink_net_calls and netlink_ns_capable calls. > Which act the same as the previous calls except they verify that the > opener of the socket had the desired permissions as well. > Hi, after this patch, zebra daemon of quagga in Debian testing fails to send routes to kernel with an -EPERM error. Reverting this patch and commit a64d90fd96 (netfilter: Fix warning in nfnetlink_receive().) fixes it for me. I haven't got time to do a proper analisys and could be that zebra it's doing something silly but this patch seems to subtly change some semantics. -- ============================================================== Jorge Boncompte - Ingenieria y Gestion de RED DTI2 - Desarrollo de la Tecnologia de las Comunicaciones -------------------------------------------------------------- C/ Abogado Enriquez Barrios, 5 14004 CORDOBA (SPAIN) Tlf: +34 957 761395 / FAX: +34 957 450380 ============================================================== - There is only so much duct tape you can put on something before it just becomes a giant ball of duct tape. ============================================================== ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-05-07 22:18 ` Jorge Boncompte [DTI2] @ 2014-05-07 22:26 ` Andy Lutomirski 2014-05-07 22:52 ` David Miller 0 siblings, 1 reply; 46+ messages in thread From: Andy Lutomirski @ 2014-05-07 22:26 UTC (permalink / raw) To: Jorge Boncompte [DTI2] Cc: Eric W. Biederman, David Miller, Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn On Wed, May 7, 2014 at 3:18 PM, Jorge Boncompte [DTI2] <jorge@dti2.net> wrote: > El 23/04/2014 23:29, Eric W. Biederman escribió: >> >> It is possible by passing a netlink socket to a more privileged >> executable and then to fool that executable into writing to the >> socket data that happens to be valid netlink message to do something >> that privileged executable did not intend to do. >> >> To keep this from happening replace bare capable and ns_capable calls >> with netlink_capable, netlink_net_calls and netlink_ns_capable calls. >> Which act the same as the previous calls except they verify that the >> opener of the socket had the desired permissions as well. >> > > Hi, after this patch, zebra daemon of quagga in Debian testing fails to > send routes to kernel with an -EPERM error. > Reverting this patch and commit a64d90fd96 (netfilter: Fix warning in > nfnetlink_receive().) fixes it for me. > > I haven't got time to do a proper analisys and could be that zebra > it's doing something silly but this patch seems to subtly change some > semantics. > Well, crap. sock = socket (AF_NETLINK, SOCK_RAW, NETLINK_ROUTE); if (sock < 0) { zlog (NULL, LOG_ERR, "Can't open %s socket: %s", nl->name, safe_strerror (errno)); return -1; } memset (&snl, 0, sizeof snl); snl.nl_family = AF_NETLINK; snl.nl_groups = groups; /* Bind the socket to the netlink structure for anything. */ if (zserv_privs.change (ZPRIVS_RAISE)) { zlog (NULL, LOG_ERR, "Can't raise privileges"); return -1; } ret = bind (sock, (struct sockaddr *) &snl, sizeof snl); save_errno = errno; if (zserv_privs.change (ZPRIVS_LOWER)) zlog (NULL, LOG_ERR, "Can't lower privileges"); if (ret < 0) { zlog (NULL, LOG_ERR, "Can't bind %s socket to group 0x%x: %s", nl->name, snl.nl_groups, safe_strerror (save_errno)); close (sock); return -1; } So what do we do? Check permissions on connect and then use the cached result for send on a connected socket? Check permitted caps instead of effective caps? This sucks. --Andy ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-05-07 22:26 ` Andy Lutomirski @ 2014-05-07 22:52 ` David Miller 2014-05-07 23:01 ` Andy Lutomirski 0 siblings, 1 reply; 46+ messages in thread From: David Miller @ 2014-05-07 22:52 UTC (permalink / raw) To: luto; +Cc: jorge, ebiederm, vgoyal, ssorce, security, netdev, serge From: Andy Lutomirski <luto@amacapital.net> Date: Wed, 7 May 2014 15:26:11 -0700 > So what do we do? Check permissions on connect and then use the > cached result for send on a connected socket? Check permitted caps > instead of effective caps? It should create the socket after changing perms. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-05-07 22:52 ` David Miller @ 2014-05-07 23:01 ` Andy Lutomirski 2014-05-07 23:34 ` Linus Torvalds 2014-05-07 23:45 ` David Miller 0 siblings, 2 replies; 46+ messages in thread From: Andy Lutomirski @ 2014-05-07 23:01 UTC (permalink / raw) To: David Miller Cc: Jorge Boncompte [DTI2], Eric W. Biederman, Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn On Wed, May 7, 2014 at 3:52 PM, David Miller <davem@davemloft.net> wrote: > From: Andy Lutomirski <luto@amacapital.net> > Date: Wed, 7 May 2014 15:26:11 -0700 > >> So what do we do? Check permissions on connect and then use the >> cached result for send on a connected socket? Check permitted caps >> instead of effective caps? > > It should create the socket after changing perms. I agree that it should, but it doesn't, and if these patches get backported, things will break. OTOH, if the patches don't get backported, things may still break, and we have a possibly rather severe unfixed vulnerability. --Andy ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-05-07 23:01 ` Andy Lutomirski @ 2014-05-07 23:34 ` Linus Torvalds 2014-05-07 23:45 ` Andy Lutomirski 2014-05-08 21:29 ` Stephen Hemminger 2014-05-07 23:45 ` David Miller 1 sibling, 2 replies; 46+ messages in thread From: Linus Torvalds @ 2014-05-07 23:34 UTC (permalink / raw) To: Andy Lutomirski Cc: David Miller, Jorge Boncompte [DTI2], Eric W. Biederman, Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn On Wed, May 7, 2014 at 4:01 PM, Andy Lutomirski <luto@amacapital.net> wrote: > > I agree that it should, but it doesn't, and if these patches get > backported, things will break. OTOH, if the patches don't get > backported, things may still break, and we have a possibly rather > severe unfixed vulnerability. How did this *use* to work? It looks like it drops permissions after the bind(), so the actual _IO_ must have always been done without permissions, no? Is it just a bind-time permission check that is now failing, because it uses the credentials associated with the socket open? If so, I'd suggest unding just the ns-capable change for bind(), and make that one always use the current process effective one. If you're a suid application, you're not doing "bind()" on random file descriptors that were passed to you. It's really just read/write that need to be careful. Linus ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-05-07 23:34 ` Linus Torvalds @ 2014-05-07 23:45 ` Andy Lutomirski 2014-05-22 15:05 ` Jiri Benc 2014-05-08 21:29 ` Stephen Hemminger 1 sibling, 1 reply; 46+ messages in thread From: Andy Lutomirski @ 2014-05-07 23:45 UTC (permalink / raw) To: Linus Torvalds Cc: David Miller, Jorge Boncompte [DTI2], Eric W. Biederman, Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn On Wed, May 7, 2014 at 4:34 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Wed, May 7, 2014 at 4:01 PM, Andy Lutomirski <luto@amacapital.net> wrote: >> >> I agree that it should, but it doesn't, and if these patches get >> backported, things will break. OTOH, if the patches don't get >> backported, things may still break, and we have a possibly rather >> severe unfixed vulnerability. > > How did this *use* to work? It looks like it drops permissions after > the bind(), so the actual _IO_ must have always been done without > permissions, no? > > Is it just a bind-time permission check that is now failing, because > it uses the credentials associated with the socket open? If so, I'd > suggest unding just the ns-capable change for bind(), and make that > one always use the current process effective one. > > If you're a suid application, you're not doing "bind()" on random file > descriptors that were passed to you. It's really just read/write that > need to be careful. > It looks like Zebra is mucking with its effective set. It creates the socket w/o effective caps, raises them to bind (no clue why), lowers them post-bind, then raises them again to sendto. Presumably the new checks cause the sendto to fail b/c the socket()-time credentials were insufficient. I don't see why it bound w/ elevated permissions, since I don't think this is important. And it never connected at all. Hence my suggestion that we check permissions at connect time instead of socket() time and that we just check send-time permissions on an unconnected socket. Yes, this is awful. TBH, I don't think that f_cred really ought to exist in an ideal world -- you either should or should not be able to open something, but the privileges you had when you opened it shouldn't make any difference other than determining whether open works. I don't think my ideal is going to happen, though. --Andy ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-05-07 23:45 ` Andy Lutomirski @ 2014-05-22 15:05 ` Jiri Benc 2014-05-23 23:25 ` Eric W. Biederman 0 siblings, 1 reply; 46+ messages in thread From: Jiri Benc @ 2014-05-22 15:05 UTC (permalink / raw) To: Andy Lutomirski Cc: Linus Torvalds, David Miller, Jorge Boncompte [DTI2], Eric W. Biederman, Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn On Wed, 7 May 2014 16:45:10 -0700, Andy Lutomirski wrote: > It looks like Zebra is mucking with its effective set. It creates the > socket w/o effective caps, raises them to bind (no clue why), lowers > them post-bind, then raises them again to sendto. Presumably the new > checks cause the sendto to fail b/c the socket()-time credentials were > insufficient. > > I don't see why it bound w/ elevated permissions, since I don't think > this is important. And it never connected at all. > > Hence my suggestion that we check permissions at connect time instead > of socket() time and that we just check send-time permissions on an > unconnected socket. Yes, this is awful. AFAIK this is still unresolved and this seems to be the only solution proposed so far that fixes the security problem and does not break zebra. Or have I missed something and the conclusion is zebra needs to be modified to comply with the changed semantics? Jiri -- Jiri Benc ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-05-22 15:05 ` Jiri Benc @ 2014-05-23 23:25 ` Eric W. Biederman 2014-05-23 23:51 ` Linus Torvalds 0 siblings, 1 reply; 46+ messages in thread From: Eric W. Biederman @ 2014-05-23 23:25 UTC (permalink / raw) To: Jiri Benc Cc: Andy Lutomirski, Linus Torvalds, David Miller, Jorge Boncompte [DTI2], Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn Jiri Benc <jbenc@redhat.com> writes: > On Wed, 7 May 2014 16:45:10 -0700, Andy Lutomirski wrote: >> It looks like Zebra is mucking with its effective set. It creates the >> socket w/o effective caps, raises them to bind (no clue why), lowers >> them post-bind, then raises them again to sendto. Presumably the new >> checks cause the sendto to fail b/c the socket()-time credentials were >> insufficient. >> >> I don't see why it bound w/ elevated permissions, since I don't think >> this is important. And it never connected at all. >> >> Hence my suggestion that we check permissions at connect time instead >> of socket() time and that we just check send-time permissions on an >> unconnected socket. Yes, this is awful. > > AFAIK this is still unresolved and this seems to be the only solution > proposed so far that fixes the security problem and does not break > zebra. Or have I missed something and the conclusion is zebra needs to > be modified to comply with the changed semantics? I have not seen consensus that what Zebra is doing makes sense to support. What Zebra is doing (opening a socket without privilege and then sending privileged messages over that socket) smells. This is effecting performing trusted operations over an untrusted channel. I can not see why that would ever be a desirable application architecture. There is also historical precedent for what the kernel is doing as it is effectively checking SO_PEERCRED. Furthermore I have not seen even a bad implementation of a way to remember who called connect, and the possibility of needing different permission checks for send vs sendto makes any implementation I can think of nasty and ugly. Which generally translates into complicated and not secure. So short of someone having a better idea it looks like Zebra is broken until it is patched to open sockets with privilege when Zerbra is going to send privileged messages over those sockets. Eric ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-05-23 23:25 ` Eric W. Biederman @ 2014-05-23 23:51 ` Linus Torvalds 2014-05-24 22:34 ` David Miller ` (2 more replies) 0 siblings, 3 replies; 46+ messages in thread From: Linus Torvalds @ 2014-05-23 23:51 UTC (permalink / raw) To: Eric W. Biederman Cc: Jiri Benc, Andy Lutomirski, David Miller, Jorge Boncompte [DTI2], Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn On Fri, May 23, 2014 at 4:25 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > > I have not seen consensus that what Zebra is doing makes sense to > support. Eric, stop right there. There is no "sensible to support". There is only "reality". The thing that makes "reality" be "reality" is that it exists whether you like it or not, or whether you believe in it or not. We don't break applications. Whether you like them or not is completely immaterial. Linus ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-05-23 23:51 ` Linus Torvalds @ 2014-05-24 22:34 ` David Miller 2014-05-25 5:38 ` [RFC][PATCH] netlink: Only check file credentials for implicit destinations Eric W. Biederman 2014-05-25 5:45 ` [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages Eric W. Biederman 2 siblings, 0 replies; 46+ messages in thread From: David Miller @ 2014-05-24 22:34 UTC (permalink / raw) To: torvalds Cc: ebiederm, jbenc, luto, jorge, vgoyal, ssorce, security, netdev, serge From: Linus Torvalds <torvalds@linux-foundation.org> Date: Fri, 23 May 2014 16:51:17 -0700 > On Fri, May 23, 2014 at 4:25 PM, Eric W. Biederman > <ebiederm@xmission.com> wrote: >> >> I have not seen consensus that what Zebra is doing makes sense to >> support. > > Eric, stop right there. > > There is no "sensible to support". There is only "reality". > > The thing that makes "reality" be "reality" is that it exists whether > you like it or not, or whether you believe in it or not. > > We don't break applications. Whether you like them or not is > completely immaterial. Agreed, we have to either implement Andy's suggestion (permission check at connect() time for connected sockets, and at send() time for unconnected sockets) or revert the behavioral change completely. ^ permalink raw reply [flat|nested] 46+ messages in thread
* [RFC][PATCH] netlink: Only check file credentials for implicit destinations 2014-05-23 23:51 ` Linus Torvalds 2014-05-24 22:34 ` David Miller @ 2014-05-25 5:38 ` Eric W. Biederman 2014-05-25 16:50 ` Andy Lutomirski 2014-05-25 5:45 ` [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages Eric W. Biederman 2 siblings, 1 reply; 46+ messages in thread From: Eric W. Biederman @ 2014-05-25 5:38 UTC (permalink / raw) To: Jorge Boncompte [DTI2], Jiri Benc Cc: Andy Lutomirski, David Miller, Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn, Linus Torvalds 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. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- Could someone please test and see if this fixes the Zebra case? So far I have only compile tested this, but the code should work, unless Zebra is doing something that is so absolutely weird that there is no way to close the security holes and keep Zebra working. include/linux/netlink.h | 1 + net/netlink/af_netlink.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/include/linux/netlink.h b/include/linux/netlink.h index 7a28115dd396..f289d085f87f 100644 --- a/include/linux/netlink.h +++ b/include/linux/netlink.h @@ -19,6 +19,7 @@ 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_DST = 0x8, /* Packet not socket destination */ }; struct netlink_skb_parms { diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index e0ccd84d4d67..15c731f03fa6 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -1377,7 +1377,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); @@ -2323,6 +2325,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; @@ -2344,6 +2347,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; @@ -2373,6 +2377,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)) { -- 1.9.1 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [RFC][PATCH] netlink: Only check file credentials for implicit destinations 2014-05-25 5:38 ` [RFC][PATCH] netlink: Only check file credentials for implicit destinations Eric W. Biederman @ 2014-05-25 16:50 ` Andy Lutomirski 2014-05-25 23:44 ` Eric W. Biederman 2014-05-26 8:38 ` Michael Kerrisk (man-pages) 0 siblings, 2 replies; 46+ messages in thread From: Andy Lutomirski @ 2014-05-25 16:50 UTC (permalink / raw) To: Eric W. Biederman Cc: Jorge Boncompte [DTI2], Jiri Benc, David Miller, Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn, Linus Torvalds, Michael Kerrisk-manpages On Sat, May 24, 2014 at 10:38 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > > 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. > Cute. > + NETLINK_SKB_DST = 0x8, /* Packet not socket destination */ How about "sendto/sendmsg with explicit destination" Whatever we settle on, I think this'll need to end up in the man pages. Cc: Michael Kerrisk. I hereby volunteer to write something up. Michael, for background: Pre-linux-3.15, sending netlink messages to the kernel checked the credentials of the sender. This is a security bug: the sender might be a setuid-root program with stdout or stderr redirected to a netlink socket (or an SCM_RIGHTS user, etc). The proposal in this patch is that doing privileged things using a netlink socket will require the sender to have capabilities and (either sendto/sendmsg with an explicit destination or a connected socket that was created by a privileged user). This is still not great from a security POV: if you can get a hold of a privileged socket (i.e. a socket created with CAP_NET_ADMIN available), then you can connect it and try to attack the kernel. This issue would go away if we hooked netlink_connect. I can try writing up that version of the patch tomorrow. --Andy ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC][PATCH] netlink: Only check file credentials for implicit destinations 2014-05-25 16:50 ` Andy Lutomirski @ 2014-05-25 23:44 ` Eric W. Biederman 2014-05-26 0:32 ` Linus Torvalds 2014-05-26 8:38 ` Michael Kerrisk (man-pages) 1 sibling, 1 reply; 46+ messages in thread From: Eric W. Biederman @ 2014-05-25 23:44 UTC (permalink / raw) To: Andy Lutomirski Cc: Jorge Boncompte [DTI2], Jiri Benc, David Miller, Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn, Linus Torvalds, Michael Kerrisk-manpages Andy Lutomirski <luto@amacapital.net> writes: > On Sat, May 24, 2014 at 10:38 PM, Eric W. Biederman > <ebiederm@xmission.com> wrote: >> >> 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. >> > > Cute. > >> + NETLINK_SKB_DST = 0x8, /* Packet not socket destination */ > > How about "sendto/sendmsg with explicit destination" That is a better comment. > Whatever we settle on, I think this'll need to end up in the man > pages. Cc: Michael Kerrisk. I hereby volunteer to write something > up. > > Michael, for background: Pre-linux-3.15, sending netlink messages to > the kernel checked the credentials of the sender. This is a security > bug: the sender might be a setuid-root program with stdout or stderr > redirected to a netlink socket (or an SCM_RIGHTS user, etc). > > The proposal in this patch is that doing privileged things using a > netlink socket will require the sender to have capabilities and > (either sendto/sendmsg with an explicit destination or a connected > socket that was created by a privileged user). > > This is still not great from a security POV: if you can get a hold of > a privileged socket (i.e. a socket created with CAP_NET_ADMIN > available), then you can connect it and try to attack the kernel. > This issue would go away if we hooked netlink_connect. I can try > writing up that version of the patch tomorrow. The really important part technically is skipping the socket based check in sendto and sendmsg as I have done, as it fixes the one known regression, and yields comprehensible semantics. But I agree that since connect on sockets is really the equivalent of open on files, and unprivileged users can change where a socket is connected to, using a struct cred captured at connect() time is better than the struct cred captured at socket() time. Eric ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC][PATCH] netlink: Only check file credentials for implicit destinations 2014-05-25 23:44 ` Eric W. Biederman @ 2014-05-26 0:32 ` Linus Torvalds 2014-05-26 5:36 ` [RFC][PATCH 2/1] netlink: Use the credential at the time the destination address was set Eric W. Biederman 2014-05-26 13:39 ` [RFC][PATCH] netlink: Only check file credentials for implicit destinations Willy Tarreau 0 siblings, 2 replies; 46+ messages in thread From: Linus Torvalds @ 2014-05-26 0:32 UTC (permalink / raw) To: Eric W. Biederman Cc: Andy Lutomirski, Jorge Boncompte [DTI2], Jiri Benc, David Miller, Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn, Michael Kerrisk-manpages On Sun, May 25, 2014 at 4:44 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > > But I agree that since connect on sockets is really the equivalent of > open on files, and unprivileged users can change where a socket is > connected to, using a struct cred captured at connect() time is better > than the struct cred captured at socket() time. Ack. Conceptually, "connect/listen" really ends up being the equivalent to pathname lookup, not so much "socket()", which just mostly creates the placeholder for future work. That would also be very much consistent with making "sendto" look at current creds rather than cached creds (but only _if_ it has an address, of course - using "sendto(... , NULL, 0)" should _not_ somehow be different from "send()"). So I think that from a sensibility and "please explain the semantics to me" standpoint, that would be sane semantics. Linus ^ permalink raw reply [flat|nested] 46+ messages in thread
* [RFC][PATCH 2/1] netlink: Use the credential at the time the destination address was set. 2014-05-26 0:32 ` Linus Torvalds @ 2014-05-26 5:36 ` Eric W. Biederman 2014-05-26 17:19 ` Andy Lutomirski 2014-05-26 13:39 ` [RFC][PATCH] netlink: Only check file credentials for implicit destinations Willy Tarreau 1 sibling, 1 reply; 46+ messages in thread From: Eric W. Biederman @ 2014-05-26 5:36 UTC (permalink / raw) To: Linus Torvalds Cc: Andy Lutomirski, Jorge Boncompte [DTI2], Jiri Benc, David Miller, Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn, Michael Kerrisk-manpages When sending a message over a netlink socket and then checking to see if the person is authorized to send the message it is important that we verify both the sender of the message and the whoever it is that set the destination of the message both have permission. Otherwise it becomes possible for an unpriivleged user to set the message destination and trick an suid process to write to the socket and change the network connection. For netlink sockets socket() sets the default destination address to 0 (the kernel) so we need to remember the credentials when a socket is created. For netlink sockets connect() changes the default destination address so we need to remember the credentials of the last changer of the default destination with connect. This results is there always being a valid remembered credential on the socket and so that credential is unconditionally freed in netlink_release(). This change makes the semantics of the permission checks of netlink sockets make sense,and removes the possibility of an unprivileged user getting access to a root own socket and changing the destination address with connect. Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- This winds up continuig to grab credentials when socket() is called because we actually set the destination address in socket() for netlink sockets, but now we update those credentials when connect() is called. include/linux/netlink.h | 2 +- net/netlink/af_netlink.c | 27 ++++++++++++++++++++++++--- net/netlink/af_netlink.h | 1 + 3 files changed, 26 insertions(+), 4 deletions(-) diff --git a/include/linux/netlink.h b/include/linux/netlink.h index f289d085f87f..4f4607c0a1a1 100644 --- a/include/linux/netlink.h +++ b/include/linux/netlink.h @@ -19,7 +19,7 @@ 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_DST = 0x8, /* Packet not socket destination */ + 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 15c731f03fa6..5b886ed6a648 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -1191,6 +1191,7 @@ static int __netlink_create(struct net *net, struct socket *sock, mutex_init(nlk->cb_mutex); } init_waitqueue_head(&nlk->wait); + nlk->cred = get_current_cred(); #ifdef CONFIG_NETLINK_MMAP mutex_init(&nlk->pg_vec_lock); #endif @@ -1291,6 +1292,7 @@ static int netlink_release(struct socket *sock) NETLINK_URELEASE, &n); } + put_cred(nlk->cred); module_put(nlk->module); netlink_table_grab(); @@ -1377,9 +1379,17 @@ retry: bool __netlink_ns_capable(const struct netlink_skb_parms *nsp, struct user_namespace *user_ns, int cap) { - return ((nsp->flags & NETLINK_SKB_DST) || - file_ns_capable(nsp->sk->sk_socket->file, user_ns, cap)) && - ns_capable(user_ns, cap); + const struct cred *cred; + bool capable; + + rcu_read_lock(); + cred = nlk_sk(nsp->sk)->cred; + capable = ((nsp->flags & NETLINK_SKB_DST) || + security_capable(cred, user_ns, cap)) && + ns_capable(user_ns, cap); + rcu_read_unlock(); + + return capable; } EXPORT_SYMBOL(__netlink_ns_capable); @@ -1569,14 +1579,20 @@ static int netlink_connect(struct socket *sock, struct sockaddr *addr, struct sock *sk = sock->sk; struct netlink_sock *nlk = nlk_sk(sk); struct sockaddr_nl *nladdr = (struct sockaddr_nl *)addr; + const struct cred *old_cred; if (alen < sizeof(addr->sa_family)) return -EINVAL; if (addr->sa_family == AF_UNSPEC) { + lock_sock(sk); sk->sk_state = NETLINK_UNCONNECTED; nlk->dst_portid = 0; nlk->dst_group = 0; + old_cred = nlk->cred; + nlk->cred = get_current_cred(); + put_cred(old_cred); + release_sock(sk); return 0; } if (addr->sa_family != AF_NETLINK) @@ -1590,9 +1606,14 @@ static int netlink_connect(struct socket *sock, struct sockaddr *addr, err = netlink_autobind(sock); if (err == 0) { + lock_sock(sk); sk->sk_state = NETLINK_CONNECTED; nlk->dst_portid = nladdr->nl_pid; nlk->dst_group = ffs(nladdr->nl_groups); + old_cred = nlk->cred; + nlk->cred = get_current_cred(); + put_cred(old_cred); + release_sock(sk); } return err; diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h index 0b59d441f5b6..d3c9d319e9e6 100644 --- a/net/netlink/af_netlink.h +++ b/net/netlink/af_netlink.h @@ -40,6 +40,7 @@ struct netlink_sock { void (*netlink_rcv)(struct sk_buff *skb); int (*netlink_bind)(int group); void (*netlink_unbind)(int group); + const struct cred *cred; struct module *module; #ifdef CONFIG_NETLINK_MMAP struct mutex pg_vec_lock; -- 1.9.1 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [RFC][PATCH 2/1] netlink: Use the credential at the time the destination address was set. 2014-05-26 5:36 ` [RFC][PATCH 2/1] netlink: Use the credential at the time the destination address was set Eric W. Biederman @ 2014-05-26 17:19 ` Andy Lutomirski 2014-05-27 4:24 ` Eric W. Biederman 0 siblings, 1 reply; 46+ messages in thread From: Andy Lutomirski @ 2014-05-26 17:19 UTC (permalink / raw) To: Eric W. Biederman Cc: Linus Torvalds, Jorge Boncompte [DTI2], Jiri Benc, David Miller, Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn, Michael Kerrisk-manpages On Sun, May 25, 2014 at 10:36 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > > When sending a message over a netlink socket and then checking to see if > the person is authorized to send the message it is important that we verify > both the sender of the message and the whoever it is that set the destination > of the message both have permission. Otherwise it becomes possible for an > unpriivleged user to set the message destination and trick an suid process > to write to the socket and change the network connection. > > For netlink sockets socket() sets the default destination address to 0 (the kernel) > so we need to remember the credentials when a socket is created. > > For netlink sockets connect() changes the default destination address so > we need to remember the credentials of the last changer of the default destination > with connect. > > This results is there always being a valid remembered credential on the socket > and so that credential is unconditionally freed in netlink_release(). > > This change makes the semantics of the permission checks of netlink sockets > make sense,and removes the possibility of an unprivileged user getting access > to a root own socket and changing the destination address with connect. > > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > --- > > This winds up continuig to grab credentials when socket() is called > because we actually set the destination address in socket() for netlink > sockets, but now we update those credentials when connect() is called. > > include/linux/netlink.h | 2 +- > net/netlink/af_netlink.c | 27 ++++++++++++++++++++++++--- > net/netlink/af_netlink.h | 1 + > 3 files changed, 26 insertions(+), 4 deletions(-) > > diff --git a/include/linux/netlink.h b/include/linux/netlink.h > index f289d085f87f..4f4607c0a1a1 100644 > --- a/include/linux/netlink.h > +++ b/include/linux/netlink.h > @@ -19,7 +19,7 @@ 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_DST = 0x8, /* Packet not socket destination */ > + 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 15c731f03fa6..5b886ed6a648 100644 > --- a/net/netlink/af_netlink.c > +++ b/net/netlink/af_netlink.c > @@ -1191,6 +1191,7 @@ static int __netlink_create(struct net *net, struct socket *sock, > mutex_init(nlk->cb_mutex); > } > init_waitqueue_head(&nlk->wait); > + nlk->cred = get_current_cred(); > #ifdef CONFIG_NETLINK_MMAP > mutex_init(&nlk->pg_vec_lock); > #endif > @@ -1291,6 +1292,7 @@ static int netlink_release(struct socket *sock) > NETLINK_URELEASE, &n); > } > > + put_cred(nlk->cred); > module_put(nlk->module); > > netlink_table_grab(); > @@ -1377,9 +1379,17 @@ retry: > bool __netlink_ns_capable(const struct netlink_skb_parms *nsp, > struct user_namespace *user_ns, int cap) > { > - return ((nsp->flags & NETLINK_SKB_DST) || > - file_ns_capable(nsp->sk->sk_socket->file, user_ns, cap)) && > - ns_capable(user_ns, cap); > + const struct cred *cred; > + bool capable; > + > + rcu_read_lock(); > + cred = nlk_sk(nsp->sk)->cred; > + capable = ((nsp->flags & NETLINK_SKB_DST) || > + security_capable(cred, user_ns, cap)) && > + ns_capable(user_ns, cap); > + rcu_read_unlock(); > + > + return capable; > } > EXPORT_SYMBOL(__netlink_ns_capable); > > @@ -1569,14 +1579,20 @@ static int netlink_connect(struct socket *sock, struct sockaddr *addr, > struct sock *sk = sock->sk; > struct netlink_sock *nlk = nlk_sk(sk); > struct sockaddr_nl *nladdr = (struct sockaddr_nl *)addr; > + const struct cred *old_cred; > > if (alen < sizeof(addr->sa_family)) > return -EINVAL; > > if (addr->sa_family == AF_UNSPEC) { > + lock_sock(sk); > sk->sk_state = NETLINK_UNCONNECTED; > nlk->dst_portid = 0; > nlk->dst_group = 0; > + old_cred = nlk->cred; > + nlk->cred = get_current_cred(); > + put_cred(old_cred); > + release_sock(sk); > return 0; I'm confused about the locking. Either lock_sock prevents concurrent sendmsg, in which case the rcu_read_lock should be unnecessary, or it doesn't, in which case I think this is racy, because sendmsg could see a cred and dst_whatever that don't match. Admittedly, exploiting that race is probably extremely difficult. If the RCU protection is needed, shouldn't this use rcu_assign_pointer and rcu_dereference? --Andy ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC][PATCH 2/1] netlink: Use the credential at the time the destination address was set. 2014-05-26 17:19 ` Andy Lutomirski @ 2014-05-27 4:24 ` Eric W. Biederman 0 siblings, 0 replies; 46+ messages in thread From: Eric W. Biederman @ 2014-05-27 4:24 UTC (permalink / raw) To: Andy Lutomirski Cc: Linus Torvalds, Jorge Boncompte [DTI2], Jiri Benc, David Miller, Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn, Michael Kerrisk-manpages Andy Lutomirski <luto@amacapital.net> writes: > On Sun, May 25, 2014 at 10:36 PM, Eric W. Biederman > <ebiederm@xmission.com> wrote: >> >> When sending a message over a netlink socket and then checking to see if >> the person is authorized to send the message it is important that we verify >> both the sender of the message and the whoever it is that set the destination >> of the message both have permission. Otherwise it becomes possible for an >> unpriivleged user to set the message destination and trick an suid process >> to write to the socket and change the network connection. >> >> For netlink sockets socket() sets the default destination address to 0 (the kernel) >> so we need to remember the credentials when a socket is created. >> >> For netlink sockets connect() changes the default destination address so >> we need to remember the credentials of the last changer of the default destination >> with connect. >> >> This results is there always being a valid remembered credential on the socket >> and so that credential is unconditionally freed in netlink_release(). >> >> This change makes the semantics of the permission checks of netlink sockets >> make sense,and removes the possibility of an unprivileged user getting access >> to a root own socket and changing the destination address with connect. >> >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> >> --- >> >> This winds up continuig to grab credentials when socket() is called >> because we actually set the destination address in socket() for netlink >> sockets, but now we update those credentials when connect() is called. >> >> include/linux/netlink.h | 2 +- >> net/netlink/af_netlink.c | 27 ++++++++++++++++++++++++--- >> net/netlink/af_netlink.h | 1 + >> 3 files changed, 26 insertions(+), 4 deletions(-) >> >> diff --git a/include/linux/netlink.h b/include/linux/netlink.h >> index f289d085f87f..4f4607c0a1a1 100644 >> --- a/include/linux/netlink.h >> +++ b/include/linux/netlink.h >> @@ -19,7 +19,7 @@ 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_DST = 0x8, /* Packet not socket destination */ >> + 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 15c731f03fa6..5b886ed6a648 100644 >> --- a/net/netlink/af_netlink.c >> +++ b/net/netlink/af_netlink.c >> @@ -1191,6 +1191,7 @@ static int __netlink_create(struct net *net, struct socket *sock, >> mutex_init(nlk->cb_mutex); >> } >> init_waitqueue_head(&nlk->wait); >> + nlk->cred = get_current_cred(); >> #ifdef CONFIG_NETLINK_MMAP >> mutex_init(&nlk->pg_vec_lock); >> #endif >> @@ -1291,6 +1292,7 @@ static int netlink_release(struct socket *sock) >> NETLINK_URELEASE, &n); >> } >> >> + put_cred(nlk->cred); >> module_put(nlk->module); >> >> netlink_table_grab(); >> @@ -1377,9 +1379,17 @@ retry: >> bool __netlink_ns_capable(const struct netlink_skb_parms *nsp, >> struct user_namespace *user_ns, int cap) >> { >> - return ((nsp->flags & NETLINK_SKB_DST) || >> - file_ns_capable(nsp->sk->sk_socket->file, user_ns, cap)) && >> - ns_capable(user_ns, cap); >> + const struct cred *cred; >> + bool capable; >> + >> + rcu_read_lock(); >> + cred = nlk_sk(nsp->sk)->cred; >> + capable = ((nsp->flags & NETLINK_SKB_DST) || >> + security_capable(cred, user_ns, cap)) && >> + ns_capable(user_ns, cap); >> + rcu_read_unlock(); >> + >> + return capable; >> } >> EXPORT_SYMBOL(__netlink_ns_capable); >> >> @@ -1569,14 +1579,20 @@ static int netlink_connect(struct socket *sock, struct sockaddr *addr, >> struct sock *sk = sock->sk; >> struct netlink_sock *nlk = nlk_sk(sk); >> struct sockaddr_nl *nladdr = (struct sockaddr_nl *)addr; >> + const struct cred *old_cred; >> >> if (alen < sizeof(addr->sa_family)) >> return -EINVAL; >> >> if (addr->sa_family == AF_UNSPEC) { >> + lock_sock(sk); >> sk->sk_state = NETLINK_UNCONNECTED; >> nlk->dst_portid = 0; >> nlk->dst_group = 0; >> + old_cred = nlk->cred; >> + nlk->cred = get_current_cred(); >> + put_cred(old_cred); >> + release_sock(sk); >> return 0; > > I'm confused about the locking. Either lock_sock prevents concurrent > sendmsg, in which case the rcu_read_lock should be unnecessary, or it > doesn't, in which case I think this is racy, because sendmsg could see > a cred and dst_whatever that don't match. Admittedly, exploiting that > race is probably extremely difficult. My intention was to prevent races during connect which can cause the wrong cred to be freed, which could be pretty nasty. The intention of the rcu bits was just to guarantee that the cred doesn't get freed during the write. As for mismatches. There is only an issue with write/send that does not specify a destination address. Although I admit sendmsg differing could be a problem as well. Getting the races out of the send path without destroying performance is beyond my imagination today. It would be nice if we could make the locking simply that connect waits until sendmmsg and similar functions are complete preventing races. I hate interfaces where things can be set multiple times on a kernel data structure the races are absolutely horrible. > If the RCU protection is needed, shouldn't this use rcu_assign_pointer > and rcu_dereference? Yep that would be good. Any chance you could pick this up. I am going to be busy for the next couple of days. Eric ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC][PATCH] netlink: Only check file credentials for implicit destinations 2014-05-26 0:32 ` Linus Torvalds 2014-05-26 5:36 ` [RFC][PATCH 2/1] netlink: Use the credential at the time the destination address was set Eric W. Biederman @ 2014-05-26 13:39 ` Willy Tarreau 1 sibling, 0 replies; 46+ messages in thread From: Willy Tarreau @ 2014-05-26 13:39 UTC (permalink / raw) To: Linus Torvalds Cc: Eric W. Biederman, Andy Lutomirski, Jorge Boncompte [DTI2], Jiri Benc, David Miller, Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn, Michael Kerrisk-manpages On Sun, May 25, 2014 at 05:32:55PM -0700, Linus Torvalds wrote: > On Sun, May 25, 2014 at 4:44 PM, Eric W. Biederman > <ebiederm@xmission.com> wrote: > > > > But I agree that since connect on sockets is really the equivalent of > > open on files, and unprivileged users can change where a socket is > > connected to, using a struct cred captured at connect() time is better > > than the struct cred captured at socket() time. > > Ack. Conceptually, "connect/listen" really ends up being the > equivalent to pathname lookup, not so much "socket()", which just > mostly creates the placeholder for future work. > > That would also be very much consistent with making "sendto" look at > current creds rather than cached creds (but only _if_ it has an > address, of course - using "sendto(... , NULL, 0)" should _not_ > somehow be different from "send()"). So I think that from a > sensibility and "please explain the semantics to me" standpoint, that > would be sane semantics. I like this! And it's very much consistent with sendto() being used as an alternative to connect() with TCP Fastopen. Willy ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [RFC][PATCH] netlink: Only check file credentials for implicit destinations 2014-05-25 16:50 ` Andy Lutomirski 2014-05-25 23:44 ` Eric W. Biederman @ 2014-05-26 8:38 ` Michael Kerrisk (man-pages) 1 sibling, 0 replies; 46+ messages in thread From: Michael Kerrisk (man-pages) @ 2014-05-26 8:38 UTC (permalink / raw) To: Andy Lutomirski, Eric W. Biederman Cc: mtk.manpages, Jorge Boncompte [DTI2], Jiri Benc, David Miller, Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn, Linus Torvalds On 05/25/2014 06:50 PM, Andy Lutomirski wrote: > On Sat, May 24, 2014 at 10:38 PM, Eric W. Biederman > <ebiederm@xmission.com> wrote: >> >> 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. >> > > Cute. > >> + NETLINK_SKB_DST = 0x8, /* Packet not socket destination */ > > How about "sendto/sendmsg with explicit destination" > > Whatever we settle on, I think this'll need to end up in the man > pages. Cc: Michael Kerrisk. I hereby volunteer to write something > up. > > Michael, for background: Pre-linux-3.15, sending netlink messages to > the kernel checked the credentials of the sender. This is a security > bug: the sender might be a setuid-root program with stdout or stderr > redirected to a netlink socket (or an SCM_RIGHTS user, etc). Andy, thanks for putting your hand-up, and thanks especially for paragraph of background. (Too often, I get CCed into a thread with the implication that something needs to be fixed in man-pages without any explanation of what or why.) Cheers, Michael > The proposal in this patch is that doing privileged things using a > netlink socket will require the sender to have capabilities and > (either sendto/sendmsg with an explicit destination or a connected > socket that was created by a privileged user). > > This is still not great from a security POV: if you can get a hold of > a privileged socket (i.e. a socket created with CAP_NET_ADMIN > available), then you can connect it and try to attack the kernel. > This issue would go away if we hooked netlink_connect. I can try > writing up that version of the patch tomorrow. > > --Andy > -- Michael Kerrisk Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ Linux/UNIX System Programming Training: http://man7.org/training/ ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-05-23 23:51 ` Linus Torvalds 2014-05-24 22:34 ` David Miller 2014-05-25 5:38 ` [RFC][PATCH] netlink: Only check file credentials for implicit destinations Eric W. Biederman @ 2014-05-25 5:45 ` Eric W. Biederman 2014-05-25 16:27 ` Andy Lutomirski 2 siblings, 1 reply; 46+ messages in thread From: Eric W. Biederman @ 2014-05-25 5:45 UTC (permalink / raw) To: Linus Torvalds Cc: Jiri Benc, Andy Lutomirski, David Miller, Jorge Boncompte [DTI2], Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn Linus Torvalds <torvalds@linux-foundation.org> writes: > On Fri, May 23, 2014 at 4:25 PM, Eric W. Biederman > <ebiederm@xmission.com> wrote: >> >> I have not seen consensus that what Zebra is doing makes sense to >> support. > > Eric, stop right there. > > There is no "sensible to support". There is only "reality". > > The thing that makes "reality" be "reality" is that it exists whether > you like it or not, or whether you believe in it or not. > > We don't break applications. Whether you like them or not is > completely immaterial. You stop right there. You are shooting the messenger. I like Zebra just fine, and I hate breaking applications. We don't retain bug compatibility when the semantics of kernel interfaces are security vulnerabilities. I don't appreciate being shot when I am just the messenger saying that there is not a known fix for Zebra, that it might be unfixable, and that no one had thought of a anything. What Andy Lutormiski suggested of checking permissions at connect time will break a whole lot more than just Zebra. Unprivileged connect is a supported feature in netlink, and all information rtnetlink queries are non-privileged as is listening to rtnetlink brodacsts of network state chagnes. In concrete form, no special privileges are requires to run "ip link" or "ip monitor". Those among other commands are what Andy has proposed breaking, all in the name of "supporting" Zebra. I care just enough I have thrown a patch over the wall and we will see if it sticks. Eric ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-05-25 5:45 ` [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages Eric W. Biederman @ 2014-05-25 16:27 ` Andy Lutomirski 0 siblings, 0 replies; 46+ messages in thread From: Andy Lutomirski @ 2014-05-25 16:27 UTC (permalink / raw) To: Eric W. Biederman Cc: Linus Torvalds, Jiri Benc, David Miller, Jorge Boncompte [DTI2], Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn On Sat, May 24, 2014 at 10:45 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > Linus Torvalds <torvalds@linux-foundation.org> writes: > >> On Fri, May 23, 2014 at 4:25 PM, Eric W. Biederman >> <ebiederm@xmission.com> wrote: >>> >>> I have not seen consensus that what Zebra is doing makes sense to >>> support. >> >> Eric, stop right there. >> >> There is no "sensible to support". There is only "reality". >> >> The thing that makes "reality" be "reality" is that it exists whether >> you like it or not, or whether you believe in it or not. >> >> We don't break applications. Whether you like them or not is >> completely immaterial. > > You stop right there. You are shooting the messenger. > > I like Zebra just fine, and I hate breaking applications. > > We don't retain bug compatibility when the semantics of kernel > interfaces are security vulnerabilities. > > I don't appreciate being shot when I am just the messenger saying that > there is not a known fix for Zebra, that it might be unfixable, and that > no one had thought of a anything. > > What Andy Lutormiski suggested of checking permissions at connect time > will break a whole lot more than just Zebra. Unprivileged connect is a > supported feature in netlink, and all information rtnetlink queries > are non-privileged as is listening to rtnetlink brodacsts of network > state chagnes. I'm suggesting that the connect syscall store creds or maybe just the outcome of ns_capable(socket's netns, CAP_NET_ADMIN). Yes, this is ugly and kludgey. I suspect that damn near nothing actually calls connect on netlink sockets, except for my PoC exploit. Unprivileged sendto and bind are important features, but they're not the same thing. In any event, I'm not actually proposing removing the ability for unprivileged programs to call connect on netlink sockets. > > In concrete form, no special privileges are requires to run "ip link" or > "ip monitor". Those among other commands are what Andy has proposed > breaking, all in the name of "supporting" Zebra. Nope. See above. ip monitor doesn't call connect. Neither does ip link, at least in its iproute2 incarnation. --Andy ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-05-07 23:34 ` Linus Torvalds 2014-05-07 23:45 ` Andy Lutomirski @ 2014-05-08 21:29 ` Stephen Hemminger 2014-05-08 21:32 ` Andy Lutomirski [not found] ` <CA+55aFzOHZcw2o6Cq6rSddSBDZvhgzYToBruak9SLCHxx-fA3Q@mail.gmail.com> 1 sibling, 2 replies; 46+ messages in thread From: Stephen Hemminger @ 2014-05-08 21:29 UTC (permalink / raw) To: Linus Torvalds Cc: Andy Lutomirski, David Miller, Jorge Boncompte [DTI2], Eric W. Biederman, Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn On Wed, 7 May 2014 16:34:08 -0700 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Wed, May 7, 2014 at 4:01 PM, Andy Lutomirski <luto@amacapital.net> wrote: > > > > I agree that it should, but it doesn't, and if these patches get > > backported, things will break. OTOH, if the patches don't get > > backported, things may still break, and we have a possibly rather > > severe unfixed vulnerability. > > How did this *use* to work? It looks like it drops permissions after > the bind(), so the actual _IO_ must have always been done without > permissions, no? > > Is it just a bind-time permission check that is now failing, because > it uses the credentials associated with the socket open? If so, I'd > suggest unding just the ns-capable change for bind(), and make that > one always use the current process effective one. > > If you're a suid application, you're not doing "bind()" on random file > descriptors that were passed to you. It's really just read/write that > need to be careful. > > Linus Quagga drops privileges at startup then selectively raises them. The code is doing raise caps in netlink code for bind and each sendto and recvmsg call. Ideally it should be able to not have to raise/lower on each send/recvmsg call. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-05-08 21:29 ` Stephen Hemminger @ 2014-05-08 21:32 ` Andy Lutomirski [not found] ` <CA+55aFzOHZcw2o6Cq6rSddSBDZvhgzYToBruak9SLCHxx-fA3Q@mail.gmail.com> 1 sibling, 0 replies; 46+ messages in thread From: Andy Lutomirski @ 2014-05-08 21:32 UTC (permalink / raw) To: Stephen Hemminger Cc: Linus Torvalds, David Miller, Jorge Boncompte [DTI2], Eric W. Biederman, Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn On Thu, May 8, 2014 at 2:29 PM, Stephen Hemminger <stephen@networkplumber.org> wrote: > On Wed, 7 May 2014 16:34:08 -0700 > Linus Torvalds <torvalds@linux-foundation.org> wrote: > >> On Wed, May 7, 2014 at 4:01 PM, Andy Lutomirski <luto@amacapital.net> wrote: >> > >> > I agree that it should, but it doesn't, and if these patches get >> > backported, things will break. OTOH, if the patches don't get >> > backported, things may still break, and we have a possibly rather >> > severe unfixed vulnerability. >> >> How did this *use* to work? It looks like it drops permissions after >> the bind(), so the actual _IO_ must have always been done without >> permissions, no? >> >> Is it just a bind-time permission check that is now failing, because >> it uses the credentials associated with the socket open? If so, I'd >> suggest unding just the ns-capable change for bind(), and make that >> one always use the current process effective one. >> >> If you're a suid application, you're not doing "bind()" on random file >> descriptors that were passed to you. It's really just read/write that >> need to be careful. >> >> Linus > > Quagga drops privileges at startup then selectively raises them. > The code is doing raise caps in netlink code for bind and each sendto > and recvmsg call. > > Ideally it should be able to not have to raise/lower on each send/recvmsg > call. > Why doesn't it just raise them and keep them raised? --Andy ^ permalink raw reply [flat|nested] 46+ messages in thread
[parent not found: <CA+55aFzOHZcw2o6Cq6rSddSBDZvhgzYToBruak9SLCHxx-fA3Q@mail.gmail.com>]
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages [not found] ` <CA+55aFzOHZcw2o6Cq6rSddSBDZvhgzYToBruak9SLCHxx-fA3Q@mail.gmail.com> @ 2014-05-08 21:49 ` Andy Lutomirski 2014-05-08 22:07 ` Stephen Hemminger 2014-05-08 21:54 ` David Miller 1 sibling, 1 reply; 46+ messages in thread From: Andy Lutomirski @ 2014-05-08 21:49 UTC (permalink / raw) To: Linus Torvalds Cc: Stephen Hemminger, security, Vivek Goyal, Serge E. Hallyn, Simo Sorce, Eric W. Biederman, Network Development, Jorge Boncompte [DTI2], David Miller On Thu, May 8, 2014 at 2:41 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On May 8, 2014 2:29 PM, "Stephen Hemminger" <stephen@networkplumber.org> > wrote: >> >> Ideally it should be able to not have to raise/lower on each send/recvmsg >> call. > > Well, that's actually what the *new* semantics are: only raise them for the > socket open, then the reads and writes well be done with the elevated > privileges. > > Sadly, the thing apparently does exactly the reverse, which is wasteful > _and_ now breaks with the fixed semantics. > > Annoying. We may have to revert the changes because of the regression, even > though it sounds like the new semantics would actually be preferred for the > very application that regresses... > The current code checks capabilities on socket *and* send{msg,to,}. I think is this because we were worried that dropping privilege might stop working. TBH, I'm starting to think that the real solution is to add a new syscall: sys_net_ctl(int nl_proto, const void *intput, size_t inlen, void *output, size_t outlen) This sends a netlink message to the kernel and returns the response, if any. We could eventually deprecate the use of netlink sockets for anything other than receiving notifications from the kernel and dealing with netfilter logs, queues, and such. Heck, the kernel tinification people might love this. --Andy ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-05-08 21:49 ` Andy Lutomirski @ 2014-05-08 22:07 ` Stephen Hemminger 0 siblings, 0 replies; 46+ messages in thread From: Stephen Hemminger @ 2014-05-08 22:07 UTC (permalink / raw) To: Andy Lutomirski Cc: Linus Torvalds, security, Vivek Goyal, Serge E. Hallyn, Simo Sorce, Eric W. Biederman, Network Development, Jorge Boncompte [DTI2], David Miller On Thu, 8 May 2014 14:49:11 -0700 Andy Lutomirski <luto@amacapital.net> wrote: > TBH, I'm starting to think that the real solution is to add a new syscall: > > sys_net_ctl(int nl_proto, const void *intput, size_t inlen, void > *output, size_t outlen) No. Changing all the applications based on netlink is a really bad idea. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages [not found] ` <CA+55aFzOHZcw2o6Cq6rSddSBDZvhgzYToBruak9SLCHxx-fA3Q@mail.gmail.com> 2014-05-08 21:49 ` Andy Lutomirski @ 2014-05-08 21:54 ` David Miller 1 sibling, 0 replies; 46+ messages in thread From: David Miller @ 2014-05-08 21:54 UTC (permalink / raw) To: torvalds Cc: stephen, luto, security, vgoyal, serge, ssorce, ebiederm, netdev, jorge From: Linus Torvalds <torvalds@linux-foundation.org> Date: Thu, 8 May 2014 14:41:43 -0700 > Annoying. We may have to revert the changes because of the > regression, even though it sounds like the new semantics would > actually be preferred for the very application that regresses... I think we really have to go with the new semantics, personally. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-05-07 23:01 ` Andy Lutomirski 2014-05-07 23:34 ` Linus Torvalds @ 2014-05-07 23:45 ` David Miller 2014-05-08 21:21 ` Stephen Hemminger 1 sibling, 1 reply; 46+ messages in thread From: David Miller @ 2014-05-07 23:45 UTC (permalink / raw) To: luto; +Cc: jorge, ebiederm, vgoyal, ssorce, security, netdev, serge From: Andy Lutomirski <luto@amacapital.net> Date: Wed, 7 May 2014 16:01:33 -0700 > On Wed, May 7, 2014 at 3:52 PM, David Miller <davem@davemloft.net> wrote: >> From: Andy Lutomirski <luto@amacapital.net> >> Date: Wed, 7 May 2014 15:26:11 -0700 >> >>> So what do we do? Check permissions on connect and then use the >>> cached result for send on a connected socket? Check permitted caps >>> instead of effective caps? >> >> It should create the socket after changing perms. > > I agree that it should, but it doesn't, and if these patches get > backported, things will break. OTOH, if the patches don't get > backported, things may still break, and we have a possibly rather > severe unfixed vulnerability. I think the kernel change is justified as the privilege allowance that happened before was very much unintentional and as you've shown us countless times a very real problem that we must fix. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-05-07 23:45 ` David Miller @ 2014-05-08 21:21 ` Stephen Hemminger 2014-05-08 21:52 ` David Miller 0 siblings, 1 reply; 46+ messages in thread From: Stephen Hemminger @ 2014-05-08 21:21 UTC (permalink / raw) To: David Miller Cc: luto, jorge, ebiederm, vgoyal, ssorce, security, netdev, serge On Wed, 07 May 2014 19:45:14 -0400 (EDT) David Miller <davem@davemloft.net> wrote: > From: Andy Lutomirski <luto@amacapital.net> > Date: Wed, 7 May 2014 16:01:33 -0700 > > > On Wed, May 7, 2014 at 3:52 PM, David Miller <davem@davemloft.net> wrote: > >> From: Andy Lutomirski <luto@amacapital.net> > >> Date: Wed, 7 May 2014 15:26:11 -0700 > >> > >>> So what do we do? Check permissions on connect and then use the > >>> cached result for send on a connected socket? Check permitted caps > >>> instead of effective caps? > >> > >> It should create the socket after changing perms. > > > > I agree that it should, but it doesn't, and if these patches get > > backported, things will break. OTOH, if the patches don't get > > backported, things may still break, and we have a possibly rather > > severe unfixed vulnerability. > > I think the kernel change is justified as the privilege allowance > that happened before was very much unintentional and as you've > shown us countless times a very real problem that we must fix. One of the problems here is that Quagga may generate millions of netlink messages to change routes in response to link flap. Raising/lowering the permissions around each request would have a significant performance impact. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-05-08 21:21 ` Stephen Hemminger @ 2014-05-08 21:52 ` David Miller 2014-05-08 21:54 ` Andy Lutomirski 0 siblings, 1 reply; 46+ messages in thread From: David Miller @ 2014-05-08 21:52 UTC (permalink / raw) To: stephen; +Cc: luto, jorge, ebiederm, vgoyal, ssorce, security, netdev, serge From: Stephen Hemminger <stephen@networkplumber.org> Date: Thu, 8 May 2014 14:21:21 -0700 > On Wed, 07 May 2014 19:45:14 -0400 (EDT) > David Miller <davem@davemloft.net> wrote: > >> From: Andy Lutomirski <luto@amacapital.net> >> Date: Wed, 7 May 2014 16:01:33 -0700 >> >> > On Wed, May 7, 2014 at 3:52 PM, David Miller <davem@davemloft.net> wrote: >> >> From: Andy Lutomirski <luto@amacapital.net> >> >> Date: Wed, 7 May 2014 15:26:11 -0700 >> >> >> >>> So what do we do? Check permissions on connect and then use the >> >>> cached result for send on a connected socket? Check permitted caps >> >>> instead of effective caps? >> >> >> >> It should create the socket after changing perms. >> > >> > I agree that it should, but it doesn't, and if these patches get >> > backported, things will break. OTOH, if the patches don't get >> > backported, things may still break, and we have a possibly rather >> > severe unfixed vulnerability. >> >> I think the kernel change is justified as the privilege allowance >> that happened before was very much unintentional and as you've >> shown us countless times a very real problem that we must fix. > > One of the problems here is that Quagga may generate millions of > netlink messages to change routes in response to link flap. > Raising/lowering the permissions around each request would have > a significant performance impact. What we want to matter are the permissions the process had when the socket was created/bound, after that you can drop privs and send netlink requests as you like. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-05-08 21:52 ` David Miller @ 2014-05-08 21:54 ` Andy Lutomirski 0 siblings, 0 replies; 46+ messages in thread From: Andy Lutomirski @ 2014-05-08 21:54 UTC (permalink / raw) To: David Miller Cc: stephen, Jorge Boncompte [DTI2], Eric W. Biederman, Vivek Goyal, Simo Sorce, security, Network Development, Serge E. Hallyn On Thu, May 8, 2014 at 2:52 PM, David Miller <davem@davemloft.net> wrote: > From: Stephen Hemminger <stephen@networkplumber.org> > Date: Thu, 8 May 2014 14:21:21 -0700 > >> On Wed, 07 May 2014 19:45:14 -0400 (EDT) >> David Miller <davem@davemloft.net> wrote: >> >>> From: Andy Lutomirski <luto@amacapital.net> >>> Date: Wed, 7 May 2014 16:01:33 -0700 >>> >>> > On Wed, May 7, 2014 at 3:52 PM, David Miller <davem@davemloft.net> wrote: >>> >> From: Andy Lutomirski <luto@amacapital.net> >>> >> Date: Wed, 7 May 2014 15:26:11 -0700 >>> >> >>> >>> So what do we do? Check permissions on connect and then use the >>> >>> cached result for send on a connected socket? Check permitted caps >>> >>> instead of effective caps? >>> >> >>> >> It should create the socket after changing perms. >>> > >>> > I agree that it should, but it doesn't, and if these patches get >>> > backported, things will break. OTOH, if the patches don't get >>> > backported, things may still break, and we have a possibly rather >>> > severe unfixed vulnerability. >>> >>> I think the kernel change is justified as the privilege allowance >>> that happened before was very much unintentional and as you've >>> shown us countless times a very real problem that we must fix. >> >> One of the problems here is that Quagga may generate millions of >> netlink messages to change routes in response to link flap. >> Raising/lowering the permissions around each request would have >> a significant performance impact. > > What we want to matter are the permissions the process had when the > socket was created/bound, after that you can drop privs and send > netlink requests as you like. I would agree if this were a brand new interface. Unfortunately, there might be programs that assume it's okay to do things like creating a non-O_CLOEXEC netlink socket, dropping privileges, and execing. --Andy ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH 0/5]: Preventing abuse when passing file descriptors 2014-04-23 21:24 ` [PATCH 0/5]: " Eric W. Biederman ` (4 preceding siblings ...) 2014-04-23 21:29 ` [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of " Eric W. Biederman @ 2014-04-24 17:45 ` David Miller 5 siblings, 0 replies; 46+ messages in thread From: David Miller @ 2014-04-24 17:45 UTC (permalink / raw) To: ebiederm; +Cc: vgoyal, ssorce, security, luto, netdev, serge From: ebiederm@xmission.com (Eric W. Biederman) Date: Wed, 23 Apr 2014 14:24:47 -0700 > Andy Lutomirski when looking at the networking stack noticed that it is > possible to trick privilged processes into calling write on a netlink > socket and send netlink messages they did not intend. > > In particular from time to time there are suid applications that will > write to stdout or stderr without checking exactly what kind of file > descriptors those are and can be tricked into acting as a limited form > of suid cat. In other conversations the magic string CVE-2014-0181 has > been used to talk about this issue. > > This patchset cleans things up a bit, adds some clean abstractions that > when used prevent this kind of problem and then finally changes all of > the handlers of netlink messages that I could find that call capable to > use netlink_ns_capable or an appropriate wrapper. > > The abstraction netlink_ns_capable verifies that the original creator of > the netlink socket a message is sent from had the necessary capabilities > as well as verifying that the current sender of a netlink packet has the > necessary capabilities. > > The idea is to prevent file descriptor passing of any form from > resulting in a file descriptor that can do more than it can for the > creator of the file descriptor. Series applied, thanks Eric. ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 0/5] Backport to 3.10.y for fix CVE-2014-0181 @ 2014-06-13 1:31 Xiangyu Lu 2014-06-13 1:31 ` [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages Xiangyu Lu 0 siblings, 1 reply; 46+ messages in thread From: Xiangyu Lu @ 2014-06-13 1:31 UTC (permalink / raw) To: gregkh; +Cc: stable, netdev, Xiangyu Lu Hi Greg, This backport fix CVE-2014-0181 which would still be vulnerable in the latest 3.10.y, please add it. Eric W. Biederman (5): netlink: Rename netlink_capable netlink_allowed net: Move the permission check in sock_diag_put_filterinfo to packet_diag_dump net: Add variants of capable for use on on sockets net: Add variants of capable for use on netlink messages net: Use netlink_ns_capable to verify the permisions of netlink messages crypto/crypto_user.c | 2 +- drivers/connector/cn_proc.c | 2 +- drivers/scsi/scsi_netlink.c | 2 +- include/linux/netlink.h | 7 ++++ include/linux/sock_diag.h | 2 +- include/net/sock.h | 5 +++ kernel/audit.c | 4 +-- net/can/gw.c | 4 +-- net/core/rtnetlink.c | 20 ++++++----- net/core/sock.c | 49 +++++++++++++++++++++++++++ net/core/sock_diag.c | 4 +-- net/dcb/dcbnl.c | 2 +- net/decnet/dn_dev.c | 4 +-- net/decnet/dn_fib.c | 4 +-- net/decnet/netfilter/dn_rtmsg.c | 2 +- net/netfilter/nfnetlink.c | 2 +- net/netlink/af_netlink.c | 75 ++++++++++++++++++++++++++++++++++++++--- net/netlink/genetlink.c | 2 +- net/packet/diag.c | 7 +++- net/phonet/pn_netlink.c | 8 ++--- net/sched/act_api.c | 2 +- net/sched/cls_api.c | 2 +- net/sched/sch_api.c | 6 ++-- net/tipc/netlink.c | 2 +- net/xfrm/xfrm_user.c | 2 +- 25 files changed, 177 insertions(+), 44 deletions(-) -- 1.8.3.4 ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages 2014-06-13 1:31 [PATCH 0/5] Backport to 3.10.y for fix CVE-2014-0181 Xiangyu Lu @ 2014-06-13 1:31 ` Xiangyu Lu 0 siblings, 0 replies; 46+ messages in thread From: Xiangyu Lu @ 2014-06-13 1:31 UTC (permalink / raw) To: gregkh; +Cc: stable, netdev, Eric W. Biederman, David S. Miller, Xiangyu Lu From: "Eric W. Biederman" <ebiederm@xmission.com> commit 90f62cf30a78721641e08737bda787552428061e upstream. It is possible by passing a netlink socket to a more privileged executable and then to fool that executable into writing to the socket data that happens to be valid netlink message to do something that privileged executable did not intend to do. To keep this from happening replace bare capable and ns_capable calls with netlink_capable, netlink_net_calls and netlink_ns_capable calls. Which act the same as the previous calls except they verify that the opener of the socket had the desired permissions as well. Reported-by: Andy Lutomirski <luto@amacapital.net> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> Signed-off-by: David S. Miller <davem@davemloft.net> [xl: Backported to 3.10: - Adjust context - ns_capable() is used by nfnetlink_rcv_msg() but not nfnetlink_rcv()] Signed-off-by: Xiangyu Lu <luxiangyu@huawei.com> --- crypto/crypto_user.c | 2 +- drivers/connector/cn_proc.c | 2 +- drivers/scsi/scsi_netlink.c | 2 +- kernel/audit.c | 4 ++-- net/can/gw.c | 4 ++-- net/core/rtnetlink.c | 20 +++++++++++--------- net/dcb/dcbnl.c | 2 +- net/decnet/dn_dev.c | 4 ++-- net/decnet/dn_fib.c | 4 ++-- net/decnet/netfilter/dn_rtmsg.c | 2 +- net/netfilter/nfnetlink.c | 2 +- net/netlink/genetlink.c | 2 +- net/packet/diag.c | 2 +- net/phonet/pn_netlink.c | 8 ++++---- net/sched/act_api.c | 2 +- net/sched/cls_api.c | 2 +- net/sched/sch_api.c | 6 +++--- net/tipc/netlink.c | 2 +- net/xfrm/xfrm_user.c | 2 +- 19 files changed, 38 insertions(+), 36 deletions(-) diff --git a/crypto/crypto_user.c b/crypto/crypto_user.c index 1512e41..43665d0 100644 --- a/crypto/crypto_user.c +++ b/crypto/crypto_user.c @@ -466,7 +466,7 @@ static int crypto_user_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) type -= CRYPTO_MSG_BASE; link = &crypto_dispatch[type]; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if ((type == (CRYPTO_MSG_GETALG - CRYPTO_MSG_BASE) && diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c index 18c5b9b..3165811 100644 --- a/drivers/connector/cn_proc.c +++ b/drivers/connector/cn_proc.c @@ -369,7 +369,7 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg, return; /* Can only change if privileged. */ - if (!capable(CAP_NET_ADMIN)) { + if (!__netlink_ns_capable(nsp, &init_user_ns, CAP_NET_ADMIN)) { err = EPERM; goto out; } diff --git a/drivers/scsi/scsi_netlink.c b/drivers/scsi/scsi_netlink.c index fe30ea9..109802f 100644 --- a/drivers/scsi/scsi_netlink.c +++ b/drivers/scsi/scsi_netlink.c @@ -77,7 +77,7 @@ scsi_nl_rcv_msg(struct sk_buff *skb) goto next_msg; } - if (!capable(CAP_SYS_ADMIN)) { + if (!netlink_capable(skb, CAP_SYS_ADMIN)) { err = -EPERM; goto next_msg; } diff --git a/kernel/audit.c b/kernel/audit.c index 6def25f..a6c6327 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -593,13 +593,13 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type) case AUDIT_TTY_SET: case AUDIT_TRIM: case AUDIT_MAKE_EQUIV: - if (!capable(CAP_AUDIT_CONTROL)) + if (!netlink_capable(skb, CAP_AUDIT_CONTROL)) 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 (!capable(CAP_AUDIT_WRITE)) + if (!netlink_capable(skb, CAP_AUDIT_WRITE)) err = -EPERM; break; default: /* bad msg */ diff --git a/net/can/gw.c b/net/can/gw.c index 3ee690e..de25455 100644 --- a/net/can/gw.c +++ b/net/can/gw.c @@ -784,7 +784,7 @@ static int cgw_create_job(struct sk_buff *skb, struct nlmsghdr *nlh) struct cgw_job *gwj; int err = 0; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (nlmsg_len(nlh) < sizeof(*r)) @@ -876,7 +876,7 @@ static int cgw_remove_job(struct sk_buff *skb, struct nlmsghdr *nlh) struct can_can_gw ccgw; int err = 0; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (nlmsg_len(nlh) < sizeof(*r)) diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 87ec574..054ebbf 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -1294,7 +1294,8 @@ static int do_set_master(struct net_device *dev, int ifindex) return 0; } -static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm, +static int do_setlink(const struct sk_buff *skb, + struct net_device *dev, struct ifinfomsg *ifm, struct nlattr **tb, char *ifname, int modified) { const struct net_device_ops *ops = dev->netdev_ops; @@ -1306,7 +1307,7 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm, err = PTR_ERR(net); goto errout; } - if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) { + if (!netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN)) { err = -EPERM; goto errout; } @@ -1560,7 +1561,7 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh) if (err < 0) goto errout; - err = do_setlink(dev, ifm, tb, ifname, 0); + err = do_setlink(skb, dev, ifm, tb, ifname, 0); errout: return err; } @@ -1678,7 +1679,8 @@ err: } EXPORT_SYMBOL(rtnl_create_link); -static int rtnl_group_changelink(struct net *net, int group, +static int rtnl_group_changelink(const struct sk_buff *skb, + struct net *net, int group, struct ifinfomsg *ifm, struct nlattr **tb) { @@ -1687,7 +1689,7 @@ static int rtnl_group_changelink(struct net *net, int group, for_each_netdev(net, dev) { if (dev->group == group) { - err = do_setlink(dev, ifm, tb, NULL, 0); + err = do_setlink(skb, dev, ifm, tb, NULL, 0); if (err < 0) return err; } @@ -1789,12 +1791,12 @@ replay: modified = 1; } - return do_setlink(dev, ifm, tb, ifname, modified); + return do_setlink(skb, dev, ifm, tb, ifname, modified); } if (!(nlh->nlmsg_flags & NLM_F_CREATE)) { if (ifm->ifi_index == 0 && tb[IFLA_GROUP]) - return rtnl_group_changelink(net, + return rtnl_group_changelink(skb, net, nla_get_u32(tb[IFLA_GROUP]), ifm, tb); return -ENODEV; @@ -2179,7 +2181,7 @@ static int rtnl_fdb_del(struct sk_buff *skb, struct nlmsghdr *nlh) int err = -EINVAL; __u8 *addr; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; err = nlmsg_parse(nlh, sizeof(*ndm), tb, NDA_MAX, NULL); @@ -2635,7 +2637,7 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) sz_idx = type>>2; kind = type&3; - if (kind != 2 && !ns_capable(net->user_ns, CAP_NET_ADMIN)) + if (kind != 2 && !netlink_net_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (kind == 2 && nlh->nlmsg_flags&NLM_F_DUMP) { diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c index 40d5829..1074ffb 100644 --- a/net/dcb/dcbnl.c +++ b/net/dcb/dcbnl.c @@ -1670,7 +1670,7 @@ static int dcb_doit(struct sk_buff *skb, struct nlmsghdr *nlh) struct nlmsghdr *reply_nlh = NULL; const struct reply_func *fn; - if ((nlh->nlmsg_type == RTM_SETDCB) && !capable(CAP_NET_ADMIN)) + if ((nlh->nlmsg_type == RTM_SETDCB) && !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; ret = nlmsg_parse(nlh, sizeof(*dcb), tb, DCB_ATTR_MAX, diff --git a/net/decnet/dn_dev.c b/net/decnet/dn_dev.c index 7d91970..b5e5210 100644 --- a/net/decnet/dn_dev.c +++ b/net/decnet/dn_dev.c @@ -573,7 +573,7 @@ static int dn_nl_deladdr(struct sk_buff *skb, struct nlmsghdr *nlh) struct dn_ifaddr __rcu **ifap; int err = -EINVAL; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (!net_eq(net, &init_net)) @@ -617,7 +617,7 @@ static int dn_nl_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh) struct dn_ifaddr *ifa; int err; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (!net_eq(net, &init_net)) diff --git a/net/decnet/dn_fib.c b/net/decnet/dn_fib.c index 57dc159..d332aef 100644 --- a/net/decnet/dn_fib.c +++ b/net/decnet/dn_fib.c @@ -505,7 +505,7 @@ static int dn_fib_rtm_delroute(struct sk_buff *skb, struct nlmsghdr *nlh) struct nlattr *attrs[RTA_MAX+1]; int err; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (!net_eq(net, &init_net)) @@ -530,7 +530,7 @@ static int dn_fib_rtm_newroute(struct sk_buff *skb, struct nlmsghdr *nlh) struct nlattr *attrs[RTA_MAX+1]; int err; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (!net_eq(net, &init_net)) diff --git a/net/decnet/netfilter/dn_rtmsg.c b/net/decnet/netfilter/dn_rtmsg.c index 2a7efe3..f3dc69a 100644 --- a/net/decnet/netfilter/dn_rtmsg.c +++ b/net/decnet/netfilter/dn_rtmsg.c @@ -107,7 +107,7 @@ static inline void dnrmg_receive_user_skb(struct sk_buff *skb) if (nlh->nlmsg_len < sizeof(*nlh) || skb->len < nlh->nlmsg_len) return; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) RCV_SKB_FAIL(-EPERM); /* Eventually we might send routing messages too */ diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c index 572d87dc..0a03662 100644 --- a/net/netfilter/nfnetlink.c +++ b/net/netfilter/nfnetlink.c @@ -147,7 +147,7 @@ static int nfnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh) const struct nfnetlink_subsystem *ss; int type, err; - if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) + if (!netlink_net_capable(skb, CAP_NET_ADMIN)) return -EPERM; /* All the messages must at least contain nfgenmsg */ diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c index 393f17e..ade434b 100644 --- a/net/netlink/genetlink.c +++ b/net/netlink/genetlink.c @@ -592,7 +592,7 @@ static int genl_family_rcv_msg(struct genl_family *family, return -EOPNOTSUPP; if ((ops->flags & GENL_ADMIN_PERM) && - !capable(CAP_NET_ADMIN)) + !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; if (nlh->nlmsg_flags & NLM_F_DUMP) { diff --git a/net/packet/diag.c b/net/packet/diag.c index cbf7391..53b0300 100644 --- a/net/packet/diag.c +++ b/net/packet/diag.c @@ -193,7 +193,7 @@ static int packet_diag_dump(struct sk_buff *skb, struct netlink_callback *cb) net = sock_net(skb->sk); req = nlmsg_data(cb->nlh); - may_report_filterinfo = ns_capable(net->user_ns, CAP_NET_ADMIN); + may_report_filterinfo = netlink_net_capable(cb->skb, CAP_NET_ADMIN); mutex_lock(&net->packet.sklist_lock); sk_for_each(sk, &net->packet.sklist) { diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c index dc15f43..b64151a 100644 --- a/net/phonet/pn_netlink.c +++ b/net/phonet/pn_netlink.c @@ -70,10 +70,10 @@ static int addr_doit(struct sk_buff *skb, struct nlmsghdr *nlh) int err; u8 pnaddr; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; - if (!capable(CAP_SYS_ADMIN)) + if (!netlink_capable(skb, CAP_SYS_ADMIN)) return -EPERM; ASSERT_RTNL(); @@ -233,10 +233,10 @@ static int route_doit(struct sk_buff *skb, struct nlmsghdr *nlh) int err; u8 dst; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; - if (!capable(CAP_SYS_ADMIN)) + if (!netlink_capable(skb, CAP_SYS_ADMIN)) return -EPERM; ASSERT_RTNL(); diff --git a/net/sched/act_api.c b/net/sched/act_api.c index fd70728..15d46b9 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -989,7 +989,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n) u32 portid = skb ? NETLINK_CB(skb).portid : 0; int ret = 0, ovr = 0; - if ((n->nlmsg_type != RTM_GETACTION) && !capable(CAP_NET_ADMIN)) + if ((n->nlmsg_type != RTM_GETACTION) && !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL); diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index 8e118af..2ea40d1 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -138,7 +138,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n) int err; int tp_created = 0; - if ((n->nlmsg_type != RTM_GETTFILTER) && !capable(CAP_NET_ADMIN)) + if ((n->nlmsg_type != RTM_GETTFILTER) && !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; replay: diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index 51b968d..2d2f079 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -1024,7 +1024,7 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n) struct Qdisc *p = NULL; int err; - if ((n->nlmsg_type != RTM_GETQDISC) && !capable(CAP_NET_ADMIN)) + if ((n->nlmsg_type != RTM_GETQDISC) && !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; err = nlmsg_parse(n, sizeof(*tcm), tca, TCA_MAX, NULL); @@ -1091,7 +1091,7 @@ static int tc_modify_qdisc(struct sk_buff *skb, struct nlmsghdr *n) struct Qdisc *q, *p; int err; - if (!capable(CAP_NET_ADMIN)) + if (!netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; replay: @@ -1431,7 +1431,7 @@ static int tc_ctl_tclass(struct sk_buff *skb, struct nlmsghdr *n) u32 qid; int err; - if ((n->nlmsg_type != RTM_GETTCLASS) && !capable(CAP_NET_ADMIN)) + if ((n->nlmsg_type != RTM_GETTCLASS) && !netlink_capable(skb, CAP_NET_ADMIN)) return -EPERM; err = nlmsg_parse(n, sizeof(*tcm), tca, TCA_MAX, NULL); diff --git a/net/tipc/netlink.c b/net/tipc/netlink.c index 8bcd498..1e6081f 100644 --- a/net/tipc/netlink.c +++ b/net/tipc/netlink.c @@ -47,7 +47,7 @@ static int handle_cmd(struct sk_buff *skb, struct genl_info *info) int hdr_space = nlmsg_total_size(GENL_HDRLEN + TIPC_GENL_HDRLEN); u16 cmd; - if ((req_userhdr->cmd & 0xC000) && (!capable(CAP_NET_ADMIN))) + if ((req_userhdr->cmd & 0xC000) && (!netlink_capable(skb, CAP_NET_ADMIN))) cmd = TIPC_CMD_NOT_NET_ADMIN; else cmd = req_userhdr->cmd; diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index 3f565e4..7a70a5a 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -2362,7 +2362,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 (!ns_capable(net->user_ns, CAP_NET_ADMIN)) + if (!netlink_net_capable(skb, CAP_NET_ADMIN)) return -EPERM; if ((type == (XFRM_MSG_GETSA - XFRM_MSG_BASE) || -- 1.8.3.4 ^ permalink raw reply related [flat|nested] 46+ messages in thread
end of thread, other threads:[~2014-06-13 1:44 UTC | newest] Thread overview: 46+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CALCETrUaYhh6Dkzn0TMEUz-GEO9-6ObByk5d_xRViSMBbp5Pkg@mail.gmail.com> [not found] ` <cover.1397840611.git.luto@amacapital.net> [not found] ` <6daf425e2023266d52d181e4d2ee18747d4f1fa8.1397840611.git.luto@amacapital.net> [not found] ` <87tx9nuxf6.fsf@x220.int.ebiederm.org> [not found] ` <CALCETrUqNVRBse4rUeUKfgYt0d+9x1JrEHGcZ_DnWyq7W6Yyzw@mail.gmail.com> [not found] ` <87r44qtabz.fsf@x220.int.ebiederm.org> [not found] ` <CALCETrWzUQ7QjykT85ExDfX-+9eDD-D-dcxofUMPvLK=ia9arg@mail.gmail.com> [not found] ` <87r44qrt8v.fsf_-_@x220.int.ebiederm.org> 2014-04-22 21:13 ` [PATCH 0/6]: Preventing abuse when passing file descriptors Eric W. Biederman 2014-04-22 21:14 ` [PATCH 1/6] netlink: Rename netlink_capable netlink_allowed Eric W. Biederman 2014-04-22 21:15 ` [PATCH 2/6] net: Move the permission check in sock_diag_put_filterinfo to packet_diag_dump Eric W. Biederman 2014-04-22 21:15 ` [PATCH 3/6] net: Fix ns_capable check in packet_diag_dump Eric W. Biederman 2014-04-22 21:16 ` [PATCH 4/6] net: Add variants of capable for use on on sockets Eric W. Biederman 2014-04-22 21:16 ` [PATCH 5/6] net: Add variants of capable for use on netlink messages Eric W. Biederman 2014-04-22 21:17 ` [PATCH 6/6] net: Use netlink_ns_capable to verify the permisions of " Eric W. Biederman 2014-04-23 19:32 ` [PATCH 0/6]: Preventing abuse when passing file descriptors David Miller 2014-04-23 21:24 ` [PATCH 0/5]: " Eric W. Biederman 2014-04-23 21:25 ` [PATCH 1/5] netlink: Rename netlink_capable netlink_allowed Eric W. Biederman 2014-04-23 21:26 ` [PATCH 2/5] net: Move the permission check in sock_diag_put_filterinfo to packet_diag_dump Eric W. Biederman 2014-04-23 21:26 ` [PATCH 3/5] net: Add variants of capable for use on on sockets Eric W. Biederman 2014-04-23 21:28 ` [PATCH 4/5] net: Add variants of capable for use on netlink messages Eric W. Biederman 2014-04-23 21:29 ` [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of " Eric W. Biederman 2014-05-07 22:18 ` Jorge Boncompte [DTI2] 2014-05-07 22:26 ` Andy Lutomirski 2014-05-07 22:52 ` David Miller 2014-05-07 23:01 ` Andy Lutomirski 2014-05-07 23:34 ` Linus Torvalds 2014-05-07 23:45 ` Andy Lutomirski 2014-05-22 15:05 ` Jiri Benc 2014-05-23 23:25 ` Eric W. Biederman 2014-05-23 23:51 ` Linus Torvalds 2014-05-24 22:34 ` David Miller 2014-05-25 5:38 ` [RFC][PATCH] netlink: Only check file credentials for implicit destinations Eric W. Biederman 2014-05-25 16:50 ` Andy Lutomirski 2014-05-25 23:44 ` Eric W. Biederman 2014-05-26 0:32 ` Linus Torvalds 2014-05-26 5:36 ` [RFC][PATCH 2/1] netlink: Use the credential at the time the destination address was set Eric W. Biederman 2014-05-26 17:19 ` Andy Lutomirski 2014-05-27 4:24 ` Eric W. Biederman 2014-05-26 13:39 ` [RFC][PATCH] netlink: Only check file credentials for implicit destinations Willy Tarreau 2014-05-26 8:38 ` Michael Kerrisk (man-pages) 2014-05-25 5:45 ` [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages Eric W. Biederman 2014-05-25 16:27 ` Andy Lutomirski 2014-05-08 21:29 ` Stephen Hemminger 2014-05-08 21:32 ` Andy Lutomirski [not found] ` <CA+55aFzOHZcw2o6Cq6rSddSBDZvhgzYToBruak9SLCHxx-fA3Q@mail.gmail.com> 2014-05-08 21:49 ` Andy Lutomirski 2014-05-08 22:07 ` Stephen Hemminger 2014-05-08 21:54 ` David Miller 2014-05-07 23:45 ` David Miller 2014-05-08 21:21 ` Stephen Hemminger 2014-05-08 21:52 ` David Miller 2014-05-08 21:54 ` Andy Lutomirski 2014-04-24 17:45 ` [PATCH 0/5]: Preventing abuse when passing file descriptors David Miller 2014-06-13 1:31 [PATCH 0/5] Backport to 3.10.y for fix CVE-2014-0181 Xiangyu Lu 2014-06-13 1:31 ` [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages Xiangyu Lu
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).