Re: [RFC PATCH ghak21 4/4] audit: add parent of refused symlink to audit_names

2018-02-16 Thread Paul Moore 
On Thu, Feb 15, 2018 at 9:59 PM, Richard Guy Briggs  wrote:
> On 2018-02-15 18:34, Paul Moore wrote:
>> On Wed, Feb 14, 2018 at 11:18 AM, Richard Guy Briggs  wrote:
>> > Audit link denied events for symlinks were missing the parent PATH
>> > record.  Add it.  Since the full pathname may not be available,
>> > reconstruct it from the path in the nameidata supplied.
>> >
>> > See: https://github.com/linux-audit/audit-kernel/issues/21
>> > Signed-off-by: Richard Guy Briggs 
>> > ---
>> >  fs/namei.c | 9 +
>> >  1 file changed, 9 insertions(+)
>> >
>> > diff --git a/fs/namei.c b/fs/namei.c
>> > index 0edf133..bf1c046b 100644
>> > --- a/fs/namei.c
>> > +++ b/fs/namei.c
>> > @@ -923,6 +923,7 @@ static inline int may_follow_link(struct nameidata *nd)
>> > const struct inode *inode;
>> > const struct inode *parent;
>> > kuid_t puid;
>> > +   char *pathname;
>> >
>> > if (!sysctl_protected_symlinks)
>> > return 0;
>> > @@ -945,6 +946,14 @@ static inline int may_follow_link(struct nameidata 
>> > *nd)
>> > if (nd->flags & LOOKUP_RCU)
>> > return -ECHILD;
>> >
>> > +   pathname = kmalloc(PATH_MAX + 1, GFP_KERNEL);
>> > +   if (!pathname)
>> > +   return -ENOMEM;
>> > +   audit_inode(getname_kernel(d_absolute_path(>stack[0].link, 
>> > pathname,
>> > +  PATH_MAX + 1)),
>> > +   nd->stack[0].link.dentry, 0);
>>
>> Hmm, it's been a while since I've looked at the audit vfs/inode code,
>> but isn't the audit_inode() call directly above effectively a
>> duplicate of the audit_inode(nd->name, nd->stack[0].link.dentry, 0)
>> call you added in patch 3/4?
>
> It gets the full pathname string of the link, then converts it to a
> struct filename*, and then below, we feed it that struct filename* with
> the parent's dentry and the LOOKUP_PARENT flag to drop the last part of
> the pathname and set the filetype to PARENT.

I think that last bit is what I was forgetting, thanks.

> I didn't try, it, but I expect if I feed it parent's dentry above, I'd
> get only the parent pathname and when I feed it to audit_inode() below
> it would again drop the last part of the pathname when the LOOKUP_PARENT
> flag is included, or not label it as a filetype PARENT if we don't
> include the flag to get the full parent pathname.
>
>> > +   audit_inode(nd->name, nd->stack[0].link.dentry->d_parent, 
>> > LOOKUP_PARENT);
>> > +
>> > audit_inode(nd->name, nd->stack[0].link.dentry, 0);
>> > audit_log_link_denied("follow_link", >stack[0].link);
>> > return -EACCES;
>> > --
>> > 1.8.3.1

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH ghak8 ALT4 V4 0/3] audit: show more information for entries with anonymous parents

2018-02-16 Thread Paul Moore 
On Fri, Feb 16, 2018 at 3:23 AM, Richard Guy Briggs  wrote:
> On 2018-02-15 17:15, Paul Moore wrote:
>> On Mon, Feb 12, 2018 at 12:02 AM, Richard Guy Briggs  wrote:
>> > More than one filesystem was causing hundreds to thousands of null PATH
>> > records to be associated with the *init_module SYSCALL records on a few
>> > modules with corresponding audit syscall rules.
>> >
>> > This patchset adds extra information to those PATH records to provide
>> > insight into what is generating them, including a partial pathname,
>> > fstype field, and two new filetypes that indicate the pathname isn't
>> > anchored at the root of the task's root filesystem.
>> >
>> > Richard Guy Briggs (3):
>> >   audit: show partial pathname for entries with anonymous parents
>> >   audit: append new fstype field for anonymous PATH records
>> >   audit: add new filetypes CREATE_ANON and PARENT_ANON
>>
>> The more I look at this, the more I prefer your original approach that
>> prefixed the relative pathname with the fstype.  Yes, I do realize
>> that you sort of work around that by including the fstype as a new
>> field in the PATH records, but we're still stuck with those odd
>> relative/un-rooted name fields.
>
> They are signalled as being unrooted by the ANON filetypes.  And now
> that you mention it, should fail the ausearch-test since it isn't a "full
> path", as claimed is necessary in ghak70 (so I don't see why the
> KERN_MODULE name= record/field fails that test).

Yes.  I still prefer your original approach.

>> Further, I don't recall ever hearing a good reason why the original
>> approach wasn't acceptable to Steve's userspace.  I know he did make
>> some very last minute hand-wavy comments, but none of those made any
>> sense to me; I don't understand why Steve's audit record parser is
>> even looking in the pathname string.
>>
>> I'm going to park these patches in limbo for the time being.
>
> Can you give me an idea how long that might be?

If you need an answer right now, consider it to be "indefinitely".

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[PATCH V2] audit: remove arch_f pointer from struct audit_krule

2018-02-16 Thread Richard Guy Briggs 
In the process of trying to track down a potential bug altering the
registered arch for a syscall rule, I propose this simplification of
struct audit_krule that removes an unnecessary member.

The arch_f pointer was added to the struct audit_krule in commit:
e54dc2431d740a79a6bd013babade99d71b1714f ("audit signal recipients")

This is only used on addition and deletion of rules which isn't time
critical and the arch field is the first field if it is present at all,
easily found iterating over the field type.  This isn't worth the
additional complexity and storage.  Delete the field.

Passes audit-testsuite.
Signed-off-by: Richard Guy Briggs 
---
 include/linux/audit.h |  1 -
 kernel/auditfilter.c  | 12 
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index af410d9..64a3b0e 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -58,7 +58,6 @@ struct audit_krule {
u32 field_count;
char*filterkey; /* ties events to rules */
struct audit_field  *fields;
-   struct audit_field  *arch_f; /* quick access to arch field */
struct audit_field  *inode_f; /* quick access to an inode field */
struct audit_watch  *watch; /* associated watch */
struct audit_tree   *tree;  /* associated watched tree */
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 739a6d2..a39090d 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -220,7 +220,14 @@ static inline int audit_match_class_bits(int class, u32 
*mask)
 
 static int audit_match_signal(struct audit_entry *entry)
 {
-   struct audit_field *arch = entry->rule.arch_f;
+   int i;
+   struct audit_field *arch = NULL;
+
+   for (i = 0; i < entry->rule.field_count; i++)
+   if (entry->rule.fields[i].type == AUDIT_ARCH) {
+   arch = >rule.fields[i];
+   break;
+   }
 
if (!arch) {
/* When arch is unspecified, we must check both masks on biarch
@@ -496,9 +503,6 @@ static struct audit_entry *audit_data_to_entry(struct 
audit_rule_data *data,
if (!gid_valid(f->gid))
goto exit_free;
break;
-   case AUDIT_ARCH:
-   entry->rule.arch_f = f;
-   break;
case AUDIT_SUBJ_USER:
case AUDIT_SUBJ_ROLE:
case AUDIT_SUBJ_TYPE:
-- 
1.8.3.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [RFC PATCH 1/3] audit: remove arch_f pointer from struct audit_krule

2018-02-16 Thread Richard Guy Briggs 
On 2018-02-15 15:42, Paul Moore wrote:
> On Mon, Feb 12, 2018 at 7:29 AM, Richard Guy Briggs  wrote:
> > The arch_f pointer was added to the struct audit_krule in commit:
> > e54dc2431d740a79a6bd013babade99d71b1714f ("audit signal recipients")
> >
> > This is only used on addition and deletion of rules which isn't time
> > critical and the arch field is likely to be one of the first fields,
> > easily found iterating over the field type.  This isn't worth the
> > additional complexity and storage.  Delete the field.
> >
> > Signed-off-by: Richard Guy Briggs 
> > ---
> >  include/linux/audit.h |  1 -
> >  kernel/auditfilter.c  | 12 
> >  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> I haven't decided if I like the removal of arch_f or not, but I think
> I might know where your oops/panic is coming from, thoughts below ...
> 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index af410d9..64a3b0e 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -58,7 +58,6 @@ struct audit_krule {
> > u32 field_count;
> > char*filterkey; /* ties events to rules */
> > struct audit_field  *fields;
> > -   struct audit_field  *arch_f; /* quick access to arch field */
> > struct audit_field  *inode_f; /* quick access to an inode field 
> > */
> > struct audit_watch  *watch; /* associated watch */
> > struct audit_tree   *tree;  /* associated watched tree */
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 739a6d2..3343d1c 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -220,7 +220,14 @@ static inline int audit_match_class_bits(int class, 
> > u32 *mask)
> >
> >  static int audit_match_signal(struct audit_entry *entry)
> >  {
> > -   struct audit_field *arch = entry->rule.arch_f;
> > +   int i;
> > +   struct audit_field *arch;
> > +
> > +   for (i = 0; i < entry->rule.field_count; i++)
> > +   if (entry->rule.fields[i].type == AUDIT_ARCH) {
> > +   arch = >rule.fields[i];
> > +   break;
> > +   }
> 
> In the original code arch_f was initialized to NULL via the allocator
> so the arch local variable was guaranteed to have a valid value or
> NULL.  Unfortunately, in your code if there is no AUDIT_ARCH field
> arch could remain uninitialized which I believe could lead to the
> oops/panic you are seeing.

Ok, so the function local stack variable allocator doesn't initialze
local variables, and what was happenning was it was skipping the "if
(!arch)" clause (due to the spurrious "9" value of arch) jumping
straight in to the arch->val evaluation with a bogus arch pointer.  I
was fairly certain it was happenning on a rule that had an arch field,
but in fact it was happenning on one that didn't.  I was hinging on the
assumption local variables getting initialized to zero (or NULL).

Problem solved.  Thank you.

> > if (!arch) {
> > /* When arch is unspecified, we must check both masks on 
> > biarch
> > @@ -496,9 +503,6 @@ static struct audit_entry *audit_data_to_entry(struct 
> > audit_rule_data *data,
> > if (!gid_valid(f->gid))
> > goto exit_free;
> > break;
> > -   case AUDIT_ARCH:
> > -   entry->rule.arch_f = f;
> > -   break;
> > case AUDIT_SUBJ_USER:
> > case AUDIT_SUBJ_ROLE:
> > case AUDIT_SUBJ_TYPE:
> > --
> > 1.8.3.1
> 
> paul moore

- RGB

--
Richard Guy Briggs 
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH ghak8 ALT4 V4 0/3] audit: show more information for entries with anonymous parents

2018-02-16 Thread Richard Guy Briggs 
On 2018-02-15 17:15, Paul Moore wrote:
> On Mon, Feb 12, 2018 at 12:02 AM, Richard Guy Briggs  wrote:
> > More than one filesystem was causing hundreds to thousands of null PATH
> > records to be associated with the *init_module SYSCALL records on a few
> > modules with corresponding audit syscall rules.
> >
> > This patchset adds extra information to those PATH records to provide
> > insight into what is generating them, including a partial pathname,
> > fstype field, and two new filetypes that indicate the pathname isn't
> > anchored at the root of the task's root filesystem.
> >
> > Richard Guy Briggs (3):
> >   audit: show partial pathname for entries with anonymous parents
> >   audit: append new fstype field for anonymous PATH records
> >   audit: add new filetypes CREATE_ANON and PARENT_ANON
> 
> The more I look at this, the more I prefer your original approach that
> prefixed the relative pathname with the fstype.  Yes, I do realize
> that you sort of work around that by including the fstype as a new
> field in the PATH records, but we're still stuck with those odd
> relative/un-rooted name fields.

They are signalled as being unrooted by the ANON filetypes.  And now
that you mention it, should fail the ausearch-test since it isn't a "full
path", as claimed is necessary in ghak70 (so I don't see why the
KERN_MODULE name= record/field fails that test).

> Further, I don't recall ever hearing a good reason why the original
> approach wasn't acceptable to Steve's userspace.  I know he did make
> some very last minute hand-wavy comments, but none of those made any
> sense to me; I don't understand why Steve's audit record parser is
> even looking in the pathname string.
> 
> I'm going to park these patches in limbo for the time being.

Can you give me an idea how long that might be?

> paul moore

- RGB

--
Richard Guy Briggs 
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit