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

[Paul Moore]

On Mon, Mar 12, 2018 at 3:59 AM, Richard Guy Briggs  wrote:
> On 2018-03-08 19:50, Paul Moore wrote:

...

>> (Point #2 is why I didn't merge patch 3/4, just include it in this
>> revised patch)
>
> On reviewing this, I'm not totally convinced the parent record is
> necessary to fully understand what happenned since there is a CWD
> record.  I left 3 and 4 as seperate patches in case it is found that
> only 3 is necessary.

You need to figure this out and decide what is necessary.

-- 
paul moore
www.paul-moore.com

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

[Paul Moore]

On Mon, Mar 12, 2018 at 3:59 AM, Richard Guy Briggs  wrote:
> On 2018-03-08 19:50, Paul Moore wrote:

...

>> (Point #2 is why I didn't merge patch 3/4, just include it in this
>> revised patch)
>
> On reviewing this, I'm not totally convinced the parent record is
> necessary to fully understand what happenned since there is a CWD
> record.  I left 3 and 4 as seperate patches in case it is found that
> only 3 is necessary.

You need to figure this out and decide what is necessary.

-- 
paul moore
www.paul-moore.com

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

[Richard Guy Briggs]

On 2018-03-08 19:50, 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;
> 
> Two things here:
> 
> 1. We need to allocate a buffer to feed to d_absolute_path(), and
> getname_kernel() is just going to make another copy so we need to make
> sure we clean up after ourselves.  Perhaps I missing it, but I'm not
> seeing where we free the kmalloc'd buffer or call putname().  Feel
> free to correct me if I'm missing something obvious.

That is put very diplomatically!  Both fixed, with audit_panic() as
needed.

> 2. While the audit_* calls are going to bail early in the cases where
> audit is disabled, or not configured, we are going to pay the penalty
> for the kmalloc() call above, as well as the getname_kernel() and
> d_absolute_path() calls below.  I think it might be beneficial to
> create a new function (audit_log_symlink_denied() perhaps?) which
> encapsulates all the audit bits in may_follow_link() and exits early
> when needed.  What do you think?

I agree a seperate function is appropriate.  There was some back and
forth between namei.c and audit.c due to struct nameidata and
audit_panic() not able to co-exist due to them both being local to their
respective sub-systems.

> (Point #2 is why I didn't merge patch 3/4, just include it in this
> revised patch)

On reviewing this, I'm not totally convinced the parent record is
necessary to fully understand what happenned since there is a CWD
record.  I left 3 and 4 as seperate patches in case it is found that
only 3 is necessary.

> > +   audit_inode(getname_kernel(d_absolute_path(>stack[0].link, 
> > pathname,
> > +  PATH_MAX + 1)),
> > +   nd->stack[0].link.dentry, 0);
> > +   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

- 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

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

[Richard Guy Briggs]

On 2018-03-08 19:50, 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;
> 
> Two things here:
> 
> 1. We need to allocate a buffer to feed to d_absolute_path(), and
> getname_kernel() is just going to make another copy so we need to make
> sure we clean up after ourselves.  Perhaps I missing it, but I'm not
> seeing where we free the kmalloc'd buffer or call putname().  Feel
> free to correct me if I'm missing something obvious.

That is put very diplomatically!  Both fixed, with audit_panic() as
needed.

> 2. While the audit_* calls are going to bail early in the cases where
> audit is disabled, or not configured, we are going to pay the penalty
> for the kmalloc() call above, as well as the getname_kernel() and
> d_absolute_path() calls below.  I think it might be beneficial to
> create a new function (audit_log_symlink_denied() perhaps?) which
> encapsulates all the audit bits in may_follow_link() and exits early
> when needed.  What do you think?

I agree a seperate function is appropriate.  There was some back and
forth between namei.c and audit.c due to struct nameidata and
audit_panic() not able to co-exist due to them both being local to their
respective sub-systems.

> (Point #2 is why I didn't merge patch 3/4, just include it in this
> revised patch)

On reviewing this, I'm not totally convinced the parent record is
necessary to fully understand what happenned since there is a CWD
record.  I left 3 and 4 as seperate patches in case it is found that
only 3 is necessary.

> > +   audit_inode(getname_kernel(d_absolute_path(>stack[0].link, 
> > pathname,
> > +  PATH_MAX + 1)),
> > +   nd->stack[0].link.dentry, 0);
> > +   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

- 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

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

[Paul Moore]

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;

Two things here:

1. We need to allocate a buffer to feed to d_absolute_path(), and
getname_kernel() is just going to make another copy so we need to make
sure we clean up after ourselves.  Perhaps I missing it, but I'm not
seeing where we free the kmalloc'd buffer or call putname().  Feel
free to correct me if I'm missing something obvious.

2. While the audit_* calls are going to bail early in the cases where
audit is disabled, or not configured, we are going to pay the penalty
for the kmalloc() call above, as well as the getname_kernel() and
d_absolute_path() calls below.  I think it might be beneficial to
create a new function (audit_log_symlink_denied() perhaps?) which
encapsulates all the audit bits in may_follow_link() and exits early
when needed.  What do you think?

(Point #2 is why I didn't merge patch 3/4, just include it in this
revised patch)

> +   audit_inode(getname_kernel(d_absolute_path(>stack[0].link, 
> pathname,
> +  PATH_MAX + 1)),
> +   nd->stack[0].link.dentry, 0);
> +   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

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

[Paul Moore]

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;

Two things here:

1. We need to allocate a buffer to feed to d_absolute_path(), and
getname_kernel() is just going to make another copy so we need to make
sure we clean up after ourselves.  Perhaps I missing it, but I'm not
seeing where we free the kmalloc'd buffer or call putname().  Feel
free to correct me if I'm missing something obvious.

2. While the audit_* calls are going to bail early in the cases where
audit is disabled, or not configured, we are going to pay the penalty
for the kmalloc() call above, as well as the getname_kernel() and
d_absolute_path() calls below.  I think it might be beneficial to
create a new function (audit_log_symlink_denied() perhaps?) which
encapsulates all the audit bits in may_follow_link() and exits early
when needed.  What do you think?

(Point #2 is why I didn't merge patch 3/4, just include it in this
revised patch)

> +   audit_inode(getname_kernel(d_absolute_path(>stack[0].link, 
> pathname,
> +  PATH_MAX + 1)),
> +   nd->stack[0].link.dentry, 0);
> +   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

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

[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

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

[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

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

[Richard Guy Briggs]

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 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-au...@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- 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

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

[Richard Guy Briggs]

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 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-au...@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- 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

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

[Paul Moore]

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?

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

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

[Paul Moore]

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?

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

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

[Richard Guy Briggs]

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);
+   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

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

[Richard Guy Briggs]

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);
+   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