Postfix operator for matchBinaries selector

[anfedotoff]

Details can be found in #2679.

• BPF selector part
• Tetragon selector part
• Postfix operator test for matchBinaries selector
• Update docs

Add the Postfix and NotPostfix operators to the matchBinaries selector.

[netlify]

✅ Deploy Preview for tetragon ready!

Name	Link
🔨 Latest commit	a750259
🔍 Latest deploy log	https://app.netlify.com/sites/tetragon/deploys/66ba07b02e4fa7000898a747
😎 Deploy Preview	https://deploy-preview-2689--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.

[mtardy]

Thanks that's a great addition! Would love an ack from Kev since we are touching a few bits of its STRING code.

My main nit would be to add a test using the perfring framework to check for a missing event so that we are sure this thing works as intended, and I'm good! :)

[mtardy]

I think the limitation here is outdated but it's out of the scope of this PR

[kevsecurity]

I think we need to store the last 128 chars of the binary path in execve_send (bpf/process/bpf_execve_event.c) in a new part of the struct binary, specifically for postfix matching.
I think we need a copy of copy_reverse() (called file_copy_reverse()) that only copies file paths from buffer expected to be BINARY_PATH_MAX_LENGTH in size, leaving the original with the PATH_MASK position masking.

[mtardy]

I think we need to store the last 128 chars of the binary path in execve_send (bpf/process/bpf_execve_event.c) in a new part of the struct binary, specifically for postfix matching.

I think we need a copy of copy_reverse() (called file_copy_reverse()) that only copies file paths from buffer expected to be BINARY_PATH_MAX_LENGTH in size, leaving the original with the PATH_MASK position masking.

Thanks kev I completely overlooked this previous struggle. Indeed, for the binary we don't store the whole path so you have a few cases to take into account:

• path is short enough you can do what you did
• path is too long and you need to store at exec time the end of it (at least 128 chars). You can lazily reverse it when needed.

[kevsecurity]

Thanks kev I completely overlooked this previous struggle. Indeed, for the binary we don't store the whole path so you have a few cases to take into account:

• path is short enough you can do what you did
• path is too long and you need to store at exec time the end of it (at least 128 chars). You can lazily reverse it when needed.

Personally, I'd always copy the end into the new end slot to save extra logic when matching. I agree that you should lazily reverse it the first time you do a postfix match on it, but this has race conditions. I think the simple solution (not necessarily the cheapest or best) is to have two extra slots, not one extra slot! So struct binary could look like:

struct binary {
	// length of the path stored in path, this should be < BINARY_PATH_MAX_LEN
	// but can contain negative value in case of copy error.
	// While s16 would be sufficient, 32 bits are handy for alignment.
	__s32 path_length;
	__u8 reversed;
	__u8 pad[3];
	// BINARY_PATH_MAX_LEN first bytes of the path
	char path[BINARY_PATH_MAX_LEN];
	char end[BINARY_PATH_MAX_LEN];
	char end_r[BINARY_PATH_MAX_LEN];
}; // All fields aligned so no 'packed' attribute

On execution, copy the end of the path into end. On first postfix lookup, reverse end into end_r and set reversed to true. If another thread is doing the same then end will be unaffected, so end_r will end up with the same values, even if one thread is still reversing end into end_r while another is using end_r. If reversed is true, then just match on end_r and assume it has already been reversed.

[anfedotoff]

Thanks kev I completely overlooked this previous struggle. Indeed, for the binary we don't store the whole path so you have a few cases to take into account:

• path is short enough you can do what you did
• path is too long and you need to store at exec time the end of it (at least 128 chars). You can lazily reverse it when needed.

Personally, I'd always copy the end into the new end slot to save extra logic when matching. I agree that you should lazily reverse it the first time you do a postfix match on it, but this has race conditions. I think the simple solution (not necessarily the cheapest or best) is to have two extra slots, not one extra slot! So struct binary could look like:

struct binary {
	// length of the path stored in path, this should be < BINARY_PATH_MAX_LEN
	// but can contain negative value in case of copy error.
	// While s16 would be sufficient, 32 bits are handy for alignment.
	__s32 path_length;
	__u8 reversed;
	__u8 pad[3];
	// BINARY_PATH_MAX_LEN first bytes of the path
	char path[BINARY_PATH_MAX_LEN];
	char end[BINARY_PATH_MAX_LEN];
	char end_r[BINARY_PATH_MAX_LEN];
}; // All fields aligned so no 'packed' attribute

On execution, copy the end of the path into end. On first postfix lookup, reverse end into end_r and set reversed to true. If another thread is doing the same then end will be unaffected, so end_r will end up with the same values, even if one thread is still reversing end into end_r while another is using end_r. If reversed is true, then just match on end_r and assume it has already been reversed.

Thanks for such detailed advice! I'll try to implement the logic you described.

Remember that they are challenges for long path

[mtardy]

I just remember I had a very early draft branch from October but I don't think it'll really help you here, it was similar to the advice given here.

[anfedotoff]

@mtardy , @kevsecurity , guys, can I ask you to review again, please?

[mtardy]

@mtardy , @kevsecurity , guys, can I ask you to review again, please?

sure, note that you can use that btw even if a message is fine of course.

[mtardy]

Thanks a lot for those updates :), here are some comments. My main point would be to add a test with a long binary like in 67348f6 so that we make sure this PR works and doesn't break.

[kevsecurity]

It's looking good. Address the few things including some tests and we should be good to merge.

[kevsecurity]

LGTM

[mtardy]

Thanks, it's almost ready, the only small blocker for me is to have a first commit that compiles & runs (the struct should be synchronized between kernel/user), the rest is nits and followups.

[mtardy]

nit: you could also write revlen into the struct so that you don't need to recompute it again, not sure it's better than this but you could downsize again here and use 16/16 bytes, so that you get path_len and end_len/rev_len?

// if end_r contains reversed path postfix
__u32 reversed;

[anfedotoff]

@mtardy , I think now it's ready:)

[mtardy]

@mtardy , I think now it's ready:)

Yep thanks again, let's merge this! 🌮 🎉