netlink: have netlink per-protocol bind function return an error code.

Have the netlink per-protocol optional bind function return an int error code rather than void to signal a failure.

This will enable netlink protocols to perform extra checks including capabilities and permissions verifications when updating memberships in multicast groups.

In netlink_bind() and netlink_setsockopt() the call to the per-protocol bind function was moved above the multicast group update to prevent any access to the multicast socket groups before checking with the per-protocol bind function. This will enable the per-protocol bind function to be used to check permissions which could be denied before making them available, and to avoid the messy job of undoing the addition should the per-protocol bind function fail.

The netfilter subsystem seems to be the only one currently using the per-protocol bind function.

---
In particular, the audit subsystem (NETLINK_AUDIT protocol) could benefit by being able to check specific capabilities for each multicast group before granting membership to the requesting socket. Currently, all NETLINK_AUDIT sockets must have the capability CAP_NET_ADMIN. No other capabilities are required to join a multicast group. This capability is too broad allowing access to this socket by many applications that must not have access to this information. It is proposed to add capability CAP_AUDIT_READ to allow this access while dropping the exessively broad capability CAP_NET_ADMIN.

There has also been some interest expressed by IETF ForCES folk.

---
 include/linux/netlink.h   |    2 +-
 net/netfilter/nfnetlink.c |    3 ++-
 net/netlink/af_netlink.c  |   30 +++++++++++++++++-------------
 net/netlink/af_netlink.h  |    4 ++--
 4 files changed, 22 insertions(+), 17 deletions(-)

index 7a6c396..4402653 100644
@@ -45,7 +45,7 @@ struct netlink_kernel_cfg {
 	unsigned int	flags;
 	void		(*input)(struct sk_buff *skb);
 	struct mutex	*cb_mutex;
-	void		(*bind)(int group);
+	int		(*bind)(int group);
 	bool		(*compare)(struct net *net, struct sock *sk);
 };
 

index 75619f9..10a4cf5 100644
@@ -392,7 +392,7 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 }
 
 #ifdef CONFIG_MODULES
-static void nfnetlink_bind(int group)
+static int nfnetlink_bind(int group)
 {
 	const struct nfnetlink_subsystem *ss;
 	int type = nfnl_group2type[group];

@@ -403,6 +403,7 @@ static void nfnetlink_bind(int group)
 	if (!ss) {
 		request_module("nfnetlink-subsys-%d", type);
 	}
+	return 0;
 }
 #endif
 

index bca50b9..4224dc5 100644
@@ -1198,7 +1198,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	struct module *module = NULL;
 	struct mutex *cb_mutex;
 	struct netlink_sock *nlk;
-	void (*bind)(int group);
+	int (*bind)(int group);
 	int err = 0;
 
 	sock->state = SS_UNCONNECTED;

@@ -1441,6 +1441,17 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
 		return 0;
 
+	if (nlk->netlink_bind && nladdr->nl_groups) {
+		int i;
+
+		for (i = 0; i < nlk->ngroups; i++)
+			if (test_bit(i, (long unsigned int *)&nladdr->nl_groups)) {
+				err = nlk->netlink_bind(i);
+				if (err)
+					return err;
+			}
+	}
+
 	netlink_table_grab();
 	netlink_update_subscriptions(sk, nlk->subscriptions +
 					 hweight32(nladdr->nl_groups) -

@@ -1449,15 +1460,6 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	netlink_update_listeners(sk);
 	netlink_table_ungrab();
 
-	if (nlk->netlink_bind && nlk->groups[0]) {
-		int i;
-
-		for (i=0; i<nlk->ngroups; i++) {
-			if (test_bit(i, nlk->groups))
-				nlk->netlink_bind(i);
-		}
-	}
-
 	return 0;
 }
 

@@ -2095,14 +2097,16 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
+		if (nlk->netlink_bind) {
+			err = nlk->netlink_bind(val);
+			if (err)
+				return err;
+		}
 		netlink_table_grab();
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
 
-		if (nlk->netlink_bind)
-			nlk->netlink_bind(val);
-
 		err = 0;
 		break;
 	}

index acbd774..0edb8d5 100644
@@ -37,7 +37,7 @@ struct netlink_sock {
 	struct mutex		*cb_mutex;
 	struct mutex		cb_def_mutex;
 	void			(*netlink_rcv)(struct sk_buff *skb);
-	void			(*netlink_bind)(int group);
+	int			(*netlink_bind)(int group);
 	struct module		*module;
 #ifdef CONFIG_NETLINK_MMAP
 	struct mutex		pg_vec_lock;

@@ -73,7 +73,7 @@ struct netlink_table {
 	unsigned int		groups;
 	struct mutex		*cb_mutex;
 	struct module		*module;
-	void			(*bind)(int group);
+	int			(*bind)(int group);
 	bool			(*compare)(struct net *net, struct sock *sock);
 	int			registered;
 };

1.7.1

From: Richard Guy Briggs <[email protected]>
Date: Fri, 21 Mar 2014 12:39:11 -0400

In the general case, I agree.

In the one existing case (netfilter), it adds a module that is never unloaded. (refcounts are bumped up and down, but I don't see an auto-reap based on cleared multicast group subscriptions.) For that matter, netlink_realloc_groups() isn't reversed on error either.

In the proposed case (audit) it is only a permissions check, so there is nothing to undo.

So, I was being lazy looking at the existing situation.

Is it time to add a ".unbind = netlink_unbind" to struct proto_ops netlink_ops? (I am only half serious here...)

Fair enough. I'll go back and look at updating subscriptions and listeners first and undoing those actions if the bind fails. In the case of netlink_setsockopt() it is just one to undo, which is easy. netlink_bind() is a bit more complex, but doable.

The whole purpose here was to add a way for each protocol to be able to add its own permissions check and signal a way for netlink to refuse the subscription if the userspace process doesn't have the required permissions, so not returning an error defeats that whole purpose.

- RGB

--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Ok, in netlink_bind(), netlink_insert()/netlink_autobind() also need to be undone with netlink_remove() if nlk->portid was not set.

At this stage, that function would be a no-op for netfilter and audit. Are there any out-of-tree users of this per-protocol bind function?

- RGB

--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Have the netlink per-protocol optional bind function return an int error code rather than void to signal a failure.

This will enable netlink protocols to perform extra checks including capabilities and permissions verifications when updating memberships in multicast groups.

In netlink_bind() and netlink_setsockopt() the call to the per-protocol bind function was moved above the multicast group update to prevent any access to the multicast socket groups before checking with the per-protocol bind function. This will enable the per-protocol bind function to be used to check permissions which could be denied before making them available, and to avoid the messy job of undoing the addition should the per-protocol bind function fail.

The netfilter subsystem seems to be the only one currently using the per-protocol bind function.

---
In particular, the audit subsystem (NETLINK_AUDIT protocol) could benefit by being able to check specific capabilities for each multicast group before granting membership to the requesting socket. Currently, all NETLINK_AUDIT sockets must have the capability CAP_NET_ADMIN. No other capabilities are required to join a multicast group. This capability is too broad allowing access to this socket by many applications that must not have access to this information. It is proposed to add capability CAP_AUDIT_READ to allow this access while dropping the exessively broad capability CAP_NET_ADMIN.

There has also been some interest expressed by IETF ForCES folk.

---
 include/linux/netlink.h   |    3 ++-
 net/netfilter/nfnetlink.c |    3 ++-
 net/netlink/af_netlink.c  |   39 ++++++++++++++++++++++++++-------------
 net/netlink/af_netlink.h  |    6 ++++--
 4 files changed, 34 insertions(+), 17 deletions(-)

index 7a6c396..0ba2edd 100644
@@ -45,7 +45,8 @@ struct netlink_kernel_cfg {
 	unsigned int	flags;
 	void		(*input)(struct sk_buff *skb);
 	struct mutex	*cb_mutex;
-	void		(*bind)(int group);
+	int		(*bind)(int group);
+	void		(*unbind)(int group);
 	bool		(*compare)(struct net *net, struct sock *sk);
 };
 

index 1da5ef1..14c8ac8 100644
@@ -392,7 +392,7 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 }
 
 #ifdef CONFIG_MODULES
-static void nfnetlink_bind(int group)
+static int nfnetlink_bind(int group)
 {
 	const struct nfnetlink_subsystem *ss;
 	int type = nfnl_group2type[group];

@@ -402,6 +402,7 @@ static void nfnetlink_bind(int group)
 	rcu_read_unlock();
 	if (!ss)
 		request_module("nfnetlink-subsys-%d", type);
+	return 0;
 }
 #endif
 

index bca50b9..301dfe9 100644
@@ -1198,7 +1198,8 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	struct module *module = NULL;
 	struct mutex *cb_mutex;
 	struct netlink_sock *nlk;
-	void (*bind)(int group);
+	int (*bind)(int group);
+	void (*unbind)(int group);
 	int err = 0;
 
 	sock->state = SS_UNCONNECTED;

@@ -1224,6 +1225,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 		err = -EPROTONOSUPPORT;
 	cb_mutex = nl_table[protocol].cb_mutex;
 	bind = nl_table[protocol].bind;
+	unbind = nl_table[protocol].unbind;
 	netlink_unlock_table();
 
 	if (err < 0)

@@ -1240,6 +1242,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	nlk = nlk_sk(sock->sk);
 	nlk->module = module;
 	nlk->netlink_bind = bind;
+	nlk->netlink_unbind = unbind;
 out:
 	return err;
 

@@ -1293,6 +1296,7 @@ static int netlink_release(struct socket *sock)
 			kfree_rcu(old, rcu);
 			nl_table[sk->sk_protocol].module = NULL;
 			nl_table[sk->sk_protocol].bind = NULL;
+			nl_table[sk->sk_protocol].unbind = NULL;
 			nl_table[sk->sk_protocol].flags = 0;
 			nl_table[sk->sk_protocol].registered = 0;
 		}

@@ -1441,6 +1445,22 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
 		return 0;
 
+	if (nlk->netlink_bind && nladdr->nl_groups) {
+		int i;
+
+		for (i = 0; i < nlk->ngroups; i++)
+			if (test_bit(i, (long unsigned int *)&nladdr->nl_groups)) {
+				err = nlk->netlink_bind(i);
+				if (err)
+					if (!nlk->portid)
+						netlink_remove(sk);
+					for (int undo = 0; undo < i; undo++)
+						if (nlk->netlink_unbind)
+							nlk->netlink_unbind(undo);
+					return err;
+			}
+	}
+
 	netlink_table_grab();
 	netlink_update_subscriptions(sk, nlk->subscriptions +
 					 hweight32(nladdr->nl_groups) -

@@ -1449,15 +1469,6 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	netlink_update_listeners(sk);
 	netlink_table_ungrab();
 
-	if (nlk->netlink_bind && nlk->groups[0]) {
-		int i;
-
-		for (i=0; i<nlk->ngroups; i++) {
-			if (test_bit(i, nlk->groups))
-				nlk->netlink_bind(i);
-		}
-	}
-
 	return 0;
 }
 

@@ -2095,14 +2106,16 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
+		if (nlk->netlink_bind) {
+			err = nlk->netlink_bind(val);
+			if (err)
+				return err;
+		}
 		netlink_table_grab();
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
 
-		if (nlk->netlink_bind)
-			nlk->netlink_bind(val);
-
 		err = 0;
 		break;
 	}

index acbd774..2aeae8a 100644
@@ -37,7 +37,8 @@ struct netlink_sock {
 	struct mutex		*cb_mutex;
 	struct mutex		cb_def_mutex;
 	void			(*netlink_rcv)(struct sk_buff *skb);
-	void			(*netlink_bind)(int group);
+	int			(*netlink_bind)(int group);
+	void			(*netlink_unbind)(int group);
 	struct module		*module;
 #ifdef CONFIG_NETLINK_MMAP
 	struct mutex		pg_vec_lock;

@@ -73,7 +74,8 @@ struct netlink_table {
 	unsigned int		groups;
 	struct mutex		*cb_mutex;
 	struct module		*module;
-	void			(*bind)(int group);
+	int			(*bind)(int group);
+	void			(*unbind)(int group);
 	bool			(*compare)(struct net *net, struct sock *sock);
 	int			registered;
 };

1.7.1

Have the netlink per-protocol optional bind function return an int error code rather than void to signal a failure.

This will enable netlink protocols to perform extra checks including capabilities and permissions verifications when updating memberships in multicast groups.

In netlink_bind() and netlink_setsockopt() the call to the per-protocol bind function was moved above the multicast group update to prevent any access to the multicast socket groups before checking with the per-protocol bind function. This will enable the per-protocol bind function to be used to check permissions which could be denied before making them available, and to avoid the messy job of undoing the addition should the per-protocol bind function fail.

The netfilter subsystem seems to be the only one currently using the per-protocol bind function.

---
In particular, the audit subsystem (NETLINK_AUDIT protocol) could benefit by being able to check specific capabilities for each multicast group before granting membership to the requesting socket. Currently, all NETLINK_AUDIT sockets must have the capability CAP_NET_ADMIN. No other capabilities are required to join a multicast group. This capability is too broad allowing access to this socket by many applications that must not have access to this information. It is proposed to add capability CAP_AUDIT_READ to allow this access while dropping the exessively broad capability CAP_NET_ADMIN.

There has also been some interest expressed by IETF ForCES folk.

v4: fixed a logic error (missing braces) and a C99 error.

v3: added netlink_unbind() option and unwound netlink_insert().

---
 include/linux/netlink.h   |    3 ++-
 net/netfilter/nfnetlink.c |    3 ++-
 net/netlink/af_netlink.c  |   39 ++++++++++++++++++++++++++-------------
 net/netlink/af_netlink.h  |    6 ++++--
 4 files changed, 34 insertions(+), 17 deletions(-)

index 7a6c396..0ba2edd 100644
@@ -45,7 +45,8 @@ struct netlink_kernel_cfg {
 	unsigned int	flags;
 	void		(*input)(struct sk_buff *skb);
 	struct mutex	*cb_mutex;
-	void		(*bind)(int group);
+	int		(*bind)(int group);
+	void		(*unbind)(int group);
 	bool		(*compare)(struct net *net, struct sock *sk);
 };
 

index 1da5ef1..14c8ac8 100644
@@ -392,7 +392,7 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 }
 
 #ifdef CONFIG_MODULES
-static void nfnetlink_bind(int group)
+static int nfnetlink_bind(int group)
 {
 	const struct nfnetlink_subsystem *ss;
 	int type = nfnl_group2type[group];

@@ -402,6 +402,7 @@ static void nfnetlink_bind(int group)
 	rcu_read_unlock();
 	if (!ss)
 		request_module("nfnetlink-subsys-%d", type);
+	return 0;
 }
 #endif
 

index bca50b9..301dfe9 100644
@@ -1198,7 +1198,8 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	struct module *module = NULL;
 	struct mutex *cb_mutex;
 	struct netlink_sock *nlk;
-	void (*bind)(int group);
+	int (*bind)(int group);
+	void (*unbind)(int group);
 	int err = 0;
 
 	sock->state = SS_UNCONNECTED;

@@ -1224,6 +1225,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 		err = -EPROTONOSUPPORT;
 	cb_mutex = nl_table[protocol].cb_mutex;
 	bind = nl_table[protocol].bind;
+	unbind = nl_table[protocol].unbind;
 	netlink_unlock_table();
 
 	if (err < 0)

@@ -1240,6 +1242,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	nlk = nlk_sk(sock->sk);
 	nlk->module = module;
 	nlk->netlink_bind = bind;
+	nlk->netlink_unbind = unbind;
 out:
 	return err;
 

@@ -1293,6 +1296,7 @@ static int netlink_release(struct socket *sock)
 			kfree_rcu(old, rcu);
 			nl_table[sk->sk_protocol].module = NULL;
 			nl_table[sk->sk_protocol].bind = NULL;
+			nl_table[sk->sk_protocol].unbind = NULL;
 			nl_table[sk->sk_protocol].flags = 0;
 			nl_table[sk->sk_protocol].registered = 0;
 		}

@@ -1441,6 +1445,22 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
 		return 0;
 
+	if (nlk->netlink_bind && nladdr->nl_groups) {
+		int i;
+
+		for (i = 0; i < nlk->ngroups; i++)
+			if (test_bit(i, (long unsigned int *)&nladdr->nl_groups)) {
+				err = nlk->netlink_bind(i);
+				if (err)
+					if (!nlk->portid)
+						netlink_remove(sk);
+					for (int undo = 0; undo < i; undo++)
+						if (nlk->netlink_unbind)
+							nlk->netlink_unbind(undo);
+					return err;
+			}
+	}
+
 	netlink_table_grab();
 	netlink_update_subscriptions(sk, nlk->subscriptions +
 					 hweight32(nladdr->nl_groups) -

@@ -1449,15 +1469,6 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	netlink_update_listeners(sk);
 	netlink_table_ungrab();
 
-	if (nlk->netlink_bind && nlk->groups[0]) {
-		int i;
-
-		for (i=0; i<nlk->ngroups; i++) {
-			if (test_bit(i, nlk->groups))
-				nlk->netlink_bind(i);
-		}
-	}
-
 	return 0;
 }
 

@@ -2095,14 +2106,16 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
+		if (nlk->netlink_bind) {
+			err = nlk->netlink_bind(val);
+			if (err)
+				return err;
+		}
 		netlink_table_grab();
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
 
-		if (nlk->netlink_bind)
-			nlk->netlink_bind(val);
-
 		err = 0;
 		break;
 	}

index acbd774..2aeae8a 100644
@@ -37,7 +37,8 @@ struct netlink_sock {
 	struct mutex		*cb_mutex;
 	struct mutex		cb_def_mutex;
 	void			(*netlink_rcv)(struct sk_buff *skb);
-	void			(*netlink_bind)(int group);
+	int			(*netlink_bind)(int group);
+	void			(*netlink_unbind)(int group);
 	struct module		*module;
 #ifdef CONFIG_NETLINK_MMAP
 	struct mutex		pg_vec_lock;

@@ -73,7 +74,8 @@ struct netlink_table {
 	unsigned int		groups;
 	struct mutex		*cb_mutex;
 	struct module		*module;
-	void			(*bind)(int group);
+	int			(*bind)(int group);
+	void			(*unbind)(int group);
 	bool			(*compare)(struct net *net, struct sock *sock);
 	int			registered;
 };

1.7.1

Have the netlink per-protocol optional bind function return an int error code rather than void to signal a failure.

This will enable netlink protocols to perform extra checks including capabilities and permissions verifications when updating memberships in multicast groups.

In netlink_bind() and netlink_setsockopt() the call to the per-protocol bind function was moved above the multicast group update to prevent any access to the multicast socket groups before checking with the per-protocol bind function. This will enable the per-protocol bind function to be used to check permissions which could be denied before making them available, and to avoid the messy job of undoing the addition should the per-protocol bind function fail.

The netfilter subsystem seems to be the only one currently using the per-protocol bind function.

---
In particular, the audit subsystem (NETLINK_AUDIT protocol) could benefit by being able to check specific capabilities for each multicast group before granting membership to the requesting socket. Currently, all NETLINK_AUDIT sockets must have the capability CAP_NET_ADMIN. No other capabilities are required to join a multicast group. This capability is too broad allowing access to this socket by many applications that must not have access to this information. It is proposed to add capability CAP_AUDIT_READ to allow this access while dropping the exessively broad capability CAP_NET_ADMIN.

There has also been some interest expressed by IETF ForCES folk.

v5: *really* fixed a logic error (missing braces) and a C99 error.

v3: added netlink_unbind() option and unwound netlink_insert().

---
 include/linux/netlink.h   |    3 ++-
 net/netfilter/nfnetlink.c |    3 ++-
 net/netlink/af_netlink.c  |   39 ++++++++++++++++++++++++++-------------
 net/netlink/af_netlink.h  |    6 ++++--
 4 files changed, 34 insertions(+), 17 deletions(-)

index 7a6c396..0ba2edd 100644
@@ -45,7 +45,8 @@ struct netlink_kernel_cfg {
 	unsigned int	flags;
 	void		(*input)(struct sk_buff *skb);
 	struct mutex	*cb_mutex;
-	void		(*bind)(int group);
+	int		(*bind)(int group);
+	void		(*unbind)(int group);
 	bool		(*compare)(struct net *net, struct sock *sk);
 };
 

index 1da5ef1..14c8ac8 100644
@@ -392,7 +392,7 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 }
 
 #ifdef CONFIG_MODULES
-static void nfnetlink_bind(int group)
+static int nfnetlink_bind(int group)
 {
 	const struct nfnetlink_subsystem *ss;
 	int type = nfnl_group2type[group];

@@ -402,6 +402,7 @@ static void nfnetlink_bind(int group)
 	rcu_read_unlock();
 	if (!ss)
 		request_module("nfnetlink-subsys-%d", type);
+	return 0;
 }
 #endif
 

index bca50b9..301dfe9 100644
@@ -1198,7 +1198,8 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	struct module *module = NULL;
 	struct mutex *cb_mutex;
 	struct netlink_sock *nlk;
-	void (*bind)(int group);
+	int (*bind)(int group);
+	void (*unbind)(int group);
 	int err = 0;
 
 	sock->state = SS_UNCONNECTED;

@@ -1224,6 +1225,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 		err = -EPROTONOSUPPORT;
 	cb_mutex = nl_table[protocol].cb_mutex;
 	bind = nl_table[protocol].bind;
+	unbind = nl_table[protocol].unbind;
 	netlink_unlock_table();
 
 	if (err < 0)

@@ -1240,6 +1242,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	nlk = nlk_sk(sock->sk);
 	nlk->module = module;
 	nlk->netlink_bind = bind;
+	nlk->netlink_unbind = unbind;
 out:
 	return err;
 

@@ -1293,6 +1296,7 @@ static int netlink_release(struct socket *sock)
 			kfree_rcu(old, rcu);
 			nl_table[sk->sk_protocol].module = NULL;
 			nl_table[sk->sk_protocol].bind = NULL;
+			nl_table[sk->sk_protocol].unbind = NULL;
 			nl_table[sk->sk_protocol].flags = 0;
 			nl_table[sk->sk_protocol].registered = 0;
 		}

@@ -1441,6 +1445,22 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
 		return 0;
 
+	if (nlk->netlink_bind && nladdr->nl_groups) {
+		int i;
+
+		for (i = 0; i < nlk->ngroups; i++)
+			if (test_bit(i, (long unsigned int *)&nladdr->nl_groups)) {
+				err = nlk->netlink_bind(i);
+				if (err)
+					if (!nlk->portid)
+						netlink_remove(sk);
+					for (int undo = 0; undo < i; undo++)
+						if (nlk->netlink_unbind)
+							nlk->netlink_unbind(undo);
+					return err;
+			}
+	}
+
 	netlink_table_grab();
 	netlink_update_subscriptions(sk, nlk->subscriptions +
 					 hweight32(nladdr->nl_groups) -

@@ -1449,15 +1469,6 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	netlink_update_listeners(sk);
 	netlink_table_ungrab();
 
-	if (nlk->netlink_bind && nlk->groups[0]) {
-		int i;
-
-		for (i=0; i<nlk->ngroups; i++) {
-			if (test_bit(i, nlk->groups))
-				nlk->netlink_bind(i);
-		}
-	}
-
 	return 0;
 }
 

@@ -2095,14 +2106,16 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
+		if (nlk->netlink_bind) {
+			err = nlk->netlink_bind(val);
+			if (err)
+				return err;
+		}
 		netlink_table_grab();
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
 
-		if (nlk->netlink_bind)
-			nlk->netlink_bind(val);
-
 		err = 0;
 		break;
 	}

index acbd774..2aeae8a 100644
@@ -37,7 +37,8 @@ struct netlink_sock {
 	struct mutex		*cb_mutex;
 	struct mutex		cb_def_mutex;
 	void			(*netlink_rcv)(struct sk_buff *skb);
-	void			(*netlink_bind)(int group);
+	int			(*netlink_bind)(int group);
+	void			(*netlink_unbind)(int group);
 	struct module		*module;
 #ifdef CONFIG_NETLINK_MMAP
 	struct mutex		pg_vec_lock;

@@ -73,7 +74,8 @@ struct netlink_table {
 	unsigned int		groups;
 	struct mutex		*cb_mutex;
 	struct module		*module;
-	void			(*bind)(int group);
+	int			(*bind)(int group);
+	void			(*unbind)(int group);
 	bool			(*compare)(struct net *net, struct sock *sock);
 	int			registered;
 };

1.7.1

From: Richard Guy Briggs <[email protected]>
Date: Mon, 24 Mar 2014 16:59:23 -0400

Did you notice all the attempts to send a corrected patch? v6 finally got it right (sent Monday). Sorry for all the noise. Serve me right for not sitting on my hands before sending the email for a bit while I checked it.

It was tested, but the patched code wasn't actually checked in. It was still in my tree. I ran "git commit --amend" more than once, forgetting the "-a", and didn't carefully enough inspect the resulting patch.

It is above the for now (also in v6).

- RGB

--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Have the netlink per-protocol optional bind function return an int error code rather than void to signal a failure.

This will enable netlink protocols to perform extra checks including capabilities and permissions verifications when updating memberships in multicast groups.

In netlink_bind() and netlink_setsockopt() the call to the per-protocol bind function was moved above the multicast group update to prevent any access to the multicast socket groups before checking with the per-protocol bind function. This will enable the per-protocol bind function to be used to check permissions which could be denied before making them available, and to avoid the messy job of undoing the addition should the per-protocol bind function fail.

The netfilter subsystem seems to be the only one currently using the per-protocol bind function.

---
In particular, the audit subsystem (NETLINK_AUDIT protocol) could benefit by being able to check specific capabilities for each multicast group before granting membership to the requesting socket. Currently, all NETLINK_AUDIT sockets must have the capability CAP_NET_ADMIN. No other capabilities are required to join a multicast group. This capability is too broad allowing access to this socket by many applications that must not have access to this information. It is proposed to add capability CAP_AUDIT_READ to allow this access while dropping the exessively broad capability CAP_NET_ADMIN.

There has also been some interest expressed by IETF ForCES folk.

v6: *really* fixed a logic error (missing braces) and a C99 error.

v3: added netlink_unbind() option and unwound netlink_insert().

---
 include/linux/netlink.h   |    3 ++-
 net/netfilter/nfnetlink.c |    3 ++-
 net/netlink/af_netlink.c  |   41 ++++++++++++++++++++++++++++-------------
 net/netlink/af_netlink.h  |    6 ++++--
 4 files changed, 36 insertions(+), 17 deletions(-)

index 7a6c396..0ba2edd 100644
@@ -45,7 +45,8 @@ struct netlink_kernel_cfg {
 	unsigned int	flags;
 	void		(*input)(struct sk_buff *skb);
 	struct mutex	*cb_mutex;
-	void		(*bind)(int group);
+	int		(*bind)(int group);
+	void		(*unbind)(int group);
 	bool		(*compare)(struct net *net, struct sock *sk);
 };
 

index 1da5ef1..14c8ac8 100644
@@ -392,7 +392,7 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 }
 
 #ifdef CONFIG_MODULES
-static void nfnetlink_bind(int group)
+static int nfnetlink_bind(int group)
 {
 	const struct nfnetlink_subsystem *ss;
 	int type = nfnl_group2type[group];

@@ -402,6 +402,7 @@ static void nfnetlink_bind(int group)
 	rcu_read_unlock();
 	if (!ss)
 		request_module("nfnetlink-subsys-%d", type);
+	return 0;
 }
 #endif
 

index acbd774..2aeae8a 100644
@@ -37,7 +37,8 @@ struct netlink_sock {
 	struct mutex		*cb_mutex;
 	struct mutex		cb_def_mutex;
 	void			(*netlink_rcv)(struct sk_buff *skb);
-	void			(*netlink_bind)(int group);
+	int			(*netlink_bind)(int group);
+	void			(*netlink_unbind)(int group);
 	struct module		*module;
 #ifdef CONFIG_NETLINK_MMAP
 	struct mutex		pg_vec_lock;

@@ -73,7 +74,8 @@ struct netlink_table {
 	unsigned int		groups;
 	struct mutex		*cb_mutex;
 	struct module		*module;
-	void			(*bind)(int group);
+	int			(*bind)(int group);
+	void			(*unbind)(int group);
 	bool			(*compare)(struct net *net, struct sock *sock);
 	int			registered;
 };

1.7.1

From: Richard Guy Briggs <[email protected]>
Date: Tue, 25 Mar 2014 08:50:56 -0400

...

Ok, sorry for all the noise. I had a problem between chair and keyboard which can explain the number of useless revs of this patch... v6 is current with potentially v7 with the inline hunk below...

This is also true with netlink_setsockopt() in the NETLINK_{ADD,DROP}_MEMBERSHIP case, where on ADD we should call netlink_bind() but on DROP we should *not* call it and instead call netlink_unbind() afterwards, perhaps such as:

index 45dccff..3354d54 100644
@@ -2108,7 +2108,7 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
-		if (nlk->netlink_bind) {
+		if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind) {
 			err = nlk->netlink_bind(val);
 			if (err)
 				return err;

@@ -2117,6 +2117,8 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
+		if (optname == NETLINK_DROP_MEMBERSHIP && nlk->netlink_unbind)
+			nlk->netlink_unbind(val);
 
 		err = 0;
 		break;

So netlink_setsockopt() was right, but ADD/DROP need slightly different treatment.

netlink_bind() just needs to undo netlink_bind() on error.

We could do all we wanted in audit, but that would still not signal netlink to block that subscription.

- RGB

--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

This set provides a way for per-protocol bind functions to signal an error and to be able to clean up after themselves.

The first patch has already been accepted, but is included just in case to avoid a merge error.

The second patch adds the per-protocol bind return code to signal to the netlink code that no further processing should be done and to undo the work already done. This rev has fixed DaveM's last issue and flattened the intentation as requested by Patrick McHardy by two by reworking the logic.

The third provides a way per protocol to undo actions on DROP.

Thanks for the feedback.

Richard Guy Briggs (3):
netlink: simplify nfnetlink_bind
netlink: have netlink per-protocol bind function return an error code.
netlink: implement unbind to netlink_setsockopt NETLINK_DROP_MEMBERSHIP

 include/linux/netlink.h   |    3 ++-
 net/netfilter/nfnetlink.c |   10 ++++------
 net/netlink/af_netlink.c  |   44 +++++++++++++++++++++++++++++++-------------
 net/netlink/af_netlink.h  |    6 ++++--
 4 files changed, 41 insertions(+), 22 deletions(-)

From: Richard Guy Briggs <[email protected]>
Date: Tue, 1 Apr 2014 10:14:55 -0400

I would like to defer this to the next merge window.

I'd also like to see how the AUDIT code is going to use this, provide the user in your next submission.

Right now the only user is nfnetlink and it's merely to do a (sub-)module request.

Therefore it's no surprise that we've never had any real well thought out semantics defined for the bind method, and it's also why we never thought of adding an unbind method before.

I was hoping to get it into this merge window, but but I agree it is a bit late for that. If I had succeeded in posting it to the correct list address back in February it wouldn't be late.

That context was already posted here:

	https://www.redhat.com/archives/linux-audit/2014-February/msg00102.html
	https://lkml.org/lkml/2014/2/19/481

I discovered later I used a stale list address for netdev and didn't Cc you directly, so you likely would have missed it.

No problem. It was recommended I resend patch 3/5 of that set, isolated, to get it reviewed here. These recent changes to that patch should not affect patches 1, 2, 4, 5 of that original patch context.

Does that help?

- RGB

--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

From: Richard Guy Briggs <[email protected]>
Date: Tue, 1 Apr 2014 18:12:15 -0400

It will when you include in your resubmission when net-next opens back up, I don't have time to look at it right now as I'm working on the current merge window fevorishly.

Remove duplicity and simplify code flow by moving the rcu_read_unlock() above the condition and let the flow control exit naturally at the end of the function.

---
 net/netfilter/nfnetlink.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

index 046aa13..1da5ef1 100644
@@ -399,12 +399,9 @@ static void nfnetlink_bind(int group)
 
 	rcu_read_lock();
 	ss = nfnetlink_get_subsys(type);
-	if (!ss) {
-		rcu_read_unlock();
-		request_module("nfnetlink-subsys-%d", type);
-		return;
-	}
 	rcu_read_unlock();
+	if (!ss)
+		request_module("nfnetlink-subsys-%d", type);
 }
 #endif
 

1.7.1

Have the netlink per-protocol optional bind function return an int error code rather than void to signal a failure.

This will enable netlink protocols to perform extra checks including capabilities and permissions verifications when updating memberships in multicast groups.

In netlink_bind() and netlink_setsockopt() the call to the per-protocol bind function was moved above the multicast group update to prevent any access to the multicast socket groups before checking with the per-protocol bind function. This will enable the per-protocol bind function to be used to check permissions which could be denied before making them available, and to avoid the messy job of undoing the addition should the per-protocol bind function fail.

The netfilter subsystem seems to be the only one currently using the per-protocol bind function.

---
In particular, the audit subsystem (NETLINK_AUDIT protocol) could benefit by being able to check specific capabilities for each multicast group before granting membership to the requesting socket. Currently, all NETLINK_AUDIT sockets must have the capability CAP_NET_ADMIN. No other capabilities are required to join a multicast group. This capability is too broad allowing access to this socket by many applications that must not have access to this information. It is proposed to add capability CAP_AUDIT_READ to allow this access while dropping the exessively broad capability CAP_NET_ADMIN.

There has also been some interest expressed by IETF ForCES folk.

v7: fix formatting and rework logic to flatten indentation.

v6: *really* fixed a logic error (missing braces) and a C99 error.

v3: added netlink_unbind() option and unwound netlink_insert().

---
 include/linux/netlink.h   |    3 ++-
 net/netfilter/nfnetlink.c |    3 ++-
 net/netlink/af_netlink.c  |   42 +++++++++++++++++++++++++++++-------------
 net/netlink/af_netlink.h  |    6 ++++--
 4 files changed, 37 insertions(+), 17 deletions(-)

index 7a6c396..0ba2edd 100644
@@ -45,7 +45,8 @@ struct netlink_kernel_cfg {
 	unsigned int	flags;
 	void		(*input)(struct sk_buff *skb);
 	struct mutex	*cb_mutex;
-	void		(*bind)(int group);
+	int		(*bind)(int group);
+	void		(*unbind)(int group);
 	bool		(*compare)(struct net *net, struct sock *sk);
 };
 

index 1da5ef1..14c8ac8 100644
@@ -392,7 +392,7 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 }
 
 #ifdef CONFIG_MODULES
