diff --git a/fact-ebpf/src/bpf/events.h b/fact-ebpf/src/bpf/events.h index ac1d1bd8..efedf86e 100644 --- a/fact-ebpf/src/bpf/events.h +++ b/fact-ebpf/src/bpf/events.h @@ -37,7 +37,11 @@ __always_inline static void __submit_event(struct submit_event_args_t* args, event->monitored = args->monitored; inode_copy(&event->inode, &args->inode); inode_copy(&event->parent_inode, &args->parent_inode); - bpf_probe_read_str(event->filename, PATH_MAX, args->filename); + if (args->filename != NULL) { + bpf_probe_read_str(event->filename, PATH_MAX, args->filename); + } else { + event->filename[0] = '\0'; + } struct helper_t* helper = get_helper(); if (helper == NULL) { @@ -144,3 +148,15 @@ __always_inline static void submit_rmdir_event(struct submit_event_args_t* args) __submit_event(args, path_hooks_support_bpf_d_path); } + +__always_inline static void submit_xattr_event(struct submit_event_args_t* args, + file_activity_type_t event_type, + const char* xattr_name) { + if (!reserve_event(args)) { + return; + } + args->event->type = event_type; + bpf_probe_read_str(args->event->xattr.name, XATTR_NAME_MAX_LEN, xattr_name); + + __submit_event(args, false); +} diff --git a/fact-ebpf/src/bpf/main.c b/fact-ebpf/src/bpf/main.c index eb2033e8..4eacdd79 100644 --- a/fact-ebpf/src/bpf/main.c +++ b/fact-ebpf/src/bpf/main.c @@ -389,6 +389,48 @@ int BPF_PROG(trace_d_instantiate, struct dentry* dentry, struct inode* inode) { return 0; } +__always_inline static int handle_xattr(struct metrics_by_hook_t* hook_metrics, + struct dentry* dentry, + const char* xattr_name, + file_activity_type_t event_type) { + struct submit_event_args_t args = {.metrics = hook_metrics}; + + args.metrics->total++; + + args.inode = inode_to_key(dentry->d_inode); + args.parent_inode = inode_to_key(BPF_CORE_READ(dentry, d_parent, d_inode)); + + args.monitored = inode_is_monitored(inode_get(&args.inode), inode_get(&args.parent_inode)); + + if (args.monitored == NOT_MONITORED) { + args.metrics->ignored++; + return 0; + } + + submit_xattr_event(&args, event_type, xattr_name); + return 0; +} + +SEC("lsm/inode_setxattr") +int BPF_PROG(trace_inode_setxattr, struct mnt_idmap* idmap, struct dentry* dentry, + const char* name, const void* value, size_t size, int flags) { + struct metrics_t* m = get_metrics(); + if (m == NULL) { + return 0; + } + return handle_xattr(&m->inode_setxattr, dentry, name, FILE_ACTIVITY_SETXATTR); +} + +SEC("lsm/inode_removexattr") +int BPF_PROG(trace_inode_removexattr, struct mnt_idmap* idmap, struct dentry* dentry, + const char* name) { + struct metrics_t* m = get_metrics(); + if (m == NULL) { + return 0; + } + return handle_xattr(&m->inode_removexattr, dentry, name, FILE_ACTIVITY_REMOVEXATTR); +} + SEC("lsm/path_rmdir") int BPF_PROG(trace_path_rmdir, struct path* dir, struct dentry* dentry) { struct metrics_t* m = get_metrics(); diff --git a/fact-ebpf/src/bpf/types.h b/fact-ebpf/src/bpf/types.h index 2f11c0db..3d3ec49e 100644 --- a/fact-ebpf/src/bpf/types.h +++ b/fact-ebpf/src/bpf/types.h @@ -15,6 +15,10 @@ #define LINEAGE_MAX 2 +// Matches Linux kernel XATTR_NAME_MAX (255) + null terminator. +// https://github.com/torvalds/linux/blob/66affa37cfac0aec061cc4bcf4a065b0c52f7e19/include/uapi/linux/limits.h#L15 +#define XATTR_NAME_MAX_LEN 256 + #define LPM_SIZE_MAX 256 typedef struct lineage_t { @@ -64,6 +68,8 @@ typedef enum file_activity_type_t { FILE_ACTIVITY_RENAME, DIR_ACTIVITY_CREATION, DIR_ACTIVITY_UNLINK, + FILE_ACTIVITY_SETXATTR, + FILE_ACTIVITY_REMOVEXATTR, } file_activity_type_t; struct event_t { @@ -90,6 +96,9 @@ struct event_t { inode_key_t inode; monitored_t monitored; } rename; + struct { + char name[XATTR_NAME_MAX_LEN]; + } xattr; }; }; @@ -132,4 +141,6 @@ struct metrics_t { struct metrics_by_hook_t path_mkdir; struct metrics_by_hook_t d_instantiate; struct metrics_by_hook_t path_rmdir; + struct metrics_by_hook_t inode_setxattr; + struct metrics_by_hook_t inode_removexattr; }; diff --git a/fact-ebpf/src/lib.rs b/fact-ebpf/src/lib.rs index f38fe801..a1dd046a 100644 --- a/fact-ebpf/src/lib.rs +++ b/fact-ebpf/src/lib.rs @@ -145,6 +145,8 @@ impl metrics_t { self.path_mkdir = self.path_mkdir.accumulate(&other.path_mkdir); self.path_rmdir = self.path_rmdir.accumulate(&other.path_rmdir); self.d_instantiate = self.d_instantiate.accumulate(&other.d_instantiate); + self.inode_setxattr = self.inode_setxattr.accumulate(&other.inode_setxattr); + self.inode_removexattr = self.inode_removexattr.accumulate(&other.inode_removexattr); self } } diff --git a/fact/src/event/mod.rs b/fact/src/event/mod.rs index 7944ada3..f09ebb4a 100644 --- a/fact/src/event/mod.rs +++ b/fact/src/event/mod.rs @@ -9,7 +9,9 @@ use std::{ use globset::GlobSet; use serde::Serialize; -use fact_ebpf::{PATH_MAX, event_t, file_activity_type_t, inode_key_t, monitored_t}; +use fact_ebpf::{ + PATH_MAX, XATTR_NAME_MAX_LEN, event_t, file_activity_type_t, inode_key_t, monitored_t, +}; use crate::host_info; use process::Process; @@ -131,6 +133,10 @@ impl Event { matches!(self.file, FileData::Creation(_) | FileData::MkDir(_)) } + pub fn is_xattr(&self) -> bool { + matches!(self.file, FileData::SetXattr(_) | FileData::RemoveXattr(_)) + } + pub fn is_mkdir(&self) -> bool { matches!(self.file, FileData::MkDir(_)) } @@ -162,6 +168,8 @@ impl Event { FileData::Chmod(data) => &data.inner.inode, FileData::Chown(data) => &data.inner.inode, FileData::Rename(data) => &data.new.inode, + FileData::SetXattr(data) => &data.inner.inode, + FileData::RemoveXattr(data) => &data.inner.inode, } } @@ -176,6 +184,8 @@ impl Event { FileData::Chmod(data) => &data.inner.parent_inode, FileData::Chown(data) => &data.inner.parent_inode, FileData::Rename(data) => &data.new.parent_inode, + FileData::SetXattr(data) => &data.inner.parent_inode, + FileData::RemoveXattr(data) => &data.inner.parent_inode, } } @@ -199,6 +209,8 @@ impl Event { FileData::Chmod(data) => &data.inner.filename, FileData::Chown(data) => &data.inner.filename, FileData::Rename(data) => &data.new.filename, + FileData::SetXattr(data) => &data.inner.filename, + FileData::RemoveXattr(data) => &data.inner.filename, } } @@ -219,6 +231,8 @@ impl Event { FileData::Chmod(data) => &data.inner.host_file, FileData::Chown(data) => &data.inner.host_file, FileData::Rename(data) => &data.new.host_file, + FileData::SetXattr(data) => &data.inner.host_file, + FileData::RemoveXattr(data) => &data.inner.host_file, } } @@ -243,6 +257,8 @@ impl Event { FileData::Chmod(data) => data.inner.host_file = host_path, FileData::Chown(data) => data.inner.host_file = host_path, FileData::Rename(data) => data.new.host_file = host_path, + FileData::SetXattr(data) => data.inner.host_file = host_path, + FileData::RemoveXattr(data) => data.inner.host_file = host_path, } } @@ -264,6 +280,8 @@ impl Event { FileData::Chmod(data) => data.inner.monitored, FileData::Chown(data) => data.inner.monitored, FileData::Rename(data) => data.new.monitored, + FileData::SetXattr(data) => data.inner.monitored, + FileData::RemoveXattr(data) => data.inner.monitored, } } @@ -356,6 +374,8 @@ pub enum FileData { Chmod(ChmodFileData), Chown(ChownFileData), Rename(RenameFileData), + SetXattr(XattrFileData), + RemoveXattr(XattrFileData), } impl FileData { @@ -407,6 +427,26 @@ impl FileData { }; FileData::Rename(data) } + file_activity_type_t::FILE_ACTIVITY_SETXATTR => { + let xattr_name = slice_to_string( + &unsafe { extra_data.xattr }.name[..XATTR_NAME_MAX_LEN as usize], + )?; + FileData::SetXattr(XattrFileData { + inner, + xattr_name, + operation: XattrOperation::Set, + }) + } + file_activity_type_t::FILE_ACTIVITY_REMOVEXATTR => { + let xattr_name = slice_to_string( + &unsafe { extra_data.xattr }.name[..XATTR_NAME_MAX_LEN as usize], + )?; + FileData::RemoveXattr(XattrFileData { + inner, + xattr_name, + operation: XattrOperation::Remove, + }) + } invalid => unreachable!("Invalid event type: {invalid:?}"), }; @@ -433,6 +473,14 @@ impl From for fact_api::file_activity::File { FileData::RmDir(_) => { unreachable!("RmDir event reached protobuf conversion"); } + FileData::SetXattr(event) => { + let f_act = fact_api::FileXattrChange::from(event); + fact_api::file_activity::File::Xattr(f_act) + } + FileData::RemoveXattr(event) => { + let f_act = fact_api::FileXattrChange::from(event); + fact_api::file_activity::File::Xattr(f_act) + } FileData::Unlink(event) => { let activity = Some(fact_api::FileActivityBase::from(event)); let f_act = fact_api::FileUnlink { activity }; @@ -465,6 +513,8 @@ impl PartialEq for FileData { (FileData::Unlink(this), FileData::Unlink(other)) => this == other, (FileData::Chmod(this), FileData::Chmod(other)) => this == other, (FileData::Rename(this), FileData::Rename(other)) => this == other, + (FileData::SetXattr(this), FileData::SetXattr(other)) => this == other, + (FileData::RemoveXattr(this), FileData::RemoveXattr(other)) => this == other, _ => false, } } @@ -595,6 +645,41 @@ impl PartialEq for RenameFileData { } } +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] +pub enum XattrOperation { + Set, + Remove, +} + +#[derive(Debug, Clone, Serialize)] +pub struct XattrFileData { + inner: BaseFileData, + xattr_name: String, + operation: XattrOperation, +} + +impl From for fact_api::FileXattrChange { + fn from(value: XattrFileData) -> Self { + let activity = fact_api::FileActivityBase::from(value.inner); + let operation = match value.operation { + XattrOperation::Set => fact_api::file_xattr_change::Operation::Set, + XattrOperation::Remove => fact_api::file_xattr_change::Operation::Remove, + }; + fact_api::FileXattrChange { + activity: Some(activity), + xattr_name: value.xattr_name, + operation: operation.into(), + } + } +} + +#[cfg(test)] +impl PartialEq for XattrFileData { + fn eq(&self, other: &Self) -> bool { + self.xattr_name == other.xattr_name && self.inner == other.inner + } +} + #[cfg(test)] mod test_utils { use std::os::raw::c_char; diff --git a/fact/src/metrics/kernel_metrics.rs b/fact/src/metrics/kernel_metrics.rs index 15da3993..df51f432 100644 --- a/fact/src/metrics/kernel_metrics.rs +++ b/fact/src/metrics/kernel_metrics.rs @@ -16,6 +16,8 @@ pub struct KernelMetrics { path_mkdir: EventCounter, path_rmdir: EventCounter, d_instantiate: EventCounter, + inode_setxattr: EventCounter, + inode_removexattr: EventCounter, map: PerCpuArray, } @@ -61,6 +63,16 @@ impl KernelMetrics { "Events processed by the d_instantiate LSM hook", &[], // Labels are not needed since `collect` will add them all ); + let inode_setxattr = EventCounter::new( + "kernel_inode_setxattr_events", + "Events processed by the inode_setxattr LSM hook", + &[], // Labels are not needed since `collect` will add them all + ); + let inode_removexattr = EventCounter::new( + "kernel_inode_removexattr_events", + "Events processed by the inode_removexattr LSM hook", + &[], // Labels are not needed since `collect` will add them all + ); file_open.register(reg); path_unlink.register(reg); @@ -70,6 +82,8 @@ impl KernelMetrics { path_mkdir.register(reg); path_rmdir.register(reg); d_instantiate.register(reg); + inode_setxattr.register(reg); + inode_removexattr.register(reg); KernelMetrics { file_open, @@ -80,6 +94,8 @@ impl KernelMetrics { path_mkdir, path_rmdir, d_instantiate, + inode_setxattr, + inode_removexattr, map: kernel_metrics, } } @@ -132,6 +148,8 @@ impl KernelMetrics { KernelMetrics::refresh_labels(&self.path_mkdir, &metrics.path_mkdir); KernelMetrics::refresh_labels(&self.path_rmdir, &metrics.path_rmdir); KernelMetrics::refresh_labels(&self.d_instantiate, &metrics.d_instantiate); + KernelMetrics::refresh_labels(&self.inode_setxattr, &metrics.inode_setxattr); + KernelMetrics::refresh_labels(&self.inode_removexattr, &metrics.inode_removexattr); Ok(()) } diff --git a/tests/event.py b/tests/event.py index 11bca769..d8639ac5 100644 --- a/tests/event.py +++ b/tests/event.py @@ -36,6 +36,7 @@ class EventType(Enum): PERMISSION = 4 OWNERSHIP = 5 RENAME = 6 + XATTR = 7 class Process: @@ -223,6 +224,7 @@ def __init__( owner_gid: int | None = None, old_file: str | Pattern[str] | None = None, old_host_path: str | Pattern[str] | None = None, + xattr_name: str | None = None, ): self._type: EventType = event_type self._process: Process = process @@ -233,6 +235,7 @@ def __init__( self._owner_gid: int | None = owner_gid self._old_file: str | Pattern[str] | None = old_file self._old_host_path: str | Pattern[str] | None = old_host_path + self._xattr_name: str | None = xattr_name @property def event_type(self) -> EventType: @@ -270,6 +273,10 @@ def old_file(self) -> str | Pattern[str] | None: def old_host_path(self) -> str | Pattern[str] | None: return self._old_host_path + @property + def xattr_name(self) -> str | None: + return self._xattr_name + @classmethod def _diff_field(cls, diff: dict, name: str, expected: Any, actual: Any): if expected != actual: @@ -378,6 +385,13 @@ def diff(self, other: FileActivity) -> dict | None: self.owner_gid, event_field.gid, ) + elif self.event_type == EventType.XATTR: + Event._diff_field( + diff, + 'xattr_name', + self.xattr_name, + event_field.xattr_name, + ) return diff if diff else None @@ -401,6 +415,9 @@ def __str__(self) -> str: f', old_host_path="{self.old_host_path}"' ) + if self.event_type == EventType.XATTR: + s += f', xattr_name="{self.xattr_name}"' + s += ')' return s diff --git a/tests/server.py b/tests/server.py index 048b4b26..2056bdad 100644 --- a/tests/server.py +++ b/tests/server.py @@ -109,7 +109,12 @@ def _wait_events( if len(events) == 0: return elif strict: - raise ValueError(json.dumps(diff, indent=4)) + # In strict mode, fail when the event type matches but + # the content differs. Events of a different type are + # skipped since they may be system noise that the test + # cannot predict (e.g. SELinux xattr changes). + if 'event_type' not in diff: + raise ValueError(json.dumps(diff, indent=4)) def wait_events(self, events: list[Event], strict: bool = True): """ diff --git a/tests/test_xattr.py b/tests/test_xattr.py new file mode 100644 index 00000000..61c89d68 --- /dev/null +++ b/tests/test_xattr.py @@ -0,0 +1,266 @@ +from __future__ import annotations + +import os + +import pytest + +from event import Event, EventType, Process +from server import FileActivityService +from utils import join_path_with_filename, path_to_string + + +def test_setxattr( + test_file: str, + server: FileActivityService, +): + """ + Tests that setting a user xattr on a monitored file generates + a gRPC xattr event. + + The test_file fixture creates a file before fact starts, so it is + picked up by the initial scan and its inode is already tracked. + + Args: + test_file: File monitored on the host. + server: The server instance to communicate with. + """ + process = Process.from_proc() + + os.setxattr(test_file, 'user.fact_test', b'test_value') + + server.wait_events( + [ + Event( + process=process, + event_type=EventType.XATTR, + file='', + host_path=test_file, + xattr_name='user.fact_test', + ), + ], + ) + + +def test_removexattr( + test_file: str, + server: FileActivityService, +): + """ + Tests that removing a user xattr from a monitored file generates + a gRPC xattr event. + + Args: + test_file: File monitored on the host. + server: The server instance to communicate with. + """ + process = Process.from_proc() + + os.setxattr(test_file, 'user.fact_remove', b'to_remove') + os.removexattr(test_file, 'user.fact_remove') + + server.wait_events( + [ + Event( + process=process, + event_type=EventType.XATTR, + file='', + host_path=test_file, + xattr_name='user.fact_remove', + ), + ], + ) + + +def test_setxattr_multiple( + test_file: str, + server: FileActivityService, +): + """ + Tests that setting multiple xattrs on a monitored file generates + a gRPC event for each. + + Args: + test_file: File monitored on the host. + server: The server instance to communicate with. + """ + process = Process.from_proc() + + os.setxattr(test_file, 'user.attr1', b'value1') + os.setxattr(test_file, 'user.attr2', b'value2') + os.setxattr(test_file, 'user.attr3', b'value3') + + server.wait_events( + [ + Event( + process=process, + event_type=EventType.XATTR, + file='', + host_path=test_file, + xattr_name='user.attr1', + ), + Event( + process=process, + event_type=EventType.XATTR, + file='', + host_path=test_file, + xattr_name='user.attr2', + ), + Event( + process=process, + event_type=EventType.XATTR, + file='', + host_path=test_file, + xattr_name='user.attr3', + ), + ], + ) + + +def test_setxattr_ignored( + test_file: str, + ignored_dir: str, + server: FileActivityService, +): + """ + Tests that xattr changes on unmonitored files are not tracked, + while xattr changes on monitored files are. + + Args: + test_file: File monitored on the host. + ignored_dir: Temporary directory that is not monitored by fact. + server: The server instance to communicate with. + """ + process = Process.from_proc() + + ignored_file = os.path.join(ignored_dir, 'ignored.txt') + with open(ignored_file, 'w') as f: + f.write('ignored') + + # Set xattr on ignored file - should NOT generate an event + os.setxattr(ignored_file, 'user.ignored', b'value') + + # Set xattr on monitored file - should generate an event + os.setxattr(test_file, 'user.monitored', b'value') + + # Only the monitored file's xattr event should arrive + server.wait_events( + [ + Event( + process=process, + event_type=EventType.XATTR, + file='', + host_path=test_file, + xattr_name='user.monitored', + ), + ], + ) + + +def test_setxattr_new_file( + monitored_dir: str, + server: FileActivityService, +): + """ + Tests that xattr tracking works for files created while fact is + running, not just files from the initial scan. + + A new file is created in the monitored directory and its creation + event is awaited to ensure the inode is tracked before setting + an xattr. + + Args: + monitored_dir: Temporary directory path that is monitored. + server: The server instance to communicate with. + """ + process = Process.from_proc() + + test_file = os.path.join(monitored_dir, 'xattr_new.txt') + with open(test_file, 'w') as f: + f.write('new file') + + server.wait_events( + [ + Event( + process=process, + event_type=EventType.CREATION, + file=test_file, + host_path=test_file, + ), + ], + ) + + os.setxattr(test_file, 'user.new_file', b'value') + + server.wait_events( + [ + Event( + process=process, + event_type=EventType.XATTR, + file='', + host_path=test_file, + xattr_name='user.new_file', + ), + ], + ) + + +@pytest.mark.parametrize( + 'filename', + [ + pytest.param('xattr.txt', id='ASCII'), + pytest.param('café.txt', id='French'), + pytest.param('файл.txt', id='Cyrillic'), + pytest.param('测试.txt', id='Chinese'), + pytest.param('🔒secure.txt', id='Emoji'), + pytest.param(b'xattr\xff\xfe.txt', id='InvalidUTF8'), + ], +) +def test_setxattr_utf8_filenames( + monitored_dir: str, + server: FileActivityService, + filename: str | bytes, +): + """ + Tests that xattr events are correctly tracked on files with + various UTF-8 and non-UTF-8 filenames. + + Args: + monitored_dir: Temporary directory path for creating the test file. + server: The server instance to communicate with. + filename: Name of the file to create (includes UTF-8 test cases). + """ + fut = join_path_with_filename(monitored_dir, filename) + + with open(fut, 'w') as f: + f.write('test') + + # gRPC events use lossy UTF-8 conversion, but os.setxattr + # needs the original path to find the file on disk. + fut_str = path_to_string(fut) + + process = Process.from_proc() + + server.wait_events( + [ + Event( + process=process, + event_type=EventType.CREATION, + file=fut_str, + host_path=fut_str, + ), + ], + ) + + os.setxattr(fut, 'user.utf8_test', b'value') + + server.wait_events( + [ + Event( + process=process, + event_type=EventType.XATTR, + file='', + host_path=fut_str, + xattr_name='user.utf8_test', + ), + ], + ) diff --git a/third_party/stackrox b/third_party/stackrox index e6bcbecf..d8239afc 160000 --- a/third_party/stackrox +++ b/third_party/stackrox @@ -1 +1 @@ -Subproject commit e6bcbecfe70a809fbcc809b8be496cd4c70a0691 +Subproject commit d8239afc33c2041c92fba2cb485738cf63a634ef