Patchwork [net-next,v2,3/5] sock_diag: allow to dump bpf filters

login
register
mail settings
Submitter Nicolas Dichtel
Date April 24, 2013, 3:52 p.m.
Message ID <1366818756-4234-4-git-send-email-nicolas.dichtel@6wind.com>
Download mbox | patch
Permalink /patch/239253/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Nicolas Dichtel - April 24, 2013, 3:52 p.m.
This patch allows to dump BPF filters attached to a socket with
SO_ATTACH_FILTER. In other words, users allowing to open netlink sockets can
see filters set on a socket (when the diag module of the socket family is
loaded).

For now, only AF_PACKET sockets use this feature.

Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
 include/linux/sock_diag.h        |  2 ++
 include/uapi/linux/packet_diag.h |  2 ++
 net/core/sock_diag.c             | 27 +++++++++++++++++++++++++++
 net/packet/diag.c                |  4 ++++
 4 files changed, 35 insertions(+)
Eric Dumazet - April 24, 2013, 4:22 p.m.
On Wed, 2013-04-24 at 17:52 +0200, Nicolas Dichtel wrote:
> This patch allows to dump BPF filters attached to a socket with
> SO_ATTACH_FILTER. In other words, users allowing to open netlink sockets can
> see filters set on a socket (when the diag module of the socket family is
> loaded).

To my knowledge, opening netlink sockets is not restricted.

I do not want user lambda being able to see my BPF filters.

I am root, and was assuming user lambda could not spy on me.

$ cat /proc/net/packet 
sk       RefCnt Type Proto  Iface R Rmem   User   Inode
0000000000000000 3      10   0003   3     1 0      0      1089989
0000000000000000 3      10   0003   2     1 0      0      1050535
0000000000000000 3      2    888e   3     1 0      0      1041970


With this information, it seems safe enough, but the whole BPF could
give interesting ideas to user lambda.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - April 25, 2013, 5:16 a.m.
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 24 Apr 2013 09:22:45 -0700

> On Wed, 2013-04-24 at 17:52 +0200, Nicolas Dichtel wrote:
>> This patch allows to dump BPF filters attached to a socket with
>> SO_ATTACH_FILTER. In other words, users allowing to open netlink sockets can
>> see filters set on a socket (when the diag module of the socket family is
>> loaded).
> 
> To my knowledge, opening netlink sockets is not restricted.
> 
> I do not want user lambda being able to see my BPF filters.

I agree, this change is not reasonable.  And therefore the notifiers
added in the subsequent patches of this series are not appropriate
either.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Dichtel - April 25, 2013, 8:37 a.m.
Le 24/04/2013 18:22, Eric Dumazet a écrit :
> On Wed, 2013-04-24 at 17:52 +0200, Nicolas Dichtel wrote:
>> This patch allows to dump BPF filters attached to a socket with
>> SO_ATTACH_FILTER. In other words, users allowing to open netlink sockets can
>> see filters set on a socket (when the diag module of the socket family is
>> loaded).
>
> To my knowledge, opening netlink sockets is not restricted.
>
> I do not want user lambda being able to see my BPF filters.
>
> I am root, and was assuming user lambda could not spy on me.
>
> $ cat /proc/net/packet
> sk       RefCnt Type Proto  Iface R Rmem   User   Inode
> 0000000000000000 3      10   0003   3     1 0      0      1089989
> 0000000000000000 3      10   0003   2     1 0      0      1050535
> 0000000000000000 3      2    888e   3     1 0      0      1041970
>
>
> With this information, it seems safe enough, but the whole BPF could
> give interesting ideas to user lambda.
I agree. But then you just have to avoid loading the module packet_diag. This 
module already give some clue to users, because it sends the socket pointer 
through netlink.
Maybe I'm wrong, but I was thinking that this module is used for debug purpose.
If the module is not loaded, my patch has no effect on the system.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - April 25, 2013, 9 a.m.
From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Thu, 25 Apr 2013 10:37:00 +0200

> Maybe I'm wrong, but I was thinking that this module is used for debug
> purpose.

No, it's a normal tool.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Dichtel - April 25, 2013, 1:21 p.m.
The goal of this patchset is to be able to monitor packet sockets.
The two first patches add new attributes for packet_diag subsystem, so that all
information exported via the /proc/net/packet are also exported via netlink.
The third patch avoid to disclose socket pointer to all user. Via the /proc,
kptr_restrict is used.
The last patch allows allowed users to get details about filter attached to a
packet socket.

As usual, the patch against iproute2 will be sent once the patches are included
and net-next merged. I can send it on demand.

v2: add sock_diag_notify_del() to avoid confusion of the meaning of the second
    arg of __sock_diag_notify()
    enhance commitlog of patch 3/5

v3: drop previous 4/5 and 5/5 patches
    add patch 3/4 (sock_diag: do not disclose sock ptr to all users)
    disclose filters only to allowed users

 include/linux/sock_diag.h        |  5 ++++-
 include/uapi/linux/packet_diag.h |  5 +++++
 net/core/sock_diag.c             | 45 +++++++++++++++++++++++++++++++++++++---
 net/ipv4/inet_diag.c             | 13 +++++++-----
 net/netlink/diag.c               |  5 ++++-
 net/packet/diag.c                | 29 ++++++++++++++++++++------
 net/unix/diag.c                  | 14 ++++++++-----
 7 files changed, 95 insertions(+), 21 deletions(-)

Comments are welcome.

Regards,
Nicolas
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - April 25, 2013, 1:51 p.m.
On Thu, 2013-04-25 at 10:37 +0200, Nicolas Dichtel wrote:
> I agree. But then you just have to avoid loading the module packet_diag. This 
> module already give some clue to users, because it sends the socket pointer 
> through netlink.

You probably missed a lot of discussion about this socket pointer being
leaked. Supposedly security guys wanted to remove this at one point.

Thats why we should make sure to add yet another security issue.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/sock_diag.h b/include/linux/sock_diag.h
index e8d702e..3957c14 100644
--- a/include/linux/sock_diag.h
+++ b/include/linux/sock_diag.h
@@ -22,5 +22,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, struct sk_buff *skb,
+			     int attrtype);
 
 #endif
diff --git a/include/uapi/linux/packet_diag.h b/include/uapi/linux/packet_diag.h
index c0802c1..b2cc0cd 100644
--- a/include/uapi/linux/packet_diag.h
+++ b/include/uapi/linux/packet_diag.h
@@ -17,6 +17,7 @@  struct packet_diag_req {
 #define PACKET_SHOW_RING_CFG	0x00000004 /* Rings configuration parameters */
 #define PACKET_SHOW_FANOUT	0x00000008
 #define PACKET_SHOW_MEMINFO	0x00000010
+#define PACKET_SHOW_FILTER	0x00000020
 
 struct packet_diag_msg {
 	__u8	pdiag_family;
@@ -35,6 +36,7 @@  enum {
 	PACKET_DIAG_FANOUT,
 	PACKET_DIAG_UID,
 	PACKET_DIAG_MEMINFO,
+	PACKET_DIAG_FILTER,
 
 	__PACKET_DIAG_MAX,
 };
diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c
index a29e90c..51e75f4 100644
--- a/net/core/sock_diag.c
+++ b/net/core/sock_diag.c
@@ -49,6 +49,33 @@  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, struct sk_buff *skb, int attrtype)
+{
+	struct nlattr *attr;
+	struct sk_filter *filter;
+	unsigned int len;
+	int err = 0;
+
+	rcu_read_lock();
+
+	filter = rcu_dereference(sk->sk_filter);
+	len = filter ? filter->len * sizeof(struct sock_filter) : 0;
+
+	attr = nla_reserve(skb, attrtype, len);
+	if (attr == NULL) {
+		err = -EMSGSIZE;
+		goto out;
+	}
+
+	if (filter)
+		memcpy(nla_data(attr), filter->insns, len);
+
+out:
+	rcu_read_unlock();
+	return err;
+}
+EXPORT_SYMBOL(sock_diag_put_filterinfo);
+
 void sock_diag_register_inet_compat(int (*fn)(struct sk_buff *skb, struct nlmsghdr *nlh))
 {
 	mutex_lock(&sock_diag_table_mutex);
diff --git a/net/packet/diag.c b/net/packet/diag.c
index 822fe9b..ec8b6e8 100644
--- a/net/packet/diag.c
+++ b/net/packet/diag.c
@@ -170,6 +170,10 @@  static int sk_diag_fill(struct sock *sk, struct sk_buff *skb,
 	    sock_diag_put_meminfo(sk, skb, PACKET_DIAG_MEMINFO))
 		goto out_nlmsg_trim;
 
+	if ((req->pdiag_show & PACKET_SHOW_FILTER) &&
+	    sock_diag_put_filterinfo(sk, skb, PACKET_DIAG_FILTER))
+		goto out_nlmsg_trim;
+
 	return nlmsg_end(skb, nlh);
 
 out_nlmsg_trim: