feat!: isolate modern probe TOCTOU mitigation logic #2576
Conversation
poiana
added
kind/feature
|
Please double check driver/API_VERSION file. See versioning. /hold |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2576 +/- ##
=======================================
Coverage 78.17% 78.17%
=======================================
Files 298 298
Lines 32098 32098
Branches 4693 4691 -2
=======================================
Hits 25092 25092
Misses 7006 7006
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ekoops
force-pushed
the
ekoops/isolate-modern-toctou
branch
2 times, most recently
from
1a55cba to
65adf8e
Compare
ekoops
changed the title
|
Not bumping the driver schema version is currently a practice we are following as we will bump it in a single shot once we are done with the #2068 proposal. |
Perf diff from master - unit testsHeap diff from master - unit testsHeap diff from master - scap fileBenchmarks diff from master |
ekoops
force-pushed
the
ekoops/isolate-modern-toctou
branch
from
65adf8e to
1c5d390
Compare
X64 kernel testing matrix
ARM64 kernel testing matrix
|
ekoops
left a comment
ekoops
left a comment
There was a problem hiding this comment.
Added some clarifying comments
| // We don't want to send DROP_E/DROP_X events from the enter tracepoint because it would requires us | ||
| // to create a dedicated tail table for the enter. It is enough to send DROP_E/DROP_X events from | ||
| // the exit tracepoint. | ||
| static __always_inline bool syscalls_dispatcher__sampling_logic_enter(uint32_t syscall_id) { |
There was a problem hiding this comment.
Moved from driver/modern_bpf/programs/attached/dispatchers/syscall_enter.bpf.c as it is shared by sys_enter dispatcher and TOCTOU mitigation progs.
| * @return never returns in case of success; otherwise, returns 0 | ||
| */ | ||
| static __always_inline int toctou_mitigation__call_prog( |
There was a problem hiding this comment.
Taken from sys_enter dispatcher logic
| return 0; | ||
| } | ||
|
|
||
| static __always_inline void toctou_mitigation__push_connect_enter_event( |
There was a problem hiding this comment.
This helper allows to share code between the TTM socketcall and the TTM connect programs.
| /* Extract syscall arguments from socketcall args pointer. */ | ||
| unsigned long args[3] = {0}; | ||
| void *args_pointer = (void *)ctx->args; | ||
| if(bpf_in_ia32_syscall()) { | ||
| // First read all arguments on 32 bits. | ||
| uint32_t args_u32[3] = {}; | ||
| bpf_probe_read_user(args_u32, 3 * sizeof(uint32_t), args_pointer); | ||
| for(int i = 0; i < 3; i++) { | ||
| args[i] = (unsigned long)args_u32[i]; | ||
| } | ||
| } else { | ||
| bpf_probe_read_user(args, 3 * sizeof(unsigned long), args_pointer); | ||
| } |
There was a problem hiding this comment.
This is a simplified version of the extract__network_args helper. We cannot reuse that helper as it expects the caller to have access to a registers pointer. For the moment, since this is the only place we use it, just inline the code.
There was a problem hiding this comment.
I'm just wondering if renaming ttm_socketcall_e as ttm_socketcall_connect_e would make it simpler to follow the implementation...
There was a problem hiding this comment.
I call it this way to follow the convention defined in the README, and also because in the future we can decide to use it to implement other socketcall-related mitigations (e.g.: accept).
There was a problem hiding this comment.
Makes sense. The reason I was thinking its name was that it currently assumes the parameter structure to be for connect and indeed it gets called only for connect (as socketcall_e makes sure). Perhaps then just stating in the comment:
/* Extract connect syscall arguments from socketcall args pointer. */
There was a problem hiding this comment.
Yes totally agree. If you agree, I'm gonna do it in the upcoming PR, as I guess it doesn't worth waiting for another entire CI cycle for this small comment update.
|
|
||
| typedef struct { | ||
| char* name; | ||
| enum bpf_prog_type prog_type; |
There was a problem hiding this comment.
Add the program type information, because now programs in userspace/libpman/src/events_prog_table.c can have two types: BPF_PROG_TYPE_RAW_TRACEPOINT (as before) and BPF_PROG_TYPE_TRACEPOINT (new TTM mitigation programs). This information is used in https://github.com/falcosecurity/libs/pull/2576/files#diff-da696150f59c988d4ffde0c0a853f1d258c0db247a2be3f543ed945bdf734603R45
| } | ||
|
|
||
| /* Enable desired tracepoints */ | ||
| if(sys_exit) { |
There was a problem hiding this comment.
No need to attach TOCTOU mitigation programs (dispatchers) if the sys_exit dispatcher is not attached. The reason behind this is that enter events, generated by the tail-called TOCTOU mitigation programs, are conceived to support exit events.
| if(sys_exit) { | ||
| sys_enter_connect = sc_set[PPM_SC_CONNECT]; | ||
| if(sys_enter_connect) { | ||
| sys_enter_socketcall = true; |
There was a problem hiding this comment.
socketcall TOCTOU mitigation program is only attach to support TOCTOU mitigation for connect.
|
LGTM label has been added. DetailsGit tree hash: 6f4c87257ce7735e3d4bdf9f06f9b51409230a9f |
|
@terror96: changing LGTM is restricted to collaborators DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| if(syscall_id == socketcall_syscall_id) { | ||
| syscall_id = convert_socketcall_call_to_syscall_id(socketcall_call); | ||
| if(syscall_id == -1) { | ||
| // We can't do anything since modern bpf filler jump table is syscall indexed. | ||
| return 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
I wonder if we should introduce explicit casting because we compare unsigned value against signed? But this is okay unless -Wsign-compare is used.
There was a problem hiding this comment.
Conversion is implicitly performed in this case 😄
ekoops
force-pushed
the
ekoops/isolate-modern-toctou
branch
from
1c5d390 to
f17e914
Compare
ekoops
force-pushed
the
ekoops/isolate-modern-toctou
branch
3 times, most recently
from
bfa0a4e to
bb5d4af
Compare
Current TOCTOU mitigation implementation leverages the enter events generated by eBPF programs which are tail-called by the `sys_enter` dispatcher. As the architecture is moving towards dropping enter event generation and collection, the `sys_enter` dispatcher will be ultimately removed. This requires to isolate TOCTOU mitigation handling logic in order to make it independent from `sys_enter` dispatcher removal. This patch moves TOCTOU mitigation handling logic into separate ad-hoc eBPF tracepoint programs attached on corresponding `syscalls/sys_enter_*` hooks. For each system call `<syscall>`, two eBPF programs are defined. These programs perform: - syscall events sampling/filtering - information gathering - enter event generation - enter submission to userspace The two program classes have the following naming schema and purpose: - `<syscall>_e` - attached to `tracepoint/syscalls/sys_enter_<syscall>` tracepoint hook; provide support for 64 bit system calls - `ia32_<syscall>_e` - attached to `fentry/__ia32_sys_<syscall>` fentry hook; provide support for ia32 emulated system calls Tail-called TOCTOU mitigation programs are inserted into a separate tail call map, called `syscall_enter_toctou_mitigation_tail_table`. As the tracepoint attachment procedure generates an `openat` exit event on `/sys/kernel/tracing/events/.../id` that would pollute the stream of events read by the probe, the implementation takes care of attaching them before attaching the `sys_exit` dispatcher. Notice that the new logic make the TOCTOU mitigation programs attachment dependent on the presence of the `sys_exit` dispatcher. BREAKING CHANGE: change prerequisites for attaching some enter progs Signed-off-by: Leonardo Di Giovanna <[email protected]>
ekoops
force-pushed
the
ekoops/isolate-modern-toctou
branch
from
bb5d4af to
bfad8ce
Compare
Molter73
left a comment
Molter73
left a comment
There was a problem hiding this comment.
/lgtm
Thanks for the effort put into this! I left a couple comments but most are cleanups and nitpicks.
| int socketcall_syscall_id; | ||
|
|
||
| #ifdef __NR_socketcall | ||
| socketcall_syscall_id = __NR_socketcall; | ||
| #else | ||
| socketcall_syscall_id = -1; | ||
| #endif |
There was a problem hiding this comment.
[nitpick]
| int socketcall_syscall_id; | |
| #ifdef __NR_socketcall | |
| socketcall_syscall_id = __NR_socketcall; | |
| #else | |
| socketcall_syscall_id = -1; | |
| #endif | |
| #ifdef __NR_socketcall | |
| int socketcall_syscall_id = __NR_socketcall; | |
| #else | |
| int socketcall_syscall_id = -1; | |
| #endif |
| // Convert the socketcall id into the network syscall id. | ||
| // In this way the syscall will be treated exactly as the original one. | ||
| if(syscall_id == socketcall_syscall_id) { | ||
| syscall_id = convert_socketcall_call_to_syscall_id(socketcall_call); | ||
| if(syscall_id == -1) { | ||
| // We can't do anything since modern bpf filler jump table is syscall indexed. | ||
| return 1; | ||
| } | ||
| } | ||
|
|
||
| if(!syscalls_dispatcher__64bit_interesting_syscall(syscall_id)) { | ||
| return 1; | ||
| } | ||
|
|
||
| if(syscalls_dispatcher__sampling_logic_enter(syscall_id)) { | ||
| return 1; | ||
| } | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
Should this code be moved into the #if defined(__TARGET_ARCH_x86) block? Since the #else for that block will just return 1;
| #if defined(__NR_connect) || defined(__NR_creat) || defined(__NR_open) || defined(__NR_openat) | ||
| #ifdef __NR_connect | ||
| case __NR_connect: | ||
| #endif // __NR_connect | ||
| #ifdef __NR_creat | ||
| case __NR_creat: | ||
| #endif // __NR_creat | ||
| #ifdef __NR_open | ||
| case __NR_open: | ||
| #endif // __NR_open | ||
| #ifdef __NR_openat | ||
| case __NR_openat: | ||
| #endif // __NR_openat | ||
| return 0; | ||
| #endif // __NR_connect ||__NR_creat || __NR_open || __NR_openat |
There was a problem hiding this comment.
[nitpick] This can be a bit more compact and readable in this way IMO
| #if defined(__NR_connect) || defined(__NR_creat) || defined(__NR_open) || defined(__NR_openat) | |
| #ifdef __NR_connect | |
| case __NR_connect: | |
| #endif // __NR_connect | |
| #ifdef __NR_creat | |
| case __NR_creat: | |
| #endif // __NR_creat | |
| #ifdef __NR_open | |
| case __NR_open: | |
| #endif // __NR_open | |
| #ifdef __NR_openat | |
| case __NR_openat: | |
| #endif // __NR_openat | |
| return 0; | |
| #endif // __NR_connect ||__NR_creat || __NR_open || __NR_openat | |
| #ifdef __NR_connect | |
| case __NR_connect: return 0; | |
| #endif // __NR_connect | |
| #ifdef __NR_creat | |
| case __NR_creat: return 0; | |
| #endif // __NR_creat | |
| #ifdef __NR_open | |
| case __NR_open: return 0; | |
| #endif // __NR_open | |
| #ifdef __NR_openat | |
| case __NR_openat: return 0; | |
| #endif // __NR_openat |
| /* Extract connect syscall arguments from registers. */ | ||
| void *args_pointer = (void *)extract__syscall_argument(regs, 1); | ||
| unsigned long args[3] = {0}; | ||
| uint32_t args_u32[3] = {}; |
There was a problem hiding this comment.
[nitpick] Just for consistency.
| uint32_t args_u32[3] = {}; | |
| uint32_t args_u32[3] = {0}; |
| /** | ||
| * @brief Attach only the sys_enter_socketcall tracepoint | ||
| * | ||
| * @return `0` on success, `errno` in case of error. | ||
| */ | ||
| int pman_attach_sys_enter_socketcall(void); | ||
|
|
||
| /** | ||
| * @brief Detach only the sys_enter_socketcall tracepoint | ||
| * | ||
| * @return `0` on success, `errno` in case of error. | ||
| */ | ||
| int pman_detach_sys_enter_socketcall(void); | ||
|
|
||
| /** | ||
| * @brief Attach only the sys_enter_connect tracepoint | ||
| * | ||
| * @return `0` on success, `errno` in case of error. | ||
| */ | ||
| int pman_attach_sys_enter_connect(void); | ||
|
|
||
| /** | ||
| * @brief Detach only the sys_enter_connect tracepoint | ||
| * | ||
| * @return `0` on success, `errno` in case of error. | ||
| */ | ||
| int pman_detach_sys_enter_connect(void); | ||
|
|
||
| /** | ||
| * @brief Attach only the sys_enter_creat tracepoint | ||
| * | ||
| * @return `0` on success, `errno` in case of error. | ||
| */ | ||
| int pman_attach_sys_enter_creat(void); | ||
|
|
||
| /** | ||
| * @brief Detach only the sys_enter_creat tracepoint | ||
| * | ||
| * @return `0` on success, `errno` in case of error. | ||
| */ | ||
| int pman_detach_sys_enter_creat(void); | ||
|
|
||
| /** | ||
| * @brief Attach only the sys_enter_open tracepoint | ||
| * | ||
| * @return `0` on success, `errno` in case of error. | ||
| */ | ||
| int pman_attach_sys_enter_open(void); | ||
|
|
||
| /** | ||
| * @brief Detach only the sys_enter_open tracepoint | ||
| * | ||
| * @return `0` on success, `errno` in case of error. | ||
| */ | ||
| int pman_detach_sys_enter_open(void); | ||
|
|
||
| /** | ||
| * @brief Attach only the sys_enter_openat tracepoint | ||
| * | ||
| * @return `0` on success, `errno` in case of error. | ||
| */ | ||
| int pman_attach_sys_enter_openat(void); | ||
|
|
||
| /** | ||
| * @brief Detach only the sys_enter_openat tracepoint | ||
| * | ||
| * @return `0` on success, `errno` in case of error. | ||
| */ | ||
| int pman_detach_sys_enter_openat(void); | ||
|
|
||
| /** | ||
| * @brief Attach only the sys_enter_openat2 tracepoint | ||
| * | ||
| * @return `0` on success, `errno` in case of error. | ||
| */ | ||
| int pman_attach_sys_enter_openat2(void); | ||
|
|
||
| /** | ||
| * @brief Detach only the sys_enter_openat2 tracepoint | ||
| * | ||
| * @return `0` on success, `errno` in case of error. | ||
| */ | ||
| int pman_detach_sys_enter_openat2(void); | ||
|
|
There was a problem hiding this comment.
Maybe a question for a wider PR, but do we need all of these definitions here? Usually having a header file for a library I would expect the definitions in there to be the publicly accessible part of the library, I don't think the intent would be that a user of libpman (like libscap) will want to call pman_detach_sys_enter_openat2(), but rather use something more generic and these would be lower level implementation details that live inside the library itself. IDK, I may be wrong here.
| pman_print_error("failed to load BPF object"); | ||
| return errno; | ||
| } | ||
| printf("AFTER LOADING\n"); |
There was a problem hiding this comment.
| printf("AFTER LOADING\n"); |
| const event_prog_t* enter_prog = | ||
| (const event_prog_t*)&event_prog_table[enter_event_type]; | ||
| const char* enter_prog_name = enter_prog->name; | ||
| if(!enter_prog_name) { |
There was a problem hiding this comment.
[nitpick] Checking for NULL is clearer IMO. I also believe is more correct from a type point of view, ! I would argue is for boolean operations, not pointers (and I know this is not how C works, I still just think this makes more sense).
| if(!enter_prog_name) { | |
| if(enter_prog_name == NULL) { |
| const event_prog_t* enter_prog = | ||
| (const event_prog_t*)&event_prog_table[enter_event_type]; | ||
| const char* enter_prog_name = enter_prog->name; | ||
| if(!enter_prog_name) { | ||
| enter_prog = (const event_prog_t*)&event_prog_table[PPME_GENERIC_E]; | ||
| enter_prog_name = enter_prog->name; | ||
| } | ||
|
|
||
| /* No programs other tail raw tracepoints are currently tail-called by the sys_enter | ||
| * dispatcher. */ | ||
| if(enter_prog->prog_type == BPF_PROG_TYPE_RAW_TRACEPOINT) { | ||
| if(add_bpf_program_to_tail_table(syscall_enter_tail_table_fd, | ||
| enter_prog_name, | ||
| syscall_id)) { | ||
| goto clean_fill_syscalls_tail_table; | ||
| } |
There was a problem hiding this comment.
I don't think we need enter_prog_name here.
| const event_prog_t* enter_prog = | |
| (const event_prog_t*)&event_prog_table[enter_event_type]; | |
| const char* enter_prog_name = enter_prog->name; | |
| if(!enter_prog_name) { | |
| enter_prog = (const event_prog_t*)&event_prog_table[PPME_GENERIC_E]; | |
| enter_prog_name = enter_prog->name; | |
| } | |
| /* No programs other tail raw tracepoints are currently tail-called by the sys_enter | |
| * dispatcher. */ | |
| if(enter_prog->prog_type == BPF_PROG_TYPE_RAW_TRACEPOINT) { | |
| if(add_bpf_program_to_tail_table(syscall_enter_tail_table_fd, | |
| enter_prog_name, | |
| syscall_id)) { | |
| goto clean_fill_syscalls_tail_table; | |
| } | |
| const event_prog_t* enter_prog = | |
| (const event_prog_t*)&event_prog_table[enter_event_type]; | |
| if(enter_prog->name == NULL) { | |
| enter_prog = (const event_prog_t*)&event_prog_table[PPME_GENERIC_E]; | |
| } | |
| /* No programs other tail raw tracepoints are currently tail-called by the sys_enter | |
| * dispatcher. */ | |
| if(enter_prog->prog_type == BPF_PROG_TYPE_RAW_TRACEPOINT) { | |
| if(add_bpf_program_to_tail_table(syscall_enter_tail_table_fd, | |
| enter_prog->name, | |
| syscall_id)) { | |
| goto clean_fill_syscalls_tail_table; | |
| } |
| } | ||
|
|
||
| const event_prog_t* exit_prog = (const event_prog_t*)&event_prog_table[exit_event_type]; | ||
| const char* exit_prog_name = exit_prog->name; |
There was a problem hiding this comment.
Same as my previous comment on enter_prog_name, we probably don't need exit_prog_name.
| // static int detach(struct bpf_link **64_bit_prog_link, struct bpf_link **ia32_compat_prog_link, | ||
| // struct bpf_link **ia32_prog_link) { bpf_link if(openat_e && bpf_link__destroy(openat_e)) { | ||
| // pman_print_error("failed to detach the 'openat_e' program"); | ||
| // return errno; | ||
| // } | ||
| // openat_e = NULL; | ||
| // | ||
| // if(ia32_openat_e && bpf_link__destroy(ia32_openat_e)) { | ||
| // pman_print_error("failed to detach the 'ia32_openat_e' program"); | ||
| // return errno; | ||
| // } | ||
| // ia32_openat_e = NULL; | ||
| // | ||
| // if(ia32_compat_openat_e && | ||
| // bpf_link__destroy(ia32_compat_openat_e)) { | ||
| // pman_print_error("failed to detach the 'ia32_compat_openat_e' program"); | ||
| // return errno; | ||
| // } | ||
| // ia32_compat_openat_e = NULL; | ||
| // | ||
| // return 0; | ||
| // } |
There was a problem hiding this comment.
Does this need to be removed?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ekoops, FedeDP, Molter73 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
LGTM label has been added. DetailsGit tree hash: 4b1523083605a63212a1ebdc9062be0e99a240a6 |
|
Closing in favor of #2581 |
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area driver-modern-bpf
/area libscap-engine-modern-bpf
Does this PR require a change in the driver versions?
What this PR does / why we need it:
This PR is related to #2407.
Current TOCTOU mitigation implementation leverages the enter events generated by eBPF programs which are tail-called by the
sys_enterdispatcher. As the architecture is moving towards dropping enter event generation and collection, thesys_enterdispatcher will be ultimately removed. This requires to isolate TOCTOU mitigation handling logic in order to make it independent fromsys_enterdispatcher removal.This patch moves TOCTOU mitigation handling logic into separate ad-hoc eBPF tracepoint programs attached on corresponding
syscalls/sys_enter_*hooks.For each system call
<syscall>that already supports TOCTOU mitigation, two eBPF tracepoint programs are defined:ttm_<syscall>_e- this program is responsible for tail calling the<syscall>_eprogram after having performed syscall ID normalization and applied any required sampling/filtering logic<syscall>_e- this program is responsible for collecting the information needed to generate the proper enter event and sending the generated event to userspaceTail-called TOCTOU mitigation programs are inserted into a separate tail call map, called
syscall_enter_toctou_mitigation_tail_table.As the tracepoint attachment procedure generates an
openatexit event on/sys/kernel/tracing/events/.../idthat would pollute the stream of events read by the probe, the implementation takes care of attaching them before attaching thesys_exitdispatcher.Notice that the new logic make the TOCTOU mitigation programs attachment dependent on the presence of the
sys_exitdispatcher.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
/milestone 0.22.0
Does this PR introduce a user-facing change?: