-
Notifications
You must be signed in to change notification settings - Fork 349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IMA hashes in LSM events #2818
base: main
Are you sure you want to change the base?
IMA hashes in LSM events #2818
Conversation
15d918b
to
758ec7e
Compare
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
3ec1c3d
to
7525121
Compare
f9e5ac0
to
20a3c09
Compare
b84e2bd
to
ef33031
Compare
@kkourt , hi! May I ask you for a code review? Next Monday, on Tetragon community meeting I'll show a demo. I want to talk about some design decisions I made. Also I'm going to speak about problems that I overcame and those are still exists. |
Hello :) Thanks for the heads up! Will try and have a look before the community meeting. Looking forward to the demo! |
2ef75a3
to
58c2771
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @anfedotoff! I had a first look. Please find some comments below.
bpf/process/bpf_generic_lsm.c
Outdated
|
||
e = map_lookup_elem(&process_call_heap, &zero); | ||
if (e && e->common.flags & MSG_COMMON_FLAG_IMA_HASH) | ||
tail_call(ctx, &lsm_calls, TAIL_CALL_IMA_HASH); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't there a race with this approach?
That is, what guarantees are there that user-space will do the map lookup before the entry is added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right... It might be that user-space will do map lookup before entry is added. For now, I have only one idea how to overcome this problem. Do not use hash_map, use ring buffer to send event from lsm.s program.
But ring buffers are not supported in tetragon (#208). Do you have any ideas how to to this with out ring buffer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we tailcall to a program that does the generic_output
from an lsm.s program?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we tailcall to a program that does the
generic_output
from an lsm.s program?
Unfortunately, no(. Becase we can't use BPF_MAP_TYPE_PROG_ARRAY
maps in lsm.s programs. We can tail call in, but we can't taill call out :).
pkg/sensors/program/loader.go
Outdated
@@ -423,6 +423,9 @@ func TracingAttach() AttachFunc { | |||
|
|||
func LSMOpen(load *Program) OpenFunc { | |||
return func(coll *ebpf.CollectionSpec) error { | |||
delete(coll.Programs, "ima_file_open") | |||
delete(coll.Programs, "ima_mmap_file") | |||
delete(coll.Programs, "ima_bprm_check_security") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we remove the programs here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because, we can't load them all (they are not generic). We need to load one of them only if imaHash: true
in the policy. I load ima_* programs on demand, after all lsm generic programs are loaded.
I refreshed on LSM programs and I think we can do this differently so LSM programs are attached through trampolines on specific bpf_lsm_XXX and trampoline allows to attach and invoke multiple bpf programs to so I think we could have the sleepable lsm hooks invoked first to get the
to ensure the generic lsm is invoked AFTER lsm.s/XXX hook we can rely I guess there might be some hiccups when you'll implement that WDYT? |
pkg/sensors/tracing/genericlsm.go
Outdated
return fmt.Errorf("opening BTF file '%s' failed: %w", btfFilePath, err) | ||
} | ||
} | ||
spec, err := ebpf.LoadCollectionSpec(args.Load.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whatever solution we pick for this I think the IMA lsm hooks should go in separate object and we should be able to load it through normal LoadLSMProgram interface
Thanks for making it clear! We have a default ima_policy=tcb:
In particular, this ima_policy will measure hashes when files owned by root are opened for reading. So, in solution that you proposed will call bpf_ima_file_hash on every hook. The question is what helper will return, if ima_policy is not satisfied? If error, than it is OK, I think. But if it calculates hash despite the ima_policy it is not OK. I think, I can check this. To sum up, I think, you solution is a nice compromise if ima_policy is taking to account during |
yea, it might be better to do the initial solution first as a base and split to 3 parts lsm sensor as a follow up.. we might learn other things that could shape up the final solution when doing that ;-) |
58c2771
to
2561d5a
Compare
I thought, I can implement some example of our idea using ebpf-go, before coding in tetragon. Here is the repo: I have 3 bpf programs: three parts of LSM sensor. In the first program I emulate filtering and enforcing. Second program is used to get hash and the last one to send event via perf buffer. What I noticed.
Also, I'm not sure, that it is enough just to attach programs this way to preserve the order when hook is triggered. So far it seems that approach is not working for us, but I may be wrong. |
nice, but I'm not sure I understand what's wrong, could you please elaborate? I'll try it anyway |
Yes, sure, I'll try to explain.
The question is can we rely on the order of execution when Here is some logs:
According this logs we see, that the 1st program always starts and finishes first. Some times the third program starts before second is finished. |
3e8bb67
to
3fc4ac4
Compare
I think we can, it's hardcoded in kernel.. and if it changes we could actually detect the way it's added however it's the other way round (check https://pastebin.com/Y8EGQ0fg for the trampoline dump), could you plz try the change below? thanks,
|
Hmm, I changed the order (swap init and send programs) as you suggest. I got no events collected, but execution is blocked. Are you sure that the order is really proper? It seems to me, that send program is now executed before init program. (send -> ima -> init). |
also I think we need to use hash for the data shared by the program, it looks like cpu migration is not actually disabled, |
hum, when I remove the 'filter' from init function, I get all executed files output:
|
Yes, that works for me too, no hashes are missed. I remove filter with out changing order of execution, some hashes were missed. Now I'm looking at __bpf_prog_enter_sleepable and __bpf_prog_enter
It looks so... To use hash map is a good idea, but for now I don't know how to share key for this map. If we can't use per-cpu heap. |
I think just bpf_get_current_pid_tgid() should be ok? it's always under same task (but maybe not same cpu) |
This Is the first thing that came up to my mind. But what if we have file_open hook and the same process opens several files will it be OK? I think I can try to check this, nevertheless. |
hum, so if the key is thread id then single thread could have just one active file_open call and we should be fine, right? |
Ahh, yes, we have tid. You are right! It seems to be fine than. I'll try to fix my example. If every thing is fine, I'll start to refactor code in th PR. |
Everything is fine! I can start refactoring 🎆 |
3fc4ac4
to
9981b69
Compare
Due to restrictions of bpf sleepable programs (no tailcalls, no perf buffer and per_cpu maps, etc.), we need to split generic LSM sensor into three parts (collections) and load them in this order: - bpf_generic_output sends event using perf buffer - bpf_generic_lsm_ima_* calculates hash using IMA helpers - bpf_generic_lsm_core does everything else Signed-off-by: Andrei Fedotov <[email protected]>
Adding support for IMA hash collection in Post Action. Adding IMA hashes in LSM events. Hash is represented by a string algorithm:value. Support loading lsm.s/generic_lsm_ima_* programs. Signed-off-by: Andrei Fedotov <[email protected]>
Signed-off-by: Andrei Fedotov <[email protected]>
9981b69
to
e3b21d8
Compare
Adding test for ImaHash Post action. Signed-off-by: Andrei Fedotov <[email protected]>
Add imaHash post action to lsm_bprm_check.yaml Signed-off-by: Andrei Fedotov <[email protected]>
e3b21d8
to
64374c4
Compare
Signed-off-by: Andrei Fedotov <[email protected]>
64374c4
to
a85b65e
Compare
It works! No race condition! ✨ |
Adding support for collecting file hashes calculated by Linux integrity subsystem (IMA). IMA hashes will be contained in LSM events. To make it work you need to:
There are limitations on implementation due to the requirement to use bpf_ima_file_hash/bpf_ima_inode_hash helpers. The more information you can get from #2409.
Some design insights:
TODO list: