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@xxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/