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

[PATCH 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_PING) to the old auditd containing a u32
with the PID of the new auditd.  If the audit ping 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.

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

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index d3475e1..4c97e30 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_PING		1020	/* Ping auditd to see if it is alive */
 
 #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 18cdfe2..3399ab2 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -810,6 +810,15 @@ static int audit_set_feature(struct sk_buff *skb)
 	return 0;
 }
 
+static int audit_ping(pid_t pid, u32 seq, u32 portid)
+{
+	struct sk_buff *skb = audit_make_reply(portid, seq, AUDIT_PING, 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;
@@ -871,13 +880,19 @@ 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);
+			u32 portid = NETLINK_CB(skb).portid;
 
-			if ((!new_pid) && (task_tgid_vnr(current) != audit_pid))
+			if ((!new_pid) && (requesting_pid != audit_pid))
 				return -EACCES;
+			if (audit_pid && new_pid &&
+			    audit_ping(requesting_pid, nlmsg_hdr(skb)->nlmsg_seq, portid) !=
+			    -ECONNREFUSED)
+				return -EEXIST;
 			if (audit_enabled != AUDIT_OFF)
 				audit_log_config_change("audit_pid", new_pid, audit_pid, 1);
 			audit_pid = new_pid;
-			audit_nlk_portid = NETLINK_CB(skb).portid;
+			audit_nlk_portid = portid;
 			audit_sock = skb->sk;
 		}
 		if (s.mask & AUDIT_STATUS_RATE_LIMIT) {
-- 
1.7.1

[PATCH 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 3399ab2..65dcd45 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -883,12 +883,16 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 			pid_t requesting_pid = task_tgid_vnr(current);
 			u32 portid = NETLINK_CB(skb).portid;
 
-			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_ping(requesting_pid, nlmsg_hdr(skb)->nlmsg_seq, portid) !=
-			    -ECONNREFUSED)
+			    -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 1/2] audit: stop an old auditd being starved out by a new auditd

[Paul Moore]

On Friday, September 18, 2015 03:59:58 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.
> 
> 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_PING) to the old auditd containing a u32
> with the PID of the new auditd.  If the audit ping 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.
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/uapi/linux/audit.h |    1 +
>  kernel/audit.c             |   19 +++++++++++++++++--
>  2 files changed, 18 insertions(+), 2 deletions(-)

XXX

> diff --git a/kernel/audit.c b/kernel/audit.c
> index 18cdfe2..3399ab2 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -810,6 +810,15 @@ static int audit_set_feature(struct sk_buff *skb)
>  	return 0;
>  }
> 
> +static int audit_ping(pid_t pid, u32 seq, u32 portid)
> +{
> +	struct sk_buff *skb = audit_make_reply(portid, seq, AUDIT_PING, 0, 0,
> +                                       &pid, sizeof(pid));

This is almost surely going to end up using the wrong netlink sequence number 
and portid since you are passing the new requestor's information below.  I 
didn't chase down the netlink_unicast() guts to see if it replaces the portid, 
it might (it probably does), but that still leaves the sequence number.

Also, this is more of a attempted hijack message and not a simple ping, right?  
If we want to create a simple ping message, leave the pid out of it; if we 
want to indicate to an existing auditd that another process is attempting to 
hijack the audit connection then we should probably create a proper audit 
record with a type other than AUDIT_PING.  I tend to think there is more value 
in the hijack message than the ping message, but I can be convinced either 
way.

> +	if (!skb)
> +		return -ENOMEM;
> +	return netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> +}

...

> @@ -871,13 +880,19 @@ static int audit_receive_msg(struct sk_buff *skb,
>  		if (s.mask & AUDIT_STATUS_PID) {
>  			int new_pid = s.pid;
> +			pid_t requesting_pid = task_tgid_vnr(current);
> +			u32 portid = NETLINK_CB(skb).portid;
> 
> -			if ((!new_pid) && (task_tgid_vnr(current) != audit_pid))
> +			if ((!new_pid) && (requesting_pid != audit_pid))
>  				return -EACCES;
> +			if (audit_pid && new_pid &&
> +			    audit_ping(requesting_pid, nlmsg_hdr(skb)->..., portid) !=
> +			    -ECONNREFUSED)
> +				return -EEXIST;

See my comments above about audit_ping().

>  			if (audit_enabled != AUDIT_OFF)
>  				audit_log_config_change("audit_pid", new_pid, audit_pid, 1);
>  			audit_pid = new_pid;
> -			audit_nlk_portid = NETLINK_CB(skb).portid;
> +			audit_nlk_portid = portid;
>  			audit_sock = skb->sk;
>  		}
>  		if (s.mask & AUDIT_STATUS_RATE_LIMIT) {

-- 
paul moore
security @ redhat

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

[Paul Moore]

On Friday, September 18, 2015 03:59:59 AM Richard Guy Briggs wrote:
> 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(-)

This patch tends to reinforce the idea of a hijack message instead of a ping 
message.  Unfortunately, we can't use audit_log_config_change() to generate 
the hijack message as it queues the record, but you get the idea.

> diff --git a/kernel/audit.c b/kernel/audit.c
> index 3399ab2..65dcd45 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -883,12 +883,16 @@ static int audit_receive_msg(struct sk_buff *skb,
> struct nlmsghdr *nlh) pid_t requesting_pid = task_tgid_vnr(current);
>  			u32 portid = NETLINK_CB(skb).portid;
> 
> -			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_ping(requesting_pid, nlmsg_hdr(skb)->nlmsg_seq, portid) 
!=
> -			    -ECONNREFUSED)
> +			    -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;

-- 
paul moore
security @ redhat

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

[Richard Guy Briggs]

On 15/09/24, Paul Moore wrote:
> On Friday, September 18, 2015 03:59:58 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.
> > 
> > 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_PING) to the old auditd containing a u32
> > with the PID of the new auditd.  If the audit ping 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.
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  include/uapi/linux/audit.h |    1 +
> >  kernel/audit.c             |   19 +++++++++++++++++--
> >  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> XXX

???

> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 18cdfe2..3399ab2 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -810,6 +810,15 @@ static int audit_set_feature(struct sk_buff *skb)
> >  	return 0;
> >  }
> > 
> > +static int audit_ping(pid_t pid, u32 seq, u32 portid)
> > +{
> > +	struct sk_buff *skb = audit_make_reply(portid, seq, AUDIT_PING, 0, 0,
> > +                                       &pid, sizeof(pid));
> 
> This is almost surely going to end up using the wrong netlink sequence number 
> and portid since you are passing the new requestor's information below.  I 
> didn't chase down the netlink_unicast() guts to see if it replaces the portid, 
> it might (it probably does), but that still leaves the sequence number.

It is intended to use the new pid and new netlink sequence number to the
old audit_sock and old portid.  There is no other sequence number
available and it is this new sequence number and pid that needs
reporting to the old auditd.

> Also, this is more of a attempted hijack message and not a simple ping, right?  

Ok, so maybe AUDIT_PING is not the appropriate name for it.  I don't
have a problem changing it, but I think the pid of the hijacker would be
useful information to the ping-ee unless the ping message was only ever
issues in a contextless kernel-initiated message.

> If we want to create a simple ping message, leave the pid out of it; if we 
> want to indicate to an existing auditd that another process is attempting to 
> hijack the audit connection then we should probably create a proper audit 
> record with a type other than AUDIT_PING.  I tend to think there is more value 
> in the hijack message than the ping message, but I can be convinced either 
> way.

Is there any compelling reason to create a pure ping message that gets
sent out periodically to test if auditd is still alive (audit_pid,
audit_sock and audit_nlk_portid are valid)?  Is there any reason to
reserve that AUDIT_PING macro at this time should it be determined that
it is necessary in the future?

> > +	if (!skb)
> > +		return -ENOMEM;
> > +	return netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > +}
> 
> ...
> 
> > @@ -871,13 +880,19 @@ static int audit_receive_msg(struct sk_buff *skb,
> >  		if (s.mask & AUDIT_STATUS_PID) {
> >  			int new_pid = s.pid;
> > +			pid_t requesting_pid = task_tgid_vnr(current);
> > +			u32 portid = NETLINK_CB(skb).portid;
> > 
> > -			if ((!new_pid) && (task_tgid_vnr(current) != audit_pid))
> > +			if ((!new_pid) && (requesting_pid != audit_pid))
> >  				return -EACCES;
> > +			if (audit_pid && new_pid &&
> > +			    audit_ping(requesting_pid, nlmsg_hdr(skb)->..., portid) !=
> > +			    -ECONNREFUSED)
> > +				return -EEXIST;
> 
> See my comments above about audit_ping().
> 
> >  			if (audit_enabled != AUDIT_OFF)
> >  				audit_log_config_change("audit_pid", new_pid, audit_pid, 1);
> >  			audit_pid = new_pid;
> > -			audit_nlk_portid = NETLINK_CB(skb).portid;
> > +			audit_nlk_portid = portid;
> >  			audit_sock = skb->sk;
> >  		}
> >  		if (s.mask & AUDIT_STATUS_RATE_LIMIT) {
> 
> paul moore

- 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

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

[Paul Moore]

On Friday, September 25, 2015 07:10:19 AM Richard Guy Briggs wrote:
> On 15/09/24, Paul Moore wrote:
> > On Friday, September 18, 2015 03:59:58 AM Richard Guy Briggs wrote:

...

> > XXX
> 
> ???

Sorry, ignore that.  The "XXX" was a placeholder for me while I was reviewing 
your patch; normally I got back and replace those all with text or drop them, 
but one slipped through in this case.

> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index 18cdfe2..3399ab2 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -810,6 +810,15 @@ static int audit_set_feature(struct sk_buff *skb)
> > > 
> > >  	return 0;
> > >  
> > >  }
> > > 
> > > +static int audit_ping(pid_t pid, u32 seq, u32 portid)
> > > +{
> > > +	struct sk_buff *skb = audit_make_reply(portid,seq,AUDIT_PING,0,0,
> > > +                                       &pid, sizeof(pid));
> > 
> > This is almost surely going to end up using the wrong netlink sequence
> > number and portid since you are passing the new requestor's information
> > below.  I didn't chase down the netlink_unicast() guts to see if it
> > replaces the portid, it might (it probably does), but that still leaves
> > the sequence number.
>
> It is intended to use the new pid and new netlink sequence number to the
> old audit_sock and old portid.

You don't want to put the new portid/seqno in a netlink header that is being 
sent to the old auditd.

> There is no other sequence number available and it is this new sequence
> number and pid that needs reporting to the old auditd.

The audit_make_reply() function is the wrong thing to be using here, we should 
create our own buffer from scratch like most other records.  Also, yes, we 
want to include the new pid, but I really don't think there is any value in 
including the seqno of the AUDIT_SET/AUDIT_STATUS_PID message.
 
> > Also, this is more of a attempted hijack message and not a simple ping,
> > right?
>
> Ok, so maybe AUDIT_PING is not the appropriate name for it.  I don't
> have a problem changing it, but I think the pid of the hijacker would be
> useful information to the ping-ee unless the ping message was only ever
> issues in a contextless kernel-initiated message.

Let's change the message name, this isn't a ping message and we may want to 
have a ping message at some point in the future.
 
> > If we want to create a simple ping message, leave the pid out of it; if we
> > want to indicate to an existing auditd that another process is attempting
> > to hijack the audit connection then we should probably create a proper
> > audit record with a type other than AUDIT_PING.  I tend to think there is
> > more value in the hijack message than the ping message, but I can be
> > convinced either way.
> 
> Is there any compelling reason to create a pure ping message that gets
> sent out periodically to test if auditd is still alive (audit_pid,
> audit_sock and audit_nlk_portid are valid)?

Possibly, but let's leave that as a future experiment.

> Is there any reason to reserve that AUDIT_PING macro at this time should it
> be determined that it is necessary in the future?

I don't think we need to reserve it now, we can allocate the ping message type 
if/when we decide to implement it.

-- 
paul moore
security @ redhat

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

[Richard Guy Briggs]

On 15/09/25, Paul Moore wrote:
> On Friday, September 25, 2015 07:10:19 AM Richard Guy Briggs wrote:
> > On 15/09/24, Paul Moore wrote:
> > > On Friday, September 18, 2015 03:59:58 AM Richard Guy Briggs wrote:
> > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > index 18cdfe2..3399ab2 100644
> > > > --- a/kernel/audit.c
> > > > +++ b/kernel/audit.c
> > > > @@ -810,6 +810,15 @@ static int audit_set_feature(struct sk_buff *skb)
> > > > 
> > > >  	return 0;
> > > >  
> > > >  }
> > > > 
> > > > +static int audit_ping(pid_t pid, u32 seq, u32 portid)
> > > > +{
> > > > +	struct sk_buff *skb = audit_make_reply(portid,seq,AUDIT_PING,0,0,
> > > > +                                       &pid, sizeof(pid));
> > > 
> > > This is almost surely going to end up using the wrong netlink sequence
> > > number and portid since you are passing the new requestor's information
> > > below.  I didn't chase down the netlink_unicast() guts to see if it
> > > replaces the portid, it might (it probably does), but that still leaves
> > > the sequence number.
> >
> > It is intended to use the new pid and new netlink sequence number to the
> > old audit_sock and old portid.
> 
> You don't want to put the new portid/seqno in a netlink header that is being 
> sent to the old auditd.

Ok, you are right, both should be set to zero, since nothing else makes sense.

> > There is no other sequence number available and it is this new sequence
> > number and pid that needs reporting to the old auditd.
> 
> The audit_make_reply() function is the wrong thing to be using here, we should 
> create our own buffer from scratch like most other records.  Also, yes, we 
> want to include the new pid, but I really don't think there is any value in 
> including the seqno of the AUDIT_SET/AUDIT_STATUS_PID message.

Most other records use audit_log_start(), which isn't what we want here,
since we want to bypass the queue to test if it is still alive.  We
don't care if it is delivered.  We just care if the socket is still
alive.  We don't want a context either.

So, I believ audit_make_reply() can be used just fine, setting portid,
seq, done and multi to zero.

> > > Also, this is more of a attempted hijack message and not a simple ping,
> > > right?
> >
> > Ok, so maybe AUDIT_PING is not the appropriate name for it.  I don't
> > have a problem changing it, but I think the pid of the hijacker would be
> > useful information to the ping-ee unless the ping message was only ever
> > issues in a contextless kernel-initiated message.
> 
> Let's change the message name, this isn't a ping message and we may want to 
> have a ping message at some point in the future.

Ok, how about AUDIT_HIJACK_TEST, with a payload of the u32
representation of the PID of the task attempting to replace it.

> > > If we want to create a simple ping message, leave the pid out of it; if we
> > > want to indicate to an existing auditd that another process is attempting
> > > to hijack the audit connection then we should probably create a proper
> > > audit record with a type other than AUDIT_PING.  I tend to think there is
> > > more value in the hijack message than the ping message, but I can be
> > > convinced either way.
> > 
> > Is there any compelling reason to create a pure ping message that gets
> > sent out periodically to test if auditd is still alive (audit_pid,
> > audit_sock and audit_nlk_portid are valid)?
> 
> Possibly, but let's leave that as a future experiment.

Ok.

> > Is there any reason to reserve that AUDIT_PING macro at this time should it
> > be determined that it is necessary in the future?
> 
> I don't think we need to reserve it now, we can allocate the ping message type 
> if/when we decide to implement it.

Ok.

> paul moore

- 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

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

[Paul Moore]

On Monday, September 28, 2015 07:17:31 AM Richard Guy Briggs wrote:
> On 15/09/25, Paul Moore wrote:

...

> > The audit_make_reply() function is the wrong thing to be using here, we
> > should create our own buffer from scratch like most other records.  Also,
> > yes, we want to include the new pid, but I really don't think there is
> > any value in including the seqno of the AUDIT_SET/AUDIT_STATUS_PID
> > message.
> 
> Most other records use audit_log_start(), which isn't what we want here,
> since we want to bypass the queue to test if it is still alive.  We
> don't care if it is delivered.  We just care if the socket is still
> alive.  We don't want a context either.

Yes, that is why I mentioned creating the buffer from scratch.

> So, I believ audit_make_reply() can be used just fine, setting portid,
> seq, done and multi to zero.

The 'multi' flag should definitely be set to zero, 'seq' is fine at zero, but 
I think we can do better with 'portid'; we know the 'portid' value so just use 
it in the call to audit_make_reply().

I don't like that we are reusing audit_make_reply() for non-reply netlink 
messages, but I'll get over that.  This will likely get a revamp when we get 
around to a proper fix of the queuing system.

> > > > Also, this is more of a attempted hijack message and not a simple
> > > > ping,
> > > > right?
> > > 
> > > Ok, so maybe AUDIT_PING is not the appropriate name for it.  I don't
> > > have a problem changing it, but I think the pid of the hijacker would be
> > > useful information to the ping-ee unless the ping message was only ever
> > > issues in a contextless kernel-initiated message.
> > 
> > Let's change the message name, this isn't a ping message and we may want
> > to have a ping message at some point in the future.
> 
> Ok, how about AUDIT_HIJACK_TEST, with a payload of the u32
> representation of the PID of the task attempting to replace it.

Why add the TEST?  It is a hijack attempt, or at least it is if the record is 
emitted successfully :)  I would go simply with AUDIT_HIJACK or maybe 
AUDIT_REPLACE (or similar) if "hijack" is a bit too inflammatory (it probably 
is ...).

-- 
paul moore
security @ redhat

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

[Richard Guy Briggs]

On 15/09/28, Paul Moore wrote:
> On Monday, September 28, 2015 07:17:31 AM Richard Guy Briggs wrote:
> > On 15/09/25, Paul Moore wrote:
> > > The audit_make_reply() function is the wrong thing to be using here, we
> > > should create our own buffer from scratch like most other records.  Also,
> > > yes, we want to include the new pid, but I really don't think there is
> > > any value in including the seqno of the AUDIT_SET/AUDIT_STATUS_PID
> > > message.
> > 
> > Most other records use audit_log_start(), which isn't what we want here,
> > since we want to bypass the queue to test if it is still alive.  We
> > don't care if it is delivered.  We just care if the socket is still
> > alive.  We don't want a context either.
> 
> Yes, that is why I mentioned creating the buffer from scratch.
> 
> > So, I believ audit_make_reply() can be used just fine, setting portid,
> > seq, done and multi to zero.
> 
> The 'multi' flag should definitely be set to zero, 'seq' is fine at zero, but 
> I think we can do better with 'portid'; we know the 'portid' value so just use 
> it in the call to audit_make_reply().

Most other audit_log_start() created messages set portid to zero except
user messages, and those are set using the initiating process' portid
and not the destination id.  So here I think portid should be zero.  The
target task should know its own portid and the netlink field for portid
isn't used for routing to that destination that I can discern from the
netlink code.

> I don't like that we are reusing audit_make_reply() for non-reply netlink 
> messages, but I'll get over that.  This will likely get a revamp when we get 
> around to a proper fix of the queuing system.

This could even be renamed audit_make_message() and possibly be
generalized to be useful to audit_log_start(), or rather
audit_buffer_alloc().  Later...

> > > > > Also, this is more of a attempted hijack message and not a
> > > > > simple ping, right?
> > > > 
> > > > Ok, so maybe AUDIT_PING is not the appropriate name for it.  I don't
> > > > have a problem changing it, but I think the pid of the hijacker would be
> > > > useful information to the ping-ee unless the ping message was only ever
> > > > issues in a contextless kernel-initiated message.
> > > 
> > > Let's change the message name, this isn't a ping message and we may want
> > > to have a ping message at some point in the future.
> > 
> > Ok, how about AUDIT_HIJACK_TEST, with a payload of the u32
> > representation of the PID of the task attempting to replace it.
> 
> Why add the TEST?  It is a hijack attempt, or at least it is if the record is 
> emitted successfully :)  I would go simply with AUDIT_HIJACK or maybe 
> AUDIT_REPLACE (or similar) if "hijack" is a bit too inflammatory (it probably 
> is ...).

I had actually named it AUDIT_REPLACE_TEST, but your repeated use of the
term "hijack" swayed me...  I'd still lean towards *_TEST since it is
testing to replace a stale socket and not a live one.

> paul moore

- 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

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

[Paul Moore]

On Tuesday, September 29, 2015 12:36:11 AM Richard Guy Briggs wrote:
> On 15/09/28, Paul Moore wrote:
> > On Monday, September 28, 2015 07:17:31 AM Richard Guy Briggs wrote:

...

> > 
> > > So, I believ audit_make_reply() can be used just fine, setting portid,
> > > seq, done and multi to zero.
> > 
> > The 'multi' flag should definitely be set to zero, 'seq' is fine at zero,
> > but I think we can do better with 'portid'; we know the 'portid' value so
> > just use it in the call to audit_make_reply().
> 
> Most other audit_log_start() created messages set portid to zero except
> user messages, and those are set using the initiating process' portid
> and not the destination id.

Sorry, I was confusing the portid in sockaddr_nl with the portid in the 
nlmsghdr ... anyway, yes, the portid in the netlink header should always be 
zero since it references the sender and not the destination and the kernel has 
portid 0.

> > I don't like that we are reusing audit_make_reply() for non-reply netlink
> > messages, but I'll get over that.  This will likely get a revamp when we
> > get around to a proper fix of the queuing system.
> 
> This could even be renamed audit_make_message() and possibly be
> generalized to be useful to audit_log_start(), or rather
> audit_buffer_alloc().  Later...

Not exactly what I was thinking, but as I said, not worth worrying about right 
now.

> > > Ok, how about AUDIT_HIJACK_TEST, with a payload of the u32
> > > representation of the PID of the task attempting to replace it.
> > 
> > Why add the TEST?  It is a hijack attempt, or at least it is if the record
> > is emitted successfully :)  I would go simply with AUDIT_HIJACK or maybe
> > AUDIT_REPLACE (or similar) if "hijack" is a bit too inflammatory (it
> > probably is ...).
> 
> I had actually named it AUDIT_REPLACE_TEST, but your repeated use of the
> term "hijack" swayed me...  I'd still lean towards *_TEST since it is
> testing to replace a stale socket and not a live one.

While we are using the record for a test, that is not its only purpose and we 
might arrive at a future need to indicate a "hijack" that isn't a test.  Keep 
the "hijack" if you want, remove the "test".

-- 
paul moore
security @ redhat