Skip to content
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.

Sign up for GitHub

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

matchBinaries: support followChildren #2720

Merged
merged 11 commits into from
Aug 5, 2024
+459 −7
Merged

matchBinaries: support followChildren #2720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
abb0a1e
crd: add followChildren in matchBinaries
kkourt Jul 23, 2024
962e158
autochore: make crds && make -C install/kubernetes
kkourt Jul 23, 2024
f37a618
bpf: move MATCH_BINARIES_PATH_MAX_LENGTH
kkourt Jul 23, 2024
fd0720c
mbset: introduce the tg_mbset_map
kkourt Jul 23, 2024
5d5a248
mbset: initialize tg_mbset_map
kkourt Jul 23, 2024
e2be2b7
exec: track matchbinary children
kkourt Jul 23, 2024
2acaaa9
bpf: introduce fallthrough in compiler.h
kkourt Aug 4, 2024
1ad85a8
bpf: check the bitset in match_binaries
kkourt Jul 23, 2024
9ec1054
testutils: add a CopyFileToTmp function
kkourt Jul 24, 2024
3c43d96
testutils: improve loadGenericSensorTest
kkourt Jul 24, 2024
dc2d413
tracing: add a test for MatchBinaries followChidren
kkourt Jul 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions bpf/include/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,8 @@
#define FUNC_INLINE static inline __attribute__((always_inline))
#endif

#ifndef fallthrough
#define fallthrough __attribute__((fallthrough))
#endif

#endif /* __BPF_COMPILER_H__ */
4 changes: 4 additions & 0 deletions bpf/lib/process.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,8 @@ struct msg_execve_event {
#endif
}; // All fields aligned so no 'packed' attribute.

typedef __u64 mbset_t;

// This structure stores the binary path that was recorded on execve.
// Technically PATH_MAX is 4096 but we limit the length we store since we have
// limits on the length of the string to compare:
Expand All @@ -318,6 +320,8 @@ struct binary {
__s64 path_length;
// BINARY_PATH_MAX_LEN first bytes of the path
char path[BINARY_PATH_MAX_LEN];
// matchBinary bitset for binary
mbset_t mb_bitset;
}; // All fields aligned so no 'packed' attribute

// The execve_map_value is tracked by the TGID of the thread group
Expand Down
36 changes: 36 additions & 0 deletions bpf/process/bpf_execve_event.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,17 @@ struct {
__type(value, struct msg_data);
} data_heap SEC(".maps");

/* tg_mbset_map holds a mapping from (binary) paths to a bitset of ids that it matches. The map is
* written by user-space and read in the exec hook to determine the bitset of ids of a binary that
* is executed.
*/
struct {
__uint(type, BPF_MAP_TYPE_HASH);
__uint(max_entries, 1024);
__type(key, __u8[MATCH_BINARIES_PATH_MAX_LENGTH]);
__type(value, mbset_t);
kkourt marked this conversation as resolved.
Show resolved Hide resolved
kkourt marked this conversation as resolved.
Show resolved Hide resolved
} tg_mbset_map SEC(".maps");

FUNC_INLINE __u32
read_args(void *ctx, struct msg_execve_event *event)
{
Expand Down Expand Up @@ -258,6 +269,29 @@ execve_rate(void *ctx)
return 0;
}

/* update bitset mark */
FUNC_INLINE
void update_mb_bitset(struct binary *bin)
{
__u64 *bitsetp;
struct execve_map_value *parent;

parent = event_find_parent();
if (parent) {
/* ->mb_bitset is used to track matchBinary matches to children (followChildren), so
* here we propagate the parent value to the child.
*/
bin->mb_bitset = parent->bin.mb_bitset;
}

/* check the map and see if the binary path matches a binary
* NB: only the In operator is supported for now
*/
bitsetp = map_lookup_elem(&tg_mbset_map, bin->path);
if (bitsetp)
bin->mb_bitset |= *bitsetp;
}

/**
* execve_send() sends the collected execve event data.
*
Expand Down Expand Up @@ -344,6 +378,8 @@ execve_send(void *ctx)
curr->bin.path_length--;
}
#endif

update_mb_bitset(&curr->bin);
}

event->common.flags = 0;
Expand Down
3 changes: 2 additions & 1 deletion bpf/process/bpf_process_event.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@

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

#define MAX_BUF_LEN 256
#define MATCH_BINARIES_PATH_MAX_LENGTH 256
Copy link

@vparla vparla Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@kkourt kkourt Jul 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yap, I've also thought the same :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#define MAX_BUF_LEN 256

struct buffer_heap_map_value {
// Buffer is twice the needed size because of the verifier. In prepend_name
Expand Down
11 changes: 9 additions & 2 deletions bpf/process/types/basic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

};

// This map is used by the matchBinaries selectors to retrieve their options
Expand All @@ -1530,8 +1531,6 @@ struct {
__type(value, struct match_binaries_sel_opts);
} tg_mb_sel_opts SEC(".maps");

#define MATCH_BINARIES_PATH_MAX_LENGTH 256

struct {
__uint(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
__uint(max_entries, MAX_SELECTORS); // only one matchBinaries per selector
Expand Down Expand Up @@ -1579,6 +1578,14 @@ FUNC_INLINE int match_binaries(__u32 selidx)

switch (selector_options->op) {
case op_filter_in:
/* Check if we match the selector's bit in ->mb_bitset, which means that the
* process matches a matchBinaries section with a followChidren:true
* attribute either because the binary matches or because the binary of a
* parent matched.
*/
if (current->bin.mb_bitset & (1UL << selector_options->mbset_id))
return 1;
kkourt marked this conversation as resolved.
Show resolved Hide resolved
fallthrough;
case op_filter_notin:
path_map = map_lookup_elem(&tg_mb_paths, &selidx);
if (!path_map)
Expand Down
20 changes: 20 additions & 0 deletions install/kubernetes/tetragon/crds-yaml/cilium.io_tracingpolicies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,10 @@ spec:
description: A list of binary exec name filters.
items:
properties:
followChildren:
description: In addition to binaries, match children
processes of specified binaries.
type: boolean
operator:
description: Filter operation.
enum:
Expand All @@ -458,6 +462,7 @@ spec:
type: string
type: array
required:
- followChildren
- operator
- values
type: object
Expand Down Expand Up @@ -1048,6 +1053,10 @@ spec:
description: A list of binary exec name filters.
items:
properties:
followChildren:
description: In addition to binaries, match children
processes of specified binaries.
type: boolean
operator:
description: Filter operation.
enum:
Expand All @@ -1062,6 +1071,7 @@ spec:
type: string
type: array
required:
- followChildren
- operator
- values
type: object
Expand Down Expand Up @@ -1683,6 +1693,10 @@ spec:
description: A list of binary exec name filters.
items:
properties:
followChildren:
description: In addition to binaries, match children
processes of specified binaries.
type: boolean
operator:
description: Filter operation.
enum:
Expand All @@ -1697,6 +1711,7 @@ spec:
type: string
type: array
required:
- followChildren
- operator
- values
type: object
Expand Down Expand Up @@ -2255,6 +2270,10 @@ spec:
description: A list of binary exec name filters.
items:
properties:
followChildren:
description: In addition to binaries, match children
processes of specified binaries.
type: boolean
operator:
description: Filter operation.
enum:
Expand All @@ -2269,6 +2288,7 @@ spec:
type: string
type: array
required:
- followChildren
- operator
- values
type: object
Expand Down
20 changes: 20 additions & 0 deletions install/kubernetes/tetragon/crds-yaml/cilium.io_tracingpoliciesnamespaced.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,10 @@ spec:
description: A list of binary exec name filters.
items:
properties:
followChildren:
description: In addition to binaries, match children
processes of specified binaries.
type: boolean
operator:
description: Filter operation.
enum:
Expand All @@ -458,6 +462,7 @@ spec:
type: string
type: array
required:
- followChildren
- operator
- values
type: object
Expand Down Expand Up @@ -1048,6 +1053,10 @@ spec:
description: A list of binary exec name filters.
items:
properties:
followChildren:
description: In addition to binaries, match children
processes of specified binaries.
type: boolean
operator:
description: Filter operation.
enum:
Expand All @@ -1062,6 +1071,7 @@ spec:
type: string
type: array
required:
- followChildren
- operator
- values
type: object
Expand Down Expand Up @@ -1683,6 +1693,10 @@ spec:
description: A list of binary exec name filters.
items:
properties:
followChildren:
description: In addition to binaries, match children
processes of specified binaries.
type: boolean
operator:
description: Filter operation.
enum:
Expand All @@ -1697,6 +1711,7 @@ spec:
type: string
type: array
required:
- followChildren
- operator
- values
type: object
Expand Down Expand Up @@ -2255,6 +2270,10 @@ spec:
description: A list of binary exec name filters.
items:
properties:
followChildren:
description: In addition to binaries, match children
processes of specified binaries.
type: boolean
operator:
description: Filter operation.
enum:
Expand All @@ -2269,6 +2288,7 @@ spec:
type: string
type: array
required:
- followChildren
- operator
- values
type: object
Expand Down
1 change: 1 addition & 0 deletions pkg/api/processapi/processapi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ type MsgCapabilities struct {
type Binary struct {
PathLength int64
Path [BINARY_PATH_MAX_LEN]byte
MBSet uint64
}

type MsgNamespaces struct {
Expand Down
20 changes: 20 additions & 0 deletions pkg/k8s/apis/cilium.io/client/crds/v1alpha1/cilium.io_tracingpolicies.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,10 @@ spec:
description: A list of binary exec name filters.
items:
properties:
followChildren:
description: In addition to binaries, match children
processes of specified binaries.
type: boolean
operator:
description: Filter operation.
enum:
Expand All @@ -458,6 +462,7 @@ spec:
type: string
type: array
required:
- followChildren
- operator
- values
type: object
Expand Down Expand Up @@ -1048,6 +1053,10 @@ spec:
description: A list of binary exec name filters.
items:
properties:
followChildren:
description: In addition to binaries, match children
processes of specified binaries.
type: boolean
operator:
description: Filter operation.
enum:
Expand All @@ -1062,6 +1071,7 @@ spec:
type: string
type: array
required:
- followChildren
- operator
- values
type: object
Expand Down Expand Up @@ -1683,6 +1693,10 @@ spec:
description: A list of binary exec name filters.
items:
properties:
followChildren:
description: In addition to binaries, match children
processes of specified binaries.
type: boolean
operator:
description: Filter operation.
enum:
Expand All @@ -1697,6 +1711,7 @@ spec:
type: string
type: array
required:
- followChildren
- operator
- values
type: object
Expand Down Expand Up @@ -2255,6 +2270,10 @@ spec:
description: A list of binary exec name filters.
items:
properties:
followChildren:
description: In addition to binaries, match children
processes of specified binaries.
type: boolean
operator:
description: Filter operation.
enum:
Expand All @@ -2269,6 +2288,7 @@ spec:
type: string
type: array
required:
- followChildren
- operator
- values
type: object
Expand Down
Loading
Loading