[PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Ondrej Mosnacek]

When a relative path has just a single component and we want to emit a
nametype=PARENT record, the current implementation just reports the full
CWD path (which is alrady available in the audit context).

This is wrong for three reasons:
1. Wasting log space for redundant data (CWD path is already in the CWD
   record).
2. Inconsistency with other PATH records (if a relative PARENT directory
   path contains at least one component, only the verbatim relative path
   is logged).
3. In some syscalls (e.g. openat(2)) the relative path may not even be
   relative to the CWD, but to another directory specified as a file
   descriptor. In that case the logged path is simply plain wrong.

This patch modifies this behavior to simply report "." in the
aforementioned case, which is equivalent to an "empty" directory path
and can be concatenated with the actual base directory path (CWD or
dirfd from openat(2)-like syscall) once support for its logging is added
later. In the meantime, defaulting to CWD as base directory on relative
paths (as already done by the userspace tools) will be enough to achieve
results equivalent to the current behavior.

See: https://github.com/linux-audit/audit-kernel/issues/95

Fixes: 9c937dcc7102 ("[PATCH] log more info for directory entry change events")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 kernel/audit.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index 2a8058764aa6..4f18bd48eb4b 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -2127,28 +2127,27 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
 
 	audit_log_format(ab, "item=%d", record_num);
 
+	audit_log_format(ab, " name=");
 	if (path)
-		audit_log_d_path(ab, " name=", path);
+		audit_log_d_path(ab, NULL, path);
 	else if (n->name) {
 		switch (n->name_len) {
 		case AUDIT_NAME_FULL:
 			/* log the full path */
-			audit_log_format(ab, " name=");
 			audit_log_untrustedstring(ab, n->name->name);
 			break;
 		case 0:
 			/* name was specified as a relative path and the
 			 * directory component is the cwd */
-			audit_log_d_path(ab, " name=", &context->pwd);
+			audit_log_untrustedstring(ab, ".");
 			break;
 		default:
 			/* log the name's directory component */
-			audit_log_format(ab, " name=");
 			audit_log_n_untrustedstring(ab, n->name->name,
 						    n->name_len);
 		}
 	} else
-		audit_log_format(ab, " name=(null)");
+		audit_log_format(ab, "(null)");
 
 	if (n->ino != AUDIT_INO_UNSET)
 		audit_log_format(ab, " inode=%lu"
-- 
2.17.1

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Richard Guy Briggs]

On 2018-08-02 13:44, Ondrej Mosnacek wrote:
> When a relative path has just a single component and we want to emit a
> nametype=PARENT record, the current implementation just reports the full
> CWD path (which is alrady available in the audit context).
> 
> This is wrong for three reasons:
> 1. Wasting log space for redundant data (CWD path is already in the CWD
>    record).
> 2. Inconsistency with other PATH records (if a relative PARENT directory
>    path contains at least one component, only the verbatim relative path
>    is logged).
> 3. In some syscalls (e.g. openat(2)) the relative path may not even be
>    relative to the CWD, but to another directory specified as a file
>    descriptor. In that case the logged path is simply plain wrong.
> 
> This patch modifies this behavior to simply report "." in the
> aforementioned case, which is equivalent to an "empty" directory path
> and can be concatenated with the actual base directory path (CWD or
> dirfd from openat(2)-like syscall) once support for its logging is added
> later. In the meantime, defaulting to CWD as base directory on relative
> paths (as already done by the userspace tools) will be enough to achieve
> results equivalent to the current behavior.
> 
> See: https://github.com/linux-audit/audit-kernel/issues/95

Untested.  This looks like a reasonable rationale and solution to me.

> Fixes: 9c937dcc7102 ("[PATCH] log more info for directory entry change events")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

> ---
>  kernel/audit.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 2a8058764aa6..4f18bd48eb4b 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2127,28 +2127,27 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
>  
>  	audit_log_format(ab, "item=%d", record_num);
>  
> +	audit_log_format(ab, " name=");
>  	if (path)
> -		audit_log_d_path(ab, " name=", path);
> +		audit_log_d_path(ab, NULL, path);
>  	else if (n->name) {
>  		switch (n->name_len) {
>  		case AUDIT_NAME_FULL:
>  			/* log the full path */
> -			audit_log_format(ab, " name=");
>  			audit_log_untrustedstring(ab, n->name->name);
>  			break;
>  		case 0:
>  			/* name was specified as a relative path and the
>  			 * directory component is the cwd */
> -			audit_log_d_path(ab, " name=", &context->pwd);
> +			audit_log_untrustedstring(ab, ".");
>  			break;
>  		default:
>  			/* log the name's directory component */
> -			audit_log_format(ab, " name=");
>  			audit_log_n_untrustedstring(ab, n->name->name,
>  						    n->name_len);
>  		}
>  	} else
> -		audit_log_format(ab, " name=(null)");
> +		audit_log_format(ab, "(null)");
>  
>  	if (n->ino != AUDIT_INO_UNSET)
>  		audit_log_format(ab, " inode=%lu"
> -- 
> 2.17.1
> 

- 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: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Paul Moore]

On Thu, Aug 2, 2018 at 7:45 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> When a relative path has just a single component and we want to emit a
> nametype=PARENT record, the current implementation just reports the full
> CWD path (which is alrady available in the audit context).
>
> This is wrong for three reasons:
> 1. Wasting log space for redundant data (CWD path is already in the CWD
>    record).
> 2. Inconsistency with other PATH records (if a relative PARENT directory
>    path contains at least one component, only the verbatim relative path
>    is logged).
> 3. In some syscalls (e.g. openat(2)) the relative path may not even be
>    relative to the CWD, but to another directory specified as a file
>    descriptor. In that case the logged path is simply plain wrong.
>
> This patch modifies this behavior to simply report "." in the
> aforementioned case, which is equivalent to an "empty" directory path
> and can be concatenated with the actual base directory path (CWD or
> dirfd from openat(2)-like syscall) once support for its logging is added
> later. In the meantime, defaulting to CWD as base directory on relative
> paths (as already done by the userspace tools) will be enough to achieve
> results equivalent to the current behavior.
>
> See: https://github.com/linux-audit/audit-kernel/issues/95
>
> Fixes: 9c937dcc7102 ("[PATCH] log more info for directory entry change events")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  kernel/audit.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 2a8058764aa6..4f18bd48eb4b 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2127,28 +2127,27 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
>
>         audit_log_format(ab, "item=%d", record_num);
>
> +       audit_log_format(ab, " name=");
>         if (path)
> -               audit_log_d_path(ab, " name=", path);
> +               audit_log_d_path(ab, NULL, path);
>         else if (n->name) {
>                 switch (n->name_len) {
>                 case AUDIT_NAME_FULL:
>                         /* log the full path */
> -                       audit_log_format(ab, " name=");
>                         audit_log_untrustedstring(ab, n->name->name);
>                         break;
>                 case 0:
>                         /* name was specified as a relative path and the
>                          * directory component is the cwd */
> -                       audit_log_d_path(ab, " name=", &context->pwd);
> +                       audit_log_untrustedstring(ab, ".");

This isn't a comprehensive review, I only gave this a quick look, but
considering you are only logging "." above we can safely use
audit_log_string() and safe a few cycles.

Honestly, looking at the rest of this function, why are we using
audit_log_format() in the case where we are simply writing a string
literal?  While I haven't tested it, simple code inspection would seem
to indicate that audit_log_string() should be much quicker than
audit_log_format()?  I realize this isn't strictly a problem from this
patch, but we probably shouldn't be propagating this mistake any
further.

--
paul moore
www.paul-moore.com

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Paul Moore]

On Thu, Aug 2, 2018 at 7:45 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> When a relative path has just a single component and we want to emit a
> nametype=PARENT record, the current implementation just reports the full
> CWD path (which is alrady available in the audit context).
>
> This is wrong for three reasons:
> 1. Wasting log space for redundant data (CWD path is already in the CWD
>    record).
> 2. Inconsistency with other PATH records (if a relative PARENT directory
>    path contains at least one component, only the verbatim relative path
>    is logged).
> 3. In some syscalls (e.g. openat(2)) the relative path may not even be
>    relative to the CWD, but to another directory specified as a file
>    descriptor. In that case the logged path is simply plain wrong.
>
> This patch modifies this behavior to simply report "." in the
> aforementioned case, which is equivalent to an "empty" directory path
> and can be concatenated with the actual base directory path (CWD or
> dirfd from openat(2)-like syscall) once support for its logging is added
> later. In the meantime, defaulting to CWD as base directory on relative
> paths (as already done by the userspace tools) will be enough to achieve
> results equivalent to the current behavior.

I have to ask the obvious question, if we already have the necessary
parent path in the CWD record, why do we need a nametype=parent PATH
record anyway?  Can we safely remove it or will that cause problems
for Steve's userspace tools?

> See: https://github.com/linux-audit/audit-kernel/issues/95
>
> Fixes: 9c937dcc7102 ("[PATCH] log more info for directory entry change events")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  kernel/audit.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 2a8058764aa6..4f18bd48eb4b 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2127,28 +2127,27 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
>
>         audit_log_format(ab, "item=%d", record_num);
>
> +       audit_log_format(ab, " name=");
>         if (path)
> -               audit_log_d_path(ab, " name=", path);
> +               audit_log_d_path(ab, NULL, path);
>         else if (n->name) {
>                 switch (n->name_len) {
>                 case AUDIT_NAME_FULL:
>                         /* log the full path */
> -                       audit_log_format(ab, " name=");
>                         audit_log_untrustedstring(ab, n->name->name);
>                         break;
>                 case 0:
>                         /* name was specified as a relative path and the
>                          * directory component is the cwd */
> -                       audit_log_d_path(ab, " name=", &context->pwd);
> +                       audit_log_untrustedstring(ab, ".");
>                         break;
>                 default:
>                         /* log the name's directory component */
> -                       audit_log_format(ab, " name=");
>                         audit_log_n_untrustedstring(ab, n->name->name,
>                                                     n->name_len);
>                 }
>         } else
> -               audit_log_format(ab, " name=(null)");
> +               audit_log_format(ab, "(null)");
>
>         if (n->ino != AUDIT_INO_UNSET)
>                 audit_log_format(ab, " inode=%lu"
> --
> 2.17.1
>


-- 
paul moore
www.paul-moore.com

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Ondrej Mosnacek]

On Fri, Aug 3, 2018 at 12:24 AM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Aug 2, 2018 at 7:45 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > When a relative path has just a single component and we want to emit a
> > nametype=PARENT record, the current implementation just reports the full
> > CWD path (which is alrady available in the audit context).
> >
> > This is wrong for three reasons:
> > 1. Wasting log space for redundant data (CWD path is already in the CWD
> >    record).
> > 2. Inconsistency with other PATH records (if a relative PARENT directory
> >    path contains at least one component, only the verbatim relative path
> >    is logged).
> > 3. In some syscalls (e.g. openat(2)) the relative path may not even be
> >    relative to the CWD, but to another directory specified as a file
> >    descriptor. In that case the logged path is simply plain wrong.
> >
> > This patch modifies this behavior to simply report "." in the
> > aforementioned case, which is equivalent to an "empty" directory path
> > and can be concatenated with the actual base directory path (CWD or
> > dirfd from openat(2)-like syscall) once support for its logging is added
> > later. In the meantime, defaulting to CWD as base directory on relative
> > paths (as already done by the userspace tools) will be enough to achieve
> > results equivalent to the current behavior.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/95
> >
> > Fixes: 9c937dcc7102 ("[PATCH] log more info for directory entry change events")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  kernel/audit.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 2a8058764aa6..4f18bd48eb4b 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -2127,28 +2127,27 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
> >
> >         audit_log_format(ab, "item=%d", record_num);
> >
> > +       audit_log_format(ab, " name=");
> >         if (path)
> > -               audit_log_d_path(ab, " name=", path);
> > +               audit_log_d_path(ab, NULL, path);
> >         else if (n->name) {
> >                 switch (n->name_len) {
> >                 case AUDIT_NAME_FULL:
> >                         /* log the full path */
> > -                       audit_log_format(ab, " name=");
> >                         audit_log_untrustedstring(ab, n->name->name);
> >                         break;
> >                 case 0:
> >                         /* name was specified as a relative path and the
> >                          * directory component is the cwd */
> > -                       audit_log_d_path(ab, " name=", &context->pwd);
> > +                       audit_log_untrustedstring(ab, ".");
>
> This isn't a comprehensive review, I only gave this a quick look, but
> considering you are only logging "." above we can safely use
> audit_log_string() and safe a few cycles.

I used audit_log_untrustedstring() to maintain the current norm that
the name= field would always contain a quoted string (either in
double-quotes or hex-escaped). I don't know if such consistency is
important for parsing in userspace (it should probably be robust
enough to handle any format), but I wanted to be on the safe side.

> Honestly, looking at the rest of this function, why are we using
> audit_log_format() in the case where we are simply writing a string
> literal?  While I haven't tested it, simple code inspection would seem
> to indicate that audit_log_string() should be much quicker than
> audit_log_format()?  I realize this isn't strictly a problem from this
> patch, but we probably shouldn't be propagating this mistake any
> further.

Good point. If I happen to be sending a v2 of the patch, I will switch
to audit_log_string() where possible. Otherwise, I'll leave it to you
to fix up when applying (it will probably clash with your patch
anyway).

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

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Ondrej Mosnacek]

On Thu, Aug 2, 2018 at 1:45 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> When a relative path has just a single component and we want to emit a
> nametype=PARENT record, the current implementation just reports the full
> CWD path (which is alrady available in the audit context).
>
> This is wrong for three reasons:
> 1. Wasting log space for redundant data (CWD path is already in the CWD
>    record).
> 2. Inconsistency with other PATH records (if a relative PARENT directory
>    path contains at least one component, only the verbatim relative path
>    is logged).
> 3. In some syscalls (e.g. openat(2)) the relative path may not even be
>    relative to the CWD, but to another directory specified as a file
>    descriptor. In that case the logged path is simply plain wrong.
>
> This patch modifies this behavior to simply report "." in the
> aforementioned case, which is equivalent to an "empty" directory path
> and can be concatenated with the actual base directory path (CWD or
> dirfd from openat(2)-like syscall) once support for its logging is added
> later. In the meantime, defaulting to CWD as base directory on relative
> paths (as already done by the userspace tools) will be enough to achieve
> results equivalent to the current behavior.

I tested this patch a little bit with the libauparse library (it has
some functions for normalizing paths extracted from the records) and
it seems to behave as intended. The userspace functions seem to take a
rather crude approach and I think they won't always properly normalize
some paths, but that is a separate issue and this patch at least
definitely doesn't make it worse.

>
> See: https://github.com/linux-audit/audit-kernel/issues/95
>
> Fixes: 9c937dcc7102 ("[PATCH] log more info for directory entry change events")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  kernel/audit.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 2a8058764aa6..4f18bd48eb4b 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2127,28 +2127,27 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
>
>         audit_log_format(ab, "item=%d", record_num);
>
> +       audit_log_format(ab, " name=");
>         if (path)
> -               audit_log_d_path(ab, " name=", path);
> +               audit_log_d_path(ab, NULL, path);
>         else if (n->name) {
>                 switch (n->name_len) {
>                 case AUDIT_NAME_FULL:
>                         /* log the full path */
> -                       audit_log_format(ab, " name=");
>                         audit_log_untrustedstring(ab, n->name->name);
>                         break;
>                 case 0:
>                         /* name was specified as a relative path and the
>                          * directory component is the cwd */
> -                       audit_log_d_path(ab, " name=", &context->pwd);
> +                       audit_log_untrustedstring(ab, ".");
>                         break;
>                 default:
>                         /* log the name's directory component */
> -                       audit_log_format(ab, " name=");
>                         audit_log_n_untrustedstring(ab, n->name->name,
>                                                     n->name_len);
>                 }
>         } else
> -               audit_log_format(ab, " name=(null)");
> +               audit_log_format(ab, "(null)");
>
>         if (n->ino != AUDIT_INO_UNSET)
>                 audit_log_format(ab, " inode=%lu"
> --
> 2.17.1
>

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

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Paul Moore]

On Fri, Aug 3, 2018 at 3:08 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Fri, Aug 3, 2018 at 12:24 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Thu, Aug 2, 2018 at 7:45 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > When a relative path has just a single component and we want to emit a
> > > nametype=PARENT record, the current implementation just reports the full
> > > CWD path (which is alrady available in the audit context).
> > >
> > > This is wrong for three reasons:
> > > 1. Wasting log space for redundant data (CWD path is already in the CWD
> > >    record).
> > > 2. Inconsistency with other PATH records (if a relative PARENT directory
> > >    path contains at least one component, only the verbatim relative path
> > >    is logged).
> > > 3. In some syscalls (e.g. openat(2)) the relative path may not even be
> > >    relative to the CWD, but to another directory specified as a file
> > >    descriptor. In that case the logged path is simply plain wrong.
> > >
> > > This patch modifies this behavior to simply report "." in the
> > > aforementioned case, which is equivalent to an "empty" directory path
> > > and can be concatenated with the actual base directory path (CWD or
> > > dirfd from openat(2)-like syscall) once support for its logging is added
> > > later. In the meantime, defaulting to CWD as base directory on relative
> > > paths (as already done by the userspace tools) will be enough to achieve
> > > results equivalent to the current behavior.
> > >
> > > See: https://github.com/linux-audit/audit-kernel/issues/95
> > >
> > > Fixes: 9c937dcc7102 ("[PATCH] log more info for directory entry change events")
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > ---
> > >  kernel/audit.c | 9 ++++-----
> > >  1 file changed, 4 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index 2a8058764aa6..4f18bd48eb4b 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -2127,28 +2127,27 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
> > >
> > >         audit_log_format(ab, "item=%d", record_num);
> > >
> > > +       audit_log_format(ab, " name=");
> > >         if (path)
> > > -               audit_log_d_path(ab, " name=", path);
> > > +               audit_log_d_path(ab, NULL, path);
> > >         else if (n->name) {
> > >                 switch (n->name_len) {
> > >                 case AUDIT_NAME_FULL:
> > >                         /* log the full path */
> > > -                       audit_log_format(ab, " name=");
> > >                         audit_log_untrustedstring(ab, n->name->name);
> > >                         break;
> > >                 case 0:
> > >                         /* name was specified as a relative path and the
> > >                          * directory component is the cwd */
> > > -                       audit_log_d_path(ab, " name=", &context->pwd);
> > > +                       audit_log_untrustedstring(ab, ".");
> >
> > This isn't a comprehensive review, I only gave this a quick look, but
> > considering you are only logging "." above we can safely use
> > audit_log_string() and safe a few cycles.
>
> I used audit_log_untrustedstring() to maintain the current norm that
> the name= field would always contain a quoted string (either in
> double-quotes or hex-escaped). I don't know if such consistency is
> important for parsing in userspace (it should probably be robust
> enough to handle any format), but I wanted to be on the safe side.

In this particular case there should be no visible difference in the
resulting record and audit_log_string() is going to have less overhead
in the kernel.

> > Honestly, looking at the rest of this function, why are we using
> > audit_log_format() in the case where we are simply writing a string
> > literal?  While I haven't tested it, simple code inspection would seem
> > to indicate that audit_log_string() should be much quicker than
> > audit_log_format()?  I realize this isn't strictly a problem from this
> > patch, but we probably shouldn't be propagating this mistake any
> > further.
>
> Good point. If I happen to be sending a v2 of the patch, I will switch
> to audit_log_string() where possible. Otherwise, I'll leave it to you
> to fix up when applying (it will probably clash with your patch
> anyway).

Please fit it yourself and resubmit.

As a general rule I shouldn't need to fix things like this when
merging.  While things like this sometimes happen, they are special
cases and are usually the result of short deadlines, missing devs,
etc.  In general I try to limit my modifications to merge fuzz and
minor code changes like whitespace fixes, silly checkpatch warnings,
etc.

-- 
paul moore
www.paul-moore.com

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Steve Grubb]

On Friday, August 24, 2018 8:59:10 AM EDT Ondrej Mosnacek wrote:
> On Thu, Aug 2, 2018 at 1:45 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > When a relative path has just a single component and we want to emit a
> > nametype=PARENT record, the current implementation just reports the full
> > CWD path (which is alrady available in the audit context).
> > 
> > This is wrong for three reasons:
> > 1. Wasting log space for redundant data (CWD path is already in the CWD
> > 
> >    record).
> > 
> > 2. Inconsistency with other PATH records (if a relative PARENT directory
> > 
> >    path contains at least one component, only the verbatim relative path
> >    is logged).
> > 
> > 3. In some syscalls (e.g. openat(2)) the relative path may not even be
> > 
> >    relative to the CWD, but to another directory specified as a file
> >    descriptor. In that case the logged path is simply plain wrong.
> > 
> > This patch modifies this behavior to simply report "." in the
> > aforementioned case, which is equivalent to an "empty" directory path
> > and can be concatenated with the actual base directory path (CWD or
> > dirfd from openat(2)-like syscall) once support for its logging is added
> > later. In the meantime, defaulting to CWD as base directory on relative
> > paths (as already done by the userspace tools) will be enough to achieve
> > results equivalent to the current behavior.
> 
> I tested this patch a little bit with the libauparse library (it has
> some functions for normalizing paths extracted from the records) and
> it seems to behave as intended. The userspace functions seem to take a
> rather crude approach and I think they won't always properly normalize
> some paths, but that is a separate issue and this patch at least
> definitely doesn't make it worse.

Be sure to test with directory names that have spaces in them as well as file 
names with spaces in them.

-Steve

> > See: https://github.com/linux-audit/audit-kernel/issues/95
> > 
> > Fixes: 9c937dcc7102 ("[PATCH] log more info for directory entry change
> > events") Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> > 
> >  kernel/audit.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 2a8058764aa6..4f18bd48eb4b 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -2127,28 +2127,27 @@ void audit_log_name(struct audit_context
> > *context, struct audit_names *n,> 
> >         audit_log_format(ab, "item=%d", record_num);
> > 
> > +       audit_log_format(ab, " name=");
> > 
> >         if (path)
> > 
> > -               audit_log_d_path(ab, " name=", path);
> > +               audit_log_d_path(ab, NULL, path);
> > 
> >         else if (n->name) {
> >         
> >                 switch (n->name_len) {
> >                 
> >                 case AUDIT_NAME_FULL:
> >                         /* log the full path */
> > 
> > -                       audit_log_format(ab, " name=");
> > 
> >                         audit_log_untrustedstring(ab, n->name->name);
> >                         break;
> >                 
> >                 case 0:
> >                         /* name was specified as a relative path and the
> >                         
> >                          * directory component is the cwd */
> > 
> > -                       audit_log_d_path(ab, " name=", &context->pwd);
> > +                       audit_log_untrustedstring(ab, ".");
> > 
> >                         break;
> >                 
> >                 default:
> >                         /* log the name's directory component */
> > 
> > -                       audit_log_format(ab, " name=");
> > 
> >                         audit_log_n_untrustedstring(ab, n->name->name,
> >                         
> >                                                     n->name_len);
> >                 
> >                 }
> >         
> >         } else
> > 
> > -               audit_log_format(ab, " name=(null)");
> > +               audit_log_format(ab, "(null)");
> > 
> >         if (n->ino != AUDIT_INO_UNSET)
> >         
> >                 audit_log_format(ab, " inode=%lu"
> > 
> > --
> > 2.17.1
> 
> --
> Ondrej Mosnacek <omosnace at redhat dot com>
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Paul Moore]

On Thu, Aug 2, 2018 at 8:03 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Thu, Aug 2, 2018 at 7:45 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > When a relative path has just a single component and we want to emit a
> > nametype=PARENT record, the current implementation just reports the full
> > CWD path (which is alrady available in the audit context).
> >
> > This is wrong for three reasons:
> > 1. Wasting log space for redundant data (CWD path is already in the CWD
> >    record).
> > 2. Inconsistency with other PATH records (if a relative PARENT directory
> >    path contains at least one component, only the verbatim relative path
> >    is logged).
> > 3. In some syscalls (e.g. openat(2)) the relative path may not even be
> >    relative to the CWD, but to another directory specified as a file
> >    descriptor. In that case the logged path is simply plain wrong.
> >
> > This patch modifies this behavior to simply report "." in the
> > aforementioned case, which is equivalent to an "empty" directory path
> > and can be concatenated with the actual base directory path (CWD or
> > dirfd from openat(2)-like syscall) once support for its logging is added
> > later. In the meantime, defaulting to CWD as base directory on relative
> > paths (as already done by the userspace tools) will be enough to achieve
> > results equivalent to the current behavior.
>
> I have to ask the obvious question, if we already have the necessary
> parent path in the CWD record, why do we need a nametype=parent PATH
> record anyway?  Can we safely remove it or will that cause problems
> for Steve's userspace tools?

As a reminder, these questions still need answers.

> > See: https://github.com/linux-audit/audit-kernel/issues/95
> >
> > Fixes: 9c937dcc7102 ("[PATCH] log more info for directory entry change events")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  kernel/audit.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 2a8058764aa6..4f18bd48eb4b 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -2127,28 +2127,27 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
> >
> >         audit_log_format(ab, "item=%d", record_num);
> >
> > +       audit_log_format(ab, " name=");
> >         if (path)
> > -               audit_log_d_path(ab, " name=", path);
> > +               audit_log_d_path(ab, NULL, path);
> >         else if (n->name) {
> >                 switch (n->name_len) {
> >                 case AUDIT_NAME_FULL:
> >                         /* log the full path */
> > -                       audit_log_format(ab, " name=");
> >                         audit_log_untrustedstring(ab, n->name->name);
> >                         break;
> >                 case 0:
> >                         /* name was specified as a relative path and the
> >                          * directory component is the cwd */
> > -                       audit_log_d_path(ab, " name=", &context->pwd);
> > +                       audit_log_untrustedstring(ab, ".");
> >                         break;
> >                 default:
> >                         /* log the name's directory component */
> > -                       audit_log_format(ab, " name=");
> >                         audit_log_n_untrustedstring(ab, n->name->name,
> >                                                     n->name_len);
> >                 }
> >         } else
> > -               audit_log_format(ab, " name=(null)");
> > +               audit_log_format(ab, "(null)");
> >
> >         if (n->ino != AUDIT_INO_UNSET)
> >                 audit_log_format(ab, " inode=%lu"
> > --
> > 2.17.1
> >
>
>
> --
> paul moore
> www.paul-moore.com



-- 
paul moore
www.paul-moore.com

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Steve Grubb]

On Friday, August 24, 2018 11:00:35 AM EDT Paul Moore wrote:
> On Thu, Aug 2, 2018 at 8:03 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Thu, Aug 2, 2018 at 7:45 AM Ondrej Mosnacek <omosnace@redhat.com> 
wrote:
> > > When a relative path has just a single component and we want to emit a
> > > nametype=PARENT record, the current implementation just reports the
> > > full CWD path (which is alrady available in the audit context).

It is supposed to report the parent directory of the object (file). Never 
mind about CWD. That tells us where the command was issued from. Sometimes 
that is important even if it is already in a PATH record. It is more forensic 
information.

> > > This is wrong for three reasons:
> > > 1. Wasting log space for redundant data (CWD path is already in the CWD
> > > record).

A CWD record is always expected for a file system operation. We are not 
missing any right now. Just don't want to lose them.

> > > 2. Inconsistency with other PATH records (if a relative PARENT
> > > directory path contains at least one component, only the verbatim
> > > relative path is logged).
> > > 3. In some syscalls (e.g. openat(2)) the relative path may not even be
> > > relative to the CWD, but to another directory specified as a file
> > > descriptor. In that case the logged path is simply plain wrong.

This can be fixed in the reporting tools. The biggest problem is when we have 
several PATH records figuring our how they are all related.

> > > This patch modifies this behavior to simply report "." in the
> > > aforementioned case, which is equivalent to an "empty" directory path
> > > and can be concatenated with the actual base directory path (CWD or
> > > dirfd from openat(2)-like syscall) once support for its logging is
> > > added later. In the meantime, defaulting to CWD as base directory on
> > > relative paths (as already done by the userspace tools) will be enough
> > > to achieve results equivalent to the current behavior.
> > 
> > I have to ask the obvious question, if we already have the necessary
> > parent path in the CWD record, why do we need a nametype=parent PATH
> > record anyway?

CWD is where the command was issued from. Sometimes it can be used as a 
PARENT PATH record. But what if name resolution fails at the parent 
directory? That record turns out to be all we get.

> > Can we safely remove it or will that cause problems for Steve's userspace
> > tools?

The PARENT records are used in figuring out what is really happening in 
certain cases.

-Steve

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Ondrej Mosnacek]

On Fri, Aug 24, 2018 at 5:14 PM Steve Grubb <sgrubb@redhat.com> wrote:
> On Friday, August 24, 2018 11:00:35 AM EDT Paul Moore wrote:
> > On Thu, Aug 2, 2018 at 8:03 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Thu, Aug 2, 2018 at 7:45 AM Ondrej Mosnacek <omosnace@redhat.com>
> wrote:
> > > > When a relative path has just a single component and we want to emit a
> > > > nametype=PARENT record, the current implementation just reports the
> > > > full CWD path (which is alrady available in the audit context).
>
> It is supposed to report the parent directory of the object (file). Never
> mind about CWD. That tells us where the command was issued from. Sometimes
> that is important even if it is already in a PATH record. It is more forensic
> information.

Yes, the problem is that if the path is just a file name (say,
"file.txt"), then the kernel assumes that the parent directory is the
CWD and puts its absolute path into the PATH record (probably because
putting in an empty path seemed weird or is not even valid). But with
*at(2) syscalls the parent directory can be some other directory
(specified by the dirfd argument of the syscall), so the assumption is
wrong. In both cases we really should report relative path to the
directory, which is always just ".". This is consistent with how other
relative paths are handled, e.g. "dir/subdir/file.txt" will produce a
parent PATH record with "dir/subdir" (the path stays relative) and a
child record with "file.txt".

>
> > > > This is wrong for three reasons:
> > > > 1. Wasting log space for redundant data (CWD path is already in the CWD
> > > > record).
>
> A CWD record is always expected for a file system operation. We are not
> missing any right now. Just don't want to lose them.
>
> > > > 2. Inconsistency with other PATH records (if a relative PARENT
> > > > directory path contains at least one component, only the verbatim
> > > > relative path is logged).
> > > > 3. In some syscalls (e.g. openat(2)) the relative path may not even be
> > > > relative to the CWD, but to another directory specified as a file
> > > > descriptor. In that case the logged path is simply plain wrong.
>
> This can be fixed in the reporting tools. The biggest problem is when we have
> several PATH records figuring our how they are all related.

Yes, but to enable fixing this, we need to keep relative paths
relative. Only then can we properly do the next step of:
a) adding sufficient information to specify what directory the path is
relative to (basically what GHAK #9 is about),
b) recognizing this information in userspace and converting relative
paths to absolute based on that information.

>
> > > > This patch modifies this behavior to simply report "." in the
> > > > aforementioned case, which is equivalent to an "empty" directory path
> > > > and can be concatenated with the actual base directory path (CWD or
> > > > dirfd from openat(2)-like syscall) once support for its logging is
> > > > added later. In the meantime, defaulting to CWD as base directory on
> > > > relative paths (as already done by the userspace tools) will be enough
> > > > to achieve results equivalent to the current behavior.
> > >
> > > I have to ask the obvious question, if we already have the necessary
> > > parent path in the CWD record, why do we need a nametype=parent PATH
> > > record anyway?
>
> CWD is where the command was issued from. Sometimes it can be used as a
> PARENT PATH record. But what if name resolution fails at the parent
> directory? That record turns out to be all we get.
>
> > > Can we safely remove it or will that cause problems for Steve's userspace
> > > tools?
>
> The PARENT records are used in figuring out what is really happening in
> certain cases.
>
> -Steve
>
>

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

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Ondrej Mosnacek]

On Fri, Aug 24, 2018 at 4:09 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Aug 3, 2018 at 3:08 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Fri, Aug 3, 2018 at 12:24 AM Paul Moore <paul@paul-moore.com> wrote:
> > > On Thu, Aug 2, 2018 at 7:45 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > >
> > > > When a relative path has just a single component and we want to emit a
> > > > nametype=PARENT record, the current implementation just reports the full
> > > > CWD path (which is alrady available in the audit context).
> > > >
> > > > This is wrong for three reasons:
> > > > 1. Wasting log space for redundant data (CWD path is already in the CWD
> > > >    record).
> > > > 2. Inconsistency with other PATH records (if a relative PARENT directory
> > > >    path contains at least one component, only the verbatim relative path
> > > >    is logged).
> > > > 3. In some syscalls (e.g. openat(2)) the relative path may not even be
> > > >    relative to the CWD, but to another directory specified as a file
> > > >    descriptor. In that case the logged path is simply plain wrong.
> > > >
> > > > This patch modifies this behavior to simply report "." in the
> > > > aforementioned case, which is equivalent to an "empty" directory path
> > > > and can be concatenated with the actual base directory path (CWD or
> > > > dirfd from openat(2)-like syscall) once support for its logging is added
> > > > later. In the meantime, defaulting to CWD as base directory on relative
> > > > paths (as already done by the userspace tools) will be enough to achieve
> > > > results equivalent to the current behavior.
> > > >
> > > > See: https://github.com/linux-audit/audit-kernel/issues/95
> > > >
> > > > Fixes: 9c937dcc7102 ("[PATCH] log more info for directory entry change events")
> > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > > ---
> > > >  kernel/audit.c | 9 ++++-----
> > > >  1 file changed, 4 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > index 2a8058764aa6..4f18bd48eb4b 100644
> > > > --- a/kernel/audit.c
> > > > +++ b/kernel/audit.c
> > > > @@ -2127,28 +2127,27 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
> > > >
> > > >         audit_log_format(ab, "item=%d", record_num);
> > > >
> > > > +       audit_log_format(ab, " name=");
> > > >         if (path)
> > > > -               audit_log_d_path(ab, " name=", path);
> > > > +               audit_log_d_path(ab, NULL, path);
> > > >         else if (n->name) {
> > > >                 switch (n->name_len) {
> > > >                 case AUDIT_NAME_FULL:
> > > >                         /* log the full path */
> > > > -                       audit_log_format(ab, " name=");
> > > >                         audit_log_untrustedstring(ab, n->name->name);
> > > >                         break;
> > > >                 case 0:
> > > >                         /* name was specified as a relative path and the
> > > >                          * directory component is the cwd */
> > > > -                       audit_log_d_path(ab, " name=", &context->pwd);
> > > > +                       audit_log_untrustedstring(ab, ".");
> > >
> > > This isn't a comprehensive review, I only gave this a quick look, but
> > > considering you are only logging "." above we can safely use
> > > audit_log_string() and safe a few cycles.
> >
> > I used audit_log_untrustedstring() to maintain the current norm that
> > the name= field would always contain a quoted string (either in
> > double-quotes or hex-escaped). I don't know if such consistency is
> > important for parsing in userspace (it should probably be robust
> > enough to handle any format), but I wanted to be on the safe side.
>
> In this particular case there should be no visible difference in the
> resulting record and audit_log_string() is going to have less overhead
> in the kernel.

OK, so should I just replace it with:

    audit_log_string(ab, ".");

or should I still escape the path manually:

    audit_log_string(ab, "\".\"");

?

>
> > > Honestly, looking at the rest of this function, why are we using
> > > audit_log_format() in the case where we are simply writing a string
> > > literal?  While I haven't tested it, simple code inspection would seem
> > > to indicate that audit_log_string() should be much quicker than
> > > audit_log_format()?  I realize this isn't strictly a problem from this
> > > patch, but we probably shouldn't be propagating this mistake any
> > > further.
> >
> > Good point. If I happen to be sending a v2 of the patch, I will switch
> > to audit_log_string() where possible. Otherwise, I'll leave it to you
> > to fix up when applying (it will probably clash with your patch
> > anyway).
>
> Please fit it yourself and resubmit.
>
> As a general rule I shouldn't need to fix things like this when
> merging.  While things like this sometimes happen, they are special
> cases and are usually the result of short deadlines, missing devs,
> etc.  In general I try to limit my modifications to merge fuzz and
> minor code changes like whitespace fixes, silly checkpatch warnings,
> etc.

Understood, will do.

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

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Ondrej Mosnacek]

On Mon, Aug 27, 2018 at 3:00 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Fri, Aug 24, 2018 at 4:09 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Fri, Aug 3, 2018 at 3:08 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > On Fri, Aug 3, 2018 at 12:24 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Thu, Aug 2, 2018 at 7:45 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > >
> > > > > When a relative path has just a single component and we want to emit a
> > > > > nametype=PARENT record, the current implementation just reports the full
> > > > > CWD path (which is alrady available in the audit context).
> > > > >
> > > > > This is wrong for three reasons:
> > > > > 1. Wasting log space for redundant data (CWD path is already in the CWD
> > > > >    record).
> > > > > 2. Inconsistency with other PATH records (if a relative PARENT directory
> > > > >    path contains at least one component, only the verbatim relative path
> > > > >    is logged).
> > > > > 3. In some syscalls (e.g. openat(2)) the relative path may not even be
> > > > >    relative to the CWD, but to another directory specified as a file
> > > > >    descriptor. In that case the logged path is simply plain wrong.
> > > > >
> > > > > This patch modifies this behavior to simply report "." in the
> > > > > aforementioned case, which is equivalent to an "empty" directory path
> > > > > and can be concatenated with the actual base directory path (CWD or
> > > > > dirfd from openat(2)-like syscall) once support for its logging is added
> > > > > later. In the meantime, defaulting to CWD as base directory on relative
> > > > > paths (as already done by the userspace tools) will be enough to achieve
> > > > > results equivalent to the current behavior.
> > > > >
> > > > > See: https://github.com/linux-audit/audit-kernel/issues/95
> > > > >
> > > > > Fixes: 9c937dcc7102 ("[PATCH] log more info for directory entry change events")
> > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > > > ---
> > > > >  kernel/audit.c | 9 ++++-----
> > > > >  1 file changed, 4 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > > index 2a8058764aa6..4f18bd48eb4b 100644
> > > > > --- a/kernel/audit.c
> > > > > +++ b/kernel/audit.c
> > > > > @@ -2127,28 +2127,27 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
> > > > >
> > > > >         audit_log_format(ab, "item=%d", record_num);
> > > > >
> > > > > +       audit_log_format(ab, " name=");
> > > > >         if (path)
> > > > > -               audit_log_d_path(ab, " name=", path);
> > > > > +               audit_log_d_path(ab, NULL, path);
> > > > >         else if (n->name) {
> > > > >                 switch (n->name_len) {
> > > > >                 case AUDIT_NAME_FULL:
> > > > >                         /* log the full path */
> > > > > -                       audit_log_format(ab, " name=");
> > > > >                         audit_log_untrustedstring(ab, n->name->name);
> > > > >                         break;
> > > > >                 case 0:
> > > > >                         /* name was specified as a relative path and the
> > > > >                          * directory component is the cwd */
> > > > > -                       audit_log_d_path(ab, " name=", &context->pwd);
> > > > > +                       audit_log_untrustedstring(ab, ".");
> > > >
> > > > This isn't a comprehensive review, I only gave this a quick look, but
> > > > considering you are only logging "." above we can safely use
> > > > audit_log_string() and safe a few cycles.
> > >
> > > I used audit_log_untrustedstring() to maintain the current norm that
> > > the name= field would always contain a quoted string (either in
> > > double-quotes or hex-escaped). I don't know if such consistency is
> > > important for parsing in userspace (it should probably be robust
> > > enough to handle any format), but I wanted to be on the safe side.
> >
> > In this particular case there should be no visible difference in the
> > resulting record and audit_log_string() is going to have less overhead
> > in the kernel.
>
> OK, so should I just replace it with:
>
>     audit_log_string(ab, ".");
>
> or should I still escape the path manually:
>
>     audit_log_string(ab, "\".\"");
>
> ?

Paul, could you please answer this question so I can move forward? :)

>
> >
> > > > Honestly, looking at the rest of this function, why are we using
> > > > audit_log_format() in the case where we are simply writing a string
> > > > literal?  While I haven't tested it, simple code inspection would seem
> > > > to indicate that audit_log_string() should be much quicker than
> > > > audit_log_format()?  I realize this isn't strictly a problem from this
> > > > patch, but we probably shouldn't be propagating this mistake any
> > > > further.
> > >
> > > Good point. If I happen to be sending a v2 of the patch, I will switch
> > > to audit_log_string() where possible. Otherwise, I'll leave it to you
> > > to fix up when applying (it will probably clash with your patch
> > > anyway).
> >
> > Please fit it yourself and resubmit.
> >
> > As a general rule I shouldn't need to fix things like this when
> > merging.  While things like this sometimes happen, they are special
> > cases and are usually the result of short deadlines, missing devs,
> > etc.  In general I try to limit my modifications to merge fuzz and
> > minor code changes like whitespace fixes, silly checkpatch warnings,
> > etc.
>
> Understood, will do.
>
> --
> Ondrej Mosnacek <omosnace at redhat dot com>
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.

Thanks,

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

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Paul Moore]

On Thu, Sep 13, 2018 at 9:58 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> Paul, could you please answer this question so I can move forward? :)

Yep, sorry for the delay; there have been some serious bugs over in
SELinux land (as well as some employer related things that should be
public soon) that have taken my focus since I've returned from the
Labor Day holidays in the US.  I'm going to start working through the
audit backlog this afternoon/evening; your patches and Jan's fixes
will likely get priority (yours are small and easy-ish to review,
Jan's fixes some potentially ugly problems).

-- 
paul moore
www.paul-moore.com

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Paul Moore]

On Thu, Sep 13, 2018 at 10:13 AM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Sep 13, 2018 at 9:58 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > Paul, could you please answer this question so I can move forward? :)
>
> Yep, sorry for the delay ...

I just went back over the original problem, your proposed fix, and all
of the discussion in this thread.

Sadly, I don't think the patch you have proposed is the right fix.

As Steve has pointed out, the CWD path is the working directory from
which the current process was executed.  I believe we should log the
full path, or as complete a path as possible, in the nametype=CWD PATH
records.  While the nametype=PARENT PATH records have a connection
with some of the other PATH records (e.g. DELETE and CREATE), the
nametype=PARENT PATH records are independent of the current working
directory, although they sometimes may be the same; in the cases where
they are the same, this is purely a coincidence and is due to
operation being performed, not something that should be seen as a
flaw.

>From what I can tell, there are issues involving the nametype=PARENT
PATH records, especially when it comes to the *at() syscalls, but no
issue where the nametype=CWD PATH records have been wrong, is that
correct?

-- 
paul moore
www.paul-moore.com

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Ondrej Mosnacek]

On Wed, Sep 19, 2018 at 3:35 AM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Sep 13, 2018 at 10:13 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Thu, Sep 13, 2018 at 9:58 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > Paul, could you please answer this question so I can move forward? :)
> >
> > Yep, sorry for the delay ...
>
> I just went back over the original problem, your proposed fix, and all
> of the discussion in this thread.
>
> Sadly, I don't think the patch you have proposed is the right fix.
>
> As Steve has pointed out, the CWD path is the working directory from
> which the current process was executed.  I believe we should log the
> full path, or as complete a path as possible, in the nametype=CWD PATH
> records.  While the nametype=PARENT PATH records have a connection
> with some of the other PATH records (e.g. DELETE and CREATE), the
> nametype=PARENT PATH records are independent of the current working
> directory, although they sometimes may be the same; in the cases where
> they are the same, this is purely a coincidence and is due to
> operation being performed, not something that should be seen as a
> flaw.
>
> From what I can tell, there are issues involving the nametype=PARENT
> PATH records, especially when it comes to the *at() syscalls, but no
> issue where the nametype=CWD PATH records have been wrong, is that
> correct?

Sorry, but I think you are completely misunderstanding the problem...
I tried to explain it several times in different ways, but apparently
I'm still not doing it right...

First of all, there is no "nametype=CWD" PATH record. There is only
the classic CWD record that is associated to every syscall and I don't
touch that one at all. The information in the CWD record is perfectly
fine.

Let me try to demonstrate it with some more verbose examples. (TL;DR:
The info in the CWD record is correct, but it is being abused in
audit_log_name().)

EXAMPLE #1 (The non-edge case):
1. A userspace process calls rename("dir1/file1", "dir2/file2") with
CWD set to "/home/user".
2. The syscall causes four calls to __audit_inode(), which generate
four 'struct audit_names' objects with the following information
(maybe not in this specific order, but that doesn't matter):
.name = "dir1/file1", .type = AUDIT_TYPE_PARENT, .name_len = 5
.name = "dir1/file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
.name = "dir2/file2", .type = AUDIT_TYPE_PARENT, .name_len = 5
.name = "dir2/file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
3. At the end of the syscall, audit_log_name()  is called on each of
these objects and produces the following PATH records (simplifed):
nametype=PARENT name="dir1/"
nametype=DELETE name="dir1/file1"
nametype=PARENT name="dir2/"
nametype=CREATE name="dir2/file2"

Notice that for the PARENT objects the .name_len is truncated to only
the directory component.

EXAMPLE #2 (The single-path-component case):
1. A userspace process calls rename("file1", "file2") with CWD set to
"/home/user".
2. The 'struct audit_names' objects will now be:
.name = "file1", .type = AUDIT_TYPE_PARENT, .name_len = 0
.name = "file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
.name = "file2", .type = AUDIT_TYPE_PARENT, .name_len = 0
.name = "file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
3. At the end of the syscall, audit_log_name()  is called on each of
these objects and produces the following PATH records (simplifed):
nametype=PARENT name="/home/user"
nametype=DELETE name="file1"
nametype=PARENT name="/home/user"
nametype=CREATE name="file2"

Notice that in this case, the "clever" logic in audit_log_name()
wanted to avoid logging an empty path (name="") in the PARENT records,
so it instead put the CWD path in there ("/home/user"). In this case
this is perfectly valid (although could be a bit surprising that there
is suddenly a full path instead of a relative one), since the full
path of "file1" is actually "/home/user/file1".

EXAMPLE #3 (The non-edge renameat(2) case):
1. A userspace process calls the following syscalls (with CWD set to
"/home/user"):
int srcfd = open("/some/path1", O_DIRECTORY | O_PATH);
int dstfd = open("/another/path2", O_DIRECTORY | O_PATH);
renameat(srcfd, "dir1/file1", dstfd, "dir2/file2");
2. The 'name', 'type' and 'name_len' fields of the 'struct
audit_names' objects will now be exactly the same as in EXAMPLE #1:
.name = "dir1/file1", .type = AUDIT_TYPE_PARENT, .name_len = 5
.name = "dir1/file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
.name = "dir2/file2", .type = AUDIT_TYPE_PARENT, .name_len = 5
.name = "dir2/file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
3. The type and name information in the PATH records will be also
exactly the same:
nametype=PARENT name="dir1/"
nametype=DELETE name="dir1/file1"
nametype=PARENT name="dir2/"
nametype=CREATE name="dir2/file2"

Even in this case the information in the records is correct (although
there is no information telling us that "dir1/..." actually
corresponds to "/some/path1/dir1/..." and "dir2/..." actually
corresponds to "/another/path2/dir2/...").

So far so good, but now we are coming to...

EXAMPLE #4 (The single-path-component renameat(2) case):
1. A userspace process calls the following syscalls (with CWD set to
"/home/user"):
int srcfd = open("/some/path1", O_DIRECTORY | O_PATH);
int dstfd = open("/another/path2", O_DIRECTORY | O_PATH);
renameat(srcfd, "file1", dstfd, "file2");
2. The 'name', 'type' and 'name_len' fields of the 'struct
audit_names' objects will now be exactly the same as in EXAMPLE #2:
.name = "file1", .type = AUDIT_TYPE_PARENT, .name_len = 0
.name = "file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
.name = "file2", .type = AUDIT_TYPE_PARENT, .name_len = 0
.name = "file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
3. The type and name information in the PATH records will be also
exactly the same:
nametype=PARENT name="/home/user"
nametype=DELETE name="file1"
nametype=PARENT name="/home/user"
nametype=CREATE name="file2"

...and HERE is the problem. The parent of "file1" is not "/home/user",
it is "/some/path1", and the parent of "file2" is also not
"/home/user", it is "/another/path2".

The proposed fix simply changes the handling of the name_len == 0 case
to output "." instead of the CWD. This doesn't solve the wider problem
that we don't have the dirfd path information (GHAK #9), but it at
least makes the situation in EXAMPLE #4 *not worse* than in EXAMPLE #3
(i.e. it fixes the less demanding GHAK #95). As an additional minor
benefit it also brings a bit more consistency - with it the PATH
records will contain relative (resp. absolute) paths if and only if
the corresponding path given to the syscall was relative (resp.
absolute).

I hope this finally clears things up.

BTW, you still didn't answer my brief question from a few replies
above (quoted below with context):

> > > > >                 case 0:
> > > > >                         /* name was specified as a relative path and the
> > > > >                          * directory component is the cwd */
> > > > > -                       audit_log_d_path(ab, " name=", &context->pwd);
> > > > > +                       audit_log_untrustedstring(ab, ".");
> > > >
> > > > This isn't a comprehensive review, I only gave this a quick look, but
> > > > considering you are only logging "." above we can safely use
> > > > audit_log_string() and safe a few cycles.
> > >
> > > I used audit_log_untrustedstring() to maintain the current norm that
> > > the name= field would always contain a quoted string (either in
> > > double-quotes or hex-escaped). I don't know if such consistency is
> > > important for parsing in userspace (it should probably be robust
> > > enough to handle any format), but I wanted to be on the safe side.
> >
> > In this particular case there should be no visible difference in the
> > resulting record and audit_log_string() is going to have less overhead
> > in the kernel.
>
> OK, so should I just replace it with:
>
>     audit_log_string(ab, ".");
>
> or should I still escape the path manually:
>
>     audit_log_string(ab, "\".\"");
>
> ?

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

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Paul Moore]

On Wed, Sep 19, 2018 at 7:01 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Wed, Sep 19, 2018 at 3:35 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Thu, Sep 13, 2018 at 10:13 AM Paul Moore <paul@paul-moore.com> wrote:
> > > On Thu, Sep 13, 2018 at 9:58 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > Paul, could you please answer this question so I can move forward? :)
> > >
> > > Yep, sorry for the delay ...
> >
> > I just went back over the original problem, your proposed fix, and all
> > of the discussion in this thread.
> >
> > Sadly, I don't think the patch you have proposed is the right fix.
> >
> > As Steve has pointed out, the CWD path is the working directory from
> > which the current process was executed.  I believe we should log the
> > full path, or as complete a path as possible, in the nametype=CWD PATH
> > records.  While the nametype=PARENT PATH records have a connection
> > with some of the other PATH records (e.g. DELETE and CREATE), the
> > nametype=PARENT PATH records are independent of the current working
> > directory, although they sometimes may be the same; in the cases where
> > they are the same, this is purely a coincidence and is due to
> > operation being performed, not something that should be seen as a
> > flaw.
> >
> > From what I can tell, there are issues involving the nametype=PARENT
> > PATH records, especially when it comes to the *at() syscalls, but no
> > issue where the nametype=CWD PATH records have been wrong, is that
> > correct?
>
> Sorry, but I think you are completely misunderstanding the problem...
> I tried to explain it several times in different ways, but apparently
> I'm still not doing it right...
>
> First of all, there is no "nametype=CWD" PATH record. There is only
> the classic CWD record that is associated to every syscall and I don't
> touch that one at all. The information in the CWD record is perfectly
> fine.

Yes, that was a casualty of me looking at too many audit logs too late
in the day, I mistakenly typed it as a nametype PATH record when CWD
is its own record type.  My apologies.

> Let me try to demonstrate it with some more verbose examples. (TL;DR:
> The info in the CWD record is correct, but it is being abused in
> audit_log_name().)
>
> EXAMPLE #1 (The non-edge case):
> 1. A userspace process calls rename("dir1/file1", "dir2/file2") with
> CWD set to "/home/user".
> 2. The syscall causes four calls to __audit_inode(), which generate
> four 'struct audit_names' objects with the following information
> (maybe not in this specific order, but that doesn't matter):
> .name = "dir1/file1", .type = AUDIT_TYPE_PARENT, .name_len = 5
> .name = "dir1/file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
> .name = "dir2/file2", .type = AUDIT_TYPE_PARENT, .name_len = 5
> .name = "dir2/file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
> 3. At the end of the syscall, audit_log_name()  is called on each of
> these objects and produces the following PATH records (simplifed):
> nametype=PARENT name="dir1/"
> nametype=DELETE name="dir1/file1"
> nametype=PARENT name="dir2/"
> nametype=CREATE name="dir2/file2"
>
> Notice that for the PARENT objects the .name_len is truncated to only
> the directory component.
>
> EXAMPLE #2 (The single-path-component case):
> 1. A userspace process calls rename("file1", "file2") with CWD set to
> "/home/user".
> 2. The 'struct audit_names' objects will now be:
> .name = "file1", .type = AUDIT_TYPE_PARENT, .name_len = 0
> .name = "file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
> .name = "file2", .type = AUDIT_TYPE_PARENT, .name_len = 0
> .name = "file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
> 3. At the end of the syscall, audit_log_name()  is called on each of
> these objects and produces the following PATH records (simplifed):
> nametype=PARENT name="/home/user"
> nametype=DELETE name="file1"
> nametype=PARENT name="/home/user"
> nametype=CREATE name="file2"
>
> Notice that in this case, the "clever" logic in audit_log_name()
> wanted to avoid logging an empty path (name="") in the PARENT records,
> so it instead put the CWD path in there ("/home/user"). In this case
> this is perfectly valid (although could be a bit surprising that there
> is suddenly a full path instead of a relative one), since the full
> path of "file1" is actually "/home/user/file1".

A quick comment on the "surprising" nature of seeing the "/home/user"
path in the PARENT record instead of "/home/user/file1" - the PARENT
record is the file's parent directory (as you mention above), so it
would be surprising to see "/home/user/file1" in the PARENT record,
seeing just "/home/user" is valid and could be expected.

> EXAMPLE #3 (The non-edge renameat(2) case):
> 1. A userspace process calls the following syscalls (with CWD set to
> "/home/user"):
> int srcfd = open("/some/path1", O_DIRECTORY | O_PATH);
> int dstfd = open("/another/path2", O_DIRECTORY | O_PATH);
> renameat(srcfd, "dir1/file1", dstfd, "dir2/file2");
> 2. The 'name', 'type' and 'name_len' fields of the 'struct
> audit_names' objects will now be exactly the same as in EXAMPLE #1:
> .name = "dir1/file1", .type = AUDIT_TYPE_PARENT, .name_len = 5
> .name = "dir1/file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
> .name = "dir2/file2", .type = AUDIT_TYPE_PARENT, .name_len = 5
> .name = "dir2/file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
> 3. The type and name information in the PATH records will be also
> exactly the same:
> nametype=PARENT name="dir1/"
> nametype=DELETE name="dir1/file1"
> nametype=PARENT name="dir2/"
> nametype=CREATE name="dir2/file2"
>
> Even in this case the information in the records is correct (although
> there is no information telling us that "dir1/..." actually
> corresponds to "/some/path1/dir1/..." and "dir2/..." actually
> corresponds to "/another/path2/dir2/...").

Yeah, I'm starting to think we should always log the absolute path in
the PARENT record.

> So far so good, but now we are coming to...
>
> EXAMPLE #4 (The single-path-component renameat(2) case):
> 1. A userspace process calls the following syscalls (with CWD set to
> "/home/user"):
> int srcfd = open("/some/path1", O_DIRECTORY | O_PATH);
> int dstfd = open("/another/path2", O_DIRECTORY | O_PATH);
> renameat(srcfd, "file1", dstfd, "file2");
> 2. The 'name', 'type' and 'name_len' fields of the 'struct
> audit_names' objects will now be exactly the same as in EXAMPLE #2:
> .name = "file1", .type = AUDIT_TYPE_PARENT, .name_len = 0
> .name = "file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
> .name = "file2", .type = AUDIT_TYPE_PARENT, .name_len = 0
> .name = "file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
> 3. The type and name information in the PATH records will be also
> exactly the same:
> nametype=PARENT name="/home/user"
> nametype=DELETE name="file1"
> nametype=PARENT name="/home/user"
> nametype=CREATE name="file2"
>
> ...and HERE is the problem. The parent of "file1" is not "/home/user",
> it is "/some/path1", and the parent of "file2" is also not
> "/home/user", it is "/another/path2".
>
> The proposed fix simply changes the handling of the name_len == 0 case
> to output "." instead of the CWD. This doesn't solve the wider problem
> that we don't have the dirfd path information (GHAK #9), but it at
> least makes the situation in EXAMPLE #4 *not worse* than in EXAMPLE #3
> (i.e. it fixes the less demanding GHAK #95). As an additional minor
> benefit it also brings a bit more consistency - with it the PATH
> records will contain relative (resp. absolute) paths if and only if
> the corresponding path given to the syscall was relative (resp.
> absolute).
>
> I hope this finally clears things up.

Yes, it does, thanks.  My apologies for getting tangled up in the CWD logging.

My current thinking is that logging relative paths in the
nametype=PARENT PATH record is a mistake.  Yes, I understand there are
some cases where this information will be the same as the CWD
information, and that could add some additional log overhead, but as
tricky as path resolution can be I think this is the safest option and
the one I would like to see us pursue.  This will likely require some
extra work for the *at() syscalls, but those aren't reported correctly
right now as discussed here and elsewhere.

I would expect the non-PARENT PATH records to stay as they are, in
other words this should only affect things which are *not* (.name_len
== AUDIT_NAME_FULL).

> BTW, you still didn't answer my brief question from a few replies
> above (quoted below with context):

FYI, I didn't answer it in my previous email because I thought logging
the "." as a PATH name was wrong, so that bit of your patch wasn't
desirable regardless of the quotes.  However, to answer your question
in a general sense, the usual guidance is to follow existing
convention and that would indicate that the 'name' field value in the
PATH record should be enclosed in double quotes.

Hopefully that answers your question, but if you're still unclear let me know.

-- 
paul moore
www.paul-moore.com

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Ondrej Mosnacek]

Sorry for the long-delayed reply, the SELinux world is keeping me
quite busy right now :)

On Wed, Sep 19, 2018 at 5:44 PM Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Sep 19, 2018 at 7:01 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Wed, Sep 19, 2018 at 3:35 AM Paul Moore <paul@paul-moore.com> wrote:
> > > On Thu, Sep 13, 2018 at 10:13 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Thu, Sep 13, 2018 at 9:58 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > > Paul, could you please answer this question so I can move forward? :)
> > > >
> > > > Yep, sorry for the delay ...
> > >
> > > I just went back over the original problem, your proposed fix, and all
> > > of the discussion in this thread.
> > >
> > > Sadly, I don't think the patch you have proposed is the right fix.
> > >
> > > As Steve has pointed out, the CWD path is the working directory from
> > > which the current process was executed.  I believe we should log the
> > > full path, or as complete a path as possible, in the nametype=CWD PATH
> > > records.  While the nametype=PARENT PATH records have a connection
> > > with some of the other PATH records (e.g. DELETE and CREATE), the
> > > nametype=PARENT PATH records are independent of the current working
> > > directory, although they sometimes may be the same; in the cases where
> > > they are the same, this is purely a coincidence and is due to
> > > operation being performed, not something that should be seen as a
> > > flaw.
> > >
> > > From what I can tell, there are issues involving the nametype=PARENT
> > > PATH records, especially when it comes to the *at() syscalls, but no
> > > issue where the nametype=CWD PATH records have been wrong, is that
> > > correct?
> >
> > Sorry, but I think you are completely misunderstanding the problem...
> > I tried to explain it several times in different ways, but apparently
> > I'm still not doing it right...
> >
> > First of all, there is no "nametype=CWD" PATH record. There is only
> > the classic CWD record that is associated to every syscall and I don't
> > touch that one at all. The information in the CWD record is perfectly
> > fine.
>
> Yes, that was a casualty of me looking at too many audit logs too late
> in the day, I mistakenly typed it as a nametype PATH record when CWD
> is its own record type.  My apologies.
>
> > Let me try to demonstrate it with some more verbose examples. (TL;DR:
> > The info in the CWD record is correct, but it is being abused in
> > audit_log_name().)
> >
> > EXAMPLE #1 (The non-edge case):
> > 1. A userspace process calls rename("dir1/file1", "dir2/file2") with
> > CWD set to "/home/user".
> > 2. The syscall causes four calls to __audit_inode(), which generate
> > four 'struct audit_names' objects with the following information
> > (maybe not in this specific order, but that doesn't matter):
> > .name = "dir1/file1", .type = AUDIT_TYPE_PARENT, .name_len = 5
> > .name = "dir1/file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
> > .name = "dir2/file2", .type = AUDIT_TYPE_PARENT, .name_len = 5
> > .name = "dir2/file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
> > 3. At the end of the syscall, audit_log_name()  is called on each of
> > these objects and produces the following PATH records (simplifed):
> > nametype=PARENT name="dir1/"
> > nametype=DELETE name="dir1/file1"
> > nametype=PARENT name="dir2/"
> > nametype=CREATE name="dir2/file2"
> >
> > Notice that for the PARENT objects the .name_len is truncated to only
> > the directory component.
> >
> > EXAMPLE #2 (The single-path-component case):
> > 1. A userspace process calls rename("file1", "file2") with CWD set to
> > "/home/user".
> > 2. The 'struct audit_names' objects will now be:
> > .name = "file1", .type = AUDIT_TYPE_PARENT, .name_len = 0
> > .name = "file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
> > .name = "file2", .type = AUDIT_TYPE_PARENT, .name_len = 0
> > .name = "file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
> > 3. At the end of the syscall, audit_log_name()  is called on each of
> > these objects and produces the following PATH records (simplifed):
> > nametype=PARENT name="/home/user"
> > nametype=DELETE name="file1"
> > nametype=PARENT name="/home/user"
> > nametype=CREATE name="file2"
> >
> > Notice that in this case, the "clever" logic in audit_log_name()
> > wanted to avoid logging an empty path (name="") in the PARENT records,
> > so it instead put the CWD path in there ("/home/user"). In this case
> > this is perfectly valid (although could be a bit surprising that there
> > is suddenly a full path instead of a relative one), since the full
> > path of "file1" is actually "/home/user/file1".
>
> A quick comment on the "surprising" nature of seeing the "/home/user"
> path in the PARENT record instead of "/home/user/file1" - the PARENT
> record is the file's parent directory (as you mention above), so it
> would be surprising to see "/home/user/file1" in the PARENT record,
> seeing just "/home/user" is valid and could be expected.

Wait, no... I meant it is surprising that there is suddenly a full
path to directory ("/home/user") instead of a relative one (which
would be "." in this case).  This happens if and only if the original
relative path has just a single component, which is a strange
condition for anyone who doesn't know how the audit_log_name()
function is implemented.  The fact that the PARENT record has a path
to the parent si obviously logical and sound, I have no problem with
that :)

>
> > EXAMPLE #3 (The non-edge renameat(2) case):
> > 1. A userspace process calls the following syscalls (with CWD set to
> > "/home/user"):
> > int srcfd = open("/some/path1", O_DIRECTORY | O_PATH);
> > int dstfd = open("/another/path2", O_DIRECTORY | O_PATH);
> > renameat(srcfd, "dir1/file1", dstfd, "dir2/file2");
> > 2. The 'name', 'type' and 'name_len' fields of the 'struct
> > audit_names' objects will now be exactly the same as in EXAMPLE #1:
> > .name = "dir1/file1", .type = AUDIT_TYPE_PARENT, .name_len = 5
> > .name = "dir1/file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
> > .name = "dir2/file2", .type = AUDIT_TYPE_PARENT, .name_len = 5
> > .name = "dir2/file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
> > 3. The type and name information in the PATH records will be also
> > exactly the same:
> > nametype=PARENT name="dir1/"
> > nametype=DELETE name="dir1/file1"
> > nametype=PARENT name="dir2/"
> > nametype=CREATE name="dir2/file2"
> >
> > Even in this case the information in the records is correct (although
> > there is no information telling us that "dir1/..." actually
> > corresponds to "/some/path1/dir1/..." and "dir2/..." actually
> > corresponds to "/another/path2/dir2/...").
>
> Yeah, I'm starting to think we should always log the absolute path in
> the PARENT record.
>
> > So far so good, but now we are coming to...
> >
> > EXAMPLE #4 (The single-path-component renameat(2) case):
> > 1. A userspace process calls the following syscalls (with CWD set to
> > "/home/user"):
> > int srcfd = open("/some/path1", O_DIRECTORY | O_PATH);
> > int dstfd = open("/another/path2", O_DIRECTORY | O_PATH);
> > renameat(srcfd, "file1", dstfd, "file2");
> > 2. The 'name', 'type' and 'name_len' fields of the 'struct
> > audit_names' objects will now be exactly the same as in EXAMPLE #2:
> > .name = "file1", .type = AUDIT_TYPE_PARENT, .name_len = 0
> > .name = "file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
> > .name = "file2", .type = AUDIT_TYPE_PARENT, .name_len = 0
> > .name = "file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
> > 3. The type and name information in the PATH records will be also
> > exactly the same:
> > nametype=PARENT name="/home/user"
> > nametype=DELETE name="file1"
> > nametype=PARENT name="/home/user"
> > nametype=CREATE name="file2"
> >
> > ...and HERE is the problem. The parent of "file1" is not "/home/user",
> > it is "/some/path1", and the parent of "file2" is also not
> > "/home/user", it is "/another/path2".
> >
> > The proposed fix simply changes the handling of the name_len == 0 case
> > to output "." instead of the CWD. This doesn't solve the wider problem
> > that we don't have the dirfd path information (GHAK #9), but it at
> > least makes the situation in EXAMPLE #4 *not worse* than in EXAMPLE #3
> > (i.e. it fixes the less demanding GHAK #95). As an additional minor
> > benefit it also brings a bit more consistency - with it the PATH
> > records will contain relative (resp. absolute) paths if and only if
> > the corresponding path given to the syscall was relative (resp.
> > absolute).
> >
> > I hope this finally clears things up.
>
> Yes, it does, thanks.  My apologies for getting tangled up in the CWD logging.
>
> My current thinking is that logging relative paths in the
> nametype=PARENT PATH record is a mistake.  Yes, I understand there are
> some cases where this information will be the same as the CWD
> information, and that could add some additional log overhead, but as
> tricky as path resolution can be I think this is the safest option and
> the one I would like to see us pursue.  This will likely require some
> extra work for the *at() syscalls, but those aren't reported correctly
> right now as discussed here and elsewhere.
>
> I would expect the non-PARENT PATH records to stay as they are, in
> other words this should only affect things which are *not* (.name_len
> == AUDIT_NAME_FULL).

Well, logging (correct) absolute path in *all* PATH records would
certainly solve the problem (and also GHAK #9) and considering all the
problems with relative paths it might even be the most reasonable
solution.  However, doing so only in the case of PARENT records
doesn't seem like a good idea to me...  Consider the first two
arguments of a renameat(2) syscall with olddirfd pointing to
"/some/dir" and an oldpath of "subdir/file".  We would get PATH
records like this:

nametype=PARENT path="/some/dir/subdir"
nametype=DELETE path="subdir/file"
[...]

Remember that whenever the audit userspace wants to turn a relative
path into absolute, it just prepends it with PWD, so we would get
paths "/some/dir/subdir" and "/path/to/pwd/subdir/file", which is even
more confusing than the current situation (where we at least get the
wrong PWD-based path consistently...).  Granted, we could make
userspace always find the corresponding PARENT record (which is not
really possible reliably as there is no linking between them), and
append the last component of CREATE/DELETE to that, but that seems
quite messy to me...

>
> > BTW, you still didn't answer my brief question from a few replies
> > above (quoted below with context):
>
> FYI, I didn't answer it in my previous email because I thought logging
> the "." as a PATH name was wrong, so that bit of your patch wasn't
> desirable regardless of the quotes.  However, to answer your question
> in a general sense, the usual guidance is to follow existing
> convention and that would indicate that the 'name' field value in the
> PATH record should be enclosed in double quotes.

Yeah, I figured but I was still curious :)

>
> Hopefully that answers your question, but if you're still unclear let me know.
>
> --
> paul moore
> www.paul-moore.com--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Paul Moore]

On Wed, Oct 31, 2018 at 4:54 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Wed, Sep 19, 2018 at 5:44 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Wed, Sep 19, 2018 at 7:01 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > On Wed, Sep 19, 2018 at 3:35 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Thu, Sep 13, 2018 at 10:13 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > > On Thu, Sep 13, 2018 at 9:58 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > > > Paul, could you please answer this question so I can move forward? :)
> > > > >
> > > > > Yep, sorry for the delay ...
> > > >
> > > > I just went back over the original problem, your proposed fix, and all
> > > > of the discussion in this thread.
> > > >
> > > > Sadly, I don't think the patch you have proposed is the right fix.
> > > >
> > > > As Steve has pointed out, the CWD path is the working directory from
> > > > which the current process was executed.  I believe we should log the
> > > > full path, or as complete a path as possible, in the nametype=CWD PATH
> > > > records.  While the nametype=PARENT PATH records have a connection
> > > > with some of the other PATH records (e.g. DELETE and CREATE), the
> > > > nametype=PARENT PATH records are independent of the current working
> > > > directory, although they sometimes may be the same; in the cases where
> > > > they are the same, this is purely a coincidence and is due to
> > > > operation being performed, not something that should be seen as a
> > > > flaw.
> > > >
> > > > From what I can tell, there are issues involving the nametype=PARENT
> > > > PATH records, especially when it comes to the *at() syscalls, but no
> > > > issue where the nametype=CWD PATH records have been wrong, is that
> > > > correct?
> > >
> > > Sorry, but I think you are completely misunderstanding the problem...
> > > I tried to explain it several times in different ways, but apparently
> > > I'm still not doing it right...
> > >
> > > First of all, there is no "nametype=CWD" PATH record. There is only
> > > the classic CWD record that is associated to every syscall and I don't
> > > touch that one at all. The information in the CWD record is perfectly
> > > fine.
> >
> > Yes, that was a casualty of me looking at too many audit logs too late
> > in the day, I mistakenly typed it as a nametype PATH record when CWD
> > is its own record type.  My apologies.
> >
> > > Let me try to demonstrate it with some more verbose examples. (TL;DR:
> > > The info in the CWD record is correct, but it is being abused in
> > > audit_log_name().)
> > >
> > > EXAMPLE #1 (The non-edge case):
> > > 1. A userspace process calls rename("dir1/file1", "dir2/file2") with
> > > CWD set to "/home/user".
> > > 2. The syscall causes four calls to __audit_inode(), which generate
> > > four 'struct audit_names' objects with the following information
> > > (maybe not in this specific order, but that doesn't matter):
> > > .name = "dir1/file1", .type = AUDIT_TYPE_PARENT, .name_len = 5
> > > .name = "dir1/file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
> > > .name = "dir2/file2", .type = AUDIT_TYPE_PARENT, .name_len = 5
> > > .name = "dir2/file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
> > > 3. At the end of the syscall, audit_log_name()  is called on each of
> > > these objects and produces the following PATH records (simplifed):
> > > nametype=PARENT name="dir1/"
> > > nametype=DELETE name="dir1/file1"
> > > nametype=PARENT name="dir2/"
> > > nametype=CREATE name="dir2/file2"
> > >
> > > Notice that for the PARENT objects the .name_len is truncated to only
> > > the directory component.
> > >
> > > EXAMPLE #2 (The single-path-component case):
> > > 1. A userspace process calls rename("file1", "file2") with CWD set to
> > > "/home/user".
> > > 2. The 'struct audit_names' objects will now be:
> > > .name = "file1", .type = AUDIT_TYPE_PARENT, .name_len = 0
> > > .name = "file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
> > > .name = "file2", .type = AUDIT_TYPE_PARENT, .name_len = 0
> > > .name = "file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
> > > 3. At the end of the syscall, audit_log_name()  is called on each of
> > > these objects and produces the following PATH records (simplifed):
> > > nametype=PARENT name="/home/user"
> > > nametype=DELETE name="file1"
> > > nametype=PARENT name="/home/user"
> > > nametype=CREATE name="file2"
> > >
> > > Notice that in this case, the "clever" logic in audit_log_name()
> > > wanted to avoid logging an empty path (name="") in the PARENT records,
> > > so it instead put the CWD path in there ("/home/user"). In this case
> > > this is perfectly valid (although could be a bit surprising that there
> > > is suddenly a full path instead of a relative one), since the full
> > > path of "file1" is actually "/home/user/file1".
> >
> > A quick comment on the "surprising" nature of seeing the "/home/user"
> > path in the PARENT record instead of "/home/user/file1" - the PARENT
> > record is the file's parent directory (as you mention above), so it
> > would be surprising to see "/home/user/file1" in the PARENT record,
> > seeing just "/home/user" is valid and could be expected.
>
> Wait, no... I meant it is surprising that there is suddenly a full
> path to directory ("/home/user") instead of a relative one (which
> would be "." in this case).  This happens if and only if the original
> relative path has just a single component, which is a strange
> condition for anyone who doesn't know how the audit_log_name()
> function is implemented.  The fact that the PARENT record has a path
> to the parent si obviously logical and sound, I have no problem with
> that :)
>
> >
> > > EXAMPLE #3 (The non-edge renameat(2) case):
> > > 1. A userspace process calls the following syscalls (with CWD set to
> > > "/home/user"):
> > > int srcfd = open("/some/path1", O_DIRECTORY | O_PATH);
> > > int dstfd = open("/another/path2", O_DIRECTORY | O_PATH);
> > > renameat(srcfd, "dir1/file1", dstfd, "dir2/file2");
> > > 2. The 'name', 'type' and 'name_len' fields of the 'struct
> > > audit_names' objects will now be exactly the same as in EXAMPLE #1:
> > > .name = "dir1/file1", .type = AUDIT_TYPE_PARENT, .name_len = 5
> > > .name = "dir1/file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
> > > .name = "dir2/file2", .type = AUDIT_TYPE_PARENT, .name_len = 5
> > > .name = "dir2/file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
> > > 3. The type and name information in the PATH records will be also
> > > exactly the same:
> > > nametype=PARENT name="dir1/"
> > > nametype=DELETE name="dir1/file1"
> > > nametype=PARENT name="dir2/"
> > > nametype=CREATE name="dir2/file2"
> > >
> > > Even in this case the information in the records is correct (although
> > > there is no information telling us that "dir1/..." actually
> > > corresponds to "/some/path1/dir1/..." and "dir2/..." actually
> > > corresponds to "/another/path2/dir2/...").
> >
> > Yeah, I'm starting to think we should always log the absolute path in
> > the PARENT record.
> >
> > > So far so good, but now we are coming to...
> > >
> > > EXAMPLE #4 (The single-path-component renameat(2) case):
> > > 1. A userspace process calls the following syscalls (with CWD set to
> > > "/home/user"):
> > > int srcfd = open("/some/path1", O_DIRECTORY | O_PATH);
> > > int dstfd = open("/another/path2", O_DIRECTORY | O_PATH);
> > > renameat(srcfd, "file1", dstfd, "file2");
> > > 2. The 'name', 'type' and 'name_len' fields of the 'struct
> > > audit_names' objects will now be exactly the same as in EXAMPLE #2:
> > > .name = "file1", .type = AUDIT_TYPE_PARENT, .name_len = 0
> > > .name = "file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
> > > .name = "file2", .type = AUDIT_TYPE_PARENT, .name_len = 0
> > > .name = "file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
> > > 3. The type and name information in the PATH records will be also
> > > exactly the same:
> > > nametype=PARENT name="/home/user"
> > > nametype=DELETE name="file1"
> > > nametype=PARENT name="/home/user"
> > > nametype=CREATE name="file2"
> > >
> > > ...and HERE is the problem. The parent of "file1" is not "/home/user",
> > > it is "/some/path1", and the parent of "file2" is also not
> > > "/home/user", it is "/another/path2".
> > >
> > > The proposed fix simply changes the handling of the name_len == 0 case
> > > to output "." instead of the CWD. This doesn't solve the wider problem
> > > that we don't have the dirfd path information (GHAK #9), but it at
> > > least makes the situation in EXAMPLE #4 *not worse* than in EXAMPLE #3
> > > (i.e. it fixes the less demanding GHAK #95). As an additional minor
> > > benefit it also brings a bit more consistency - with it the PATH
> > > records will contain relative (resp. absolute) paths if and only if
> > > the corresponding path given to the syscall was relative (resp.
> > > absolute).
> > >
> > > I hope this finally clears things up.
> >
> > Yes, it does, thanks.  My apologies for getting tangled up in the CWD logging.
> >
> > My current thinking is that logging relative paths in the
> > nametype=PARENT PATH record is a mistake.  Yes, I understand there are
> > some cases where this information will be the same as the CWD
> > information, and that could add some additional log overhead, but as
> > tricky as path resolution can be I think this is the safest option and
> > the one I would like to see us pursue.  This will likely require some
> > extra work for the *at() syscalls, but those aren't reported correctly
> > right now as discussed here and elsewhere.
> >
> > I would expect the non-PARENT PATH records to stay as they are, in
> > other words this should only affect things which are *not* (.name_len
> > == AUDIT_NAME_FULL).
>
> Well, logging (correct) absolute path in *all* PATH records would
> certainly solve the problem (and also GHAK #9) and considering all the
> problems with relative paths it might even be the most reasonable
> solution.  However, doing so only in the case of PARENT records
> doesn't seem like a good idea to me...  Consider the first two
> arguments of a renameat(2) syscall with olddirfd pointing to
> "/some/dir" and an oldpath of "subdir/file".  We would get PATH
> records like this:
>
> nametype=PARENT path="/some/dir/subdir"
> nametype=DELETE path="subdir/file"
> [...]

Let's reset this discussion a bit ... if we abolish relative paths and
make everything absolute, is there even a need to log PARENT?

-- 
paul moore
www.paul-moore.com

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Ondrej Mosnacek]

On Tue, Nov 6, 2018 at 12:30 AM Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Oct 31, 2018 at 4:54 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Wed, Sep 19, 2018 at 5:44 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Wed, Sep 19, 2018 at 7:01 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > On Wed, Sep 19, 2018 at 3:35 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > > On Thu, Sep 13, 2018 at 10:13 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > > > On Thu, Sep 13, 2018 at 9:58 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > > > > Paul, could you please answer this question so I can move forward? :)
> > > > > >
> > > > > > Yep, sorry for the delay ...
> > > > >
> > > > > I just went back over the original problem, your proposed fix, and all
> > > > > of the discussion in this thread.
> > > > >
> > > > > Sadly, I don't think the patch you have proposed is the right fix.
> > > > >
> > > > > As Steve has pointed out, the CWD path is the working directory from
> > > > > which the current process was executed.  I believe we should log the
> > > > > full path, or as complete a path as possible, in the nametype=CWD PATH
> > > > > records.  While the nametype=PARENT PATH records have a connection
> > > > > with some of the other PATH records (e.g. DELETE and CREATE), the
> > > > > nametype=PARENT PATH records are independent of the current working
> > > > > directory, although they sometimes may be the same; in the cases where
> > > > > they are the same, this is purely a coincidence and is due to
> > > > > operation being performed, not something that should be seen as a
> > > > > flaw.
> > > > >
> > > > > From what I can tell, there are issues involving the nametype=PARENT
> > > > > PATH records, especially when it comes to the *at() syscalls, but no
> > > > > issue where the nametype=CWD PATH records have been wrong, is that
> > > > > correct?
> > > >
> > > > Sorry, but I think you are completely misunderstanding the problem...
> > > > I tried to explain it several times in different ways, but apparently
> > > > I'm still not doing it right...
> > > >
> > > > First of all, there is no "nametype=CWD" PATH record. There is only
> > > > the classic CWD record that is associated to every syscall and I don't
> > > > touch that one at all. The information in the CWD record is perfectly
> > > > fine.
> > >
> > > Yes, that was a casualty of me looking at too many audit logs too late
> > > in the day, I mistakenly typed it as a nametype PATH record when CWD
> > > is its own record type.  My apologies.
> > >
> > > > Let me try to demonstrate it with some more verbose examples. (TL;DR:
> > > > The info in the CWD record is correct, but it is being abused in
> > > > audit_log_name().)
> > > >
> > > > EXAMPLE #1 (The non-edge case):
> > > > 1. A userspace process calls rename("dir1/file1", "dir2/file2") with
> > > > CWD set to "/home/user".
> > > > 2. The syscall causes four calls to __audit_inode(), which generate
> > > > four 'struct audit_names' objects with the following information
> > > > (maybe not in this specific order, but that doesn't matter):
> > > > .name = "dir1/file1", .type = AUDIT_TYPE_PARENT, .name_len = 5
> > > > .name = "dir1/file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
> > > > .name = "dir2/file2", .type = AUDIT_TYPE_PARENT, .name_len = 5
> > > > .name = "dir2/file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
> > > > 3. At the end of the syscall, audit_log_name()  is called on each of
> > > > these objects and produces the following PATH records (simplifed):
> > > > nametype=PARENT name="dir1/"
> > > > nametype=DELETE name="dir1/file1"
> > > > nametype=PARENT name="dir2/"
> > > > nametype=CREATE name="dir2/file2"
> > > >
> > > > Notice that for the PARENT objects the .name_len is truncated to only
> > > > the directory component.
> > > >
> > > > EXAMPLE #2 (The single-path-component case):
> > > > 1. A userspace process calls rename("file1", "file2") with CWD set to
> > > > "/home/user".
> > > > 2. The 'struct audit_names' objects will now be:
> > > > .name = "file1", .type = AUDIT_TYPE_PARENT, .name_len = 0
> > > > .name = "file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
> > > > .name = "file2", .type = AUDIT_TYPE_PARENT, .name_len = 0
> > > > .name = "file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
> > > > 3. At the end of the syscall, audit_log_name()  is called on each of
> > > > these objects and produces the following PATH records (simplifed):
> > > > nametype=PARENT name="/home/user"
> > > > nametype=DELETE name="file1"
> > > > nametype=PARENT name="/home/user"
> > > > nametype=CREATE name="file2"
> > > >
> > > > Notice that in this case, the "clever" logic in audit_log_name()
> > > > wanted to avoid logging an empty path (name="") in the PARENT records,
> > > > so it instead put the CWD path in there ("/home/user"). In this case
> > > > this is perfectly valid (although could be a bit surprising that there
> > > > is suddenly a full path instead of a relative one), since the full
> > > > path of "file1" is actually "/home/user/file1".
> > >
> > > A quick comment on the "surprising" nature of seeing the "/home/user"
> > > path in the PARENT record instead of "/home/user/file1" - the PARENT
> > > record is the file's parent directory (as you mention above), so it
> > > would be surprising to see "/home/user/file1" in the PARENT record,
> > > seeing just "/home/user" is valid and could be expected.
> >
> > Wait, no... I meant it is surprising that there is suddenly a full
> > path to directory ("/home/user") instead of a relative one (which
> > would be "." in this case).  This happens if and only if the original
> > relative path has just a single component, which is a strange
> > condition for anyone who doesn't know how the audit_log_name()
> > function is implemented.  The fact that the PARENT record has a path
> > to the parent si obviously logical and sound, I have no problem with
> > that :)
> >
> > >
> > > > EXAMPLE #3 (The non-edge renameat(2) case):
> > > > 1. A userspace process calls the following syscalls (with CWD set to
> > > > "/home/user"):
> > > > int srcfd = open("/some/path1", O_DIRECTORY | O_PATH);
> > > > int dstfd = open("/another/path2", O_DIRECTORY | O_PATH);
> > > > renameat(srcfd, "dir1/file1", dstfd, "dir2/file2");
> > > > 2. The 'name', 'type' and 'name_len' fields of the 'struct
> > > > audit_names' objects will now be exactly the same as in EXAMPLE #1:
> > > > .name = "dir1/file1", .type = AUDIT_TYPE_PARENT, .name_len = 5
> > > > .name = "dir1/file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
> > > > .name = "dir2/file2", .type = AUDIT_TYPE_PARENT, .name_len = 5
> > > > .name = "dir2/file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
> > > > 3. The type and name information in the PATH records will be also
> > > > exactly the same:
> > > > nametype=PARENT name="dir1/"
> > > > nametype=DELETE name="dir1/file1"
> > > > nametype=PARENT name="dir2/"
> > > > nametype=CREATE name="dir2/file2"
> > > >
> > > > Even in this case the information in the records is correct (although
> > > > there is no information telling us that "dir1/..." actually
> > > > corresponds to "/some/path1/dir1/..." and "dir2/..." actually
> > > > corresponds to "/another/path2/dir2/...").
> > >
> > > Yeah, I'm starting to think we should always log the absolute path in
> > > the PARENT record.
> > >
> > > > So far so good, but now we are coming to...
> > > >
> > > > EXAMPLE #4 (The single-path-component renameat(2) case):
> > > > 1. A userspace process calls the following syscalls (with CWD set to
> > > > "/home/user"):
> > > > int srcfd = open("/some/path1", O_DIRECTORY | O_PATH);
> > > > int dstfd = open("/another/path2", O_DIRECTORY | O_PATH);
> > > > renameat(srcfd, "file1", dstfd, "file2");
> > > > 2. The 'name', 'type' and 'name_len' fields of the 'struct
> > > > audit_names' objects will now be exactly the same as in EXAMPLE #2:
> > > > .name = "file1", .type = AUDIT_TYPE_PARENT, .name_len = 0
> > > > .name = "file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL
> > > > .name = "file2", .type = AUDIT_TYPE_PARENT, .name_len = 0
> > > > .name = "file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL
> > > > 3. The type and name information in the PATH records will be also
> > > > exactly the same:
> > > > nametype=PARENT name="/home/user"
> > > > nametype=DELETE name="file1"
> > > > nametype=PARENT name="/home/user"
> > > > nametype=CREATE name="file2"
> > > >
> > > > ...and HERE is the problem. The parent of "file1" is not "/home/user",
> > > > it is "/some/path1", and the parent of "file2" is also not
> > > > "/home/user", it is "/another/path2".
> > > >
> > > > The proposed fix simply changes the handling of the name_len == 0 case
> > > > to output "." instead of the CWD. This doesn't solve the wider problem
> > > > that we don't have the dirfd path information (GHAK #9), but it at
> > > > least makes the situation in EXAMPLE #4 *not worse* than in EXAMPLE #3
> > > > (i.e. it fixes the less demanding GHAK #95). As an additional minor
> > > > benefit it also brings a bit more consistency - with it the PATH
> > > > records will contain relative (resp. absolute) paths if and only if
> > > > the corresponding path given to the syscall was relative (resp.
> > > > absolute).
> > > >
> > > > I hope this finally clears things up.
> > >
> > > Yes, it does, thanks.  My apologies for getting tangled up in the CWD logging.
> > >
> > > My current thinking is that logging relative paths in the
> > > nametype=PARENT PATH record is a mistake.  Yes, I understand there are
> > > some cases where this information will be the same as the CWD
> > > information, and that could add some additional log overhead, but as
> > > tricky as path resolution can be I think this is the safest option and
> > > the one I would like to see us pursue.  This will likely require some
> > > extra work for the *at() syscalls, but those aren't reported correctly
> > > right now as discussed here and elsewhere.
> > >
> > > I would expect the non-PARENT PATH records to stay as they are, in
> > > other words this should only affect things which are *not* (.name_len
> > > == AUDIT_NAME_FULL).
> >
> > Well, logging (correct) absolute path in *all* PATH records would
> > certainly solve the problem (and also GHAK #9) and considering all the
> > problems with relative paths it might even be the most reasonable
> > solution.  However, doing so only in the case of PARENT records
> > doesn't seem like a good idea to me...  Consider the first two
> > arguments of a renameat(2) syscall with olddirfd pointing to
> > "/some/dir" and an oldpath of "subdir/file".  We would get PATH
> > records like this:
> >
> > nametype=PARENT path="/some/dir/subdir"
> > nametype=DELETE path="subdir/file"
> > [...]
>
> Let's reset this discussion a bit ... if we abolish relative paths and
> make everything absolute, is there even a need to log PARENT?

If there ever was such need, then this won't change when we switch to
absolute paths. The PATH records contain some fields (inode, dev, obj,
...) that can be different for the child and parent and I would say
these are the only new information that the PARENT records provide
over the corresponding CREATE/DELETE records. The parent's path could
be always inferred from the CREATE/DELETE record anyway (by simply
removing the last element). I think the only reason that PARENT PATH
records contain a "path" field is simply consistency (it is a
mandatory field). The advantage of this is that userspace then doesn't
need to jump through the hoops to process the records (the parent path
is "just there", no need to look for other corresponding records).

Of course all of the above are just educated guesses on the original
intentions, but when you look at some actual PATH records it does make
sense (IMHO).

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

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Paul Moore]

On Tue, Nov 6, 2018 at 3:09 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Tue, Nov 6, 2018 at 12:30 AM Paul Moore <paul@paul-moore.com> wrote:
> > Let's reset this discussion a bit ... if we abolish relative paths and
> > make everything absolute, is there even a need to log PARENT?
>
> If there ever was such need, then this won't change when we switch to
> absolute paths. The PATH records contain some fields (inode, dev, obj,
> ...) that can be different for the child and parent and I would say
> these are the only new information that the PARENT records provide
> over the corresponding CREATE/DELETE records.

Sigh.  Of course the inode information is going to be different
between the object in question and the parent, they are different
filesystem objects.  Ask your self the bigger question: does the
PARENT record provide me any security relevant information related to
the filesystem object that is being accessed?

With the messed up state of path name auditing, the PARENT records are
useful when trying to recreate the full path used by the process to
access a given filesystem object (transient as it may be, the path
name can still be useful after the fact).  If we switch to always
recording absolute path names, why do we care about recording the
PARENT filesystem object at all (both the path and the inode
information)?

-- 
paul moore
www.paul-moore.com

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Ondrej Mosnacek]

On Tue, Nov 6, 2018 at 9:19 PM Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Nov 6, 2018 at 3:09 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Tue, Nov 6, 2018 at 12:30 AM Paul Moore <paul@paul-moore.com> wrote:
> > > Let's reset this discussion a bit ... if we abolish relative paths and
> > > make everything absolute, is there even a need to log PARENT?
> >
> > If there ever was such need, then this won't change when we switch to
> > absolute paths. The PATH records contain some fields (inode, dev, obj,
> > ...) that can be different for the child and parent and I would say
> > these are the only new information that the PARENT records provide
> > over the corresponding CREATE/DELETE records.
>
> Sigh.  Of course the inode information is going to be different
> between the object in question and the parent, they are different
> filesystem objects.  Ask your self the bigger question: does the
> PARENT record provide me any security relevant information related to
> the filesystem object that is being accessed?

I would say it does. Consider e.g. the "mode" and "obj" fields. When
you move (rename) a file from one directory to another (which is the
main, if not the only, case when a PARENT record is emitted), then you
are usually more interested in the values for the parent directory
than the file itself (that's what determines if you can move the
file).

For example, assume you have a rule that logs whenever some sensitive
file gets moved. You do not expect that to happen because you set the
file/directory permissions and labels so that it can't be done by
anyone unauthorized. But something goes wrong, the permissions/labels
get changed somehow and a bad actor leverages the situation to move
the file. Then later you want to investigate this security incident
and as part of it you want to know what permissions were set on the
directories involved that had allowed the file to be moved, because
this may give you a useful lead. With PARENT records, you get such
information, without them you don't.

>
> With the messed up state of path name auditing, the PARENT records are
> useful when trying to recreate the full path used by the process to
> access a given filesystem object (transient as it may be, the path
> name can still be useful after the fact).  If we switch to always
> recording absolute path names, why do we care about recording the
> PARENT filesystem object at all (both the path and the inode
> information)?

I disagree with your assumption that the PARENT record somehow helps
to determine the full path of the (child) filesystem object, in the
sense that it provides more information than what is already contained
in the corresponding CREATE/DELETE record. (Please correct me if
that's not what you were trying to say.) When we log the paths as we
do now, you either get a relative path in both PARENT and
CREATE/DELETE records (the PARENT path just being one element shorter)
or you get a full path in both records (again both will be the same
except the PARENT path will have the last component stripped),
depending on whether the user passed a relative or absolute path to
the syscall [*]. If we switch to always logging full paths, we simply
eliminate the first case (both paths will be always absolute).

Whether we switch to always logging absolute paths or not, the value
of the "path" field of the PARENT record is somewhat redundant (but I
don't see that as a problem).

[*] Disregarding the occasional glitch of getting the (full) path of
current directory as PARENT path when the child path is relative with
just a single component, a.k.a. GHAK #95...

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

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Paul Moore]

On Tue, Nov 13, 2018 at 10:25 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Tue, Nov 6, 2018 at 9:19 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Nov 6, 2018 at 3:09 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > On Tue, Nov 6, 2018 at 12:30 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > Let's reset this discussion a bit ... if we abolish relative paths and
> > > > make everything absolute, is there even a need to log PARENT?
> > >
> > > If there ever was such need, then this won't change when we switch to
> > > absolute paths. The PATH records contain some fields (inode, dev, obj,
> > > ...) that can be different for the child and parent and I would say
> > > these are the only new information that the PARENT records provide
> > > over the corresponding CREATE/DELETE records.
> >
> > Sigh.  Of course the inode information is going to be different
> > between the object in question and the parent, they are different
> > filesystem objects.  Ask your self the bigger question: does the
> > PARENT record provide me any security relevant information related to
> > the filesystem object that is being accessed?
>
> I would say it does. Consider e.g. the "mode" and "obj" fields. When
> you move (rename) a file from one directory to another (which is the
> main, if not the only, case when a PARENT record is emitted), then you
> are usually more interested in the values for the parent directory
> than the file itself (that's what determines if you can move the
> file).

I disagree on the importance of the mode/obj of the parent in a rename
operation.  From my perspective I really only care about the
filesystem object that is being moved and if it succeeded or not.  The
idea that you care more about the parent than the object being moved
makes no sense to me at all.

> For example, assume you have a rule that logs whenever some sensitive
> file gets moved. You do not expect that to happen because you set the
> file/directory permissions and labels so that it can't be done by
> anyone unauthorized. But something goes wrong, the permissions/labels
> get changed somehow ...

In which case you should be watching for changes to the filesystem
metadata which affect access rights.  That is how you should catch
changes to permissions on a filesystem object as it gives you
information about the change as well as the subject information of the
user/process which made the change.

> ... and a bad actor leverages the situation to move
> the file. Then later you want to investigate this security incident
> and as part of it you want to know what permissions were set on the
> directories involved that had allowed the file to be moved, because
> this may give you a useful lead. With PARENT records, you get such
> information, without them you don't.

If you only have that information in the parent record then you are
missing half the story, and it may be the important half as the
interesting bit of information in this example is the identity of the
user/process which was able to change permissions to enable the rename
to take place.

Unless Steve provides evidence of some compelling certification
requirement which necessitates the need for a parent record, I see no
reason to keep it.

-- 
paul moore
www.paul-moore.com

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Steve Grubb]

On Tuesday, November 13, 2018 11:30:55 AM EST Paul Moore wrote:
> On Tue, Nov 13, 2018 at 10:25 AM Ondrej Mosnacek <omosnace@redhat.com> 
wrote:
> > On Tue, Nov 6, 2018 at 9:19 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Tue, Nov 6, 2018 at 3:09 AM Ondrej Mosnacek <omosnace@redhat.com> 
wrote:
> > > > On Tue, Nov 6, 2018 at 12:30 AM Paul Moore <paul@paul-moore.com> 
wrote:
> > > > > Let's reset this discussion a bit ... if we abolish relative paths
> > > > > and make everything absolute, is there even a need to log PARENT?

I believe that Al Viro has said that sometime paths are not resolvable in the 
future. For example process A opens a file. It passes the descriptor to 
another process using scm rights. Directory tree is deleted. Process B 
receives the descriptor. Process A exits. Process B is now accessing what?


> > > > If there ever was such need, then this won't change when we switch to
> > > > absolute paths. The PATH records contain some fields (inode, dev,
> > > > obj, that can be different for the child and parent and I would say
> > > > these are the only new information that the PARENT records provide
> > > > over the corresponding CREATE/DELETE records.
> > > 
> > > Sigh.  Of course the inode information is going to be different
> > > between the object in question and the parent, they are different
> > > filesystem objects.  Ask your self the bigger question: does the
> > > PARENT record provide me any security relevant information related to
> > > the filesystem object that is being accessed?
> > 
> > I would say it does. Consider e.g. the "mode" and "obj" fields. When
> > you move (rename) a file from one directory to another (which is the
> > main, if not the only, case when a PARENT record is emitted), then you
> > are usually more interested in the values for the parent directory
> > than the file itself (that's what determines if you can move the
> > file).
> 
> I disagree on the importance of the mode/obj of the parent in a rename
> operation.  From my perspective I really only care about the
> filesystem object that is being moved and if it succeeded or not.  The
> idea that you care more about the parent than the object being moved
> makes no sense to me at all.

The mode is really not important.

> > For example, assume you have a rule that logs whenever some sensitive
> > file gets moved. You do not expect that to happen because you set the
> > file/directory permissions and labels so that it can't be done by
> > anyone unauthorized. But something goes wrong, the permissions/labels
> > get changed somehow ...
> 
> In which case you should be watching for changes to the filesystem
> metadata which affect access rights.  That is how you should catch
> changes to permissions on a filesystem object as it gives you
> information about the change as well as the subject information of the
> user/process which made the change.

Right. You would watch for attribute changes on a directory.


> > ... and a bad actor leverages the situation to move
> > the file. Then later you want to investigate this security incident
> > and as part of it you want to know what permissions were set on the
> > directories involved that had allowed the file to be moved, because
> > this may give you a useful lead. With PARENT records, you get such
> > information, without them you don't.
> 
> If you only have that information in the parent record then you are
> missing half the story, and it may be the important half as the
> interesting bit of information in this example is the identity of the
> user/process which was able to change permissions to enable the rename
> to take place.
> 
> Unless Steve provides evidence of some compelling certification
> requirement which necessitates the need for a parent record, I see no
> reason to keep it.

Certification does not care about parent records. What is cares about is being 
able to say what the object was that is acted upon. So, that would be device 
and inode. But that is not nice for people. So, we would also like to know 
the path name. If parent record are necessary because of the *at syscalls, 
then that may be the only purpose. And they do not need to be emitted each 
time. If we also have a full path, then they are not needed. If we have a 
relative path, then CWD is needed, not the parent.

I also understand that at times full path resolution may not work out due to 
directory permissions blocking access at a deeper level of the directory 
tree. In those cases, we probably do want a PARENT record for the failed 
access attempt. I think that discussion may have prompted creation of PARENT 
records a long time ago. But at the same time, I also mentioned that the path 
was passed as an arguement and we could always emit that...but we do not have 
any other information such as mode or security label.

In summary, certification does not say we need PARENT directories of the 
object. We need the object. And we only need help when its not clear what the 
object was.

Hope this helps...

-Steve

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Paul Moore]

On Sat, Dec 1, 2018 at 11:50 AM Steve Grubb <sgrubb@redhat.com> wrote:
> On Tuesday, November 13, 2018 11:30:55 AM EST Paul Moore wrote:
> > On Tue, Nov 13, 2018 at 10:25 AM Ondrej Mosnacek <omosnace@redhat.com>
> wrote:
> > > On Tue, Nov 6, 2018 at 9:19 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Tue, Nov 6, 2018 at 3:09 AM Ondrej Mosnacek <omosnace@redhat.com>
> wrote:
> > > > > On Tue, Nov 6, 2018 at 12:30 AM Paul Moore <paul@paul-moore.com>
> wrote:
> > > > > > Let's reset this discussion a bit ... if we abolish relative paths
> > > > > > and make everything absolute, is there even a need to log PARENT?
>
> I believe that Al Viro has said that sometime paths are not resolvable in the
> future. For example process A opens a file. It passes the descriptor to
> another process using scm rights. Directory tree is deleted. Process B
> receives the descriptor. Process A exits. Process B is now accessing what?

The window is even more narrow than that, in theory the file path
could change before the thread returns from open/openat/etc.  The only
time a path is valid for any given file is when is is being resolved;
this is why we have to go through all the pain that we do when
auditing path names, you can't get them after opening the file.

> > > > > If there ever was such need, then this won't change when we switch to
> > > > > absolute paths. The PATH records contain some fields (inode, dev,
> > > > > obj, that can be different for the child and parent and I would say
> > > > > these are the only new information that the PARENT records provide
> > > > > over the corresponding CREATE/DELETE records.
> > > >
> > > > Sigh.  Of course the inode information is going to be different
> > > > between the object in question and the parent, they are different
> > > > filesystem objects.  Ask your self the bigger question: does the
> > > > PARENT record provide me any security relevant information related to
> > > > the filesystem object that is being accessed?
> > >
> > > I would say it does. Consider e.g. the "mode" and "obj" fields. When
> > > you move (rename) a file from one directory to another (which is the
> > > main, if not the only, case when a PARENT record is emitted), then you
> > > are usually more interested in the values for the parent directory
> > > than the file itself (that's what determines if you can move the
> > > file).
> >
> > I disagree on the importance of the mode/obj of the parent in a rename
> > operation.  From my perspective I really only care about the
> > filesystem object that is being moved and if it succeeded or not.  The
> > idea that you care more about the parent than the object being moved
> > makes no sense to me at all.
>
> The mode is really not important.
>
> > > For example, assume you have a rule that logs whenever some sensitive
> > > file gets moved. You do not expect that to happen because you set the
> > > file/directory permissions and labels so that it can't be done by
> > > anyone unauthorized. But something goes wrong, the permissions/labels
> > > get changed somehow ...
> >
> > In which case you should be watching for changes to the filesystem
> > metadata which affect access rights.  That is how you should catch
> > changes to permissions on a filesystem object as it gives you
> > information about the change as well as the subject information of the
> > user/process which made the change.
>
> Right. You would watch for attribute changes on a directory.
>
>
> > > ... and a bad actor leverages the situation to move
> > > the file. Then later you want to investigate this security incident
> > > and as part of it you want to know what permissions were set on the
> > > directories involved that had allowed the file to be moved, because
> > > this may give you a useful lead. With PARENT records, you get such
> > > information, without them you don't.
> >
> > If you only have that information in the parent record then you are
> > missing half the story, and it may be the important half as the
> > interesting bit of information in this example is the identity of the
> > user/process which was able to change permissions to enable the rename
> > to take place.
> >
> > Unless Steve provides evidence of some compelling certification
> > requirement which necessitates the need for a parent record, I see no
> > reason to keep it.
>
> Certification does not care about parent records. What is cares about is being
> able to say what the object was that is acted upon. So, that would be device
> and inode.

... which we track in the PATH record, so we should be okay on this point ...

> But that is not nice for people. So, we would also like to know
> the path name. If parent record are necessary because of the *at syscalls,
> then that may be the only purpose. And they do not need to be emitted each
> time. If we also have a full path, then they are not needed.

... that is what I was thinking, and why I suggested we can get rid of
them if we get rid of relative paths.

> If we have a relative path, then CWD is needed, not the parent.
>
> I also understand that at times full path resolution may not work out due to
> directory permissions blocking access at a deeper level of the directory
> tree. In those cases, we probably do want a PARENT record for the failed
> access attempt. I think that discussion may have prompted creation of PARENT
> records a long time ago. But at the same time, I also mentioned that the path
> was passed as an arguement and we could always emit that...but we do not have
> any other information such as mode or security label.

I wonder what we log now in this case?

> In summary, certification does not say we need PARENT directories of the
> object. We need the object. And we only need help when its not clear what the
> object was.

-- 
paul moore
www.paul-moore.com

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Ondrej Mosnacek]

On Sat, Dec 1, 2018 at 5:50 PM Steve Grubb <sgrubb@redhat.com> wrote:
> On Tuesday, November 13, 2018 11:30:55 AM EST Paul Moore wrote:
> > On Tue, Nov 13, 2018 at 10:25 AM Ondrej Mosnacek <omosnace@redhat.com>
> wrote:
> > > On Tue, Nov 6, 2018 at 9:19 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Tue, Nov 6, 2018 at 3:09 AM Ondrej Mosnacek <omosnace@redhat.com>
> wrote:
> > > > > On Tue, Nov 6, 2018 at 12:30 AM Paul Moore <paul@paul-moore.com>
> wrote:
> > > > > > Let's reset this discussion a bit ... if we abolish relative paths
> > > > > > and make everything absolute, is there even a need to log PARENT?
>
> I believe that Al Viro has said that sometime paths are not resolvable in the
> future. For example process A opens a file. It passes the descriptor to
> another process using scm rights. Directory tree is deleted. Process B
> receives the descriptor. Process A exits. Process B is now accessing what?

Yes, this can happen, but even in cases where the path cannot be
determined any more, the PARENT records won't help you, because the
path in PARENT records is in fact based on the path of the child, the
code just cuts away the last component. See:
https://elixir.bootlin.com/linux/v4.19.6/source/kernel/auditsc.c#L1839

>
>
> > > > > If there ever was such need, then this won't change when we switch to
> > > > > absolute paths. The PATH records contain some fields (inode, dev,
> > > > > obj, that can be different for the child and parent and I would say
> > > > > these are the only new information that the PARENT records provide
> > > > > over the corresponding CREATE/DELETE records.
> > > >
> > > > Sigh.  Of course the inode information is going to be different
> > > > between the object in question and the parent, they are different
> > > > filesystem objects.  Ask your self the bigger question: does the
> > > > PARENT record provide me any security relevant information related to
> > > > the filesystem object that is being accessed?
> > >
> > > I would say it does. Consider e.g. the "mode" and "obj" fields. When
> > > you move (rename) a file from one directory to another (which is the
> > > main, if not the only, case when a PARENT record is emitted), then you
> > > are usually more interested in the values for the parent directory
> > > than the file itself (that's what determines if you can move the
> > > file).
> >
> > I disagree on the importance of the mode/obj of the parent in a rename
> > operation.  From my perspective I really only care about the
> > filesystem object that is being moved and if it succeeded or not.  The
> > idea that you care more about the parent than the object being moved
> > makes no sense to me at all.
>
> The mode is really not important.

All right, forget about the mode/context fields... I went in the wrong
direction there.

>
> > > For example, assume you have a rule that logs whenever some sensitive
> > > file gets moved. You do not expect that to happen because you set the
> > > file/directory permissions and labels so that it can't be done by
> > > anyone unauthorized. But something goes wrong, the permissions/labels
> > > get changed somehow ...
> >
> > In which case you should be watching for changes to the filesystem
> > metadata which affect access rights.  That is how you should catch
> > changes to permissions on a filesystem object as it gives you
> > information about the change as well as the subject information of the
> > user/process which made the change.
>
> Right. You would watch for attribute changes on a directory.
>
>
> > > ... and a bad actor leverages the situation to move
> > > the file. Then later you want to investigate this security incident
> > > and as part of it you want to know what permissions were set on the
> > > directories involved that had allowed the file to be moved, because
> > > this may give you a useful lead. With PARENT records, you get such
> > > information, without them you don't.
> >
> > If you only have that information in the parent record then you are
> > missing half the story, and it may be the important half as the
> > interesting bit of information in this example is the identity of the
> > user/process which was able to change permissions to enable the rename
> > to take place.
> >
> > Unless Steve provides evidence of some compelling certification
> > requirement which necessitates the need for a parent record, I see no
> > reason to keep it.
>
> Certification does not care about parent records. What is cares about is being
> able to say what the object was that is acted upon. So, that would be device
> and inode. But that is not nice for people. So, we would also like to know
> the path name. If parent record are necessary because of the *at syscalls,
> then that may be the only purpose. And they do not need to be emitted each
> time. If we also have a full path, then they are not needed. If we have a
> relative path, then CWD is needed, not the parent.

The PARENT records are only emitted with syscalls that operate with
directories (i.e. open(..., O_CREAT|...), unlink(), rename(), and
friends). In this case the object being modified is actually the
containing directory (you need write permission to the dir but you
don't any permissions to the file itself). Of course from a more high
level perspective the object being moved is equally (if not more)
important so to cover both perspectives we need to log the { inode,
device, path } info for both the object and its container. At least
that is how I (now) understand the primary purpose of the PARENT
record. I think the supposed usefulness in reconstructing the path to
the child is just a red herring...

>
> I also understand that at times full path resolution may not work out due to
> directory permissions blocking access at a deeper level of the directory
> tree. In those cases, we probably do want a PARENT record for the failed
> access attempt. I think that discussion may have prompted creation of PARENT
> records a long time ago. But at the same time, I also mentioned that the path
> was passed as an arguement and we could always emit that...but we do not have
> any other information such as mode or security label.
>
> In summary, certification does not say we need PARENT directories of the
> object. We need the object. And we only need help when its not clear what the
> object was.

But are you sure that 'the object' in the case of creating/deleting a
file is not also the containing directory? The parent directory is
being modified/written to and the child is being created/deleted so
logically we should log information about both objects. (That said, I
probably expect too much logic from certifications...)

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

Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths

[Paul Moore]

On Tue, Dec 4, 2018 at 3:07 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Sat, Dec 1, 2018 at 5:50 PM Steve Grubb <sgrubb@redhat.com> wrote:
> > On Tuesday, November 13, 2018 11:30:55 AM EST Paul Moore wrote:
> > > On Tue, Nov 13, 2018 at 10:25 AM Ondrej Mosnacek <omosnace@redhat.com>
> > wrote:
> > > > On Tue, Nov 6, 2018 at 9:19 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > > On Tue, Nov 6, 2018 at 3:09 AM Ondrej Mosnacek <omosnace@redhat.com>
> > wrote:
> > > > > > On Tue, Nov 6, 2018 at 12:30 AM Paul Moore <paul@paul-moore.com>
> > wrote:
> > > > > > > Let's reset this discussion a bit ... if we abolish relative paths
> > > > > > > and make everything absolute, is there even a need to log PARENT?
> >
> > I believe that Al Viro has said that sometime paths are not resolvable in the
> > future. For example process A opens a file. It passes the descriptor to
> > another process using scm rights. Directory tree is deleted. Process B
> > receives the descriptor. Process A exits. Process B is now accessing what?
>
> Yes, this can happen, but even in cases where the path cannot be
> determined any more, the PARENT records won't help you, because the
> path in PARENT records is in fact based on the path of the child, the
> code just cuts away the last component. See:
> https://elixir.bootlin.com/linux/v4.19.6/source/kernel/auditsc.c#L1839
>
> >
> >
> > > > > > If there ever was such need, then this won't change when we switch to
> > > > > > absolute paths. The PATH records contain some fields (inode, dev,
> > > > > > obj, that can be different for the child and parent and I would say
> > > > > > these are the only new information that the PARENT records provide
> > > > > > over the corresponding CREATE/DELETE records.
> > > > >
> > > > > Sigh.  Of course the inode information is going to be different
> > > > > between the object in question and the parent, they are different
> > > > > filesystem objects.  Ask your self the bigger question: does the
> > > > > PARENT record provide me any security relevant information related to
> > > > > the filesystem object that is being accessed?
> > > >
> > > > I would say it does. Consider e.g. the "mode" and "obj" fields. When
> > > > you move (rename) a file from one directory to another (which is the
> > > > main, if not the only, case when a PARENT record is emitted), then you
> > > > are usually more interested in the values for the parent directory
> > > > than the file itself (that's what determines if you can move the
> > > > file).
> > >
> > > I disagree on the importance of the mode/obj of the parent in a rename
> > > operation.  From my perspective I really only care about the
> > > filesystem object that is being moved and if it succeeded or not.  The
> > > idea that you care more about the parent than the object being moved
> > > makes no sense to me at all.
> >
> > The mode is really not important.
>
> All right, forget about the mode/context fields... I went in the wrong
> direction there.
>
> >
> > > > For example, assume you have a rule that logs whenever some sensitive
> > > > file gets moved. You do not expect that to happen because you set the
> > > > file/directory permissions and labels so that it can't be done by
> > > > anyone unauthorized. But something goes wrong, the permissions/labels
> > > > get changed somehow ...
> > >
> > > In which case you should be watching for changes to the filesystem
> > > metadata which affect access rights.  That is how you should catch
> > > changes to permissions on a filesystem object as it gives you
> > > information about the change as well as the subject information of the
> > > user/process which made the change.
> >
> > Right. You would watch for attribute changes on a directory.
> >
> >
> > > > ... and a bad actor leverages the situation to move
> > > > the file. Then later you want to investigate this security incident
> > > > and as part of it you want to know what permissions were set on the
> > > > directories involved that had allowed the file to be moved, because
> > > > this may give you a useful lead. With PARENT records, you get such
> > > > information, without them you don't.
> > >
> > > If you only have that information in the parent record then you are
> > > missing half the story, and it may be the important half as the
> > > interesting bit of information in this example is the identity of the
> > > user/process which was able to change permissions to enable the rename
> > > to take place.
> > >
> > > Unless Steve provides evidence of some compelling certification
> > > requirement which necessitates the need for a parent record, I see no
> > > reason to keep it.
> >
> > Certification does not care about parent records. What is cares about is being
> > able to say what the object was that is acted upon. So, that would be device
> > and inode. But that is not nice for people. So, we would also like to know
> > the path name. If parent record are necessary because of the *at syscalls,
> > then that may be the only purpose. And they do not need to be emitted each
> > time. If we also have a full path, then they are not needed. If we have a
> > relative path, then CWD is needed, not the parent.
>
> The PARENT records are only emitted with syscalls that operate with
> directories (i.e. open(..., O_CREAT|...), unlink(), rename(), and
> friends). In this case the object being modified is actually the
> containing directory (you need write permission to the dir but you
> don't any permissions to the file itself). Of course from a more high
> level perspective the object being moved is equally (if not more)
> important so to cover both perspectives we need to log the { inode,
> device, path } info for both the object and its container. At least
> that is how I (now) understand the primary purpose of the PARENT
> record.

While you are correct, moving a file does result in changes to the
directory file, the directory file is generally uninteresting as it
contains no user data, just references to the files contained in the
directory and in this particular case we already have that information
as we are logging the actual file move.

> ... I think the supposed usefulness in reconstructing the path to
> the child is just a red herring...

-- 
paul moore
www.paul-moore.com