-static void nfnetlink_bind(int group)
+static int nfnetlink_bind(int group)
 {
 	const struct nfnetlink_subsystem *ss;
 	int type = nfnl_group2type[group];

@@ -402,6 +402,7 @@ static void nfnetlink_bind(int group)
 	rcu_read_unlock();
 	if (!ss)
 		request_module("nfnetlink-subsys-%d", type);
+	return 0;
 }
 #endif
 

index acbd774..2aeae8a 100644
@@ -37,7 +37,8 @@ struct netlink_sock {
 	struct mutex		*cb_mutex;
 	struct mutex		cb_def_mutex;
 	void			(*netlink_rcv)(struct sk_buff *skb);
-	void			(*netlink_bind)(int group);
+	int			(*netlink_bind)(int group);
+	void			(*netlink_unbind)(int group);
 	struct module		*module;
 #ifdef CONFIG_NETLINK_MMAP
 	struct mutex		pg_vec_lock;

@@ -73,7 +74,8 @@ struct netlink_table {
 	unsigned int		groups;
 	struct mutex		*cb_mutex;
 	struct module		*module;
-	void			(*bind)(int group);
+	int			(*bind)(int group);
+	void			(*unbind)(int group);
 	bool			(*compare)(struct net *net, struct sock *sock);
 	int			registered;
 };

1.7.1

Call the per-protocol unbind function rather than bind function on NETLINK_DROP_MEMBERSHIP in netlink_setsockopt().

---
 net/netlink/af_netlink.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

index 9ae6e0f..3938138 100644
@@ -2109,7 +2109,7 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
-		if (nlk->netlink_bind) {
+		if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind) {
 			err = nlk->netlink_bind(val);
 			if (err)
 				return err;

@@ -2118,6 +2118,8 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
+		if (optname == NETLINK_DROP_MEMBERSHIP && nlk->netlink_unbind)
+			nlk->netlink_unbind(val);
 
 		err = 0;
 		break;

1.7.1

This is a patch set Eric Paris and I have been working on to add a restricted capability read-only netlink multicast socket to kernel audit to enable userspace clients such as systemd/journald to receive audit logs, in addition to the bidirectional auditd userspace client.

Currently, auditd has the CAP_AUDIT_CONTROL and CAP_AUDIT_WRITE capabilities (but uses CAP_NET_ADMIN). The CAP_AUDIT_READ capability will be added for use by read-only AUDIT_NLGRP_READLOG multicast group clients to the kaudit subsystem. This will remove the dependence on CAP_NET_ADMIN for the multicast read-only socket.

Patches 1-3 provide a way for per-protocol bind functions to signal an error and to be able to clean up after themselves.

The first netfilter cleanup patch has already been accepted by a netfilter maintainer, though I don't see it upstream yet, so it is included for completeness.

The second patch adds the per-protocol bind function return code to signal to the netlink code that no further processing should be done and to undo the work already done. This rev fixes a bug introduced by flattening the code in the last posting.

The third provides a way per protocol to undo bind actions on DROP.

Patches 4-6 implement the audit multicast socket with capability checking.

The fourth patch adds the bind function capability check to multicast join requests for audit.

The fifth patch adds the audit log read multicast group. An assumption has been made that systemd/journald reside in the initial network namespace. This could be changed to check the actual network namespace of systemd/journald should this assumption no longer be true since audit now supports all network namespaces. This version of the patch now directly sends the broadcast when the packet is ready rather than waiting until it passes the queue.

The sixth checks if any clients actually exist before sending.

Since the net tree is busier than the audit tree, conflicts are more likely and the audit patches depend on the net patches, it is proposed to have the net tree carry this entire patchset for 3.16. Are the net maintainers ok with this?

https://bugzilla.redhat.com/show_bug.cgi?id=887992

First posted: https://www.redhat.com/archives/linux-audit/2013-January/msg00008.html

                https://lkml.org/lkml/2013/1/27/279

Please find source for a test program at:
http://people.redhat.com/rbriggs/audit-multicast-listen/

Richard Guy Briggs (6):
netlink: simplify nfnetlink_bind
netlink: have netlink per-protocol bind function return an error
code.
netlink: implement unbind to netlink_setsockopt
NETLINK_DROP_MEMBERSHIP
audit: add netlink audit protocol bind to check capabilities on
multicast join
audit: add netlink multicast group for log read
audit: send multicast messages only if there are listeners

 include/linux/netlink.h             |    3 +-
 include/uapi/linux/audit.h          |    8 ++++
 include/uapi/linux/capability.h     |    7 +++-
 kernel/audit.c                      |   64 ++++++++++++++++++++++++++++++++--
 net/netfilter/nfnetlink.c           |   10 ++---
 net/netlink/af_netlink.c            |   45 +++++++++++++++++-------
 net/netlink/af_netlink.h            |    6 ++-
 security/selinux/include/classmap.h |    2 +-
 8 files changed, 117 insertions(+), 28 deletions(-)

Remove duplicity and simplify code flow by moving the rcu_read_unlock() above the condition and let the flow control exit naturally at the end of the function.

---
 net/netfilter/nfnetlink.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

index e8138da..0df800a 100644
@@ -407,12 +407,9 @@ static void nfnetlink_bind(int group)
 
 	rcu_read_lock();
 	ss = nfnetlink_get_subsys(type);
-	if (!ss) {
-		rcu_read_unlock();
-		request_module("nfnetlink-subsys-%d", type);
-		return;
-	}
 	rcu_read_unlock();
+	if (!ss)
+		request_module("nfnetlink-subsys-%d", type);
 }
 #endif
 

1.7.1

Have the netlink per-protocol optional bind function return an int error code rather than void to signal a failure.

This will enable netlink protocols to perform extra checks including capabilities and permissions verifications when updating memberships in multicast groups.

In netlink_bind() and netlink_setsockopt() the call to the per-protocol bind function was moved above the multicast group update to prevent any access to the multicast socket groups before checking with the per-protocol bind function. This will enable the per-protocol bind function to be used to check permissions which could be denied before making them available, and to avoid the messy job of undoing the addition should the per-protocol bind function fail.

The netfilter subsystem seems to be the only one currently using the per-protocol bind function.

---
In particular, the audit subsystem (NETLINK_AUDIT protocol) could benefit by being able to check specific capabilities for each multicast group before granting membership to the requesting socket. Currently, all NETLINK_AUDIT sockets must have the capability CAP_NET_ADMIN. No other capabilities are required to join a multicast group. This capability is too broad allowing access to this socket by many applications that must not have access to this information. It is proposed to add capability CAP_AUDIT_READ to allow this access while dropping the exessively broad capability CAP_NET_ADMIN.

There has also been some interest expressed by IETF ForCES folk.

---
 include/linux/netlink.h   |    3 ++-
 net/netfilter/nfnetlink.c |    3 ++-
 net/netlink/af_netlink.c  |   43 ++++++++++++++++++++++++++++++-------------
 net/netlink/af_netlink.h  |    6 ++++--
 4 files changed, 38 insertions(+), 17 deletions(-)

index aad8eea..5146ce0 100644
@@ -45,7 +45,8 @@ struct netlink_kernel_cfg {
 	unsigned int	flags;
 	void		(*input)(struct sk_buff *skb);
 	struct mutex	*cb_mutex;
-	void		(*bind)(int group);
+	int		(*bind)(int group);
+	void		(*unbind)(int group);
 	bool		(*compare)(struct net *net, struct sock *sk);
 };
 

index 0df800a..6e42dcf 100644
@@ -400,7 +400,7 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 }
 
 #ifdef CONFIG_MODULES
