linux-kernel - Re: [PATCH ghak21 V4 1/2] audit: remove path param from link denied function

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 16 Apr 2018 13:55:04 -0400
From:   Richard Guy Briggs <rgb@...hat.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Linux-Audit Mailing List <linux-audit@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Eric Paris <eparis@...hat.com>,
        Paul Moore <paul@...l-moore.com>,
        Steve Grubb <sgrubb@...hat.com>
Subject: Re: [PATCH ghak21 V4 1/2] audit: remove path param from link denied
 function

On 2018-04-16 10:55, Kees Cook wrote:
> On Wed, Mar 21, 2018 at 1:42 AM, Richard Guy Briggs <rgb@...hat.com> wrote:
> > In commit 45b578fe4c3cade6f4ca1fc934ce199afd857edc
> > ("audit: link denied should not directly generate PATH record")
> > the need for the struct path *link parameter was removed.
> > Remove the now useless struct path argument.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@...hat.com>
> > ---
> >  fs/namei.c            | 4 ++--
> >  include/linux/audit.h | 6 ++----
> >  kernel/audit.c        | 3 +--
> >  3 files changed, 5 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 9cc91fb..e3682bb 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -945,7 +945,7 @@ static inline int may_follow_link(struct nameidata *nd)
> >         if (nd->flags & LOOKUP_RCU)
> >                 return -ECHILD;
> >
> > -       audit_log_link_denied("follow_link", &nd->stack[0].link);
> > +       audit_log_link_denied("follow_link");
> >         return -EACCES;
> >  }
> >
> > @@ -1011,7 +1011,7 @@ static int may_linkat(struct path *link)
> >         if (safe_hardlink_source(inode) || inode_owner_or_capable(inode))
> >                 return 0;
> >
> > -       audit_log_link_denied("linkat", link);
> > +       audit_log_link_denied("linkat");
> >         return -EPERM;
> >  }
> 
> This removed the "link" details in both cases, and then commit
> ea841bafda3f ("audit: add refused symlink to audit_names") added back
> one of them:
> 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index e3682bb72cb5..5f8e8e2732e1 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -945,6 +945,7 @@ static inline int may_follow_link(struct nameidata *nd)
> >         if (nd->flags & LOOKUP_RCU)
> >                 return -ECHILD;
> >
> > +       audit_inode(nd->name, nd->stack[0].link.dentry, 0);
> >         audit_log_link_denied("follow_link");
> >         return -EACCES;
> >  }
> 
> Why remove it in the first place, and why add it back open-coded in
> only one place?

In both cases, the extra PATH record didn't make sense with the SYSCALL
record "items=" field and conflicted with another PATH record's "item="
field.

All the necessary details were already available in the hardlink case
via two PATH records associated with the SYSCALL record.

There were two conflicting PATH records in the symlink case, one of them
incomplete, so it made more sense to make the existing one complete.

> -Kees

- RGB

--
Richard Guy Briggs <rgb@...hat.com>
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