From 79aa938d899c451fed517005c22d00cb03f4bad2 Mon Sep 17 00:00:00 2001 From: Alice Frosi Date: Fri, 31 Mar 2023 11:48:40 +0200 Subject: filter: fix filter An additional notification is need either when we jump from an instruction without arguments then at the end of the argument checks. --- cooker/filter.c | 99 +++++++++++++++++++++++++++++++----------------- tests/unit/test_filter.c | 74 +++++++++++++++++++++++++----------- tests/unit/testutil.h | 1 + tests/unit/util.c | 15 ++++++++ 4 files changed, 133 insertions(+), 56 deletions(-) diff --git a/cooker/filter.c b/cooker/filter.c index 717e525..350d6bc 100644 --- a/cooker/filter.c +++ b/cooker/filter.c @@ -172,43 +172,63 @@ static unsigned get_n_args_syscall(const struct syscall_entry *table) return n; } -static unsigned int get_total_args_instr(const struct syscall_entry table[], - unsigned int n_syscall) +static unsigned int get_n_args_syscall_instr(const struct syscall_entry *table) { - unsigned i, k, j, n = 0, total_instr = 0; - const struct syscall_entry *t = &table[0]; const struct bpf_call *entry; - - for (j = 0; j < n_syscall; j++) { - t = &table[j]; - for (i = 0; i < t->count; i++) { - entry = t->entry + i; - n = 0; - /* For every argument there are 2 instructions, one to - * load the value and the second to evaluate the - * argument - */ - for (k = 0; k < 6; k++) { - if (entry->check_arg[k]) - n += 2; + bool has_arg = false; + unsigned n = 0, total_instr = 0; + + for (unsigned int i = 0; i < table->count; i++) { + entry = table->entry + i; + n = 0; + + /* For every argument there are 2 instructions, one to + * load the value and the second to evaluate the + * argument + */ + for (unsigned int k = 0; k < 6; k++) { + if (entry->check_arg[k]) { + n += 2; } - total_instr += n; - /* If there is at least an arguments then there is an additional - * instruction for the notification - */ - if (n > 0) - total_instr++; + } + total_instr += n; + if (n > 0) { + has_arg = true; + total_instr++; } } + if (has_arg) + total_instr++; + return total_instr; } +static unsigned int get_total_args_instr(const struct syscall_entry table[], + unsigned int n_syscall) +{ + unsigned i, n = 0; + + for (i = 0; i < n_syscall; i++) { + n += get_n_args_syscall_instr(&table[i]); + } + return n; +} + static bool check_args_syscall_entry(const struct bpf_call *entry){ return entry->check_arg[0] || entry->check_arg[1] || entry->check_arg[2] || entry->check_arg[3] || entry->check_arg[4] || entry->check_arg[5]; } +static bool check_args_syscall(const struct syscall_entry *table) +{ + for (unsigned int i = 0; i < table->count; i++) { + if (check_args_syscall_entry(table->entry + i)) + return true; + } + return false; +} + unsigned int create_bpf_program_log(struct sock_filter filter[]) { filter[0] = (struct sock_filter)BPF_STMT( @@ -232,20 +252,22 @@ unsigned int create_bfp_program(struct syscall_entry table[], unsigned int n_args, n_nodes; unsigned int notify, accept; unsigned int i, j, k, size; - unsigned int next_offset; + unsigned int next_offset, offset; unsigned int next_args_off; int nodes[MAX_JUMPS]; create_lookup_nodes(nodes, n_syscall); + /* First 3 checks */ size = 3; /* No nodes if there is a single syscall */ n_nodes = (1 << count_shift_right(n_syscall - 1)) - 1; n_args = get_total_args_instr(table, n_syscall); - accept = 2 + n_nodes + 2 * n_syscall + n_args + 1; - notify = 2 + n_nodes + 2 * n_syscall + n_args + 2; + /* pre-check instruction + load syscall number (4 instructions) */ + accept = 3 + n_nodes + n_syscall + n_args; + notify = accept + 1; /* Pre */ /* cppcheck-suppress badBitmaskCheck */ @@ -274,8 +296,15 @@ unsigned int create_bfp_program(struct syscall_entry table[], next_offset = n_syscall - 1; /* Insert leaves */ for (i = 0; i < n_syscall; i++) { - filter[size++] = (struct sock_filter)EQ( - table[i].nr, next_offset, accept - size); + /* If the syscall doesn't have any arguments, jump directly to + * the notification + */ + if (check_args_syscall(&table[i])) + offset = next_offset; + else + offset = notify - size - 1; + filter[size++] = (struct sock_filter)EQ(table[i].nr, offset, + accept - size); next_offset += get_n_args_syscall(&table[i]); } @@ -285,11 +314,12 @@ unsigned int create_bfp_program(struct syscall_entry table[], * check the following entry of the syscall and its arguments. */ for (i = 0; i < n_syscall; i++) { + bool has_arg = false; for (j = 0; j < (table[i]).count; j++) { unsigned n_checks = 0; entry = table[i].entry + j; next_args_off = get_n_args_syscall_entry(entry); - for (k = 0; k < 6; k++) + for (k = 0; k < 6; k++) { if (entry->check_arg[k]) { filter[size++] = (struct sock_filter) LOAD((offsetof( @@ -299,18 +329,19 @@ unsigned int create_bfp_program(struct syscall_entry table[], (table[i].entry + j)->args[k], 0, next_args_off - n_checks); n_checks++; + has_arg = true; } - /* If there is at least an argument, it needs to notify - * if all the arguments checks have passed. - */ - if (check_args_syscall_entry(entry)) + } + if (check_args_syscall_entry(table[i].entry)) filter[size++] = (struct sock_filter)JUMPA( notify - size); } /* At this point none of the checks was positive, it jumps to * the default behavior */ - filter[size++] = (struct sock_filter)JUMPA(accept - size); + if (has_arg) + filter[size++] = + (struct sock_filter)JUMPA(accept - size); } /* Seccomp accept and notify instruction */ diff --git a/tests/unit/test_filter.c b/tests/unit/test_filter.c index 5ff9d65..9583b56 100644 --- a/tests/unit/test_filter.c +++ b/tests/unit/test_filter.c @@ -6,21 +6,9 @@ #define _GNU_SOURCE #include #include -#include -#include -#include -#include -#include -#include #include -#include -#include -#include -#include -#include #include -#include -#include +#include #include @@ -28,52 +16,94 @@ #include "common.h" #include "testutil.h" #include "filter.h" +#include "disasm.h" static int generate_install_filter(struct args_target *at) { - struct bpf_call calls[] = { {} }; + unsigned int i; + struct bpf_call calls[1]; struct syscall_entry table[] = { { .count = 1, .nr = at->nr, .entry = &calls[0] } }; struct sock_filter filter[30]; unsigned int size; + for (i = 0; i < 6; i++) { + if (at->args[i] != NULL) { + calls[0].args[i] = (int)at->args[i]; + calls[0].check_arg[i] = true; + } else { + calls[0].check_arg[i] = false; + } + } size = create_bfp_program(table, filter, 1); + //bpf_disasm_all(filter, size); return install_filter(filter, size); } -void setup_build_filter() +START_TEST(no_args) { at = mmap(NULL, sizeof(struct args_target), PROT_READ | PROT_WRITE, MAP_SHARED | MAP_ANONYMOUS, -1, 0); at->check_fd = false; at->nr = __NR_getpid; - at->args[0] = NULL; at->install_filter = generate_install_filter; setup(); + mock_syscall_target(); } +END_TEST -START_TEST(filter) +START_TEST(with_getsid) { - continue_target(); + int id = 12345; + at = mmap(NULL, sizeof(struct args_target), PROT_READ | PROT_WRITE, + MAP_SHARED | MAP_ANONYMOUS, -1, 0); + at->check_fd = false; + at->nr = __NR_getsid; + at->args[0] = &id; + at->install_filter = generate_install_filter; + setup(); + mock_syscall_target(); } END_TEST +START_TEST(with_getpriority) +{ + int which = 12345; + id_t who = PRIO_PROCESS; + at = mmap(NULL, sizeof(struct args_target), PROT_READ | PROT_WRITE, + MAP_SHARED | MAP_ANONYMOUS, -1, 0); + at->check_fd = false; + at->nr = __NR_getpriority; + at->args[0] = &which; + at->args[1] = &who; + at->install_filter = generate_install_filter; + setup(); + mock_syscall_target(); +} +END_TEST Suite *op_call_suite(void) { Suite *s; int timeout = 30; - TCase *simple; + TCase *simple, *args32; s = suite_create("Test filter with target"); - simple = tcase_create("simple"); - tcase_add_checked_fixture(simple, setup_build_filter, teardown); + simple = tcase_create("no args"); + tcase_add_checked_fixture(simple, NULL, teardown); tcase_set_timeout(simple, timeout); - tcase_add_test(simple, filter); + tcase_add_test(simple, no_args); suite_add_tcase(s, simple); + args32 = tcase_create("with args 32 bit"); + tcase_add_checked_fixture(args32, NULL, teardown); + tcase_set_timeout(args32, timeout); + tcase_add_test(args32, with_getsid); + tcase_add_test(args32, with_getpriority); + suite_add_tcase(s, args32); + return s; } diff --git a/tests/unit/testutil.h b/tests/unit/testutil.h index d4f83af..dd4f1e9 100644 --- a/tests/unit/testutil.h +++ b/tests/unit/testutil.h @@ -42,5 +42,6 @@ void setup(); void teardown(); int install_notification_filter(struct args_target *at); void continue_target(); +void mock_syscall_target(); #endif /* TESTUTIL_H */ diff --git a/tests/unit/util.c b/tests/unit/util.c index c6fc3fb..5a1c5aa 100644 --- a/tests/unit/util.c +++ b/tests/unit/util.c @@ -170,6 +170,21 @@ void continue_target() ck_assert_msg(ret == 0, strerror(errno)); } +void mock_syscall_target() +{ + struct seccomp_notif_resp resp; + int ret; + + ret = ioctl(notifyfd, SECCOMP_IOCTL_NOTIF_ID_VALID, &req.id); + ck_assert_msg(ret == 0, strerror(errno)); + resp.id = req.id; + resp.flags = 0; + resp.error = 0; + resp.val = 0; + ret = ioctl(notifyfd, SECCOMP_IOCTL_NOTIF_SEND, &resp); + ck_assert_msg(ret == 0, strerror(errno)); +} + void setup() { int ret; -- cgit v1.2.3