events: improve hidden_kernel_module #3001
Conversation
|
|
b369dfc to
210297e
Compare
There was a problem hiding this comment.
The design of channel messaging between derive and tracee.go causes direct coupling between the implementation of the derived event and tracee itself.
As in: If we wanted to separate the event out into a plugin for example, it would be difficult, if not impossible to do so.
As i've written in the first comment, I think most of the logic here should move to a new pipeline stage happening after the derive stage.
Alternatively: maybe a separate modscan package should be added with parallel module scanning logic that can be initialized by the tracee object (although this will require some future development to how tracee registers such objects).
65e651c to
cb9826b
Compare
|
I also want to review this one. One thing I spotted in code already merged: func clearMap(bpfMap *bpf.BPFMap) error {
var err error
var iter = bpfMap.Iterator()
for iter.Next() {
addr := binary.LittleEndian.Uint64(iter.Key())
err = bpfMap.DeleteKey(unsafe.Pointer(&addr))
if err != nil {
logger.Errorw("err occurred DeleteKey: " + err.Error())
return err
}
}
return nil
}There is a batch libbpf function for that, way more performatic I believe. Check: https://github.com/aquasecurity/libbpfgo/blob/main/selftest/map-batch/main.go/#L112-L118
|
|
Algo, you forgot to raise capabilities at: https://github.com/aquasecurity/tracee/blob/main/pkg/ebpf/tracee.go/#L1455-L1459 modsMap, err := t.bpfModule.GetMap("modules_map")
if err != nil {
logger.Errorw("err occurred GetMap: " + err.Error())
return
}So, now that E2E tests are dropping capabilities by default, they're failing with:
|
There was a problem hiding this comment.
@OriGlassman its a very ingenuous work (sorry it took me sometime to read your initial PR already merged and this one improving it). With that said, I left some comments on better documenting it, a few simple changes that might be good, positional changes into new files AND some capabilities raising fixes needed.
I did not take the time to check if the execution path of probes you used, for the race windows u mentioned, between the probes and event processing, could be mitigated by other type of probes, etc.
I know its more of a "research" thing, but I was wondering if this "flag based state machine" approach could be simplified by eBPF only logic making sure there are no races.
Either way, at the end, talking is cheaper than doing.. so good job =D
|
Hey @OriGlassman, we're releasing tomorrow. I'm moving this to the next milestone, ok ? Let me know if there is any real need from something here to this milestone and we can, maybe, spin off the code and have another PR with a simple change. If not, then next milestone is good I believe. |
|
@OriGlassman not sure what's the status here, |
Was OOO for a couple of weeks, but I'm back now... PR should be ready in the next couple of days. |
f25879f to
5290ad5
Compare
2530ca4 to
ccd41ee
Compare
|
Ready for review from my side guys.. Thanks! |
pkg/ebpf/c/tracee.bpf.c
Outdated
| @@ -688,21 +743,25 @@ static __always_inline bool find_modules_from_module_kset_list(program_data_t *p | |||
| return !finished_iterating; | |||
| } | |||
|
|
|||
| BPF_QUEUE(walk_mod_tree_queue, rb_node_t, 2048); // used to walk a rb tree | |||
| #if LINUX_VERSION_CODE >= KERNEL_VERSION(4, 20, 0) || defined(CORE) | |||
There was a problem hiding this comment.
4,20 ? Either way, we do not support non CORE any longer. So you can remove this ifdef here.
pkg/ebpf/c/tracee.bpf.c
Outdated
|
|
||
| #ifdef CORE // in non CORE builds it's already defined |
There was a problem hiding this comment.
no need (only CORE supported now)
pkg/ebpf/c/tracee.bpf.c
Outdated
| { | ||
| reset_event_args(p); | ||
| u64 mod_addr = (u64) mod; | ||
| char *mod_name = mod->name; | ||
| const char *mod_srcversion = READ_KERN(mod->srcversion); |
There was a problem hiding this comment.
READ_KERN was dropped, should use BPF_CORE_READ (and similar) macros (directly from libbpf UAPI headers).
pkg/ebpf/c/tracee.bpf.c
Outdated
| // Check again with the address being the start of the memory area, since | ||
| // there's a chance the module is in /proc/modules but not in /proc/kallsyms (since the | ||
| // file can be hooked). | ||
| key = (u64) READ_KERN(pos->core_layout.base); | ||
| mod_from_map = bpf_map_lookup_elem(&modules_map, &key); | ||
| mod = (u64) READ_KERN(pos->core_layout.base); |
pkg/ebpf/c/tracee.bpf.c
Outdated
|
|
||
| return 0; | ||
| const char *version = READ_KERN(mod->version); |
pkg/ebpf/c/tracee.bpf.c
Outdated
| // get previous of original next | ||
| struct list_head *orig_next_ptr = (struct list_head *) (orig_module_data->next); | ||
| u64 orig_next_prev_addr = (u64) READ_KERN(orig_next_ptr->prev); | ||
| const char *version = READ_KERN(mod->version); |
pkg/ebpf/c/tracee.bpf.c
Outdated
| } | ||
|
|
||
| // save strings to buf | ||
| const char *version = READ_KERN(mod->version); |
| // If found hidden, it sends a message in the channel, which causes the address and the flags to get passed | ||
| // to the lkm submitter program (eBPF), which sends it back to userspace, | ||
| // this time with flags that will cause it to get submitted to the user as an event, | ||
| err := capabilities.GetInstance().Specific( |
There was a problem hiding this comment.
Should use the EBPF capabilities RING (it already contains the base line for all capabilities needed by picked events PLUS the ones needed for eBPF functions).
| func clearMap(bpfMap *bpf.BPFMap) error { | ||
| err := capabilities.GetInstance().EBPF( | ||
| err := capabilities.GetInstance().Specific( |
There was a problem hiding this comment.
Should use the EBPF capabilities RING (it already contains the base line for all capabilities needed by picked events PLUS the ones needed for eBPF functions).
| err = capabilities.GetInstance().Specific( | ||
| // FillModulesFromProcFs fills a map with modules from /proc/modules, to be checked in kernel-space for inconsistencies. | ||
| func FillModulesFromProcFs(kernelSymbols helpers.KernelSymbolTable) error { | ||
| err := capabilities.GetInstance().Specific( |
There was a problem hiding this comment.
Should use the EBPF capabilities RING (it already contains the base line for all capabilities needed by picked events PLUS the ones needed for eBPF functions).
There was a problem hiding this comment.
IF reading /proc/modules here needs another capability, then you can either add the capability to the base line (by adding as a requirement in the events.go event) OR, if this code is not that frequent you can use Specific() her
10000
e, but with the capability event needed to read /proc/ fs only, not CAP_SYS_ADMIN).
There was a problem hiding this comment.
I have added a few comments. The PR needs rebasing (and changing READ_KERN macro to BPF_CORE_READ). The rest, I have proposed a chore commit to your personal branch at OriGlassman#1).
I believe we're close to merge, and I really want to spin this code out of tracee.bpf.c to something more "event specific" (I'll do it as chore actions later).
Nice work! I'll rebase myself tomorrow if you're out (Friday for you), lets see if we can get this merged before freezing the tree for the release. Cheers!
50da68f to
071b3f3
Compare
- lkm_seeker: run after a kernel module was loaded (in addition to periodically) - lkm_seeker: address possible race conditions that may cause false-positive - lkm_seeker: add technique for newly loaded modules - Removed logic from do_init_module event - Added module_free event - Added module_load event
Done |
Rafael reviewing before freeze date.
- lkm_seeker: run after a kernel module was loaded (in addition to periodically) - lkm_seeker: address possible race conditions that may cause false-positive - lkm_seeker: add technique for newly loaded modules - removed logic from do_init_module event - added module_free event - added module_load event
This reverts commit b855a20.
Add improvements:
2. Explain how to test it
./tracee -f e=hidden_kernel_module