[PATCH 0/6]: Preventing abuse when passing file descriptors

[PATCH 0/6]: Preventing abuse when passing file descriptors

[Eric W. Biederman]

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

[PATCH 1/6] netlink: Rename netlink_capable netlink_allowed

[Eric W. Biederman]

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

[PATCH 2/6] net: Move the permission check in sock_diag_put_filterinfo to packet_diag_dump

[Eric W. Biederman]

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

[PATCH 3/6] net: Fix ns_capable check in packet_diag_dump

[Eric W. Biederman]

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

[PATCH 4/6] net: Add variants of capable for use on on sockets

[Eric W. Biederman]

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

[PATCH 5/6] net: Add variants of capable for use on netlink messages

[Eric W. Biederman]

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

[PATCH 6/6] net: Use netlink_ns_capable to verify the permisions of netlink messages

[Eric W. Biederman]

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

Re: [PATCH 0/6]: Preventing abuse when passing file descriptors

[David Miller]

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.

[PATCH 0/5]: Preventing abuse when passing file descriptors

[Eric W. Biederman]

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

[PATCH 1/5] netlink: Rename netlink_capable netlink_allowed

[Eric W. Biederman]

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

[PATCH 2/5] net: Move the permission check in sock_diag_put_filterinfo to packet_diag_dump

[Eric W. Biederman]

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

[PATCH 3/5] net: Add variants of capable for use on on sockets

[Eric W. Biederman]

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

[PATCH 4/5] net: Add variants of capable for use on netlink messages

[Eric W. Biederman]

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

[PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[Eric W. Biederman]

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

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[Jorge Boncompte [DTI2]]

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.
==============================================================

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[Andy Lutomirski]

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

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[David Miller]

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.

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[Andy Lutomirski]

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

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[Linus Torvalds]

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

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[Andy Lutomirski]

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

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[Jiri Benc]

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

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[Eric W. Biederman]

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

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[Linus Torvalds]

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

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[David Miller]

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.

[RFC][PATCH] netlink: Only check file credentials for implicit destinations

[Eric W. Biederman]

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

Re: [RFC][PATCH] netlink: Only check file credentials for implicit destinations

[Andy Lutomirski]

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

Re: [RFC][PATCH] netlink: Only check file credentials for implicit destinations

[Eric W. Biederman]

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

Re: [RFC][PATCH] netlink: Only check file credentials for implicit destinations

[Linus Torvalds]

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

[RFC][PATCH 2/1] netlink: Use the credential at the time the destination address was set.

[Eric W. Biederman]

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

Re: [RFC][PATCH 2/1] netlink: Use the credential at the time the destination address was set.

[Andy Lutomirski]

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

Re: [RFC][PATCH 2/1] netlink: Use the credential at the time the destination address was set.

[Eric W. Biederman]

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

Re: [RFC][PATCH] netlink: Only check file credentials for implicit destinations

[Willy Tarreau]

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

Re: [RFC][PATCH] netlink: Only check file credentials for implicit destinations

[Michael Kerrisk (man-pages)]

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/

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[Eric W. Biederman]

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

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[Andy Lutomirski]

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

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[Stephen Hemminger]

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.

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[Andy Lutomirski]

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

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[Andy Lutomirski]

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

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[Stephen Hemminger]

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.

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[David Miller]

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.

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[David Miller]

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.

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[Stephen Hemminger]

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.

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[David Miller]

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.

Re: [PATCH 5/5] net: Use netlink_ns_capable to verify the permisions of netlink messages

[Andy Lutomirski]

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

Re: [PATCH 0/5]: Preventing abuse when passing file descriptors

[David Miller]

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.