-
Notifications
You must be signed in to change notification settings - Fork 490
selectors: add matchParentBinaries selector #4254
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
4f73f6d to
e31cabe
Compare
|
Probably I should add docs for this selector |
e31cabe to
70ac377
Compare
kkourt
left a comment
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!
One thing that would help reviewing this PR would be to split it into multiple commits.
See https://tetragon.io/docs/contribution-guide/submitting-a-pull-request/.
Before the implementation, though, it would be interesting to specify the fetaure so that's it is clear what the semantics are. This can be done in the PR itself (e.g,. in a commit message), in an issue, or in a CFP (see https://github.com/cilium/design-cfps). Whatever works best for you!
For example, it would be very useful to have an example policy that uses the newly introduced operator and discuss how it works
|
Another thing to note is that the functionality is similar to https://tetragon.io/docs/concepts/tracing-policy/selectors/#follow-children. So I wonder if something like: Would achieve a similar result. The first thing to check would be if |
|
@kkourt So listing multiple operators in match binary selector will fail. |
|
One more thing to note is that this looks related to
Thanks! I don't remember why this limitation exists, but my guess is that this is something that can be fixed. So, then, the question in my mind is whether the use-case you are describing is better served by matching on the immediate parent or all on ancestors (for which some support already exists). PS: Might be worth updating the docs to reflect above. |
I wonder if supporting 2 binary selectors would be easier than supporting N, but would potentially achieve the result without supporting parent selectors altogether? We could maybe have a limit to how many "generations" of children we follow. In this case, a limit of 1 generation would prevent reporting grandchildren. Haven't looked to see if this is easy or not however. |
Currently exactly 1 Moreover, maybe explicit parents filtering, working in the same way as current process binary filtering, is easier to understand and to read. |
With current existing support we cannot exclude the binary itself because of the selector number limitation. |
70ac377 to
d503dbd
Compare
If we want to match concrete child process, seems like this wouldn't achieve the same result. |
d503dbd to
12bf008
Compare
|
@kkourt |
Thanks! Can you please add some context in the commit messages (see: https://tetragon.io/docs/contribution-guide/submitting-a-pull-request/)? Before merging the PR, we would need tests and documentation updates. If you are looking for early feedback, can you add an example (maybe in the commit message) on how to use the new selector? |
|
I'm also seeing some CI failures: |
Is it an issue of this PR? |
eeab3e7 to
5d54ba8
Compare
mtardy
left a comment
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, overall looks good to me, just a few nits
Probably I should add docs for this selector
Please update the documentation with this new selector (see https://tetragon.io/docs/concepts/tracing-policy/selectors/).
|
I see checkpatch is complaining and other CI checks, you can run |
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.
LGTM (barring the red test results).
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.
would it be possible to handle this as special case of current BinarySelector, having some parent bool like:
diff --git a/pkg/k8s/apis/cilium.io/v1alpha1/types.go b/pkg/k8s/apis/cilium.io/v1alpha1/types.go
index 6c3210b0acf5..b5133b1e5fe2 100644
--- a/pkg/k8s/apis/cilium.io/v1alpha1/types.go
+++ b/pkg/k8s/apis/cilium.io/v1alpha1/types.go
@@ -124,6 +124,10 @@ type BinarySelector struct {
// +kubebuilder:validation:Optional
// +kubebuilder:default=false
FollowChildren bool `json:"followChildren"`
+ // match parent binary
+ // +kubebuilder:validation:Optional
+ // +kubebuilder:default=false
+ Parent bool `json:"parent"`
}
then all the code dups with binary selectors would go away
we could perhaps think a bit about some maps max_entries tuning
5d54ba8 to
600672b
Compare
|
@olsajiri Hi!
Moreover, |
|
@olsajiri @kkourt The bug is that when we match for parent binaries, we search for parent by current process real parent pid. But if There are 2 possible solutions:
And we will work with them in And then, when we match parent binaries, we check for the entry in Separate map could give us not so big memory overhead as parent binary field in execve map value, because we could set its size to be N times less than execve map (because case when parent pid and current process pid are same is not common) What do you think about it? |
we were discussing this with @kkourt and IIUC we lean to have have documented that both binaries selectors match only existing processes.. so we disregard cases were process does multiple execs that said, if you would have the change for option 2) it'd be great to check that and see if that might be way out |
|
@olsajiri @kkourt What changed:
Also I've added more tests, which include running I've checked changes locally, now it works fine with all cases, including using All tests passed, I'll fix checkpatch in the end if everything else is fine. If you don't like the idea, I'm okay with reverting these changes and documenting that current selectors work only with real processes and don't with for processes created with multiple execs. |
|
Thanks! I'll take a look when I get a chance.
I don't see this as an option. We either support this feature for everything or not at all. |
a73ef81 to
da56f8c
Compare
|
Hi! I've implemented @kevsecurity idea with parents map switch.
Tests cover both variants when process is executed with same pid (when we just run |
|
@olsajiri Hi! Could you check current changes as well, please? |
| } | ||
|
|
||
| func writePrefixStrings(k *KernelSelectorState, values []string) error { | ||
| mid, err := writePrefix(k, values, "MatchArgs") |
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.
I don't see why this is removed, we could leave this debug info, right?
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.
we have info about selectors called this function in its callers, so this info is actually redundant. if we leave it, we should also add info about match parent binaries selector here
| struct binary *parent_bin = map_lookup_elem(&parent_binaries_map, &enter->key.pid); | ||
|
|
||
| if (parent_bin) | ||
| /* matchParentBinaries key is in rage [MAX_SELECTORS; MAX_SELECTORS * 2) */ |
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.
angry selector ;-)
|
|
||
| struct binary *parent_bin = map_lookup_elem(&parent_binaries_map, &enter->key.pid); | ||
|
|
||
| if (parent_bin) |
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.
please use the { } for the condition code
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.
I think I removed them because of clang-format errors in CI, but I'll try to put them back
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.
Warning: WARNING:BRACES: braces {} are not necessary for single statement blocks
there are only warnings in style ci job, but it is still red
|
|
||
| if (parent) | ||
| map_update_elem(&parent_binaries_map, &curr->key.pid, &parent->bin, BPF_ANY); | ||
| } |
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.
so this is still solution just fort the immediate parent? please put some explanation on how this new map works.. there's nothing in changelog or code AFAICS
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.
not sure what you meant. the whole selector is for matching current process immediate parent (and not only immediate if followChildren is true).
in branch upper if we don't have event_clone flag, instead of real parent binary we fill map with current binary, because no event_clone flag means we had multiple execs in same process.
I'll add comments to the map in bpf code
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.
current solution helps to mitigate two problems of the initial implementation:
- if parent process exited and it is not stored in execve_map, we will still have its binary in parent_binaries_map
- if multiple execs are performed, we can understand it by not having event_clone flag, so we use current binary instead of parent binary
This adds genericMatchBinariesSelector enumeration, renames match binaries maps and changes match binaries parsing to allow parsing other matchBinaries-like selectors in future with the same code. Signed-off-by: Kobrin Ilay <[email protected]>
This commit introduces new matchParentBinaries selector,
which is used for filtering parent process binaries. New
selector works similarly to matchBinaries selector, including
followChildren option, which allows to match not only
direct parent binaries, but transitive parent binaries
as well.
matchParentBinaries selector is needed in cases when we
want to have granular filters on parent binaries for some
events. For example, we may want to intercept calls for
specific system call from specific binary only in case
it was executed with interactive shell. For that we can
add following selector, which will match only process with
bash, sh or zsh parent:
```
- matchParentBinaries:
- operator: "In"
values:
- "/usr/bin/bash"
- "/usr/bin/sh"
- "/usr/bin/zsh"
```
Signed-off-by: Kobrin Ilay <[email protected]>
This commit adds crds generated for matchParentBinaries selector Signed-off-by: Kobrin Ilay <[email protected]>
This commits adds logic for parsing matchParentBinaries selector. New selector is stored in same maps as matchBinaries selector, but has key offset equal to MaxSelectors. Signed-off-by: Kobrin Ilay <[email protected]>
This commit adds parents binary map to sensors and restricts using matchParentBinariesMap selector when parents map is not enabled. Parents map is disabled by default due to additional memory overhead from storing info about parent binaries. Default parents map size is same as default execve map size. Signed-off-by: Kobrin Ilay <[email protected]>
This commit adds bpf code for parent binaries filtering for new matchParentBinaries selector. Match binaries bpf maps sizes are increased to MAX_SELECTORS * 2 to store both selectors options. Parent filtering is processed with the same code, but key for matchParentBinaries has offset equal to MAX_SELECTORS. Signed-off-by: Kobrin Ilay <[email protected]>
This commit adds test for new matchParentBinaries selector. In the test we verify that all operations (In, NotIn, Prefix, NotPrefix, Postfix, NotPostfix) work correctly. Signed-off-by: Kobrin Ilay <[email protected]>
Signed-off-by: Kobrin Ilay <[email protected]>
8e33265 to
6f2a2db
Compare
pin and update map only when option is enabled. check for event_clone flag instead of cleanup process pid Signed-off-by: Kobrin Ilay <[email protected]>
6f2a2db to
142f650
Compare
Description
This change adds
matchParentBinariesselector, which might be useful for proper and granular filtering of parent binaries, which is needed in some specific cases.For instance, there is a python script, which invokes some system calls, which we want to intercept and report. If such script is executed by some system process, we want to filter it out. Otherwise, we report it. For this filtering we need a selector for parent binary, because we cannot filter out events only by current binary, which in case of python script execution is always
python.For more real example, consider we want to hook all calls of
chmodsystem call to prevent creating new binaries on the system manually.apt-keybinary, when it installs some packages (such cases we don't want to report), doesn't callchmoddirectly, but uses/usr/bin/chmodbinary, which callschmodsystem call inside.matchParentBinariesselector would help to create accurate exclusion for this case.Example of policy with
matchParentBinariesselector:Consider we want to get events about all files, which were made executable, with
chmodsyscall, but don't want to get events aboutapt-keymaking files executable. Unfortunately,apt-keydoesn't make files executable with syscall directly, but uses/usr/bin/chmodbinary, which callschmodfunction, so to filter such events we need to have selectors for both parent and current binary, so the resulting policy will look like this:If current binary is
/usr/bin/chmod, we don't care about parent binary, but if current binary is/usr/bin/chmod, we don't want the parent binary to be/usr/bin/apt-key.Changelog