Date:	Fri, 25 Sep 2015 17:14:29 -0400
From:	Paul Moore <pmoore@...hat.com>
To:	Richard Guy Briggs <rgb@...hat.com>
Cc:	linux-audit@...hat.com, linux-kernel@...r.kernel.org,
	sgrubb@...hat.com, eparis@...hat.com, v.rathor@...il.com,
	ctcard@...mail.com
Subject: Re: [PATCH 1/2] audit: stop an old auditd being starved out by a new auditd

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists