* Re: auditing kdbus service names
From: Steve Grubb @ 2015-08-12 21:38 UTC (permalink / raw)
To: linux-audit; +Cc: Paul Osmialowski, linux-security-module, selinux
In-Reply-To: <3754565.WlII3JJvve@sifl>
On Wednesday, August 12, 2015 08:40:34 AM Paul Moore wrote:
> Hello all,
>
> I'm currently working on a set of LSM hooks for the new kdbus IPC mechanism
> and one of the things that I believe we will need to add is a new audit
> field for the kdbus service name (very similar to the old fashioned dbus
> service name). I was thinking "kdbus_svc" for the field name, any
> objections?
What was used on the old dbus events?
-Steve
^ permalink raw reply
* Re: [PATCH V10] fixup: audit: implement audit by executable
From: Richard Guy Briggs @ 2015-08-12 15:19 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, linux-kernel, sgrubb, eparis, peter
In-Reply-To: <55367298.NHbID3lDFH@sifl>
On 15/08/12, Paul Moore wrote:
> On Wednesday, August 12, 2015 05:48:48 AM Richard Guy Briggs wrote:
> >
> > Do you plan to push this fix to next?
>
> Patience. Yes, I'll be pushing this to next sometime this week; as usual I'll
> send mail when I do.
Ok, no problem, I'm not rushing. I was unsure what your intentions were
or whether there was more to do to help it happen.
> 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
^ permalink raw reply
* Re: [PATCH V10] fixup: audit: implement audit by executable
From: Paul Moore @ 2015-08-12 14:21 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel, sgrubb, eparis, peter
In-Reply-To: <20150812094848.GA25337@madcap2.tricolour.ca>
On Wednesday, August 12, 2015 05:48:48 AM Richard Guy Briggs wrote:
>
> Do you plan to push this fix to next?
Patience. Yes, I'll be pushing this to next sometime this week; as usual I'll
send mail when I do.
--
paul moore
security @ redhat
^ permalink raw reply
* auditing kdbus service names
From: Paul Moore @ 2015-08-12 12:40 UTC (permalink / raw)
To: linux-audit; +Cc: Paul Osmialowski, linux-security-module, selinux
Hello all,
I'm currently working on a set of LSM hooks for the new kdbus IPC mechanism
and one of the things that I believe we will need to add is a new audit field
for the kdbus service name (very similar to the old fashioned dbus service
name). I was thinking "kdbus_svc" for the field name, any objections?
--
paul moore
security @ redhat
^ permalink raw reply
* Re: [PATCH V10] fixup: audit: implement audit by executable
From: Richard Guy Briggs @ 2015-08-12 9:48 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, linux-kernel, sgrubb, eparis, peter
In-Reply-To: <20150811041504.GC15959@madcap2.tricolour.ca>
On 15/08/11, Richard Guy Briggs wrote:
> On 15/08/10, Paul Moore wrote:
> > On Monday, August 10, 2015 01:29:43 PM Richard Guy Briggs wrote:
> > > On 15/08/10, Paul Moore wrote:
> > > > On Saturday, August 08, 2015 10:20:25 AM Richard Guy Briggs wrote:
> > > > > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> > > > > index 1255dbf..656c7e9 100644
> > > > > --- a/kernel/audit_watch.c
> > > > > +++ b/kernel/audit_watch.c
> > > > > @@ -540,8 +540,14 @@ int audit_dupe_exe(struct audit_krule *new, struct
> > > > > audit_krule *old)
> > > > >
> > > > > int audit_exe_compare(struct task_struct *tsk, struct
> > > > > audit_fsnotify_mark
> > > > >
> > > > > *mark) {
> > > > > - unsigned long ino = tsk->mm->exe_file->f_inode->i_ino;
> > > > > - dev_t dev = tsk->mm->exe_file->f_inode->i_sb->s_dev;
> > > > > -
> > > > > + struct file *exe_file;
> > > > > + unsigned long ino;
> > > > > + dev_t dev;
> > > > > +
> > > > > + rcu_read_lock();
> > > > > + exe_file = rcu_dereference(tsk->mm->exe_file);
> > > >
> > > > This line is triggering a sparse error on my system:
> > > > # make C=1 M=kernel
> > > > ...
> > > >
> > > > CHECK kernel/audit_watch.c
> > > >
> > > > kernel/audit_watch.c:548:20: error: incompatible types in comparison
> > > > expression (different address spaces)
> > >
> > > That's odd. I got this complaint when I had forgotten to add the
> > > rcu_dereference() call, but not in its current state. Mind you, I get
> > > swamped with errors and warnings from all over the system (starting with
> > > ptrace.c, signal.c, exit.c, fork.c, audit.c, ftrace.c,
> > > selinux/netnode.c, ...) when I add
> > > "M=kernel" to my build command, so I start to wonder how valid that
> > > result is or that flag. Where is M=kernel documented?
> >
> > I don't remember anymore, it's something I've been using so long that I've
> > forgotten where I first learned of it. The "M=<X>" flag signals that you only
> > want to build a specific module/directory, e.g. M=kernel builds everything
> > under kernel/ whereas M=security/selinux builds everything under
> > security/selinux.
> >
> > So you don't see this error?
>
> No, I don't, but I see a number of unrelated ones that I will address in time...
Do you plan to push this fix to next?
> > paul moore
>
> - RGB
- 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
^ permalink raw reply
* Re: [PATCH V10] fixup: audit: implement audit by executable
From: Richard Guy Briggs @ 2015-08-11 4:15 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, linux-kernel, sgrubb, eparis, peter
In-Reply-To: <4901537.l1uZvA2rRE@sifl>
On 15/08/10, Paul Moore wrote:
> On Monday, August 10, 2015 01:29:43 PM Richard Guy Briggs wrote:
> > On 15/08/10, Paul Moore wrote:
> > > On Saturday, August 08, 2015 10:20:25 AM Richard Guy Briggs wrote:
> > > > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> > > > index 1255dbf..656c7e9 100644
> > > > --- a/kernel/audit_watch.c
> > > > +++ b/kernel/audit_watch.c
> > > > @@ -540,8 +540,14 @@ int audit_dupe_exe(struct audit_krule *new, struct
> > > > audit_krule *old)
> > > >
> > > > int audit_exe_compare(struct task_struct *tsk, struct
> > > > audit_fsnotify_mark
> > > >
> > > > *mark) {
> > > > - unsigned long ino = tsk->mm->exe_file->f_inode->i_ino;
> > > > - dev_t dev = tsk->mm->exe_file->f_inode->i_sb->s_dev;
> > > > -
> > > > + struct file *exe_file;
> > > > + unsigned long ino;
> > > > + dev_t dev;
> > > > +
> > > > + rcu_read_lock();
> > > > + exe_file = rcu_dereference(tsk->mm->exe_file);
> > >
> > > This line is triggering a sparse error on my system:
> > > # make C=1 M=kernel
> > > ...
> > >
> > > CHECK kernel/audit_watch.c
> > >
> > > kernel/audit_watch.c:548:20: error: incompatible types in comparison
> > > expression (different address spaces)
> >
> > That's odd. I got this complaint when I had forgotten to add the
> > rcu_dereference() call, but not in its current state. Mind you, I get
> > swamped with errors and warnings from all over the system (starting with
> > ptrace.c, signal.c, exit.c, fork.c, audit.c, ftrace.c,
> > selinux/netnode.c, ...) when I add
> > "M=kernel" to my build command, so I start to wonder how valid that
> > result is or that flag. Where is M=kernel documented?
>
> I don't remember anymore, it's something I've been using so long that I've
> forgotten where I first learned of it. The "M=<X>" flag signals that you only
> want to build a specific module/directory, e.g. M=kernel builds everything
> under kernel/ whereas M=security/selinux builds everything under
> security/selinux.
>
> So you don't see this error?
No, I don't, but I see a number of unrelated ones that I will address in time...
> 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
^ permalink raw reply
* Re: [PATCH V10] fixup! audit: add audit by children of executable path
From: Paul Moore @ 2015-08-10 18:45 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel, sgrubb, eparis, peter
In-Reply-To: <20150810165354.GA15959@madcap2.tricolour.ca>
On Monday, August 10, 2015 12:53:54 PM Richard Guy Briggs wrote:
> On 15/08/10, Paul Moore wrote:
> > I'm still not convinced that we need to merge exe child filtering patch so
> > I'm not going to apply this, or your v10 patch, at this point in time.
> > If you want to hold on to the code in case you and/or Steve think you can
> > convince me at a later date, you might as well merge this fixup patch
> > into the v10 patch.
>
> I fixed the known problems and reposted it for reference and public
> archive so that it wasn't buried privately in my tree.
No problem, that's smart. I just wanted to respond so that there wasn't any
confusion about the status of this patch; I figure everybody who takes the
time to post a patch deserves a response, even if it is "no thanks".
> Aside from the need for the child filtering patch, do the RCU treatments
> here look reasonable?
Yep, it looks reasonable to me.
--
paul moore
security @ redhat
^ permalink raw reply
* Re: [PATCH V10] fixup: audit: implement audit by executable
From: Paul Moore @ 2015-08-10 18:43 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel, sgrubb, eparis, peter
In-Reply-To: <20150810172943.GB15959@madcap2.tricolour.ca>
On Monday, August 10, 2015 01:29:43 PM Richard Guy Briggs wrote:
> On 15/08/10, Paul Moore wrote:
> > On Saturday, August 08, 2015 10:20:25 AM Richard Guy Briggs wrote:
> > > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> > > index 1255dbf..656c7e9 100644
> > > --- a/kernel/audit_watch.c
> > > +++ b/kernel/audit_watch.c
> > > @@ -540,8 +540,14 @@ int audit_dupe_exe(struct audit_krule *new, struct
> > > audit_krule *old)
> > >
> > > int audit_exe_compare(struct task_struct *tsk, struct
> > > audit_fsnotify_mark
> > >
> > > *mark) {
> > > - unsigned long ino = tsk->mm->exe_file->f_inode->i_ino;
> > > - dev_t dev = tsk->mm->exe_file->f_inode->i_sb->s_dev;
> > > -
> > > + struct file *exe_file;
> > > + unsigned long ino;
> > > + dev_t dev;
> > > +
> > > + rcu_read_lock();
> > > + exe_file = rcu_dereference(tsk->mm->exe_file);
> >
> > This line is triggering a sparse error on my system:
> > # make C=1 M=kernel
> > ...
> >
> > CHECK kernel/audit_watch.c
> >
> > kernel/audit_watch.c:548:20: error: incompatible types in comparison
> > expression (different address spaces)
>
> That's odd. I got this complaint when I had forgotten to add the
> rcu_dereference() call, but not in its current state. Mind you, I get
> swamped with errors and warnings from all over the system (starting with
> ptrace.c, signal.c, exit.c, fork.c, audit.c, ftrace.c,
> selinux/netnode.c, ...) when I add
> "M=kernel" to my build command, so I start to wonder how valid that
> result is or that flag. Where is M=kernel documented?
I don't remember anymore, it's something I've been using so long that I've
forgotten where I first learned of it. The "M=<X>" flag signals that you only
want to build a specific module/directory, e.g. M=kernel builds everything
under kernel/ whereas M=security/selinux builds everything under
security/selinux.
So you don't see this error?
--
paul moore
security @ redhat
^ permalink raw reply
* Re: [PATCH V10] fixup: audit: implement audit by executable
From: Richard Guy Briggs @ 2015-08-10 17:29 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, linux-kernel, sgrubb, eparis, peter
In-Reply-To: <1471794.hTPSfQy61b@sifl>
On 15/08/10, Paul Moore wrote:
> On Saturday, August 08, 2015 10:20:25 AM Richard Guy Briggs wrote:
> > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> > index 1255dbf..656c7e9 100644
> > --- a/kernel/audit_watch.c
> > +++ b/kernel/audit_watch.c
> > @@ -540,8 +540,14 @@ int audit_dupe_exe(struct audit_krule *new, struct
> > audit_krule *old)
> >
> > int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark
> > *mark) {
> > - unsigned long ino = tsk->mm->exe_file->f_inode->i_ino;
> > - dev_t dev = tsk->mm->exe_file->f_inode->i_sb->s_dev;
> > -
> > + struct file *exe_file;
> > + unsigned long ino;
> > + dev_t dev;
> > +
> > + rcu_read_lock();
> > + exe_file = rcu_dereference(tsk->mm->exe_file);
>
> This line is triggering a sparse error on my system:
>
> # make C=1 M=kernel
> ...
> CHECK kernel/audit_watch.c
> kernel/audit_watch.c:548:20: error: incompatible types in comparison
> expression (different address spaces)
That's odd. I got this complaint when I had forgotten to add the
rcu_dereference() call, but not in its current state. Mind you, I get
swamped with errors and warnings from all over the system (starting with
ptrace.c, signal.c, exit.c, fork.c, audit.c, ftrace.c,
selinux/netnode.c, ...) when I add
"M=kernel" to my build command, so I start to wonder how valid that
result is or that flag. Where is M=kernel documented?
> For the record I'm using gcc v4.9.3 and sparse v0.5.0.
gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-11)
sparse 0.4.4-7.el6
> > + ino = exe_file->f_inode->i_ino;
> > + dev = exe_file->f_inode->i_sb->s_dev;
> > + rcu_read_unlock();
> > return audit_mark_compare(mark, ino, dev);
> > }
>
> --
> paul moore
> security @ redhat
>
- 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
^ permalink raw reply
* Re: [PATCH V10] fixup! audit: add audit by children of executable path
From: Richard Guy Briggs @ 2015-08-10 16:53 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, linux-kernel, sgrubb, eparis, peter
In-Reply-To: <3269736.cyCpyM8Kln@sifl>
On 15/08/10, Paul Moore wrote:
> On Saturday, August 08, 2015 10:23:48 AM Richard Guy Briggs wrote:
> > Adding "C=1 CF=-D__CHECK_ENDIAN__" to enable sparse warnings identified a
> > warning with the
> > [PATCH V9 3/3] audit: add audit by children of executable path
> > patch posted a couple of days ago (and just re-posted due to another fix):
> >
> > kernel/auditsc.c:476:46: warning: dereference of noderef expression
> > kernel/auditsc.c:477:61: warning: dereference of noderef expression
> >
> > task_struct->parent requires RCU locking for access. This fix resolves the
> > two warnings.
> >
> > This patch should be merged with the patch it fixes once the fix is
> > confirmed to be the correct approach since the original patch hasn't been
> > accepted yet.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > My hesitation is that the rcu lock scope is too broad. If the loop were
> > re-structured to hold the rcu_read_lock() and call rcu_dereference() once
> > per iteration, would lock release and retake action cause more overhead?
> >
> > kernel/auditsc.c | 7 +++++--
> > 1 files changed, 5 insertions(+), 2 deletions(-)
>
> I'm still not convinced that we need to merge exe child filtering patch so I'm
> not going to apply this, or your v10 patch, at this point in time. If you
> want to hold on to the code in case you and/or Steve think you can convince me
> at a later date, you might as well merge this fixup patch into the v10 patch.
I fixed the known problems and reposted it for reference and public
archive so that it wasn't buried privately in my tree.
Aside from the need for the child filtering patch, do the RCU treatments
here look reasonable?
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index e1f0da2..3ed043d 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -473,13 +473,16 @@ static int audit_filter_rules(struct task_struct *tsk,
> > {
> > struct task_struct *ptsk;
> >
> > - for (ptsk = tsk; ptsk->parent->pid > 0;
> > - ptsk = find_task_by_pid_ns(ptsk->parent->pid, &init_pid_ns)) {
> > + rcu_read_lock();
> > + for (ptsk = tsk; rcu_dereference(ptsk->parent)->pid > 0;
> > + ptsk = find_task_by_pid_ns(rcu_dereference(ptsk->parent)->pid
> > + , &init_pid_ns)) {
> > if (audit_exe_compare(ptsk, rule->exe)) {
> > ++result;
> > break;
> > }
> > }
> > + rcu_read_unlock();
> > }
> > break;
> > case AUDIT_UID:
>
> 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
^ permalink raw reply
* Re: [PATCH V10] fixup! audit: add audit by children of executable path
From: Paul Moore @ 2015-08-10 16:27 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel, sgrubb, eparis, peter
In-Reply-To: <5d3a6285b9302c3f579e15b589603eb2347ee5c8.1439036885.git.rgb@redhat.com>
On Saturday, August 08, 2015 10:23:48 AM Richard Guy Briggs wrote:
> Adding "C=1 CF=-D__CHECK_ENDIAN__" to enable sparse warnings identified a
> warning with the
> [PATCH V9 3/3] audit: add audit by children of executable path
> patch posted a couple of days ago (and just re-posted due to another fix):
>
> kernel/auditsc.c:476:46: warning: dereference of noderef expression
> kernel/auditsc.c:477:61: warning: dereference of noderef expression
>
> task_struct->parent requires RCU locking for access. This fix resolves the
> two warnings.
>
> This patch should be merged with the patch it fixes once the fix is
> confirmed to be the correct approach since the original patch hasn't been
> accepted yet.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> My hesitation is that the rcu lock scope is too broad. If the loop were
> re-structured to hold the rcu_read_lock() and call rcu_dereference() once
> per iteration, would lock release and retake action cause more overhead?
>
> kernel/auditsc.c | 7 +++++--
> 1 files changed, 5 insertions(+), 2 deletions(-)
I'm still not convinced that we need to merge exe child filtering patch so I'm
not going to apply this, or your v10 patch, at this point in time. If you
want to hold on to the code in case you and/or Steve think you can convince me
at a later date, you might as well merge this fixup patch into the v10 patch.
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index e1f0da2..3ed043d 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -473,13 +473,16 @@ static int audit_filter_rules(struct task_struct *tsk,
> {
> struct task_struct *ptsk;
>
> - for (ptsk = tsk; ptsk->parent->pid > 0;
> - ptsk = find_task_by_pid_ns(ptsk->parent->pid, &init_pid_ns)) {
> + rcu_read_lock();
> + for (ptsk = tsk; rcu_dereference(ptsk->parent)->pid > 0;
> + ptsk = find_task_by_pid_ns(rcu_dereference(ptsk->parent)->pid
> + , &init_pid_ns)) {
> if (audit_exe_compare(ptsk, rule->exe)) {
> ++result;
> break;
> }
> }
> + rcu_read_unlock();
> }
> break;
> case AUDIT_UID:
--
paul moore
security @ redhat
^ permalink raw reply
* Re: [PATCH V10] fixup: audit: implement audit by executable
From: Paul Moore @ 2015-08-10 16:22 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel, sgrubb, eparis, peter
In-Reply-To: <8e78d1dca7d8ecd20d3cbdb9a008950417570866.1439036800.git.rgb@redhat.com>
On Saturday, August 08, 2015 10:20:25 AM Richard Guy Briggs wrote:
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 1255dbf..656c7e9 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -540,8 +540,14 @@ int audit_dupe_exe(struct audit_krule *new, struct
> audit_krule *old)
>
> int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark
> *mark) {
> - unsigned long ino = tsk->mm->exe_file->f_inode->i_ino;
> - dev_t dev = tsk->mm->exe_file->f_inode->i_sb->s_dev;
> -
> + struct file *exe_file;
> + unsigned long ino;
> + dev_t dev;
> +
> + rcu_read_lock();
> + exe_file = rcu_dereference(tsk->mm->exe_file);
This line is triggering a sparse error on my system:
# make C=1 M=kernel
...
CHECK kernel/audit_watch.c
kernel/audit_watch.c:548:20: error: incompatible types in comparison
expression (different address spaces)
For the record I'm using gcc v4.9.3 and sparse v0.5.0.
> + ino = exe_file->f_inode->i_ino;
> + dev = exe_file->f_inode->i_sb->s_dev;
> + rcu_read_unlock();
> return audit_mark_compare(mark, ino, dev);
> }
--
paul moore
security @ redhat
^ permalink raw reply
* sparse bogus unexpected unlock warning? untag_chunk
From: Richard Guy Briggs @ 2015-08-08 14:39 UTC (permalink / raw)
To: linux-audit, linux-kernel; +Cc: Al Viro
After adding "C=1 CF=-D__CHECK_ENDIAN__" to my build script, sparse
produced the following warning:
kernel/audit_tree.c:222:13: warning: context imbalance in 'untag_chunk' - unexpected unlock
It looks like a confused parser to me, because the locks appear to
balance properly from my review.
It doesn't say which lock triggered the warning, hash_lock, or
entry->lock. The hash_lock is locked on call and returns in the same
state. entry->lock looks fine. The fsnotify marks look get/put
balanced too.
Is sparse spewing bogons, or am I?
- 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
^ permalink raw reply
* [PATCH V10] fixup! audit: add audit by children of executable path
From: Richard Guy Briggs @ 2015-08-08 14:23 UTC (permalink / raw)
To: linux-audit, linux-kernel
Cc: Richard Guy Briggs, sgrubb, pmoore, eparis, peter
Adding "C=1 CF=-D__CHECK_ENDIAN__" to enable sparse warnings identified a
warning with the
[PATCH V9 3/3] audit: add audit by children of executable path
patch posted a couple of days ago (and just re-posted due to another fix):
kernel/auditsc.c:476:46: warning: dereference of noderef expression
kernel/auditsc.c:477:61: warning: dereference of noderef expression
task_struct->parent requires RCU locking for access. This fix resolves the two
warnings.
This patch should be merged with the patch it fixes once the fix is confirmed
to be the correct approach since the original patch hasn't been accepted yet.
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
My hesitation is that the rcu lock scope is too broad. If the loop were
re-structured to hold the rcu_read_lock() and call rcu_dereference() once per
iteration, would lock release and retake action cause more overhead?
kernel/auditsc.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index e1f0da2..3ed043d 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -473,13 +473,16 @@ static int audit_filter_rules(struct task_struct *tsk,
{
struct task_struct *ptsk;
- for (ptsk = tsk; ptsk->parent->pid > 0;
- ptsk = find_task_by_pid_ns(ptsk->parent->pid, &init_pid_ns)) {
+ rcu_read_lock();
+ for (ptsk = tsk; rcu_dereference(ptsk->parent)->pid > 0;
+ ptsk = find_task_by_pid_ns(rcu_dereference(ptsk->parent)->pid
+ , &init_pid_ns)) {
if (audit_exe_compare(ptsk, rule->exe)) {
++result;
break;
}
}
+ rcu_read_unlock();
}
break;
case AUDIT_UID:
--
1.7.1
^ permalink raw reply related
* [PATCH V10] audit: add audit by children of executable path
From: Richard Guy Briggs @ 2015-08-08 14:23 UTC (permalink / raw)
To: linux-audit, linux-kernel
Cc: Richard Guy Briggs, sgrubb, pmoore, eparis, peter
This adds the ability to audit the actions of children of a not-yet-running
process.
This is a split-out of a heavily modified version of a patch originally
submitted by Eric Paris with some ideas from Peter Moody.
V10: Always reference task_struct::pid in the initial PID namespace.
Cc: Peter Moody <peter@hda3.com>
Cc: Eric Paris <eparis@redhat.com>
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
include/uapi/linux/audit.h | 1 +
kernel/auditfilter.c | 5 +++++
kernel/auditsc.c | 13 +++++++++++++
3 files changed, 19 insertions(+), 0 deletions(-)
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 5316495..9cad417 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -267,6 +267,7 @@
#define AUDIT_OBJ_GID 110
#define AUDIT_FIELD_COMPARE 111
#define AUDIT_EXE 112
+#define AUDIT_EXE_CHILDREN 113
#define AUDIT_ARG0 200
#define AUDIT_ARG1 (AUDIT_ARG0+1)
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index c662638..802f0cc 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -406,6 +406,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
return -EINVAL;
break;
case AUDIT_EXE:
+ case AUDIT_EXE_CHILDREN:
if (f->op != Audit_equal)
return -EINVAL;
if (entry->rule.listnr != AUDIT_FILTER_EXIT)
@@ -547,6 +548,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
entry->rule.filterkey = str;
break;
case AUDIT_EXE:
+ case AUDIT_EXE_CHILDREN:
if (entry->rule.exe || f->val > PATH_MAX)
goto exit_free;
str = audit_unpack_string(&bufp, &remain, f->val);
@@ -643,6 +645,7 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule)
audit_pack_string(&bufp, krule->filterkey);
break;
case AUDIT_EXE:
+ case AUDIT_EXE_CHILDREN:
data->buflen += data->values[i] =
audit_pack_string(&bufp, audit_mark_path(krule->exe));
break;
@@ -710,6 +713,7 @@ static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
return 1;
break;
case AUDIT_EXE:
+ case AUDIT_EXE_CHILDREN:
/* both paths exist based on above type compare */
if (strcmp(audit_mark_path(a->exe),
audit_mark_path(b->exe)))
@@ -838,6 +842,7 @@ struct audit_entry *audit_dupe_rule(struct audit_krule *old)
new->filterkey = fk;
break;
case AUDIT_EXE:
+ case AUDIT_EXE_CHILDREN:
err = audit_dupe_exe(new, old);
break;
}
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index e9bac2b..e1f0da2 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -469,6 +469,19 @@ static int audit_filter_rules(struct task_struct *tsk,
case AUDIT_EXE:
result = audit_exe_compare(tsk, rule->exe);
break;
+ case AUDIT_EXE_CHILDREN:
+ {
+ struct task_struct *ptsk;
+
+ for (ptsk = tsk; ptsk->parent->pid > 0;
+ ptsk = find_task_by_pid_ns(ptsk->parent->pid, &init_pid_ns)) {
+ if (audit_exe_compare(ptsk, rule->exe)) {
+ ++result;
+ break;
+ }
+ }
+ }
+ break;
case AUDIT_UID:
result = audit_uid_comparator(cred->uid, f->op, f->uid);
break;
--
1.7.1
^ permalink raw reply related
* [PATCH V10] fixup: audit: implement audit by executable
From: Richard Guy Briggs @ 2015-08-08 14:20 UTC (permalink / raw)
To: linux-audit, linux-kernel; +Cc: Richard Guy Briggs
The Intel build-bot detected a sparse warning with with a patch I posted a
couple of days ago that was accepted in the audit/next tree:
Subject: [linux-next:master 6689/6751] kernel/audit_watch.c:543:36: sparse: dereference of noderef expression
Date: Friday, August 07, 2015, 06:57:55 PM
From: kbuild test robot <fengguang.wu@intel.com>
tree: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
head: e6455bc5b91f41f842f30465c9193320f0568707
commit: 2e3a8aeb63e5335d4f837d453787c71bcb479796 [6689/6751] Merge remote- tracking branch 'audit/next'
sparse warnings: (new ones prefixed by >>)
>> kernel/audit_watch.c:543:36: sparse: dereference of noderef expression
kernel/audit_watch.c:544:28: sparse: dereference of noderef expression
34d99af5 Richard Guy Briggs 2015-08-05 541 int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark *mark)
34d99af5 Richard Guy Briggs 2015-08-05 542 {
34d99af5 Richard Guy Briggs 2015-08-05 @543 unsigned long ino = tsk->mm- >exe_file->f_inode->i_ino;
34d99af5 Richard Guy Briggs 2015-08-05 544 dev_t dev = tsk->mm->exe_file- >f_inode->i_sb->s_dev;
:::::: The code at line 543 was first introduced by commit
:::::: 34d99af52ad40bd498ba66970579a5bc1fb1a3bc audit: implement audit by executable
tsk->mm->exe_file requires RCU access. The warning was reproduceable by adding
"C=1 CF=-D__CHECK_ENDIAN__" to the build command, and verified eliminated with
this patch.
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
kernel/audit_watch.c | 12 +++++++++---
1 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 1255dbf..656c7e9 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -540,8 +540,14 @@ int audit_dupe_exe(struct audit_krule *new, struct audit_krule *old)
int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark *mark)
{
- unsigned long ino = tsk->mm->exe_file->f_inode->i_ino;
- dev_t dev = tsk->mm->exe_file->f_inode->i_sb->s_dev;
-
+ struct file *exe_file;
+ unsigned long ino;
+ dev_t dev;
+
+ rcu_read_lock();
+ exe_file = rcu_dereference(tsk->mm->exe_file);
+ ino = exe_file->f_inode->i_ino;
+ dev = exe_file->f_inode->i_sb->s_dev;
+ rcu_read_unlock();
return audit_mark_compare(mark, ino, dev);
}
--
1.7.1
^ permalink raw reply related
* Re: [PATCH V9 3/3] audit: add audit by children of executable path
From: Richard Guy Briggs @ 2015-08-08 5:07 UTC (permalink / raw)
To: Paul Moore; +Cc: Steve Grubb, linux-audit, linux-kernel
In-Reply-To: <CAHC9VhQx9ZP=b7ZkqZMk6vaMFv6PjhigjTw=PR3gUV9K4fFfsQ@mail.gmail.com>
On 15/08/07, Paul Moore wrote:
> On Fri, Aug 7, 2015 at 12:03 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 15/08/07, Paul Moore wrote:
> >> On Friday, August 07, 2015 02:37:15 AM Richard Guy Briggs wrote:
> >> > On 15/08/06, Paul Moore wrote:
> >> >
> >> > > I guess what I'm saying is that I'm not currently convinced that
> >> > > there is enough value in this to offset the risk I feel the loop
> >> > > presents. I understand the use cases that you are mentioning, the
> >> > > are the same as the last time we discussed this, but I'm going to
> >> > > need something better than that.
> >> >
> >> > Can you better describe the loop that concerns you? I don't quite see
> >> > it.
> >>
> >> It would be the only loop in the patch, look at the for loop in
> >> audit_filter_rules() which iterates up the process' parent chain.
> >
> > Sorry, I should reword that... What risk do you see in that loop? It
> > works up the task ancestry tree until it triggers, or hits init for that
> > PID namespace that terminates the loop. Do you see a risk in the
> > numerical pids rolling underneath the loop?
>
> I suppose there is some risk of PID overlap, and while that is a
> concern, it isn't my first.
>
> My main concern is that a malicious user could add an extra level of
> burden to the system by making an absurdly tall process tree and then
> hammer the system with trivial, short lived syscalls. Granted, there
> are userspace limits which would bound the impact to some extent, but
> there is no way to really reduce the risk. You could further put hard
> limits on the loop, but what good would that do? Malicious users
> would just know to blow past that limit before they did their Evil
> Deeds.
>
> I'll say it again; I'm not completely opposed to something like this -
> perhaps in some modified form - but I have yet to see a need for this
> functionality that is great enough to counter the risk.
I am not going to lobby hard for it. I split this part of the patch out
to avoid jeopardizing the acceptability of the rest of the patchset and
to isolate it to make it easier to focus on its issues and apply it
later once they are addressed.
I'll reflect on this concern and see if I can come up with any ways to
minimize this danger. This issue is related to the request to list the
chain of processes back to the first ancestor in each record. You can
make a best effort to record or track the entire chain, but at some
point need to put a limit on it to avoid a DoS, at which point there is
no point in listing the information since it is incomplete. (Too many
"point"s in that last sentence...)
> > I *do* notice that find_task_by_vpid(pid_t) must be replaced with
> > find_task_by_pid_ns(pid_t, &init_pid_ns), since task_struct->pid is
> > always stored in the initial PID namespace.
>
> Another thing that needs to be resolved.
I've already fixed it in my tree:
ptsk = find_task_by_pid_ns(ptsk->parent->pid) &init_pid_ns)
> 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
^ permalink raw reply
* Re: [PATCH V9 3/3] audit: add audit by children of executable path
From: Paul Moore @ 2015-08-07 20:47 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: Steve Grubb, linux-audit, linux-kernel
In-Reply-To: <20150807160341.GB27522@madcap2.tricolour.ca>
On Fri, Aug 7, 2015 at 12:03 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 15/08/07, Paul Moore wrote:
>> On Friday, August 07, 2015 02:37:15 AM Richard Guy Briggs wrote:
>> > On 15/08/06, Paul Moore wrote:
>> >
>> > > I guess what I'm saying is that I'm not currently convinced that
>> > > there is enough value in this to offset the risk I feel the loop
>> > > presents. I understand the use cases that you are mentioning, the
>> > > are the same as the last time we discussed this, but I'm going to
>> > > need something better than that.
>> >
>> > Can you better describe the loop that concerns you? I don't quite see
>> > it.
>>
>> It would be the only loop in the patch, look at the for loop in
>> audit_filter_rules() which iterates up the process' parent chain.
>
> Sorry, I should reword that... What risk do you see in that loop? It
> works up the task ancestry tree until it triggers, or hits init for that
> PID namespace that terminates the loop. Do you see a risk in the
> numerical pids rolling underneath the loop?
I suppose there is some risk of PID overlap, and while that is a
concern, it isn't my first.
My main concern is that a malicious user could add an extra level of
burden to the system by making an absurdly tall process tree and then
hammer the system with trivial, short lived syscalls. Granted, there
are userspace limits which would bound the impact to some extent, but
there is no way to really reduce the risk. You could further put hard
limits on the loop, but what good would that do? Malicious users
would just know to blow past that limit before they did their Evil
Deeds.
I'll say it again; I'm not completely opposed to something like this -
perhaps in some modified form - but I have yet to see a need for this
functionality that is great enough to counter the risk.
> I *do* notice that find_task_by_vpid(pid_t) must be replaced with
> find_task_by_pid_ns(pid_t, &init_pid_ns), since task_struct->pid is
> always stored in the initial PID namespace.
Another thing that needs to be resolved.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH V9 3/3] audit: add audit by children of executable path
From: Richard Guy Briggs @ 2015-08-07 16:03 UTC (permalink / raw)
To: Paul Moore; +Cc: Steve Grubb, linux-audit, linux-kernel
In-Reply-To: <6430018.ENq3ajkyRS@sifl>
On 15/08/07, Paul Moore wrote:
> On Friday, August 07, 2015 02:37:15 AM Richard Guy Briggs wrote:
> > On 15/08/06, Paul Moore wrote:
> >
> > > I guess what I'm saying is that I'm not currently convinced that
> > > there is enough value in this to offset the risk I feel the loop
> > > presents. I understand the use cases that you are mentioning, the
> > > are the same as the last time we discussed this, but I'm going to
> > > need something better than that.
> >
> > Can you better describe the loop that concerns you? I don't quite see
> > it.
>
> It would be the only loop in the patch, look at the for loop in
> audit_filter_rules() which iterates up the process' parent chain.
Sorry, I should reword that... What risk do you see in that loop? It
works up the task ancestry tree until it triggers, or hits init for that
PID namespace that terminates the loop. Do you see a risk in the
numerical pids rolling underneath the loop?
I *do* notice that find_task_by_vpid(pid_t) must be replaced with
find_task_by_pid_ns(pid_t, &init_pid_ns), since task_struct->pid is
always stored in the initial PID namespace.
> 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
^ permalink raw reply
* Re: [PATCH V9 3/3] audit: add audit by children of executable path
From: Paul Moore @ 2015-08-07 14:30 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: Steve Grubb, linux-audit, linux-kernel
In-Reply-To: <20150807063715.GB23800@madcap2.tricolour.ca>
On Friday, August 07, 2015 02:37:15 AM Richard Guy Briggs wrote:
> On 15/08/06, Paul Moore wrote:
>
> > I guess what I'm saying is that I'm not currently convinced that
> > there is enough value in this to offset the risk I feel the loop
> > presents. I understand the use cases that you are mentioning, the
> > are the same as the last time we discussed this, but I'm going to
> > need something better than that.
>
> Can you better describe the loop that concerns you? I don't quite see
> it.
It would be the only loop in the patch, look at the for loop in
audit_filter_rules() which iterates up the process' parent chain.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH V9 2/3] audit: implement audit by executable
From: Paul Moore @ 2015-08-07 14:27 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel, sgrubb, eparis, peter
In-Reply-To: <20150807062411.GB12116@madcap2.tricolour.ca>
On Friday, August 07, 2015 02:25:14 AM Richard Guy Briggs wrote:
> On 15/08/06, Paul Moore wrote:
>
> > Merged, although some more minor whitespace tweaks were necessary for
> > checkpatch. On a related note, if you're not running
> > ./scripts/checlpatch.pl on your patches before sending them out, I would
> > recommend it. It isn't perfect, but it can catch some silly things that
> > we all do from time to time.
>
> No excuses... I have been running it pretty regularly and got lazy and
> distracted with patch revisions. I can't say I agree with the no space
> before closing round parenthesis due to legibility, but will comply.
Okay, glad to hear you run it regularly when submitting patches.
I agree that there are some things I might change about the kernel's style
choices, but I think it is more important that we remain consistent with the
kernel as a whole. I really like tools that enforce things like this, even if
I don't agree 100% with the results.
> > Also, one last thing. It is pretty late in the -rcX cycle to merge these
> > two patches, but considering that we've been talking about these for a
> > while, I'm reasonably okay merging them. In the future, if it isn't in
> > audit#next by the time -rc5 is released, it isn't going to make the merge
> > window.
>
> I've been quite aware of that looming merge window... This feature has
> been iterating for a while, so there are no big surprises. I was aiming
> for earlier. :)
Well, I think it is a little early to say there are no big surprises, we won't
know that for a few more weeks, but if we make it to -rc3/4 without any
problems I'll breathe a bit easier ;)
--
paul moore
security @ redhat
^ permalink raw reply
* Re: [PATCH V4 (was V6)] audit: use macros for unset inode and device values
From: Paul Moore @ 2015-08-07 14:22 UTC (permalink / raw)
To: Casey Schaufler
Cc: Steve Grubb, Richard Guy Briggs, linux-audit, linux-kernel
In-Reply-To: <55C3D24D.7040602@schaufler-ca.com>
On Thursday, August 06, 2015 02:31:57 PM Casey Schaufler wrote:
> I remember the Orange Book days when we were *required* to audit by
> dev/inode because it was the only true way to identify the object. Yes,
> it's analogous to auditing the pid, but we had to audit by that, too. The
> dev/indode and pid are the "true" names. Anything else is a hint at what
> you're looking at. I can easily imaging someone who really cares about the
> audit data supplying the dev/inode and pid.
Just to add a bit of clarity, my original question was if there was any value
in exposing the unset/invalid device and inode values, e.g. -1. While I agree
that there is value in auditing by dev/inode, I can't think of a reasonable
situation where the user would need to pass an unset/invalid device and/or
inode value into the kernel as part of an audit configuration command.
--
paul moore
security @ redhat
^ permalink raw reply
* Re: [PATCH V9 3/3] audit: add audit by children of executable path
From: Richard Guy Briggs @ 2015-08-07 6:37 UTC (permalink / raw)
To: Paul Moore; +Cc: Steve Grubb, Paul Moore, linux-audit, linux-kernel
In-Reply-To: <14f057b6cb8.2852.85c95baa4474aabc7814e68940a78392@paul-moore.com>
On 15/08/06, Paul Moore wrote:
> On August 6, 2015 5:11:50 PM Steve Grubb <sgrubb@redhat.com> wrote:
>
> >On Thursday, August 06, 2015 04:24:58 PM Paul Moore wrote:
> >> On Wednesday, August 05, 2015 04:29:38 PM Richard Guy Briggs wrote:
> >> > This adds the ability to audit the actions of children of a
> >> > not-yet-running
> >> > process.
> >> >
> >> >
> >> >
> >> > This is a split-out of a heavily modified version of a patch originally
> >> > submitted by Eric Paris with some ideas from Peter Moody.
> >> >
> >> >
> >> >
> >> > Cc: Peter Moody <peter@hda3.com>
> >> > Cc: Eric Paris <eparis@redhat.com>
> >> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >> > ---
> >> >
> >> > include/uapi/linux/audit.h | 1 +
> >> > kernel/auditfilter.c | 5 +++++
> >> > kernel/auditsc.c | 11 +++++++++++
> >> > 3 files changed, 17 insertions(+), 0 deletions(-)
> >>
> >> I'm still not really comfortable with that loop and since there hasn't been
> >> a really convincing use case I'm going to pass on this patch for right
> >> now. If someone comes up with a *really* compelling case in the future
> >> I'll reconsider it.
> >
> >Its the same reason strace has a -f option. Sometimes you need to also see
> >what the children did. For example, maybe you want to audit file access to a
> >specific directory and several cgi-bin programs can get there. You could write
> >a rule for apache and be done. Or maybe, you have an app that lets people have
> >shell access and you need to see files accessed or connections opened. Or maybe
> >its a control panel application with helper scripts and you need to see
> >changes that its making. Or maybe you have a program that is at risk of being
> >compromised and you want to see if someone gets a shell from it. There are a
> >lot of cases where it could be useful.
> >
> >-Steve
> >
> >--
> >Linux-audit mailing list
> >Linux-audit@redhat.com
> >https://www.redhat.com/mailman/listinfo/linux-audit
>
> I guess what I'm saying is that I'm not currently convinced that
> there is enough value in this to offset the risk I feel the loop
> presents. I understand the use cases that you are mentioning, the
> are the same as the last time we discussed this, but I'm going to
> need something better than that.
Can you better describe the loop that concerns you? I don't quite see
it.
> 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
^ permalink raw reply
* Re: [PATCH V9 2/3] audit: implement audit by executable
From: Richard Guy Briggs @ 2015-08-07 6:25 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, linux-kernel
In-Reply-To: <42326522.RDQEUGoptm@sifl>
On 15/08/06, Paul Moore wrote:
> On Wednesday, August 05, 2015 04:29:37 PM Richard Guy Briggs wrote:
> > This adds the ability audit the actions of a not-yet-running process.
> >
> > This patch implements the ability to filter on the executable path. Instead
> > of just hard coding the ino and dev of the executable we care about at the
> > moment the rule is inserted into the kernel, use the new audit_fsnotify
> > infrastructure to manage this dynamically. This means that if the filename
> > does not yet exist but the containing directory does, or if the inode in
> > question is unlinked and creat'd (aka updated) the rule will just continue
> > to work. If the containing directory is moved or deleted or the filesystem
> > is unmounted, the rule is deleted automatically. A future enhancement
> > would be to have the rule survive across directory disruptions.
> >
> > This is a heavily modified version of a patch originally submitted by Eric
> > Paris with some ideas from Peter Moody.
> >
> > Cc: Peter Moody <peter@hda3.com>
> > Cc: Eric Paris <eparis@redhat.com>
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > include/linux/audit.h | 1 +
> > include/uapi/linux/audit.h | 5 +++-
> > kernel/audit.h | 4 +++
> > kernel/audit_tree.c | 2 +
> > kernel/audit_watch.c | 31 +++++++++++++++++++++++++
> > kernel/auditfilter.c | 53 ++++++++++++++++++++++++++++++++++++++++-
> > kernel/auditsc.c | 3 ++
> > 7 files changed, 97 insertions(+), 2 deletions(-)
>
> Merged, although some more minor whitespace tweaks were necessary for
> checkpatch. On a related note, if you're not running ./scripts/checlpatch.pl
> on your patches before sending them out, I would recommend it. It isn't
> perfect, but it can catch some silly things that we all do from time to time.
No excuses... I have been running it pretty regularly and got lazy and
distracted with patch revisions. I can't say I agree with the no space
before closing round parenthesis due to legibility, but will comply.
> Also, one last thing. It is pretty late in the -rcX cycle to merge these two
> patches, but considering that we've been talking about these for a while, I'm
> reasonably okay merging them. In the future, if it isn't in audit#next by the
> time -rc5 is released, it isn't going to make the merge window.
I've been quite aware of that looming merge window... This feature has
been iterating for a while, so there are no big surprises. I was aiming
for earlier. :)
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index c2e7e3a..aee456f 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -59,6 +59,7 @@ struct audit_krule {
> > struct audit_field *inode_f; /* quick access to an inode field */
> > struct audit_watch *watch; /* associated watch */
> > struct audit_tree *tree; /* associated watched tree */
> > + struct audit_fsnotify_mark *exe;
> > struct list_head rlist; /* entry in audit_{watch,tree}.rules list */
> > struct list_head list; /* for AUDIT_LIST* purposes only */
> > u64 prio;
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 971df22..e2ca600 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -266,6 +266,7 @@
> > #define AUDIT_OBJ_UID 109
> > #define AUDIT_OBJ_GID 110
> > #define AUDIT_FIELD_COMPARE 111
> > +#define AUDIT_EXE 112
> >
> > #define AUDIT_ARG0 200
> > #define AUDIT_ARG1 (AUDIT_ARG0+1)
> > @@ -324,8 +325,10 @@ enum {
> >
> > #define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT 0x00000001
> > #define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002
> > +#define AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH 0x00000004
> > #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
> > - AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME)
> > + AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
> > + AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH )
> >
> > /* deprecated: AUDIT_VERSION_* */
> > #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index 46d10dd..dadf86a 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -277,6 +277,8 @@ extern char *audit_mark_path(struct audit_fsnotify_mark
> > *mark); extern void audit_remove_mark(struct audit_fsnotify_mark
> > *audit_mark); extern void audit_remove_mark_rule(struct audit_krule
> > *krule);
> > extern int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned
> > long ino, dev_t dev); +extern int audit_dupe_exe(struct audit_krule *new,
> > struct audit_krule *old); +extern int audit_exe_compare(struct task_struct
> > *tsk, struct audit_fsnotify_mark *mark);
> >
> > #else
> > #define audit_put_watch(w) {}
> > @@ -292,6 +294,8 @@ extern int audit_mark_compare(struct audit_fsnotify_mark
> > *mark, unsigned long in #define audit_remove_mark(m)
> > #define audit_remove_mark_rule(k)
> > #define audit_mark_compare(m, i, d) 0
> > +#define audit_exe_compare(t, m) (-EINVAL)
> > +#define audit_dupe_exe(n, o) (-EINVAL)
> > #endif /* CONFIG_AUDIT_WATCH */
> >
> > #ifdef CONFIG_AUDIT_TREE
> > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> > index b0f9877..94ecdab 100644
> > --- a/kernel/audit_tree.c
> > +++ b/kernel/audit_tree.c
> > @@ -479,6 +479,8 @@ static void kill_rules(struct audit_tree *tree)
> > if (rule->tree) {
> > /* not a half-baked one */
> > audit_tree_log_remove_rule(rule);
> > + if (entry->rule.exe)
> > + audit_remove_mark(entry->rule.exe);
> > rule->tree = NULL;
> > list_del_rcu(&entry->list);
> > list_del(&entry->rule.list);
> > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> > index c668bfc..1255dbf 100644
> > --- a/kernel/audit_watch.c
> > +++ b/kernel/audit_watch.c
> > @@ -312,6 +312,8 @@ static void audit_update_watch(struct audit_parent
> > *parent, list_replace(&oentry->rule.list,
> > &nentry->rule.list);
> > }
> > + if (oentry->rule.exe)
> > + audit_remove_mark(oentry->rule.exe);
> >
> > audit_watch_log_rule_change(r, owatch, "updated_rules");
> >
> > @@ -342,6 +344,8 @@ static void audit_remove_parent_watches(struct
> > audit_parent *parent) list_for_each_entry_safe(r, nextr, &w->rules, rlist)
> > {
> > e = container_of(r, struct audit_entry, rule);
> > audit_watch_log_rule_change(r, w, "remove_rule");
> > + if (e->rule.exe)
> > + audit_remove_mark(e->rule.exe);
> > list_del(&r->rlist);
> > list_del(&r->list);
> > list_del_rcu(&e->list);
> > @@ -514,3 +518,30 @@ static int __init audit_watch_init(void)
> > return 0;
> > }
> > device_initcall(audit_watch_init);
> > +
> > +int audit_dupe_exe(struct audit_krule *new, struct audit_krule *old)
> > +{
> > + struct audit_fsnotify_mark *audit_mark;
> > + char *pathname;
> > +
> > + pathname = kstrdup(audit_mark_path(old->exe), GFP_KERNEL);
> > + if (!pathname)
> > + return -ENOMEM;
> > +
> > + audit_mark = audit_alloc_mark(new, pathname, strlen(pathname));
> > + if (IS_ERR(audit_mark)) {
> > + kfree(pathname);
> > + return PTR_ERR(audit_mark);
> > + }
> > + new->exe = audit_mark;
> > +
> > + return 0;
> > +}
> > +
> > +int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark
> > *mark) +{
> > + unsigned long ino = tsk->mm->exe_file->f_inode->i_ino;
> > + dev_t dev = tsk->mm->exe_file->f_inode->i_sb->s_dev;
> > +
> > + return audit_mark_compare(mark, ino, dev);
> > +}
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 3d99196..c662638 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -405,6 +405,12 @@ static int audit_field_valid(struct audit_entry *entry,
> > struct audit_field *f) if (f->val > AUDIT_MAX_FIELD_COMPARE)
> > return -EINVAL;
> > break;
> > + case AUDIT_EXE:
> > + if (f->op != Audit_equal)
> > + return -EINVAL;
> > + if (entry->rule.listnr != AUDIT_FILTER_EXIT)
> > + return -EINVAL;
> > + break;
> > };
> > return 0;
> > }
> > @@ -419,6 +425,7 @@ static struct audit_entry *audit_data_to_entry(struct
> > audit_rule_data *data, size_t remain = datasz - sizeof(struct
> > audit_rule_data);
> > int i;
> > char *str;
> > + struct audit_fsnotify_mark *audit_mark;
> >
> > entry = audit_to_entry_common(data);
> > if (IS_ERR(entry))
> > @@ -539,6 +546,24 @@ static struct audit_entry *audit_data_to_entry(struct
> > audit_rule_data *data, entry->rule.buflen += f->val;
> > entry->rule.filterkey = str;
> > break;
> > + case AUDIT_EXE:
> > + if (entry->rule.exe || f->val > PATH_MAX)
> > + goto exit_free;
> > + str = audit_unpack_string(&bufp, &remain, f->val);
> > + if (IS_ERR(str)) {
> > + err = PTR_ERR(str);
> > + goto exit_free;
> > + }
> > + entry->rule.buflen += f->val;
> > +
> > + audit_mark = audit_alloc_mark(&entry->rule, str, f->val);
> > + if (IS_ERR(audit_mark)) {
> > + kfree(str);
> > + err = PTR_ERR(audit_mark);
> > + goto exit_free;
> > + }
> > + entry->rule.exe = audit_mark;
> > + break;
> > }
> > }
> >
> > @@ -551,6 +576,8 @@ exit_nofree:
> > exit_free:
> > if (entry->rule.tree)
> > audit_put_tree(entry->rule.tree); /* that's the temporary one */
> > + if (entry->rule.exe)
> > + audit_remove_mark(entry->rule.exe); /* that's the template one */
> > audit_free_rule(entry);
> > return ERR_PTR(err);
> > }
> > @@ -615,6 +642,10 @@ static struct audit_rule_data
> > *audit_krule_to_data(struct audit_krule *krule) data->buflen +=
> > data->values[i] =
> > audit_pack_string(&bufp, krule->filterkey);
> > break;
> > + case AUDIT_EXE:
> > + data->buflen += data->values[i] =
> > + audit_pack_string(&bufp, audit_mark_path(krule->exe));
> > + break;
> > case AUDIT_LOGINUID_SET:
> > if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val) {
> > data->fields[i] = AUDIT_LOGINUID;
> > @@ -678,6 +709,12 @@ static int audit_compare_rule(struct audit_krule *a,
> > struct audit_krule *b) if (strcmp(a->filterkey, b->filterkey))
> > return 1;
> > break;
> > + case AUDIT_EXE:
> > + /* both paths exist based on above type compare */
> > + if (strcmp(audit_mark_path(a->exe),
> > + audit_mark_path(b->exe)))
> > + return 1;
> > + break;
> > case AUDIT_UID:
> > case AUDIT_EUID:
> > case AUDIT_SUID:
> > @@ -799,8 +836,14 @@ struct audit_entry *audit_dupe_rule(struct audit_krule
> > *old) err = -ENOMEM;
> > else
> > new->filterkey = fk;
> > + break;
> > + case AUDIT_EXE:
> > + err = audit_dupe_exe(new, old);
> > + break;
> > }
> > if (err) {
> > + if (new->exe)
> > + audit_remove_mark(new->exe);
> > audit_free_rule(entry);
> > return ERR_PTR(err);
> > }
> > @@ -963,6 +1006,9 @@ int audit_del_rule(struct audit_entry *entry)
> > if (e->rule.tree)
> > audit_remove_tree_rule(&e->rule);
> >
> > + if (e->rule.exe)
> > + audit_remove_mark_rule(&e->rule);
> > +
> > #ifdef CONFIG_AUDITSYSCALL
> > if (!dont_count)
> > audit_n_rules--;
> > @@ -1067,8 +1113,11 @@ int audit_rule_change(int type, __u32 portid, int
> > seq, void *data, WARN_ON(1);
> > }
> >
> > - if (err || type == AUDIT_DEL_RULE)
> > + if (err || type == AUDIT_DEL_RULE) {
> > + if (entry->rule.exe)
> > + audit_remove_mark(entry->rule.exe);
> > audit_free_rule(entry);
> > + }
> >
> > return err;
> > }
> > @@ -1360,6 +1409,8 @@ static int update_lsm_rule(struct audit_krule *r)
> > return 0;
> >
> > nentry = audit_dupe_rule(r);
> > + if (entry->rule.exe)
> > + audit_remove_mark(entry->rule.exe);
> > if (IS_ERR(nentry)) {
> > /* save the first error encountered for the
> > * return value */
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 701ea5c..e9bac2b 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -466,6 +466,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> > result = audit_comparator(ctx->ppid, f->op, f->val);
> > }
> > break;
> > + case AUDIT_EXE:
> > + result = audit_exe_compare(tsk, rule->exe);
> > + break;
> > case AUDIT_UID:
> > result = audit_uid_comparator(cred->uid, f->op, f->uid);
> > break;
>
> --
> paul moore
> security @ redhat
>
- 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
^ permalink raw reply
* Re: [PATCH V9 3/3] audit: add audit by children of executable path
From: Paul Moore @ 2015-08-07 0:07 UTC (permalink / raw)
To: Steve Grubb, Paul Moore; +Cc: Richard Guy Briggs, linux-audit, linux-kernel
In-Reply-To: <18689423.1lXpkUPLpg@x2>
On August 6, 2015 5:11:50 PM Steve Grubb <sgrubb@redhat.com> wrote:
> On Thursday, August 06, 2015 04:24:58 PM Paul Moore wrote:
> > On Wednesday, August 05, 2015 04:29:38 PM Richard Guy Briggs wrote:
> > > This adds the ability to audit the actions of children of a
> > > not-yet-running
> > > process.
> > >
> > >
> > >
> > > This is a split-out of a heavily modified version of a patch originally
> > > submitted by Eric Paris with some ideas from Peter Moody.
> > >
> > >
> > >
> > > Cc: Peter Moody <peter@hda3.com>
> > > Cc: Eric Paris <eparis@redhat.com>
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > >
> > > include/uapi/linux/audit.h | 1 +
> > > kernel/auditfilter.c | 5 +++++
> > > kernel/auditsc.c | 11 +++++++++++
> > > 3 files changed, 17 insertions(+), 0 deletions(-)
> >
> > I'm still not really comfortable with that loop and since there hasn't been
> > a really convincing use case I'm going to pass on this patch for right
> > now. If someone comes up with a *really* compelling case in the future
> > I'll reconsider it.
>
> Its the same reason strace has a -f option. Sometimes you need to also see
> what the children did. For example, maybe you want to audit file access to a
> specific directory and several cgi-bin programs can get there. You could write
> a rule for apache and be done. Or maybe, you have an app that lets people have
> shell access and you need to see files accessed or connections opened. Or maybe
> its a control panel application with helper scripts and you need to see
> changes that its making. Or maybe you have a program that is at risk of being
> compromised and you want to see if someone gets a shell from it. There are a
> lot of cases where it could be useful.
>
> -Steve
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
I guess what I'm saying is that I'm not currently convinced that there is
enough value in this to offset the risk I feel the loop presents. I
understand the use cases that you are mentioning, the are the same as the
last time we discussed this, but I'm going to need something better than that.
--
paul moore
www.paul-moore.com
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox