[RFC PATCH ghak9 0/3] audit: Record the path of FDs passed to *at(2) syscalls

[RFC PATCH ghak9 0/3] audit: Record the path of FDs passed to *at(2) syscalls

[Ondrej Mosnacek]

This patchset is a prototype implementation of the feature requested in GHAK issue #9 [1]. I decided for a simple auxiliary record with just 2 fields (fd and path) that is emitted whenever we want to record the full path for a file descriptor passed to a syscall (e.g. the dirfd argument of openat(2)). I choose this approach because for some syscalls there is more than one file descriptor we might be interested in (a good example is the renameat(2) syscall).

The motivation for this feature (as I understand it) is to avoid the need to reconstruct the paths corresponding to the file descriptors passed to syscalls, as this might be difficult and time consuming or even impossible in case not all of the right sycalls are being logged. Note that it is always possible to disable these records by simply adding an exclude filter rule matching all records of type FD_PATH.

At this moment I only implement logging for a single syscall (openat(2)) to keep it simple. In the final version I plan to add support for other similar syscalls ()mkdirat, mknodeat, fchownat, ...).

Please let me know if the general approach and the proposed record format make sense to you so I can improve/complete the solution.

Thanks,
Ondrej

[1] https://github.com/linux-audit/audit-kernel/issues/9

[RFC PATCH ghak9 1/3] audit: Add AUDIT_FD_PATH auxiliary record type

[Ondrej Mosnacek]

This new record type is used to log the full path corresponding to some
important file descriptor used in a syscall.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 include/uapi/linux/audit.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 4e3eaba84175..d60041ae34a8 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -114,6 +114,7 @@
 #define AUDIT_REPLACE		1329	/* Replace auditd if this packet unanswerd */
 #define AUDIT_KERN_MODULE	1330	/* Kernel Module events */
 #define AUDIT_FANOTIFY		1331	/* Fanotify access decision */
+#define AUDIT_FD_PATH		1334	/* File descriptor path info */
 
 #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
 #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
-- 
2.17.1

Re: [RFC PATCH ghak9 1/3] audit: Add AUDIT_FD_PATH auxiliary record type

[Richard Guy Briggs]

On 2018-07-12 13:36, Ondrej Mosnacek wrote:
> This new record type is used to log the full path corresponding to some
> important file descriptor used in a syscall.
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  include/uapi/linux/audit.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 4e3eaba84175..d60041ae34a8 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -114,6 +114,7 @@
>  #define AUDIT_REPLACE		1329	/* Replace auditd if this packet unanswerd */
>  #define AUDIT_KERN_MODULE	1330	/* Kernel Module events */
>  #define AUDIT_FANOTIFY		1331	/* Fanotify access decision */
> +#define AUDIT_FD_PATH		1334	/* File descriptor path info */

The final message type number depends on other work in flight which may
or may not be accepted first, so don't count on this one being the
final.  Having said that, we usually use the next number in sequence
unless there is a hard dependence on another patchset.

This will be the maintainer's job to juggle all these when they are
merged upstream.  Unfortunately, that will make more work for the
corresponding user library patches that help identify this record type.

>  #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
>  #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */

- 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 ghak9 1/3] audit: Add AUDIT_FD_PATH auxiliary record type

[Ondrej Mosnacek]

On Fri, Jul 13, 2018 at 4:53 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-07-12 13:36, Ondrej Mosnacek wrote:
> > This new record type is used to log the full path corresponding to some
> > important file descriptor used in a syscall.
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  include/uapi/linux/audit.h | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 4e3eaba84175..d60041ae34a8 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -114,6 +114,7 @@
> >  #define AUDIT_REPLACE                1329    /* Replace auditd if this packet unanswerd */
> >  #define AUDIT_KERN_MODULE    1330    /* Kernel Module events */
> >  #define AUDIT_FANOTIFY               1331    /* Fanotify access decision */
> > +#define AUDIT_FD_PATH                1334    /* File descriptor path info */
>
> The final message type number depends on other work in flight which may
> or may not be accepted first, so don't count on this one being the
> final.  Having said that, we usually use the next number in sequence
> unless there is a hard dependence on another patchset.
>
> This will be the maintainer's job to juggle all these when they are
> merged upstream.  Unfortunately, that will make more work for the
> corresponding user library patches that help identify this record type.

Of course, I set it to a different number mainly for easier testing on
my side, I can set it to (previous+1) in the later "production-ready"
patchsets.

>
> >  #define AUDIT_AVC            1400    /* SE Linux avc denial or grant */
> >  #define AUDIT_SELINUX_ERR    1401    /* Internal SE Linux Errors */
>
> - 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

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

[RFC PATCH ghak9 2/3] audit: Add a function to log the path of an fd

[Ondrej Mosnacek]

The function logs an FD_PATH record that is associated with the current
syscall. The record associates the given file descriptor with the
current path of the file under it (if it is possible to retrieve such
path). The reader of the log can then logically connect this information
to the syscall arguments from the SYSCALL record (based on the syscall
type).

Record format:
type=FD_PATH msg=audit(...): fd=<file descriptor> path=<file path>

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 include/linux/audit.h | 10 ++++++++++
 kernel/auditsc.c      | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 9334fbef7bae..95d338bb603a 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -356,6 +356,7 @@ extern void __audit_log_capset(const struct cred *new, const struct cred *old);
 extern void __audit_mmap_fd(int fd, int flags);
 extern void __audit_log_kern_module(char *name);
 extern void __audit_fanotify(unsigned int response);
+extern void __audit_fd_path(int fd);
 
 static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
 {
@@ -458,6 +459,12 @@ static inline void audit_fanotify(unsigned int response)
 		__audit_fanotify(response);
 }
 
+static inline void audit_fd_path(int fd)
+{
+	if (fd >= 0 && !audit_dummy_context())
+		__audit_fd_path(fd);
+}
+
 extern int audit_n_rules;
 extern int audit_signals;
 #else /* CONFIG_AUDITSYSCALL */
@@ -584,6 +591,9 @@ static inline void audit_log_kern_module(char *name)
 static inline void audit_fanotify(unsigned int response)
 { }
 
+static inline void audit_fd_path(int fd)
+{ }
+
 static inline void audit_ptrace(struct task_struct *t)
 { }
 #define audit_n_rules 0
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index d762e0b8160e..82dad69213a2 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -74,6 +74,8 @@
 #include <linux/string.h>
 #include <linux/uaccess.h>
 #include <linux/fsnotify_backend.h>
+#include <linux/file.h>
+#include <linux/dcache.h>
 #include <uapi/linux/limits.h>
 
 #include "audit.h"
@@ -2422,6 +2424,40 @@ void __audit_fanotify(unsigned int response)
 		AUDIT_FANOTIFY,	"resp=%u", response);
 }
 
+void __audit_fd_path(int fd)
+{
+	struct audit_buffer *ab;
+	struct file *file;
+	char *buf, *path;
+
+	if (!audit_enabled)
+		return;
+
+	file = fget_raw(fd);
+	if (!file)
+		return;
+
+	buf = kmalloc(PATH_MAX, GFP_KERNEL);
+	if (!buf)
+		return;
+
+	path_get(&file->f_path);
+	path = d_absolute_path(&file->f_path, buf, PATH_MAX);
+	path_put(&file->f_path);
+	fput(file);
+	if (!path || IS_ERR(path))
+		goto free_buf;
+
+	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_FD_PATH);
+	if (unlikely(!ab))
+		goto free_buf;
+	audit_log_format(ab, "fd=%i path=", fd);
+	audit_log_untrustedstring(ab, path);
+	audit_log_end(ab);
+free_buf:
+	kfree(buf);
+}
+
 static void audit_log_task(struct audit_buffer *ab)
 {
 	kuid_t auid, uid;
-- 
2.17.1

Re: [RFC PATCH ghak9 2/3] audit: Add a function to log the path of an fd

[Richard Guy Briggs]

On 2018-07-12 13:36, Ondrej Mosnacek wrote:
> The function logs an FD_PATH record that is associated with the current
> syscall. The record associates the given file descriptor with the
> current path of the file under it (if it is possible to retrieve such
> path). The reader of the log can then logically connect this information
> to the syscall arguments from the SYSCALL record (based on the syscall
> type).
> 
> Record format:
> type=FD_PATH msg=audit(...): fd=<file descriptor> path=<file path>
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  include/linux/audit.h | 10 ++++++++++
>  kernel/auditsc.c      | 36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 9334fbef7bae..95d338bb603a 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -356,6 +356,7 @@ extern void __audit_log_capset(const struct cred *new, const struct cred *old);
>  extern void __audit_mmap_fd(int fd, int flags);
>  extern void __audit_log_kern_module(char *name);
>  extern void __audit_fanotify(unsigned int response);
> +extern void __audit_fd_path(int fd);
>  
>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>  {
> @@ -458,6 +459,12 @@ static inline void audit_fanotify(unsigned int response)
>  		__audit_fanotify(response);
>  }
>  
> +static inline void audit_fd_path(int fd)
> +{
> +	if (fd >= 0 && !audit_dummy_context())

Isn't an fd of 0 valid?

> +		__audit_fd_path(fd);
> +}
> +
>  extern int audit_n_rules;
>  extern int audit_signals;
>  #else /* CONFIG_AUDITSYSCALL */
> @@ -584,6 +591,9 @@ static inline void audit_log_kern_module(char *name)
>  static inline void audit_fanotify(unsigned int response)
>  { }
>  
> +static inline void audit_fd_path(int fd)
> +{ }
> +
>  static inline void audit_ptrace(struct task_struct *t)
>  { }
>  #define audit_n_rules 0
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index d762e0b8160e..82dad69213a2 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -74,6 +74,8 @@
>  #include <linux/string.h>
>  #include <linux/uaccess.h>
>  #include <linux/fsnotify_backend.h>
> +#include <linux/file.h>
> +#include <linux/dcache.h>
>  #include <uapi/linux/limits.h>
>  
>  #include "audit.h"
> @@ -2422,6 +2424,40 @@ void __audit_fanotify(unsigned int response)
>  		AUDIT_FANOTIFY,	"resp=%u", response);
>  }
>  
> +void __audit_fd_path(int fd)
> +{
> +	struct audit_buffer *ab;
> +	struct file *file;
> +	char *buf, *path;
> +
> +	if (!audit_enabled)
> +		return;
> +
> +	file = fget_raw(fd);
> +	if (!file)
> +		return;
> +
> +	buf = kmalloc(PATH_MAX, GFP_KERNEL);
> +	if (!buf)

I think we need an fput(file) here.

> +		return;
> +
> +	path_get(&file->f_path);
> +	path = d_absolute_path(&file->f_path, buf, PATH_MAX);
> +	path_put(&file->f_path);
> +	fput(file);
> +	if (!path || IS_ERR(path))
> +		goto free_buf;
> +
> +	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_FD_PATH);
> +	if (unlikely(!ab))
> +		goto free_buf;
> +	audit_log_format(ab, "fd=%i path=", fd);
> +	audit_log_untrustedstring(ab, path);
> +	audit_log_end(ab);
> +free_buf:
> +	kfree(buf);
> +}
> +
>  static void audit_log_task(struct audit_buffer *ab)
>  {
>  	kuid_t auid, uid;

- 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 ghak9 2/3] audit: Add a function to log the path of an fd

[Ondrej Mosnacek]

On Fri, Jul 13, 2018 at 5:17 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-07-12 13:36, Ondrej Mosnacek wrote:
> > The function logs an FD_PATH record that is associated with the current
> > syscall. The record associates the given file descriptor with the
> > current path of the file under it (if it is possible to retrieve such
> > path). The reader of the log can then logically connect this information
> > to the syscall arguments from the SYSCALL record (based on the syscall
> > type).
> >
> > Record format:
> > type=FD_PATH msg=audit(...): fd=<file descriptor> path=<file path>
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  include/linux/audit.h | 10 ++++++++++
> >  kernel/auditsc.c      | 36 ++++++++++++++++++++++++++++++++++++
> >  2 files changed, 46 insertions(+)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 9334fbef7bae..95d338bb603a 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -356,6 +356,7 @@ extern void __audit_log_capset(const struct cred *new, const struct cred *old);
> >  extern void __audit_mmap_fd(int fd, int flags);
> >  extern void __audit_log_kern_module(char *name);
> >  extern void __audit_fanotify(unsigned int response);
> > +extern void __audit_fd_path(int fd);
> >
> >  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> >  {
> > @@ -458,6 +459,12 @@ static inline void audit_fanotify(unsigned int response)
> >               __audit_fanotify(response);
> >  }
> >
> > +static inline void audit_fd_path(int fd)
> > +{
> > +     if (fd >= 0 && !audit_dummy_context())
>
> Isn't an fd of 0 valid?

It is treated as valid by the above condition (it only rejects
negative values), so I'm not sure if you mean "valid" or "invalid"...
I suppose an fd of 0 is unlikely to be used as dirfd in openat(2) et
al., but in general it is a valid fd and I don't think we should
explicitly exclude it here. The corresponding syscalls' input checks
will already filter out values that are invalid for them.

>
> > +             __audit_fd_path(fd);
> > +}
> > +
> >  extern int audit_n_rules;
> >  extern int audit_signals;
> >  #else /* CONFIG_AUDITSYSCALL */
> > @@ -584,6 +591,9 @@ static inline void audit_log_kern_module(char *name)
> >  static inline void audit_fanotify(unsigned int response)
> >  { }
> >
> > +static inline void audit_fd_path(int fd)
> > +{ }
> > +
> >  static inline void audit_ptrace(struct task_struct *t)
> >  { }
> >  #define audit_n_rules 0
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index d762e0b8160e..82dad69213a2 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -74,6 +74,8 @@
> >  #include <linux/string.h>
> >  #include <linux/uaccess.h>
> >  #include <linux/fsnotify_backend.h>
> > +#include <linux/file.h>
> > +#include <linux/dcache.h>
> >  #include <uapi/linux/limits.h>
> >
> >  #include "audit.h"
> > @@ -2422,6 +2424,40 @@ void __audit_fanotify(unsigned int response)
> >               AUDIT_FANOTIFY, "resp=%u", response);
> >  }
> >
> > +void __audit_fd_path(int fd)
> > +{
> > +     struct audit_buffer *ab;
> > +     struct file *file;
> > +     char *buf, *path;
> > +
> > +     if (!audit_enabled)
> > +             return;
> > +
> > +     file = fget_raw(fd);
> > +     if (!file)
> > +             return;
> > +
> > +     buf = kmalloc(PATH_MAX, GFP_KERNEL);
> > +     if (!buf)
>
> I think we need an fput(file) here.

Indeed we do, will fix in next revision.

>
> > +             return;
> > +
> > +     path_get(&file->f_path);
> > +     path = d_absolute_path(&file->f_path, buf, PATH_MAX);
> > +     path_put(&file->f_path);
> > +     fput(file);
> > +     if (!path || IS_ERR(path))
> > +             goto free_buf;
> > +
> > +     ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_FD_PATH);
> > +     if (unlikely(!ab))
> > +             goto free_buf;
> > +     audit_log_format(ab, "fd=%i path=", fd);
> > +     audit_log_untrustedstring(ab, path);
> > +     audit_log_end(ab);
> > +free_buf:
> > +     kfree(buf);
> > +}
> > +
> >  static void audit_log_task(struct audit_buffer *ab)
> >  {
> >       kuid_t auid, uid;
>
> - 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

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [RFC PATCH ghak9 2/3] audit: Add a function to log the path of an fd

[Richard Guy Briggs]

On 2018-07-16 10:29, Ondrej Mosnacek wrote:
> On Fri, Jul 13, 2018 at 5:17 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-07-12 13:36, Ondrej Mosnacek wrote:
> > > The function logs an FD_PATH record that is associated with the current
> > > syscall. The record associates the given file descriptor with the
> > > current path of the file under it (if it is possible to retrieve such
> > > path). The reader of the log can then logically connect this information
> > > to the syscall arguments from the SYSCALL record (based on the syscall
> > > type).
> > >
> > > Record format:
> > > type=FD_PATH msg=audit(...): fd=<file descriptor> path=<file path>
> > >
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > ---
> > >  include/linux/audit.h | 10 ++++++++++
> > >  kernel/auditsc.c      | 36 ++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 46 insertions(+)
> > >
> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index 9334fbef7bae..95d338bb603a 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -356,6 +356,7 @@ extern void __audit_log_capset(const struct cred *new, const struct cred *old);
> > >  extern void __audit_mmap_fd(int fd, int flags);
> > >  extern void __audit_log_kern_module(char *name);
> > >  extern void __audit_fanotify(unsigned int response);
> > > +extern void __audit_fd_path(int fd);
> > >
> > >  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> > >  {
> > > @@ -458,6 +459,12 @@ static inline void audit_fanotify(unsigned int response)
> > >               __audit_fanotify(response);
> > >  }
> > >
> > > +static inline void audit_fd_path(int fd)
> > > +{
> > > +     if (fd >= 0 && !audit_dummy_context())
> >
> > Isn't an fd of 0 valid?
> 
> It is treated as valid by the above condition (it only rejects
> negative values), so I'm not sure if you mean "valid" or "invalid"...
> I suppose an fd of 0 is unlikely to be used as dirfd in openat(2) et
> al., but in general it is a valid fd and I don't think we should
> explicitly exclude it here. The corresponding syscalls' input checks
> will already filter out values that are invalid for them.

Sorry, that must have been a brain fart on my part.  I must have seen
">" when I first reviewed it, then later saw ">="...  so that should be
fine.

> > > +             __audit_fd_path(fd);
> > > +}
> > > +
> > >  extern int audit_n_rules;
> > >  extern int audit_signals;
> > >  #else /* CONFIG_AUDITSYSCALL */
> > > @@ -584,6 +591,9 @@ static inline void audit_log_kern_module(char *name)
> > >  static inline void audit_fanotify(unsigned int response)
> > >  { }
> > >
> > > +static inline void audit_fd_path(int fd)
> > > +{ }
> > > +
> > >  static inline void audit_ptrace(struct task_struct *t)
> > >  { }
> > >  #define audit_n_rules 0
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index d762e0b8160e..82dad69213a2 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -74,6 +74,8 @@
> > >  #include <linux/string.h>
> > >  #include <linux/uaccess.h>
> > >  #include <linux/fsnotify_backend.h>
> > > +#include <linux/file.h>
> > > +#include <linux/dcache.h>
> > >  #include <uapi/linux/limits.h>
> > >
> > >  #include "audit.h"
> > > @@ -2422,6 +2424,40 @@ void __audit_fanotify(unsigned int response)
> > >               AUDIT_FANOTIFY, "resp=%u", response);
> > >  }
> > >
> > > +void __audit_fd_path(int fd)
> > > +{
> > > +     struct audit_buffer *ab;
> > > +     struct file *file;
> > > +     char *buf, *path;
> > > +
> > > +     if (!audit_enabled)
> > > +             return;
> > > +
> > > +     file = fget_raw(fd);
> > > +     if (!file)
> > > +             return;
> > > +
> > > +     buf = kmalloc(PATH_MAX, GFP_KERNEL);
> > > +     if (!buf)
> >
> > I think we need an fput(file) here.
> 
> Indeed we do, will fix in next revision.
> 
> >
> > > +             return;
> > > +
> > > +     path_get(&file->f_path);
> > > +     path = d_absolute_path(&file->f_path, buf, PATH_MAX);
> > > +     path_put(&file->f_path);
> > > +     fput(file);
> > > +     if (!path || IS_ERR(path))
> > > +             goto free_buf;
> > > +
> > > +     ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_FD_PATH);
> > > +     if (unlikely(!ab))
> > > +             goto free_buf;
> > > +     audit_log_format(ab, "fd=%i path=", fd);
> > > +     audit_log_untrustedstring(ab, path);
> > > +     audit_log_end(ab);
> > > +free_buf:
> > > +     kfree(buf);
> > > +}
> > > +
> > >  static void audit_log_task(struct audit_buffer *ab)
> > >  {
> > >       kuid_t auid, uid;
> >
> > - 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
> 
> -- 
> Ondrej Mosnacek <omosnace at redhat dot com>
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.

- 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 ghak9 2/3] audit: Add a function to log the path of an fd

[Steve Grubb]

On Thursday, July 12, 2018 7:36:32 AM EDT Ondrej Mosnacek wrote:
> The function logs an FD_PATH record that is associated with the current
> syscall. The record associates the given file descriptor with the
> current path of the file under it (if it is possible to retrieve such
> path). The reader of the log can then logically connect this information
> to the syscall arguments from the SYSCALL record (based on the syscall
> type).
> 
> Record format:
> type=FD_PATH msg=audit(...): fd=<file descriptor> path=<file path>

Event looks OK to me. However, do you check for AT_FDCWD? If so, should we 
skip generating this record?

-Steve

> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  include/linux/audit.h | 10 ++++++++++
>  kernel/auditsc.c      | 36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 9334fbef7bae..95d338bb603a 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -356,6 +356,7 @@ extern void __audit_log_capset(const struct cred *new,
> const struct cred *old); extern void __audit_mmap_fd(int fd, int flags);
>  extern void __audit_log_kern_module(char *name);
>  extern void __audit_fanotify(unsigned int response);
> +extern void __audit_fd_path(int fd);
> 
>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>  {
> @@ -458,6 +459,12 @@ static inline void audit_fanotify(unsigned int
> response) __audit_fanotify(response);
>  }
> 
> +static inline void audit_fd_path(int fd)
> +{
> +	if (fd >= 0 && !audit_dummy_context())
> +		__audit_fd_path(fd);
> +}
> +
>  extern int audit_n_rules;
>  extern int audit_signals;
>  #else /* CONFIG_AUDITSYSCALL */
> @@ -584,6 +591,9 @@ static inline void audit_log_kern_module(char *name)
>  static inline void audit_fanotify(unsigned int response)
>  { }
> 
> +static inline void audit_fd_path(int fd)
> +{ }
> +
>  static inline void audit_ptrace(struct task_struct *t)
>  { }
>  #define audit_n_rules 0
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index d762e0b8160e..82dad69213a2 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -74,6 +74,8 @@
>  #include <linux/string.h>
>  #include <linux/uaccess.h>
>  #include <linux/fsnotify_backend.h>
> +#include <linux/file.h>
> +#include <linux/dcache.h>
>  #include <uapi/linux/limits.h>
> 
>  #include "audit.h"
> @@ -2422,6 +2424,40 @@ void __audit_fanotify(unsigned int response)
>  		AUDIT_FANOTIFY,	"resp=%u", response);
>  }
> 
> +void __audit_fd_path(int fd)
> +{
> +	struct audit_buffer *ab;
> +	struct file *file;
> +	char *buf, *path;
> +
> +	if (!audit_enabled)
> +		return;
> +
> +	file = fget_raw(fd);
> +	if (!file)
> +		return;
> +
> +	buf = kmalloc(PATH_MAX, GFP_KERNEL);
> +	if (!buf)
> +		return;
> +
> +	path_get(&file->f_path);
> +	path = d_absolute_path(&file->f_path, buf, PATH_MAX);
> +	path_put(&file->f_path);
> +	fput(file);
> +	if (!path || IS_ERR(path))
> +		goto free_buf;
> +
> +	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_FD_PATH);
> +	if (unlikely(!ab))
> +		goto free_buf;
> +	audit_log_format(ab, "fd=%i path=", fd);
> +	audit_log_untrustedstring(ab, path);
> +	audit_log_end(ab);
> +free_buf:
> +	kfree(buf);
> +}
> +
>  static void audit_log_task(struct audit_buffer *ab)
>  {
>  	kuid_t auid, uid;

Re: [RFC PATCH ghak9 2/3] audit: Add a function to log the path of an fd

[Ondrej Mosnacek]

On Sat, Jul 14, 2018 at 6:26 PM Steve Grubb <sgrubb@redhat.com> wrote:
> On Thursday, July 12, 2018 7:36:32 AM EDT Ondrej Mosnacek wrote:
> > The function logs an FD_PATH record that is associated with the current
> > syscall. The record associates the given file descriptor with the
> > current path of the file under it (if it is possible to retrieve such
> > path). The reader of the log can then logically connect this information
> > to the syscall arguments from the SYSCALL record (based on the syscall
> > type).
> >
> > Record format:
> > type=FD_PATH msg=audit(...): fd=<file descriptor> path=<file path>
>
> Event looks OK to me. However, do you check for AT_FDCWD? If so, should we
> skip generating this record?

Yes, AT_FDCWD is defined as -100 and I explicitly reject all negative
file descriptors (I'll add a comment clarifying that).

>
> -Steve
>
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  include/linux/audit.h | 10 ++++++++++
> >  kernel/auditsc.c      | 36 ++++++++++++++++++++++++++++++++++++
> >  2 files changed, 46 insertions(+)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 9334fbef7bae..95d338bb603a 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -356,6 +356,7 @@ extern void __audit_log_capset(const struct cred *new,
> > const struct cred *old); extern void __audit_mmap_fd(int fd, int flags);
> >  extern void __audit_log_kern_module(char *name);
> >  extern void __audit_fanotify(unsigned int response);
> > +extern void __audit_fd_path(int fd);
> >
> >  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> >  {
> > @@ -458,6 +459,12 @@ static inline void audit_fanotify(unsigned int
> > response) __audit_fanotify(response);
> >  }
> >
> > +static inline void audit_fd_path(int fd)
> > +{
> > +     if (fd >= 0 && !audit_dummy_context())
> > +             __audit_fd_path(fd);
> > +}
> > +
> >  extern int audit_n_rules;
> >  extern int audit_signals;
> >  #else /* CONFIG_AUDITSYSCALL */
> > @@ -584,6 +591,9 @@ static inline void audit_log_kern_module(char *name)
> >  static inline void audit_fanotify(unsigned int response)
> >  { }
> >
> > +static inline void audit_fd_path(int fd)
> > +{ }
> > +
> >  static inline void audit_ptrace(struct task_struct *t)
> >  { }
> >  #define audit_n_rules 0
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index d762e0b8160e..82dad69213a2 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -74,6 +74,8 @@
> >  #include <linux/string.h>
> >  #include <linux/uaccess.h>
> >  #include <linux/fsnotify_backend.h>
> > +#include <linux/file.h>
> > +#include <linux/dcache.h>
> >  #include <uapi/linux/limits.h>
> >
> >  #include "audit.h"
> > @@ -2422,6 +2424,40 @@ void __audit_fanotify(unsigned int response)
> >               AUDIT_FANOTIFY, "resp=%u", response);
> >  }
> >
> > +void __audit_fd_path(int fd)
> > +{
> > +     struct audit_buffer *ab;
> > +     struct file *file;
> > +     char *buf, *path;
> > +
> > +     if (!audit_enabled)
> > +             return;
> > +
> > +     file = fget_raw(fd);
> > +     if (!file)
> > +             return;
> > +
> > +     buf = kmalloc(PATH_MAX, GFP_KERNEL);
> > +     if (!buf)
> > +             return;
> > +
> > +     path_get(&file->f_path);
> > +     path = d_absolute_path(&file->f_path, buf, PATH_MAX);
> > +     path_put(&file->f_path);
> > +     fput(file);
> > +     if (!path || IS_ERR(path))
> > +             goto free_buf;
> > +
> > +     ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_FD_PATH);
> > +     if (unlikely(!ab))
> > +             goto free_buf;
> > +     audit_log_format(ab, "fd=%i path=", fd);
> > +     audit_log_untrustedstring(ab, path);
> > +     audit_log_end(ab);
> > +free_buf:
> > +     kfree(buf);
> > +}
> > +
> >  static void audit_log_task(struct audit_buffer *ab)
> >  {
> >       kuid_t auid, uid;
>
>
>
>


-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

[RFC PATCH ghak9 3/3] [WIP] fs: Add audit_fd_path() calls to syscall handlers

[Ondrej Mosnacek]

So far only add one to openat(2) for testing purposes.

Example records:
type=PROCTITLE msg=audit(07/04/18 15:28:41.808:76) : proctitle=/bin/bash ./openat-audit-test.sh
type=PATH msg=audit(07/04/18 15:28:41.808:76) : item=0 name=b inode=2154 dev=00:1a mode=dir,755 ouid=root ogid=root rdev=00:00 obj=system_u:object_r:tmpfs_t:s0 nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0
type=CWD msg=audit(07/04/18 15:28:41.808:76) : cwd=/root/Dokumenty/Kernel
type=SYSCALL msg=audit(07/04/18 15:28:41.808:76) : arch=x86_64 syscall=openat success=yes exit=6 a0=0x5 a1=0x55bad2616348 a2=O_RDONLY|O_NOCTTY|O_NONBLOCK|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC a3=0x0 items=1 ppid=594 pid=630 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=(none) ses=1 comm=find exe=/usr/bin/find subj=system_u:system_r:kernel_t:s0 key=(null)
type=FD_PATH msg=audit(07/04/18 15:28:41.808:76) : fd=5 path=/tmp/a
----
type=PROCTITLE msg=audit(07/04/18 15:28:41.808:77) : proctitle=/bin/bash ./openat-audit-test.sh
type=PATH msg=audit(07/04/18 15:28:41.808:77) : item=0 name=c inode=2155 dev=00:1a mode=dir,755 ouid=root ogid=root rdev=00:00 obj=system_u:object_r:tmpfs_t:s0 nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0
type=CWD msg=audit(07/04/18 15:28:41.808:77) : cwd=/root/Dokumenty/Kernel
type=SYSCALL msg=audit(07/04/18 15:28:41.808:77) : arch=x86_64 syscall=openat success=yes exit=6 a0=0x7 a1=0x55bad260e328 a2=O_RDONLY|O_NOCTTY|O_NONBLOCK|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC a3=0x0 items=1 ppid=594 pid=630 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=(none) ses=1 comm=find exe=/usr/bin/find subj=system_u:system_r:kernel_t:s0 key=(null)
type=FD_PATH msg=audit(07/04/18 15:28:41.808:77) : fd=7 path=/tmp/a/b
----

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 fs/open.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/open.c b/fs/open.c
index d0e955b558ad..7fa326fc025d 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1125,6 +1125,8 @@ SYSCALL_DEFINE4(openat, int, dfd, const char __user *, filename, int, flags,
 	if (force_o_largefile())
 		flags |= O_LARGEFILE;
 
+	audit_fd_path(dfd);
+
 	return do_sys_open(dfd, filename, flags, mode);
 }
 
-- 
2.17.1

Re: [RFC PATCH ghak9 3/3] [WIP] fs: Add audit_fd_path() calls to syscall handlers

[Richard Guy Briggs]

On 2018-07-12 13:36, Ondrej Mosnacek wrote:
> So far only add one to openat(2) for testing purposes.
> 
> Example records:
> type=PROCTITLE msg=audit(07/04/18 15:28:41.808:76) : proctitle=/bin/bash ./openat-audit-test.sh
> type=PATH msg=audit(07/04/18 15:28:41.808:76) : item=0 name=b inode=2154 dev=00:1a mode=dir,755 ouid=root ogid=root rdev=00:00 obj=system_u:object_r:tmpfs_t:s0 nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0
> type=CWD msg=audit(07/04/18 15:28:41.808:76) : cwd=/root/Dokumenty/Kernel
> type=SYSCALL msg=audit(07/04/18 15:28:41.808:76) : arch=x86_64 syscall=openat success=yes exit=6 a0=0x5 a1=0x55bad2616348 a2=O_RDONLY|O_NOCTTY|O_NONBLOCK|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC a3=0x0 items=1 ppid=594 pid=630 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=(none) ses=1 comm=find exe=/usr/bin/find subj=system_u:system_r:kernel_t:s0 key=(null)
> type=FD_PATH msg=audit(07/04/18 15:28:41.808:76) : fd=5 path=/tmp/a
> ----
> type=PROCTITLE msg=audit(07/04/18 15:28:41.808:77) : proctitle=/bin/bash ./openat-audit-test.sh
> type=PATH msg=audit(07/04/18 15:28:41.808:77) : item=0 name=c inode=2155 dev=00:1a mode=dir,755 ouid=root ogid=root rdev=00:00 obj=system_u:object_r:tmpfs_t:s0 nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0
> type=CWD msg=audit(07/04/18 15:28:41.808:77) : cwd=/root/Dokumenty/Kernel
> type=SYSCALL msg=audit(07/04/18 15:28:41.808:77) : arch=x86_64 syscall=openat success=yes exit=6 a0=0x7 a1=0x55bad260e328 a2=O_RDONLY|O_NOCTTY|O_NONBLOCK|O_DIRECTORY|O_NOFOLLOW|O_CLOEXEC a3=0x0 items=1 ppid=594 pid=630 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=(none) ses=1 comm=find exe=/usr/bin/find subj=system_u:system_r:kernel_t:s0 key=(null)
> type=FD_PATH msg=audit(07/04/18 15:28:41.808:77) : fd=7 path=/tmp/a/b
> ----
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

This patchset looks encouraging.

> ---
>  fs/open.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/open.c b/fs/open.c
> index d0e955b558ad..7fa326fc025d 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -1125,6 +1125,8 @@ SYSCALL_DEFINE4(openat, int, dfd, const char __user *, filename, int, flags,
>  	if (force_o_largefile())
>  		flags |= O_LARGEFILE;
>  
> +	audit_fd_path(dfd);
> +
>  	return do_sys_open(dfd, filename, flags, mode);
>  }
>  

- 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 ghak9 0/3] audit: Record the path of FDs passed to *at(2) syscalls

