audit: don't attempt to lookup PIDs when changing PID filtering audit rules

PATCH In review Paul Moore 4 participants 11 messages Dec 15, 2014 Audit

Share & tools

Paul Moore

author

2014-12-15 17:14 UTC #0 lore raw

PATCH 1/1 audit: don't attempt to lookup PIDs when changing PID filtering audit rules

Commit f1dc4867 ("audit: anchor all pid references in the initial pid namespace") introduced a find_vpid() call when adding/removing audit rules with PID/PPID filters; unfortunately this is problematic as find_vpid() only works if there is a task with the associated PID alive on the system. The following commands demonstrate a simple reproducer.

	# auditctl -D
	# auditctl -l
	# autrace /bin/true
	# auditctl -l

This patch resolves the problem by simply using the PID provided by the user without any additional validation, e.g. no calls to check to see if the task/PID exists.

Cc [email protected] # 3.15

Cc Richard Guy Briggs <[email protected]>

Signed-off-by Paul Moore <[email protected]>

---
 kernel/auditfilter.c |   13 -------------
 1 file changed, 13 deletions(-)

kernel/auditfilter.c +0 ~~-13~~

index 8e9bc9c..b2e63ba 100644
@@ -433,19 +433,6 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 			f->val = 0;
 		}
 
-		if ((f->type == AUDIT_PID) || (f->type == AUDIT_PPID)) {
-			struct pid *pid;
-			rcu_read_lock();
-			pid = find_vpid(f->val);
-			if (!pid) {
-				rcu_read_unlock();
-				err = -ESRCH;
-				goto exit_free;
-			}
-			f->val = pid_nr(pid);
-			rcu_read_unlock();
-		}
-
 		err = audit_field_valid(entry, f);
 		if (err)
 			goto exit_free;

Eric Paris

maintainer

2014-12-15 17:29 UTC #1 lore raw

Lets say I and in the non-init pid namespace.

I run audictl -a exit,always -S all -F pid=1

Is the audit system going to show records for what I think is pid=1 or what the initial pid namespace thinks is pid=1 ?

Which is correct? (hint, it's impossible to know pids above my namespace, or even to know what pid the process in question thinks it is, since it could be below my namespace)

I won't pretend this is easy to solve.

Steve et al. What do you think of maybe having pid= rules automatically removed when the pid goes away? I can't think of another way to handle this (although the perf hit might be so stupidly high....)

Quoted 52 lines · Paul Moore

Commit f1dc4867 ("audit: anchor all pid references in the initial pid namespace") introduced a find_vpid() call when adding/removing audit rules with PID/PPID filters; unfortunately this is problematic as find_vpid() only works if there is a task with the associated PID alive on the system. The following commands demonstrate a simple reproducer.
	# auditctl -D
	# auditctl -l
	# autrace /bin/true
	# auditctl -l
This patch resolves the problem by simply using the PID provided by the user without any additional validation, e.g. no calls to check to see if the task/PID exists.
Cc [email protected] # 3.15
Cc Richard Guy Briggs <[email protected]>
Signed-off-by Paul Moore <[email protected]>
---
 kernel/auditfilter.c |   13 -------------
 1 file changed, 13 deletions(-)
kernel/auditfilter.c +0 ~~-14~~
index 8e9bc9c..b2e63ba 100644
@@ -433,19 +433,6 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 			f->val = 0;
 		}
 
-		if ((f->type == AUDIT_PID) || (f->type == AUDIT_PPID)) {
-			struct pid *pid;
-			rcu_read_lock();
-			pid = find_vpid(f->val);
-			if (!pid) {
-				rcu_read_unlock();
-				err = -ESRCH;
-				goto exit_free;
-			}
-			f->val = pid_nr(pid);
-			rcu_read_unlock();
-		}
-
 		err = audit_field_valid(entry, f);
 		if (err)
 			goto exit_free;
 
--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit

Richard Guy Briggs

2014-12-15 18:50 UTC #2 lore raw

Quoted 3 lines · Eric Paris

Lets say I and in the non-init pid namespace.
I run audictl -a exit,always -S all -F pid=1

That's easy (for now). Line 675 of kernel/audit.c in audit_netlink_ok() called from audit_receive_msg() will prevent that with:

	if ((task_active_pid_ns(current) != &init_pid_ns))
		return -EPERM;

Quoted 2 lines

Is the audit system going to show records for what I think is pid=1 or what the initial pid namespace thinks is pid=1 ?

Longer term this will need to be solved if we want to run commands requiring CAP_AUDIT_CONTROL in a container.

I've still got outstanding patches to store PIDs as struct pid rather than pid_t, so this was part of the motivation to start that in this code.

Quoted 5 lines

Which is correct? (hint, it's impossible to know pids above my namespace, or even to know what pid the process in question thinks it is, since it could be below my namespace)
I won't pretend this is easy to solve.

At the moment, this patch will solve this problem. It is also arguably more necessary on AUDIT_ADD_RULE than for AUDIT_DEL_RULE.

Quoted 3 lines

Steve et al. What do you think of maybe having pid= rules automatically removed when the pid goes away? I can't think of another way to handle this (although the perf hit might be so stupidly high....)

That makes sense, as the same is done for paths that vanish.

Quoted 49 lines · Paul Moore

Quoted 48 lines
Commit f1dc4867 ("audit: anchor all pid references in the initial pid namespace") introduced a find_vpid() call when adding/removing audit rules with PID/PPID filters; unfortunately this is problematic as find_vpid() only works if there is a task with the associated PID alive on the system. The following commands demonstrate a simple reproducer.
	# auditctl -D
	# auditctl -l
	# autrace /bin/true
	# auditctl -l
This patch resolves the problem by simply using the PID provided by the user without any additional validation, e.g. no calls to check to see if the task/PID exists.
Cc [email protected] # 3.15
Cc Richard Guy Briggs <[email protected]>
Signed-off-by Paul Moore <[email protected]>
---
 kernel/auditfilter.c |   13 -------------
 1 file changed, 13 deletions(-)
kernel/auditfilter.c +0 ~~-13~~
index 8e9bc9c..b2e63ba 100644
@@ -433,19 +433,6 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
 			f->val = 0;
 		}
 
-		if ((f->type == AUDIT_PID) || (f->type == AUDIT_PPID)) {
-			struct pid *pid;
-			rcu_read_lock();
-			pid = find_vpid(f->val);
-			if (!pid) {
-				rcu_read_unlock();
-				err = -ESRCH;
-				goto exit_free;
-			}
-			f->val = pid_nr(pid);
-			rcu_read_unlock();
-		}
-
 		err = audit_field_valid(entry, f);
 		if (err)
 			goto exit_free;

Quoted 6 lines

Quoted 4 lines
--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Eric Paris

maintainer

2014-12-15 18:51 UTC #3 lore raw

Quoted 13 lines · Richard Guy Briggs

Quoted 3 lines · Eric Paris
Lets say I and in the non-init pid namespace.
I run audictl -a exit,always -S all -F pid=1
That's easy (for now). Line 675 of kernel/audit.c in audit_netlink_ok() called from audit_receive_msg() will prevent that with:
	if ((task_active_pid_ns(current) != &init_pid_ns))
		return -EPERM;
Quoted 2 lines
Is the audit system going to show records for what I think is pid=1 or what the initial pid namespace thinks is pid=1 ?

ACK from me then.

Paul Moore

author

2014-12-15 19:15 UTC #4 lore raw

Quoted 11 lines · Eric Paris

Quoted 10 lines · Richard Guy Briggs
Quoted 3 lines · Eric Paris
Lets say I and in the non-init pid namespace.
I run audictl -a exit,always -S all -F pid=1
That's easy (for now). Line 675 of kernel/audit.c in audit_netlink_ok()
called from audit_receive_msg() will prevent that with:
	if ((task_active_pid_ns(current) != &init_pid_ns))

Quoted 6 lines

Quoted 4 lines
		return -EPERM;
Quoted 2 lines
Is the audit system going to show records for what I think is pid=1 or what the initial pid namespace thinks is pid=1 ?
ACK from me then.

Okay, thanks. Anybody else want to jump on the Ack/Review bandwagon?

Signature · 2 lines

paul moore
security and virtualization @ redhat

Richard Guy Briggs

2014-12-15 19:33 UTC #5 lore raw

Quoted 12 lines · Paul Moore

Quoted 11 lines · Eric Paris
Quoted 10 lines · Richard Guy Briggs
Quoted 3 lines · Eric Paris
Lets say I and in the non-init pid namespace.
I run audictl -a exit,always -S all -F pid=1
That's easy (for now). Line 675 of kernel/audit.c in audit_netlink_ok()
called from audit_receive_msg() will prevent that with:
	if ((task_active_pid_ns(current) != &init_pid_ns))

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
		return -EPERM;
Quoted 2 lines
Is the audit system going to show records for what I think is pid=1 or what the initial pid namespace thinks is pid=1 ?
ACK from me then.
Okay, thanks. Anybody else want to jump on the Ack/Review bandwagon?

Guess I should have added some text about that... Add whichever you feel is most appropriate (Ack/Review/Signed...)

Quoted 1 line

paul moore

- RGB

Paul Moore

author

2014-12-15 19:58 UTC #6 lore raw

Quoted 20 lines · Richard Guy Briggs

Quoted 19 lines · Paul Moore
Quoted 18 lines · Eric Paris
Quoted 16 lines · Richard Guy Briggs
Quoted 3 lines · Eric Paris
Lets say I and in the non-init pid namespace.
I run audictl -a exit,always -S all -F pid=1
That's easy (for now). Line 675 of kernel/audit.c in audit_netlink_ok()
called from audit_receive_msg() will prevent that with:
	if ((task_active_pid_ns(current) != &init_pid_ns))
	
		return -EPERM;
Quoted 3 lines
Is the audit system going to show records for what I think is pid=1 or
what the initial pid namespace thinks is pid=1 ?

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
ACK from me then.
Okay, thanks. Anybody else want to jump on the Ack/Review bandwagon?
Guess I should have added some text about that... Add whichever you feel is most appropriate (Ack/Review/Signed...)

I'll add your Reviewed-by tag then. Thanks.

I suppose everyone is different, but I *really* like seeing "Reviewed-by" and "Tested-by" tags on a patch since it indicates that someone who is not the patch author has looked at and/or tested the code separately and found it to be good.

To me Acked-by usually just means that the maintainer, or someone important to the effort, gave it a passing glance a said "ok". Ack's are important, but I give a higher weight to the reviewed and tested tags.

Signature · 2 lines

paul moore
security and virtualization @ redhat

Paul Moore

author

2014-12-15 19:14 UTC #7 lore raw

In reply to Richard Guy Briggs

Quoted 3 lines · Richard Guy Briggs

I've still got outstanding patches to store PIDs as struct pid rather than pid_t, so this was part of the motivation to start that in this code.

Funny you mention this, while I was hunting for the root cause of the problem, I had a patch which did just that, adding a pid struct to the audit_field struct. Eventually we will have to go that route as (sadly) a PID is no longer sufficient to identify a process on the system, you need PID+ns. We'll still have the find_pid() problem then, but there are ways around that by being smarter about how we add/delete/store filtering rules.

I opted for the patch I posted here because as you point out we really only work in the init namespace and it didn't muddle the bugfix with new functionality.

Signature · 2 lines

paul moore
security and virtualization @ redhat

Paul Moore

author

2014-12-15 19:03 UTC #8 lore raw

In reply to Eric Paris

Quoted 6 lines · Eric Paris

Lets say I and in the non-init pid namespace.
I run audictl -a exit,always -S all -F pid=1
Is the audit system going to show records for what I think is pid=1 or what the initial pid namespace thinks is pid=1 ?

The initial namespace. If we want the executing task's current namespace we should probably change audit_filter_user_rules().

Quoted 3 lines

Which is correct? (hint, it's impossible to know pids above my namespace, or even to know what pid the process in question thinks it is, since it could be below my namespace)

Heh. I'm sorry, I tend to laugh when I hear the term "correct" during an audit discussion ;)

Steve, Richard, Eric - what do you guys want: initial or current namespace?

Quoted 5 lines

I won't pretend this is easy to solve.
Steve et al. What do you think of maybe having pid= rules automatically removed when the pid goes away? I can't think of another way to handle this (although the perf hit might be so stupidly high....)

I'm personally not super excited about rules disappearing automatically and I also believe that it should be possible to remove a rule regardless (not being able to remove a PID filtering rule due to the status of the associated task is silly).

Signature · 2 lines

paul moore
security and virtualization @ redhat

Steve Grubb

2014-12-15 21:14 UTC #9 lore raw

Quoted 10 lines · Paul Moore

Quoted 6 lines
Lets say I and in the non-init pid namespace.
I run audictl -a exit,always -S all -F pid=1
Is the audit system going to show records for what I think is pid=1 or what the initial pid namespace thinks is pid=1 ?
The initial namespace. If we want the executing task's current namespace we should probably change audit_filter_user_rules().

Quoted 8 lines

Quoted 3 lines
Which is correct? (hint, it's impossible to know pids above my namespace, or even to know what pid the process in question thinks it is, since it could be below my namespace)
Heh. I'm sorry, I tend to laugh when I hear the term "correct" during an audit discussion
Steve, Richard, Eric - what do you guys want: initial or current namespace?

To be clear, this pid name space is normally used in conjunction with containers. We don't want any events from within a container unless we also have an audit name space. Everything inside the container is potentially operating out side the security policy of the system.

So, I'd be fine with them being on the init namespace since we have a lot more work to do for containers. The autrace use case is likely to be the only user of pid in the audit rules because its useless for nearly anything else. The audit by process name feature is what most people will use as soon as its upstreamed.

Thanks,
-Steve

Eric Paris

maintainer

2014-12-15 21:24 UTC #10 lore raw

Quoted 3 lines · Steve Grubb

We don't want any events from within a container unless we also have an audit name space. Everything inside the container is potentially operating out side the security policy of the system.

I am not arguing with any of the substance/meaning of what you intend in any way.

However, every time someone uses the word 'container' they are severely mis-characterizing the problem space. There are no containers. It's even worse to say 'container' than it is to say 'the path.' Containers are a userspace construct made out of numerous disjoint kernel primitives (mainly the numerous namespaces). The kernel does not, can not, and will not every know about a 'container.'

This MUST be a key concept when we think about how to make audit work in a world where people want to use kernel namespaces.