-static void nfnetlink_bind(int group)
+static int nfnetlink_bind(int group)
 {
 	const struct nfnetlink_subsystem *ss;
 	int type = nfnl_group2type[group];

@@ -410,6 +410,7 @@ static void nfnetlink_bind(int group)
 	rcu_read_unlock();
 	if (!ss)
 		request_module("nfnetlink-subsys-%d", type);
+	return 0;
 }
 #endif
 

index ed13a79..0b59d44 100644
@@ -38,7 +38,8 @@ struct netlink_sock {
 	struct mutex		*cb_mutex;
 	struct mutex		cb_def_mutex;
 	void			(*netlink_rcv)(struct sk_buff *skb);
-	void			(*netlink_bind)(int group);
+	int			(*netlink_bind)(int group);
+	void			(*netlink_unbind)(int group);
 	struct module		*module;
 #ifdef CONFIG_NETLINK_MMAP
 	struct mutex		pg_vec_lock;

@@ -74,7 +75,8 @@ struct netlink_table {
 	unsigned int		groups;
 	struct mutex		*cb_mutex;
 	struct module		*module;
-	void			(*bind)(int group);
+	int			(*bind)(int group);
+	void			(*unbind)(int group);
 	bool			(*compare)(struct net *net, struct sock *sock);
 	int			registered;
 };

1.7.1

From: Richard Guy Briggs <[email protected]>
Date: Fri, 18 Apr 2014 13:34:06 -0400

The risk seems small, but I agree it is sloppy.

Ok, done. This revision ends up being a bit cleaner than the previous one because I've refactored out the unbind loop into netlink_unbind() due to it needing to be called if the netlink_insert/autobind() fails as well. In the factorization process I noticed that unrequested groups were being unnecessarily unbound. It also provoked assigning the var "groups" from "nladdr->nl_groups" making things easier to read.

Compiled and tested. New patchset to follow.

- RGB

--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

This is a patch set Eric Paris and I have been working on to add a restricted capability read-only netlink multicast socket to kernel audit to enable userspace clients such as systemd/journald to receive audit logs, in addition to the bidirectional auditd userspace client.

Currently, auditd has the CAP_AUDIT_CONTROL and CAP_AUDIT_WRITE capabilities (but uses CAP_NET_ADMIN). The CAP_AUDIT_READ capability will be added for use by read-only AUDIT_NLGRP_READLOG multicast group clients to the kaudit subsystem. This will remove the dependence on CAP_NET_ADMIN for the multicast read-only socket.

Patches 1-3 provide a way for per-protocol bind functions to signal an error and to be able to clean up after themselves.

The first netfilter cleanup patch has already been accepted by a netfilter maintainer, though I don't see it upstream yet, so it is included for completeness.

The second patch adds the per-protocol bind function return code to signal to the netlink code that no further processing should be done and to undo the work already done.
V1: This rev fixes a bug introduced by flattening the code in the last posting. *V2: This rev moves the per-protocol bind call above the socket exposure call and refactors out the unbind procedure.

The third provides a way per protocol to undo bind actions on DROP.

Patches 4-6 implement the audit multicast socket with capability checking.

The fourth patch adds the bind function capability check to multicast join requests for audit.

The fifth patch adds the audit log read multicast group. An assumption has been made that systemd/journald reside in the initial network namespace. This could be changed to check the actual network namespace of systemd/journald should this assumption no longer be true since audit now supports all network namespaces. This version of the patch now directly sends the broadcast when the packet is ready rather than waiting until it passes the queue.

The sixth checks if any clients actually exist before sending.

Since the net tree is busier than the audit tree, conflicts are more likely and the audit patches depend on the net patches, it is proposed to have the net tree carry this entire patchset for 3.16. Are the net maintainers ok with this?

https://bugzilla.redhat.com/show_bug.cgi?id=887992

First posted: https://www.redhat.com/archives/linux-audit/2013-January/msg00008.html

                https://lkml.org/lkml/2013/1/27/279

Please find source for a test program at:
http://people.redhat.com/rbriggs/audit-multicast-listen/

Richard Guy Briggs (6):
netlink: simplify nfnetlink_bind
netlink: have netlink per-protocol bind function return an error
code.
netlink: implement unbind to netlink_setsockopt
NETLINK_DROP_MEMBERSHIP
audit: add netlink audit protocol bind to check capabilities on
multicast join
audit: add netlink multicast group for log read
audit: send multicast messages only if there are listeners

 include/linux/netlink.h             |    3 +-
 include/uapi/linux/audit.h          |    8 ++++
 include/uapi/linux/capability.h     |    7 +++-
 kernel/audit.c                      |   64 ++++++++++++++++++++++++++++++--
 net/netfilter/nfnetlink.c           |   10 ++---
 net/netlink/af_netlink.c            |   70 +++++++++++++++++++++++++----------
 net/netlink/af_netlink.h            |    6 ++-
 security/selinux/include/classmap.h |    2 +-
 8 files changed, 135 insertions(+), 35 deletions(-)

From: Richard Guy Briggs <[email protected]>
Date: Tue, 22 Apr 2014 21:31:52 -0400

Series applied, thanks Richard.

Thanks for your patience, David. Can I assume you adopted the 3 audit patches too, becuase of the dependence?

- RGB

--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

From: Richard Guy Briggs <[email protected]>
Date: Tue, 22 Apr 2014 21:49:29 -0400

Yes.

Do have the ability to separate of secadm_r and sysadm_r? By allowing this, we will leak to a sysadmin that he is being audited by the security officer. In a lot of cases, they are one in the same person. But for others, they are not. I have a feeling this will cause problems for MLS systems.

Also, shouldn't we have an audit event for every attempt to connect to this socket? We really need to know where this information is getting leaked to.

-Steve

Why? This requires CAP_AUDIT_READ. Just don't give CAP_AUDIT_READ to places you don't want to have read permission. Exactly the same as you don't give CAP_AUDIT_CONTROL to sysadm_r. (If we are giving CAP_AUDIT_CONTROL to sysadm_r and you think that any file protections on /var/log/audit/audit.log are adequate we are fooling ourselves!)

We certainly can. What would you like to see in that event?

-Eric

Here are the capabilities we currently give to sysadm_t with
sysadm_secadm 1.0.0 Disabled

allow sysadm_t sysadm_t : capability { chown dac_override
dac_read_search fowner fsetid kill setgid setuid setpcap linux_immutable net_bind_service net_broadcast net_admin net_raw ipc_lock ipc_owner sys_rawio sys_chroot sys_ptrace sys_pacct sys_admin sys_boot sys_nice sys_resource sys_time sys_tty_config mknod lease audit_write setfcap } ;
allow sysadm_t sysadm_t : capability { setgid setuid sys_chroot }

allow sysadm_t sysadm_t : capability2 { syslog block_suspend } ;

cap_audit_write might be a problem?

cap_audit_write is fine.

syslogd_t (aka journal) is going to need the new permission cap_audit_read. Also, as steve pointed out, someone may be likely to want to be able to disable that permission easily.

-Eric

I guess the problem would be that the sysadm_t would be able to look at the journal which would now contain the audit content.

right. so include it in the sysadm_secadm bool

Meaning looking at the journal would be equivalent to looking at /var/log/audit/audit.log.

They would be equivalent if and only if journald had CAP_AUDIT_READ.

I suggest you take CAP_AUDIT_READ away from journald on systems which need the secadm/sysadmin split (which is a ridiculously stupid split anyway, but who am I to complain?)

I don't disagree. I would think the real solution to this would be to not allow sysadm_t to get to SystemHigh, where all of the logging data will be stored.

make journalctl a userspace object manager and do selinux checks on if it can see individual records? so secadm_t running journalctl would see them and sysadm running journalctl wouldn't see them?

Sounds elegant. Who is going to code it? *NOT IT!*

Yes that would be the long term fix. But it would involve journal labelling individual data records. IE Records from audit.log would be audit_log_t, while messages from syslog would be var_log_t, Or some other kind of crazyness.

Hello,

Removing people that probably could care less about an audit event...

I think it should be patterned after the other "standalone" kernel audit events. We need pid, sesion, uid, auid, subj, comm, exe, and results. The event type should be something like AUDIT_EVENT_LISTENER. I am wondering about the usefulness of also adding op=connect op=disconnect to bracket the times when something else was listening in on audit events.

-Steve

I assume that order of these is not yet important and that gid should also be in this list (which will let me use audit_log_task()).

- RGB

--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Remove duplicity and simplify code flow by moving the rcu_read_unlock() above the condition and let the flow control exit naturally at the end of the function.

---
 net/netfilter/nfnetlink.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

index e8138da..0df800a 100644
@@ -407,12 +407,9 @@ static void nfnetlink_bind(int group)
 
 	rcu_read_lock();
 	ss = nfnetlink_get_subsys(type);
-	if (!ss) {
-		rcu_read_unlock();
-		request_module("nfnetlink-subsys-%d", type);
-		return;
-	}
 	rcu_read_unlock();
+	if (!ss)
+		request_module("nfnetlink-subsys-%d", type);
 }
 #endif
 

1.7.1

Have the netlink per-protocol optional bind function return an int error code rather than void to signal a failure.

This will enable netlink protocols to perform extra checks including capabilities and permissions verifications when updating memberships in multicast groups.

In netlink_bind() and netlink_setsockopt() the call to the per-protocol bind function was moved above the multicast group update to prevent any access to the multicast socket groups before checking with the per-protocol bind function. This will enable the per-protocol bind function to be used to check permissions which could be denied before making them available, and to avoid the messy job of undoing the addition should the per-protocol bind function fail.

The netfilter subsystem seems to be the only one currently using the per-protocol bind function.

---
V1: This rev fixes a bug introduced by flattening the code in the last posting. *V2: This rev moves the per-protocol bind call above the socket exposure call and refactors out the unbind procedure.

In particular, the audit subsystem (NETLINK_AUDIT protocol) could benefit by being able to check specific capabilities for each multicast group before granting membership to the requesting socket. Currently, all NETLINK_AUDIT sockets must have the capability CAP_NET_ADMIN. No other capabilities are required to join a multicast group. This capability is too broad allowing access to this socket by many applications that must not have access to this information. It is proposed to add capability CAP_AUDIT_READ to allow this access while dropping the exessively broad capability CAP_NET_ADMIN.

There has also been some interest expressed by IETF ForCES folk.

---
 include/linux/netlink.h   |    3 +-
 net/netfilter/nfnetlink.c |    3 +-
 net/netlink/af_netlink.c  |   68 +++++++++++++++++++++++++++++++-------------
 net/netlink/af_netlink.h  |    6 ++-
 4 files changed, 56 insertions(+), 24 deletions(-)

index aad8eea..5146ce0 100644
@@ -45,7 +45,8 @@ struct netlink_kernel_cfg {
 	unsigned int	flags;
 	void		(*input)(struct sk_buff *skb);
 	struct mutex	*cb_mutex;
-	void		(*bind)(int group);
+	int		(*bind)(int group);
+	void		(*unbind)(int group);
 	bool		(*compare)(struct net *net, struct sock *sk);
 };
 

index 0df800a..6e42dcf 100644
@@ -400,7 +400,7 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 }
 
 #ifdef CONFIG_MODULES
-static void nfnetlink_bind(int group)
+static int nfnetlink_bind(int group)
 {
 	const struct nfnetlink_subsystem *ss;
 	int type = nfnl_group2type[group];

@@ -410,6 +410,7 @@ static void nfnetlink_bind(int group)
 	rcu_read_unlock();
 	if (!ss)
 		request_module("nfnetlink-subsys-%d", type);
+	return 0;
 }
 #endif
 

index ed13a79..0b59d44 100644
@@ -38,7 +38,8 @@ struct netlink_sock {
 	struct mutex		*cb_mutex;
 	struct mutex		cb_def_mutex;
 	void			(*netlink_rcv)(struct sk_buff *skb);
-	void			(*netlink_bind)(int group);
+	int			(*netlink_bind)(int group);
+	void			(*netlink_unbind)(int group);
 	struct module		*module;
 #ifdef CONFIG_NETLINK_MMAP
 	struct mutex		pg_vec_lock;

@@ -74,7 +75,8 @@ struct netlink_table {
 	unsigned int		groups;
 	struct mutex		*cb_mutex;
 	struct module		*module;
-	void			(*bind)(int group);
+	int			(*bind)(int group);
+	void			(*unbind)(int group);
 	bool			(*compare)(struct net *net, struct sock *sock);
 	int			registered;
 };

1.7.1

Call the per-protocol unbind function rather than bind function on NETLINK_DROP_MEMBERSHIP in netlink_setsockopt().

---
 net/netlink/af_netlink.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

index 7e8d229..92f4b69 100644
@@ -2129,7 +2129,7 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
-		if (nlk->netlink_bind) {
+		if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind) {
 			err = nlk->netlink_bind(val);
 			if (err)
 				return err;

@@ -2138,6 +2138,8 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
+		if (optname == NETLINK_DROP_MEMBERSHIP && nlk->netlink_unbind)
+			nlk->netlink_unbind(val);
 
 		err = 0;
 		break;

1.7.1

Register a netlink per-protocol bind fuction for audit to check userspace process capabilities before allowing a multicast group connection.

---
 include/uapi/linux/capability.h     |    7 ++++++-
 kernel/audit.c                      |   10 ++++++++++
 security/selinux/include/classmap.h |    2 +-
 3 files changed, 17 insertions(+), 2 deletions(-)

index 154dd6d..12c37a1 100644
@@ -347,7 +347,12 @@ struct vfs_cap_data {
 
 #define CAP_BLOCK_SUSPEND    36
 
-#define CAP_LAST_CAP         CAP_BLOCK_SUSPEND
+/* Allow reading the audit log via multicast netlink socket */
+
+#define CAP_AUDIT_READ		37
+
+
+#define CAP_LAST_CAP         CAP_AUDIT_READ
 
 #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
 

index 7c28936..223cb74 100644
@@ -1076,10 +1076,20 @@ static void audit_receive(struct sk_buff  *skb)
 	mutex_unlock(&audit_cmd_mutex);
 }
 
+/* Run custom bind function on netlink socket group connect or bind requests. */
+static int audit_bind(int group)
+{
+	if (!capable(CAP_AUDIT_READ))
+		return -EPERM;
+
+	return 0;
+}
+
 static int __net_init audit_net_init(struct net *net)
 {
 	struct netlink_kernel_cfg cfg = {
 		.input	= audit_receive,
+		.bind	= audit_bind,
 	};
 
 	struct audit_net *aunet = net_generic(net, audit_net_id);

index 14d04e6..be491a7 100644
@@ -147,7 +147,7 @@ struct security_class_mapping secclass_map[] = {
 	{ "peer", { "recv", NULL } },
 	{ "capability2",
 	  { "mac_override", "mac_admin", "syslog", "wake_alarm", "block_suspend",
-	    NULL } },
+	    "audit_read", NULL } },
 	{ "kernel_service", { "use_as_override", "create_files_as", NULL } },
 	{ "tun_socket",
 	  { COMMON_SOCK_PERMS, "attach_queue", NULL } },

1.7.1

Add a netlink multicast socket with one group to kaudit for "best-effort" delivery to read-only userspace clients such as systemd, in addition to the existing bidirectional unicast auditd userspace client.

Currently, auditd is intended to use the CAP_AUDIT_CONTROL and CAP_AUDIT_WRITE capabilities, but actually uses CAP_NET_ADMIN. The CAP_AUDIT_READ capability is added for use by read-only AUDIT_NLGRP_READLOG netlink multicast group clients to the kaudit subsystem.

This will safely give access to services such as systemd to consume audit logs while ensuring write access remains restricted for integrity.

---
 include/uapi/linux/audit.h |    8 +++++++
 kernel/audit.c             |   51 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 55 insertions(+), 4 deletions(-)

index 11917f7..dfa4c86 100644
@@ -373,6 +373,14 @@ enum {
  */
 #define AUDIT_MESSAGE_TEXT_MAX	8560
 
+/* Multicast Netlink socket groups (default up to 32) */
+enum audit_nlgrps {
+	AUDIT_NLGRP_NONE,	/* Group 0 not used */
+	AUDIT_NLGRP_READLOG,	/* "best effort" read only socket */
+	__AUDIT_NLGRP_MAX
+};
+#define AUDIT_NLGRP_MAX                (__AUDIT_NLGRP_MAX - 1)
+
 struct audit_status {
 	__u32		mask;		/* Bit mask for valid entries */
 	__u32		enabled;	/* 1 = enabled, 0 = disabled */

1.7.1

Test first to see if there are any userspace multicast listeners bound to the socket before starting the multicast send work.

---
 kernel/audit.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

index d272cc1..33531d7 100644
@@ -435,6 +435,9 @@ static void kauditd_send_multicast_skb(struct sk_buff *skb)
 	struct audit_net	*aunet = net_generic(&init_net, audit_net_id);
 	struct sock		*sock = aunet->nlsk;
 
+	if (!netlink_has_listeners(sock, AUDIT_NLGRP_READLOG))
+		return;
+
 	/*
 	 * The seemingly wasteful skb_copy() rather than bumping the refcount
 	 * using skb_get() is necessary because non-standard mods are made to

1.7.1

Call the per-protocol unbind function rather than bind function on NETLINK_DROP_MEMBERSHIP in netlink_setsockopt().

---
 net/netlink/af_netlink.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

index 3f43e5a..cdb236f 100644
@@ -2118,7 +2118,7 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
-		if (nlk->netlink_bind) {
+		if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind) {
 			err = nlk->netlink_bind(val);
 			if (err)
 				return err;

@@ -2127,6 +2127,8 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
+		if (optname == NETLINK_DROP_MEMBERSHIP && nlk->netlink_unbind)
+			nlk->netlink_unbind(val);
 
 		err = 0;
 		break;

1.7.1

Register a netlink per-protocol bind fuction for audit to check userspace process capabilities before allowing a multicast group connection.

---
 include/uapi/linux/capability.h     |    7 ++++++-
 kernel/audit.c                      |   10 ++++++++++
 security/selinux/include/classmap.h |    2 +-
 3 files changed, 17 insertions(+), 2 deletions(-)

index 154dd6d..12c37a1 100644
@@ -347,7 +347,12 @@ struct vfs_cap_data {
 
 #define CAP_BLOCK_SUSPEND    36
 
-#define CAP_LAST_CAP         CAP_BLOCK_SUSPEND
+/* Allow reading the audit log via multicast netlink socket */
+
+#define CAP_AUDIT_READ		37
+
+
+#define CAP_LAST_CAP         CAP_AUDIT_READ
 
 #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
 

index 7c28936..223cb74 100644
@@ -1076,10 +1076,20 @@ static void audit_receive(struct sk_buff  *skb)
 	mutex_unlock(&audit_cmd_mutex);
 }
 
+/* Run custom bind function on netlink socket group connect or bind requests. */
+static int audit_bind(int group)
+{
+	if (!capable(CAP_AUDIT_READ))
+		return -EPERM;
+
+	return 0;
+}
+
 static int __net_init audit_net_init(struct net *net)
 {
 	struct netlink_kernel_cfg cfg = {
 		.input	= audit_receive,
+		.bind	= audit_bind,
 	};
 
 	struct audit_net *aunet = net_generic(net, audit_net_id);

index 14d04e6..be491a7 100644
@@ -147,7 +147,7 @@ struct security_class_mapping secclass_map[] = {
 	{ "peer", { "recv", NULL } },
 	{ "capability2",
 	  { "mac_override", "mac_admin", "syslog", "wake_alarm", "block_suspend",
-	    NULL } },
+	    "audit_read", NULL } },
 	{ "kernel_service", { "use_as_override", "create_files_as", NULL } },
 	{ "tun_socket",
 	  { COMMON_SOCK_PERMS, "attach_queue", NULL } },

1.7.1

Add a netlink multicast socket with one group to kaudit for "best-effort" delivery to read-only userspace clients such as systemd, in addition to the existing bidirectional unicast auditd userspace client.

Currently, auditd is intended to use the CAP_AUDIT_CONTROL and CAP_AUDIT_WRITE capabilities, but actually uses CAP_NET_ADMIN. The CAP_AUDIT_READ capability is added for use by read-only AUDIT_NLGRP_READLOG netlink multicast group clients to the kaudit subsystem.

This will safely give access to services such as systemd to consume audit logs while ensuring write access remains restricted for integrity.

---
 include/uapi/linux/audit.h |    8 +++++++
 kernel/audit.c             |   51 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 55 insertions(+), 4 deletions(-)

index 11917f7..dfa4c86 100644
@@ -373,6 +373,14 @@ enum {
  */
 #define AUDIT_MESSAGE_TEXT_MAX	8560
 
+/* Multicast Netlink socket groups (default up to 32) */
+enum audit_nlgrps {
+	AUDIT_NLGRP_NONE,	/* Group 0 not used */
+	AUDIT_NLGRP_READLOG,	/* "best effort" read only socket */
+	__AUDIT_NLGRP_MAX
+};
+#define AUDIT_NLGRP_MAX                (__AUDIT_NLGRP_MAX - 1)
+
 struct audit_status {
 	__u32		mask;		/* Bit mask for valid entries */
 	__u32		enabled;	/* 1 = enabled, 0 = disabled */

1.7.1

Test first to see if there are any userspace multicast listeners bound to the socket before starting the multicast send work.

---
 kernel/audit.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

index d272cc1..33531d7 100644
@@ -435,6 +435,9 @@ static void kauditd_send_multicast_skb(struct sk_buff *skb)
 	struct audit_net	*aunet = net_generic(&init_net, audit_net_id);
 	struct sock		*sock = aunet->nlsk;
 
+	if (!netlink_has_listeners(sock, AUDIT_NLGRP_READLOG))
+		return;
+
 	/*
 	 * The seemingly wasteful skb_copy() rather than bumping the refcount
 	 * using skb_get() is necessary because non-standard mods are made to

1.7.1