feat : Add `Capability` operator for `matchArgs`

[tdaudi]

This PR follow the discussion started here https://github.com/cilium/tetragon/pull/3730#discussion_r2086015875

Aim

The purpose of this PR is to allow a user to extract a capability field from the hook parameters and use it against the matchCapabilities tool to compare the capability. This can be useful if you want to detect when a program requests a new capability (for example, in security_capset).

How to test the PR

# Load the policy
./tetra tracingpolicy add examples/tracingpolicy/deny-privileged-containers.yml

# Try to run a normal container. It should work
docker run --rm hello-world

# Run a privileged container. It should return a permission denied
docker run --rm --privileged hello-world

The policy also works for KinD.

What are the major changes

The tail calls order have changed a bit. The tail call FILTER is now run after the SETUP/PROCESS. (See commit message for more details)

[netlify[bot]]

✅ Deploy Preview for tetragon ready!

Name	Link
🔨 Latest commit	1cd2ab3646c27ba9fe3e4282b33d475f11ca074c
🔍 Latest deploy log	https://app.netlify.com/projects/tetragon/deploys/685aeb687e7a3100083a792e
😎 Deploy Preview	https://deploy-preview-3772--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 project configuration.

[tdaudi]

However, one issue with the approach that the PR follows is that now we have two ways for matching arguments (matchArgs and matchCapabilities). Is there a reason why we cannot enhance the existing matchArgs mechanism to do what we want? Especially since it seems that doing so does not require any bpf changes. (See also my other comment.)

In the documentation, it is not very clear that matchCapbabilities and matchNamespaces are meant for task_struct comparison. So since I wanted to compare a capability, I thought that using such a tool was the easiest way. So I was thinking that if I face such problem (compare args: capability), maybe someone else may found the same solution as me (using matchCapability).

Also, my second motivation for doing it this way is that turning CAP_SYS_ADMIN (= 21) to 2097152 is already done in the matchCapabalities so it involves less code duplication.

---

Other than that, I what you suggest @kkourt with CapabilityMask is very good. Probably the easiest solution. But to avoid confusions with matchCapabilities it will need some documentation updates.

[kkourt]

However, one issue with the approach that the PR follows is that now we have two ways for matching arguments (matchArgs and matchCapabilities). Is there a reason why we cannot enhance the existing matchArgs mechanism to do what we want? Especially since it seems that doing so does not require any bpf changes. (See also my other comment.)

In the documentation, it is not very clear that matchCapbabilities and matchNamespaces are meant for task_struct comparison. So since I wanted to compare a capability, I thought that using such a tool was the easiest way. So I was thinking that if I face such problem (compare args: capability), maybe someone else may found the same solution as me (using matchCapability).

Also, my second motivation for doing it this way is that turning CAP_SYS_ADMIN (= 21) to 2097152 is already done in the matchCapabalities so it involves less code duplication.

Other than that, I what you suggest @kkourt with CapabilityMask is very good. Probably the easiest solution. But to avoid confusions with matchCapabilities it will need some documentation updates.

Indeed, we can update the docs to make things clearer.

Also, my second motivation for doing it this way is that turning CAP_SYS_ADMIN (= 21) to 2097152 is already done in the matchCapabalities so it involves less code duplication.

Haven't looked at the code, but we should be able to refactor things so that the code that does the mapping is reused.

[tdaudi]

Moving to draft until the changes discussed are applied

[tdaudi]

The CI fails on 4.19. I guess I should restrict the operator to programs with the __LARGE_BPF_PROG flag ?

EDIT : Done. The feature requires a 5.4+ kernel

[tdaudi]

I pushed an intermediate version to see if 4.19 CI works. If it does, I'll add the NotCapability filter. But it will probably be necessary to restrict it to kernel >5.3 because the filter tail call has too many instructions already.

[tdaudi]

Latest change includes :

• Changing operator name from Capability to CapabilityMask
• Removing all reference to the new op_filter_capability. The workflow now follows Mask operator.
• Mask operator now handles kernel_cap_t and cap_* types
• Updating the docs details to explain this Mask behavior. (Let me know if it is understandable enough)

[kkourt]

@tdaudi I was trying to fix a bug (which now I realized that you had also fixed in your PR), and ended up with: https://github.com/cilium/tetragon/pull/3852. WDYT?

[tdaudi]

lgtm, needs rebase though

Do you think the PR is good to merge with such implementation ? With the discussion in https://github.com/cilium/tetragon/pull/3852#issuecomment-3008509829, and with all the messages before, I think that the PR can be improved.

To summarize,

What is my problem

I need to check if one or many capabilities exist in a u64 field. Let's say the extracted value = 0xffffffffffffffff, but I want to see if only the CAP_SYS_ADMIN capability exists. For that, I need to convert CAP_SYS_ADMIN to a mask and use it. The current state of the PR solves this problem.

But what if I want to check if both CAP_A AND CAP_B exists? The Mask operator does not provide a way to do so (I give more context about it here https://github.com/cilium/tetragon/pull/3852#discussion_r2166696377).

Changing the Mask operator may be a breaking change because ATM it checks a single bit of the mask is set to true while I want all the bit of the mask to return true.

What I suggest

Should we introduce a new mask operator for this specific case ? Rename the current Mask operator to something like MaskContain (which means one bit of the mask exists) and create a new name MaskInclude (all the mask exists in the extracted value). Later, another MaskExclude could make sense, as suggested in this comment https://github.com/cilium/tetragon/pull/3772#discussion_r2159708529

The Include and Exclude semantic also make sense for strings, so maybe there is something to push this way

What do you think is the best option here ? CC: @kkourt

EDIT : This small change may be good enough

diff --git a/bpf/process/types/basic.h b/bpf/process/types/basic.h
index 1bbea6abd..09a5ba93b 100644
--- a/bpf/process/types/basic.h
+++ b/bpf/process/types/basic.h
@@ -1221,7 +1221,7 @@ filter_64ty_selector_val(struct selector_arg_filter *filter, char *args)
                                return 1;
                        break;
                case op_filter_mask:
-                       if (*(u64 *)args & w)
+                       if ((*(u64 *)args & w) == w)
                                return 1;
                default:
                        break;
@@ -1317,7 +1317,7 @@ filter_32ty_selector_val(struct selector_arg_filter *filter, char *args)
                                return 1;
                        break;
                case op_filter_mask:
-                       if (*(u32 *)args & w)
+                       if ((*(u32 *)args & w) == w)
                                return 1;
                default:
                        break;

[olsajiri]

But what if I want to check if both CAP_A AND CAP_B exists? The Mask operator does not provide a way to do so (I give more context about it here #3852 (comment)).

yea, current mask operator succeeds when one (or more) values are set, if you need to check all the values, do we need to do something like:

--- a/bpf/process/types/basic.h
+++ b/bpf/process/types/basic.h
@@ -1184,6 +1184,7 @@ FUNC_LOCAL long
 filter_64ty_selector_val(struct selector_arg_filter *filter, char *args)
 {
        __u64 *v = (__u64 *)&filter->value;
+       bool mask_all = true;
        int i, j = 0;
 
 #pragma unroll
@@ -1223,6 +1224,9 @@ filter_64ty_selector_val(struct selector_arg_filter *filter, char *args)
                case op_filter_mask:
                        if (*(u64 *)args & w)
                                return 1;
+                       break;
+               case op_filter_mask_all:
+                       mask_all &= *(u64 *)args & w;
                default:
                        break;
                }
@@ -1230,6 +1234,9 @@ filter_64ty_selector_val(struct selector_arg_filter *filter, char *args)
                if (j + 8 >= filter->vallen)
                        break;
        }
+
+       if (filter->op == op_filter_mask_all && mask_all)
+               return 1;
        return 0;
 }

[kkourt]

@tdaudi do you still plan to merge this PR given that we merged https://github.com/cilium/tetragon/pull/3852?

[tdaudi]

@tdaudi do you still plan to merge this PR given that we merged #3852?

If no further changes are needed, I'm closing this PR. Thanks a lot for #3852