Skip to content
Open
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ possible include a PR number for easier tracking.
* ROX-34502: reload mTLS certificates on each gRPC connection attempt (#788)
* chore: add formatting and linting to integration test code (#783, #784)
* feat: add code coverage with cargo-llvm-cov and Codecov upload (#745)
* ROX-30296: adds ACL tracking via inode_set_acl LSM hook (#878)

## 0.3.0

Expand Down
6 changes: 6 additions & 0 deletions fact-ebpf/src/bpf/checks.c
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,9 @@ int BPF_PROG(check_path_unlink_supports_bpf_d_path, struct path* dir, struct den
bpf_printk("dir: %s", p->path);
return 0;
}

SEC("lsm/inode_set_acl")
int BPF_PROG(check_inode_set_acl, struct mnt_idmap* idmap, struct dentry* dentry, const char* acl_name,
struct posix_acl* kacl) {
return 0;
}
42 changes: 42 additions & 0 deletions fact-ebpf/src/bpf/events.h
Original file line number Diff line number Diff line change
Expand Up @@ -160,3 +160,45 @@ __always_inline static void submit_xattr_event(struct submit_event_args_t* args,

__submit_event(args, false);
}

__always_inline static void submit_acl_event(struct submit_event_args_t* args,
const char* acl_name,
struct posix_acl* kacl) {
if (!reserve_event(args)) {
return;
}

args->event->type = FILE_ACTIVITY_ACL_SET;

// Determine ACL type from the xattr name.
// "system.posix_acl_access" vs "system.posix_acl_default"
char name_buf[32] = {0};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is the length here random or does it come from some kernel constant somehwere?

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.

It only needs to store "system.posix_acl_{access,default}", max 25 chars, but rounded up to nearest power of 2 to be a little neater for alignment etc. I can adjust the comment for clarity

long name_len = bpf_probe_read_kernel_str(name_buf, sizeof(name_buf), acl_name);
if (name_len == 25 && __builtin_memcmp(name_buf, "system.posix_acl_default", 24) == 0) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could there be a situation where we want to keep the full name of the ACL as opposed to just a generic marker? (I apologize if this is a dumb question, I'm not super familiar with ACLs).

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.

So ACLs can only be the xattrs system.posix_acl_access and/or system.posix_acl_default so we can make things a bit simpler by collapsing down to an enum

args->event->acl.acl_type = FACT_ACL_TYPE_DEFAULT;
} else {
args->event->acl.acl_type = FACT_ACL_TYPE_ACCESS;
}

if (kacl == NULL) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[ultra-nit] Can we negate this condition so the largest block is the one for the true condition and not the else?

args->event->acl.count = 0;
} else {
unsigned int count = 0;
bpf_probe_read_kernel(&count, sizeof(count), &kacl->a_count);
if (count > FACT_MAX_ACL_ENTRIES) {
count = FACT_MAX_ACL_ENTRIES;
}
args->event->acl.count = count;

for (unsigned int i = 0; i < FACT_MAX_ACL_ENTRIES && i < count; i++) {
struct posix_acl_entry entry = {0};
bpf_probe_read_kernel(&entry, sizeof(entry), &kacl->a_entries[i]);
args->event->acl.entries[i].e_tag = entry.e_tag;
args->event->acl.entries[i].e_perm = entry.e_perm;
args->event->acl.entries[i].e_id = entry.e_uid.val;
}
Comment on lines +193 to +199

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

git ls-files 'fact-ebpf/src/bpf/events.h' 'fact-ebpf/src/lib.rs'
wc -l fact-ebpf/src/bpf/events.h fact-ebpf/src/lib.rs
ast-grep outline fact-ebpf/src/bpf/events.h --view expanded
ast-grep outline fact-ebpf/src/lib.rs --view expanded

Repository: stackrox/fact

Length of output: 1670


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '1,240p' fact-ebpf/src/bpf/events.h
printf '\n---- lib.rs ----\n'
sed -n '1,220p' fact-ebpf/src/lib.rs

Repository: stackrox/fact

Length of output: 11141


Avoid copying entry.e_uid.val for every ACL tag

posix_acl_entry.e_uid is only valid for ACL_USER / ACL_GROUP; for ACL_USER_OBJ, ACL_GROUP_OBJ, ACL_MASK, and ACL_OTHER this reads uninitialized kernel heap data and emits a bogus e_id. Use ACL_UNDEFINED_ID for the other tags.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@fact-ebpf/src/bpf/events.h` around lines 193 - 199, The ACL copy loop in
events.h is always assigning entry.e_uid.val to
args->event->acl.entries[i].e_id, but posix_acl_entry.e_uid is only valid for
ACL_USER and ACL_GROUP. Update the logic in the ACL entry population path to
branch on entry.e_tag and set e_id from e_uid.val only for those tags; for
ACL_USER_OBJ, ACL_GROUP_OBJ, ACL_MASK, and ACL_OTHER, assign ACL_UNDEFINED_ID
instead. Keep the change localized to the ACL event-building loop so the rest of
the field copying in args->event->acl.entries remains unchanged.

}

// inode_set_acl does not support bpf_d_path (no struct path available)
__submit_event(args, false);
}
25 changes: 25 additions & 0 deletions fact-ebpf/src/bpf/main.c
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,31 @@ int BPF_PROG(trace_inode_removexattr, struct mnt_idmap* idmap, struct dentry* de
return handle_xattr(&m->inode_removexattr, dentry, name, FILE_ACTIVITY_REMOVEXATTR);
}

SEC("lsm/inode_set_acl")
int BPF_PROG(trace_inode_set_acl, struct mnt_idmap* idmap, struct dentry* dentry,
const char* acl_name, struct posix_acl* kacl) {
struct metrics_t* m = get_metrics();
if (m == NULL) {
return 0;
}
struct submit_event_args_t args = {.metrics = &m->inode_set_acl};

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_acl_event(&args, acl_name, kacl);
return 0;
}

SEC("lsm/path_rmdir")
int BPF_PROG(trace_path_rmdir, struct path* dir, struct dentry* dentry) {
struct metrics_t* m = get_metrics();
Expand Down
19 changes: 19 additions & 0 deletions fact-ebpf/src/bpf/types.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,18 @@ typedef enum monitored_t {
// For the time being we just keep a char.
typedef char inode_value_t;

#define FACT_MAX_ACL_ENTRIES 32

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity, is there a max number of ACLs in the kernel? For our implementation, 32 is probably fine as is.

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.

The max is based on the xattr limit, which appears to be about 65k (total memory) which I think means it's about ~8k theoretical possible ACL entries. In practice at the file system level the limit is lower, though probably still in the hundreds. It's hard to tell what the numbers will be "in the wild" but we can raise this limit if we hit issues with it


// ACL type constants matching the xattr names
#define FACT_ACL_TYPE_ACCESS 0
#define FACT_ACL_TYPE_DEFAULT 1

struct acl_entry_t {
short e_tag;
unsigned short e_perm;
unsigned int e_id;
};

typedef enum file_activity_type_t {
FILE_ACTIVITY_INIT = -1,
FILE_ACTIVITY_OPEN = 0,
Expand All @@ -70,6 +82,7 @@ typedef enum file_activity_type_t {
DIR_ACTIVITY_UNLINK,
FILE_ACTIVITY_SETXATTR,
FILE_ACTIVITY_REMOVEXATTR,
FILE_ACTIVITY_ACL_SET,
} file_activity_type_t;

struct event_t {
Expand Down Expand Up @@ -99,6 +112,11 @@ struct event_t {
struct {
char name[XATTR_NAME_MAX_LEN];
} xattr;
struct {
unsigned int count;
unsigned int acl_type;
struct acl_entry_t entries[FACT_MAX_ACL_ENTRIES];
} acl;
};
};

Expand Down Expand Up @@ -143,4 +161,5 @@ struct metrics_t {
struct metrics_by_hook_t path_rmdir;
struct metrics_by_hook_t inode_setxattr;
struct metrics_by_hook_t inode_removexattr;
struct metrics_by_hook_t inode_set_acl;
};
3 changes: 3 additions & 0 deletions fact-ebpf/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,9 @@ impl_metrics_t!(
path_mkdir,
path_rmdir,
d_instantiate,
inode_setxattr,
inode_removexattr,
inode_set_acl,
);

unsafe impl Pod for metrics_t {}
Expand Down
29 changes: 23 additions & 6 deletions fact/src/bpf/checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use log::debug;

pub(super) struct Checks {
pub(super) path_hooks_support_bpf_d_path: bool,
pub(super) supports_inode_set_acl: bool,
}

impl Checks {
Expand All @@ -12,15 +13,31 @@ impl Checks {
.load(fact_ebpf::CHECKS_OBJ)
.context("Failed to load checks.o")?;

let prog = obj
.program_mut("check_path_unlink_supports_bpf_d_path")
.context("Failed to find 'check_path_unlink_supports_bpf_d_path' program")?;
let prog: &mut Lsm = prog.try_into()?;
let path_hooks_support_bpf_d_path = prog.load("path_unlink", btf).is_ok();
debug!("path_unlink_supports_bpf_d_path: {path_hooks_support_bpf_d_path}");
let path_hooks_support_bpf_d_path = Self::probe_hook(
&mut obj,
"check_path_unlink_supports_bpf_d_path",
"path_unlink",
btf,
);
debug!("path_hooks_support_bpf_d_path: {path_hooks_support_bpf_d_path}");

let supports_inode_set_acl =
Self::probe_hook(&mut obj, "check_inode_set_acl", "inode_set_acl", btf);
debug!("supports_inode_set_acl: {supports_inode_set_acl}");

Ok(Checks {
path_hooks_support_bpf_d_path,
supports_inode_set_acl,
})
}

fn probe_hook(obj: &mut aya::Ebpf, prog_name: &str, hook: &str, btf: &Btf) -> bool {
let Some(prog) = obj.program_mut(prog_name) else {
return false;
};
let Ok(prog): Result<&mut Lsm, _> = prog.try_into() else {
return false;
};
prog.load(hook, btf).is_ok()
}
}
32 changes: 22 additions & 10 deletions fact/src/bpf/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const RINGBUFFER_NAME: &str = "rb";

pub struct Bpf {
obj: Ebpf,
checks: Checks,

tx: mpsc::Sender<Event>,

Expand Down Expand Up @@ -64,6 +65,7 @@ impl Bpf {
let paths = Vec::new();
let mut bpf = Bpf {
obj,
checks,
tx,
paths,
paths_config,
Expand Down Expand Up @@ -178,28 +180,38 @@ impl Bpf {
let Some(hook) = name.strip_prefix("trace_") else {
bail!("Invalid hook name: {name}");
};

// Skip hooks that the kernel doesn't support
if hook == "inode_set_acl" && !self.checks.supports_inode_set_acl {
info!("Skipping {hook}: not supported on this kernel");
continue;
}

match prog {
Program::Lsm(prog) => prog.load(hook, btf)?,
u => unimplemented!("{u:?}"),
}
};
}
Ok(())
}

/// Attaches all BPF programs. If any attach fails, all previously
/// attached programs are automatically detached via drop.
/// Attaches all loaded BPF programs. Programs that were not loaded
/// (e.g. optional hooks on unsupported kernels) are skipped.
/// If any attach fails, all previously attached programs are
/// automatically detached via drop.
fn attach_progs(&mut self) -> anyhow::Result<()> {
self.links = self
.obj
.programs_mut()
.map(|(_, prog)| match prog {
for (_, prog) in self.obj.programs_mut() {
match prog {
Program::Lsm(prog) => {
if prog.fd().is_err() {
continue;
}
let link_id = prog.attach()?;
prog.take_link(link_id)
self.links.push(prog.take_link(link_id)?);
}
u => unimplemented!("{u:?}"),
})
.collect::<Result<_, _>>()?;
}
}
Comment on lines +203 to +214

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The new approach is fine, but if you wanted to keep the functional one you could do so using filter_map like this:

        self.links = self
            .obj
            .programs_mut()
            .filter_map(|(_, prog)| match prog {
                Program::Lsm(prog) => {
                    if prog.fd().is_err() {
                        return None;
                    }
                    let link_id = match prog.attach() {
                        Ok(link_id) => link_id,
                        Err(e) => return Some(Err(e)),
                    };
                    Some(prog.take_link(link_id))
                }
                u => unimplemented!("{u:?}"),
            })
            .collect::<Result<_, _>>()?;

There is also one small change that I think won't matter, but the new approach does not drop any existing links from the vector before pushing the new ones, we might want to clear the vector just in case if we want to keep this new approach.

Ok(())
}

Expand Down
Loading
Loading