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

On Wed, Sep 19, 2018 at 3:35 AM Paul Moore <paul(a)paul-moore.com&gt; wrote: Sorry, but I think you are completely misunderstanding the problem... I tried to explain it several times in different ways, but apparently I'm still not doing it right... First of all, there is no "nametype=CWD" PATH record. There is only the classic CWD record that is associated to every syscall and I don't touch that one at all. The information in the CWD record is perfectly fine. Let me try to demonstrate it with some more verbose examples. (TL;DR: The info in the CWD record is correct, but it is being abused in audit_log_name().) EXAMPLE #1 (The non-edge case): 1. A userspace process calls rename("dir1/file1", "dir2/file2") with CWD set to "/home/user". 2. The syscall causes four calls to __audit_inode(), which generate four 'struct audit_names' objects with the following information (maybe not in this specific order, but that doesn't matter): .name = "dir1/file1", .type = AUDIT_TYPE_PARENT, .name_len = 5 .name = "dir1/file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL .name = "dir2/file2", .type = AUDIT_TYPE_PARENT, .name_len = 5 .name = "dir2/file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL 3. At the end of the syscall, audit_log_name() is called on each of these objects and produces the following PATH records (simplifed): nametype=PARENT name="dir1/" nametype=DELETE name="dir1/file1" nametype=PARENT name="dir2/" nametype=CREATE name="dir2/file2" Notice that for the PARENT objects the .name_len is truncated to only the directory component. EXAMPLE #2 (The single-path-component case): 1. A userspace process calls rename("file1", "file2") with CWD set to "/home/user". 2. The 'struct audit_names' objects will now be: .name = "file1", .type = AUDIT_TYPE_PARENT, .name_len = 0 .name = "file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL .name = "file2", .type = AUDIT_TYPE_PARENT, .name_len = 0 .name = "file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL 3. At the end of the syscall, audit_log_name() is called on each of these objects and produces the following PATH records (simplifed): nametype=PARENT name="/home/user" nametype=DELETE name="file1" nametype=PARENT name="/home/user" nametype=CREATE name="file2" Notice that in this case, the "clever" logic in audit_log_name() wanted to avoid logging an empty path (name="") in the PARENT records, so it instead put the CWD path in there ("/home/user"). In this case this is perfectly valid (although could be a bit surprising that there is suddenly a full path instead of a relative one), since the full path of "file1" is actually "/home/user/file1". EXAMPLE #3 (The non-edge renameat(2) case): 1. A userspace process calls the following syscalls (with CWD set to "/home/user"): int srcfd = open("/some/path1", O_DIRECTORY | O_PATH); int dstfd = open("/another/path2", O_DIRECTORY | O_PATH); renameat(srcfd, "dir1/file1", dstfd, "dir2/file2"); 2. The 'name', 'type' and 'name_len' fields of the 'struct audit_names' objects will now be exactly the same as in EXAMPLE #1: .name = "dir1/file1", .type = AUDIT_TYPE_PARENT, .name_len = 5 .name = "dir1/file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL .name = "dir2/file2", .type = AUDIT_TYPE_PARENT, .name_len = 5 .name = "dir2/file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL 3. The type and name information in the PATH records will be also exactly the same: nametype=PARENT name="dir1/" nametype=DELETE name="dir1/file1" nametype=PARENT name="dir2/" nametype=CREATE name="dir2/file2" Even in this case the information in the records is correct (although there is no information telling us that "dir1/..." actually corresponds to "/some/path1/dir1/..." and "dir2/..." actually corresponds to "/another/path2/dir2/..."). So far so good, but now we are coming to... EXAMPLE #4 (The single-path-component renameat(2) case): 1. A userspace process calls the following syscalls (with CWD set to "/home/user"): int srcfd = open("/some/path1", O_DIRECTORY | O_PATH); int dstfd = open("/another/path2", O_DIRECTORY | O_PATH); renameat(srcfd, "file1", dstfd, "file2"); 2. The 'name', 'type' and 'name_len' fields of the 'struct audit_names' objects will now be exactly the same as in EXAMPLE #2: .name = "file1", .type = AUDIT_TYPE_PARENT, .name_len = 0 .name = "file1", .type = AUDIT_TYPE_DELETE, .name_len = AUDIT_NAME_FULL .name = "file2", .type = AUDIT_TYPE_PARENT, .name_len = 0 .name = "file2", .type = AUDIT_TYPE_CREATE, .name_len = AUDIT_NAME_FULL 3. The type and name information in the PATH records will be also exactly the same: nametype=PARENT name="/home/user" nametype=DELETE name="file1" nametype=PARENT name="/home/user" nametype=CREATE name="file2" ...and HERE is the problem. The parent of "file1" is not "/home/user", it is "/some/path1", and the parent of "file2" is also not "/home/user", it is "/another/path2". The proposed fix simply changes the handling of the name_len == 0 case to output "." instead of the CWD. This doesn't solve the wider problem that we don't have the dirfd path information (GHAK #9), but it at least makes the situation in EXAMPLE #4 *not worse* than in EXAMPLE #3 (i.e. it fixes the less demanding GHAK #95). As an additional minor benefit it also brings a bit more consistency - with it the PATH records will contain relative (resp. absolute) paths if and only if the corresponding path given to the syscall was relative (resp. absolute). I hope this finally clears things up. BTW, you still didn't answer my brief question from a few replies above (quoted below with context): Thanks, -- Ondrej Mosnacek <omosnace at redhat dot com> Associate Software Engineer, Security Technologies Red Hat, Inc.