Re: [PATCH ghak21 V2 3/4] audit: add refused symlink to audit_names
On 2018-03-13 16:24, Paul Moore wrote: > On Tue, Mar 13, 2018 at 6:52 AM, Richard Guy Briggs wrote: > > On 2018-03-13 11:38, Steve Grubb wrote: > >> On Tue, 13 Mar 2018 06:11:08 -0400 > >> Richard Guy Briggs wrote: > >> > >> > On 2018-03-13 09:35, Steve Grubb wrote: > >> > > On Mon, 12 Mar 2018 11:52:56 -0400 > >> > > Richard Guy Briggs wrote: > >> > > > >> > > > On 2018-03-12 11:53, Paul Moore wrote: > >> > > > > On Mon, Mar 12, 2018 at 11:26 AM, Richard Guy Briggs > >> > > > > wrote: > >> > > > > > On 2018-03-12 11:12, Paul Moore wrote: > >> > > > > >> On Mon, Mar 12, 2018 at 2:31 AM, Richard Guy Briggs > >> > > > > >> wrote: > >> > > > > >> > Audit link denied events for symlinks had duplicate PATH > >> > > > > >> > records rather than just updating the existing PATH record. > >> > > > > >> > Update the symlink's PATH record with the current dentry > >> > > > > >> > and inode information. > >> > > > > >> > > >> > > > > >> > See: https://github.com/linux-audit/audit-kernel/issues/21 > >> > > > > >> > Signed-off-by: Richard Guy Briggs > >> > > > > >> > --- > >> > > > > >> > fs/namei.c | 1 + > >> > > > > >> > 1 file changed, 1 insertion(+) > >> > > > > >> > >> > > > > >> Why didn't you include this in patch 4/4 like I asked during > >> > > > > >> the previous review? > >> > > > > > > >> > > > > > Please see the last comment of: > >> > > > > > https://www.redhat.com/archives/linux-audit/2018-March/msg00070.html > >> > > > > > >> > > > > Yes, I just saw that ... I hadn't seen your replies on the v1 > >> > > > > patches until I had finished reviewing v2. I just replied to > >> > > > > that mail in the v1 thread, but basically you need to figure > >> > > > > out what is necessary here and let us know. If I have to > >> > > > > figure it out it likely isn't going to get done with enough > >> > > > > soak time prior to the upcoming merge window. > >> > > > > >> > > > Steve? I was hoping you could chime in here. > >> > > > >> > > If the CWD record will always be the same as the PARENT record, > >> > > then we do not need the parent record. Duplicate information is > >> > > bad. Like all the duplicate SYSCALL information. > >> > > >> > The CWD record could be different from the PARENT record, since I > >> > could have SYMLINK=/tmp/test/symlink, CWD=/tmp, PARENT=/tmp/test. > >> > Does the parent record even matter since it might not be a directory > >> > operation like creat, unlink or rename? > >> > >> There's 2 issues. One is creating the path if what we have is relative. > >> In this situation CWD should be enough. But if the question is whether > >> the PARENT directory should be included...what if the PARENT > >> permissions do not allow the successful name resolution? In that case > >> we might only get a PARENT record no? In that case we would need it. > > > > I think in the case of symlink creation, normal file create code path > > would be in effect, and would properly log parent and symlink source > > file paths (if a rule to log it was in effect) which is not something > > that would trigger a symlink link denied error. Symlink link denied > > happens only when trying to actually follow the link before > > resolving the target path of a read/write/exec of the symlink target. > > > > If the parent permissions of the link's target don't allow successful > > name resolution then the symlink link denied condition isn't met, but > > rather any other rule that applies to the target path. > > I'm guessing you are in the process of tracking all this down, but if > not, lets get to a point where we can answer this definitively and not > guess :) I was fairly certain but being polite, expecting confirmation or possibly correction if I've overlooked something. Additionally, this denial message only happens in certain parts of the permission check for symlinks: /proc/sys/fs/protected_symlinks == 1 and follower and link owner don't match and parent sticky and world-writable and link parent and link owner don't match If you want other symlink denials logged, you need to set a rule for the target filtering on operation failure such as unix file permissions. The similar situation exists for hardlinks. > 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
Re: [PATCH ghak21 V2 3/4] audit: add refused symlink to audit_names
On Tue, Mar 13, 2018 at 6:52 AM, Richard Guy Briggs wrote: > On 2018-03-13 11:38, Steve Grubb wrote: >> On Tue, 13 Mar 2018 06:11:08 -0400 >> Richard Guy Briggs wrote: >> >> > On 2018-03-13 09:35, Steve Grubb wrote: >> > > On Mon, 12 Mar 2018 11:52:56 -0400 >> > > Richard Guy Briggs wrote: >> > > >> > > > On 2018-03-12 11:53, Paul Moore wrote: >> > > > > On Mon, Mar 12, 2018 at 11:26 AM, Richard Guy Briggs >> > > > > wrote: >> > > > > > On 2018-03-12 11:12, Paul Moore wrote: >> > > > > >> On Mon, Mar 12, 2018 at 2:31 AM, Richard Guy Briggs >> > > > > >> wrote: >> > > > > >> > Audit link denied events for symlinks had duplicate PATH >> > > > > >> > records rather than just updating the existing PATH record. >> > > > > >> > Update the symlink's PATH record with the current dentry >> > > > > >> > and inode information. >> > > > > >> > >> > > > > >> > See: https://github.com/linux-audit/audit-kernel/issues/21 >> > > > > >> > Signed-off-by: Richard Guy Briggs >> > > > > >> > --- >> > > > > >> > fs/namei.c | 1 + >> > > > > >> > 1 file changed, 1 insertion(+) >> > > > > >> >> > > > > >> Why didn't you include this in patch 4/4 like I asked during >> > > > > >> the previous review? >> > > > > > >> > > > > > Please see the last comment of: >> > > > > > https://www.redhat.com/archives/linux-audit/2018-March/msg00070.html >> > > > > >> > > > > Yes, I just saw that ... I hadn't seen your replies on the v1 >> > > > > patches until I had finished reviewing v2. I just replied to >> > > > > that mail in the v1 thread, but basically you need to figure >> > > > > out what is necessary here and let us know. If I have to >> > > > > figure it out it likely isn't going to get done with enough >> > > > > soak time prior to the upcoming merge window. >> > > > >> > > > Steve? I was hoping you could chime in here. >> > > >> > > If the CWD record will always be the same as the PARENT record, >> > > then we do not need the parent record. Duplicate information is >> > > bad. Like all the duplicate SYSCALL information. >> > >> > The CWD record could be different from the PARENT record, since I >> > could have SYMLINK=/tmp/test/symlink, CWD=/tmp, PARENT=/tmp/test. >> > Does the parent record even matter since it might not be a directory >> > operation like creat, unlink or rename? >> >> There's 2 issues. One is creating the path if what we have is relative. >> In this situation CWD should be enough. But if the question is whether >> the PARENT directory should be included...what if the PARENT >> permissions do not allow the successful name resolution? In that case >> we might only get a PARENT record no? In that case we would need it. > > I think in the case of symlink creation, normal file create code path > would be in effect, and would properly log parent and symlink source > file paths (if a rule to log it was in effect) which is not something > that would trigger a symlink link denied error. Symlink link denied > happens only when trying to actually follow the link before > resolving the target path of a read/write/exec of the symlink target. > > If the parent permissions of the link's target don't allow successful > name resolution then the symlink link denied condition isn't met, but > rather any other rule that applies to the target path. I'm guessing you are in the process of tracking all this down, but if not, lets get to a point where we can answer this definitively and not guess :) -- paul moore www.paul-moore.com
Re: [PATCH ghak21 V2 3/4] audit: add refused symlink to audit_names
On Tue, 13 Mar 2018 06:52:51 -0400
Richard Guy Briggs wrote:
> On 2018-03-13 11:38, Steve Grubb wrote:
> > On Tue, 13 Mar 2018 06:11:08 -0400
> > Richard Guy Briggs wrote:
> >
> > > On 2018-03-13 09:35, Steve Grubb wrote:
> > > > On Mon, 12 Mar 2018 11:52:56 -0400
> > > > Richard Guy Briggs wrote:
> > > >
> > > > > On 2018-03-12 11:53, Paul Moore wrote:
> > > > > > On Mon, Mar 12, 2018 at 11:26 AM, Richard Guy Briggs
> > > > > > wrote:
> > > > > > > On 2018-03-12 11:12, Paul Moore wrote:
> > > > > > >> On Mon, Mar 12, 2018 at 2:31 AM, Richard Guy Briggs
> > > > > > >> wrote:
> > > > > > >> > Audit link denied events for symlinks had duplicate
> > > > > > >> > PATH records rather than just updating the existing
> > > > > > >> > PATH record. Update the symlink's PATH record with the
> > > > > > >> > current dentry and inode information.
> > > > > > >> >
> > > > > > >> > See:
> > > > > > >> > https://github.com/linux-audit/audit-kernel/issues/21
> > > > > > >> > Signed-off-by: Richard Guy Briggs ---
> > > > > > >> > fs/namei.c | 1 +
> > > > > > >> > 1 file changed, 1 insertion(+)
> > > > > > >>
> > > > > > >> Why didn't you include this in patch 4/4 like I asked
> > > > > > >> during the previous review?
> > > > > > >
> > > > > > > Please see the last comment of:
> > > > > > > https://www.redhat.com/archives/linux-audit/2018-March/msg00070.html
> > > > > > >
> > > > > >
> > > > > > Yes, I just saw that ... I hadn't seen your replies on the
> > > > > > v1 patches until I had finished reviewing v2. I just
> > > > > > replied to that mail in the v1 thread, but basically you
> > > > > > need to figure out what is necessary here and let us know.
> > > > > > If I have to figure it out it likely isn't going to get
> > > > > > done with enough soak time prior to the upcoming merge
> > > > > > window.
> > > > >
> > > > > Steve? I was hoping you could chime in here.
> > > >
> > > > If the CWD record will always be the same as the PARENT record,
> > > > then we do not need the parent record. Duplicate information is
> > > > bad. Like all the duplicate SYSCALL information.
> > >
> > > The CWD record could be different from the PARENT record, since I
> > > could have SYMLINK=/tmp/test/symlink, CWD=/tmp, PARENT=/tmp/test.
> > > Does the parent record even matter since it might not be a
> > > directory operation like creat, unlink or rename?
> >
> > There's 2 issues. One is creating the path if what we have is
> > relative. In this situation CWD should be enough. But if the
> > question is whether the PARENT directory should be included...what
> > if the PARENT permissions do not allow the successful name
> > resolution? In that case we might only get a PARENT record no? In
> > that case we would need it.
>
> I think in the case of symlink creation, normal file create code path
> would be in effect, and would properly log parent and symlink source
> file paths (if a rule to log it was in effect) which is not something
> that would trigger a symlink link denied error. Symlink link denied
> happens only when trying to actually follow the link before
> resolving the target path of a read/write/exec of the symlink target.
>
> If the parent permissions of the link's target don't allow successful
> name resolution then the symlink link denied condition isn't met, but
> rather any other rule that applies to the target path.
Then I guess the PARENT record is not needed.
-Steve
> > > > > I'd just include it for completeness unless Steve thinks it
> > > > > will stand on its own and doesn't want the overhead.
> > > > >
> > > > > > >> > diff --git a/fs/namei.c b/fs/namei.c
> > > > > > >> > index 50d2533..00f5041 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",
> > > > > > >> > &nd->stack[0].link); return -EACCES; }
> > > > > > >>
> > > > > > >> paul moore
> > > > > > >
> > > > > > > - RGB
> > > > > >
> > > > > > paul moore
> > > > >
> > > > > - RGB
> > >
> > > - 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
> >
>
> - 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: [PATCH ghak21 V2 3/4] audit: add refused symlink to audit_names
On 2018-03-13 11:38, Steve Grubb wrote:
> On Tue, 13 Mar 2018 06:11:08 -0400
> Richard Guy Briggs wrote:
>
> > On 2018-03-13 09:35, Steve Grubb wrote:
> > > On Mon, 12 Mar 2018 11:52:56 -0400
> > > Richard Guy Briggs wrote:
> > >
> > > > On 2018-03-12 11:53, Paul Moore wrote:
> > > > > On Mon, Mar 12, 2018 at 11:26 AM, Richard Guy Briggs
> > > > > wrote:
> > > > > > On 2018-03-12 11:12, Paul Moore wrote:
> > > > > >> On Mon, Mar 12, 2018 at 2:31 AM, Richard Guy Briggs
> > > > > >> wrote:
> > > > > >> > Audit link denied events for symlinks had duplicate PATH
> > > > > >> > records rather than just updating the existing PATH record.
> > > > > >> > Update the symlink's PATH record with the current dentry
> > > > > >> > and inode information.
> > > > > >> >
> > > > > >> > See: https://github.com/linux-audit/audit-kernel/issues/21
> > > > > >> > Signed-off-by: Richard Guy Briggs
> > > > > >> > ---
> > > > > >> > fs/namei.c | 1 +
> > > > > >> > 1 file changed, 1 insertion(+)
> > > > > >>
> > > > > >> Why didn't you include this in patch 4/4 like I asked during
> > > > > >> the previous review?
> > > > > >
> > > > > > Please see the last comment of:
> > > > > > https://www.redhat.com/archives/linux-audit/2018-March/msg00070.html
> > > > > >
> > > > >
> > > > > Yes, I just saw that ... I hadn't seen your replies on the v1
> > > > > patches until I had finished reviewing v2. I just replied to
> > > > > that mail in the v1 thread, but basically you need to figure
> > > > > out what is necessary here and let us know. If I have to
> > > > > figure it out it likely isn't going to get done with enough
> > > > > soak time prior to the upcoming merge window.
> > > >
> > > > Steve? I was hoping you could chime in here.
> > >
> > > If the CWD record will always be the same as the PARENT record,
> > > then we do not need the parent record. Duplicate information is
> > > bad. Like all the duplicate SYSCALL information.
> >
> > The CWD record could be different from the PARENT record, since I
> > could have SYMLINK=/tmp/test/symlink, CWD=/tmp, PARENT=/tmp/test.
> > Does the parent record even matter since it might not be a directory
> > operation like creat, unlink or rename?
>
> There's 2 issues. One is creating the path if what we have is relative.
> In this situation CWD should be enough. But if the question is whether
> the PARENT directory should be included...what if the PARENT
> permissions do not allow the successful name resolution? In that case
> we might only get a PARENT record no? In that case we would need it.
I think in the case of symlink creation, normal file create code path
would be in effect, and would properly log parent and symlink source
file paths (if a rule to log it was in effect) which is not something
that would trigger a symlink link denied error. Symlink link denied
happens only when trying to actually follow the link before
resolving the target path of a read/write/exec of the symlink target.
If the parent permissions of the link's target don't allow successful
name resolution then the symlink link denied condition isn't met, but
rather any other rule that applies to the target path.
> -Steve
>
> > > > I'd just include it for completeness unless Steve thinks it will
> > > > stand on its own and doesn't want the overhead.
> > > >
> > > > > >> > diff --git a/fs/namei.c b/fs/namei.c
> > > > > >> > index 50d2533..00f5041 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",
> > > > > >> > &nd->stack[0].link); return -EACCES;
> > > > > >> > }
> > > > > >>
> > > > > >> paul moore
> > > > > >
> > > > > > - RGB
> > > > >
> > > > > paul moore
> > > >
> > > > - RGB
> >
> > - 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
>
- 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: [PATCH ghak21 V2 3/4] audit: add refused symlink to audit_names
On Tue, 13 Mar 2018 06:11:08 -0400
Richard Guy Briggs wrote:
> On 2018-03-13 09:35, Steve Grubb wrote:
> > On Mon, 12 Mar 2018 11:52:56 -0400
> > Richard Guy Briggs wrote:
> >
> > > On 2018-03-12 11:53, Paul Moore wrote:
> > > > On Mon, Mar 12, 2018 at 11:26 AM, Richard Guy Briggs
> > > > wrote:
> > > > > On 2018-03-12 11:12, Paul Moore wrote:
> > > > >> On Mon, Mar 12, 2018 at 2:31 AM, Richard Guy Briggs
> > > > >> wrote:
> > > > >> > Audit link denied events for symlinks had duplicate PATH
> > > > >> > records rather than just updating the existing PATH record.
> > > > >> > Update the symlink's PATH record with the current dentry
> > > > >> > and inode information.
> > > > >> >
> > > > >> > See: https://github.com/linux-audit/audit-kernel/issues/21
> > > > >> > Signed-off-by: Richard Guy Briggs
> > > > >> > ---
> > > > >> > fs/namei.c | 1 +
> > > > >> > 1 file changed, 1 insertion(+)
> > > > >>
> > > > >> Why didn't you include this in patch 4/4 like I asked during
> > > > >> the previous review?
> > > > >
> > > > > Please see the last comment of:
> > > > > https://www.redhat.com/archives/linux-audit/2018-March/msg00070.html
> > > > >
> > > >
> > > > Yes, I just saw that ... I hadn't seen your replies on the v1
> > > > patches until I had finished reviewing v2. I just replied to
> > > > that mail in the v1 thread, but basically you need to figure
> > > > out what is necessary here and let us know. If I have to
> > > > figure it out it likely isn't going to get done with enough
> > > > soak time prior to the upcoming merge window.
> > >
> > > Steve? I was hoping you could chime in here.
> >
> > If the CWD record will always be the same as the PARENT record,
> > then we do not need the parent record. Duplicate information is
> > bad. Like all the duplicate SYSCALL information.
>
> The CWD record could be different from the PARENT record, since I
> could have SYMLINK=/tmp/test/symlink, CWD=/tmp, PARENT=/tmp/test.
> Does the parent record even matter since it might not be a directory
> operation like creat, unlink or rename?
There's 2 issues. One is creating the path if what we have is relative.
In this situation CWD should be enough. But if the question is whether
the PARENT directory should be included...what if the PARENT
permissions do not allow the successful name resolution? In that case
we might only get a PARENT record no? In that case we would need it.
-Steve
> > > I'd just include it for completeness unless Steve thinks it will
> > > stand on its own and doesn't want the overhead.
> > >
> > > > >> > diff --git a/fs/namei.c b/fs/namei.c
> > > > >> > index 50d2533..00f5041 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",
> > > > >> > &nd->stack[0].link); return -EACCES;
> > > > >> > }
> > > > >>
> > > > >> paul moore
> > > > >
> > > > > - RGB
> > > >
> > > > paul moore
> > >
> > > - RGB
>
> - 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: [PATCH ghak21 V2 3/4] audit: add refused symlink to audit_names
On 2018-03-13 09:35, Steve Grubb wrote:
> On Mon, 12 Mar 2018 11:52:56 -0400
> Richard Guy Briggs wrote:
>
> > On 2018-03-12 11:53, Paul Moore wrote:
> > > On Mon, Mar 12, 2018 at 11:26 AM, Richard Guy Briggs
> > > wrote:
> > > > On 2018-03-12 11:12, Paul Moore wrote:
> > > >> On Mon, Mar 12, 2018 at 2:31 AM, Richard Guy Briggs
> > > >> wrote:
> > > >> > Audit link denied events for symlinks had duplicate PATH
> > > >> > records rather than just updating the existing PATH record.
> > > >> > Update the symlink's PATH record with the current dentry and
> > > >> > inode information.
> > > >> >
> > > >> > See: https://github.com/linux-audit/audit-kernel/issues/21
> > > >> > Signed-off-by: Richard Guy Briggs
> > > >> > ---
> > > >> > fs/namei.c | 1 +
> > > >> > 1 file changed, 1 insertion(+)
> > > >>
> > > >> Why didn't you include this in patch 4/4 like I asked during the
> > > >> previous review?
> > > >
> > > > Please see the last comment of:
> > > > https://www.redhat.com/archives/linux-audit/2018-March/msg00070.html
> > >
> > > Yes, I just saw that ... I hadn't seen your replies on the v1
> > > patches until I had finished reviewing v2. I just replied to that
> > > mail in the v1 thread, but basically you need to figure out what is
> > > necessary here and let us know. If I have to figure it out it
> > > likely isn't going to get done with enough soak time prior to the
> > > upcoming merge window.
> >
> > Steve? I was hoping you could chime in here.
>
> If the CWD record will always be the same as the PARENT record, then we
> do not need the parent record. Duplicate information is bad. Like all
> the duplicate SYSCALL information.
The CWD record could be different from the PARENT record, since I could
have SYMLINK=/tmp/test/symlink, CWD=/tmp, PARENT=/tmp/test. Does the
parent record even matter since it might not be a directory operation
like creat, unlink or rename?
> -Steve
>
> > I'd just include it for completeness unless Steve thinks it will stand
> > on its own and doesn't want the overhead.
> >
> > > >> > diff --git a/fs/namei.c b/fs/namei.c
> > > >> > index 50d2533..00f5041 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",
> > > >> > &nd->stack[0].link); return -EACCES;
> > > >> > }
> > > >>
> > > >> paul moore
> > > >
> > > > - RGB
> > >
> > > paul moore
> >
> > - RGB
- 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: [PATCH ghak21 V2 3/4] audit: add refused symlink to audit_names
On Mon, 12 Mar 2018 11:52:56 -0400
Richard Guy Briggs wrote:
> On 2018-03-12 11:53, Paul Moore wrote:
> > On Mon, Mar 12, 2018 at 11:26 AM, Richard Guy Briggs
> > wrote:
> > > On 2018-03-12 11:12, Paul Moore wrote:
> > >> On Mon, Mar 12, 2018 at 2:31 AM, Richard Guy Briggs
> > >> wrote:
> > >> > Audit link denied events for symlinks had duplicate PATH
> > >> > records rather than just updating the existing PATH record.
> > >> > Update the symlink's PATH record with the current dentry and
> > >> > inode information.
> > >> >
> > >> > See: https://github.com/linux-audit/audit-kernel/issues/21
> > >> > Signed-off-by: Richard Guy Briggs
> > >> > ---
> > >> > fs/namei.c | 1 +
> > >> > 1 file changed, 1 insertion(+)
> > >>
> > >> Why didn't you include this in patch 4/4 like I asked during the
> > >> previous review?
> > >
> > > Please see the last comment of:
> > > https://www.redhat.com/archives/linux-audit/2018-March/msg00070.html
> >
> > Yes, I just saw that ... I hadn't seen your replies on the v1
> > patches until I had finished reviewing v2. I just replied to that
> > mail in the v1 thread, but basically you need to figure out what is
> > necessary here and let us know. If I have to figure it out it
> > likely isn't going to get done with enough soak time prior to the
> > upcoming merge window.
>
> Steve? I was hoping you could chime in here.
If the CWD record will always be the same as the PARENT record, then we
do not need the parent record. Duplicate information is bad. Like all
the duplicate SYSCALL information.
-Steve
> I'd just include it for completeness unless Steve thinks it will stand
> on its own and doesn't want the overhead.
>
> > >> > diff --git a/fs/namei.c b/fs/namei.c
> > >> > index 50d2533..00f5041 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",
> > >> > &nd->stack[0].link); return -EACCES;
> > >> > }
> > >>
> > >> paul moore
> > >
> > > - RGB
> >
> > 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-au...@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak21 V2 3/4] audit: add refused symlink to audit_names
On Mon, Mar 12, 2018 at 11:52 AM, Richard Guy Briggs wrote: > On 2018-03-12 11:53, Paul Moore wrote: >> On Mon, Mar 12, 2018 at 11:26 AM, Richard Guy Briggs wrote: >> > On 2018-03-12 11:12, Paul Moore wrote: >> >> On Mon, Mar 12, 2018 at 2:31 AM, Richard Guy Briggs >> >> wrote: >> >> > Audit link denied events for symlinks had duplicate PATH records rather >> >> > than just updating the existing PATH record. Update the symlink's PATH >> >> > record with the current dentry and inode information. >> >> > >> >> > See: https://github.com/linux-audit/audit-kernel/issues/21 >> >> > Signed-off-by: Richard Guy Briggs >> >> > --- >> >> > fs/namei.c | 1 + >> >> > 1 file changed, 1 insertion(+) >> >> >> >> Why didn't you include this in patch 4/4 like I asked during the >> >> previous review? >> > >> > Please see the last comment of: >> > https://www.redhat.com/archives/linux-audit/2018-March/msg00070.html >> >> Yes, I just saw that ... I hadn't seen your replies on the v1 patches >> until I had finished reviewing v2. I just replied to that mail in the >> v1 thread, but basically you need to figure out what is necessary here >> and let us know. If I have to figure it out it likely isn't going to >> get done with enough soak time prior to the upcoming merge window. > > Steve? I was hoping you could chime in here. > > I'd just include it for completeness unless Steve thinks it will stand > on its own and doesn't want the overhead. If that's the argument, I'd rather just include it. We've been burned by not including stuff in the past and fixing those omissions has proven to be a major source of contention. -- paul moore www.paul-moore.com
Re: [PATCH ghak21 V2 3/4] audit: add refused symlink to audit_names
On 2018-03-12 11:53, Paul Moore wrote:
> On Mon, Mar 12, 2018 at 11:26 AM, Richard Guy Briggs wrote:
> > On 2018-03-12 11:12, Paul Moore wrote:
> >> On Mon, Mar 12, 2018 at 2:31 AM, Richard Guy Briggs
> >> wrote:
> >> > Audit link denied events for symlinks had duplicate PATH records rather
> >> > than just updating the existing PATH record. Update the symlink's PATH
> >> > record with the current dentry and inode information.
> >> >
> >> > See: https://github.com/linux-audit/audit-kernel/issues/21
> >> > Signed-off-by: Richard Guy Briggs
> >> > ---
> >> > fs/namei.c | 1 +
> >> > 1 file changed, 1 insertion(+)
> >>
> >> Why didn't you include this in patch 4/4 like I asked during the
> >> previous review?
> >
> > Please see the last comment of:
> > https://www.redhat.com/archives/linux-audit/2018-March/msg00070.html
>
> Yes, I just saw that ... I hadn't seen your replies on the v1 patches
> until I had finished reviewing v2. I just replied to that mail in the
> v1 thread, but basically you need to figure out what is necessary here
> and let us know. If I have to figure it out it likely isn't going to
> get done with enough soak time prior to the upcoming merge window.
Steve? I was hoping you could chime in here.
I'd just include it for completeness unless Steve thinks it will stand
on its own and doesn't want the overhead.
> >> > diff --git a/fs/namei.c b/fs/namei.c
> >> > index 50d2533..00f5041 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", &nd->stack[0].link);
> >> > return -EACCES;
> >> > }
> >>
> >> paul moore
> >
> > - RGB
>
> 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
Re: [PATCH ghak21 V2 3/4] audit: add refused symlink to audit_names
On Mon, Mar 12, 2018 at 11:26 AM, Richard Guy Briggs wrote:
> On 2018-03-12 11:12, Paul Moore wrote:
>> On Mon, Mar 12, 2018 at 2:31 AM, Richard Guy Briggs wrote:
>> > Audit link denied events for symlinks had duplicate PATH records rather
>> > than just updating the existing PATH record. Update the symlink's PATH
>> > record with the current dentry and inode information.
>> >
>> > See: https://github.com/linux-audit/audit-kernel/issues/21
>> > Signed-off-by: Richard Guy Briggs
>> > ---
>> > fs/namei.c | 1 +
>> > 1 file changed, 1 insertion(+)
>>
>> Why didn't you include this in patch 4/4 like I asked during the
>> previous review?
>
> Please see the last comment of:
> https://www.redhat.com/archives/linux-audit/2018-March/msg00070.html
Yes, I just saw that ... I hadn't seen your replies on the v1 patches
until I had finished reviewing v2. I just replied to that mail in the
v1 thread, but basically you need to figure out what is necessary here
and let us know. If I have to figure it out it likely isn't going to
get done with enough soak time prior to the upcoming merge window.
>> > diff --git a/fs/namei.c b/fs/namei.c
>> > index 50d2533..00f5041 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", &nd->stack[0].link);
>> > return -EACCES;
>> > }
>>
>> 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
--
paul moore
www.paul-moore.com
Re: [PATCH ghak21 V2 3/4] audit: add refused symlink to audit_names
On 2018-03-12 11:12, Paul Moore wrote:
> On Mon, Mar 12, 2018 at 2:31 AM, Richard Guy Briggs wrote:
> > Audit link denied events for symlinks had duplicate PATH records rather
> > than just updating the existing PATH record. Update the symlink's PATH
> > record with the current dentry and inode information.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/21
> > Signed-off-by: Richard Guy Briggs
> > ---
> > fs/namei.c | 1 +
> > 1 file changed, 1 insertion(+)
>
> Why didn't you include this in patch 4/4 like I asked during the
> previous review?
Please see the last comment of:
https://www.redhat.com/archives/linux-audit/2018-March/msg00070.html
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 50d2533..00f5041 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", &nd->stack[0].link);
> > return -EACCES;
> > }
>
> 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
Re: [PATCH ghak21 V2 3/4] audit: add refused symlink to audit_names
On Mon, Mar 12, 2018 at 2:31 AM, Richard Guy Briggs wrote:
> Audit link denied events for symlinks had duplicate PATH records rather
> than just updating the existing PATH record. Update the symlink's PATH
> record with the current dentry and inode information.
>
> See: https://github.com/linux-audit/audit-kernel/issues/21
> Signed-off-by: Richard Guy Briggs
> ---
> fs/namei.c | 1 +
> 1 file changed, 1 insertion(+)
Why didn't you include this in patch 4/4 like I asked during the
previous review?
> diff --git a/fs/namei.c b/fs/namei.c
> index 50d2533..00f5041 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", &nd->stack[0].link);
> return -EACCES;
> }
> --
> 1.8.3.1
--
paul moore
www.paul-moore.com
[PATCH ghak21 V2 3/4] audit: add refused symlink to audit_names
Audit link denied events for symlinks had duplicate PATH records rather
than just updating the existing PATH record. Update the symlink's PATH
record with the current dentry and inode information.
See: https://github.com/linux-audit/audit-kernel/issues/21
Signed-off-by: Richard Guy Briggs
---
fs/namei.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/fs/namei.c b/fs/namei.c
index 50d2533..00f5041 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", &nd->stack[0].link);
return -EACCES;
}
--
1.8.3.1
