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
* Pavel Emelyanov <[email protected]> 2008-03-14 19:22
> 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 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.
Thomas Graf wrote:
> * Pavel Emelyanov <[email protected]> 2008-03-14 19:22
>> 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 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...
> 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.
> 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.
> 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.
* Pavel Emelyanov <[email protected]> 2008-03-14 20:05
> 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.
> 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.
> 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 <[email protected]> 2008-03-14 19:29
> * Pavel Emelyanov <[email protected]> 2008-03-14 20:05
> > 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!
Index: net-2.6.26/kernel/audit.c
===================================================================
--- net-2.6.26.orig/kernel/audit.c 2008-03-14 19:31:53.000000000 +0100
+++ net-2.6.26/kernel/audit.c 2008-03-14 19:38:35.000000000 +0100
@@ -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);
Thomas Graf wrote:
> * Pavel Emelyanov <[email protected]> 2008-03-14 20:05
>> 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.
>
>> 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 :)
>> 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 wrote:
> * Thomas Graf <[email protected]> 2008-03-14 19:29
>> * Pavel Emelyanov <[email protected]> 2008-03-14 20:05
>>> 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
> Index: net-2.6.26/kernel/audit.c
> ===================================================================
> --- net-2.6.26.orig/kernel/audit.c 2008-03-14 19:31:53.000000000 +0100
> +++ net-2.6.26/kernel/audit.c 2008-03-14 19:38:35.000000000 +0100
> @@ -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);
>
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
On 3/17/08, Pavel Emelyanov <[email protected]> wrote:
> Thomas Graf wrote:
> > * Thomas Graf <[email protected]> 2008-03-14 19:29
> >> * Pavel Emelyanov <[email protected]> 2008-03-14 20:05
> >>> 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
>
>
> > Index: net-2.6.26/kernel/audit.c
> > ===================================================================
> > --- net-2.6.26.orig/kernel/audit.c 2008-03-14 19:31:53.000000000 +0100
> > +++ net-2.6.26/kernel/audit.c 2008-03-14 19:38:35.000000000 +0100
> > @@ -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);
> >
>
> --
>
> 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
>