ROX-33198: Instrument inode tracking on file open lsm hook#391
ROX-33198: Instrument inode tracking on file open lsm hook#391JoukoVirtanen wants to merge 4 commits intomainfrom
Conversation
fact/src/host_scanner.rs
Outdated
| return Ok(()); | ||
| } | ||
|
|
||
| let host_path = host_info::prepend_host_mount(event.get_filename()); |
There was a problem hiding this comment.
The initial plan was to look up the parent, get its file path, and then add the file name. However, it seems possible to get the entire path from the event.
There was a problem hiding this comment.
This only works if the file is not mounted to a different directory. Try monitoring /etc/monitored on your host, then run something like docker run --rm -it -v /etc/monitored:/in-container fedora:43 and access some files in /in-container. You should see the events have a filename in /in-container with a host path of /etc/monitored.
| # Wait for creation event | ||
| process = Process.from_proc() | ||
| creation_event = Event(process=process, event_type=EventType.CREATION, | ||
| file=fut, host_path='') |
There was a problem hiding this comment.
Should host_path be populated here.
| """ | ||
| cwd = os.getcwd() | ||
| config = { | ||
| 'paths': [f'{monitored_dir}/**', '/mounted/**', '/container-dir/**'], |
There was a problem hiding this comment.
I am not sure what globbing to use here.
fact/src/host_scanner.rs
Outdated
| if path.is_file() || path.is_dir() { | ||
| self.metrics.scan_inc(ScanLabels::FileScanned); |
There was a problem hiding this comment.
Would be nice to have separate metrics here, so we can check how many files and directories we are tracking exactly.
fact/src/host_scanner.rs
Outdated
| return Ok(()); | ||
| } | ||
|
|
||
| let host_path = host_info::prepend_host_mount(event.get_filename()); |
There was a problem hiding this comment.
This only works if the file is not mounted to a different directory. Try monitoring /etc/monitored on your host, then run something like docker run --rm -it -v /etc/monitored:/in-container fedora:43 and access some files in /in-container. You should see the events have a filename in /in-container with a host path of /etc/monitored.
| .with_context(|| format!("Failed to add creation event entry for {}", host_path.display()))?; | ||
| } else { | ||
| debug!("Creation event for non-existent file: {}", host_path.display()); | ||
| self.metrics.scan_inc(ScanLabels::FileRemoved); |
There was a problem hiding this comment.
It's probably worth it to add a separate label for this case, missing a host file is a separate condition IMO.
| if event.is_creation() { | ||
| if let Err(e) = self.handle_creation_event(&event) { |
There was a problem hiding this comment.
If we change edition in fact/Cargo.toml to 2024 we can rewrite this like:
if event.is_creation() &&
let Err(e) = self.handle_creation_event(&event) {|
|
||
| /// Handle file creation events by adding new inodes to the map. | ||
| fn handle_creation_event(&self, event: &Event) -> anyhow::Result<()> { | ||
| if self.get_host_path(Some(event.get_inode())).is_some() { |
There was a problem hiding this comment.
This condition can never be true, the events coming from the kernel don't have the host path, I would remove the block.
There was a problem hiding this comment.
Why do we need a separate test for this? I assume you should be able to tweak the tests in test_file_open.py to test your changes.
… can now use untrusted pointers
| } | ||
|
|
||
| unsigned long magic = inode->i_sb->s_magic; | ||
| unsigned long magic = BPF_CORE_READ(inode, i_sb, s_magic); |
There was a problem hiding this comment.
inode_to_key needs to be able to use untrusted pointers, so that it can get the key for a parent inode. To get it to be able to use untrusted pointers, BPF_CORE_READ is used.
| // For file creation events, check if the parent directory is being | ||
| // monitored. If so, add the new file's inode to the tracked set. | ||
| if (event_type == FILE_ACTIVITY_CREATION) { | ||
| struct dentry* parent_dentry = BPF_CORE_READ(file, f_path.dentry, d_parent); | ||
| if (parent_dentry) { | ||
| struct inode* parent_inode = BPF_CORE_READ(parent_dentry, d_inode); | ||
| inode_key_t parent_key = inode_to_key(parent_inode); | ||
|
|
||
| if (inode_is_monitored(inode_get(&parent_key)) == MONITORED) { | ||
| inode_add(&inode_key); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we move some of this logic to is_monitored? We can change it to return an enum with values like MONITORED, NOT_MONITORED and PARENT_MONITORED, then we can decide what to do based on that returned value (i.e, add the inode if the event is creation and the parent is monitored).
| const char filename[PATH_MAX], | ||
| inode_key_t* inode) { | ||
| inode_key_t* inode, | ||
| inode_key_t* parent_inode) { |
There was a problem hiding this comment.
The parent inode was added to the event so that it could be used to find the parent path. It is added for all event types, not just when a file is opened. Perhaps NULL could be passed instead of the actual parent inode for the other type of events for now.
2 participants
Description
A detailed explanation of the changes in your PR.
Feel free to remove this section if it is overkill for your PR, and the title of your PR is sufficiently descriptive.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
TODO(replace-me)
Use this space to explain how you tested your PR, or, if you didn't test it, why you did not do so. (Valid reasons include "CI is sufficient" or "No testable changes")
In addition to reviewing your code, reviewers must also review your testing instructions, and make sure they are sufficient.
For more details, ref the Confluence page about this section.