[PATCH 0/6] audit: add restricted capability read-only netlink multicast socket

[PATCH 0/6] audit: add restricted capability read-only netlink multicast socket

[Richard Guy Briggs]

Hi,

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

Currently, auditd has the CAP_AUDIT_CONTROL and CAP_AUDIT_WRITE capabilities
(bot 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.

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

Feedback please!

Richard Guy Briggs (6):
  audit: refactor hold queue flush
  audit: flatten kauditd_thread wait queue code
  audit: move kaudit thread start from auditd registration to kaudit
    init
  netlink: add send and receive capability requirement and capability
    flags
  audit: add the first netlink multicast socket group
  audit: send multicast messages only if there are listeners

 include/linux/netlink.h             |   4 +
 include/uapi/linux/audit.h          |   8 ++
 include/uapi/linux/capability.h     |   5 +-
 kernel/audit.c                      | 142 +++++++++++++++++++++++++-----------
 net/netlink/af_netlink.c            |  35 +++++++--
 security/selinux/include/classmap.h |   2 +-
 6 files changed, 144 insertions(+), 52 deletions(-)

-- 
1.8.0.2

[PATCH 1/6] audit: refactor hold queue flush

[Richard Guy Briggs]

The hold queue flush code is an autonomous chunk of code that can be
refactored, removed from kauditd_thread() into flush_hold_queue() and
flattenned for better legibility.

Signed-off-by: Richard Guy Briggs <rbriggs@redhat.com>
---

This is a code clean up in preparation to add a multicast netlink socket to
kaudit for read-only userspace clients such as systemd, in addition to the
bidirectional audit userspace client.

 kernel/audit.c | 62 +++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 22 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index d596e53..4bf486c 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -417,34 +417,52 @@ static void kauditd_send_skb(struct sk_buff *skb)
 		consume_skb(skb);
 }
 
+/*
+ * flush_hold_queue - empty the hold queue if auditd appears
+ *
+ * If auditd just started, drain the queue of messages already
+ * sent to syslog/printk.  Remember loss here is ok.  We already
+ * called audit_log_lost() if it didn't go out normally.  so the
+ * race between the skb_dequeue and the next check for audit_pid
+ * doesn't matter.
+ *
+ * If you ever find kauditd to be too slow we can get a perf win
+ * by doing our own locking and keeping better track if there
+ * are messages in this queue.  I don't see the need now, but
+ * in 5 years when I want to play with this again I'll see this
+ * note and still have no friggin idea what i'm thinking today.
+ */
+static void flush_hold_queue(void)
+{
+	struct sk_buff *skb;
+
+	if (!audit_default || !audit_pid)
+		return;
+
+	skb = skb_dequeue(&audit_skb_hold_queue);
+	if (likely(!skb))
+		return;
+
+	while (skb && audit_pid) {
+		kauditd_send_skb(skb);
+		skb = skb_dequeue(&audit_skb_hold_queue);
+	}
+
+	/*
+	 * if auditd just disappeared but we
+	 * dequeued an skb we need to drop ref
+	 */
+	if (skb)
+		consume_skb(skb);
+}
+
 static int kauditd_thread(void *dummy)
 {
 	struct sk_buff *skb;
 
 	set_freezable();
 	while (!kthread_should_stop()) {
-		/*
-		 * if auditd just started drain the queue of messages already
-		 * sent to syslog/printk.  remember loss here is ok.  we already
-		 * called audit_log_lost() if it didn't go out normally.  so the
-		 * race between the skb_dequeue and the next check for audit_pid
-		 * doesn't matter.
-		 *
-		 * if you ever find kauditd to be too slow we can get a perf win
-		 * by doing our own locking and keeping better track if there
-		 * are messages in this queue.  I don't see the need now, but
-		 * in 5 years when I want to play with this again I'll see this
-		 * note and still have no friggin idea what i'm thinking today.
-		 */
-		if (audit_default && audit_pid) {
-			skb = skb_dequeue(&audit_skb_hold_queue);
-			if (unlikely(skb)) {
-				while (skb && audit_pid) {
-					kauditd_send_skb(skb);
-					skb = skb_dequeue(&audit_skb_hold_queue);
-				}
-			}
-		}
+		flush_hold_queue();
 
 		skb = skb_dequeue(&audit_skb_queue);
 		wake_up(&audit_backlog_wait);
-- 
1.8.0.2

[PATCH 2/6] audit: flatten kauditd_thread wait queue code

[Richard Guy Briggs]

The wait queue control code in kauditd_thread() was nested deeper than
necessary.  The function has been flattened for better legibility.

Signed-off-by: Richard Guy Briggs <rbriggs@redhat.com>
---

This is a code clean up in preparation to add a multicast netlink socket to
kaudit for read-only userspace clients such as systemd, in addition to the
bidirectional audit userspace client.

 kernel/audit.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 4bf486c..1531efb 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -458,10 +458,11 @@ static void flush_hold_queue(void)
 
 static int kauditd_thread(void *dummy)
 {
-	struct sk_buff *skb;
-
 	set_freezable();
 	while (!kthread_should_stop()) {
+		struct sk_buff *skb;
+		DECLARE_WAITQUEUE(wait, current);
+
 		flush_hold_queue();
 
 		skb = skb_dequeue(&audit_skb_queue);
@@ -471,19 +472,18 @@ static int kauditd_thread(void *dummy)
 				kauditd_send_skb(skb);
 			else
 				audit_printk_skb(skb);
-		} else {
-			DECLARE_WAITQUEUE(wait, current);
-			set_current_state(TASK_INTERRUPTIBLE);
-			add_wait_queue(&kauditd_wait, &wait);
-
-			if (!skb_queue_len(&audit_skb_queue)) {
-				try_to_freeze();
-				schedule();
-			}
+			continue;
+		}
+		set_current_state(TASK_INTERRUPTIBLE);
+		add_wait_queue(&kauditd_wait, &wait);
 
-			__set_current_state(TASK_RUNNING);
-			remove_wait_queue(&kauditd_wait, &wait);
+		if (!skb_queue_len(&audit_skb_queue)) {
+			try_to_freeze();
+			schedule();
 		}
+
+		__set_current_state(TASK_RUNNING);
+		remove_wait_queue(&kauditd_wait, &wait);
 	}
 	return 0;
 }
-- 
1.8.0.2

[PATCH 3/6] audit: move kaudit thread start from auditd registration to kaudit init

[Richard Guy Briggs]

The kauditd_thread() task was started only after the auditd userspace daemon
registers itself with kaudit.  This was fine when only auditd consumed messages
from the kaudit netlink unicast socket.  With the addition of a multicast group
to that socket it is more convenient to have the thread start on init of the
kaudit kernel subsystem.

Signed-off-by: Richard Guy Briggs <rbriggs@redhat.com>
---

This is a code clean up in preparation to add a multicast netlink socket to
kaudit for read-only userspace clients such as systemd, in addition to the
bidirectional audit userspace client.

 kernel/audit.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 1531efb..02a5d9e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -676,16 +676,6 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 	if (err)
 		return err;
 
-	/* As soon as there's any sign of userspace auditd,
-	 * start kauditd to talk to it */
-	if (!kauditd_task)
-		kauditd_task = kthread_run(kauditd_thread, NULL, "kauditd");
-	if (IS_ERR(kauditd_task)) {
-		err = PTR_ERR(kauditd_task);
-		kauditd_task = NULL;
-		return err;
-	}
-
 	loginuid = audit_get_loginuid(current);
 	sessionid = audit_get_sessionid(current);
 	security_task_getsecid(current, &sid);
@@ -974,6 +964,10 @@ static int __init audit_init(void)
 	else
 		audit_sock->sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
 
+	kauditd_task = kthread_run(kauditd_thread, NULL, "kauditd");
+	if (IS_ERR(kauditd_task))
+		return PTR_ERR(kauditd_task);
+
 	skb_queue_head_init(&audit_skb_queue);
 	skb_queue_head_init(&audit_skb_hold_queue);
 	audit_initialized = AUDIT_INITIALIZED;
-- 
1.8.0.2

[PATCH 4/6] netlink: add send and receive capability requirement and capability flags

[Richard Guy Briggs]

Currently netlink socket permissions are controlled by the
NL_CFG_F_NONROOT_{RECV,SEND} flags in the kernel socket configuration or by the
CAP_NET_ADMIN capability of the client.  The former allows non-root users
access to the socket.  The latter allows all network admin clients access to
the socket.  It would be useful to be able to further restrict this access to
send or receive capabilities individually within specific subsystems with a
more targetted capability.  Two more flags, NL_CFG_F_CAPABILITY_{RECV,SEND},
have been added to specifically require a named capability should the subsystem
request it, allowing the client to drop other broad unneeded capabilities.

Signed-off-by: Richard Guy Briggs <rbriggs@redhat.com>
---

This is a feature addition in preparation to add a multicast netlink socket to
kaudit for read-only userspace clients such as systemd, in addition to the
bidirectional unicast auditd userspace client.

Currently, auditd has the CAP_AUDIT_CONTROL and CAP_AUDIT_WRITE capabilities
(bot 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.

 include/linux/netlink.h  |  4 ++++
 net/netlink/af_netlink.c | 35 +++++++++++++++++++++++++++++------
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index e0f746b..30daf11 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -31,6 +31,8 @@ extern void netlink_table_ungrab(void);
 
 #define NL_CFG_F_NONROOT_RECV	(1 << 0)
 #define NL_CFG_F_NONROOT_SEND	(1 << 1)
+#define NL_CFG_F_CAPABILITY_RECV (1 << 2)
+#define NL_CFG_F_CAPABILITY_SEND (1 << 3)
 
 /* optional Netlink kernel configuration parameters */
 struct netlink_kernel_cfg {
@@ -39,6 +41,8 @@ struct netlink_kernel_cfg {
 	void		(*input)(struct sk_buff *skb);
 	struct mutex	*cb_mutex;
 	void		(*bind)(int group);
+	int		cap_send_requires;
+	int		cap_recv_requires;
 };
 
 extern struct sock *__netlink_kernel_create(struct net *net, int unit,
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index c0353d5..cce1fe3 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -127,6 +127,8 @@ struct netlink_table {
 	struct module		*module;
 	void			(*bind)(int group);
 	int			registered;
+	int			cap_send_requires;
+	int			cap_recv_requires;
 };
 
 static struct netlink_table *nl_table;
@@ -552,6 +554,8 @@ static int netlink_release(struct socket *sock)
 			nl_table[sk->sk_protocol].bind = NULL;
 			nl_table[sk->sk_protocol].flags = 0;
 			nl_table[sk->sk_protocol].registered = 0;
+			nl_table[sk->sk_protocol].cap_send_requires = 0;
+			nl_table[sk->sk_protocol].cap_recv_requires = 0;
 		}
 	} else if (nlk->subscriptions) {
 		netlink_update_listeners(sk);
@@ -611,8 +615,20 @@ retry:
 
 static inline int netlink_capable(const struct socket *sock, unsigned int flag)
 {
-	return (nl_table[sock->sk->sk_protocol].flags & flag) ||
-		ns_capable(sock_net(sock->sk)->user_ns, CAP_NET_ADMIN);
+	struct netlink_table *nlt = &nl_table[sock->sk->sk_protocol];
+
+	switch (flag & nlt->flags) {
+	case NL_CFG_F_NONROOT_RECV:
+	case NL_CFG_F_NONROOT_SEND:
+		return true;
+	case NL_CFG_F_CAPABILITY_RECV:
+		return capable(nlt->cap_recv_requires);
+	case NL_CFG_F_CAPABILITY_SEND:
+		return capable(nlt->cap_send_requires);
+	default:
+		return capable(CAP_NET_ADMIN);
+	}
+	return false;
 }
 
 static void
@@ -677,7 +693,8 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 
 	/* Only superuser is allowed to listen multicasts */
 	if (nladdr->nl_groups) {
-		if (!netlink_capable(sock, NL_CFG_F_NONROOT_RECV))
+		if (!netlink_capable(sock, NL_CFG_F_NONROOT_RECV |
+					   NL_CFG_F_CAPABILITY_RECV))
 			return -EPERM;
 		err = netlink_realloc_groups(sk);
 		if (err)
@@ -739,7 +756,9 @@ static int netlink_connect(struct socket *sock, struct sockaddr *addr,
 		return -EINVAL;
 
 	/* Only superuser is allowed to send multicasts */
-	if (nladdr->nl_groups && !netlink_capable(sock, NL_CFG_F_NONROOT_SEND))
+	if (nladdr->nl_groups &&
+	    !netlink_capable(sock, NL_CFG_F_NONROOT_SEND |
+				   NL_CFG_F_CAPABILITY_SEND))
 		return -EPERM;
 
 	if (!nlk->portid)
@@ -1262,7 +1281,8 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 		break;
 	case NETLINK_ADD_MEMBERSHIP:
 	case NETLINK_DROP_MEMBERSHIP: {
-		if (!netlink_capable(sock, NL_CFG_F_NONROOT_RECV))
+		if (!netlink_capable(sock, NL_CFG_F_NONROOT_RECV |
+					   NL_CFG_F_CAPABILITY_RECV))
 			return -EPERM;
 		err = netlink_realloc_groups(sk);
 		if (err)
@@ -1394,7 +1414,8 @@ static int netlink_sendmsg(struct kiocb *kiocb, struct socket *sock,
 		dst_group = ffs(addr->nl_groups);
 		err =  -EPERM;
 		if ((dst_group || dst_portid) &&
-		    !netlink_capable(sock, NL_CFG_F_NONROOT_SEND))
+		    !netlink_capable(sock, NL_CFG_F_NONROOT_SEND |
+					   NL_CFG_F_CAPABILITY_SEND))
 			goto out;
 	} else {
 		dst_portid = nlk->dst_portid;
@@ -1600,6 +1621,8 @@ __netlink_kernel_create(struct net *net, int unit, struct module *module,
 		if (cfg) {
 			nl_table[unit].bind = cfg->bind;
 			nl_table[unit].flags = cfg->flags;
+			nl_table[unit].cap_send_requires = cfg->cap_send_requires;
+			nl_table[unit].cap_recv_requires = cfg->cap_recv_requires;
 		}
 		nl_table[unit].registered = 1;
 	} else {
-- 
1.8.0.2

[PATCH 5/6] audit: add restricted capability read-only netlink multicast socket

[Richard Guy Briggs]

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.

Signed-off-by: Richard Guy Briggs <rbriggs@redhat.com>
---

(The seemingly wasteful skb_copy() is necessary because the original kaudit
unicast socket sends up messages with nlmsg_len set to the payload length
rather than the entire message length.  This breaks the convention used by
netlink.  The existing auditd daemon assumes this breakage.  Fixing this would
require co-ordinating a change in the established protocol between kaudit
kernel code and auditd userspace code.  There is no reason for new multicast
clients to continue with this breakage.)

 include/uapi/linux/audit.h          |  8 ++++++++
 include/uapi/linux/capability.h     |  5 ++++-
 kernel/audit.c                      | 40 +++++++++++++++++++++++++++++++++++++
 security/selinux/include/classmap.h |  2 +-
 4 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 9f096f1..6296e5d9 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -357,6 +357,14 @@ enum {
 #define AUDIT_PERM_READ		4
 #define AUDIT_PERM_ATTR		8
 
+/* 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 */
diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
index ba478fa..f579a06 100644
--- a/include/uapi/linux/capability.h
+++ b/include/uapi/linux/capability.h
@@ -343,7 +343,10 @@ struct vfs_cap_data {
 
 #define CAP_BLOCK_SUSPEND    36
 
-#define CAP_LAST_CAP         CAP_BLOCK_SUSPEND
+/* Allowed to read the audit log */
+#define CAP_AUDIT_READ		37
+
+#define CAP_LAST_CAP         CAP_AUDIT_READ
 
 #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
 
diff --git a/kernel/audit.c b/kernel/audit.c
index 02a5d9e..9eef05b 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -418,6 +418,37 @@ static void kauditd_send_skb(struct sk_buff *skb)
 }
 
 /*
+ * kauditd_send_multicast_skb - send the skb to multicast userspace listeners
+ *
+ * This function doesn't consume an skb as might be expected since it has to
+ * copy it anyways.
+ */
+static void kauditd_send_multicast_skb(struct sk_buff *skb)
+{
+	struct sk_buff *copy;
+	struct nlmsghdr *nlh;
+
+	/*
+	 * The seemingly wasteful skb_copy() is necessary because the original
+	 * kaudit unicast socket sends up messages with nlmsg_len set to the
+	 * payload length rather than the entire message length.  This breaks
+	 * the standard set by netlink.  The existing auditd daemon assumes
+	 * this breakage.  Fixing this would require co-ordinating a change in
+	 * the established protocol between the kaudit kernel subsystem and
+	 * the auditd userspace code.  There is no reason for new multicast
+	 * clients to continue with this non-compliance.
+	 */
+	copy = skb_copy(skb, GFP_KERNEL);
+	if (!copy)
+		return;
+
+	nlh = nlmsg_hdr(copy);
+	nlh->nlmsg_len = copy->len;
+
+	nlmsg_multicast(audit_sock, copy, 0, AUDIT_NLGRP_READLOG, GFP_KERNEL);
+}
+
+/*
  * flush_hold_queue - empty the hold queue if auditd appears
  *
  * If auditd just started, drain the queue of messages already
@@ -468,6 +499,12 @@ static int kauditd_thread(void *dummy)
 		skb = skb_dequeue(&audit_skb_queue);
 		wake_up(&audit_backlog_wait);
 		if (skb) {
+			/* Don't bump skb refcount for multicast send since
+			 * kauditd_send_multicast_skb() copies the skb anyway
+			 * due to audit unicast netlink protocol
+			 * non-compliance.
+			 */
+			kauditd_send_multicast_skb(skb);
 			if (audit_pid)
 				kauditd_send_skb(skb);
 			else
@@ -951,6 +988,9 @@ static int __init audit_init(void)
 	int i;
 	struct netlink_kernel_cfg cfg = {
 		.input	= audit_receive,
+		.groups	= AUDIT_NLGRP_MAX,
+		.flags = NL_CFG_F_CAPABILITY_RECV,
+		.cap_recv_requires = CAP_AUDIT_READ,
 	};
 
 	if (audit_initialized == AUDIT_DISABLED)
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index df2de54..c0bac6f 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -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, NULL } },
-- 
1.8.0.2

[PATCH 6/6] audit: send multicast messages only if there are listeners

[Richard Guy Briggs]

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

Signed-off-by: Richard Guy Briggs <rbriggs@redhat.com>
---
 kernel/audit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/audit.c b/kernel/audit.c
index 9eef05b..d153a6b 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -428,6 +428,8 @@ static void kauditd_send_multicast_skb(struct sk_buff *skb)
 	struct sk_buff *copy;
 	struct nlmsghdr *nlh;
 
+	if (!netlink_has_listeners(audit_sock, AUDIT_NLGRP_READLOG))
+		return;
 	/*
 	 * The seemingly wasteful skb_copy() is necessary because the original
 	 * kaudit unicast socket sends up messages with nlmsg_len set to the
-- 
1.8.0.2

Re: [PATCH] netlink: have netlink per-protocol bind function return an error code.

[Richard Guy Briggs]

On 14/03/24, Richard Guy Briggs wrote:
> On 14/03/23, David Miller wrote:
> > From: Richard Guy Briggs <rgb@redhat.com>
> > Date: Fri, 21 Mar 2014 12:39:11 -0400
> > 
> > > @@ -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;
> > > +			}
> > > +	}
> > > +
> > 
> > You can't just leave a partially set of completed bindings in place.
> 
> In the general case, I agree.
> 
> > It's not valid to leave half-baked state like this.
> 
> 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.

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

> 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.
> 
> > If you return an error, all of the binding state changes must be
> > completely undone.
> 
> Is it time to add a ".unbind = netlink_unbind" to struct proto_ops
> netlink_ops?  (I am only half serious here...)

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?

> > If you can't find a way to do this cleanly, you'll need to find
> > a way for the audit code to not return an error.
> 
> 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

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
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

[PATCH 0/6] audit: implement multicast socket for journald

[Richard Guy Briggs]

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(-)

[PATCH 6/6] audit: send multicast messages only if there are listeners

[Richard Guy Briggs]

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

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/audit.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index d272cc1..33531d7 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -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