[RFC PATCH ghak21 0/4] audit: address ANOM_LINK excess records

[RFC PATCH ghak21 0/4] audit: address ANOM_LINK excess records

[Richard Guy Briggs]

Audit link denied events were being unexpectedly produced in a disjoint
way when audit was disabled, and when they were expected, there were
duplicate PATH records.  This patchset addresses both issues for
symlinks and hardlinks.

This was introduced with
	commit b24a30a7305418ff138ff51776fc555ec57c011a
	("audit: fix event coverage of AUDIT_ANOM_LINK")
	commit a51d9eaa41866ab6b4b6ecad7b621f8b66ece0dc
	("fs: add link restriction audit reporting")

Here are the resulting events:

symlink:
type=PROCTITLE msg=audit(02/14/2018 04:40:21.635:238) : proctitle=cat my-passwd
type=PATH msg=audit(02/14/2018 04:40:21.635:238) : item=1 name=/tmp/my-passwd inode=17618 dev=00:27 mode=link,777 ouid=rgb ogid=rgb rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0
type=PATH msg=audit(02/14/2018 04:40:21.635:238) : item=0 name=/tmp inode=13446 dev=00:27 mode=dir,sticky,777 ouid=root ogid=root rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype=PARENT cap_fp=none cap_fi=none cap_fe=0 cap_fver=0
type=CWD msg=audit(02/14/2018 04:40:21.635:238) : cwd=/tmp
type=SYSCALL msg=audit(02/14/2018 04:40:21.635:238) : arch=x86_64 syscall=openat success=no exit=EACCES(Permission denied) a0=0xffffff9c a1=0x7ffc6c1acdda 
a2=O_RDONLY a3=0x0 items=2 ppid=549 pid=606 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 ses=1 comm=
cat exe=/usr/bin/cat subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
type=ANOM_LINK msg=audit(02/14/2018 04:40:21.635:238) : op=follow_link ppid=549 pid=606 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=roo
t sgid=root fsgid=root tty=ttyS0 ses=1 comm=cat exe=/usr/bin/cat subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 res=no
----
hardlink:
type=PROCTITLE msg=audit(02/14/2018 04:40:25.373:239) : proctitle=ln test test-ln
type=PATH msg=audit(02/14/2018 04:40:25.373:239) : item=1 name=/tmp inode=13446 dev=00:27 mode=dir,sticky,777 ouid=root ogid=root rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype=PARENT cap_fp=none cap_fi=none cap_fe=0 cap_fver=0
type=PATH msg=audit(02/14/2018 04:40:25.373:239) : item=0 name=test inode=17619 dev=00:27 mode=file,700 ouid=root ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0
type=CWD msg=audit(02/14/2018 04:40:25.373:239) : cwd=/tmp
type=SYSCALL msg=audit(02/14/2018 04:40:25.373:239) : arch=x86_64 syscall=linkat success=no exit=EPERM(Operation not permitted) a0=0xffffff9c a1=0x7fffe6c3f628 a2=0xffffff9c a3=0x7fffe6c3f62d items=2 ppid=578 pid=607 auid=rgb uid=rgb gid=rgb euid=rgb suid=rgb fsuid=rgb egid=rgb sgid=rgb fsgid=rgb tty=pts0 ses=3 comm=ln exe=/usr/bin/ln subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
type=ANOM_LINK msg=audit(02/14/2018 04:40:25.373:239) : op=linkat ppid=578 pid=607 auid=rgb uid=rgb gid=rgb euid=rgb suid=rgb fsuid=rgb egid=rgb sgid=rgb fsgid=rgb tty=pts0 ses=3 comm=ln exe=/usr/bin/ln subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 res=no

The remaining problem is how to address this when syscall logging is
disabled since it needs a parent path record and/or a CWD record to
complete it.  It could also use a proctitle record too.  In fact, it
looks like we need a way to have multiple auxiliary records to support
an arbitrary record.  Comments please.

See: https://github.com/linux-audit/audit-kernel/issues/21
See also: https://github.com/linux-audit/audit-kernel/issues/51

Richard Guy Briggs (4):
  audit: make ANOM_LINK obey audit_enabled and audit_dummy_context
  audit: link denied should not directly generate PATH record
  audit: add refused symlink to audit_names
  audit: add parent of refused symlink to audit_names

 fs/namei.c     | 10 ++++++++++
 kernel/audit.c | 13 ++-----------
 2 files changed, 12 insertions(+), 11 deletions(-)

-- 
1.8.3.1

[RFC PATCH ghak21 1/4] audit: make ANOM_LINK obey audit_enabled and audit_dummy_context

[Richard Guy Briggs]

Audit link denied events emit disjointed records when audit is disabled.
No records should be emitted when audit is disabled.

See: https://github.com/linux-audit/audit-kernel/issues/21
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/audit.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/audit.c b/kernel/audit.c
index 227db99..4c3fd24 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -2261,6 +2261,9 @@ void audit_log_link_denied(const char *operation, const struct path *link)
 	struct audit_buffer *ab;
 	struct audit_names *name;
 
+	if (!audit_enabled || audit_dummy_context())
+		return;
+
 	name = kzalloc(sizeof(*name), GFP_NOFS);
 	if (!name)
 		return;
-- 
1.8.3.1

[RFC PATCH ghak21 2/4] audit: link denied should not directly generate PATH record

[Richard Guy Briggs]

Audit link denied events generate duplicate PATH records which disagree
in different ways from symlink and hardlink denials.
audit_log_link_denied() should not directly generate PATH records.

See: https://github.com/linux-audit/audit-kernel/issues/21
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/audit.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 4c3fd24..683b249 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -2259,31 +2259,19 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
 void audit_log_link_denied(const char *operation, const struct path *link)
 {
 	struct audit_buffer *ab;
-	struct audit_names *name;
 
 	if (!audit_enabled || audit_dummy_context())
 		return;
 
-	name = kzalloc(sizeof(*name), GFP_NOFS);
-	if (!name)
-		return;
-
 	/* Generate AUDIT_ANOM_LINK with subject, operation, outcome. */
 	ab = audit_log_start(current->audit_context, GFP_KERNEL,
 			     AUDIT_ANOM_LINK);
 	if (!ab)
-		goto out;
+		return;
 	audit_log_format(ab, "op=%s", operation);
 	audit_log_task_info(ab, current);
 	audit_log_format(ab, " res=0");
 	audit_log_end(ab);
-
-	/* Generate AUDIT_PATH record with object. */
-	name->type = AUDIT_TYPE_NORMAL;
-	audit_copy_inode(name, link->dentry, d_backing_inode(link->dentry));
-	audit_log_name(current->audit_context, name, link, 0, NULL);
-out:
-	kfree(name);
 }
 
 /**
-- 
1.8.3.1

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

[Richard Guy Briggs]

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 <rgb@redhat.com>
---
 fs/namei.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/namei.c b/fs/namei.c
index 9cc91fb..0edf133 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

[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 <rgb@redhat.com>
---
 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(&nd->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", &nd->stack[0].link);
 	return -EACCES;
-- 
1.8.3.1

Re: [RFC PATCH ghak21 0/4] audit: address ANOM_LINK excess records

[Steve Grubb]

On Wednesday, February 14, 2018 11:18:20 AM EST Richard Guy Briggs wrote:
> Audit link denied events were being unexpectedly produced in a disjoint
> way when audit was disabled, and when they were expected, there were
> duplicate PATH records.  This patchset addresses both issues for
> symlinks and hardlinks.
> 
> This was introduced with
> 	commit b24a30a7305418ff138ff51776fc555ec57c011a
> 	("audit: fix event coverage of AUDIT_ANOM_LINK")
> 	commit a51d9eaa41866ab6b4b6ecad7b621f8b66ece0dc
> 	("fs: add link restriction audit reporting")
> 
> Here are the resulting events:

Have these been tested with ausearch-test?

> symlink:
> type=PROCTITLE msg=audit(02/14/2018 04:40:21.635:238) : proctitle=cat
> my-passwd type=PATH msg=audit(02/14/2018 04:40:21.635:238) : item=1
> name=/tmp/my-passwd inode=17618 dev=00:27 mode=link,777 ouid=rgb ogid=rgb
> rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=NORMAL
> cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 type=PATH msg=audit(02/14/2018
> 04:40:21.635:238) : item=0 name=/tmp inode=13446 dev=00:27
> mode=dir,sticky,777 ouid=root ogid=root rdev=00:00
> obj=system_u:object_r:tmp_t:s0 nametype=PARENT cap_fp=none cap_fi=none
> cap_fe=0 cap_fver=0 type=CWD msg=audit(02/14/2018 04:40:21.635:238) :
> cwd=/tmp
> type=SYSCALL msg=audit(02/14/2018 04:40:21.635:238) : arch=x86_64
> syscall=openat success=no exit=EACCES(Permission denied) a0=0xffffff9c
> a1=0x7ffc6c1acdda a2=O_RDONLY a3=0x0 items=2 ppid=549 pid=606 auid=root
> uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root
> fsgid=root tty=ttyS0 ses=1 comm= cat exe=/usr/bin/cat
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
> type=ANOM_LINK msg=audit(02/14/2018 04:40:21.635:238) : op=follow_link
> ppid=549 pid=606 auid=root uid=root gid=root euid=root suid=root
> fsuid=root egid=roo t sgid=root fsgid=root tty=ttyS0 ses=1 comm=cat
> exe=/usr/bin/cat
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 res=no

This record duplicates the SYSCALL event except for the op field. I would 
suggest that is the only field needed.

> ----
> hardlink:
> type=PROCTITLE msg=audit(02/14/2018 04:40:25.373:239) : proctitle=ln test
> test-ln type=PATH msg=audit(02/14/2018 04:40:25.373:239) : item=1
> name=/tmp inode=13446 dev=00:27 mode=dir,sticky,777 ouid=root ogid=root
> rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype=PARENT cap_fp=none
> cap_fi=none cap_fe=0 cap_fver=0 type=PATH msg=audit(02/14/2018
> 04:40:25.373:239) : item=0 name=test inode=17619 dev=00:27 mode=file,700
> ouid=root ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0
> nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 type=CWD
> msg=audit(02/14/2018 04:40:25.373:239) : cwd=/tmp
> type=SYSCALL msg=audit(02/14/2018 04:40:25.373:239) : arch=x86_64
> syscall=linkat success=no exit=EPERM(Operation not permitted)
> a0=0xffffff9c a1=0x7fffe6c3f628 a2=0xffffff9c a3=0x7fffe6c3f62d items=2
> ppid=578 pid=607 auid=rgb uid=rgb gid=rgb euid=rgb suid=rgb fsuid=rgb
> egid=rgb sgid=rgb fsgid=rgb tty=pts0 ses=3 comm=ln exe=/usr/bin/ln
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
> type=ANOM_LINK msg=audit(02/14/2018 04:40:25.373:239) : op=linkat ppid=578
> pid=607 auid=rgb uid=rgb gid=rgb euid=rgb suid=rgb fsuid=rgb egid=rgb
> sgid=rgb fsgid=rgb tty=pts0 ses=3 comm=ln exe=/usr/bin/ln
> subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 res=no
> 
> The remaining problem is how to address this when syscall logging is
> disabled since it needs a parent path record and/or a CWD record to
> complete it.  It could also use a proctitle record too.  In fact, it
> looks like we need a way to have multiple auxiliary records to support
> an arbitrary record.  Comments please.

Perhaps this can only be emitted correctly with SYSCALL auditing enabled. 
Otherwise, the event should stand completely on its own without syscall and 
path records. The information from them can be added, but it risks hitting 
the record size limit.

-Steve

> See: https://github.com/linux-audit/audit-kernel/issues/21
> See also: https://github.com/linux-audit/audit-kernel/issues/51
> 
> Richard Guy Briggs (4):
>   audit: make ANOM_LINK obey audit_enabled and audit_dummy_context
>   audit: link denied should not directly generate PATH record
>   audit: add refused symlink to audit_names
>   audit: add parent of refused symlink to audit_names
> 
>  fs/namei.c     | 10 ++++++++++
>  kernel/audit.c | 13 ++-----------
>  2 files changed, 12 insertions(+), 11 deletions(-)

Re: [RFC PATCH ghak21 1/4] audit: make ANOM_LINK obey audit_enabled and audit_dummy_context

[Kees Cook]

On Wed, Feb 14, 2018 at 8:18 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Audit link denied events emit disjointed records when audit is disabled.
> No records should be emitted when audit is disabled.
>
> See: https://github.com/linux-audit/audit-kernel/issues/21
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/audit.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 227db99..4c3fd24 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2261,6 +2261,9 @@ void audit_log_link_denied(const char *operation, const struct path *link)
>         struct audit_buffer *ab;
>         struct audit_names *name;
>
> +       if (!audit_enabled || audit_dummy_context())
> +               return;
> +
>         name = kzalloc(sizeof(*name), GFP_NOFS);
>         if (!name)
>                 return;

Doesn't this means errors here would be silent if audit isn't enabled?
I don't that; sysadmins should see this notification regardless of the
audit state...

-Kees

-- 
Kees Cook
Pixel Security

Re: [RFC PATCH ghak21 1/4] audit: make ANOM_LINK obey audit_enabled and audit_dummy_context

[Richard Guy Briggs]

On 2018-02-14 09:51, Kees Cook wrote:
> On Wed, Feb 14, 2018 at 8:18 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Audit link denied events emit disjointed records when audit is disabled.
> > No records should be emitted when audit is disabled.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/21
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  kernel/audit.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 227db99..4c3fd24 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -2261,6 +2261,9 @@ void audit_log_link_denied(const char *operation, const struct path *link)
> >         struct audit_buffer *ab;
> >         struct audit_names *name;
> >
> > +       if (!audit_enabled || audit_dummy_context())
> > +               return;
> > +
> >         name = kzalloc(sizeof(*name), GFP_NOFS);
> >         if (!name)
> >                 return;
> 
> Doesn't this means errors here would be silent if audit isn't enabled?
> I don't that; sysadmins should see this notification regardless of the
> audit state...

This is a user error and not a system error, so I would think if system
auditing is disabled, they don't care about this kind of error.

Steve?

> -Kees

- RGB

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

Re: [RFC PATCH ghak21 0/4] audit: address ANOM_LINK excess records

[Richard Guy Briggs]

On 2018-02-14 11:49, Steve Grubb wrote:
> On Wednesday, February 14, 2018 11:18:20 AM EST Richard Guy Briggs wrote:
> > Audit link denied events were being unexpectedly produced in a disjoint
> > way when audit was disabled, and when they were expected, there were
> > duplicate PATH records.  This patchset addresses both issues for
> > symlinks and hardlinks.
> > 
> > This was introduced with
> > 	commit b24a30a7305418ff138ff51776fc555ec57c011a
> > 	("audit: fix event coverage of AUDIT_ANOM_LINK")
> > 	commit a51d9eaa41866ab6b4b6ecad7b621f8b66ece0dc
> > 	("fs: add link restriction audit reporting")
> > 
> > Here are the resulting events:
> 
> Have these been tested with ausearch-test?

Not yet.

> > symlink:
> > type=PROCTITLE msg=audit(02/14/2018 04:40:21.635:238) : proctitle=cat
> > my-passwd type=PATH msg=audit(02/14/2018 04:40:21.635:238) : item=1
> > name=/tmp/my-passwd inode=17618 dev=00:27 mode=link,777 ouid=rgb ogid=rgb
> > rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=NORMAL
> > cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 type=PATH msg=audit(02/14/2018
> > 04:40:21.635:238) : item=0 name=/tmp inode=13446 dev=00:27
> > mode=dir,sticky,777 ouid=root ogid=root rdev=00:00
> > obj=system_u:object_r:tmp_t:s0 nametype=PARENT cap_fp=none cap_fi=none
> > cap_fe=0 cap_fver=0 type=CWD msg=audit(02/14/2018 04:40:21.635:238) :
> > cwd=/tmp
> > type=SYSCALL msg=audit(02/14/2018 04:40:21.635:238) : arch=x86_64
> > syscall=openat success=no exit=EACCES(Permission denied) a0=0xffffff9c
> > a1=0x7ffc6c1acdda a2=O_RDONLY a3=0x0 items=2 ppid=549 pid=606 auid=root
> > uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root
> > fsgid=root tty=ttyS0 ses=1 comm= cat exe=/usr/bin/cat
> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
> > type=ANOM_LINK msg=audit(02/14/2018 04:40:21.635:238) : op=follow_link
> > ppid=549 pid=606 auid=root uid=root gid=root euid=root suid=root
> > fsuid=root egid=roo t sgid=root fsgid=root tty=ttyS0 ses=1 comm=cat
> > exe=/usr/bin/cat
> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 res=no
> 
> This record duplicates the SYSCALL event except for the op field. I would 
> suggest that is the only field needed.

Agreed, but at the moment, removal of fields isn't possible unless there
is a conflict, and even then the value should simply be corrected if
possible.

> > ----
> > hardlink:
> > type=PROCTITLE msg=audit(02/14/2018 04:40:25.373:239) : proctitle=ln test
> > test-ln type=PATH msg=audit(02/14/2018 04:40:25.373:239) : item=1
> > name=/tmp inode=13446 dev=00:27 mode=dir,sticky,777 ouid=root ogid=root
> > rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype=PARENT cap_fp=none
> > cap_fi=none cap_fe=0 cap_fver=0 type=PATH msg=audit(02/14/2018
> > 04:40:25.373:239) : item=0 name=test inode=17619 dev=00:27 mode=file,700
> > ouid=root ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0
> > nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 type=CWD
> > msg=audit(02/14/2018 04:40:25.373:239) : cwd=/tmp
> > type=SYSCALL msg=audit(02/14/2018 04:40:25.373:239) : arch=x86_64
> > syscall=linkat success=no exit=EPERM(Operation not permitted)
> > a0=0xffffff9c a1=0x7fffe6c3f628 a2=0xffffff9c a3=0x7fffe6c3f62d items=2
> > ppid=578 pid=607 auid=rgb uid=rgb gid=rgb euid=rgb suid=rgb fsuid=rgb
> > egid=rgb sgid=rgb fsgid=rgb tty=pts0 ses=3 comm=ln exe=/usr/bin/ln
> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
> > type=ANOM_LINK msg=audit(02/14/2018 04:40:25.373:239) : op=linkat ppid=578
> > pid=607 auid=rgb uid=rgb gid=rgb euid=rgb suid=rgb fsuid=rgb egid=rgb
> > sgid=rgb fsgid=rgb tty=pts0 ses=3 comm=ln exe=/usr/bin/ln
> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 res=no
> > 
> > The remaining problem is how to address this when syscall logging is
> > disabled since it needs a parent path record and/or a CWD record to
> > complete it.  It could also use a proctitle record too.  In fact, it
> > looks like we need a way to have multiple auxiliary records to support
> > an arbitrary record.  Comments please.
> 
> Perhaps this can only be emitted correctly with SYSCALL auditing enabled. 
> Otherwise, the event should stand completely on its own without syscall and 
> path records. The information from them can be added, but it risks hitting 
> the record size limit.

As Paul just pointed out (which rang a bell...) in:
	https://github.com/linux-audit/audit-kernel/issues/51#issuecomment-365759325
CONFIG_AUDITSYSCALL is now forced on and if you sabbotage your
audit.rules with -a task,never, your warranty is void.

So now, the lurking questions in the back of my head about the
availability of syscall records has been alleviated and we should always
see a syscall record available unless an audit rule says we are not
interested.

> -Steve
> 
> > See: https://github.com/linux-audit/audit-kernel/issues/21
> > See also: https://github.com/linux-audit/audit-kernel/issues/51
> > 
> > Richard Guy Briggs (4):
> >   audit: make ANOM_LINK obey audit_enabled and audit_dummy_context
> >   audit: link denied should not directly generate PATH record
> >   audit: add refused symlink to audit_names
> >   audit: add parent of refused symlink to audit_names
> > 
> >  fs/namei.c     | 10 ++++++++++
> >  kernel/audit.c | 13 ++-----------
> >  2 files changed, 12 insertions(+), 11 deletions(-)
> 
> 
> 
> 

- RGB

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

Re: [RFC PATCH ghak21 1/4] audit: make ANOM_LINK obey audit_enabled and audit_dummy_context

[Kees Cook]

On Wed, Feb 14, 2018 at 6:33 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-02-14 09:51, Kees Cook wrote:
>> On Wed, Feb 14, 2018 at 8:18 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > Audit link denied events emit disjointed records when audit is disabled.
>> > No records should be emitted when audit is disabled.
>> >
>> > See: https://github.com/linux-audit/audit-kernel/issues/21
>> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> > ---
>> >  kernel/audit.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/kernel/audit.c b/kernel/audit.c
>> > index 227db99..4c3fd24 100644
>> > --- a/kernel/audit.c
>> > +++ b/kernel/audit.c
>> > @@ -2261,6 +2261,9 @@ void audit_log_link_denied(const char *operation, const struct path *link)
>> >         struct audit_buffer *ab;
>> >         struct audit_names *name;
>> >
>> > +       if (!audit_enabled || audit_dummy_context())
>> > +               return;
>> > +
>> >         name = kzalloc(sizeof(*name), GFP_NOFS);
>> >         if (!name)
>> >                 return;
>>
>> Doesn't this means errors here would be silent if audit isn't enabled?
>> I don't that; sysadmins should see this notification regardless of the
>> audit state...
>
> This is a user error and not a system error, so I would think if system
> auditing is disabled, they don't care about this kind of error.

It could indicate an attack attempt...

-Kees

>
> Steve?
>
>> -Kees
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.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



-- 
Kees Cook
Pixel Security

Re: [RFC PATCH ghak21 1/4] audit: make ANOM_LINK obey audit_enabled and audit_dummy_context

[Paul Moore]

On Thu, Feb 15, 2018 at 1:16 AM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Feb 14, 2018 at 6:33 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> On 2018-02-14 09:51, Kees Cook wrote:
>>> On Wed, Feb 14, 2018 at 8:18 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>>> > Audit link denied events emit disjointed records when audit is disabled.
>>> > No records should be emitted when audit is disabled.
>>> >
>>> > See: https://github.com/linux-audit/audit-kernel/issues/21
>>> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>>> > ---
>>> >  kernel/audit.c | 3 +++
>>> >  1 file changed, 3 insertions(+)
>>> >
>>> > diff --git a/kernel/audit.c b/kernel/audit.c
>>> > index 227db99..4c3fd24 100644
>>> > --- a/kernel/audit.c
>>> > +++ b/kernel/audit.c
>>> > @@ -2261,6 +2261,9 @@ void audit_log_link_denied(const char *operation, const struct path *link)
>>> >         struct audit_buffer *ab;
>>> >         struct audit_names *name;
>>> >
>>> > +       if (!audit_enabled || audit_dummy_context())
>>> > +               return;
>>> > +
>>> >         name = kzalloc(sizeof(*name), GFP_NOFS);
>>> >         if (!name)
>>> >                 return;
>>>
>>> Doesn't this means errors here would be silent if audit isn't enabled?
>>> I don't that; sysadmins should see this notification regardless of the
>>> audit state...
>>
>> This is a user error and not a system error, so I would think if system
>> auditing is disabled, they don't care about this kind of error.
>
> It could indicate an attack attempt...

We get beat up by several folks when we emit audit records with audit
disabled, and they have a very valid point.

I'm not arguing that the information isn't useful, I'm arguing that if
you are interested in the sort of information that audit provides you
should enable audit.  :)

-- 
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 <rgb@redhat.com> 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 <rgb@redhat.com>
> ---
>  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(&nd->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", &nd->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 <rgb@redhat.com> 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 <rgb@redhat.com>
> > ---
> >  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(&nd->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", &nd->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

- RGB

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

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 <rgb@redhat.com> wrote:
> On 2018-02-15 18:34, Paul Moore wrote:
>> On Wed, Feb 14, 2018 at 11:18 AM, Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
>> > ---
>> >  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(&nd->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", &nd->stack[0].link);
>> >         return -EACCES;
>> > --
>> > 1.8.3.1

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak21 0/4] audit: address ANOM_LINK excess records

[Richard Guy Briggs]

On 2018-02-14 22:46, Richard Guy Briggs wrote:
> On 2018-02-14 11:49, Steve Grubb wrote:
> > On Wednesday, February 14, 2018 11:18:20 AM EST Richard Guy Briggs wrote:
> > > Audit link denied events were being unexpectedly produced in a disjoint
> > > way when audit was disabled, and when they were expected, there were
> > > duplicate PATH records.  This patchset addresses both issues for
> > > symlinks and hardlinks.
> > > 
> > > This was introduced with
> > > 	commit b24a30a7305418ff138ff51776fc555ec57c011a
> > > 	("audit: fix event coverage of AUDIT_ANOM_LINK")
> > > 	commit a51d9eaa41866ab6b4b6ecad7b621f8b66ece0dc
> > > 	("fs: add link restriction audit reporting")
> > > 
> > > Here are the resulting events:
> > 
> > Have these been tested with ausearch-test?
> 
> Not yet.

I should have reported that a day or two later I ran the ausearch-test
which passed.

> > > symlink:
> > > type=PROCTITLE msg=audit(02/14/2018 04:40:21.635:238) : proctitle=cat
> > > my-passwd type=PATH msg=audit(02/14/2018 04:40:21.635:238) : item=1
> > > name=/tmp/my-passwd inode=17618 dev=00:27 mode=link,777 ouid=rgb ogid=rgb
> > > rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=NORMAL
> > > cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 type=PATH msg=audit(02/14/2018
> > > 04:40:21.635:238) : item=0 name=/tmp inode=13446 dev=00:27
> > > mode=dir,sticky,777 ouid=root ogid=root rdev=00:00
> > > obj=system_u:object_r:tmp_t:s0 nametype=PARENT cap_fp=none cap_fi=none
> > > cap_fe=0 cap_fver=0 type=CWD msg=audit(02/14/2018 04:40:21.635:238) :
> > > cwd=/tmp
> > > type=SYSCALL msg=audit(02/14/2018 04:40:21.635:238) : arch=x86_64
> > > syscall=openat success=no exit=EACCES(Permission denied) a0=0xffffff9c
> > > a1=0x7ffc6c1acdda a2=O_RDONLY a3=0x0 items=2 ppid=549 pid=606 auid=root
> > > uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root
> > > fsgid=root tty=ttyS0 ses=1 comm= cat exe=/usr/bin/cat
> > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
> > > type=ANOM_LINK msg=audit(02/14/2018 04:40:21.635:238) : op=follow_link
> > > ppid=549 pid=606 auid=root uid=root gid=root euid=root suid=root
> > > fsuid=root egid=roo t sgid=root fsgid=root tty=ttyS0 ses=1 comm=cat
> > > exe=/usr/bin/cat
> > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 res=no
> > 
> > This record duplicates the SYSCALL event except for the op field. I would 
> > suggest that is the only field needed.
> 
> Agreed, but at the moment, removal of fields isn't possible unless there
> is a conflict, and even then the value should simply be corrected if
> possible.
> 
> > > ----
> > > hardlink:
> > > type=PROCTITLE msg=audit(02/14/2018 04:40:25.373:239) : proctitle=ln test
> > > test-ln type=PATH msg=audit(02/14/2018 04:40:25.373:239) : item=1
> > > name=/tmp inode=13446 dev=00:27 mode=dir,sticky,777 ouid=root ogid=root
> > > rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype=PARENT cap_fp=none
> > > cap_fi=none cap_fe=0 cap_fver=0 type=PATH msg=audit(02/14/2018
> > > 04:40:25.373:239) : item=0 name=test inode=17619 dev=00:27 mode=file,700
> > > ouid=root ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0
> > > nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 type=CWD
> > > msg=audit(02/14/2018 04:40:25.373:239) : cwd=/tmp
> > > type=SYSCALL msg=audit(02/14/2018 04:40:25.373:239) : arch=x86_64
> > > syscall=linkat success=no exit=EPERM(Operation not permitted)
> > > a0=0xffffff9c a1=0x7fffe6c3f628 a2=0xffffff9c a3=0x7fffe6c3f62d items=2
> > > ppid=578 pid=607 auid=rgb uid=rgb gid=rgb euid=rgb suid=rgb fsuid=rgb
> > > egid=rgb sgid=rgb fsgid=rgb tty=pts0 ses=3 comm=ln exe=/usr/bin/ln
> > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)
> > > type=ANOM_LINK msg=audit(02/14/2018 04:40:25.373:239) : op=linkat ppid=578
> > > pid=607 auid=rgb uid=rgb gid=rgb euid=rgb suid=rgb fsuid=rgb egid=rgb
> > > sgid=rgb fsgid=rgb tty=pts0 ses=3 comm=ln exe=/usr/bin/ln
> > > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 res=no
> > > 
> > > The remaining problem is how to address this when syscall logging is
> > > disabled since it needs a parent path record and/or a CWD record to
> > > complete it.  It could also use a proctitle record too.  In fact, it
> > > looks like we need a way to have multiple auxiliary records to support
> > > an arbitrary record.  Comments please.
> > 
> > Perhaps this can only be emitted correctly with SYSCALL auditing enabled. 
> > Otherwise, the event should stand completely on its own without syscall and 
> > path records. The information from them can be added, but it risks hitting 
> > the record size limit.
> 
> As Paul just pointed out (which rang a bell...) in:
> 	https://github.com/linux-audit/audit-kernel/issues/51#issuecomment-365759325
> CONFIG_AUDITSYSCALL is now forced on and if you sabbotage your
> audit.rules with -a task,never, your warranty is void.
> 
> So now, the lurking questions in the back of my head about the
> availability of syscall records has been alleviated and we should always
> see a syscall record available unless an audit rule says we are not
> interested.
> 
> > -Steve
> > 
> > > See: https://github.com/linux-audit/audit-kernel/issues/21
> > > See also: https://github.com/linux-audit/audit-kernel/issues/51
> > > 
> > > Richard Guy Briggs (4):
> > >   audit: make ANOM_LINK obey audit_enabled and audit_dummy_context
> > >   audit: link denied should not directly generate PATH record
> > >   audit: add refused symlink to audit_names
> > >   audit: add parent of refused symlink to audit_names
> > > 
> > >  fs/namei.c     | 10 ++++++++++
> > >  kernel/audit.c | 13 ++-----------
> > >  2 files changed, 12 insertions(+), 11 deletions(-)
> 
> - RGB

- RGB

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

Re: [RFC PATCH ghak21 1/4] audit: make ANOM_LINK obey audit_enabled and audit_dummy_context

[Paul Moore]

On Thu, Feb 15, 2018 at 5:51 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Feb 15, 2018 at 1:16 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Wed, Feb 14, 2018 at 6:33 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
>>> On 2018-02-14 09:51, Kees Cook wrote:
>>>> On Wed, Feb 14, 2018 at 8:18 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>>>> > Audit link denied events emit disjointed records when audit is disabled.
>>>> > No records should be emitted when audit is disabled.
>>>> >
>>>> > See: https://github.com/linux-audit/audit-kernel/issues/21
>>>> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>>>> > ---
>>>> >  kernel/audit.c | 3 +++
>>>> >  1 file changed, 3 insertions(+)
>>>> >
>>>> > diff --git a/kernel/audit.c b/kernel/audit.c
>>>> > index 227db99..4c3fd24 100644
>>>> > --- a/kernel/audit.c
>>>> > +++ b/kernel/audit.c
>>>> > @@ -2261,6 +2261,9 @@ void audit_log_link_denied(const char *operation, const struct path *link)
>>>> >         struct audit_buffer *ab;
>>>> >         struct audit_names *name;
>>>> >
>>>> > +       if (!audit_enabled || audit_dummy_context())
>>>> > +               return;
>>>> > +
>>>> >         name = kzalloc(sizeof(*name), GFP_NOFS);
>>>> >         if (!name)
>>>> >                 return;
>>>>
>>>> Doesn't this means errors here would be silent if audit isn't enabled?
>>>> I don't that; sysadmins should see this notification regardless of the
>>>> audit state...
>>>
>>> This is a user error and not a system error, so I would think if system
>>> auditing is disabled, they don't care about this kind of error.
>>
>> It could indicate an attack attempt...
>
> We get beat up by several folks when we emit audit records with audit
> disabled, and they have a very valid point.
>
> I'm not arguing that the information isn't useful, I'm arguing that if
> you are interested in the sort of information that audit provides you
> should enable audit.  :)

FYI, merged into audit/next.

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak21 2/4] audit: link denied should not directly generate PATH record

[Paul Moore]

On Wed, Feb 14, 2018 at 11:18 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Audit link denied events generate duplicate PATH records which disagree
> in different ways from symlink and hardlink denials.
> audit_log_link_denied() should not directly generate PATH records.
>
> See: https://github.com/linux-audit/audit-kernel/issues/21
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/audit.c | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)

Merged, thanks.

> diff --git a/kernel/audit.c b/kernel/audit.c
> index 4c3fd24..683b249 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2259,31 +2259,19 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
>  void audit_log_link_denied(const char *operation, const struct path *link)
>  {
>         struct audit_buffer *ab;
> -       struct audit_names *name;
>
>         if (!audit_enabled || audit_dummy_context())
>                 return;
>
> -       name = kzalloc(sizeof(*name), GFP_NOFS);
> -       if (!name)
> -               return;
> -
>         /* Generate AUDIT_ANOM_LINK with subject, operation, outcome. */
>         ab = audit_log_start(current->audit_context, GFP_KERNEL,
>                              AUDIT_ANOM_LINK);
>         if (!ab)
> -               goto out;
> +               return;
>         audit_log_format(ab, "op=%s", operation);
>         audit_log_task_info(ab, current);
>         audit_log_format(ab, " res=0");
>         audit_log_end(ab);
> -
> -       /* Generate AUDIT_PATH record with object. */
> -       name->type = AUDIT_TYPE_NORMAL;
> -       audit_copy_inode(name, link->dentry, d_backing_inode(link->dentry));
> -       audit_log_name(current->audit_context, name, link, 0, NULL);
> -out:
> -       kfree(name);
>  }
>
>  /**
> --
> 1.8.3.1
>



-- 
paul moore
www.paul-moore.com

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

[Paul Moore]

On Wed, Feb 14, 2018 at 11:18 AM, Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
> ---
>  fs/namei.c | 1 +
>  1 file changed, 1 insertion(+)

Merged.

> diff --git a/fs/namei.c b/fs/namei.c
> index 9cc91fb..0edf133 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

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

[Paul Moore]

On Thu, Mar 8, 2018 at 7:30 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Feb 14, 2018 at 11:18 AM, Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
>> ---
>>  fs/namei.c | 1 +
>>  1 file changed, 1 insertion(+)
>
> Merged.

Scratch that, not merged, although only because I think we need to
refactor patch 4/4 and the refactoring can/should encompass this
patch.

See my comments on 4/4.

>> diff --git a/fs/namei.c b/fs/namei.c
>> index 9cc91fb..0edf133 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

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 <rgb@redhat.com> 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 <rgb@redhat.com>
> ---
>  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(&nd->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", &nd->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-03-08 19:50, Paul Moore wrote:
> On Wed, Feb 14, 2018 at 11:18 AM, Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
> > ---
> >  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(&nd->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", &nd->stack[0].link);
> >
> >         return -EACCES;
> > --
> > 1.8.3.1
> 
> -- 
> paul moore
> www.paul-moore.com

- RGB

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

Re: [RFC PATCH ghak21 2/4] audit: link denied should not directly generate PATH record

[Richard Guy Briggs]

On 2018-03-08 19:26, Paul Moore wrote:
> On Wed, Feb 14, 2018 at 11:18 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Audit link denied events generate duplicate PATH records which disagree
> > in different ways from symlink and hardlink denials.
> > audit_log_link_denied() should not directly generate PATH records.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/21
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  kernel/audit.c | 14 +-------------
> >  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> Merged, thanks.

Self-NACK.  Please un-merge this RFC v1 version and merge instead the v2
patch since it removes the now-unnecessary struct path * parameter from
audit_log_link_denied().

> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 4c3fd24..683b249 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -2259,31 +2259,19 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
> >  void audit_log_link_denied(const char *operation, const struct path *link)
> >  {
> >         struct audit_buffer *ab;
> > -       struct audit_names *name;
> >
> >         if (!audit_enabled || audit_dummy_context())
> >                 return;
> >
> > -       name = kzalloc(sizeof(*name), GFP_NOFS);
> > -       if (!name)
> > -               return;
> > -
> >         /* Generate AUDIT_ANOM_LINK with subject, operation, outcome. */
> >         ab = audit_log_start(current->audit_context, GFP_KERNEL,
> >                              AUDIT_ANOM_LINK);
> >         if (!ab)
> > -               goto out;
> > +               return;
> >         audit_log_format(ab, "op=%s", operation);
> >         audit_log_task_info(ab, current);
> >         audit_log_format(ab, " res=0");
> >         audit_log_end(ab);
> > -
> > -       /* Generate AUDIT_PATH record with object. */
> > -       name->type = AUDIT_TYPE_NORMAL;
> > -       audit_copy_inode(name, link->dentry, d_backing_inode(link->dentry));
> > -       audit_log_name(current->audit_context, name, link, 0, NULL);
> > -out:
> > -       kfree(name);
> >  }
> >
> >  /**
> > --
> > 1.8.3.1
> >
> 
> 
> 
> -- 
> paul moore
> www.paul-moore.com

- RGB

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

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 <rgb@redhat.com> 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