[PATCH V2 1/2] audit: stop an old auditd being starved out by a new auditd

[PATCH V2 1/2] audit: stop an old auditd being starved out by a new auditd

[Richard Guy Briggs]

Nothing prevents a new auditd starting up and replacing a valid
audit_pid when an old auditd is still running, effectively starving out
the old auditd since audit_pid no longer points to the old valid auditd.

If no message to auditd has been attempted since auditd died unnaturally
or got killed, audit_pid will still indicate it is alive.  There isn't
an easy way to detect if an old auditd is still running on the existing
audit_pid other than attempting to send a message to see if it fails.
An -ECONNREFUSED almost certainly means it disappeared and can be
replaced.  Other errors are not so straightforward and may indicate
transient problems that will resolve themselves and the old auditd will
recover.  Yet others will likely need manual intervention for which a
new auditd will not solve the problem.

Send a new message type (AUDIT_REPLACE) to the old auditd containing a
u32 with the PID of the new auditd.  If the audit replace message
succeeds (or doesn't fail with certainty), fail to register the new
auditd and return an error (-EEXIST).

This is expected to make the patch preventing an old auditd orphaning a
new auditd redundant.

V2: Rename audit_ping to audit_replace, set seq and portid to 0 in
the call to audit_make_reply().

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/uapi/linux/audit.h |    1 +
 kernel/audit.c             |   16 +++++++++++++++-
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 843540c..cf84991 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -70,6 +70,7 @@
 #define AUDIT_TTY_SET		1017	/* Set TTY auditing status */
 #define AUDIT_SET_FEATURE	1018	/* Turn an audit feature on or off */
 #define AUDIT_GET_FEATURE	1019	/* Get which features are enabled */
+#define AUDIT_REPLACE		1020	/* Replace auditd if this packet unanswerd */
 
 #define AUDIT_FIRST_USER_MSG	1100	/* Userspace messages mostly uninteresting to kernel */
 #define AUDIT_USER_AVC		1107	/* We filter this differently */
diff --git a/kernel/audit.c b/kernel/audit.c
index 36989a1..0368be2 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -809,6 +809,16 @@ static int audit_set_feature(struct sk_buff *skb)
 	return 0;
 }
 
+static int audit_replace(pid_t pid)
+{
+	struct sk_buff *skb = audit_make_reply(0, 0, AUDIT_REPLACE, 0, 0,
+					       &pid, sizeof(pid));
+
+	if (!skb)
+		return -ENOMEM;
+	return netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
+}
+
 static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 {
 	u32			seq;
@@ -870,9 +880,13 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		}
 		if (s.mask & AUDIT_STATUS_PID) {
 			int new_pid = s.pid;
+			pid_t requesting_pid = task_tgid_vnr(current);
 
-			if ((!new_pid) && (task_tgid_vnr(current) != audit_pid))
+			if ((!new_pid) && (requesting_pid != audit_pid))
 				return -EACCES;
+			if (audit_pid && new_pid &&
+			    audit_replace(requesting_pid) != -ECONNREFUSED)
+				return -EEXIST;
 			if (audit_enabled != AUDIT_OFF)
 				audit_log_config_change("audit_pid", new_pid, audit_pid, 1);
 			audit_pid = new_pid;
-- 
1.7.1

[PATCH V2 2/2] audit: log failed attempts to change audit_pid configuration

[Richard Guy Briggs]

Failed attempts to change the audit_pid configuration are not presently
logged.  One case is an attempt to starve an old auditd by starting up a
new auditd when the old one is still alive and active.  The other case
is an attempt to orphan a new auditd when an old auditd shuts down.

Log both as AUDIT_CONFIG_CHANGE messages with failure result.

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

diff --git a/kernel/audit.c b/kernel/audit.c
index 0368be2..9000c6f 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -882,11 +882,15 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 			int new_pid = s.pid;
 			pid_t requesting_pid = task_tgid_vnr(current);
 
-			if ((!new_pid) && (requesting_pid != audit_pid))
+			if ((!new_pid) && (requesting_pid != audit_pid)) {
+				audit_log_config_change("audit_pid", new_pid, audit_pid, 0);
 				return -EACCES;
+			}
 			if (audit_pid && new_pid &&
-			    audit_replace(requesting_pid) != -ECONNREFUSED)
+			    audit_replace(requesting_pid) != -ECONNREFUSED) {
+				audit_log_config_change("audit_pid", new_pid, audit_pid, 0);
 				return -EEXIST;
+			}
 			if (audit_enabled != AUDIT_OFF)
 				audit_log_config_change("audit_pid", new_pid, audit_pid, 1);
 			audit_pid = new_pid;
-- 
1.7.1

[PATCH V2 2/2] audit: log failed attempts to change audit_pid configuration

[Richard Guy Briggs]

Failed attempts to change the audit_pid configuration are not presently
logged.  One case is an attempt to starve an old auditd by starting up a
new auditd when the old one is still alive and active.  The other case
is an attempt to orphan a new auditd when an old auditd shuts down.

Log both as AUDIT_CONFIG_CHANGE messages with failure result.

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

diff --git a/kernel/audit.c b/kernel/audit.c
index 0368be2..9000c6f 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -882,11 +882,15 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 			int new_pid = s.pid;
 			pid_t requesting_pid = task_tgid_vnr(current);
 
-			if ((!new_pid) && (requesting_pid != audit_pid))
+			if ((!new_pid) && (requesting_pid != audit_pid)) {
+				audit_log_config_change("audit_pid", new_pid, audit_pid, 0);
 				return -EACCES;
+			}
 			if (audit_pid && new_pid &&
-			    audit_replace(requesting_pid) != -ECONNREFUSED)
+			    audit_replace(requesting_pid) != -ECONNREFUSED) {
+				audit_log_config_change("audit_pid", new_pid, audit_pid, 0);
 				return -EEXIST;
+			}
 			if (audit_enabled != AUDIT_OFF)
 				audit_log_config_change("audit_pid", new_pid, audit_pid, 1);
 			audit_pid = new_pid;
-- 
1.7.1

Re: [PATCH V2 1/2] audit: stop an old auditd being starved out by a new auditd

[Steve Grubb]

Hello Richard,

Public reply this time.  :-)

On Wednesday, December 16, 2015 10:42:32 AM Richard Guy Briggs wrote:
> Nothing prevents a new auditd starting up and replacing a valid
> audit_pid when an old auditd is still running, effectively starving out
> the old auditd since audit_pid no longer points to the old valid auditd.

I guess the first question is why do we allow something to start up a new 
auditd without killing off the old one? Would that be a simpler fix?


> If no message to auditd has been attempted since auditd died unnaturally
> or got killed, audit_pid will still indicate it is alive.  There isn't
> an easy way to detect if an old auditd is still running on the existing
> audit_pid other than attempting to send a message to see if it fails.
> An -ECONNREFUSED almost certainly means it disappeared and can be
> replaced.  Other errors are not so straightforward and may indicate
> transient problems that will resolve themselves and the old auditd will
> recover.  Yet others will likely need manual intervention for which a
> new auditd will not solve the problem.
> 
> Send a new message type (AUDIT_REPLACE) to the old auditd containing a
> u32 with the PID of the new auditd.  If the audit replace message
> succeeds (or doesn't fail with certainty), fail to register the new
> auditd and return an error (-EEXIST).
> 
> This is expected to make the patch preventing an old auditd orphaning a
> new auditd redundant.
> 
> V2: Rename audit_ping to audit_replace, set seq and portid to 0 in
> the call to audit_make_reply().
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/uapi/linux/audit.h |    1 +
>  kernel/audit.c             |   16 +++++++++++++++-
>  2 files changed, 16 insertions(+), 1 deletions(-)
> 
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 843540c..cf84991 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -70,6 +70,7 @@
>  #define AUDIT_TTY_SET		1017	/* Set TTY auditing status */
>  #define AUDIT_SET_FEATURE	1018	/* Turn an audit feature on or off */
>  #define AUDIT_GET_FEATURE	1019	/* Get which features are enabled */
> +#define AUDIT_REPLACE		1020	/* Replace auditd if this packet 
unanswerd */

In every case, events in the 1000 block are to configure the kernel or to ask 
the kernel a question. This is user space initiating. Kernel initiating events 
are in the 1300 block of numbers.

-Steve


>  #define AUDIT_FIRST_USER_MSG	1100	/* Userspace messages mostly
> uninteresting to kernel */ #define AUDIT_USER_AVC		1107	/* We filter 
this
> differently */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 36989a1..0368be2 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -809,6 +809,16 @@ static int audit_set_feature(struct sk_buff *skb)
>  	return 0;
>  }
> 
> +static int audit_replace(pid_t pid)
> +{
> +	struct sk_buff *skb = audit_make_reply(0, 0, AUDIT_REPLACE, 0, 0,
> +					       &pid, sizeof(pid));
> +
> +	if (!skb)
> +		return -ENOMEM;
> +	return netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> +}
> +
>  static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>  {
>  	u32			seq;
> @@ -870,9 +880,13 @@ static int audit_receive_msg(struct sk_buff *skb,
> struct nlmsghdr *nlh) }
>  		if (s.mask & AUDIT_STATUS_PID) {
>  			int new_pid = s.pid;
> +			pid_t requesting_pid = task_tgid_vnr(current);
> 
> -			if ((!new_pid) && (task_tgid_vnr(current) != audit_pid))
> +			if ((!new_pid) && (requesting_pid != audit_pid))
>  				return -EACCES;
> +			if (audit_pid && new_pid &&
> +			    audit_replace(requesting_pid) != -ECONNREFUSED)
> +				return -EEXIST;
>  			if (audit_enabled != AUDIT_OFF)
>  				audit_log_config_change("audit_pid", new_pid, audit_pid, 
1);
>  			audit_pid = new_pid;

Re: [PATCH V2 1/2] audit: stop an old auditd being starved out by a new auditd

[Paul Moore]

On Wednesday, December 16, 2015 11:23:19 AM Steve Grubb wrote:
> On Wednesday, December 16, 2015 10:42:32 AM Richard Guy Briggs wrote:
> > Nothing prevents a new auditd starting up and replacing a valid
> > audit_pid when an old auditd is still running, effectively starving out
> > the old auditd since audit_pid no longer points to the old valid auditd.
> 
> I guess the first question is why do we allow something to start up a new
> auditd without killing off the old one?  Would that be a simpler fix?

I imagine there might be scenarios where you need to forcibly kill an instance 
of auditd such that things might not get fully cleaned up in the kernel, 
audit_{pid,sock,etc.}.  Keeping the ability to reset the kernel's auditd 
state, even when the kernel *thinks* auditd is still alive might be a nice 
thing to keep around for a while longer.

> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 843540c..cf84991 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -70,6 +70,7 @@
> > 
> >  #define AUDIT_TTY_SET		1017	/* Set TTY auditing status */
> >  #define AUDIT_SET_FEATURE	1018	/* Turn an audit feature on or off */
> >  #define AUDIT_GET_FEATURE	1019	/* Get which features are enabled */
> > 
> > +#define AUDIT_REPLACE		1020	/* Replace auditd if this pack... */
> 
> In every case, events in the 1000 block are to configure the kernel or to
> ask the kernel a question. This is user space initiating. Kernel initiating
> events are in the 1300 block of numbers.

Change the audit event number as Steve suggests and I'll toss the patches into 
my audit next queue, although considering we are at 4.4-rc6 at present, I'll 
probably hold this until after the merge window closes, meaning it is 4.6 
material.

-- 
paul moore
security @ redhat

Re: [PATCH V2 1/2] audit: stop an old auditd being starved out by a new auditd

[Steve Grubb]

On Monday, December 21, 2015 04:48:00 PM Paul Moore wrote:
> On Wednesday, December 16, 2015 11:23:19 AM Steve Grubb wrote:
> > On Wednesday, December 16, 2015 10:42:32 AM Richard Guy Briggs wrote:
> > > Nothing prevents a new auditd starting up and replacing a valid
> > > audit_pid when an old auditd is still running, effectively starving out
> > > the old auditd since audit_pid no longer points to the old valid auditd.
> > 
> > I guess the first question is why do we allow something to start up a new
> > auditd without killing off the old one?  Would that be a simpler fix?
> 
> I imagine there might be scenarios where you need to forcibly kill an
> instance of auditd such that things might not get fully cleaned up in the
> kernel, audit_{pid,sock,etc.}. 

But the first time an event is sent and auditd doesn't exist, it resets the 
audit_pid to 0.

static void kauditd_send_skb(struct sk_buff *skb)
{
        int err;
        /* take a reference in case we can't send it and we want to hold it */
        skb_get(skb);
        err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
        if (err < 0) {
                BUG_ON(err != -ECONNREFUSED); /* Shouldn't happen */
                if (audit_pid) {
                        pr_err("*NO* daemon at audit_pid=%d\n", audit_pid);
                        audit_log_lost("auditd disappeared");
                        audit_pid = 0;
                        audit_sock = NULL;
                }



> Keeping the ability to reset the kernel's auditd state, even when the kernel
> *thinks* auditd is still alive might be a nice thing to keep around for a
> while longer.

I'm just thinking its rare that anyone would try to steal away the audit 
socket. Its more work for everyone to create a new event and send it than to 
just not allow it. you can even force an event with "auditctl -m test" which 
should reset the pid if the kernel was out of sync.

The 

 
> > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > > index 843540c..cf84991 100644
> > > --- a/include/uapi/linux/audit.h
> > > +++ b/include/uapi/linux/audit.h
> > > @@ -70,6 +70,7 @@
> > > 
> > >  #define AUDIT_TTY_SET		1017	/* Set TTY auditing status */
> > >  #define AUDIT_SET_FEATURE	1018	/* Turn an audit feature on or off 
*/
> > >  #define AUDIT_GET_FEATURE	1019	/* Get which features are enabled 
*/
> > > 
> > > +#define AUDIT_REPLACE		1020	/* Replace auditd if this pack... 
*/
> > 
> > In every case, events in the 1000 block are to configure the kernel or to
> > ask the kernel a question. This is user space initiating. Kernel
> > initiating
> > events are in the 1300 block of numbers.
> 
> Change the audit event number as Steve suggests and I'll toss the patches
> into my audit next queue, although considering we are at 4.4-rc6 at
> present, I'll probably hold this until after the merge window closes,
> meaning it is 4.6 material.

Re: [PATCH V2 1/2] audit: stop an old auditd being starved out by a new auditd

[Paul Moore]

On Monday, December 21, 2015 05:18:15 PM Steve Grubb wrote:
> On Monday, December 21, 2015 04:48:00 PM Paul Moore wrote:
> > On Wednesday, December 16, 2015 11:23:19 AM Steve Grubb wrote:
> > > On Wednesday, December 16, 2015 10:42:32 AM Richard Guy Briggs wrote:
> > > > Nothing prevents a new auditd starting up and replacing a valid
> > > > audit_pid when an old auditd is still running, effectively starving
> > > > out the old auditd since audit_pid no longer points to the old valid
> > > > auditd.
> > > 
> > > I guess the first question is why do we allow something to start up a
> > > new auditd without killing off the old one?  Would that be a simpler
> > > fix?
> > 
> > I imagine there might be scenarios where you need to forcibly kill an
> > instance of auditd such that things might not get fully cleaned up in the
> > kernel, audit_{pid,sock,etc.}.
> 
> But the first time an event is sent and auditd doesn't exist, it resets the
> audit_pid to 0.
> 
> static void kauditd_send_skb(struct sk_buff *skb)
> {
>         int err;
>         /* take a reference in case we can't send it and we want...
>         skb_get(skb);
>         err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
>         if (err < 0) {
>                 BUG_ON(err != -ECONNREFUSED); /* Shouldn't happen */
>                 if (audit_pid) {
>                         pr_err("*NO* daemon at audit_pid=%d\n", audit_pid);
>                         audit_log_lost("auditd disappeared");
>                         audit_pid = 0;
>                         audit_sock = NULL;
>                 }

As an aside, it doesn't matter in this particular case, but the above code is 
not current.  Please try to use either what is in Linus' tree or audit#next 
when pasting code snippets; it's less confusing.

I still think there is some value in having the ability for an admin to reset 
the kernel's auditd tracking manually as relying on an event to be emitted 
does not seem like a solution I would want to have to justify.  Although I do 
admit that for most systems this shouldn't be a problem as events should 
likely occur often enough.

There really is no harm in merging these patches, and they do provide some, 
admittedly small, value.

> > Keeping the ability to reset the kernel's auditd state, even when the
> > kernel *thinks* auditd is still alive might be a nice thing to keep
> > around for a while longer.
> 
> I'm just thinking its rare that anyone would try to steal away the audit
> socket. Its more work for everyone to create a new event and send it than to
> just not allow it. you can even force an event with "auditctl -m test"
> which should reset the pid if the kernel was out of sync.

I do not want to disallow starting an new instance of auditd, so this patchset 
looks reasonable to me.

-- 
paul moore
security @ redhat