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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 16 Dec 2015 11:23:19 -0500
From:	Steve Grubb <sgrubb@...hat.com>
To:	Richard Guy Briggs <rgb@...hat.com>
Cc:	linux-audit@...hat.com, linux-kernel@...r.kernel.org,
	pmoore@...hat.com, eparis@...hat.com, v.rathor@...il.com,
	ctcard@...mail.com
Subject: Re: [PATCH V2 1/2] audit: stop an old auditd being starved out by a new auditd

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@...hat.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;

--
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/