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

2015-12-21 Thread 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

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

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

2015-12-21 Thread 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_SET1017/* Set TTY auditing status */
> > >  #define AUDIT_SET_FEATURE1018/* Turn an audit feature on or 
> > > off 
*/
> > >  #define AUDIT_GET_FEATURE1019/* Get which features are 
> > > enabled 
*/
> > > 
> > > +#define AUDIT_REPLACE1020/* 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.

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

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

2015-12-21 Thread 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

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

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

2015-12-21 Thread 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

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

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

2015-12-21 Thread 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

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

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

2015-12-21 Thread 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_SET1017/* Set TTY auditing status */
> > >  #define AUDIT_SET_FEATURE1018/* Turn an audit feature on or 
> > > off 
*/
> > >  #define AUDIT_GET_FEATURE1019/* Get which features are 
> > > enabled 
*/
> > > 
> > > +#define AUDIT_REPLACE1020/* 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.

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

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

2015-12-16 Thread 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 
> ---
>  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_SET1017/* Set TTY auditing status */
>  #define AUDIT_SET_FEATURE1018/* Turn an audit feature on or off */
>  #define AUDIT_GET_FEATURE1019/* Get which features are enabled */
> +#define AUDIT_REPLACE1020/* 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,
> +, 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;

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

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

2015-12-16 Thread 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 
---
 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,
+  , 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

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

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

2015-12-16 Thread 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 
---
 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,
+  , 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

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

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

2015-12-16 Thread 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 
> ---
>  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_SET1017/* Set TTY auditing status */
>  #define AUDIT_SET_FEATURE1018/* Turn an audit feature on or off */
>  #define AUDIT_GET_FEATURE1019/* Get which features are enabled */
> +#define AUDIT_REPLACE1020/* 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,
> +, 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;

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