| Message ID | 20241105123807.1257948-1-rrobaina@redhat.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v1] audit: fix suffixed '/' filename matching in __audit_inode_child() | expand |
On Nov 5, 2024 Ricardo Robaina <rrobaina@redhat.com> wrote: > > When the user specifies a directory to delete with the suffix '/', > the audit record fails to collect the filename, resulting in the > following logs: > > type=PATH msg=audit(10/30/2024 14:11:17.796:6304) : item=2 name=(null) > type=PATH msg=audit(10/30/2024 14:11:17.796:6304) : item=1 name=(null) > > It happens because the value of the variables dname, and n->name->name > in __audit_inode_child() differ only by the suffix '/'. This commit > treats this corner case by cleaning the input and passing the correct > filename to audit_compare_dname_path(). > > Steps to reproduce the issue: > > # auditctl -w /tmp > $ mkdir /tmp/foo > $ rm -r /tmp/foo/ or rmdir /tmp/foo/ > # ausearch -i | grep PATH | tail -3 > > This patch is based on a GitHub patch/PR by user @hqh2010. > https://github.com/linux-audit/audit-kernel/pull/148 > > Signed-off-by: Ricardo Robaina <rrobaina@redhat.com> > --- > kernel/auditsc.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index 6f0d6fb6523f..d4fbac6b71a8 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -2419,7 +2419,8 @@ void __audit_inode_child(struct inode *parent, > struct audit_names *n, *found_parent = NULL, *found_child = NULL; > struct audit_entry *e; > struct list_head *list = &audit_filter_list[AUDIT_FILTER_FS]; > - int i; > + int i, dlen, nlen; > + char *fn = NULL; > > if (context->context == AUDIT_CTX_UNUSED) > return; > @@ -2443,6 +2444,7 @@ void __audit_inode_child(struct inode *parent, > if (inode) > handle_one(inode); > > + dlen = strlen(dname->name); > /* look for a parent entry first */ > list_for_each_entry(n, &context->names_list, list) { > if (!n->name || > @@ -2450,15 +2452,27 @@ void __audit_inode_child(struct inode *parent, > n->type != AUDIT_TYPE_UNKNOWN)) > continue; > > + /* special case, entry name has the sufix "/" */ /sufix/suffix/ > + nlen = strlen(n->name->name); > + if (dname->name[dlen - 1] != '/' && n->name->name[nlen - 1] == '/') { I'm guessing @dname is never going to have a trailing slash so we don't care about @n missing the trailing slash? > + fn = kmalloc(PATH_MAX, GFP_KERNEL); > + if (!fn) { > + audit_panic("out of memory in __audit_inode_child()"); > + return; > + } > + strscpy(fn, n->name->name, nlen); > + } I'm looking at the extra work involved above with the alloc/copy and I'm wondering if we can't solve this a bit more generically (I suspect all the audit_compare_dname_path() callers may have similar issues) and with out the additional alloc/copy. This is completely untested, I didn't even compile it, but what about something like the following? We do add an extra strlen(), but that is going to be faster than the alloc/copy. diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c index 470041c49a44..c30c2ee9fb77 100644 --- a/kernel/auditfilter.c +++ b/kernel/auditfilter.c @@ -1320,10 +1320,13 @@ int audit_compare_dname_path(const struct qstr *dname, const char *path, int par return 1; parentlen = parentlen == AUDIT_NAME_FULL ? parent_len(path) : parentlen; - if (pathlen - parentlen != dlen) - return 1; - p = path + parentlen; + pathlen = strlen(p); + if (p[pathlen - 1] == '/') + pathlen--; + + if (pathlen != dlen) + return 1; return strncmp(p, dname->name, dlen); } > if (n->ino == parent->i_ino && n->dev == parent->i_sb->s_dev && > !audit_compare_dname_path(dname, > - n->name->name, n->name_len)) { > + fn ? fn : n->name->name, n->name_len)) { > if (n->type == AUDIT_TYPE_UNKNOWN) > n->type = AUDIT_TYPE_PARENT; > found_parent = n; > break; > } > } > + kfree(fn); > > cond_resched(); > > -- > 2.47.0 -- paul-moore.com
On 2024-11-11 17:06, Paul Moore wrote: > On Nov 5, 2024 Ricardo Robaina <rrobaina@redhat.com> wrote: > > > > When the user specifies a directory to delete with the suffix '/', > > the audit record fails to collect the filename, resulting in the > > following logs: > > > > type=PATH msg=audit(10/30/2024 14:11:17.796:6304) : item=2 name=(null) > > type=PATH msg=audit(10/30/2024 14:11:17.796:6304) : item=1 name=(null) > > > > It happens because the value of the variables dname, and n->name->name > > in __audit_inode_child() differ only by the suffix '/'. This commit > > treats this corner case by cleaning the input and passing the correct > > filename to audit_compare_dname_path(). > > > > Steps to reproduce the issue: > > > > # auditctl -w /tmp > > $ mkdir /tmp/foo > > $ rm -r /tmp/foo/ or rmdir /tmp/foo/ > > # ausearch -i | grep PATH | tail -3 > > > > This patch is based on a GitHub patch/PR by user @hqh2010. > > https://github.com/linux-audit/audit-kernel/pull/148 > > > > Signed-off-by: Ricardo Robaina <rrobaina@redhat.com> > > --- > > kernel/auditsc.c | 18 ++++++++++++++++-- > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index 6f0d6fb6523f..d4fbac6b71a8 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -2419,7 +2419,8 @@ void __audit_inode_child(struct inode *parent, > > struct audit_names *n, *found_parent = NULL, *found_child = NULL; > > struct audit_entry *e; > > struct list_head *list = &audit_filter_list[AUDIT_FILTER_FS]; > > - int i; > > + int i, dlen, nlen; > > + char *fn = NULL; > > > > if (context->context == AUDIT_CTX_UNUSED) > > return; > > @@ -2443,6 +2444,7 @@ void __audit_inode_child(struct inode *parent, > > if (inode) > > handle_one(inode); > > > > + dlen = strlen(dname->name); > > /* look for a parent entry first */ > > list_for_each_entry(n, &context->names_list, list) { > > if (!n->name || > > @@ -2450,15 +2452,27 @@ void __audit_inode_child(struct inode *parent, > > n->type != AUDIT_TYPE_UNKNOWN)) > > continue; > > > > + /* special case, entry name has the sufix "/" */ > > /sufix/suffix/ > > > + nlen = strlen(n->name->name); > > + if (dname->name[dlen - 1] != '/' && n->name->name[nlen - 1] == '/') { > > I'm guessing @dname is never going to have a trailing slash so we don't > care about @n missing the trailing slash? > > > + fn = kmalloc(PATH_MAX, GFP_KERNEL); > > + if (!fn) { > > + audit_panic("out of memory in __audit_inode_child()"); > > + return; > > + } > > + strscpy(fn, n->name->name, nlen); > > + } > > I'm looking at the extra work involved above with the alloc/copy and I'm > wondering if we can't solve this a bit more generically (I suspect all > the audit_compare_dname_path() callers may have similar issues) and with > out the additional alloc/copy. I had similar concerns about the alloc/copy and using a fixed length compare but hadn't thought of generalizing it. > This is completely untested, I didn't even compile it, but what about > something like the following? We do add an extra strlen(), but that is > going to be faster than the alloc/copy. I agree this is a better approach. > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > index 470041c49a44..c30c2ee9fb77 100644 > --- a/kernel/auditfilter.c > +++ b/kernel/auditfilter.c > @@ -1320,10 +1320,13 @@ int audit_compare_dname_path(const struct qstr *dname, const char *path, int par > return 1; > > parentlen = parentlen == AUDIT_NAME_FULL ? parent_len(path) : parentlen; > - if (pathlen - parentlen != dlen) > - return 1; > - > p = path + parentlen; > + pathlen = strlen(p); > + if (p[pathlen - 1] == '/') > + pathlen--; > + > + if (pathlen != dlen) > + return 1; > > return strncmp(p, dname->name, dlen); > } > > > if (n->ino == parent->i_ino && n->dev == parent->i_sb->s_dev && > > !audit_compare_dname_path(dname, > > - n->name->name, n->name_len)) { > > + fn ? fn : n->name->name, n->name_len)) { > > if (n->type == AUDIT_TYPE_UNKNOWN) > > n->type = AUDIT_TYPE_PARENT; > > found_parent = n; > > break; > > } > > } > > + kfree(fn); > > > > cond_resched(); > > > > -- > > 2.47.0 > > -- > paul-moore.com - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada Upstream IRC: SunRaycer Voice: +1.613.860 2354 SMS: +1.613.518.6570
On Mon, Nov 11, 2024 at 05:06:43PM -0500, Paul Moore wrote: > This is completely untested, I didn't even compile it, but what about > something like the following? We do add an extra strlen(), but that is > going to be faster than the alloc/copy. > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > index 470041c49a44..c30c2ee9fb77 100644 > --- a/kernel/auditfilter.c > +++ b/kernel/auditfilter.c > @@ -1320,10 +1320,13 @@ int audit_compare_dname_path(const struct qstr *dname, const char *path, int par > return 1; > > parentlen = parentlen == AUDIT_NAME_FULL ? parent_len(path) : parentlen; > - if (pathlen - parentlen != dlen) > - return 1; > - > p = path + parentlen; > + pathlen = strlen(p); Huh? We have pathlen = strlen(path); several lines prior to this. So unless parentlen + path manages to exceed strlen(path) (in which case your strlen() is really asking for trouble), this is simply pathlen -= parentlen; What am I missing here? And while we are at it, parentlen = parentlen == AUDIT_NAME_FULL ? parent_len(path) : parentlen; is a bloody awful way to spell if (parentlen == AUDIT_NAME_FULL) parentlen = parent_len(path); What's more, parent_len(path) starts with *yet* *another* strlen(path), followed by really awful crap - we trim the trailing slashes (if any), then search for the last slash before that... is that really worth the chance to skip that strncmp()? > + if (p[pathlen - 1] == '/') > + pathlen--; > + > + if (pathlen != dlen) > + return 1; > > return strncmp(p, dname->name, dlen); ... which really should've been memcmp(), at that.
On November 13, 2024 6:04:27 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > On Mon, Nov 11, 2024 at 05:06:43PM -0500, Paul Moore wrote: > >> This is completely untested, I didn't even compile it, but what about >> something like the following? We do add an extra strlen(), but that is >> going to be faster than the alloc/copy. >> >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c >> index 470041c49a44..c30c2ee9fb77 100644 >> --- a/kernel/auditfilter.c >> +++ b/kernel/auditfilter.c >> @@ -1320,10 +1320,13 @@ int audit_compare_dname_path(const struct qstr >> *dname, const char *path, int par >> return 1; >> >> parentlen = parentlen == AUDIT_NAME_FULL ? parent_len(path) : parentlen; >> - if (pathlen - parentlen != dlen) >> - return 1; >> - >> p = path + parentlen; >> + pathlen = strlen(p); > > Huh? We have > pathlen = strlen(path); > several lines prior to this. So unless parentlen + path manages to exceed > strlen(path) (in which case your strlen() is really asking for trouble), > this is simply > pathlen -= parentlen; > > What am I missing here? [NOTE: network access is patchy right now so you're getting a phone reply without an opportunity to look more closely at the code] To be fair, this was a quick example of "do something like this" to demonstrate a different idea than was proposed in the original patch. The bit of code I shared was not a fully baked patch; I thought that would have been clear from the context, if not my comments. Of course improvements are welcome, but in the future know that unless I'm submitting a proper patch, the code snippets I share during review are going to be *rough* and not fully developed. Additional work by the original author is expected. > And while we are at it, > parentlen = parentlen == AUDIT_NAME_FULL ? parent_len(path) : parentlen; > is a bloody awful way to spell > if (parentlen == AUDIT_NAME_FULL) > parentlen = parent_len(path); > What's more, parent_len(path) starts with *yet* *another* strlen(path), > followed by really awful crap - we trim the trailing slashes (if any), > then search for the last slash before that... is that really worth > the chance to skip that strncmp()? Pretty much all of the audit code is awkward at best Al, you should know that. We're not going to fix it all in one patch, and considering the nature of this patch effort, I think there is going to be a lot of value in keeping the initial fix patch to a minimum to ease backporting. We can improve on some of those other issues in a second patch or at a later time. As a reminder to everyone, patches are always welcome. Fixing things is a great way to channel disgust into something much more useful. > >> + if (p[pathlen - 1] == '/') >> + pathlen--; >> + >> + if (pathlen != dlen) >> + return 1; >> >> return strncmp(p, dname->name, dlen); > > ... which really should've been memcmp(), at that. Agreed. See above. -- paul-moore.com
On Wed, Nov 13, 2024 at 10:23:55PM -0500, Paul Moore wrote: > > And while we are at it, > > parentlen = parentlen == AUDIT_NAME_FULL ? parent_len(path) : parentlen; > > is a bloody awful way to spell > > if (parentlen == AUDIT_NAME_FULL) > > parentlen = parent_len(path); > > What's more, parent_len(path) starts with *yet* *another* strlen(path), > > followed by really awful crap - we trim the trailing slashes (if any), > > then search for the last slash before that... is that really worth > > the chance to skip that strncmp()? > > Pretty much all of the audit code is awkward at best Al, you should know > that. Do I ever... > We're not going to fix it all in one patch, and considering the nature > of this patch effort, I think there is going to be a lot of value in keeping > the initial fix patch to a minimum to ease backporting. We can improve on > some of those other issues in a second patch or at a later time. > > As a reminder to everyone, patches are always welcome. Fixing things is a > great way to channel disgust into something much more useful. > > > > > + if (p[pathlen - 1] == '/') > > > + pathlen--; > > > + > > > + if (pathlen != dlen) > > > + return 1; > > > > > > return strncmp(p, dname->name, dlen); > > > > ... which really should've been memcmp(), at that. > > Agreed. See above. OK, my primary interest here is to separate struct filename from that stuff as much as possible. So we will end up stomping on the same ground for a while here. FWIW, my current filename-related pile is in #next.filename; there will be a lot more on the VFS side, but one of the obvious targets is ->aname, so __audit_inode() and its vicinity will get affected. We'll need to coordinate that stuff. Anyway, regarding audit_compare_dname_path(), handling just the last '/' is not enough - there might be any number of trailing slashes, not just one. Another fun issue with looking for matches is this: mkdir /tmp/foo mkdir /tmp/foo/bar mkdir /tmp/blah ln -s ../foo/bar/baz /tmp/blah/barf echo crap > /tmp/blah/barf The last one will create a regular file "baz" in /tmp/foo/bar and write "crap\n" into it. With the only pathname passed to open(2) being "/tmp/blah/barf". And there may be a longer chain of symlinks like that. What do you want to see reported in such case?
On Wed, Nov 13, 2024 at 11:09 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > On Wed, Nov 13, 2024 at 10:23:55PM -0500, Paul Moore wrote: > > > And while we are at it, > > > parentlen = parentlen == AUDIT_NAME_FULL ? parent_len(path) : parentlen; > > > is a bloody awful way to spell > > > if (parentlen == AUDIT_NAME_FULL) > > > parentlen = parent_len(path); > > > What's more, parent_len(path) starts with *yet* *another* strlen(path), > > > followed by really awful crap - we trim the trailing slashes (if any), > > > then search for the last slash before that... is that really worth > > > the chance to skip that strncmp()? > > > > Pretty much all of the audit code is awkward at best Al, you should know > > that. > > Do I ever... > > > We're not going to fix it all in one patch, and considering the nature > > of this patch effort, I think there is going to be a lot of value in keeping > > the initial fix patch to a minimum to ease backporting. We can improve on > > some of those other issues in a second patch or at a later time. > > > > As a reminder to everyone, patches are always welcome. Fixing things is a > > great way to channel disgust into something much more useful. > > > > > > > > + if (p[pathlen - 1] == '/') > > > > + pathlen--; > > > > + > > > > + if (pathlen != dlen) > > > > + return 1; > > > > > > > > return strncmp(p, dname->name, dlen); > > > > > > ... which really should've been memcmp(), at that. > > > > Agreed. See above. > > OK, my primary interest here is to separate struct filename from that stuff > as much as possible. So we will end up stomping on the same ground for > a while here. FWIW, my current filename-related pile is in #next.filename; > there will be a lot more on the VFS side, but one of the obvious targets is > ->aname, so __audit_inode() and its vicinity will get affected. We'll need > to coordinate that stuff. [Sorry for the delay, it took me a bit longer than expected to get through my inbox upon returning home, but I should be mostly caught up now.] We've talked a bit in other threads about the struct filename changes you're working on, and while I haven't looked at your next.filename branch, the stuff you were talking about doing made sense. As far as cross-tree conflicts go, I don't think we'll have too many problems there as I don't foresee any major audit work happening in the immediate future; sure we'll likely have a small number of fixes but those should be easy enough to manage with your changes. I'm assuming your next.filename finds its way into linux-next? If so, that should give us a heads-up regarding conflicts. If there is a nasty collision, I'm confident we can figure something out. > Anyway, regarding audit_compare_dname_path(), handling just the last '/' is > not enough - there might be any number of trailing slashes, not just one. Good reminder, thanks. I see that Ricardo has sent an updated patch, I haven't looked at it yet (waiting for the merge window to close), but hopefully he has taken that into account (hint: now would be a good time to verify that Ricardo <g>). > Another fun issue with looking for matches is this: > > mkdir /tmp/foo > mkdir /tmp/foo/bar > mkdir /tmp/blah > ln -s ../foo/bar/baz /tmp/blah/barf > echo crap > /tmp/blah/barf > > The last one will create a regular file "baz" in /tmp/foo/bar and write > "crap\n" into it. With the only pathname passed to open(2) being > "/tmp/blah/barf". And there may be a longer chain of symlinks like that. > > What do you want to see reported in such case? In the absence of anyone pushing third party requirements on the audit subsystem to meet various security certification goals, my guiding light has been to try and do the simplest thing that makes sense. Looking at this quickly, there are three different types of events: mkdir, symlink, and a file lookup/write. The mkdir events in the example above are trivial, you log the usual info when walking the pathname and call it good. The symlink is mostly the same as mkdir, but you've got the relative path to contend with so the current working directory is more important and should be logged as well. The file lookup/write can be a bit more complicated if we have all of the symlink information as part of the pathname lookup/walk, but if we don't have that I think we can manage with just the "/tmp/blah/barf". Users/admins that really care about links will likely be logging the various link*/symlink*/unlink* syscalls for the directories/files they care about. Of course if anyone has any requirements or reasoned opinions about what we should log here, I'd love to hear it. This includes you Al.
diff --git a/kernel/auditsc.c b/kernel/auditsc.c index 6f0d6fb6523f..d4fbac6b71a8 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -2419,7 +2419,8 @@ void __audit_inode_child(struct inode *parent, struct audit_names *n, *found_parent = NULL, *found_child = NULL; struct audit_entry *e; struct list_head *list = &audit_filter_list[AUDIT_FILTER_FS]; - int i; + int i, dlen, nlen; + char *fn = NULL; if (context->context == AUDIT_CTX_UNUSED) return; @@ -2443,6 +2444,7 @@ void __audit_inode_child(struct inode *parent, if (inode) handle_one(inode); + dlen = strlen(dname->name); /* look for a parent entry first */ list_for_each_entry(n, &context->names_list, list) { if (!n->name || @@ -2450,15 +2452,27 @@ void __audit_inode_child(struct inode *parent, n->type != AUDIT_TYPE_UNKNOWN)) continue; + /* special case, entry name has the sufix "/" */ + nlen = strlen(n->name->name); + if (dname->name[dlen - 1] != '/' && n->name->name[nlen - 1] == '/') { + fn = kmalloc(PATH_MAX, GFP_KERNEL); + if (!fn) { + audit_panic("out of memory in __audit_inode_child()"); + return; + } + strscpy(fn, n->name->name, nlen); + } + if (n->ino == parent->i_ino && n->dev == parent->i_sb->s_dev && !audit_compare_dname_path(dname, - n->name->name, n->name_len)) { + fn ? fn : n->name->name, n->name_len)) { if (n->type == AUDIT_TYPE_UNKNOWN) n->type = AUDIT_TYPE_PARENT; found_parent = n; break; } } + kfree(fn); cond_resched();
When the user specifies a directory to delete with the suffix '/', the audit record fails to collect the filename, resulting in the following logs: type=PATH msg=audit(10/30/2024 14:11:17.796:6304) : item=2 name=(null) type=PATH msg=audit(10/30/2024 14:11:17.796:6304) : item=1 name=(null) It happens because the value of the variables dname, and n->name->name in __audit_inode_child() differ only by the suffix '/'. This commit treats this corner case by cleaning the input and passing the correct filename to audit_compare_dname_path(). Steps to reproduce the issue: # auditctl -w /tmp $ mkdir /tmp/foo $ rm -r /tmp/foo/ or rmdir /tmp/foo/ # ausearch -i | grep PATH | tail -3 This patch is based on a GitHub patch/PR by user @hqh2010. https://github.com/linux-audit/audit-kernel/pull/148 Signed-off-by: Ricardo Robaina <rrobaina@redhat.com> --- kernel/auditsc.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)