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