Audit vs netlink interaction problem

DISCUSSION Pavel Emelyanov 3 participants 8 messages Mar 14, 2008 – Mar 17, 2008

Subsystems (3)

Networking Network drivers Audit

lore

Share & tools

lore

Pavel Emelyanov

author

2008-03-14 16:23 UTC #0 lore raw

Hi, David!

I've found an interesting feature of how audit uses netlink for communications. Look.

The kauditd_thread() calls netlink_unicast() and passes the audit_pid to it. The audit_pid, in turn, is received from the user space and the tool (I've checked the audit v1.6.9) uses getpid() to pass one in the kernel. Besides, this tool doesn't bind the netlink socket to this id, but simply creates it allowing the kernel to auto-bind one.

That's the preamble.

The problem is that netlink_autobind() _does_not_ guarantees that the socket will be auto-binded to the current pid. Instead it uses the current pid as a hint to start looking for a free id. So, in case of conflict, the audit messages can be sent to a wrong socket. This can happen (it's unlikely, but can be) in case some task opens more than one netlink sockets and then the audit one starts - in this case the audit's pid can be busy and its socket will be bound to another id.

The reason I raised this problem is that I'm now working on pid and network namespaces and found, that this problem doesn't allow to resolve the following issue gracefully.

The task_struct->tgid field, which is currently used in netlink for auto-binding, is going to be deprecated. Thus I have to make netlink auto-binding play another rules when selecting a pid, but this will break the audit work for sure, since it implicitly relies, that the netlink socket will be bound to the current task pid.

What do you think about it?

Thanks,
Pavel

Thomas Graf

2008-03-14 16:39 UTC #1 lore raw

* Pavel Emelyanov <[email protected]> 2008-03-14 19:22

Quoted 12 lines

I've found an interesting feature of how audit uses netlink for communications. Look.
The kauditd_thread() calls netlink_unicast() and passes the audit_pid to it. The audit_pid, in turn, is received from the user space and the tool (I've checked the audit v1.6.9) uses getpid() to pass one in the kernel. Besides, this tool doesn't bind the netlink socket to this id, but simply creates it allowing the kernel to auto-bind one.
That's the preamble.

Quoted 8 lines

The problem is that netlink_autobind() _does_not_ guarantees that the socket will be auto-binded to the current pid. Instead it uses the current pid as a hint to start looking for a free id. So, in case of conflict, the audit messages can be sent to a wrong socket. This can happen (it's unlikely, but can be) in case some task opens more than one netlink sockets and then the audit one starts - in this case the audit's pid can be busy and its socket will be bound to another id.

The audit userspace tool should be fixed, no question. It can continue to auto bind but must report the correct netlink pid.

As a workaround: Assuming that the audit daemon is the only application to issue a AUDIT_SET command to set the status pid, the kernel can compare the netlink source pid of the AUDIT_SET message and compare it against the status pid provided. If they differ, issue a warning but use the netlink source pid thus covering the case where the auto bound netlink pid actually differs from the process pid.

Pavel Emelyanov

author

2008-03-14 17:06 UTC #2 lore raw

Quoted 22 lines · Thomas Graf

* Pavel Emelyanov <[email protected]> 2008-03-14 19:22
Quoted 12 lines
I've found an interesting feature of how audit uses netlink for communications. Look.
The kauditd_thread() calls netlink_unicast() and passes the audit_pid to it. The audit_pid, in turn, is received from the user space and the tool (I've checked the audit v1.6.9) uses getpid() to pass one in the kernel. Besides, this tool doesn't bind the netlink socket to this id, but simply creates it allowing the kernel to auto-bind one.
That's the preamble.
Quoted 8 lines
The problem is that netlink_autobind() _does_not_ guarantees that the socket will be auto-binded to the current pid. Instead it uses the current pid as a hint to start looking for a free id. So, in case of conflict, the audit messages can be sent to a wrong socket. This can happen (it's unlikely, but can be) in case some task opens more than one netlink sockets and then the audit one starts - in this case the audit's pid can be busy and its socket will be bound to another id.

Quoted 1 line

The audit userspace tool should be fixed, no question. It can continue

Oh, this is good.
I was afraid, that we'd have to stick to this logic...

Quoted 1 line

to auto bind but must report the correct netlink pid.

Hmmm... I'm afraid, that this can break the audit filtering and signal auditing. I haven't yet looked deep into it, but it compares the task->tgid with this audit_pid for different purposes. If audit_pid changes this code will be broken.

That's why I asked David for comments.

Quoted 3 lines

As a workaround: Assuming that the audit daemon is the only application to issue a AUDIT_SET command to set the status pid, the kernel can compare the netlink source pid of the AUDIT_SET message and compare it

Bu we have no the netlink socket at the moment of setting the pid to check this. The audit_reveive_msg() call which does this set is received via another (pre-created global) socket.

Quoted 3 lines

against the status pid provided. If they differ, issue a warning but use the netlink source pid thus covering the case where the auto bound netlink pid actually differs from the process pid.

I though, that proper behavior would be to split audit_pid, used for filtering from the audit_nlk_pid used for netlink communications.

Thomas Graf

2008-03-14 18:29 UTC #3 lore raw

* Pavel Emelyanov <[email protected]> 2008-03-14 20:05

Quoted 4 lines

Hmmm... I'm afraid, that this can break the audit filtering and signal auditing. I haven't yet looked deep into it, but it compares the task->tgid with this audit_pid for different purposes. If audit_pid changes this code will be broken.

OK, then both pids have to be stored. audit_pid remains as-is but is no longer used as destination netlink pid. A second pid is stored and updated whenever a netlink message is received from userspace.

Quoted 3 lines

Bu we have no the netlink socket at the moment of setting the pid to check this. The audit_reveive_msg() call which does this set is received via another (pre-created global) socket.

I don't understand this. As far as I can read the code, a plain kernel side netlink socket is created in audit_init(). But it doesn't matter, as soon as we receive the first message from userspace, we know the netlink source pid.

Quoted 2 lines

I though, that proper behavior would be to split audit_pid, used for filtering from the audit_nlk_pid used for netlink communications.

Yes, exactly.

Thomas Graf

2008-03-14 18:40 UTC #4 lore raw

* Thomas Graf <[email protected]> 2008-03-14 19:29

Quoted 9 lines

* Pavel Emelyanov <[email protected]> 2008-03-14 20:05
Quoted 4 lines
Hmmm... I'm afraid, that this can break the audit filtering and signal auditing. I haven't yet looked deep into it, but it compares the task->tgid with this audit_pid for different purposes. If audit_pid changes this code will be broken.
OK, then both pids have to be stored. audit_pid remains as-is but is no longer used as destination netlink pid. A second pid is stored and updated whenever a netlink message is received from userspace.

The following patch represents what I mean. Untested!

kernel/audit.c +14 -5

Index: net-2.6.26/kernel/audit.c
===================================================================
@@ -82,6 +82,9 @@
  * contains the (non-zero) pid. */
 int		audit_pid;
 
+/* Actual netlink pid of the userspace process */
+static int	audit_nlk_pid;
+
 /* If audit_rate_limit is non-zero, limit the rate of sending audit records
  * to that number per second.  This prevents DoS attacks, but results in
  * audit records being dropped. */

@@ -347,12 +350,12 @@
 		skb = skb_dequeue(&audit_skb_queue);
 		wake_up(&audit_backlog_wait);
 		if (skb) {
-			if (audit_pid) {
-				int err = netlink_unicast(audit_sock, skb, audit_pid, 0);
+			if (audit_nlk_pid) {
+				int err = netlink_unicast(audit_sock, skb, audit_nlk_pid, 0);
 				if (err < 0) {
 					BUG_ON(err != -ECONNREFUSED); /* Shoudn't happen */
-					printk(KERN_ERR "audit: *NO* daemon at audit_pid=%d\n", audit_pid);
-					audit_pid = 0;
+					printk(KERN_ERR "audit: *NO* daemon at audit_nlk_pid=%d\n", audit_nlk_pid);
+					audit_nlk_pid = 0;
 				}
 			} else {
 				if (printk_ratelimit())

@@ -623,6 +626,12 @@
 							sid, 1);
 
 			audit_pid = new_pid;
+
+			/*
+			 * Netlink pid is only updated here to avoid overwrites
+			 * from potential processes only querying the interface.
+			 */
+			audit_nlk_pid = NETLINK_CB(skb).pid;
 		}
 		if (status_get->mask & AUDIT_STATUS_RATE_LIMIT)
 			err = audit_set_rate_limit(status_get->rate_limit,

@@ -1350,7 +1359,7 @@
 	if (!audit_rate_check()) {
 		audit_log_lost("rate limit exceeded");
 	} else {
-		if (audit_pid) {
+		if (audit_nlk_pid) {
 			struct nlmsghdr *nlh = nlmsg_hdr(ab->skb);
 			nlh->nlmsg_len = ab->skb->len - NLMSG_SPACE(0);
 			skb_queue_tail(&audit_skb_queue, ab->skb);

Pavel Emelyanov

author

2008-03-17 08:00 UTC #7 lore raw

In reply to Thomas Graf

Quoted 10 lines · Thomas Graf

* Pavel Emelyanov <[email protected]> 2008-03-14 20:05
Quoted 4 lines
Hmmm... I'm afraid, that this can break the audit filtering and signal auditing. I haven't yet looked deep into it, but it compares the task->tgid with this audit_pid for different purposes. If audit_pid changes this code will be broken.
OK, then both pids have to be stored. audit_pid remains as-is but is no longer used as destination netlink pid. A second pid is stored and updated whenever a netlink message is received from userspace.

Quoted 8 lines

Quoted 3 lines
Bu we have no the netlink socket at the moment of setting the pid to check this. The audit_reveive_msg() call which does this set is received via another (pre-created global) socket.
I don't understand this. As far as I can read the code, a plain kernel side netlink socket is created in audit_init(). But it doesn't matter, as soon as we receive the first message from userspace, we know the netlink source pid.

audit_init() creates a kernel-side socket, while we need to know the pid of a user-side one. But I saw your patch, seems like the NETLINK_CB(skb).pid is what we need for this check :)

Quoted 5 lines

Quoted 2 lines
I though, that proper behavior would be to split audit_pid, used for filtering from the audit_nlk_pid used for netlink communications.
Yes, exactly.

Pavel Emelyanov

author

2008-03-17 08:01 UTC #5 lore raw

In reply to Thomas Graf

Quoted 11 lines · Thomas Graf

* Thomas Graf <[email protected]> 2008-03-14 19:29
Quoted 8 lines
* Pavel Emelyanov <[email protected]> 2008-03-14 20:05
Quoted 4 lines
Hmmm... I'm afraid, that this can break the audit filtering and signal auditing. I haven't yet looked deep into it, but it compares the task->tgid with this audit_pid for different purposes. If audit_pid changes this code will be broken.
OK, then both pids have to be stored. audit_pid remains as-is but is no longer used as destination netlink pid. A second pid is stored and updated whenever a netlink message is received from userspace.
The following patch represents what I mean. Untested!

Looks great, all the more so I created very similar patch. David, can we have this in mainline some day?

Thanks,
Pavel

Quoted 54 lines

Index: net-2.6.26/kernel/audit.c
===================================================================

kernel/audit.c +14 -5

@@ -82,6 +82,9 @@
  * contains the (non-zero) pid. */
 int		audit_pid;
 
+/* Actual netlink pid of the userspace process */
+static int	audit_nlk_pid;
+
 /* If audit_rate_limit is non-zero, limit the rate of sending audit records
  * to that number per second.  This prevents DoS attacks, but results in
  * audit records being dropped. */

@@ -347,12 +350,12 @@
 		skb = skb_dequeue(&audit_skb_queue);
 		wake_up(&audit_backlog_wait);
 		if (skb) {
-			if (audit_pid) {
-				int err = netlink_unicast(audit_sock, skb, audit_pid, 0);
+			if (audit_nlk_pid) {
+				int err = netlink_unicast(audit_sock, skb, audit_nlk_pid, 0);
 				if (err < 0) {
 					BUG_ON(err != -ECONNREFUSED); /* Shoudn't happen */
-					printk(KERN_ERR "audit: *NO* daemon at audit_pid=%d\n", audit_pid);
-					audit_pid = 0;
+					printk(KERN_ERR "audit: *NO* daemon at audit_nlk_pid=%d\n", audit_nlk_pid);
+					audit_nlk_pid = 0;
 				}
 			} else {
 				if (printk_ratelimit())

@@ -623,6 +626,12 @@
 							sid, 1);
 
 			audit_pid = new_pid;
+
+			/*
+			 * Netlink pid is only updated here to avoid overwrites
+			 * from potential processes only querying the interface.
+			 */
+			audit_nlk_pid = NETLINK_CB(skb).pid;
 		}
 		if (status_get->mask & AUDIT_STATUS_RATE_LIMIT)
 			err = audit_set_rate_limit(status_get->rate_limit,

@@ -1350,7 +1359,7 @@
 	if (!audit_rate_check()) {
 		audit_log_lost("rate limit exceeded");
 	} else {
-		if (audit_pid) {
+		if (audit_nlk_pid) {
 			struct nlmsghdr *nlh = nlmsg_hdr(ab->skb);
 			nlh->nlmsg_len = ab->skb->len - NLMSG_SPACE(0);
 			skb_queue_tail(&audit_skb_queue, ab->skb);

Eric Paris

2008-03-17 19:41 UTC #6 lore raw

It looks reasonable to me. If you can send a tested patch with a good description to [email protected] I can work it into mainline.

-Eric

Quoted 83 lines · Pavel Emelyanov

Quoted 11 lines · Thomas Graf

* Thomas Graf <[email protected]> 2008-03-14 19:29
Quoted 8 lines
* Pavel Emelyanov <[email protected]> 2008-03-14 20:05
Quoted 4 lines
Hmmm... I'm afraid, that this can break the audit filtering and signal auditing. I haven't yet looked deep into it, but it compares the task->tgid with this audit_pid for different purposes. If audit_pid changes this code will be broken.
OK, then both pids have to be stored. audit_pid remains as-is but is no longer used as destination netlink pid. A second pid is stored and updated whenever a netlink message is received from userspace.
The following patch represents what I mean. Untested!

Looks great, all the more so I created very similar patch.
David, can we have this in mainline some day?

Thanks,
Pavel

Quoted 46 lines

Index: net-2.6.26/kernel/audit.c
===================================================================

kernel/audit.c 2008-03-14 19:38:35.000000000 +0100 renamed from kernel/audit.c 2008-03-14 19:31:53.000000000 +0100 +13 -4

@@ -82,6 +82,9 @@
  * contains the (non-zero) pid. */
 int          audit_pid;
 
+/* Actual netlink pid of the userspace process */
+static int   audit_nlk_pid;
+
 /* If audit_rate_limit is non-zero, limit the rate of sending audit records
  * to that number per second.  This prevents DoS attacks, but results in
  * audit records being dropped. */

@@ -347,12 +350,12 @@
              skb = skb_dequeue(&audit_skb_queue);
              wake_up(&audit_backlog_wait);
              if (skb) {
-                     if (audit_pid) {
-                             int err = netlink_unicast(audit_sock, skb, audit_pid, 0);
+                     if (audit_nlk_pid) {
+                             int err = netlink_unicast(audit_sock, skb, audit_nlk_pid, 0);
                              if (err < 0) {
                                      BUG_ON(err != -ECONNREFUSED); /* Shoudn't happen */
-                                     printk(KERN_ERR "audit: *NO* daemon at audit_pid=%d\n", audit_pid);
-                                     audit_pid = 0;
+                                     printk(KERN_ERR "audit: *NO* daemon at audit_nlk_pid=%d\n", audit_nlk_pid);
+                                     audit_nlk_pid = 0;
                              }
                      } else {
                              if (printk_ratelimit())

@@ -623,6 +626,12 @@
                                                      sid, 1);
 
                      audit_pid = new_pid;
+
+                     /*
+                      * Netlink pid is only updated here to avoid overwrites
+                      * from potential processes only querying the interface.
+                      */
+                     audit_nlk_pid = NETLINK_CB(skb).pid;
              }
              if (status_get->mask & AUDIT_STATUS_RATE_LIMIT)
                      err = audit_set_rate_limit(status_get->rate_limit,

kernel/audit.c 2008-03-14 19:38:35.000000000 +0100 line 1365

Quoted 8 lines

kernel/audit.c 2008-03-14 19:38:35.000000000 +0100 renamed from kernel/audit.c 2008-03-14 19:31:53.000000000 +0100 +1 -1

@@ -1350,7 +1359,7 @@
      if (!audit_rate_check()) {
              audit_log_lost("rate limit exceeded");
      } else {
-             if (audit_pid) {
+             if (audit_nlk_pid) {
                      struct nlmsghdr *nlh = nlmsg_hdr(ab->skb);
                      nlh->nlmsg_len = ab->skb->len - NLMSG_SPACE(0);
                      skb_queue_tail(&audit_skb_queue, ab->skb);

To unsubscribe from this list: send the line "unsubscribe netdev" in

the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html