From 505440b2ab09ecc94e629d2989a9502c55f90ef5 Mon Sep 17 00:00:00 2001 From: Anastasios Papagiannis Date: Tue, 1 Aug 2023 13:23:05 +0000 Subject: [PATCH 1/2] selectors: Fix file argument in matchArgs In https://raw.githubusercontent.com/cilium/tetragon/main/examples/tracingpolicy/file_monitoring.yaml Prefix does not seem to work as expected. This means that we also get events with different prefix compared to /etc/. The issues for that is that we do not calculate correctly the offsets inside the buffer of the specific argument. Now we use structs to make that easier to read and argue about. Signed-off-by: Anastasios Papagiannis --- bpf/process/types/basic.h | 41 ++++++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/bpf/process/types/basic.h b/bpf/process/types/basic.h index f39bb87e1ec..056851cb533 100644 --- a/bpf/process/types/basic.h +++ b/bpf/process/types/basic.h @@ -649,17 +649,17 @@ filter_char_buf(struct selector_arg_filter *filter, char *args, int value_off) return 0; } +struct string_buf { + __u32 len; + char buf[]; +}; + static inline __attribute__((always_inline)) long -__filter_file_buf(char *value, char *args, __u32 op) +__filter_file_buf(struct string_buf *value, struct string_buf *args, __u32 op) { + __u32 v = value->len, a = args->len; int err; - __u64 v, a; - /* filter->vallen is pulled from user input so we also need to - * ensure its bounded. - */ - v = (unsigned int)value[0]; - a = (unsigned int)args[0]; /* There are cases where file pointer may not contain a path. * An example is using an unnamed pipe. This is not a match. */ @@ -672,39 +672,48 @@ __filter_file_buf(char *value, char *args, __u32 op) if (a < v) goto skip_string; } else if (op == op_filter_str_postfix) { - err = rcmpbytes(&value[4], &args[4], v - 1, a - 1); + err = rcmpbytes(value->buf, args->buf, v - 1, a - 1); if (!err) return 0; goto skip_string; } - err = cmpbytes(&value[4], &args[4], v); + err = cmpbytes(value->buf, args->buf, v); if (!err) return 0; skip_string: - return v + 4; + return v + sizeof(struct string_buf); } +struct file_buf { + __u32 index; // index of filter + __u32 op; // op_filter_eq, op_filter_str_prefix, or op_filter_str_postfix + __u32 total_sz; // size of all values (len + bytes) + sizeof(total_sz) + sizeof(type) + __u32 type; // path_ty, file_ty, or fd_ty +}; + /* filter_file_buf: runs a comparison between the file path in args against the * filter file path. For 'equal' and 'prefix' operators we compare the file path * and the filter file path in the normal order. For the 'postfix' operator we do * a reverse search. */ static inline __attribute__((always_inline)) long -filter_file_buf(struct selector_arg_filter *filter, char *args) +filter_file_buf(struct file_buf *filter, char *args) { - char *value = (char *)&filter->value; + char *values = (char *)filter + sizeof(struct file_buf); // all values are after the header + __u32 remain = filter->total_sz - sizeof(__u32) - sizeof(__u32); int i, next; #ifndef __LARGE_BPF_PROG #pragma unroll #endif for (i = 0; i < MAX_MATCH_FILE_VALUES; ++i) { - next = __filter_file_buf(value, args, filter->op); + next = __filter_file_buf((struct string_buf *)values, (struct string_buf *)args, filter->op); if (!next) return 1; - else if (next + 8 > filter->vallen) + remain -= next; + if (remain == 0) return 0; - value += (next & 0x7f); + values += (next & 0x7f); } return 0; @@ -1350,7 +1359,7 @@ selector_arg_offset(__u8 *f, struct msg_generic_kprobe *e, __u32 selidx, args += 4; case file_ty: case path_ty: - pass &= filter_file_buf(filter, args); + pass &= filter_file_buf((struct file_buf *)filter, args); break; case string_type: /* for strings, we just encode the length */ From abfbc80e1d689dbc89492260a88ad5c46cc079d9 Mon Sep 17 00:00:00 2001 From: Anastasios Papagiannis Date: Tue, 1 Aug 2023 14:10:27 +0000 Subject: [PATCH 2/2] testing: Add file-monitoring test case This test uses the tracing policy described https://raw.githubusercontent.com/cilium/tetragon/main/examples/tracingpolicy/file_monitoring.yaml. Signed-off-by: Anastasios Papagiannis --- pkg/sensors/tracing/kprobe_test.go | 60 ++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/pkg/sensors/tracing/kprobe_test.go b/pkg/sensors/tracing/kprobe_test.go index e9179f19203..ab3c523c5a4 100644 --- a/pkg/sensors/tracing/kprobe_test.go +++ b/pkg/sensors/tracing/kprobe_test.go @@ -3150,6 +3150,66 @@ func TestKprobeMatchArgsFdPrefix(t *testing.T) { assert.NoError(t, err) } +func getMatchArgsFileFIMCrd(vals []string) string { + configHook := `apiVersion: cilium.io/v1alpha1 +kind: TracingPolicy +metadata: + name: "file-monitoring" +spec: + kprobes: + - call: "security_file_permission" + syscall: false + return: false + args: + - index: 0 + type: "file" + - index: 1 + type: "int" + selectors: + - matchArgs: + - index: 0 + operator: "Prefix" + values: ` + for _, f := range vals { + configHook += fmt.Sprintf("\n - \"%s\"", f) + } + return configHook +} + +func TestKprobeMatchArgsFileMonitoringPrefix(t *testing.T) { + var doneWG, readyWG sync.WaitGroup + defer doneWG.Wait() + + ctx, cancel := context.WithTimeout(context.Background(), tus.Conf().CmdWaitTime) + defer cancel() + + createCrdFile(t, getMatchArgsFileFIMCrd([]string{"/etc/"})) + + obs, err := observer.GetDefaultObserverWithFile(t, ctx, testConfigFile, tus.Conf().TetragonLib, observer.WithMyPid()) + if err != nil { + t.Fatalf("GetDefaultObserverWithFile error: %s", err) + } + observer.LoopEvents(ctx, t, &doneWG, &readyWG, obs) + readyWG.Wait() + + kpCheckers := make([]ec.EventChecker, len(allFiles)) + for i, f := range allFiles { + readFile(t, f) + kpCheckers[i] = ec.NewProcessKprobeChecker(""). + WithFunctionName(sm.Full("security_file_permission")). + WithArgs(ec.NewKprobeArgumentListMatcher(). + WithOperator(lc.Ordered). + WithValues( + ec.NewKprobeArgumentChecker().WithFileArg(ec.NewKprobeFileChecker().WithPath(sm.Full(f))), + ec.NewKprobeArgumentChecker().WithIntArg(4), // MAY_READ + )) + } + + checker := ec.NewUnorderedEventChecker(kpCheckers...) + err = jsonchecker.JsonTestCheck(t, checker) + assert.NoError(t, err) +} + func getMatchBinariesCrd(opStr string, vals []string) string { configHook := `apiVersion: cilium.io/v1alpha1 kind: TracingPolicy