[Paul Moore]

On Thu, Jul 12, 2018 at 7:36 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> This patchset is a prototype implementation of the feature requested in GHAK issue #9 [1]. I decided for a simple auxiliary record with just 2 fields (fd and path) that is emitted whenever we want to record the full path for a file descriptor passed to a syscall (e.g. the dirfd argument of openat(2)). I choose this approach because for some syscalls there is more than one file descriptor we might be interested in (a good example is the renameat(2) syscall).
>
> The motivation for this feature (as I understand it) is to avoid the need to reconstruct the paths corresponding to the file descriptors passed to syscalls, as this might be difficult and time consuming or even impossible in case not all of the right sycalls are being logged. Note that it is always possible to disable these records by simply adding an exclude filter rule matching all records of type FD_PATH.
>
> At this moment I only implement logging for a single syscall (openat(2)) to keep it simple. In the final version I plan to add support for other similar syscalls ()mkdirat, mknodeat, fchownat, ...).
>
> Please let me know if the general approach and the proposed record format make sense to you so I can improve/complete the solution.
>
> Thanks,
> Ondrej
>
> [1] https://github.com/linux-audit/audit-kernel/issues/9

While I recognize that the GH issue did raise the idea of possibly
creating a new record type, looking at these patches I'm not sure a
new record type is justified, I think reusing the existing PATH record
type would be more beneficial.  I recognize that this proposed FD_PATH
record also contains the file descriptor number, but that information
should also be contained in the associated SYSCALL record and arguably
the fd number is only useful if you are logging the SYSCALL
information.

Can you explain what advantage the FS_PATH record type has over
reusing the existing PATH record?  I know you mention multiple
fd/paths as in the case of renameat(2), but we already have to deal
with this in the non-at rename(2) case.

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak9 0/3] audit: Record the path of FDs passed to *at(2) syscalls

[Ondrej Mosnacek]

On Wed, Jul 18, 2018 at 10:41 PM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Jul 12, 2018 at 7:36 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > This patchset is a prototype implementation of the feature requested in GHAK issue #9 [1]. I decided for a simple auxiliary record with just 2 fields (fd and path) that is emitted whenever we want to record the full path for a file descriptor passed to a syscall (e.g. the dirfd argument of openat(2)). I choose this approach because for some syscalls there is more than one file descriptor we might be interested in (a good example is the renameat(2) syscall).
> >
> > The motivation for this feature (as I understand it) is to avoid the need to reconstruct the paths corresponding to the file descriptors passed to syscalls, as this might be difficult and time consuming or even impossible in case not all of the right sycalls are being logged. Note that it is always possible to disable these records by simply adding an exclude filter rule matching all records of type FD_PATH.
> >
> > At this moment I only implement logging for a single syscall (openat(2)) to keep it simple. In the final version I plan to add support for other similar syscalls ()mkdirat, mknodeat, fchownat, ...).
> >
> > Please let me know if the general approach and the proposed record format make sense to you so I can improve/complete the solution.
> >
> > Thanks,
> > Ondrej
> >
> > [1] https://github.com/linux-audit/audit-kernel/issues/9
>
> While I recognize that the GH issue did raise the idea of possibly
> creating a new record type, looking at these patches I'm not sure a
> new record type is justified, I think reusing the existing PATH record
> type would be more beneficial.  I recognize that this proposed FD_PATH
> record also contains the file descriptor number, but that information
> should also be contained in the associated SYSCALL record and arguably
> the fd number is only useful if you are logging the SYSCALL
> information.

To be honest, I'm not sure what is the exact semantic of the PATH
record... Is it simply "log information about files that the syscall
somehow touched"? Or "log information about any string syscall
argument that represents a file path"? In the PATH record samples I
have seen, the name=... field sometimes contains just the last segment
of the path, other times it contains the full path (huh?). When we log
the full path in PATH, then I guess the FD_PATH record would be
(almost) redundant, but it seems that whenever openat(2) is called
with dirfd != AT_FDCWD, PATH name=... contains just the relative path
supplied to the syscall (thus FD_PATH actually provides the missing
directory path).

So... assuming we would want to provide the missing information in the
existing PATH record, how should it look like? Should the name=...
field simply always contain the full path? Should there be another
PATH record for the directory? If so, how do we indicate the
association between the two PATH records?

> Can you explain what advantage the FS_PATH record type has over
> reusing the existing PATH record?  I know you mention multiple
> fd/paths as in the case of renameat(2), but we already have to deal
> with this in the non-at rename(2) case.

As I said above, I don't fully understand the PATH record so I can't
compare them right now. Multiple fd arguments was meant as more of a
justification for the presence of the fd=... field in the new FD_PATH
record.

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [RFC PATCH ghak9 0/3] audit: Record the path of FDs passed to *at(2) syscalls

[Paul Moore]

On Fri, Jul 20, 2018 at 6:12 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Wed, Jul 18, 2018 at 10:41 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Thu, Jul 12, 2018 at 7:36 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > This patchset is a prototype implementation of the feature requested in GHAK issue #9 [1]. I decided for a simple auxiliary record with just 2 fields (fd and path) that is emitted whenever we want to record the full path for a file descriptor passed to a syscall (e.g. the dirfd argument of openat(2)). I choose this approach because for some syscalls there is more than one file descriptor we might be interested in (a good example is the renameat(2) syscall).
> > >
> > > The motivation for this feature (as I understand it) is to avoid the need to reconstruct the paths corresponding to the file descriptors passed to syscalls, as this might be difficult and time consuming or even impossible in case not all of the right sycalls are being logged. Note that it is always possible to disable these records by simply adding an exclude filter rule matching all records of type FD_PATH.
> > >
> > > At this moment I only implement logging for a single syscall (openat(2)) to keep it simple. In the final version I plan to add support for other similar syscalls ()mkdirat, mknodeat, fchownat, ...).
> > >
> > > Please let me know if the general approach and the proposed record format make sense to you so I can improve/complete the solution.
> > >
> > > Thanks,
> > > Ondrej
> > >
> > > [1] https://github.com/linux-audit/audit-kernel/issues/9
> >
> > While I recognize that the GH issue did raise the idea of possibly
> > creating a new record type, looking at these patches I'm not sure a
> > new record type is justified, I think reusing the existing PATH record
> > type would be more beneficial.  I recognize that this proposed FD_PATH
> > record also contains the file descriptor number, but that information
> > should also be contained in the associated SYSCALL record and arguably
> > the fd number is only useful if you are logging the SYSCALL
> > information.
>
> To be honest, I'm not sure what is the exact semantic of the PATH
> record... Is it simply "log information about files that the syscall
> somehow touched"? Or "log information about any string syscall
> argument that represents a file path"?

The PATH record exists to record path name information about files
related to the audit event, depending on the circumstances, both
relative and absolute paths could be expected.  Look at the functions,
and callers, that add to the audit_context->names_list to get a better
understanding of where the path name information is collected.  I
doubt you will be surprised at this, but fair warning, it's a bit ugly
in places.

> In the PATH record samples I
> have seen, the name=... field sometimes contains just the last segment
> of the path, other times it contains the full path (huh?).

Depending on the syscall, the syscall arguments, and the "nametype" of
the PATH record you could see either a full or partial path.  In the
cases of a partial path, there *should* be additional PATH records
which can help recreate a full path to the file for that particular
event instance.

> When we log
> the full path in PATH, then I guess the FD_PATH record would be
> (almost) redundant, but it seems that whenever openat(2) is called
> with dirfd != AT_FDCWD, PATH name=... contains just the relative path
> supplied to the syscall (thus FD_PATH actually provides the missing
> directory path).
>
> So... assuming we would want to provide the missing information in the
> existing PATH record, how should it look like? Should the name=...
> field simply always contain the full path? Should there be another
> PATH record for the directory?

The only real hard requirement is that there be enough to information
to determine the file full path(s) for any given audit event using
one, or a combination, of PATH records associated with the event.

> If so, how do we indicate the association between the two PATH records?

Look at how the "nametype" field is used.

> > Can you explain what advantage the FS_PATH record type has over
> > reusing the existing PATH record?  I know you mention multiple
> > fd/paths as in the case of renameat(2), but we already have to deal
> > with this in the non-at rename(2) case.
>
> As I said above, I don't fully understand the PATH record so I can't
> compare them right now. Multiple fd arguments was meant as more of a
> justification for the presence of the fd=... field in the new FD_PATH
> record.

I suggest taking some time to better understand how the current file
path auditing works and then revisiting this patchset.

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak9 0/3] audit: Record the path of FDs passed to *at(2) syscalls

[Ondrej Mosnacek]

On Mon, Jul 23, 2018 at 10:49 PM Paul Moore <paul@paul-moore.com> wrote:
> On Fri, Jul 20, 2018 at 6:12 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Wed, Jul 18, 2018 at 10:41 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Thu, Jul 12, 2018 at 7:36 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > This patchset is a prototype implementation of the feature requested in GHAK issue #9 [1]. I decided for a simple auxiliary record with just 2 fields (fd and path) that is emitted whenever we want to record the full path for a file descriptor passed to a syscall (e.g. the dirfd argument of openat(2)). I choose this approach because for some syscalls there is more than one file descriptor we might be interested in (a good example is the renameat(2) syscall).
> > > >
> > > > The motivation for this feature (as I understand it) is to avoid the need to reconstruct the paths corresponding to the file descriptors passed to syscalls, as this might be difficult and time consuming or even impossible in case not all of the right sycalls are being logged. Note that it is always possible to disable these records by simply adding an exclude filter rule matching all records of type FD_PATH.
> > > >
> > > > At this moment I only implement logging for a single syscall (openat(2)) to keep it simple. In the final version I plan to add support for other similar syscalls ()mkdirat, mknodeat, fchownat, ...).
> > > >
> > > > Please let me know if the general approach and the proposed record format make sense to you so I can improve/complete the solution.
> > > >
> > > > Thanks,
> > > > Ondrej
> > > >
> > > > [1] https://github.com/linux-audit/audit-kernel/issues/9
> > >
> > > While I recognize that the GH issue did raise the idea of possibly
> > > creating a new record type, looking at these patches I'm not sure a
> > > new record type is justified, I think reusing the existing PATH record
> > > type would be more beneficial.  I recognize that this proposed FD_PATH
> > > record also contains the file descriptor number, but that information
> > > should also be contained in the associated SYSCALL record and arguably
> > > the fd number is only useful if you are logging the SYSCALL
> > > information.
> >
> > To be honest, I'm not sure what is the exact semantic of the PATH
> > record... Is it simply "log information about files that the syscall
> > somehow touched"? Or "log information about any string syscall
> > argument that represents a file path"?
>
> The PATH record exists to record path name information about files
> related to the audit event, depending on the circumstances, both
> relative and absolute paths could be expected.  Look at the functions,
> and callers, that add to the audit_context->names_list to get a better
> understanding of where the path name information is collected.  I
> doubt you will be surprised at this, but fair warning, it's a bit ugly
> in places.
>
> > In the PATH record samples I
> > have seen, the name=... field sometimes contains just the last segment
> > of the path, other times it contains the full path (huh?).
>
> Depending on the syscall, the syscall arguments, and the "nametype" of
> the PATH record you could see either a full or partial path.  In the
> cases of a partial path, there *should* be additional PATH records
> which can help recreate a full path to the file for that particular
> event instance.
>
> > When we log
> > the full path in PATH, then I guess the FD_PATH record would be
> > (almost) redundant, but it seems that whenever openat(2) is called
> > with dirfd != AT_FDCWD, PATH name=... contains just the relative path
> > supplied to the syscall (thus FD_PATH actually provides the missing
> > directory path).
> >
> > So... assuming we would want to provide the missing information in the
> > existing PATH record, how should it look like? Should the name=...
> > field simply always contain the full path? Should there be another
> > PATH record for the directory?
>
> The only real hard requirement is that there be enough to information
> to determine the file full path(s) for any given audit event using
> one, or a combination, of PATH records associated with the event.
>
> > If so, how do we indicate the association between the two PATH records?
>
> Look at how the "nametype" field is used.

OK, so I just wrote a small script to see what PATH records would be
generated for a renameat(2) syscall with non-AT_FDCWD fd arguments,
and it seems the current implementation is not lacking information,
but actually buggy.

strace output:
openat(AT_FDCWD, "/tmp/tmp.CXtBRafonK/a", O_RDONLY|O_PATH|O_DIRECTORY) = 3
openat(AT_FDCWD, "/tmp/tmp.CXtBRafonK/b", O_RDONLY|O_PATH|O_DIRECTORY) = 4
renameat(3, "f", 4, "g")                = 0
close(3)                                = 0
close(4)                                = 0

Corresponding audit records for renameat():
type=SYSCALL msg=audit(1532439957.660:5): arch=c000003e syscall=264
success=yes exit=0 a0=3 a1=7ffcc364de2a a2=4 a3=7ffcc364de42 items=4
ppid=594 pid=635 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0
sgid=0 fsgid=0 tty=(none) ses=1 comm="trigger-renamea"
exe="/tmp/tmp.GEfuEtn1II/trigger-renameat"
subj=system_u:system_r:kernel_t:s0 key=(null)
type=CWD msg=audit(1532439957.660:5): cwd="/root/Dokumenty/Kernel"
type=PATH msg=audit(1532439957.660:5): item=0
name="/root/Dokumenty/Kernel" inode=2155 dev=00:1a mode=040755 ouid=0
ogid=0 rdev=00:00 obj=system_u:object_r:tmpfs_t:s0 nametype=PARENT
cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
type=PATH msg=audit(1532439957.660:5): item=1
name="/root/Dokumenty/Kernel" inode=2156 dev=00:1a mode=040755 ouid=0
ogid=0 rdev=00:00 obj=system_u:object_r:tmpfs_t:s0 nametype=PARENT
cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
type=PATH msg=audit(1532439957.660:5): item=2 name="f" inode=2157
dev=00:1a mode=0100644 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:tmpfs_t:s0 nametype=DELETE
cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
type=PATH msg=audit(1532439957.660:5): item=3 name="g" inode=2157
dev=00:1a mode=0100644 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:tmpfs_t:s0 nametype=CREATE
cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
type=PROCTITLE msg=audit(1532439957.660:5):
proctitle=2F746D702F746D702E4745667545746E3149492F747269676765722D72656E616D656174002F746D702F746D702E4745667545746E3149492F610066002F746D702F746D702E4745667545746E3149492F620067

The PARENT paths are incorrectly reporting the CWD path instead of the
path of the source/destination directories, even though the inode
numbers seem to be correct.

Beyond that, there is really no information in the records that would
allow reconstructing which PARENT path belongs to which CREATE/DELETE
path... (Intuitively you can guess that src will come before dst, but
that is not very reliable.) I think a "parent inode" field in the PATH
records could fix this, but maybe there is a better solution...

I'll see if/how I can fix these issues (especially the first one) and
then I'll get back to the original issue. renameat() and maybe a few
other syscalls should be OK after the fix, but at least openat() will
need some further work (right now it only emits just one PATH record
with only relative path).

> > > Can you explain what advantage the FS_PATH record type has over
> > > reusing the existing PATH record?  I know you mention multiple
> > > fd/paths as in the case of renameat(2), but we already have to deal
> > > with this in the non-at rename(2) case.
> >
> > As I said above, I don't fully understand the PATH record so I can't
> > compare them right now. Multiple fd arguments was meant as more of a
> > justification for the presence of the fd=... field in the new FD_PATH
> > record.
>
> I suggest taking some time to better understand how the current file
> path auditing works and then revisiting this patchset.
>
> --
> paul moore
> www.paul-moore.com--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [RFC PATCH ghak9 0/3] audit: Record the path of FDs passed to *at(2) syscalls

[Paul Moore]

On Tue, Jul 24, 2018 at 10:12 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> OK, so I just wrote a small script to see what PATH records would be
> generated for a renameat(2) syscall with non-AT_FDCWD fd arguments,
> and it seems the current implementation is not lacking information,
> but actually buggy.
>
> strace output:
> openat(AT_FDCWD, "/tmp/tmp.CXtBRafonK/a", O_RDONLY|O_PATH|O_DIRECTORY) = 3
> openat(AT_FDCWD, "/tmp/tmp.CXtBRafonK/b", O_RDONLY|O_PATH|O_DIRECTORY) = 4
> renameat(3, "f", 4, "g")                = 0
> close(3)                                = 0
> close(4)                                = 0
>
> Corresponding audit records for renameat():
> type=SYSCALL msg=audit(1532439957.660:5): arch=c000003e syscall=264
> success=yes exit=0 a0=3 a1=7ffcc364de2a a2=4 a3=7ffcc364de42 items=4
> ppid=594 pid=635 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0
> sgid=0 fsgid=0 tty=(none) ses=1 comm="trigger-renamea"
> exe="/tmp/tmp.GEfuEtn1II/trigger-renameat"
> subj=system_u:system_r:kernel_t:s0 key=(null)
> type=CWD msg=audit(1532439957.660:5): cwd="/root/Dokumenty/Kernel"

...

> type=PATH msg=audit(1532439957.660:5): item=0
> name="/root/Dokumenty/Kernel" inode=2155 dev=00:1a mode=040755 ouid=0
> ogid=0 rdev=00:00 obj=system_u:object_r:tmpfs_t:s0 nametype=PARENT
> cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> type=PATH msg=audit(1532439957.660:5): item=1
> name="/root/Dokumenty/Kernel" inode=2156 dev=00:1a mode=040755 ouid=0
> ogid=0 rdev=00:00 obj=system_u:object_r:tmpfs_t:s0 nametype=PARENT
> cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> type=PATH msg=audit(1532439957.660:5): item=2 name="f" inode=2157
> dev=00:1a mode=0100644 ouid=0 ogid=0 rdev=00:00
> obj=system_u:object_r:tmpfs_t:s0 nametype=DELETE
> cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> type=PATH msg=audit(1532439957.660:5): item=3 name="g" inode=2157
> dev=00:1a mode=0100644 ouid=0 ogid=0 rdev=00:00
> obj=system_u:object_r:tmpfs_t:s0 nametype=CREATE
> cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0

...

> The PARENT paths are incorrectly reporting the CWD path instead of the
> path of the source/destination directories, even though the inode
> numbers seem to be correct.

Yes, that's odd, and not desirable.

> Beyond that, there is really no information in the records that would
> allow reconstructing which PARENT path belongs to which CREATE/DELETE
> path... (Intuitively you can guess that src will come before dst, but
> that is not very reliable.) I think a "parent inode" field in the PATH
> records could fix this, but maybe there is a better solution...

I have my suspicions, but I would be curious to hear from Steve how
the reconstruction is typically handled.

> I'll see if/how I can fix these issues (especially the first one) and
> then I'll get back to the original issue. renameat() and maybe a few
> other syscalls should be OK after the fix, but at least openat() will
> need some further work (right now it only emits just one PATH record
> with only relative path).

Yes, let's fix this first and go from there.

Thanks for looking into this.

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak9 0/3] audit: Record the path of FDs passed to *at(2) syscalls

[Steve Grubb]

On Tuesday, July 24, 2018 6:15:54 PM EDT Paul Moore wrote:
> On Tue, Jul 24, 2018 at 10:12 AM Ondrej Mosnacek <omosnace@redhat.com> 
wrote:
> > OK, so I just wrote a small script to see what PATH records would be
> > generated for a renameat(2) syscall with non-AT_FDCWD fd arguments,
> > and it seems the current implementation is not lacking information,
> > but actually buggy.
> > 
> > strace output:
> > openat(AT_FDCWD, "/tmp/tmp.CXtBRafonK/a", O_RDONLY|O_PATH|O_DIRECTORY) =
> > 3
> > openat(AT_FDCWD, "/tmp/tmp.CXtBRafonK/b", O_RDONLY|O_PATH|O_DIRECTORY) =
> > 4
> > renameat(3, "f", 4, "g")                = 0
> > close(3)                                = 0
> > close(4)                                = 0
> > 
> > Corresponding audit records for renameat():
> > type=SYSCALL msg=audit(1532439957.660:5): arch=c000003e syscall=264
> > success=yes exit=0 a0=3 a1=7ffcc364de2a a2=4 a3=7ffcc364de42 items=4
> > ppid=594 pid=635 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0
> > sgid=0 fsgid=0 tty=(none) ses=1 comm="trigger-renamea"
> > exe="/tmp/tmp.GEfuEtn1II/trigger-renameat"
> > subj=system_u:system_r:kernel_t:s0 key=(null)
> > type=CWD msg=audit(1532439957.660:5): cwd="/root/Dokumenty/Kernel"
> 
> ...
> 
> > type=PATH msg=audit(1532439957.660:5): item=0
> > name="/root/Dokumenty/Kernel" inode=2155 dev=00:1a mode=040755 ouid=0
> > ogid=0 rdev=00:00 obj=system_u:object_r:tmpfs_t:s0 nametype=PARENT
> > cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> > type=PATH msg=audit(1532439957.660:5): item=1
> > name="/root/Dokumenty/Kernel" inode=2156 dev=00:1a mode=040755 ouid=0
> > ogid=0 rdev=00:00 obj=system_u:object_r:tmpfs_t:s0 nametype=PARENT
> > cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> > type=PATH msg=audit(1532439957.660:5): item=2 name="f" inode=2157
> > dev=00:1a mode=0100644 ouid=0 ogid=0 rdev=00:00
> > obj=system_u:object_r:tmpfs_t:s0 nametype=DELETE
> > cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> > type=PATH msg=audit(1532439957.660:5): item=3 name="g" inode=2157
> > dev=00:1a mode=0100644 ouid=0 ogid=0 rdev=00:00
> > obj=system_u:object_r:tmpfs_t:s0 nametype=CREATE
> > cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> 
> ...
> 
> > The PARENT paths are incorrectly reporting the CWD path instead of the
> > path of the source/destination directories, even though the inode
> > numbers seem to be correct.
> 
> Yes, that's odd, and not desirable.
> 
> > Beyond that, there is really no information in the records that would
> > allow reconstructing which PARENT path belongs to which CREATE/DELETE
> > path... (Intuitively you can guess that src will come before dst, but
> > that is not very reliable.) I think a "parent inode" field in the PATH
> > records could fix this, but maybe there is a better solution...
> 
> I have my suspicions, but I would be curious to hear from Steve how
> the reconstruction is typically handled.

For any *at function when the dirfd is not AT_FDCWD, it goes badly. If its a 
old style syscall without the dirfd, then if the first character is '/' use 
that. Otherwise concatonate cwd and path and pass it to realpath to sort out.

-Steve

> > I'll see if/how I can fix these issues (especially the first one) and
> > then I'll get back to the original issue. renameat() and maybe a few
> > other syscalls should be OK after the fix, but at least openat() will
> > need some further work (right now it only emits just one PATH record
> > with only relative path).
> 
> Yes, let's fix this first and go from there.
> 
> Thanks for looking into this.

Re: [RFC PATCH ghak9 0/3] audit: Record the path of FDs passed to *at(2) syscalls

[Ondrej Mosnacek]

On Wed, Jul 25, 2018 at 3:11 AM Steve Grubb <sgrubb@redhat.com> wrote:
> On Tuesday, July 24, 2018 6:15:54 PM EDT Paul Moore wrote:
> > On Tue, Jul 24, 2018 at 10:12 AM Ondrej Mosnacek <omosnace@redhat.com>
> > > Beyond that, there is really no information in the records that would
> > > allow reconstructing which PARENT path belongs to which CREATE/DELETE
> > > path... (Intuitively you can guess that src will come before dst, but
> > > that is not very reliable.) I think a "parent inode" field in the PATH
> > > records could fix this, but maybe there is a better solution...
> >
> > I have my suspicions, but I would be curious to hear from Steve how
> > the reconstruction is typically handled.
>
> For any *at function when the dirfd is not AT_FDCWD, it goes badly. If its a
> old style syscall without the dirfd, then if the first character is '/' use
> that. Otherwise concatonate cwd and path and pass it to realpath to sort out.

In that case it seems the best fix for openat() et al. would be to
somehow always force outputting the full path when dirfd != AT_FDCWD.
Hopefully that won't require too much hacking around...

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [RFC PATCH ghak9 0/3] audit: Record the path of FDs passed to *at(2) syscalls

[Steve Grubb]

On Wednesday, July 25, 2018 3:44:07 AM EDT Ondrej Mosnacek wrote:
> On Wed, Jul 25, 2018 at 3:11 AM Steve Grubb <sgrubb@redhat.com> wrote:
> > On Tuesday, July 24, 2018 6:15:54 PM EDT Paul Moore wrote:
> > > On Tue, Jul 24, 2018 at 10:12 AM Ondrej Mosnacek <omosnace@redhat.com>
> > > 
> > > > Beyond that, there is really no information in the records that would
> > > > allow reconstructing which PARENT path belongs to which CREATE/DELETE
> > > > path... (Intuitively you can guess that src will come before dst, but
> > > > that is not very reliable.) I think a "parent inode" field in the
> > > > PATH
> > > > records could fix this, but maybe there is a better solution...
> > > 
> > > I have my suspicions, but I would be curious to hear from Steve how
> > > the reconstruction is typically handled.
> > 
> > For any *at function when the dirfd is not AT_FDCWD, it goes badly. If
> > its a old style syscall without the dirfd, then if the first character
> > is '/' use that. Otherwise concatonate cwd and path and pass it to
> > realpath to sort out.
>
> In that case it seems the best fix for openat() et al. would be to
> somehow always force outputting the full path when dirfd != AT_FDCWD.
> Hopefully that won't require too much hacking around...

What is asked for is the full path that dirfd was opened with. I can take 
care of everything else.

-Steve

Re: [RFC PATCH ghak9 0/3] audit: Record the path of FDs passed to *at(2) syscalls

[Ondrej Mosnacek]

On Wed, Jul 25, 2018 at 2:48 PM Steve Grubb <sgrubb@redhat.com> wrote:
> On Wednesday, July 25, 2018 3:44:07 AM EDT Ondrej Mosnacek wrote:
> > On Wed, Jul 25, 2018 at 3:11 AM Steve Grubb <sgrubb@redhat.com> wrote:
> > > On Tuesday, July 24, 2018 6:15:54 PM EDT Paul Moore wrote:
> > > > On Tue, Jul 24, 2018 at 10:12 AM Ondrej Mosnacek <omosnace@redhat.com>
> > > >
> > > > > Beyond that, there is really no information in the records that would
> > > > > allow reconstructing which PARENT path belongs to which CREATE/DELETE
> > > > > path... (Intuitively you can guess that src will come before dst, but
> > > > > that is not very reliable.) I think a "parent inode" field in the
> > > > > PATH
> > > > > records could fix this, but maybe there is a better solution...
> > > >
> > > > I have my suspicions, but I would be curious to hear from Steve how
> > > > the reconstruction is typically handled.
> > >
> > > For any *at function when the dirfd is not AT_FDCWD, it goes badly. If
> > > its a old style syscall without the dirfd, then if the first character
> > > is '/' use that. Otherwise concatonate cwd and path and pass it to
> > > realpath to sort out.
> >
> > In that case it seems the best fix for openat() et al. would be to
> > somehow always force outputting the full path when dirfd != AT_FDCWD.
> > Hopefully that won't require too much hacking around...
>
> What is asked for is the full path that dirfd was opened with. I can take
> care of everything else.

But where/how should that path be logged? In case of renameat(), for
example, we have 6 (!) path components:
<src_dir>/<src_parent>/<src_child> and <dst_dir>/<dst_parent>/<dst_child>

(I am assuming the child paths always represent just the last path
component based on the observed inodes of the parent/child records.)

Current record format can distinguish between PARENT and child
(DELETE/CREATE), but there is no nametype for the dirfd path. That's
why I am leaning towards just logging the full "<*_dir>/<*_parent>"
path in the PARENT record. Or do you prefer that we add a new nametype
for the dirfd path?

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [RFC PATCH ghak9 0/3] audit: Record the path of FDs passed to *at(2) syscalls

[Steve Grubb]

On Wednesday, July 25, 2018 9:02:50 AM EDT Ondrej Mosnacek wrote:
> On Wed, Jul 25, 2018 at 2:48 PM Steve Grubb <sgrubb@redhat.com> wrote:
> > On Wednesday, July 25, 2018 3:44:07 AM EDT Ondrej Mosnacek wrote:
> > > On Wed, Jul 25, 2018 at 3:11 AM Steve Grubb <sgrubb@redhat.com> wrote:
> > > > On Tuesday, July 24, 2018 6:15:54 PM EDT Paul Moore wrote:
> > > > > On Tue, Jul 24, 2018 at 10:12 AM Ondrej Mosnacek
> > > > > <omosnace@redhat.com>
> > > > > 
> > > > > > Beyond that, there is really no information in the records that
> > > > > > would
> > > > > > allow reconstructing which PARENT path belongs to which
> > > > > > CREATE/DELETE
> > > > > > path... (Intuitively you can guess that src will come before dst,
> > > > > > but
> > > > > > that is not very reliable.) I think a "parent inode" field in the
> > > > > > PATH
> > > > > > records could fix this, but maybe there is a better solution...
> > > > > 
> > > > > I have my suspicions, but I would be curious to hear from Steve how
> > > > > the reconstruction is typically handled.
> > > > 
> > > > For any *at function when the dirfd is not AT_FDCWD, it goes badly.
> > > > If
> > > > its a old style syscall without the dirfd, then if the first
> > > > character
> > > > is '/' use that. Otherwise concatonate cwd and path and pass it to
> > > > realpath to sort out.
> > > 
> > > In that case it seems the best fix for openat() et al. would be to
> > > somehow always force outputting the full path when dirfd != AT_FDCWD.
> > > Hopefully that won't require too much hacking around...
> > 
> > What is asked for is the full path that dirfd was opened with. I can take
> > care of everything else.
> 
> But where/how should that path be logged? In case of renameat(), for
> example, we have 6 (!) path components:
> <src_dir>/<src_parent>/<src_child> and <dst_dir>/<dst_parent>/<dst_child>
> 
> (I am assuming the child paths always represent just the last path
> component based on the observed inodes of the parent/child records.)
> 
> Current record format can distinguish between PARENT and child
> (DELETE/CREATE), but there is no nametype for the dirfd path. That's
> why I am leaning towards just logging the full "<*_dir>/<*_parent>"
> path in the PARENT record. Or do you prefer that we add a new nametype
> for the dirfd path?

You could make a new nametype so that we can make sense of it. But do you 
have all of the required information for a PATH record? I thought that you 
were making a new record type since you have abbreviated information.

-Steve

Re: [RFC PATCH ghak9 0/3] audit: Record the path of FDs passed to *at(2) syscalls

[Ondrej Mosnacek]

On Wed, Jul 25, 2018 at 3:11 PM Steve Grubb <sgrubb@redhat.com> wrote:
> On Wednesday, July 25, 2018 9:02:50 AM EDT Ondrej Mosnacek wrote:
> > On Wed, Jul 25, 2018 at 2:48 PM Steve Grubb <sgrubb@redhat.com> wrote:
> > > On Wednesday, July 25, 2018 3:44:07 AM EDT Ondrej Mosnacek wrote:
> > > > On Wed, Jul 25, 2018 at 3:11 AM Steve Grubb <sgrubb@redhat.com> wrote:
> > > > > On Tuesday, July 24, 2018 6:15:54 PM EDT Paul Moore wrote:
> > > > > > On Tue, Jul 24, 2018 at 10:12 AM Ondrej Mosnacek
> > > > > > <omosnace@redhat.com>
> > > > > >
> > > > > > > Beyond that, there is really no information in the records that
> > > > > > > would
> > > > > > > allow reconstructing which PARENT path belongs to which
> > > > > > > CREATE/DELETE
> > > > > > > path... (Intuitively you can guess that src will come before dst,
> > > > > > > but
> > > > > > > that is not very reliable.) I think a "parent inode" field in the
> > > > > > > PATH
> > > > > > > records could fix this, but maybe there is a better solution...
> > > > > >
> > > > > > I have my suspicions, but I would be curious to hear from Steve how
> > > > > > the reconstruction is typically handled.
> > > > >
> > > > > For any *at function when the dirfd is not AT_FDCWD, it goes badly.
> > > > > If
> > > > > its a old style syscall without the dirfd, then if the first
> > > > > character
> > > > > is '/' use that. Otherwise concatonate cwd and path and pass it to
> > > > > realpath to sort out.
> > > >
> > > > In that case it seems the best fix for openat() et al. would be to
> > > > somehow always force outputting the full path when dirfd != AT_FDCWD.
> > > > Hopefully that won't require too much hacking around...
> > >
> > > What is asked for is the full path that dirfd was opened with. I can take
> > > care of everything else.
> >
> > But where/how should that path be logged? In case of renameat(), for
> > example, we have 6 (!) path components:
> > <src_dir>/<src_parent>/<src_child> and <dst_dir>/<dst_parent>/<dst_child>
> >
> > (I am assuming the child paths always represent just the last path
> > component based on the observed inodes of the parent/child records.)
> >
> > Current record format can distinguish between PARENT and child
> > (DELETE/CREATE), but there is no nametype for the dirfd path. That's
> > why I am leaning towards just logging the full "<*_dir>/<*_parent>"
> > path in the PARENT record. Or do you prefer that we add a new nametype
> > for the dirfd path?
>
> You could make a new nametype so that we can make sense of it. But do you
> have all of the required information for a PATH record? I thought that you
> were making a new record type since you have abbreviated information.

I think it should be possible to collect that information by putting
hooks in the right places of the filesystem code (and fixing the
current ones).

To be honest, the reason why I had jumped right to making a new record
type was Paul's wording in the issue description ("We may need a new
auxiliary record type since this is neither the cwd or path." - the
second part of that sentence sounded like using the PATH record
wouldn't be a reasonable choice) and the fact that the purpose of the
PATH records was not clear to me at that time. Now that I spent some
time with them, they don't look that scary anymore :)

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [RFC PATCH ghak9 0/3] audit: Record the path of FDs passed to *at(2) syscalls

[Ondrej Mosnacek]

On Thu, Jul 26, 2018 at 10:12 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> I think it should be possible to collect that information by putting
> hooks in the right places of the filesystem code (and fixing the
> current ones).

Hm, after closer look, it seems this won't be doable (at least not
easily). The PATH record always logs the original path string from
userspace (and I think we need to preserve this behavior in case
someone relies on it). In case of PARENT records, it truncates away
the last component (because it wants to log inode information also for
the parent directory). If this truncated string ends up empty (i.e.
the original string had just one component), it just smashes in the
absolute path of the CWD (which is known), because it pretends no
*at() syscalls exist and all relative paths are relative to current
CWD.

So to fix this, we need to do one of the following:
1. Add a new field to the PATH records that would specify the path of
the directory that the value of name=... is relative to. If this is
CWD, we can just use some special value
("(null)"/"(none)"/"(cwd)"/...) or omit the field completely. I prefer
this approach, because it will best solve the case of renameat(),
where different PATH records can have different base directories.
2. If adding fields is considered A Bad Thing, we could alternatively
provide this information in separate records (either PATH with special
nametype or a new record). However in such case we need to somehow
specify to which PATH records each base directory corresponds. For
PATH records this could be guessed from their order, but this is a
fragile thing (changes in filesystem code could change the order).

Let me give an example to illustrate better how it might look like:
=== renameat() with two FD arguments ===
openat(AT_FDCWD, "/tmp/tmp.SIMqhBS0eI/a", O_RDONLY|O_PATH|O_DIRECTORY) = 3
openat(AT_FDCWD, "/tmp/tmp.SIMqhBS0eI/b", O_RDONLY|O_PATH|O_DIRECTORY) = 4
renameat(3, "x/f", 4, "y/g")            = 0
close(3)                                = 0
close(4)                                = 0

type=SYSCALL msg=audit(1532504483.814:5): [...]
type=CWD msg=audit(1532504483.814:5): cwd="/root/Dokumenty/Kernel"
type=PATH msg=audit(1532504483.814:5): item=0 name="x/" inode=2156
dev=00:1a mode=040755 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:tmpfs_t:s0 nametype=PARENT
cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
basedir="/tmp/tmp.SIMqhBS0eI/a"
type=PATH msg=audit(1532504483.814:5): item=1 name="y/" inode=2158
dev=00:1a mode=040755 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:tmpfs_t:s0 nametype=PARENT
cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
basedir="/tmp/tmp.SIMqhBS0eI/b"
type=PATH msg=audit(1532504483.814:5): item=2 name="x/f" inode=2159
dev=00:1a mode=0100644 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:tmpfs_t:s0 nametype=DELETE
cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
basedir="/tmp/tmp.SIMqhBS0eI/a"
type=PATH msg=audit(1532504483.814:5): item=3 name="y/g" inode=2159
dev=00:1a mode=0100644 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:tmpfs_t:s0 nametype=CREATE
cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
basedir="/tmp/tmp.SIMqhBS0eI/b"

=== renameat() with one CWD and one FD argument ===
openat(AT_FDCWD, "/tmp/tmp.SIMqhBS0eI/a", O_RDONLY|O_PATH|O_DIRECTORY) = 3
renameat(3, "x/f", AT_FDCWD, "y/g")            = 0
close(3)                                = 0

type=SYSCALL msg=audit(1532504483.814:5): [...]
type=CWD msg=audit(1532504483.814:5): cwd="/root/Dokumenty/Kernel"
type=PATH msg=audit(1532504483.814:5): item=0 name="x/" inode=2156
dev=00:1a mode=040755 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:tmpfs_t:s0 nametype=PARENT
cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
basedir="/tmp/tmp.SIMqhBS0eI/a"
type=PATH msg=audit(1532504483.814:5): item=1 name="y/" inode=2158
dev=00:1a mode=040755 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:tmpfs_t:s0 nametype=PARENT
cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
basedir=(cwd)
type=PATH msg=audit(1532504483.814:5): item=2 name="x/f" inode=2159
dev=00:1a mode=0100644 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:tmpfs_t:s0 nametype=DELETE
cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
basedir="/tmp/tmp.SIMqhBS0eI/a"
type=PATH msg=audit(1532504483.814:5): item=3 name="y/g" inode=2159
dev=00:1a mode=0100644 ouid=0 ogid=0 rdev=00:00
obj=system_u:object_r:tmpfs_t:s0 nametype=CREATE
cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
basedir=(cwd)

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [RFC PATCH ghak9 0/3] audit: Record the path of FDs passed to *at(2) syscalls

[Paul Moore]

On Thu, Jul 26, 2018 at 5:13 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Thu, Jul 26, 2018 at 10:12 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > I think it should be possible to collect that information by putting
> > hooks in the right places of the filesystem code (and fixing the
> > current ones).
>
> Hm, after closer look, it seems this won't be doable (at least not
> easily). The PATH record always logs the original path string from
> userspace (and I think we need to preserve this behavior in case
> someone relies on it). In case of PARENT records, it truncates away
> the last component (because it wants to log inode information also for
> the parent directory). If this truncated string ends up empty (i.e.
> the original string had just one component), it just smashes in the
> absolute path of the CWD (which is known), because it pretends no
> *at() syscalls exist and all relative paths are relative to current
> CWD.
>
> So to fix this, we need to do one of the following:
> 1. Add a new field to the PATH records that would specify the path of
> the directory that the value of name=... is relative to. If this is
> CWD, we can just use some special value
> ("(null)"/"(none)"/"(cwd)"/...) or omit the field completely. I prefer
> this approach, because it will best solve the case of renameat(),
> where different PATH records can have different base directories.

It is worth calling extra attention to the fact that we would now be
effectively recording two paths in a single record.  I'm not sure how
much we care about that, but it does increase the chances we blow past
the end of the netlink buffer; although it is worth noting that a
single PATH_MAX entry would do that today.

Also, would this new field remain empty for non-*at syscalls?

> 2. If adding fields is considered A Bad Thing, we could alternatively
> provide this information in separate records (either PATH with special
> nametype or a new record). However in such case we need to somehow
> specify to which PATH records each base directory corresponds. For
> PATH records this could be guessed from their order, but this is a
> fragile thing (changes in filesystem code could change the order).

While this may be a bit more difficult it seems like this is more in
keeping with the current methodology, and would keep the overall audit
logs smaller.  In the case of the *at syscalls I presume you would
create PARENT records for the base directory, omitting it if AT_FDCWD
was used?

I imagine this would also help the traditional rename() syscall?

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak9 0/3] audit: Record the path of FDs passed to *at(2) syscalls

[Ondrej Mosnacek]

On Fri, Aug 3, 2018 at 1:58 AM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Jul 26, 2018 at 5:13 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Thu, Jul 26, 2018 at 10:12 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > I think it should be possible to collect that information by putting
> > > hooks in the right places of the filesystem code (and fixing the
> > > current ones).
> >
> > Hm, after closer look, it seems this won't be doable (at least not
> > easily). The PATH record always logs the original path string from
> > userspace (and I think we need to preserve this behavior in case
> > someone relies on it). In case of PARENT records, it truncates away
> > the last component (because it wants to log inode information also for
> > the parent directory). If this truncated string ends up empty (i.e.
> > the original string had just one component), it just smashes in the
> > absolute path of the CWD (which is known), because it pretends no
> > *at() syscalls exist and all relative paths are relative to current
> > CWD.
> >
> > So to fix this, we need to do one of the following:
> > 1. Add a new field to the PATH records that would specify the path of
> > the directory that the value of name=... is relative to. If this is
> > CWD, we can just use some special value
> > ("(null)"/"(none)"/"(cwd)"/...) or omit the field completely. I prefer
> > this approach, because it will best solve the case of renameat(),
> > where different PATH records can have different base directories.
>
> It is worth calling extra attention to the fact that we would now be
> effectively recording two paths in a single record.  I'm not sure how
> much we care about that, but it does increase the chances we blow past
> the end of the netlink buffer; although it is worth noting that a
> single PATH_MAX entry would do that today.

Yes, but note that the two "paths" are actually just parts of a single
(full) path. The situation will be no different from when the user
would just specify a full path. But your comment just made me realize
that I will need to make sure that the new field is empty also when
dirfd != AT_FDCWD, but the path supplied to the syscall is absolute
(because then it would be just useless information).

> Also, would this new field remain empty for non-*at syscalls?

Yes, the emptiness (or some special value depending on the chosen
implementation) would indicate that dirfd is CWD (which can be looked
up in the CWD record).

> > 2. If adding fields is considered A Bad Thing, we could alternatively
> > provide this information in separate records (either PATH with special
> > nametype or a new record). However in such case we need to somehow
> > specify to which PATH records each base directory corresponds. For
> > PATH records this could be guessed from their order, but this is a
> > fragile thing (changes in filesystem code could change the order).
>
> While this may be a bit more difficult it seems like this is more in
> keeping with the current methodology, and would keep the overall audit
> logs smaller.  In the case of the *at syscalls I presume you would
> create PARENT records for the base directory, omitting it if AT_FDCWD
> was used?

Right now, the parent/child records are only emitted when a file is
moved, created, or removed (actually I'm not sure right now about the
latter two, but definitely for the first case). The PARENT record
always contains the (relative or absolute) path as given to the
syscall minus the last element and the corresponding CREATE/DELETE
record contains only the last element. I don't know what is the goal
of this splitting (probably to report details (inode, ...) also about
the containing directory or maybe to deduplicate PATH records if src
and dst directories are the same?), but I'm not sure it is a good idea
to overload the semantics of nametype=PARENT for yet another purpose
(one of the PARENTs would be parent for the other PARENT...). I'm
almost certain this would also confuse the current userspace code
(especially considering that for rename() the kernel generates both
PARENT records before the child records...).

That said, it seems to me this would actually make the logs *bigger*,
because you would also log the usual PATH record details (inode,
fcaps, ...) for the base directory (dirfd).

> I imagine this would also help the traditional rename() syscall?

I'm not sure how, given the facts above.

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [RFC PATCH ghak9 0/3] audit: Record the path of FDs passed to *at(2) syscalls

[Paul Moore]

On Thu, Jul 26, 2018 at 4:12 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Wed, Jul 25, 2018 at 3:11 PM Steve Grubb <sgrubb@redhat.com> wrote:
> > On Wednesday, July 25, 2018 9:02:50 AM EDT Ondrej Mosnacek wrote:
> > > On Wed, Jul 25, 2018 at 2:48 PM Steve Grubb <sgrubb@redhat.com> wrote:
> > > > On Wednesday, July 25, 2018 3:44:07 AM EDT Ondrej Mosnacek wrote:
> > > > > On Wed, Jul 25, 2018 at 3:11 AM Steve Grubb <sgrubb@redhat.com> wrote:
> > > > > > On Tuesday, July 24, 2018 6:15:54 PM EDT Paul Moore wrote:
> > > > > > > On Tue, Jul 24, 2018 at 10:12 AM Ondrej Mosnacek
> > > > > > > <omosnace@redhat.com>
> > > > > > >
> > > > > > > > Beyond that, there is really no information in the records that
> > > > > > > > would
> > > > > > > > allow reconstructing which PARENT path belongs to which
> > > > > > > > CREATE/DELETE
> > > > > > > > path... (Intuitively you can guess that src will come before dst,
> > > > > > > > but
> > > > > > > > that is not very reliable.) I think a "parent inode" field in the
> > > > > > > > PATH
> > > > > > > > records could fix this, but maybe there is a better solution...
> > > > > > >
> > > > > > > I have my suspicions, but I would be curious to hear from Steve how
> > > > > > > the reconstruction is typically handled.
> > > > > >
> > > > > > For any *at function when the dirfd is not AT_FDCWD, it goes badly.
> > > > > > If
> > > > > > its a old style syscall without the dirfd, then if the first
> > > > > > character
> > > > > > is '/' use that. Otherwise concatonate cwd and path and pass it to
> > > > > > realpath to sort out.
> > > > >
> > > > > In that case it seems the best fix for openat() et al. would be to
> > > > > somehow always force outputting the full path when dirfd != AT_FDCWD.
> > > > > Hopefully that won't require too much hacking around...
> > > >
> > > > What is asked for is the full path that dirfd was opened with. I can take
> > > > care of everything else.
> > >
> > > But where/how should that path be logged? In case of renameat(), for
> > > example, we have 6 (!) path components:
> > > <src_dir>/<src_parent>/<src_child> and <dst_dir>/<dst_parent>/<dst_child>
> > >
> > > (I am assuming the child paths always represent just the last path
> > > component based on the observed inodes of the parent/child records.)
> > >
> > > Current record format can distinguish between PARENT and child
> > > (DELETE/CREATE), but there is no nametype for the dirfd path. That's
> > > why I am leaning towards just logging the full "<*_dir>/<*_parent>"
> > > path in the PARENT record. Or do you prefer that we add a new nametype
> > > for the dirfd path?
> >
> > You could make a new nametype so that we can make sense of it. But do you
> > have all of the required information for a PATH record? I thought that you
> > were making a new record type since you have abbreviated information.
>
> I think it should be possible to collect that information by putting
> hooks in the right places of the filesystem code (and fixing the
> current ones).
>
> To be honest, the reason why I had jumped right to making a new record
> type was Paul's wording in the issue description ("We may need a new
> auxiliary record type since this is neither the cwd or path." ...

For future reference, when I say "may" I do really mean it; it *may*
be a good idea, but it *may* also be a garbage idea ;)

-- 
paul moore
www.paul-moore.com