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

[Paul Moore]

On Wed, Oct 31, 2018 at 4:54 AM Ondrej Mosnacek <omosnace at redhat.com> wrote:
> On Wed, Sep 19, 2018 at 5:44 PM Paul Moore <paul at paul-moore.com> wrote:
> > On Wed, Sep 19, 2018 at 7:01 AM Ondrej Mosnacek <omosnace at redhat.com> wrote:
> > > On Wed, Sep 19, 2018 at 3:35 AM Paul Moore <paul at paul-moore.com> wrote:
> > > > On Thu, Sep 13, 2018 at 10:13 AM Paul Moore <paul at paul-moore.com> wrote:
> > > > > On Thu, Sep 13, 2018 at 9:58 AM Ondrej Mosnacek <omosnace at 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.
>
> 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"
> [...]

Let's reset this discussion a bit ... if we abolish relative paths and
make everything absolute, is there even a need to log PARENT?

-- 
paul moore
www.paul-moore.com