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

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

[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: stable@vger.kernel.org # 3.15
Cc: Richard Guy Briggs <rgb@redhat.com>
Signed-off-by: Paul Moore <pmoore@redhat.com>
---
 kernel/auditfilter.c |   13 -------------
 1 file changed, 13 deletions(-)

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 8e9bc9c..b2e63ba 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -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;

Re: [PATCH] audit: don't attempt to lookup PIDs when changing PID filtering audit rules

[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 ?

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....)

On Mon, 2014-12-15 at 12:14 -0500, Paul Moore wrote:
> 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: stable@vger.kernel.org # 3.15
> Cc: Richard Guy Briggs <rgb@redhat.com>
> Signed-off-by: Paul Moore <pmoore@redhat.com>
> ---
>  kernel/auditfilter.c |   13 -------------
>  1 file changed, 13 deletions(-)
> 
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 8e9bc9c..b2e63ba 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -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
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH] audit: don't attempt to lookup PIDs when changing PID filtering audit rules

[Richard Guy Briggs]

On 14/12/15, Eric Paris wrote:
> 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;

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

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

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

> On Mon, 2014-12-15 at 12:14 -0500, Paul Moore wrote:
> > 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: stable@vger.kernel.org # 3.15
> > Cc: Richard Guy Briggs <rgb@redhat.com>
> > Signed-off-by: Paul Moore <pmoore@redhat.com>
> > ---
> >  kernel/auditfilter.c |   13 -------------
> >  1 file changed, 13 deletions(-)
> > 
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 8e9bc9c..b2e63ba 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -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
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
> 
> 

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
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

Re: [PATCH] audit: don't attempt to lookup PIDs when changing PID filtering audit rules

[Eric Paris]

On Mon, 2014-12-15 at 13:50 -0500, Richard Guy Briggs wrote:
> On 14/12/15, Eric Paris wrote:
> > 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;
> 
> > 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.

Re: [PATCH] audit: don't attempt to lookup PIDs when changing PID filtering audit rules

[Paul Moore]

On Monday, December 15, 2014 12:29:52 PM Eric Paris wrote:
> 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().

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

> 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).

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] audit: don't attempt to lookup PIDs when changing PID filtering audit rules

[Paul Moore]

On Monday, December 15, 2014 01:50:57 PM Richard Guy Briggs wrote:
> 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.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] audit: don't attempt to lookup PIDs when changing PID filtering audit rules

[Paul Moore]

On Monday, December 15, 2014 01:51:52 PM Eric Paris wrote:
> On Mon, 2014-12-15 at 13:50 -0500, Richard Guy Briggs wrote:
> > On 14/12/15, Eric Paris wrote:
> > > 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;
> > 		
> > > 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?

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] audit: don't attempt to lookup PIDs when changing PID filtering audit rules

[Richard Guy Briggs]

On 14/12/15, Paul Moore wrote:
> On Monday, December 15, 2014 01:51:52 PM Eric Paris wrote:
> > On Mon, 2014-12-15 at 13:50 -0500, Richard Guy Briggs wrote:
> > > On 14/12/15, Eric Paris wrote:
> > > > 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;
> > > 		
> > > > 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...)

> paul moore

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
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

Re: [PATCH] audit: don't attempt to lookup PIDs when changing PID filtering audit rules

[Paul Moore]

On Monday, December 15, 2014 02:33:36 PM Richard Guy Briggs wrote:
> On 14/12/15, Paul Moore wrote:
> > On Monday, December 15, 2014 01:51:52 PM Eric Paris wrote:
> > > On Mon, 2014-12-15 at 13:50 -0500, Richard Guy Briggs wrote:
> > > > On 14/12/15, Eric Paris wrote:
> > > > > 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;
> > > > 		
> > > > > 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...)

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.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] audit: don't attempt to lookup PIDs when changing PID filtering audit rules

[Steve Grubb]

On Monday, December 15, 2014 02:03:05 PM Paul Moore wrote:
> > 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().
> 
> > 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

Re: [PATCH] audit: don't attempt to lookup PIDs when changing PID filtering audit rules

[Eric Paris]

On Mon, 2014-12-15 at 16:14 -0500, Steve Grubb wrote:
> 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.