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

On Wed, Sep 19, 2018 at 5:44 PM Paul Moore <paul(a)paul-moore.com&gt; wrote: > 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. Wait, no... I meant it is surprising that there is suddenly a full path to directory ("/home/user") instead of a relative one (which would be "." in this case). This happens if and only if the original relative path has just a single component, which is a strange condition for anyone who doesn't know how the audit_log_name() function is implemented. The fact that the PARENT record has a path to the parent si obviously logical and sound, I have no problem with that :) > > > EXAMPLE #3 (The non-edge renameat(2) case): > > 1. A userspace process calls the following syscalls (with CWD set to > > "/home/user"): > > int srcfd = open("/some/path1", O_DIRECTORY | O_PATH); > > int dstfd = open("/another/path2", O_DIRECTORY | O_PATH); > > renameat(srcfd, "dir1/file1", dstfd, "dir2/file2"); > > 2. The 'name', 'type' and 'name_len' fields of the 'struct > > audit_names' objects will now be exactly the same as in EXAMPLE #1: > > .name = "dir1/file1", .type = AUDIT_TYPE_PARENT, .name_len = 5 > > .name = "dir1/file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL > > .name = "dir2/file2", .type = AUDIT_TYPE_PARENT, .name_len = 5 > > .name = "dir2/file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL > > 3. The type and name information in the PATH records will be also > > exactly the same: > > nametype=PARENT name="dir1/" > > nametype=DELETE name="dir1/file1" > > nametype=PARENT name="dir2/" > > nametype=CREATE name="dir2/file2" > > > > Even in this case the information in the records is correct (although > > there is no information telling us that "dir1/..." actually > > corresponds to "/some/path1/dir1/..." and "dir2/..." actually > > corresponds to "/another/path2/dir2/..."). > > Yeah, I'm starting to think we should always log the absolute path in > the PARENT record. > > > So far so good, but now we are coming to... > > > > EXAMPLE #4 (The single-path-component renameat(2) case): > > 1. A userspace process calls the following syscalls (with CWD set to > > "/home/user"): > > int srcfd = open("/some/path1", O_DIRECTORY | O_PATH); > > int dstfd = open("/another/path2", O_DIRECTORY | O_PATH); > > renameat(srcfd, "file1", dstfd, "file2"); > > 2. The 'name', 'type' and 'name_len' fields of the 'struct > > audit_names' objects will now be exactly the same as in EXAMPLE #2: > > .name = "file1", .type = AUDIT_TYPE_PARENT, .name_len = 0 > > .name = "file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL > > .name = "file2", .type = AUDIT_TYPE_PARENT, .name_len = 0 > > .name = "file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL > > 3. The type and name information in the PATH records will be also > > exactly the same: > > nametype=PARENT name="/home/user" > > nametype=DELETE name="file1" > > nametype=PARENT name="/home/user" > > nametype=CREATE name="file2" > > > > ...and HERE is the problem. The parent of "file1" is not "/home/user", > > it is "/some/path1", and the parent of "file2" is also not > > "/home/user", it is "/another/path2". > > > > The proposed fix simply changes the handling of the name_len == 0 case > > to output "." instead of the CWD. This doesn't solve the wider problem > > that we don't have the dirfd path information (GHAK #9), but it at > > least makes the situation in EXAMPLE #4 *not worse* than in EXAMPLE #3 > > (i.e. it fixes the less demanding GHAK #95). As an additional minor > > benefit it also brings a bit more consistency - with it the PATH > > records will contain relative (resp. absolute) paths if and only if > > the corresponding path given to the syscall was relative (resp. > > absolute). > > > > I hope this finally clears things up. > > Yes, it does, thanks. My apologies for getting tangled up in the CWD logging. > > My current thinking is that logging relative paths in the > nametype=PARENT PATH record is a mistake. Yes, I understand there are > some cases where this information will be the same as the CWD > information, and that could add some additional log overhead, but as > tricky as path resolution can be I think this is the safest option and > the one I would like to see us pursue. This will likely require some > extra work for the *at() syscalls, but those aren't reported correctly > right now as discussed here and elsewhere. > > I would expect the non-PARENT PATH records to stay as they are, in > other words this should only affect things which are *not* (.name_len > == AUDIT_NAME_FULL). Well, logging (correct) absolute path in *all* PATH records would certainly solve the problem (and also GHAK #9) and considering all the problems with relative paths it might even be the most reasonable solution. However, doing so only in the case of PARENT records doesn't seem like a good idea to me... Consider the first two arguments of a renameat(2) syscall with olddirfd pointing to "/some/dir" and an oldpath of "subdir/file". We would get PATH records like this: nametype=PARENT path="/some/dir/subdir" nametype=DELETE path="subdir/file" [...]