Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data filter in kernel #4324

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

rscampos
Copy link
Contributor

@rscampos rscampos commented Sep 24, 2024

1. Explain what the PR does

3717704 Tracee data filter equalities
0edc416 eBPF data filter (user-space)
48cc10b Enable data filter in eBPF program
f435287 eBPF data filter (kernel-space)
6ca82e6 Enable BPF_F_NO_PREALLOC for LPM TRIE

3717704 Tracee data filter equalities

- method equalities created for data filter;
- disable data filter (only pathname) for selected events.

0edc416 eBPF data filter (user-space)

- eBPF map definition for prefix, postfix and exactly match;
- create updateDataFilterLPMBPF and updateDataFilterBPF to populate eBPF
  maps;
- config map fields for prefix, postfix and exactly.

48cc10b Enable data filter in eBPF program

- how to enable data filter in the eBPF program using the function
evaluate_data_filters.

f435287 eBPF data filter (kernel-space)

- function load_str_from_buf created to retrieve str value based on index;
- function reverse_string created to revert the pathname in order to enable postfix;
- function evaluate_data_filters/match_data_filters created to apply: prefix, postfix and exactly match;
- eBPF maps for prefix, postfix and exactly. eBPF map for hold temporary LPM TRI key;
- add fields in config_map for prefix, postfix and exactly match.

2. Explain how to test it

The method for defining data filters in Tracee remains the same. However, for the security_file_open and magic_write events, if the pathname is used as a filter, the event is now filtered at the eBPF data plane, preventing it from being sent to user space for filtering.

Prefix:
Tracee:

sudo ./dist/tracee -e security_file_open.data.pathname=/etc/hosts*

Cmd:

cat /etc/hosts.allow
cat /etc/hosts.deny

Exactly:
Tracee:

sudo ./dist/tracee -e security_file_open.data.pathname=/etc/netconfig

Cmd:

cat /etc/netconfig

Postfix:
Tracee:

sudo ./dist/tracee -e security_file_open.data.pathname=*config

Cmd:

cat /etc/netconfig

3. Other comments

Next steps:

  1. Evaluate and combine all three types of string-based filters simultaneously. Currently, they work separately but not together.
  2. Improve the way we define (in user-space) which events should have an in-kernel filter enabled. Some parts of the logic I added were just for a PoC, so this needs to be reworked.
  3. Currently, we explicitly define the index in evaluate_data_filters to retrieve the pathname for string-based filtering. This works for now, but it would be better to dynamically retrieve the index based on the event ID.

- function load_str_from_buf created to retrieve str value based on index;
- function reverse_string created to revert the pathname in order to enable postfix;
- function evaluate_data_filters/match_data_filters created to apply: prefix, postfix and exactly match;
- eBPF maps for prefix, postfix and exactly. eBPF map for hold temporary LPM TRI key;
- add fields in config_map for prefix, postfix and exactly match.
- how to enable data filter in the eBPF program using the function
evaluate_data_filters.
- eBPF map definition for prefix, postfix and exactly match;
- create updateDataFilterLPMBPF and updateDataFilterBPF to populate eBPF
  maps;
- config map fields for prefix, postfix and exactly.
- method equalities created for data filter;
- disable data filter (only pathname) for selected events.
Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

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

This is just a first pass (skimming) review.

Tested and working with:

sudo ./dist/tracee -e security_file_open.data.pathname=/etc/passwd
sudo ./dist/tracee -e security_file_open.data.pathname='/etc/passwd*'
sudo ./dist/tracee -e security_file_open.data.pathname='*passwd-'

Amazing, @rscampos! 👏🏼

@@ -73,3 +73,12 @@ func Max(x, y uint64) uint64 {
func GenerateRandomDuration(min, max int) time.Duration {
return time.Duration(rand.Intn(max-min+1)+min) * time.Second
}

func ReverseString(s string) string {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this could be placed in the helpers pkg?

@@ -706,6 +829,7 @@ func (ps *policies) computePoliciesConfig() *PoliciesConfig {
}

cfg.EnabledScopes |= 1 << offset
cfg.EnabledDataFilters |= 1 << offset
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be?

Suggested change
cfg.EnabledDataFilters |= 1 << offset
cfg.EnabledDataFilters |= (ExactlyEnabledDataFilters | PrefixEnabledDataFilters | cfg.PostfixEnabledDataFilters) & (1 << offset)

@@ -121,6 +211,12 @@ func (af *DataFilter) Parse(filterName string, operatorAndValues string, eventsN
// before the filter is applied
valueHandler := func(val string) (string, error) {
switch id {
case events.SecurityFileOpen,
Copy link
Member

Choose a reason for hiding this comment

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

There's duplicate logic in here from EnableKernel().

@@ -37,6 +116,17 @@ func (af *DataFilter) Filter(eventID events.ID, data []trace.Argument) bool {
return true
}

// don't Filter this special case
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of checking a kernelFilter flag instead (it could be set on Parse)?

Comment on lines +75 to +77
if filterMap, ok := af.filters[eventID]; ok {
if pathName, ok := filterMap[dataField]; ok {
if filter, ok := pathName.(*StringFilter); ok {
Copy link
Member

Choose a reason for hiding this comment

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

We can easily use the happy path here.

@@ -284,6 +284,17 @@ typedef struct path_filter {
char path[MAX_PATH_PREF_SIZE];
} path_filter_t;

typedef struct data_filter_key {
u32 eventid;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
u32 eventid;
u32 event_id;

nit: I know that there's still some eventid, but it would be nice to standardise them as well.

} data_filter_key_t;

typedef struct data_filter_lpm_key {
u32 prefixlen;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
u32 prefixlen;
u32 prefix_len;

@@ -2209,6 +2209,9 @@ int BPF_KPROBE(trace_security_file_open)
save_to_submit_buf(&p.event->args_buf, &ctime, sizeof(u64), 4);
save_str_to_buf(&p.event->args_buf, syscall_pathname, 5);

if (!evaluate_data_filters(&p, 0))
Copy link
Member

Choose a reason for hiding this comment

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

We've already discussed about this. Just registering for possible improvements of the save_*to*_buf API.

I believe that save_str_to_buf(), for instance, could return the offset that it saved the string with a simple change. So that info should be propagated in the scrap buffers to be reused further as an argument to load_str_from_buf() and other helpers alike.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants