* [PATCH ghak95] audit: Do not log full CWD path on empty relative paths @ 2018-08-02 11:44 Ondrej Mosnacek 2018-08-02 13:29 ` Richard Guy Briggs ` (3 more replies) 0 siblings, 4 replies; 27+ messages in thread From: Ondrej Mosnacek @ 2018-08-02 11:44 UTC (permalink / raw) To: linux-audit; +Cc: Richard Guy Briggs 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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-08-02 11:44 [PATCH ghak95] audit: Do not log full CWD path on empty relative paths Ondrej Mosnacek @ 2018-08-02 13:29 ` Richard Guy Briggs 2018-08-02 22:24 ` Paul Moore ` (2 subsequent siblings) 3 siblings, 0 replies; 27+ messages in thread From: Richard Guy Briggs @ 2018-08-02 13:29 UTC (permalink / raw) To: Ondrej Mosnacek; +Cc: linux-audit 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-08-02 11:44 [PATCH ghak95] audit: Do not log full CWD path on empty relative paths Ondrej Mosnacek 2018-08-02 13:29 ` Richard Guy Briggs @ 2018-08-02 22:24 ` Paul Moore 2018-08-03 7:08 ` Ondrej Mosnacek 2018-08-03 0:03 ` Paul Moore 2018-08-24 12:59 ` Ondrej Mosnacek 3 siblings, 1 reply; 27+ messages in thread From: Paul Moore @ 2018-08-02 22:24 UTC (permalink / raw) To: omosnace; +Cc: rgb, linux-audit 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-08-02 22:24 ` Paul Moore @ 2018-08-03 7:08 ` Ondrej Mosnacek 2018-08-24 14:09 ` Paul Moore 0 siblings, 1 reply; 27+ messages in thread From: Ondrej Mosnacek @ 2018-08-03 7:08 UTC (permalink / raw) To: Paul Moore; +Cc: Richard Guy Briggs, Linux-Audit Mailing List 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-08-03 7:08 ` Ondrej Mosnacek @ 2018-08-24 14:09 ` Paul Moore 2018-08-27 13:00 ` Ondrej Mosnacek 0 siblings, 1 reply; 27+ messages in thread From: Paul Moore @ 2018-08-24 14:09 UTC (permalink / raw) To: omosnace; +Cc: rgb, linux-audit 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-08-24 14:09 ` Paul Moore @ 2018-08-27 13:00 ` Ondrej Mosnacek 2018-09-13 13:57 ` Ondrej Mosnacek 0 siblings, 1 reply; 27+ messages in thread From: Ondrej Mosnacek @ 2018-08-27 13:00 UTC (permalink / raw) To: Paul Moore; +Cc: Richard Guy Briggs, Linux-Audit Mailing List 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-08-27 13:00 ` Ondrej Mosnacek @ 2018-09-13 13:57 ` Ondrej Mosnacek 2018-09-13 14:13 ` Paul Moore 0 siblings, 1 reply; 27+ messages in thread From: Ondrej Mosnacek @ 2018-09-13 13:57 UTC (permalink / raw) To: Paul Moore; +Cc: Richard Guy Briggs, Linux-Audit Mailing List 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-09-13 13:57 ` Ondrej Mosnacek @ 2018-09-13 14:13 ` Paul Moore 2018-09-19 1:35 ` Paul Moore 0 siblings, 1 reply; 27+ messages in thread From: Paul Moore @ 2018-09-13 14:13 UTC (permalink / raw) To: omosnace; +Cc: rgb, linux-audit 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-09-13 14:13 ` Paul Moore @ 2018-09-19 1:35 ` Paul Moore 2018-09-19 11:01 ` Ondrej Mosnacek 0 siblings, 1 reply; 27+ messages in thread From: Paul Moore @ 2018-09-19 1:35 UTC (permalink / raw) To: omosnace; +Cc: rgb, linux-audit 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-09-19 1:35 ` Paul Moore @ 2018-09-19 11:01 ` Ondrej Mosnacek 2018-09-19 15:44 ` Paul Moore 0 siblings, 1 reply; 27+ messages in thread From: Ondrej Mosnacek @ 2018-09-19 11:01 UTC (permalink / raw) To: Paul Moore; +Cc: Richard Guy Briggs, Linux-Audit Mailing List 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-09-19 11:01 ` Ondrej Mosnacek @ 2018-09-19 15:44 ` Paul Moore 2018-10-31 8:54 ` Ondrej Mosnacek 0 siblings, 1 reply; 27+ messages in thread From: Paul Moore @ 2018-09-19 15:44 UTC (permalink / raw) To: omosnace; +Cc: rgb, linux-audit 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-09-19 15:44 ` Paul Moore @ 2018-10-31 8:54 ` Ondrej Mosnacek 2018-11-05 23:30 ` Paul Moore 0 siblings, 1 reply; 27+ messages in thread From: Ondrej Mosnacek @ 2018-10-31 8:54 UTC (permalink / raw) To: Paul Moore; +Cc: Linux-Audit Mailing List 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-10-31 8:54 ` Ondrej Mosnacek @ 2018-11-05 23:30 ` Paul Moore 2018-11-06 8:08 ` Ondrej Mosnacek 0 siblings, 1 reply; 27+ messages in thread From: Paul Moore @ 2018-11-05 23:30 UTC (permalink / raw) To: omosnace; +Cc: linux-audit 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-11-05 23:30 ` Paul Moore @ 2018-11-06 8:08 ` Ondrej Mosnacek 2018-11-06 20:19 ` Paul Moore 0 siblings, 1 reply; 27+ messages in thread From: Ondrej Mosnacek @ 2018-11-06 8:08 UTC (permalink / raw) To: Paul Moore; +Cc: Linux-Audit Mailing List 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-11-06 8:08 ` Ondrej Mosnacek @ 2018-11-06 20:19 ` Paul Moore 2018-11-13 15:25 ` Ondrej Mosnacek 0 siblings, 1 reply; 27+ messages in thread From: Paul Moore @ 2018-11-06 20:19 UTC (permalink / raw) To: omosnace; +Cc: linux-audit 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-11-06 20:19 ` Paul Moore @ 2018-11-13 15:25 ` Ondrej Mosnacek 2018-11-13 16:30 ` Paul Moore 0 siblings, 1 reply; 27+ messages in thread From: Ondrej Mosnacek @ 2018-11-13 15:25 UTC (permalink / raw) To: Paul Moore; +Cc: Linux-Audit Mailing List 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-11-13 15:25 ` Ondrej Mosnacek @ 2018-11-13 16:30 ` Paul Moore 2018-12-01 16:50 ` Steve Grubb 0 siblings, 1 reply; 27+ messages in thread From: Paul Moore @ 2018-11-13 16:30 UTC (permalink / raw) To: omosnace; +Cc: linux-audit 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-11-13 16:30 ` Paul Moore @ 2018-12-01 16:50 ` Steve Grubb 2018-12-04 0:17 ` Paul Moore 2018-12-04 8:07 ` Ondrej Mosnacek 0 siblings, 2 replies; 27+ messages in thread From: Steve Grubb @ 2018-12-01 16:50 UTC (permalink / raw) To: Paul Moore; +Cc: rgb, linux-audit 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-12-01 16:50 ` Steve Grubb @ 2018-12-04 0:17 ` Paul Moore 2018-12-04 8:07 ` Ondrej Mosnacek 1 sibling, 0 replies; 27+ messages in thread From: Paul Moore @ 2018-12-04 0:17 UTC (permalink / raw) To: sgrubb, omosnace; +Cc: rgb, linux-audit 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-12-01 16:50 ` Steve Grubb 2018-12-04 0:17 ` Paul Moore @ 2018-12-04 8:07 ` Ondrej Mosnacek 2018-12-04 22:19 ` Paul Moore 1 sibling, 1 reply; 27+ messages in thread From: Ondrej Mosnacek @ 2018-12-04 8:07 UTC (permalink / raw) To: Steve Grubb; +Cc: Richard Guy Briggs, Linux-Audit Mailing List 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-12-04 8:07 ` Ondrej Mosnacek @ 2018-12-04 22:19 ` Paul Moore 0 siblings, 0 replies; 27+ messages in thread From: Paul Moore @ 2018-12-04 22:19 UTC (permalink / raw) To: omosnace; +Cc: rgb, linux-audit 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-08-02 11:44 [PATCH ghak95] audit: Do not log full CWD path on empty relative paths Ondrej Mosnacek 2018-08-02 13:29 ` Richard Guy Briggs 2018-08-02 22:24 ` Paul Moore @ 2018-08-03 0:03 ` Paul Moore 2018-08-24 15:00 ` Paul Moore 2018-08-24 12:59 ` Ondrej Mosnacek 3 siblings, 1 reply; 27+ messages in thread From: Paul Moore @ 2018-08-03 0:03 UTC (permalink / raw) To: omosnace; +Cc: rgb, linux-audit 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-08-03 0:03 ` Paul Moore @ 2018-08-24 15:00 ` Paul Moore 2018-08-24 15:14 ` Steve Grubb 0 siblings, 1 reply; 27+ messages in thread From: Paul Moore @ 2018-08-24 15:00 UTC (permalink / raw) To: omosnace; +Cc: rgb, linux-audit 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-08-24 15:00 ` Paul Moore @ 2018-08-24 15:14 ` Steve Grubb 2018-08-27 12:42 ` Ondrej Mosnacek 0 siblings, 1 reply; 27+ messages in thread From: Steve Grubb @ 2018-08-24 15:14 UTC (permalink / raw) To: Paul Moore; +Cc: rgb, linux-audit 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-08-24 15:14 ` Steve Grubb @ 2018-08-27 12:42 ` Ondrej Mosnacek 0 siblings, 0 replies; 27+ messages in thread From: Ondrej Mosnacek @ 2018-08-27 12:42 UTC (permalink / raw) To: Steve Grubb; +Cc: Richard Guy Briggs, Linux-Audit Mailing List 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-08-02 11:44 [PATCH ghak95] audit: Do not log full CWD path on empty relative paths Ondrej Mosnacek ` (2 preceding siblings ...) 2018-08-03 0:03 ` Paul Moore @ 2018-08-24 12:59 ` Ondrej Mosnacek 2018-08-24 14:28 ` Steve Grubb 3 siblings, 1 reply; 27+ messages in thread From: Ondrej Mosnacek @ 2018-08-24 12:59 UTC (permalink / raw) To: Linux-Audit Mailing List; +Cc: Richard Guy Briggs 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths 2018-08-24 12:59 ` Ondrej Mosnacek @ 2018-08-24 14:28 ` Steve Grubb 0 siblings, 0 replies; 27+ messages in thread From: Steve Grubb @ 2018-08-24 14:28 UTC (permalink / raw) To: Ondrej Mosnacek; +Cc: Richard Guy Briggs, Linux-Audit Mailing List 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2018-12-04 22:19 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-02 11:44 [PATCH ghak95] audit: Do not log full CWD path on empty relative paths Ondrej Mosnacek 2018-08-02 13:29 ` Richard Guy Briggs 2018-08-02 22:24 ` Paul Moore 2018-08-03 7:08 ` Ondrej Mosnacek 2018-08-24 14:09 ` Paul Moore 2018-08-27 13:00 ` Ondrej Mosnacek 2018-09-13 13:57 ` Ondrej Mosnacek 2018-09-13 14:13 ` Paul Moore 2018-09-19 1:35 ` Paul Moore 2018-09-19 11:01 ` Ondrej Mosnacek 2018-09-19 15:44 ` Paul Moore 2018-10-31 8:54 ` Ondrej Mosnacek 2018-11-05 23:30 ` Paul Moore 2018-11-06 8:08 ` Ondrej Mosnacek 2018-11-06 20:19 ` Paul Moore 2018-11-13 15:25 ` Ondrej Mosnacek 2018-11-13 16:30 ` Paul Moore 2018-12-01 16:50 ` Steve Grubb 2018-12-04 0:17 ` Paul Moore 2018-12-04 8:07 ` Ondrej Mosnacek 2018-12-04 22:19 ` Paul Moore 2018-08-03 0:03 ` Paul Moore 2018-08-24 15:00 ` Paul Moore 2018-08-24 15:14 ` Steve Grubb 2018-08-27 12:42 ` Ondrej Mosnacek 2018-08-24 12:59 ` Ondrej Mosnacek 2018-08-24 14:28 ` Steve Grubb
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.