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 <[email protected]>
> ---
> 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
--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit
