Add rules to the pipeline #2439

josedonizetti · 2022-12-06T21:14:35Z

Initial Checklist

This PRs adds the rule engine to the events pipeline.

How Has This Been Tested?

# compiles
make

# test single rule event
./dist/tracee --trace event=anti_debugging
# test event with output json
./dist/tracee --trace event=anti_debugging --output format:json

# test single event with extra non rule event
./dist/tracee --trace event=anti_debugging,execve

# test single event with non rule dependency event
./dist/tracee --trace event=ptrace,anti_debugging

# test all events
./dist/tracee  --trace event=anti_debugging,aslr_inspection,cgroup_notify_on_release,cgroup_release_agent,core_pattern_modification,default_loader_mod,disk_mount,docker_abuse,dynamic_code_loading,fileless_execution,hidden_file_created,illegitimate_shell,k8s_api_connection,k8s_cert_theft,kernel_module_loading,ld_preload,process_vm_write_inject,proc_fops_hooking,proc_kcore_read,proc_mem_access,proc_mem_code_injection,ptrace_code_injection,rcd_modification,sched_debug_recon,scheduled_task_mod,stdio_over_socket,sudoers_modification,syscall_hooking,system_request_key_mod

# test rule event non exported, won't work because the event isn't exported
./dist/tracee --output format:json --trace event=dropped_executable
{"level":"fatal","ts":1670495492.8543997,"msg":"app","error":"invalid event to trace: dropped_executable"}

# list all events, including those that are rules
./dist/tracee --list

josedonizetti · 2022-12-06T23:22:26Z

@NDStrahilevitz I tried to address all your comments from the previous PR #2374, let me know if anything passed by.

pkg/rules/engine/engine.go

pkg/events/events.go

pkg/ebpf/events_pipeline.go

pkg/ebpf/events_engine.go

geyslan

First review pass: put some comments.

pkg/ebpf/finding.go

geyslan

Tested and LGTM. Great work!

rafaeldtinoco · 2022-12-10T22:30:02Z

You're missing alignment in the "list" command. Also, a line jump in between the last signature events and System Calls events.

Also: maybe you should change "Events" to "Signature Events" so they are not confused. At least for now?

rafaeldtinoco

I left some comments, doubts and nits for you. Nice work. Let's do another round after some answers and talks. Cheers!

cmd/tracee/main.go

pkg/ebpf/finding.go

pkg/events/events.go

signatures/rego/anti_debugging_ptraceme.rego

pkg/ebpf/finding.go

rafaeldtinoco · 2022-12-11T03:15:38Z

pkg/ebpf/events_engine.go

+		for {
+			select {
+			case finding := <-engineOutput:
+				event, err := FindingToEvent(finding)


Be aware that eventually you will have to call:

if !t.shouldProcessEvent(event) { continue }

in here before sending event to "out" channel. This will happen because the user might have choosen the event (Which is a signature) but they might be filtering the output of such event through args, for example. For now you're using the original event args, but eventually it will be the "finding data", right ? Then you might "filter" out detections as well, just like other events.

@rafaeldtinoco I hadn't considered having finding data as args, but I agree it is weird indeed to keep the original event data, need to think about it though. Do you think it is mandatory for this PR, or can be done in a follow-up for the next release?

If the feature will be a "tech preview" I think it is fine do have an issue and mark it for the next release (considering we are out of time for this one).

It won't be the default solution, for now, the only way to use it will be purposely choose it, also will make sure to note it as experimental on the release notes/docs.

yanivagman

Nice work @josedonizetti!

cmd/tracee/main.go

pkg/ebpf/events_engine.go

yanivagman · 2022-12-08T16:20:43Z

pkg/ebpf/events_engine.go

+	go func() {
+		defer close(done)
+
+		<-ctx.Done()
+		done <- true
+	}()


Maybe we can pass ctx to the engine as well instead of having this new done channel?

Yup, I also want to do it, didn't do it here because I want to avoid changing tracee-rules. I'll create a follow-up task as a refactor. Ok?

NDStrahilevitz · 2022-12-11T12:41:37Z

cmd/tracee/main.go

@@ -54,6 +88,15 @@ func main() {
 				return err
 			}

+			// parse arguments must be enabled if the rule engine is part of the pipeline
+			runner.TraceeConfig.Output.ParseArguments = true


note: we should eventually get rid of this (see #2177)

NDStrahilevitz · 2022-12-11T12:46:36Z

cmd/tracee/main_test.go

+	}
+}
+
+type fakeSignature struct {


I think we have a lot of duplicates of this mock signature, maybe we should export one at the signature package?

LIke the idea, will create a task as a follow-up refactor, ok?

rafaeldtinoco

All good on my side. Will let you handle the rest of the comments with others, but +1'ing on my side.

yanivagman

LGTM

josedonizetti force-pushed the add-rules-to-the-pipeline branch from b3ea657 to 0eb7c2e Compare December 6, 2022 22:45

josedonizetti requested review from NDStrahilevitz, yanivagman, geyslan and rafaeldtinoco and removed request for NDStrahilevitz December 6, 2022 23:21

josedonizetti marked this pull request as ready for review December 6, 2022 23:21