[RFC][PATCH 0/3] netlink check sender

[RFC][PATCH 0/3] netlink check sender

[Chris Wright]

The following patches are for comment.  They introduce a new callback
to enable netlink messages to be validated in the sender's context,
and then convert a couple kernel netlink receivers to use this callback.
This eliminates the need to copy the sender's effective capabilities into
the netlink control buffer.  It also allows the audit system to manage
the loginuid in the kernel without adding more fields to netlink_skb_parms
or requiring special case netlink code.  I think this would obsolete the
security_netlink_recv hook, and simplify the security_netlink_send hook.

Currently I've only hooked the unicast messages, because I didn't think
any of the kernel netlink input functions would be processing broadcast
messages (perhaps I missed something).

I didn't move the logic that simply ignores messages (e.g. type <
RTM_BASE), but I did move the logic that looks for invalid messages
(e.g. type > RTM_MAX) to the check_sender callback.

Thoughts?

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

[RFC][PATCH 1/3] netlink check sender

[Chris Wright]

* Add check_sender callback to struct netlink_opt.
* Add netlink_kernel_create_check() to register that callback, and turn
  netlink_kernel_create() into a simple wrapper around that.
* Invoke callback when set to verify sender in sender's context.

===== net/netlink/af_netlink.c 1.69 vs edited =====
--- 1.69/net/netlink/af_netlink.c	2005-01-21 12:25:32 -08:00
+++ edited/net/netlink/af_netlink.c	2005-02-11 18:05:59 -08:00
@@ -71,6 +71,7 @@ struct netlink_opt
 	struct netlink_callback	*cb;
 	spinlock_t		cb_lock;
 	void			(*data_ready)(struct sock *sk, int bytes);
+	int			(*check_sender)(struct sk_buff *skb);
 };
 
 #define nlk_sk(__sk) ((struct netlink_opt *)(__sk)->sk_protinfo)
@@ -636,9 +637,17 @@ int netlink_attachskb(struct sock *sk, s
 int netlink_sendskb(struct sock *sk, struct sk_buff *skb, int protocol)
 {
 	struct netlink_opt *nlk;
-	int len = skb->len;
-
+	int err, len = skb->len;
+	
 	nlk = nlk_sk(sk);
+
+	printk("%s: %s(%d) send_check %p\n", __FUNCTION__, current->comm, current->pid, nlk->check_sender);
+	if (nlk->check_sender)
+		if ((err = nlk->check_sender(skb))) {
+			netlink_detachskb(sk, skb);
+			return err;
+		}
+
 #ifdef NL_EMULATE_DEV
 	if (nlk->handler) {
 		skb_orphan(skb);
@@ -1033,7 +1042,8 @@ static void netlink_data_ready(struct so
  */
 
 struct sock *
-netlink_kernel_create(int unit, void (*input)(struct sock *sk, int len))
+netlink_kernel_create_check(int unit, void (*input)(struct sock *sk, int len),
+			    int (*check)(struct sk_buff *skb))
 {
 	struct socket *sock;
 	struct sock *sk;
@@ -1053,8 +1063,10 @@ netlink_kernel_create(int unit, void (*i
 	}
 	sk = sock->sk;
 	sk->sk_data_ready = netlink_data_ready;
-	if (input)
+	if (input) {
 		nlk_sk(sk)->data_ready = input;
+		nlk_sk(sk)->check_sender = check;
+	}
 
 	if (netlink_insert(sk, 0)) {
 		sock_release(sock);
@@ -1459,7 +1471,7 @@ MODULE_ALIAS_NETPROTO(PF_NETLINK);
 EXPORT_SYMBOL(netlink_ack);
 EXPORT_SYMBOL(netlink_broadcast);
 EXPORT_SYMBOL(netlink_dump_start);
-EXPORT_SYMBOL(netlink_kernel_create);
+EXPORT_SYMBOL(netlink_kernel_create_check);
 EXPORT_SYMBOL(netlink_register_notifier);
 EXPORT_SYMBOL(netlink_set_err);
 EXPORT_SYMBOL(netlink_set_nonroot);
===== include/linux/netlink.h 1.23 vs edited =====
--- 1.23/include/linux/netlink.h	2005-02-06 21:59:39 -08:00
+++ edited/include/linux/netlink.h	2005-02-11 14:56:23 -08:00
@@ -116,7 +116,11 @@ struct netlink_skb_parms
 #define NETLINK_CREDS(skb)	(&NETLINK_CB((skb)).creds)
 
 
-extern struct sock *netlink_kernel_create(int unit, void (*input)(struct sock *sk, int len));
+extern struct sock *netlink_kernel_create_check(int unit, void (*input)(struct sock *sk, int len), int (*check)(struct sk_buff *skb));
+static inline struct sock *netlink_kernel_create(int unit, void (*input)(struct sock *sk, int len)) 
+{
+	return netlink_kernel_create_check(unit, input, NULL);
+}
 extern void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err);
 extern int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 pid, int nonblock);
 extern int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, __u32 pid,

[RFC][PATCH 2/3] netlink check sender, audit

[Chris Wright]

Add audit_check_sender() function for audit netlink messages.  This can also
be used to set the loginuid, although I left that off for the moment.

===== kernel/audit.c 1.9 vs edited =====
--- 1.9/kernel/audit.c	2005-01-30 22:33:47 -08:00
+++ edited/kernel/audit.c	2005-02-11 22:25:33 -08:00
@@ -309,27 +309,36 @@ nlmsg_failure:			/* Used by NLMSG_PUT */
  * Check for appropriate CAP_AUDIT_ capabilities on incoming audit
  * control messages.
  */
-static int audit_netlink_ok(kernel_cap_t eff_cap, u16 msg_type)
+static int audit_check_sender(struct sk_buff *skb)
 {
-	int err = 0;
+	struct nlmsghdr *nlh;
+	u16 msg_type;
+	int err = -EINVAL;
 
+	if (skb->len < NLMSG_LENGTH(0))
+		goto out;
+
+	nlh = (struct nlmsghdr *)skb->data;
+	msg_type = nlh->nlmsg_type;
+
+	err = 0;
 	switch (msg_type) {
 	case AUDIT_GET:
 	case AUDIT_LIST:
 	case AUDIT_SET:
 	case AUDIT_ADD:
 	case AUDIT_DEL:
-		if (!cap_raised(eff_cap, CAP_AUDIT_CONTROL))
+		if (!capable(CAP_AUDIT_CONTROL))
 			err = -EPERM;
 		break;
 	case AUDIT_USER:
-		if (!cap_raised(eff_cap, CAP_AUDIT_WRITE))
+		if (!capable(CAP_AUDIT_WRITE))
 			err = -EPERM;
 		break;
 	default:  /* bad msg */
 		err = -EINVAL;
 	}
-
+out:
 	return err;
 }
 
@@ -338,14 +347,10 @@ static int audit_receive_msg(struct sk_b
 	u32			uid, pid, seq;
 	void			*data;
 	struct audit_status	*status_get, status_set;
-	int			err;
+	int			err = 0;
 	struct audit_buffer	*ab;
 	u16			msg_type = nlh->nlmsg_type;
 
-	err = audit_netlink_ok(NETLINK_CB(skb).eff_cap, msg_type);
-	if (err)
-		return err;
-
 	pid  = NETLINK_CREDS(skb)->pid;
 	uid  = NETLINK_CREDS(skb)->uid;
 	seq  = nlh->nlmsg_seq;
@@ -551,7 +556,7 @@ int __init audit_init(void)
 {
 	printk(KERN_INFO "audit: initializing netlink socket (%s)\n",
 	       audit_default ? "enabled" : "disabled");
-	audit_sock = netlink_kernel_create(NETLINK_AUDIT, audit_receive);
+	audit_sock = netlink_kernel_create_check(NETLINK_AUDIT, audit_receive, audit_check_sender);
 	if (!audit_sock)
 		audit_panic("cannot initialize netlink socket");
 

[RFC][PATCH 3/3] netlink check sender, rtnetlink

[Chris Wright]

Add rtnetlink_check_sender() function to validate rtnetlink messages in
sender context.  Invalid messages (due to content or sender privileges)
will be rejected before queued to socket.

===== net/core/rtnetlink.c 1.33 vs edited =====
--- 1.33/net/core/rtnetlink.c	2005-01-10 13:42:22 -08:00
+++ edited/net/core/rtnetlink.c	2005-02-12 00:13:46 -08:00
@@ -462,8 +462,32 @@ static int rtnetlink_done(struct netlink
 static struct rtattr **rta_buf;
 static int rtattr_max;
 
-/* Process one rtnetlink message. */
+static int rtnetlink_check_sender(struct sk_buff *skb)
+{
+	struct nlmsghdr *nlh;
+	int kind;
+	int type;
+
+	if (skb->len < NLMSG_LENGTH(0))
+		return -EINVAL;
+
+	nlh = (struct nlmsghdr *)skb->data;
+	type = nlh->nlmsg_type;
+
+	/* Unknown message: reply with EINVAL */
+	if (type > RTM_MAX)
+		return -EINVAL;
+
+	type -= RTM_BASE;
+	kind = type&3;
 
+	if (kind != 2 && !capable(CAP_NET_ADMIN))
+		return -EPERM;
+
+	return 0;
+}
+
+/* Process one rtnetlink message. */
 static __inline__ int
 rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, int *errp)
 {
@@ -485,10 +509,6 @@ rtnetlink_rcv_msg(struct sk_buff *skb, s
 	if (type < RTM_BASE)
 		return 0;
 
-	/* Unknown message: reply with EINVAL */
-	if (type > RTM_MAX)
-		goto err_inval;
-
 	type -= RTM_BASE;
 
 	/* All the messages must have at least 1 byte length */
@@ -509,11 +529,6 @@ rtnetlink_rcv_msg(struct sk_buff *skb, s
 	sz_idx = type>>2;
 	kind = type&3;
 
-	if (kind != 2 && security_netlink_recv(skb)) {
-		*errp = -EPERM;
-		return -1;
-	}
-
 	if (kind == 2 && nlh->nlmsg_flags&NLM_F_DUMP) {
 		u32 rlen;
 
@@ -690,7 +705,8 @@ void __init rtnetlink_init(void)
 	if (!rta_buf)
 		panic("rtnetlink_init: cannot allocate rta_buf\n");
 
-	rtnl = netlink_kernel_create(NETLINK_ROUTE, rtnetlink_rcv);
+	rtnl = netlink_kernel_create_check(NETLINK_ROUTE, rtnetlink_rcv,
+					   rtnetlink_check_sender);
 	if (rtnl == NULL)
 		panic("rtnetlink_init: cannot initialize rtnetlink\n");
 	netlink_set_nonroot(NETLINK_ROUTE, NL_NONROOT_RECV);

Re: [RFC][PATCH 2/3] netlink check sender, audit

[Pablo Neira]

Chris Wright wrote:

>Add audit_check_sender() function for audit netlink messages.  This can also
>be used to set the loginuid, although I left that off for the moment.
>
>===== kernel/audit.c 1.9 vs edited =====
>--- 1.9/kernel/audit.c	2005-01-30 22:33:47 -08:00
>+++ edited/kernel/audit.c	2005-02-11 22:25:33 -08:00
>@@ -309,27 +309,36 @@ nlmsg_failure:			/* Used by NLMSG_PUT */
>  * Check for appropriate CAP_AUDIT_ capabilities on incoming audit
>  * control messages.
>  */
>-static int audit_netlink_ok(kernel_cap_t eff_cap, u16 msg_type)
>+static int audit_check_sender(struct sk_buff *skb)
> {
>-	int err = 0;
>+	struct nlmsghdr *nlh;
>+	u16 msg_type;
>+	int err = -EINVAL;
> 
>+	if (skb->len < NLMSG_LENGTH(0))
>+		goto out;
>+
>+	nlh = (struct nlmsghdr *)skb->data;
>+	msg_type = nlh->nlmsg_type;
>  
>

You're introducing some kind of check for malformed packets here as 
well, don't you think that such thing should be done by the receiver ?

I also see another option which is passing as parameter such function 
which check for capabilities/audit stuff to my netlink_process_skb 
function, calling it before process_msg. But in that case, the packet 
sent by a sender that doesn't has the right to was already enqueued. I 
understand that this is exactly what you are trying to avoid.

--
Pablo

Re: [RFC][PATCH 2/3] netlink check sender, audit

[Pablo Neira]

Pablo Neira wrote:

> I also see another option which is passing as parameter such function 
> which check for capabilities/audit stuff to my netlink_process_skb 
> function, calling it before process_msg. But in that case, the packet 
> sent by a sender that doesn't has the right to was already enqueued. I 
> understand that this is exactly what you are trying to avoid.


With your patch, a message from user space process that doesn't have the 
capabilites follows this path:

sys_sendmsg() -> netlink_sendmsg() -> netlink_unicast() -> 
netlink_sendskb() = discarded here.

Currently, it continues, for example in case of rtnetlink:

... -> netlink_sendskb() -> sk_data_ready(sk, len) -> rtnetlink_rcv() -> 
rtnetlink_rcv_skb() -> rtnetlink_rcv_msg() = discarded here.

Nowadays the message is enqueued but it's discarded later. So if I'm not 
missing anything, I don't see the point of adding a new function to 
check for capabilities/audit stuff just a bit before.

--
Pablo

Re: [RFC][PATCH 2/3] netlink check sender, audit

[Stephen Smalley]

On Sat, 2005-02-12 at 16:41, Pablo Neira wrote:
> With your patch, a message from user space process that doesn't have the 
> capabilites follows this path:
> 
> sys_sendmsg() -> netlink_sendmsg() -> netlink_unicast() -> 
> netlink_sendskb() = discarded here.
> 
> Currently, it continues, for example in case of rtnetlink:
> 
> ... -> netlink_sendskb() -> sk_data_ready(sk, len) -> rtnetlink_rcv() -> 
> rtnetlink_rcv_skb() -> rtnetlink_rcv_msg() = discarded here.
> 
> Nowadays the message is enqueued but it's discarded later. So if I'm not 
> missing anything, I don't see the point of adding a new function to 
> check for capabilities/audit stuff just a bit before.

Two reasons:
1) The sender-side checks allow checking (and auditing) based on the
current task's credentials, vs. having to save the information in the
netlink_skb_parms for use on the receiver side.
2) Performing the check up front at send time allows the kernel to
reject it early and reduce extraneous processing / resource consumption
by unauthorized processes.

-- 
Stephen Smalley <sds@epoch.ncsc.mil>
National Security Agency

Re: [RFC][PATCH 2/3] netlink check sender, audit

[Chris Wright]

* Pablo Neira (pablo@eurodev.net) wrote:
> Pablo Neira wrote:
> 
> >I also see another option which is passing as parameter such function 
> >which check for capabilities/audit stuff to my netlink_process_skb 
> >function, calling it before process_msg. But in that case, the packet 
> >sent by a sender that doesn't has the right to was already enqueued. I 
> >understand that this is exactly what you are trying to avoid.
> 
> 
> With your patch, a message from user space process that doesn't have the 
> capabilites follows this path:
> 
> sys_sendmsg() -> netlink_sendmsg() -> netlink_unicast() -> 
> netlink_sendskb() = discarded here.
> 
> Currently, it continues, for example in case of rtnetlink:
> 
> ... -> netlink_sendskb() -> sk_data_ready(sk, len) -> rtnetlink_rcv() -> 
> rtnetlink_rcv_skb() -> rtnetlink_rcv_msg() = discarded here.
> 
> Nowadays the message is enqueued but it's discarded later. So if I'm not 
> missing anything, I don't see the point of adding a new function to 
> check for capabilities/audit stuff just a bit before.

The purpose is to guarantee that the checks are done in the sender's
context to avoid having to cache values such as capabilities, SELinux
SID, audit loginuid.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [RFC][PATCH 2/3] netlink check sender, audit

[Pablo Neira]

[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]

Chris Wright wrote:

>>With your patch, a message from user space process that doesn't have the 
>>capabilites follows this path:
>>
>>sys_sendmsg() -> netlink_sendmsg() -> netlink_unicast() -> 
>>netlink_sendskb() = discarded here.
>>
>>Currently, it continues, for example in case of rtnetlink:
>>
>>... -> netlink_sendskb() -> sk_data_ready(sk, len) -> rtnetlink_rcv() -> 
>>rtnetlink_rcv_skb() -> rtnetlink_rcv_msg() = discarded here.
>>
>>Nowadays the message is enqueued but it's discarded later. So if I'm not 
>>missing anything, I don't see the point of adding a new function to 
>>check for capabilities/audit stuff just a bit before.
>>    
>>
>
>The purpose is to guarantee that the checks are done in the sender's
>context to avoid having to cache values such as capabilities, SELinux
>SID, audit loginuid.
>  
>

Thanks for the explanation. I don't still like so much the new 
netlink_kernel_create_check function. I think that we could get more 
variations of netlink_kernel_create in future just to add another 
feature/checking. So I prefer new function (netlink_kernel_set_check) 
that set check_sender if it's needed once the netlink socket is created. 
I've modified your patches to use this function.

Comments welcome.

--
Pablo

[-- Attachment #2: 01 --]
[-- Type: text/plain, Size: 1642 bytes --]

===== include/linux/netlink.h 1.23 vs edited =====
--- 1.23/include/linux/netlink.h	2005-02-07 06:59:39 +01:00
+++ edited/include/linux/netlink.h	2005-02-15 02:35:36 +01:00
@@ -117,6 +117,7 @@
 
 
 extern struct sock *netlink_kernel_create(int unit, void (*input)(struct sock *sk, int len));
+extern inline void netlink_kernel_set_check(struct sock *sk, int (*check)(struct sk_buff *skb));
 extern void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err);
 extern int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 pid, int nonblock);
 extern int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, __u32 pid,
===== net/netlink/af_netlink.c 1.69 vs edited =====
--- 1.69/net/netlink/af_netlink.c	2005-01-21 21:25:32 +01:00
+++ edited/net/netlink/af_netlink.c	2005-02-15 02:35:49 +01:00
@@ -71,6 +71,7 @@
 	struct netlink_callback	*cb;
 	spinlock_t		cb_lock;
 	void			(*data_ready)(struct sock *sk, int bytes);
+	int			(*check_sender)(struct sk_buff *skb);
 };
 
 #define nlk_sk(__sk) ((struct netlink_opt *)(__sk)->sk_protinfo)
@@ -1063,6 +1064,12 @@
 	return sk;
 }
 
+inline void netlink_kernel_set_check(struct sock *sk, 
+				     int (*check)(struct sk_buff *skb))
+{
+	nlk_sk(sk)->check_sender = check;
+}
+
 void netlink_set_nonroot(int protocol, unsigned int flags)
 { 
 	if ((unsigned int)protocol < MAX_LINKS) 
@@ -1460,6 +1467,7 @@
 EXPORT_SYMBOL(netlink_broadcast);
 EXPORT_SYMBOL(netlink_dump_start);
 EXPORT_SYMBOL(netlink_kernel_create);
+EXPORT_SYMBOL(netlink_kernel_set_check);
 EXPORT_SYMBOL(netlink_register_notifier);
 EXPORT_SYMBOL(netlink_set_err);
 EXPORT_SYMBOL(netlink_set_nonroot);

[-- Attachment #3: 02 --]
[-- Type: text/plain, Size: 1683 bytes --]

===== kernel/audit.c 1.9 vs edited =====
--- 1.9/kernel/audit.c	2005-01-31 07:33:47 +01:00
+++ edited/kernel/audit.c	2005-02-15 02:17:22 +01:00
@@ -309,27 +309,36 @@
  * Check for appropriate CAP_AUDIT_ capabilities on incoming audit
  * control messages.
  */
-static int audit_netlink_ok(kernel_cap_t eff_cap, u16 msg_type)
+static int audit_check_sender(struct sk_buff *skb)
 {
-	int err = 0;
+	struct nlmsghdr *nlh;
+	u16 msg_type;
+	int err = -EINVAL;
 
+	if (skb->len < NLMSG_LENGTH(0))
+		goto out;
+
+	nlh = (struct nlmsghdr *)skb->data;
+	msg_type = nlh->nlmsg_type;
+
+	err = 0;
 	switch (msg_type) {
 	case AUDIT_GET:
 	case AUDIT_LIST:
 	case AUDIT_SET:
 	case AUDIT_ADD:
 	case AUDIT_DEL:
-		if (!cap_raised(eff_cap, CAP_AUDIT_CONTROL))
+		if (!capable(CAP_AUDIT_CONTROL))
 			err = -EPERM;
 		break;
 	case AUDIT_USER:
-		if (!cap_raised(eff_cap, CAP_AUDIT_WRITE))
+		if (!capable(CAP_AUDIT_WRITE))
 			err = -EPERM;
 		break;
 	default:  /* bad msg */
 		err = -EINVAL;
 	}
-
+out:
 	return err;
 }
 
@@ -338,14 +347,10 @@
 	u32			uid, pid, seq;
 	void			*data;
 	struct audit_status	*status_get, status_set;
-	int			err;
+	int			err = 0;
 	struct audit_buffer	*ab;
 	u16			msg_type = nlh->nlmsg_type;
 
-	err = audit_netlink_ok(NETLINK_CB(skb).eff_cap, msg_type);
-	if (err)
-		return err;
-
 	pid  = NETLINK_CREDS(skb)->pid;
 	uid  = NETLINK_CREDS(skb)->uid;
 	seq  = nlh->nlmsg_seq;
@@ -554,6 +559,8 @@
 	audit_sock = netlink_kernel_create(NETLINK_AUDIT, audit_receive);
 	if (!audit_sock)
 		audit_panic("cannot initialize netlink socket");
+	
+	netlink_kernel_set_check(audit_sock, audit_check_sender);
 
 	audit_initialized = 1;
 	audit_enabled = audit_default;

[-- Attachment #4: 03 --]
[-- Type: text/plain, Size: 1676 bytes --]

===== net/core/rtnetlink.c 1.33 vs edited =====
--- 1.33/net/core/rtnetlink.c	2005-01-10 22:42:22 +01:00
+++ edited/net/core/rtnetlink.c	2005-02-15 02:28:37 +01:00
@@ -462,8 +462,32 @@
 static struct rtattr **rta_buf;
 static int rtattr_max;
 
-/* Process one rtnetlink message. */
+static int rtnetlink_check_sender(struct sk_buff *skb)
+{
+	struct nlmsghdr *nlh;
+	int kind;
+	int type;
+
+	if (skb->len < NLMSG_LENGTH(0))
+		return -EINVAL;
+
+	nlh = (struct nlmsghdr *)skb->data;
+	type = nlh->nlmsg_type;
+
+	/* Unknown message: reply with EINVAL */
+	if (type > RTM_MAX)
+		return -EINVAL;
+
+	type -= RTM_BASE;
+	kind = type&3;
 
+	if (kind != 2 && !capable(CAP_NET_ADMIN))
+		return -EPERM;
+
+	return 0;
+}
+
+/* Process one rtnetlink message. */
 static __inline__ int
 rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, int *errp)
 {
@@ -485,10 +509,6 @@
 	if (type < RTM_BASE)
 		return 0;
 
-	/* Unknown message: reply with EINVAL */
-	if (type > RTM_MAX)
-		goto err_inval;
-
 	type -= RTM_BASE;
 
 	/* All the messages must have at least 1 byte length */
@@ -509,11 +529,6 @@
 	sz_idx = type>>2;
 	kind = type&3;
 
-	if (kind != 2 && security_netlink_recv(skb)) {
-		*errp = -EPERM;
-		return -1;
-	}
-
 	if (kind == 2 && nlh->nlmsg_flags&NLM_F_DUMP) {
 		u32 rlen;
 
@@ -693,6 +708,7 @@
 	rtnl = netlink_kernel_create(NETLINK_ROUTE, rtnetlink_rcv);
 	if (rtnl == NULL)
 		panic("rtnetlink_init: cannot initialize rtnetlink\n");
+	netlink_kernel_set_check(rtnl, rtnetlink_check_sender);
 	netlink_set_nonroot(NETLINK_ROUTE, NL_NONROOT_RECV);
 	register_netdevice_notifier(&rtnetlink_dev_notifier);
 	rtnetlink_links[PF_UNSPEC] = link_rtnetlink_table;

Re: [RFC][PATCH 2/3] netlink check sender, audit

[Pablo Neira]

[-- Attachment #1: Type: text/plain, Size: 1340 bytes --]

Pablo Neira wrote:

> Chris Wright wrote:
>
>>> With your patch, a message from user space process that doesn't have 
>>> the capabilites follows this path:
>>>
>>> sys_sendmsg() -> netlink_sendmsg() -> netlink_unicast() -> 
>>> netlink_sendskb() = discarded here.
>>>
>>> Currently, it continues, for example in case of rtnetlink:
>>>
>>> ... -> netlink_sendskb() -> sk_data_ready(sk, len) -> 
>>> rtnetlink_rcv() -> rtnetlink_rcv_skb() -> rtnetlink_rcv_msg() = 
>>> discarded here.
>>>
>>> Nowadays the message is enqueued but it's discarded later. So if I'm 
>>> not missing anything, I don't see the point of adding a new function 
>>> to check for capabilities/audit stuff just a bit before.
>>>   
>>
>>
>> The purpose is to guarantee that the checks are done in the sender's
>> context to avoid having to cache values such as capabilities, SELinux
>> SID, audit loginuid.
>>  
>>
>
> Thanks for the explanation. I don't still like so much the new 
> netlink_kernel_create_check function. I think that we could get more 
> variations of netlink_kernel_create in future just to add another 
> feature/checking. So I prefer new function (netlink_kernel_set_check) 
> that set check_sender if it's needed once the netlink socket is 
> created. I've modified your patches to use this function.


Sorry, I'm stupid. Wrong patch.

--
Pablo

[-- Attachment #2: netlink.patch --]
[-- Type: text/x-patch, Size: 2044 bytes --]

===== net/netlink/af_netlink.c 1.69 vs edited =====
--- 1.69/net/netlink/af_netlink.c	2005-01-21 21:25:32 +01:00
+++ edited/net/netlink/af_netlink.c	2005-02-15 03:34:53 +01:00
@@ -71,6 +71,7 @@
 	struct netlink_callback	*cb;
 	spinlock_t		cb_lock;
 	void			(*data_ready)(struct sock *sk, int bytes);
+	int			(*check_sender)(struct sk_buff *skb);
 };
 
 #define nlk_sk(__sk) ((struct netlink_opt *)(__sk)->sk_protinfo)
@@ -636,9 +637,15 @@
 int netlink_sendskb(struct sock *sk, struct sk_buff *skb, int protocol)
 {
 	struct netlink_opt *nlk;
-	int len = skb->len;
+	int err, len = skb->len;
+ 	nlk = nlk_sk(sk);
+
+	if (nlk->check_sender)
+		if ((err = nlk->check_sender(skb))) {
+			netlink_detachskb(sk, skb);
+			return err;
+		}
 
-	nlk = nlk_sk(sk);
 #ifdef NL_EMULATE_DEV
 	if (nlk->handler) {
 		skb_orphan(skb);
@@ -1063,6 +1070,12 @@
 	return sk;
 }
 
+inline void netlink_kernel_set_check(struct sock *sk, 
+				     int (*check)(struct sk_buff *skb))
+{
+	nlk_sk(sk)->check_sender = check;
+}
+
 void netlink_set_nonroot(int protocol, unsigned int flags)
 { 
 	if ((unsigned int)protocol < MAX_LINKS) 
@@ -1460,6 +1473,7 @@
 EXPORT_SYMBOL(netlink_broadcast);
 EXPORT_SYMBOL(netlink_dump_start);
 EXPORT_SYMBOL(netlink_kernel_create);
+EXPORT_SYMBOL(netlink_kernel_set_check);
 EXPORT_SYMBOL(netlink_register_notifier);
 EXPORT_SYMBOL(netlink_set_err);
 EXPORT_SYMBOL(netlink_set_nonroot);
===== include/linux/netlink.h 1.23 vs edited =====
--- 1.23/include/linux/netlink.h	2005-02-07 06:59:39 +01:00
+++ edited/include/linux/netlink.h	2005-02-15 02:53:35 +01:00
@@ -117,6 +117,7 @@
 
 
 extern struct sock *netlink_kernel_create(int unit, void (*input)(struct sock *sk, int len));
+extern inline void netlink_kernel_set_check(struct sock *sk, int (*check)(struct sk_buff *skb));
 extern void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err);
 extern int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 pid, int nonblock);
 extern int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, __u32 pid,

Re: [RFC][PATCH 2/3] netlink check sender, audit

[Chris Wright]

* Pablo Neira (pablo@eurodev.net) wrote:
> Thanks for the explanation. I don't still like so much the new 
> netlink_kernel_create_check function. I think that we could get more 
> variations of netlink_kernel_create in future just to add another 
> feature/checking. So I prefer new function (netlink_kernel_set_check) 

I agree, had the same concern.  I breifly considered an ops struct that
could be passed in during registration so that it could grow a little
easier.

> that set check_sender if it's needed once the netlink socket is created. 
> I've modified your patches to use this function.

Great, thanks.  This is technically racy.  It's possible (albeit small
window) that something could be delivered before this is set.  Using a
callback struct during registration would fix this.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [RFC][PATCH 2/3] netlink check sender, audit

[Pablo Neira]

[-- Attachment #1: Type: text/plain, Size: 395 bytes --]

Chris Wright wrote:

>Great, thanks.  This is technically racy.  It's possible (albeit small
>window) that something could be delivered before this is set.
>

Yes, hard trigger but that can happen and doesn't tell too much in 
favour of me.

>Using a callback struct during registration would fix this.
>  
>

I agree, maybe something like the example patch attached. Hope that helps.

--
Pablo

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 4161 bytes --]

===== net/netlink/af_netlink.c 1.69 vs edited =====
--- 1.69/net/netlink/af_netlink.c	2005-01-21 21:25:32 +01:00
+++ edited/net/netlink/af_netlink.c	2005-02-15 23:10:47 +01:00
@@ -71,6 +71,7 @@
 	struct netlink_callback	*cb;
 	spinlock_t		cb_lock;
 	void			(*data_ready)(struct sock *sk, int bytes);
+	int			(*check_sender)(struct sk_buff *skb);
 };
 
 #define nlk_sk(__sk) ((struct netlink_opt *)(__sk)->sk_protinfo)
@@ -636,9 +637,16 @@
 int netlink_sendskb(struct sock *sk, struct sk_buff *skb, int protocol)
 {
 	struct netlink_opt *nlk;
-	int len = skb->len;
+	int err, len = skb->len;
+	
+ 	nlk = nlk_sk(sk);
+
+	if (nlk->check_sender)
+		if ((err = nlk->check_sender(skb))) {
+			netlink_detachskb(sk, skb);
+			return err;
+		}
 
-	nlk = nlk_sk(sk);
 #ifdef NL_EMULATE_DEV
 	if (nlk->handler) {
 		skb_orphan(skb);
@@ -1033,7 +1041,7 @@
  */
 
 struct sock *
-netlink_kernel_create(int unit, void (*input)(struct sock *sk, int len))
+netlink_kernel_create(int unit, struct netlink_ops *nlops)
 {
 	struct socket *sock;
 	struct sock *sk;
@@ -1053,8 +1061,12 @@
 	}
 	sk = sock->sk;
 	sk->sk_data_ready = netlink_data_ready;
-	if (input)
-		nlk_sk(sk)->data_ready = input;
+	if (nlops != NULL) {
+		if (nlops->input)
+			nlk_sk(sk)->data_ready = nlops->input;
+		if (nlops->check_sender)
+			nlk_sk(sk)->check_sender = nlops->check_sender;
+	}
 
 	if (netlink_insert(sk, 0)) {
 		sock_release(sock);
===== include/linux/netlink.h 1.23 vs edited =====
--- 1.23/include/linux/netlink.h	2005-02-07 06:59:39 +01:00
+++ edited/include/linux/netlink.h	2005-02-15 22:53:01 +01:00
@@ -115,8 +115,13 @@
 #define NETLINK_CB(skb)		(*(struct netlink_skb_parms*)&((skb)->cb))
 #define NETLINK_CREDS(skb)	(&NETLINK_CB((skb)).creds)
 
+struct netlink_ops
+{
+	void    (*input)(struct sock *sk, int len);
+	int     (*check_sender)(struct sk_buff *skb);
+};
 
-extern struct sock *netlink_kernel_create(int unit, void (*input)(struct sock *sk, int len));
+extern struct sock *netlink_kernel_create(int unit, struct netlink_ops *nlops);
 extern void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err);
 extern int netlink_unicast(struct sock *ssk, struct sk_buff *skb, __u32 pid, int nonblock);
 extern int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, __u32 pid,
===== net/core/rtnetlink.c 1.33 vs edited =====
--- 1.33/net/core/rtnetlink.c	2005-01-10 22:42:22 +01:00
+++ edited/net/core/rtnetlink.c	2005-02-15 23:08:05 +01:00
@@ -462,8 +462,32 @@
 static struct rtattr **rta_buf;
 static int rtattr_max;
 
-/* Process one rtnetlink message. */
+static int rtnetlink_check_sender(struct sk_buff *skb)
+{
+	struct nlmsghdr *nlh;
+	int kind;
+	int type;
+
+	if (skb->len < NLMSG_LENGTH(0))
+		return -EINVAL;
+
+	nlh = (struct nlmsghdr *)skb->data;
+	type = nlh->nlmsg_type;
+
+	/* Unknown message: reply with EINVAL */
+	if (type > RTM_MAX)
+		return -EINVAL;
+
+	type -= RTM_BASE;
+	kind = type&3;
 
+	if (kind != 2 && !capable(CAP_NET_ADMIN))
+		return -EPERM;
+
+	return 0;
+}
+
+/* Process one rtnetlink message. */
 static __inline__ int
 rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh, int *errp)
 {
@@ -485,10 +509,6 @@
 	if (type < RTM_BASE)
 		return 0;
 
-	/* Unknown message: reply with EINVAL */
-	if (type > RTM_MAX)
-		goto err_inval;
-
 	type -= RTM_BASE;
 
 	/* All the messages must have at least 1 byte length */
@@ -509,11 +529,6 @@
 	sz_idx = type>>2;
 	kind = type&3;
 
-	if (kind != 2 && security_netlink_recv(skb)) {
-		*errp = -EPERM;
-		return -1;
-	}
-
 	if (kind == 2 && nlh->nlmsg_flags&NLM_F_DUMP) {
 		u32 rlen;
 
@@ -681,6 +696,10 @@
 void __init rtnetlink_init(void)
 {
 	int i;
+	struct netlink_ops rtnl_ops = {
+		.input 		= rtnetlink_rcv,
+		.check_sender	= rtnetlink_check_sender
+	};
 
 	rtattr_max = 0;
 	for (i = 0; i < ARRAY_SIZE(rta_max); i++)
@@ -690,7 +709,7 @@
 	if (!rta_buf)
 		panic("rtnetlink_init: cannot allocate rta_buf\n");
 
-	rtnl = netlink_kernel_create(NETLINK_ROUTE, rtnetlink_rcv);
+	rtnl = netlink_kernel_create(NETLINK_ROUTE, &rtnl_ops);
 	if (rtnl == NULL)
 		panic("rtnetlink_init: cannot initialize rtnetlink\n");
 	netlink_set_nonroot(NETLINK_ROUTE, NL_NONROOT_RECV);

Re: [RFC][PATCH 2/3] netlink check sender, audit

[Chris Wright]

* Pablo Neira (pablo@eurodev.net) wrote:
> 
> I agree, maybe something like the example patch attached. Hope that helps.

This is better, but...

> -netlink_kernel_create(int unit, void (*input)(struct sock *sk, int len))
> +netlink_kernel_create(int unit, struct netlink_ops *nlops)

...this is exported interface, so would probably require a new function.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [RFC][PATCH 2/3] netlink check sender, audit

[Pablo Neira]

Chris Wright wrote:

>* Pablo Neira (pablo@eurodev.net) wrote:
>  
>
>>I agree, maybe something like the example patch attached. Hope that helps.
>>    
>>
>
>This is better, but...
>
>  
>
>>-netlink_kernel_create(int unit, void (*input)(struct sock *sk, int len))
>>+netlink_kernel_create(int unit, struct netlink_ops *nlops)
>>    
>>
>
>...this is exported interface, so would probably require a new function.
>  
>

I was aware of that. Actually I think that we can modify all calls to 
netlink_kernel_create (that aren't that much) to fit the new interface, 
I can post a patch to do that. I prefer providing just one function to 
create a netlink socket in kernel space.

--
Pablo

Re: [RFC][PATCH 2/3] netlink check sender, audit

[Chris Wright]

* Pablo Neira (pablo@eurodev.net) wrote:
> I was aware of that. Actually I think that we can modify all calls to 
> netlink_kernel_create (that aren't that much) to fit the new interface, 
> I can post a patch to do that. I prefer providing just one function to 
> create a netlink socket in kernel space.

My only thought is as an exported symobl it's harder to be clear on
who's using it (possibly out-of-tree).

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [RFC][PATCH 2/3] netlink check sender, audit

[James Morris]

On Tue, 15 Feb 2005, Chris Wright wrote:

> * Pablo Neira (pablo@eurodev.net) wrote:
> > I was aware of that. Actually I think that we can modify all calls to 
> > netlink_kernel_create (that aren't that much) to fit the new interface, 
> > I can post a patch to do that. I prefer providing just one function to 
> > create a netlink socket in kernel space.
> 
> My only thought is as an exported symobl it's harder to be clear on
> who's using it (possibly out-of-tree).

Doesn't matter, the kernel has no ABI.

http://www.kroah.com/log/linux/stable_api_nonsense.html?seemore=y



- James
-- 
James Morris
<jmorris@redhat.com>

Re: [RFC][PATCH 2/3] netlink check sender, audit

[Chris Wright]

* Pablo Neira (pablo@eurodev.net) wrote:
> Chris Wright wrote:
> >+static int audit_check_sender(struct sk_buff *skb)
> >{
> >-	int err = 0;
> >+	struct nlmsghdr *nlh;
> >+	u16 msg_type;
> >+	int err = -EINVAL;
> >
> >+	if (skb->len < NLMSG_LENGTH(0))
> >+		goto out;
> >+
> >+	nlh = (struct nlmsghdr *)skb->data;
> >+	msg_type = nlh->nlmsg_type;
> 
> You're introducing some kind of check for malformed packets here as 
> well, don't you think that such thing should be done by the receiver ?

This has to be done to make the capability check meaningful, as it's
different per msg type.  Need to have a valid header to check msg type.

> I also see another option which is passing as parameter such function 
> which check for capabilities/audit stuff to my netlink_process_skb 
> function, calling it before process_msg. But in that case, the packet 
> sent by a sender that doesn't has the right to was already enqueued. I 
> understand that this is exactly what you are trying to avoid.

That's how it's done now.  The purpose of this patch is to guarantee the
check is done in the sender's context to avoid having to add values to
the control buffer to support protocol specific data (such as loginuid
in this case of audit).

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [RFC][PATCH 1/3] netlink check sender

[Stephen Smalley]

On Sat, 2005-02-12 at 04:02, Chris Wright wrote:
> ===== net/netlink/af_netlink.c 1.69 vs edited =====
> --- 1.69/net/netlink/af_netlink.c	2005-01-21 12:25:32 -08:00
> +++ edited/net/netlink/af_netlink.c	2005-02-11 18:05:59 -08:00
>  int netlink_sendskb(struct sock *sk, struct sk_buff *skb, int protocol)
>  {
>  	struct netlink_opt *nlk;
> -	int len = skb->len;
> -
> +	int err, len = skb->len;
> +	
>  	nlk = nlk_sk(sk);
> +
> +	printk("%s: %s(%d) send_check %p\n", __FUNCTION__, current->comm, current->pid, nlk->check_sender);
> +	if (nlk->check_sender)
> +		if ((err = nlk->check_sender(skb))) {
> +			netlink_detachskb(sk, skb);
> +			return err;
> +		}
> +

printk() is a leftover from debugging, I assume.  
Why place the check_sender() call here vs. just replacing the existing
security_netlink_send() call in netlink_sendmsg() with this new call?
 
-- 
Stephen Smalley <sds@epoch.ncsc.mil>
National Security Agency

Re: [RFC][PATCH 1/3] netlink check sender

[Stephen Smalley]

On Mon, 2005-02-14 at 07:59, Stephen Smalley wrote:
> printk() is a leftover from debugging, I assume.  
> Why place the check_sender() call here vs. just replacing the existing
> security_netlink_send() call in netlink_sendmsg() with this new call?

Sorry, replacing security_netlink_send() would be bad (for SELinux
checking), but I'm not clear on why you don't put the check_sender()
call right after it in netlink_sendmsg() so that you ensure that you
have complete coverage (vs. unicast-specific).

-- 
Stephen Smalley <sds@tycho.nsa.gov>
National Security Agency

Re: [RFC][PATCH 1/3] netlink check sender

[Chris Wright]

* Stephen Smalley (sds@tycho.nsa.gov) wrote:
> On Mon, 2005-02-14 at 07:59, Stephen Smalley wrote:
> > printk() is a leftover from debugging, I assume.  
> > Why place the check_sender() call here vs. just replacing the existing
> > security_netlink_send() call in netlink_sendmsg() with this new call?
> 
> Sorry, replacing security_netlink_send() would be bad (for SELinux
> checking), but I'm not clear on why you don't put the check_sender()
> call right after it in netlink_sendmsg() so that you ensure that you
> have complete coverage (vs. unicast-specific).

The receiver hasn't been looked up, so you don't have the
nlk_sk()->check_sender handy yet.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [RFC][PATCH 1/3] netlink check sender

[Chris Wright]

* Stephen Smalley (sds@epoch.ncsc.mil) wrote:
> On Sat, 2005-02-12 at 04:02, Chris Wright wrote:
> > ===== net/netlink/af_netlink.c 1.69 vs edited =====
> > --- 1.69/net/netlink/af_netlink.c	2005-01-21 12:25:32 -08:00
> > +++ edited/net/netlink/af_netlink.c	2005-02-11 18:05:59 -08:00
> >  int netlink_sendskb(struct sock *sk, struct sk_buff *skb, int protocol)
> >  {
> >  	struct netlink_opt *nlk;
> > -	int len = skb->len;
> > -
> > +	int err, len = skb->len;
> > +	
> >  	nlk = nlk_sk(sk);
> > +
> > +	printk("%s: %s(%d) send_check %p\n", __FUNCTION__, current->comm, current->pid, nlk->check_sender);
> > +	if (nlk->check_sender)
> > +		if ((err = nlk->check_sender(skb))) {
> > +			netlink_detachskb(sk, skb);
> > +			return err;
> > +		}
> > +
> 
> printk() is a leftover from debugging, I assume.  

Heh, yeah, just leftover gargabe.

> Why place the check_sender() call here vs. just replacing the existing
> security_netlink_send() call in netlink_sendmsg() with this new call?

That's fine, however it needs to be this late, to get the receiver looked up.
I think the sk would change in _send hook, so for RFC, I just left them
separate.  Ideal would be complete consolidation.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net