matchBinaries: support followChildren #2720

kkourt · 2024-07-23T20:25:43Z

This PR introduces a FollowChildren: attribute in MatchBinaries.
For now, only the In operator is supported but others can be added.
Another limitation is that only 64 of such sections are supported, but this number can also be increased.

tracing: introduce FollowChildren attribute in MatchBinaries selector

netlify · 2024-07-23T20:26:33Z

✅ Deploy Preview for tetragon ready!

Name	Link
🔨 Latest commit	`5715e16`
🔍 Latest deploy log	https://app.netlify.com/sites/tetragon/deploys/66af330573551f00083a88aa
😎 Deploy Preview	https://deploy-preview-2720--tetragon.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

vparla · 2024-07-24T16:58:41Z

bpf/process/bpf_process_event.h

@@ -11,7 +11,8 @@

 #define ENAMETOOLONG 36 /* File name too long */

-#define MAX_BUF_LEN 256
+#define MATCH_BINARIES_PATH_MAX_LENGTH 256


Do these lengths present a problem in terms of potential truncation?
while any path fragment can only be 255, the total path could be larger.

MATCH_BINARIES_PATH_MAX is the size of the paths users can specify in values under MatchBinaries. That is, it only limits the size of the user-specified paths that we want to match against.

For example,

- matchBinaries: - operator: "In" values: - "/usr/bin/bash"

would work, but:

- matchBinaries: - operator: "In" values: - "/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo/foo"

wouldn't.

Thank you for this work @kkourt! This will help us out immensely.

One comment, could we in the future perhaps convert this to use the same string handling as was added in #2069 ? Not for now, just in the future.

Yap, I've also thought the same :)

so this is an issue on the prefix feature because of the way we read dentries, since we read them from end to start, you can escape a prefix match using a long path (>256). There are attempts to fix at the moment (#2718) but the only durable fix would be to extend MATCH_BINARIES_PATH_MAX_LENGTH to 4096 somehow.

If we ifdef'd into 6.x kernels maybe 4096 is fine with better BPF? This way limitation only exists on <X.Y versions?

So I've been trying to extend the length to 4096, mostly making prepend_name (the function we use to read dentry) to support 4096 chars and I'm currently blocked when the compiler is replacing memset and memcpy when the object (from a map value) is too large.

mtardy

Thanks that's a cool feature!

I think my main point would be to make things a bit easier to understand as:

it's not easy to link the mbset naming to the follow children match binaries feature.
sometimes what we do in BPF can look a bit cryptic when it could be explained.

I think I understood the patch after taking a bit of time, but it might get complicated to get it in some time without more comments/better naming.

bpf/process/bpf_execve_event.c

pkg/selectors/kernel.go

mtardy · 2024-07-30T10:15:05Z

bpf/process/types/basic.h

@@ -1520,6 +1520,7 @@ FUNC_INLINE size_t type_to_min_size(int type, int argm)
 struct match_binaries_sel_opts {
 	__u32 op;
 	__u32 map_id;
+	__u32 mbset_id;


not a big fan of the naming here given it's already in the match binary selector option and it does not stand out that it's for the follow children feature. In general I think mbset for match binaries set does not give a strong idea it's behind the "follow children" feature.

I have a different opinion. The mechanism is a matchBinaries set (mbset) and is used to implement the followChidren feature. I prefer to name things based on what they are rather than how they are used. Note that the latter can change. In any case, I'm happy to change it. What would be your suggestion of a better name?

bpf/process/bpf_execve_event.c

bpf/process/types/basic.h

pkg/testutils/copyfile.go

pkg/sensors/tracing/matchbinaries_follow_children_test.go

tpapagian

LGTM, just added some small comments.

bpf/process/bpf_execve_event.c

pkg/mbset/mbset.go

pkg/sensors/tracing/matchbinaries_follow_children_test.go

pkg/mbset/mbset.go

kkourt · 2024-07-31T09:44:39Z

Thanks for the reviews, pushed a new version to address the feedback.

kkourt · 2024-07-31T09:50:29Z

checkpatch complains with:

  Warning: WARNING:PREFER_FALLTHROUGH: Prefer 'fallthrough;' over fallthrough comment

But we do not seem to have support for it:

process/types/basic.h:1588:4: error: use of undeclared identifier 'fallthrough'                                                                                                                                                                                
                        fallthrough;

mtardy · 2024-08-01T08:51:12Z

checkpatch complains with:

  Warning: WARNING:PREFER_FALLTHROUGH: Prefer 'fallthrough;' over fallthrough comment

But we do not seem to have support for it:

process/types/basic.h:1588:4: error: use of undeclared identifier 'fallthrough'                                                                                                                                                                                
                        fallthrough;

This is out of the scope of this PR and I don't want to block it on that but we could add what Cilium does in our compiler.h: https://github.com/cilium/cilium/blob/b189c578ea4da02467dc636e338683b8b6b4b012/bpf/include/bpf/compiler.h#L113-L115

pkg/mbset/mbset.go

Signed-off-by: Kornilios Kourtis <[email protected]>

Move MATCH_BINARIES_PATH_MAX_LENGTH to bpf_process_event.h so that it can be used in the exec hook in subsequent patches. Signed-off-by: Kornilios Kourtis <[email protected]>

The goal is to implement a 'followChildren: true' attribute in the MatchBinaries operator that can be used to match not only processes with the specified binary name, but also their children. To do so, we need to mark processes at exec time. This patch introduces tha bpf map that will be used for the marking. The map has paths as keys and a bitset of ids as a value. The map is written by user-space and read in BPF. Each id corresponds to a MatchBinaries section with 'followChildren: true'. The mbset package implements the user-space side of the map. For example, consider the following sections: - matchBinaries: - operator: "In" values: - "/usr/bin/bash" - "/usr/bin/dash" followChildren: true ID: 0 - matchBinaries: - operator: "In" values: - "/usr/bin/bash" - "/usr/bin/csh" followChildren: true ID: 5 The map then will look like: /usr/bin/bash -> (1<<0) | (1<<5) = 0x21 /usr/bin/dash -> (1<<0) = 0x1 /usr/bin/csh -> (1<<5) = 0x20 Signed-off-by: Kornilios Kourtis <[email protected]>

This patch initialized the tg_mbset_map based on MatchBinaries sections. An id is allocated when the we parse the section. Subsequently, we update the given id for the patches specified in the section. Signed-off-by: Kornilios Kourtis <[email protected]>

This commit adds an mb_bitset field in the binary structure. The filed is set in the exec hook using the parent bitset and the contents of the tg_mbset_map. Note that in bpf_fork, we memcpy the old binary so the bitset will be propageted to children. Signed-off-by: Kornilios Kourtis <[email protected]>

kkourt · 2024-08-04T07:52:14Z

checkpatch complains with:
  Warning: WARNING:PREFER_FALLTHROUGH: Prefer 'fallthrough;' over fallthrough comment
But we do not seem to have support for it:
process/types/basic.h:1588:4: error: use of undeclared identifier 'fallthrough'                                                                                                                                                                                
                        fallthrough; 
This is out of the scope of this PR and I don't want to block it on that but we could add what Cilium does in our compiler.h: https://github.com/cilium/cilium/blob/b189c578ea4da02467dc636e338683b8b6b4b012/bpf/include/bpf/compiler.h#L113-L115

Make sense, added fallthrough.

This is going to be used in the next patch. Signed-off-by: Kornilios Kourtis <[email protected]>

This commit adds a check in match_binaries that uses the bitset that is tracked from parent to children, allowing to track children of a given binary. Signed-off-by: Kornilios Kourtis <[email protected]>

CopyFileToTmp copies a file to /tmp and returns its name. It also chmods the destination file, so it can be used to copy binaries. It's going to be used the next patch. Signed-off-by: Kornilios Kourtis <[email protected]>

loadGenericSensorTest is a helper function to load a generic sensor. With the addition of the tg_mbset_map in the previous commits, when using a matchBinaries with followChildren: true because for the latter, we need to base sensor to be loaded. This fixes the above issue by initializing the base (exec) sensor before running SensorsFromPolicy. Signed-off-by: Kornilios Kourtis <[email protected]>

The test copies /bin/sh into a temp location and Creates a policy that mathes the temp sh and its children. It executes the getcpu program and checks that the policy caught the syscall. Signed-off-by: Kornilios Kourtis <[email protected]>

lambdanis · 2024-08-09T17:37:46Z

@kkourt I see followChildren is a required field in CRD - that's a breaking change and breaks existing policies. Can we make it not required and default to false?

kkourt · 2024-08-10T06:40:58Z

@kkourt I see followChildren is a required field in CRD - that's a breaking change and breaks existing policies. Can we make it not required and default to false?

Good point: #2790

kkourt added the release-note/minor This PR introduces a minor user-visible change label Jul 23, 2024

kkourt force-pushed the pr/kkourt/matchbin-follow branch 3 times, most recently from 4e4f81f to 6424ce1 Compare July 24, 2024 09:38

kkourt marked this pull request as ready for review July 24, 2024 10:34

kkourt requested a review from a team as a code owner July 24, 2024 10:34

kkourt requested a review from tpapagian July 24, 2024 10:34

vparla reviewed Jul 24, 2024

View reviewed changes

mtardy self-requested a review July 29, 2024 09:10

mtardy reviewed Jul 30, 2024

View reviewed changes

tpapagian approved these changes Jul 30, 2024

View reviewed changes

olsajiri reviewed Jul 30, 2024

View reviewed changes

pkg/mbset/mbset.go Show resolved Hide resolved

kkourt force-pushed the pr/kkourt/matchbin-follow branch from 6424ce1 to 7d5d536 Compare July 31, 2024 09:44

kkourt requested review from olsajiri and mtardy July 31, 2024 09:44

olsajiri approved these changes Aug 2, 2024

View reviewed changes

pkg/mbset/mbset.go Outdated Show resolved Hide resolved

kkourt added 6 commits August 4, 2024 09:50

crd: add followChildren in matchBinaries

abb0a1e

Signed-off-by: Kornilios Kourtis <[email protected]>

autochore: make crds && make -C install/kubernetes

962e158

Signed-off-by: Kornilios Kourtis <[email protected]>

bpf: move MATCH_BINARIES_PATH_MAX_LENGTH

f37a618

Move MATCH_BINARIES_PATH_MAX_LENGTH to bpf_process_event.h so that it can be used in the exec hook in subsequent patches. Signed-off-by: Kornilios Kourtis <[email protected]>

mbset: initialize tg_mbset_map

5d5a248

This patch initialized the tg_mbset_map based on MatchBinaries sections. An id is allocated when the we parse the section. Subsequently, we update the given id for the patches specified in the section. Signed-off-by: Kornilios Kourtis <[email protected]>

kkourt force-pushed the pr/kkourt/matchbin-follow branch from 7d5d536 to 5715e16 Compare August 4, 2024 07:51

kkourt added 2 commits August 4, 2024 09:52

bpf: introduce fallthrough in compiler.h

2acaaa9

This is going to be used in the next patch. Signed-off-by: Kornilios Kourtis <[email protected]>

bpf: check the bitset in match_binaries

1ad85a8

This commit adds a check in match_binaries that uses the bitset that is tracked from parent to children, allowing to track children of a given binary. Signed-off-by: Kornilios Kourtis <[email protected]>

kkourt added 3 commits August 4, 2024 09:52

testutils: add a CopyFileToTmp function

9ec1054

CopyFileToTmp copies a file to /tmp and returns its name. It also chmods the destination file, so it can be used to copy binaries. It's going to be used the next patch. Signed-off-by: Kornilios Kourtis <[email protected]>

kkourt force-pushed the pr/kkourt/matchbin-follow branch from 5715e16 to dc2d413 Compare August 4, 2024 07:53

mtardy approved these changes Aug 5, 2024

View reviewed changes

kkourt merged commit 848a38a into main Aug 5, 2024
46 checks passed

kkourt deleted the pr/kkourt/matchbin-follow branch August 5, 2024 11:01

kkourt mentioned this pull request Aug 10, 2024

crd: make followChildren optional #2790

Merged

kkourt mentioned this pull request Sep 2, 2024

docs for followChildren true #2858

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

matchBinaries: support followChildren #2720

matchBinaries: support followChildren #2720

kkourt commented Jul 23, 2024 •

edited

Loading

netlify bot commented Jul 23, 2024 •

edited

Loading

vparla Jul 24, 2024 •

edited

Loading

kkourt Jul 25, 2024 •

edited

Loading

dwindsor Jul 25, 2024

kkourt Jul 25, 2024

mtardy Jul 30, 2024

jrfastab Jul 30, 2024

mtardy Jul 31, 2024

mtardy left a comment

mtardy Jul 30, 2024

kkourt Jul 31, 2024

tpapagian left a comment

kkourt commented Jul 31, 2024

kkourt commented Jul 31, 2024

mtardy commented Aug 1, 2024

kkourt commented Aug 4, 2024

lambdanis commented Aug 9, 2024 •

edited

Loading

kkourt commented Aug 10, 2024

matchBinaries: support followChildren #2720

matchBinaries: support followChildren #2720

Conversation

kkourt commented Jul 23, 2024 • edited Loading

netlify bot commented Jul 23, 2024 • edited Loading

✅ Deploy Preview for tetragon ready!

vparla Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

kkourt Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

dwindsor Jul 25, 2024

Choose a reason for hiding this comment

kkourt Jul 25, 2024

Choose a reason for hiding this comment

mtardy Jul 30, 2024

Choose a reason for hiding this comment

jrfastab Jul 30, 2024

Choose a reason for hiding this comment

mtardy Jul 31, 2024

Choose a reason for hiding this comment

mtardy left a comment

Choose a reason for hiding this comment

mtardy Jul 30, 2024

Choose a reason for hiding this comment

kkourt Jul 31, 2024

Choose a reason for hiding this comment

tpapagian left a comment

Choose a reason for hiding this comment

kkourt commented Jul 31, 2024

kkourt commented Jul 31, 2024

mtardy commented Aug 1, 2024

kkourt commented Aug 4, 2024

lambdanis commented Aug 9, 2024 • edited Loading

kkourt commented Aug 10, 2024

kkourt commented Jul 23, 2024 •

edited

Loading

netlify bot commented Jul 23, 2024 •

edited

Loading

vparla Jul 24, 2024 •

edited

Loading

kkourt Jul 25, 2024 •

edited

Loading

lambdanis commented Aug 9, 2024 •

edited

Loading