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

[Ondrej Mosnacek]

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

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

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

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

Fixes: 9c937dcc7102 ("[PATCH] log more info for directory entry change events")
Signed-off-by: Ondrej Mosnacek <***@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"

[Richard Guy Briggs]

Post by Ondrej Mosnacek
When a relative path has just a single component and we want to emit a
nametype=PARENT record, the current implementation just reports the full
CWD path (which is alrady available in the audit context).
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.

Post by Ondrej Mosnacek
Fixes: 9c937dcc7102 ("[PATCH] log more info for directory entry change events")
---
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) {
/* log the full path */
- audit_log_format(ab, " name=");
audit_log_untrustedstring(ab, n->name->name);
break;
/* 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;
/* 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 <***@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

[Paul Moore]

Post by Ondrej Mosnacek
When a relative path has just a single component and we want to emit a
nametype=PARENT record, the current implementation just reports the full
CWD path (which is alrady available in the audit context).
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")
---
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) {
/* log the full path */
- audit_log_format(ab, " name=");
audit_log_untrustedstring(ab, n->name->name);
break;
/* 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

[Ondrej Mosnacek]

Post by Paul Moore

Post by Ondrej Mosnacek
When a relative path has just a single component and we want to emit a
nametype=PARENT record, the current implementation just reports the full
CWD path (which is alrady available in the audit context).
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")
---
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) {
/* log the full path */
- audit_log_format(ab, " name=");
audit_log_untrustedstring(ab, n->name->name);
break;
/* 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.

Post by Paul Moore
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.

[Paul Moore]

Post by Ondrej Mosnacek

Post by Paul Moore

Post by Ondrej Mosnacek
When a relative path has just a single component and we want to emit a
nametype=PARENT record, the current implementation just reports the full
CWD path (which is alrady available in the audit context).
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")
---
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) {
/* log the full path */
- audit_log_format(ab, " name=");
audit_log_untrustedstring(ab, n->name->name);
break;
/* 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.

Post by Ondrej Mosnacek

Post by Paul Moore
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.

[Ondrej Mosnacek]

Post by Paul Moore

Post by Ondrej Mosnacek

Post by Paul Moore

Post by Ondrej Mosnacek
When a relative path has just a single component and we want to emit a
nametype=PARENT record, the current implementation just reports the full
CWD path (which is alrady available in the audit context).
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")
---
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) {
/* log the full path */
- audit_log_format(ab, " name=");
audit_log_untrustedstring(ab, n->name->name);
break;
/* 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, "\".\"");

?

Post by Paul Moore

Post by Ondrej Mosnacek

Post by Paul Moore
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]

Post by Ondrej Mosnacek

Post by Paul Moore

Post by Ondrej Mosnacek

Post by Paul Moore

Post by Ondrej Mosnacek
When a relative path has just a single component and we want to emit a
nametype=PARENT record, the current implementation just reports the full
CWD path (which is alrady available in the audit context).
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")
---
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) {
/* log the full path */
- audit_log_format(ab, " name=");
audit_log_untrustedstring(ab, n->name->name);
break;
/* 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.

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

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

Post by Ondrej Mosnacek

Post by Paul Moore

Post by Ondrej Mosnacek

Post by Paul Moore
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.

[Paul Moore]

Post by Ondrej Mosnacek
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]

Post by Ondrej Mosnacek
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?

[Ondrej Mosnacek]

Post by Paul Moore

Post by Ondrej Mosnacek
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

Post by Paul Moore

Post by Ondrej Mosnacek

Post by Paul Moore

Post by Ondrej Mosnacek
/* 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.

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

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

[Paul Moore]

Post by Ondrej Mosnacek

Post by Paul Moore

Post by Ondrej Mosnacek
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.

Post by Ondrej Mosnacek
The info in the CWD record is correct, but it is being abused in
audit_log_name().)
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
.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
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.
1. A userspace process calls rename("file1", "file2") with CWD set to
"/home/user".
.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
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.

Post by Ondrej Mosnacek
1. A userspace process calls the following syscalls (with CWD set to
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
.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
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.

Post by Ondrej Mosnacek
So far so good, but now we are coming to...
1. A userspace process calls the following syscalls (with CWD set to
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
.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
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).

Post by Ondrej Mosnacek
BTW, you still didn't answer my brief question from a few replies

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.

[Ondrej Mosnacek]

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

Post by Paul Moore

Post by Ondrej Mosnacek

Post by Paul Moore

Post by Ondrej Mosnacek
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.

Post by Ondrej Mosnacek
The info in the CWD record is correct, but it is being abused in
audit_log_name().)
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
.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
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.
1. A userspace process calls rename("file1", "file2") with CWD set to
"/home/user".
.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
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 :)

Post by Paul Moore

Post by Ondrej Mosnacek
1. A userspace process calls the following syscalls (with CWD set to
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
.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
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.

Post by Ondrej Mosnacek
So far so good, but now we are coming to...
1. A userspace process calls the following syscalls (with CWD set to
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
.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
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...

Post by Paul Moore

Post by Ondrej Mosnacek
BTW, you still didn't answer my brief question from a few replies

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 :)

Post by Paul Moore
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.

[Paul Moore]

Post by Ondrej Mosnacek

Post by Paul Moore

Post by Ondrej Mosnacek

Post by Paul Moore

Post by Ondrej Mosnacek
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.

Post by Ondrej Mosnacek
The info in the CWD record is correct, but it is being abused in
audit_log_name().)
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
.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
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.
1. A userspace process calls rename("file1", "file2") with CWD set to
"/home/user".
.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
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 :)

Post by Paul Moore

Post by Ondrej Mosnacek
1. A userspace process calls the following syscalls (with CWD set to
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
.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
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.

Post by Ondrej Mosnacek
So far so good, but now we are coming to...
1. A userspace process calls the following syscalls (with CWD set to
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
.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
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
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?

[Ondrej Mosnacek]

Post by Paul Moore

Post by Ondrej Mosnacek

Post by Paul Moore

Post by Ondrej Mosnacek

Post by Paul Moore

Post by Ondrej Mosnacek
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.

Post by Ondrej Mosnacek
The info in the CWD record is correct, but it is being abused in
audit_log_name().)
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
.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
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.
1. A userspace process calls rename("file1", "file2") with CWD set to
"/home/user".
.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
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 :)

Post by Paul Moore

Post by Ondrej Mosnacek
1. A userspace process calls the following syscalls (with CWD set to
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
.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
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.

Post by Ondrej Mosnacek
So far so good, but now we are coming to...
1. A userspace process calls the following syscalls (with CWD set to
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
.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
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
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.

[Paul Moore]

Post by Ondrej Mosnacek

Post by Paul Moore
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)?

[Ondrej Mosnacek]

Post by Paul Moore

Post by Ondrej Mosnacek

Post by Paul Moore
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.

Post by Paul Moore
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.

[Paul Moore]

Post by Ondrej Mosnacek

Post by Paul Moore

Post by Ondrej Mosnacek

Post by Paul Moore
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.

Post by Ondrej Mosnacek
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.

Post by Ondrej Mosnacek
... 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.

[Steve Grubb]

Post by Paul Moore

Post by Ondrej Mosnacek

Post by Paul Moore

Post by Ondrej Mosnacek

Post by Paul Moore
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?

Post by Paul Moore

Post by Ondrej Mosnacek

Post by Paul Moore

Post by Ondrej Mosnacek
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.

Post by Paul Moore

Post by Ondrej Mosnacek
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.

Post by Paul Moore

Post by Ondrej Mosnacek
... 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

[Paul Moore]

Post by Steve Grubb

Post by Paul Moore

Post by Ondrej Mosnacek

Post by Paul Moore

Post by Ondrej Mosnacek

Post by Paul Moore
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.

Post by Steve Grubb

Post by Paul Moore

Post by Ondrej Mosnacek

Post by Paul Moore

Post by Ondrej Mosnacek
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.

Post by Paul Moore

Post by Ondrej Mosnacek
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.

Post by Paul Moore

Post by Ondrej Mosnacek
... 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 ...

Post by Steve Grubb
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.

Post by Steve Grubb
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?

Post by Steve Grubb
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]

Post by Ondrej Mosnacek
When a relative path has just a single component and we want to emit a
nametype=PARENT record, the current implementation just reports the full
CWD path (which is alrady available in the audit context).
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?

Post by Ondrej Mosnacek
See: https://github.com/linux-audit/audit-kernel/issues/95
Fixes: 9c937dcc7102 ("[PATCH] log more info for directory entry change events")
---
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) {
/* log the full path */
- audit_log_format(ab, " name=");
audit_log_untrustedstring(ab, n->name->name);
break;
/* 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;
/* 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]

Post by Paul Moore

Post by Ondrej Mosnacek
When a relative path has just a single component and we want to emit a
nametype=PARENT record, the current implementation just reports the full
CWD path (which is alrady available in the audit context).
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.

Post by Paul Moore

Post by Ondrej Mosnacek
See: https://github.com/linux-audit/audit-kernel/issues/95
Fixes: 9c937dcc7102 ("[PATCH] log more info for directory entry change events")
---
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) {
/* log the full path */
- audit_log_format(ab, " name=");
audit_log_untrustedstring(ab, n->name->name);
break;
/* 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;
/* 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

[Steve Grubb]

Post by Paul Moore

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

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.

Post by Paul Moore

Post by Ondrej Mosnacek
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.

Post by Paul Moore

Post by Ondrej Mosnacek
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.

Post by Paul Moore

Post by Ondrej Mosnacek
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.

Post by Paul Moore
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]

Post by Steve Grubb

Post by Paul Moore

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

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".

Post by Steve Grubb

Post by Paul Moore

Post by Ondrej Mosnacek
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.

Post by Paul Moore

Post by Ondrej Mosnacek
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.

Post by Steve Grubb

Post by Paul Moore

Post by Ondrej Mosnacek
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.

Post by Paul Moore
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.

[Ondrej Mosnacek]

Post by Ondrej Mosnacek
When a relative path has just a single component and we want to emit a
nametype=PARENT record, the current implementation just reports the full
CWD path (which is alrady available in the audit context).
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.

Post by Ondrej Mosnacek
See: https://github.com/linux-audit/audit-kernel/issues/95
Fixes: 9c937dcc7102 ("[PATCH] log more info for directory entry change events")
---
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) {
/* log the full path */
- audit_log_format(ab, " name=");
audit_log_untrustedstring(ab, n->name->name);
break;
/* 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;
/* 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.

[Steve Grubb]

Post by Ondrej Mosnacek

Post by Ondrej Mosnacek
When a relative path has just a single component and we want to emit a
nametype=PARENT record, the current implementation just reports the full
CWD path (which is alrady available in the audit context).
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

Post by Ondrej Mosnacek

Post by Ondrej Mosnacek
See: https://github.com/linux-audit/audit-kernel/issues/95
Fixes: 9c937dcc7102 ("[PATCH] log more info for directory entry change
---
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) {
/* log the full path */
- audit_log_format(ab, " name=");
audit_log_untrustedstring(ab, n->name->name);
break;
/* 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;
/* 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.