Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/unit-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ jobs:
# need more investigation
- rhel-8
- rhcos-412-86-202402272018-0-gcp-x86-64
- rhcos-414-92-202407091253-0-gcp-x86-64
# BPF trampolines are only implemented starting with RHEL 10
- rhel-9-arm64
EOF
Expand Down
22 changes: 22 additions & 0 deletions fact-ebpf/src/bpf/events.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,25 @@ __always_inline static void submit_mode_event(struct metrics_by_hook_t* m,

__submit_event(event, m, FILE_ACTIVITY_CHMOD, filename, inode, use_bpf_d_path);
}

__always_inline static void submit_owner_event(struct metrics_by_hook_t* m,
Comment thread
Molter73 marked this conversation as resolved.
Outdated
const char filename[PATH_MAX],
inode_key_t* inode,
unsigned long long uid,
unsigned long long gid,
unsigned long long old_uid,
unsigned long long old_gid,
bool use_bpf_d_path) {
struct event_t* event = bpf_ringbuf_reserve(&rb, sizeof(struct event_t), 0);
if (event == NULL) {
m->ringbuffer_full++;
return;
}

event->chown.new.uid = uid;
event->chown.new.gid = gid;
event->chown.old.uid = old_uid;
event->chown.old.gid = old_gid;

__submit_event(event, m, FILE_ACTIVITY_CHOWN, filename, inode, use_bpf_d_path);
}
55 changes: 55 additions & 0 deletions fact-ebpf/src/bpf/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -173,3 +173,58 @@ int BPF_PROG(trace_path_chmod, struct path* path, umode_t mode) {

return 0;
}

SEC("lsm/path_chown")
int BPF_PROG(trace_path_chown, struct path* path, unsigned long long uid, unsigned long long gid) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] It'd be nice to have a small comment here explaining why uid and gid are unsigned long long instead of kuid_t and kgid_t, in case someone comes in without the context in the future and attempts to change it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment in b381565 . I hope it makes sense.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine, maybe we can mention the verifier does not allow struct types as arguments, so we do this instead.

struct metrics_t* m = get_metrics();
if (m == NULL) {
return 0;
}

m->path_chown.total++;

struct bound_path_t* bound_path = NULL;
if (path_hooks_support_bpf_d_path) {
bound_path = path_read(path);
} else {
bound_path = path_read_no_d_path(path);
}

if (bound_path == NULL) {
bpf_printk("Failed to read path");
m->path_chown.error++;
return 0;
}

inode_key_t inode_key = inode_to_key(path->dentry->d_inode);
const inode_value_t* inode = inode_get(&inode_key);

switch (inode_is_monitored(inode)) {
case NOT_MONITORED:
if (!is_monitored(bound_path)) {
m->path_chown.ignored++;
return 0;
}
break;

case MONITORED:
break;
}

struct dentry* d = BPF_CORE_READ(path, dentry);
kuid_t kuid = BPF_CORE_READ(d, d_inode, i_uid);
kgid_t kgid = BPF_CORE_READ(d, d_inode, i_gid);
unsigned long long old_uid = BPF_CORE_READ(&kuid, val);
unsigned long long old_gid = BPF_CORE_READ(&kgid, val);
Comment thread
Molter73 marked this conversation as resolved.
Outdated

submit_owner_event(&m->path_chown,
bound_path->path,
&inode_key,
uid,
gid,
old_uid,
old_gid,
path_hooks_support_bpf_d_path);

return 0;
}
8 changes: 8 additions & 0 deletions fact-ebpf/src/bpf/types.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ typedef enum file_activity_type_t {
FILE_ACTIVITY_CREATION,
FILE_ACTIVITY_UNLINK,
FILE_ACTIVITY_CHMOD,
FILE_ACTIVITY_CHOWN,
} file_activity_type_t;

struct event_t {
Expand All @@ -66,6 +67,12 @@ struct event_t {
short unsigned int new;
short unsigned int old;
} chmod;
struct {
struct {
unsigned int uid;
unsigned int gid;
} old, new;
} chown;
};
};

Expand Down Expand Up @@ -96,4 +103,5 @@ struct metrics_t {
struct metrics_by_hook_t file_open;
struct metrics_by_hook_t path_unlink;
struct metrics_by_hook_t path_chmod;
struct metrics_by_hook_t path_chown;
};
66 changes: 66 additions & 0 deletions fact/src/event/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ impl Event {
FileData::Creation(data) => &data.inode,
FileData::Unlink(data) => &data.inode,
FileData::Chmod(data) => &data.inner.inode,
FileData::Chown(data) => &data.inner.inode,
}
}

Expand All @@ -91,6 +92,7 @@ impl Event {
FileData::Creation(data) => data.host_file = host_path,
FileData::Unlink(data) => data.host_file = host_path,
FileData::Chmod(data) => data.inner.host_file = host_path,
FileData::Chown(data) => data.inner.host_file = host_path,
}
}
}
Expand Down Expand Up @@ -145,6 +147,7 @@ pub enum FileData {
Creation(BaseFileData),
Unlink(BaseFileData),
Chmod(ChmodFileData),
Chown(ChownFileData),
}

impl FileData {
Expand All @@ -167,6 +170,16 @@ impl FileData {
};
FileData::Chmod(data)
}
file_activity_type_t::FILE_ACTIVITY_CHOWN => {
let data = ChownFileData {
inner,
new_uid: unsafe { extra_data.chown.new.uid },
new_gid: unsafe { extra_data.chown.new.gid },
old_uid: unsafe { extra_data.chown.old.uid },
old_gid: unsafe { extra_data.chown.old.gid },
};
FileData::Chown(data)
}
invalid => unreachable!("Invalid event type: {invalid:?}"),
};

Expand Down Expand Up @@ -196,6 +209,10 @@ impl From<FileData> for fact_api::file_activity::File {
let f_act = fact_api::FilePermissionChange::from(event);
fact_api::file_activity::File::Permission(f_act)
}
FileData::Chown(event) => {
let f_act = fact_api::FileOwnershipChange::from(event);
fact_api::file_activity::File::Ownership(f_act)
}
}
}
}
Expand Down Expand Up @@ -295,3 +312,52 @@ impl PartialEq for ChmodFileData {
&& self.inner == other.inner
}
}

#[derive(Debug, Clone, Serialize)]
pub struct ChownFileData {
inner: BaseFileData,
new_uid: u32,
new_gid: u32,
old_uid: u32,
old_gid: u32,
}

impl ChownFileData {
pub fn new(
Comment thread
Molter73 marked this conversation as resolved.
Outdated
filename: [c_char; PATH_MAX as usize],
inode: inode_key_t,
new_uid: u32,
new_gid: u32,
old_uid: u32,
old_gid: u32,
) -> anyhow::Result<Self> {
let file = BaseFileData::new(filename, inode)?;

Ok(ChownFileData {
inner: file,
new_uid,
new_gid,
old_uid,
old_gid,
})
}
}
impl From<ChownFileData> for fact_api::FileOwnershipChange {
fn from(value: ChownFileData) -> Self {
let ChownFileData {
inner: file,
new_uid,
new_gid,
old_uid: _,
old_gid: _,
} = value;
Comment thread
Molter73 marked this conversation as resolved.
let activity = fact_api::FileActivityBase::from(file);
fact_api::FileOwnershipChange {
activity: Some(activity),
uid: new_uid,
gid: new_gid,
username: "".to_string(),
group: "".to_string(),
}
}
}
9 changes: 9 additions & 0 deletions fact/src/metrics/kernel_metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ pub struct KernelMetrics {
file_open: EventCounter,
path_unlink: EventCounter,
path_chmod: EventCounter,
path_chown: EventCounter,
map: PerCpuArray<MapData, metrics_t>,
}

Expand All @@ -31,15 +32,22 @@ impl KernelMetrics {
"Events processed by the path_chmod LSM hook",
&[], // Labels are not needed since `collect` will add them all
);
let path_chown = EventCounter::new(
"kernel_path_chown_events",
"Events processed by the path_chown LSM hook",
&[], // Labels are not needed since `collect` will add them all
);

file_open.register(reg);
path_unlink.register(reg);
path_chmod.register(reg);
path_chown.register(reg);

KernelMetrics {
file_open,
path_unlink,
path_chmod,
path_chown,
Comment thread
Molter73 marked this conversation as resolved.
map: kernel_metrics,
}
}
Expand Down Expand Up @@ -87,6 +95,7 @@ impl KernelMetrics {
KernelMetrics::refresh_labels(&self.file_open, &metrics.file_open);
KernelMetrics::refresh_labels(&self.path_unlink, &metrics.path_unlink);
KernelMetrics::refresh_labels(&self.path_chmod, &metrics.path_chmod);
KernelMetrics::refresh_labels(&self.path_chown, &metrics.path_chown);

Ok(())
}
Expand Down
23 changes: 22 additions & 1 deletion tests/event.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class EventType(Enum):
CREATION = 2
UNLINK = 3
PERMISSION = 4
OWNERSHIP = 5


class Process:
Expand Down Expand Up @@ -171,12 +172,16 @@ def __init__(self,
event_type: EventType,
file: str,
host_path: str = '',
mode: int | None = None):
mode: int | None = None,
owner_uid: int | None = None,
owner_gid: int | None = None,):
self._type: EventType = event_type
self._process: Process = process
self._file: str = file
self._host_path: str = host_path
self._mode: int | None = mode
self._owner_uid: int | None = owner_uid
self._owner_gid: int | None = owner_gid

@property
def event_type(self) -> EventType:
Expand All @@ -198,6 +203,14 @@ def host_path(self) -> str:
def mode(self) -> int | None:
return self._mode

@property
def owner_uid(self) -> int | None:
return self._owner_uid

@property
def owner_gid(self) -> int | None:
return self._owner_gid

@override
def __eq__(self, other: Any) -> bool:
if isinstance(other, FileActivity):
Expand All @@ -217,6 +230,11 @@ def __eq__(self, other: Any) -> bool:
return self.file == other.permission.activity.path and \
self.host_path == other.permission.activity.host_path and \
self.mode == other.permission.mode
elif self.event_type == EventType.OWNERSHIP:
return self.file == other.ownership.activity.path and \
self.host_path == other.ownership.activity.host_path and \
(self.owner_uid is None or self.owner_uid == other.ownership.uid) and \
(self.owner_gid is None or self.owner_gid == other.ownership.gid)
Comment thread
Molter73 marked this conversation as resolved.
Outdated
return False
raise NotImplementedError

Expand All @@ -229,6 +247,9 @@ def __str__(self) -> str:
if self.event_type == EventType.PERMISSION:
s += f', mode={self.mode}'

if self.event_type == EventType.OWNERSHIP:
s += f', owner=(uid={self.owner_uid}, gid={self.owner_gid})'

s += ')'

return s
Loading
Loading