Re: [PATCH 1/2] audit: stop an old auditd being starved out by a new auditd
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(a)redhat.com>
---
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