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

On Wed, Sep 19, 2018 at 7:01 AM Ondrej Mosnacek <omosnace(a)redhat.com&gt; wrote: > On Wed, Sep 19, 2018 at 3:35 AM Paul Moore <paul(a)paul-moore.com&gt; wrote: > > On Thu, Sep 13, 2018 at 10:13 AM Paul Moore <paul(a)paul-moore.com&gt; wrote: > > > On Thu, Sep 13, 2018 at 9:58 AM Ondrej Mosnacek <omosnace(a)redhat.com&gt; 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--