Open Davack opened 2 years ago
Thanks for the report @Davack. What kernel were you running when you observed this?
This is just a guess, but it looks like it is using $CWD in place of the proper directory, "/tmp/test" in your example. Is this something you think you might be able to work on resolving, or are you simply reporting a problem?
uname -a
Linux davlevi-ThinkPad-T490s 5.4.0-90-generic #101-Ubuntu SMP Fri Oct 15 20:00:55 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
Actually, I will have some free time to submit a pull request next week, I will try to track down the relevant code path for this issue :)
That's great, thanks @Davack!
We've done some work to analyze the issue.
The issue result from the fact that rm -rf
calls unlinkat
for all subdirectory files, relative to the /tmp/test/
directory. ). It seems that this issue will affect all file operations relative to open fds (e.g. all *at()
syscalls)
The parent directory path is added to the audit context names_list in the context of do_unlinkat()
-> filename_parentat()
-> audit_inode()
.
Notice do_unlinkat()
calls filename_parentat()
with dfd
pointing to the parent dir ("/tmp/test/"
) and the name
argument is a relative name equal to "1"
.
The function filename_parentat()
calls path_parentat()
which gets the correct struct path parent
, the correct inode
and fills the last
parameter. However, when it calls audit_inode()
the name
passed is still the relative leaf filename ("1"
).
Inside auditd_inode()
, we arrive to out:
label with parent
flag set to true
, so the assignment
n->name_len = n->name ? parent_len(n->name->name) : AUDIT_NAME_FULL;
is resolved as folows: parent_len()
of "1"
returns 0 and n->name_len
is set to 0.
Then in audit_log_name()
we arrive with path=NULL
from audit_log_exit()
. The switch on n->name_len
lands on case 0
:
/* name was specified as a relative path and the
* directory component is the cwd
*/
if (context->pwd.dentry && context->pwd.mnt)
audit_log_d_path(ab, " name=", &context->pwd);
else
audit_log_format(ab, " name=(null)");
break;
And we get the pwd
printed, instead of the containing directory path.
It seems that auditd needs to make sure it resolves the full parent path either before, or inside audit_inode
.
I'm not proficient enough in the the kernel code, to make a PR independently, but I'm willing to do it if assisted.
Hi @sirotnikov, thanks for taking the time to debug this and write up your findings!
I'm still thinking a bit about the problem and what some of our options might be, but before I spend too much time on that I wanted to ask if you have a desire to work on a fix for this? You mentioned being willing, but I realize that sometimes there is an important difference between being willing to do something and wanting to do something ;)
I'm very happy to get more people involved in kernel development, so if you want to work on a fix for this I'll do whatever I can to help, just let me know.
However, if you would rather defer to someone else, that's fine too; you've already been a big help in finding the cause of the problem.
Hey @pcmoore I have the desire, but I'm not great on availability, due to real life commitments. If you prefer someone that can do it in a timely manner, then it's probably not me. If you're ok with a slow pace and can provide some hand-holding, then I'll be happy to take this as a learning opportunity.
Hi @sirotnikov, no worries, I think we all understand what it is like to juggle multiple commitments.
Why don't you go ahead and work on this, as you are comfortable, and let us know if you have any questions. If we run into a serious or immediate need to get this finished we can always be in touch and perhaps finish up the work collaboratively. How does that sound?
I'd rather get more people involved in audit kernel development than a quick fix ;)
So, let's discuss possible directions for solution.
My feeling is that a correct resolution would be to fix filename_parentat
to
a) reconstruct the full path from the parent
parameter
b) pass the full parent path to audit_inode
c) make sure that audit_inode
with AUDIT_INODE_PARENT
flag does what is expected
I'm not familiar enough with the kernel methods, nor with the role of filename_parentat
and audit_inode
to understand if this suggestion will introduce risk
Also, if there are resources for setting up a dev environment / tests etc, I'd appreciate a reference
Hi, While I was working around AuditD, I encountered an interesting bug when removing a folder the files within the folder are reported with the incorrect
PARENT
name
property.Repro: Creating folder with five files inside
mkdir /tmp/test
touch /tmp/1
touch /tmp/2
touch /tmp/3
touch /tmp/4
touch /tmp/5
Removing the folderrm -rf /tmp/test
AuditD log:
You can notice that AuditD reporting
unlinkat
syscall for each of the files but if you look at thePARENT
record of each of those deletions you can see that thename
property is set to CWD (/home/davlevi
) instead of the real parent folder (/tmp/test
), you can even see that inode number of the parent (917526
) is the correct inode of/tmp/test
so onlyname
property is incorrect.Thanks in advance, David.