From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ondrej Mosnacek Subject: Re: [PATCH ghak95] audit: Do not log full CWD path on empty relative paths Date: Wed, 31 Oct 2018 09:54:05 +0100 Message-ID: References: <20180802114436.1209-1-omosnace@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx1.redhat.com (ext-mx10.extmail.prod.ext.phx2.redhat.com [10.5.110.39]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 1CE2D5D75D for ; Wed, 31 Oct 2018 08:54:18 +0000 (UTC) Received: from mail-ot1-f72.google.com (mail-ot1-f72.google.com [209.85.210.72]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E9BEB58E22 for ; Wed, 31 Oct 2018 08:54:17 +0000 (UTC) Received: by mail-ot1-f72.google.com with SMTP id 34so10882514otj.2 for ; Wed, 31 Oct 2018 01:54:17 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Paul Moore Cc: Linux-Audit Mailing List List-Id: linux-audit@redhat.com Sorry for the long-delayed reply, the SELinux world is keeping me quite busy right now :) On Wed, Sep 19, 2018 at 5:44 PM Paul Moore wrote: > On Wed, Sep 19, 2018 at 7:01 AM Ondrej Mosnacek wrote: > > On Wed, Sep 19, 2018 at 3:35 AM Paul Moore wrote: > > > On Thu, Sep 13, 2018 at 10:13 AM Paul Moore wrote: > > > > On Thu, Sep 13, 2018 at 9:58 AM Ondrej Mosnacek 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" [...] Remember that whenever the audit userspace wants to turn a relative path into absolute, it just prepends it with PWD, so we would get paths "/some/dir/subdir" and "/path/to/pwd/subdir/file", which is even more confusing than the current situation (where we at least get the wrong PWD-based path consistently...). Granted, we could make userspace always find the corresponding PARENT record (which is not really possible reliably as there is no linking between them), and append the last component of CREATE/DELETE to that, but that seems quite messy to me... > > > 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. Yeah, I figured but I was still curious :) > > Hopefully that answers your question, but if you're still unclear let me know. > > -- > paul moore > www.paul-moore.com-- Ondrej Mosnacek Associate Software Engineer, Security Technologies Red Hat, Inc.