Extract jevt array misses as novalue #1601
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While testing, I found a case when creating a pod where: 1) the first container had no securityContext value 2) the second container had a security context with privileged=true and this did not match the default rule Create Privileged Pod, when it should match. The rule Create Privileged Pod uses the field ka.req.pod.containers.privileged, which in turn uses json_event_filter_check::def_extract(). def_extract() iterates over a set of json_pointers, potentially expanding arrays as they are returned. Many k8s audit fields use this extract function. For ka.req.pod.containers.privileged, the first json_pointer is /requestObject/spec/containers to find the list of containers, and the second is /securityContext/privileged to extract the privileged property out of the securityContext object. What's returned is an array of true/false noting if each container is privileged. The problem is that def_extract() aborts when iterating over arrays if extracting a pointer from an array can't be done. In this case, the first pointer extracts the array of containers, and then when iterating over the array of containers, the security context pointer doesn't extract, causing the whole filter field to abort and return ::no_value. The fix is to not abort when iterating over arrays, but use ::no_value for that array item's value instead. This allows def_extract() to extract the privileged value out of the second container. Signed-off-by: Mark Stemm <[email protected]>
Add a test that verifies that a pod where one container has no security context and the second container has a security context + privileged properly matches the Create Privileged Pod falco rule. There's a very similar test case already in trace_files/k8s_audit/create_nginx_pod_privileged_2nd_container.json, but in that case both containers have a securityContext property. Signed-off-by: Mark Stemm <[email protected]>
fntlnz
reviewed
Apr 7, 2021
| @@ -128,6 +128,16 @@ trace_files: !mux | |||
| - Create Privileged Pod: 1 | |||
| trace_file: trace_files/k8s_audit/create_nginx_pod_privileged.json | |||
|
|
|||
| create_privileged_no_secctx_1st_container_2nd_container_pod: | |||
There was a problem hiding this comment.
I don't understand how this test is related to the NA value in the code. Can you help me undertstand?
There was a problem hiding this comment.
This test ensures that for a k8s audit event where the first container doesn't have any security context and the 2nd container does have a security context and privileged is true, you still get a falco alert for the Create Privileged Pod rule.
Behind the scenes, what's changing is that older falco versions used to abort when trying to get the value for the ka.req.pod.containers.privileged field. Whenever a field can't be extracted from an event, the result is <NA>. With this fix, what's returned instead is an array with values for each container, where each container will have "true" if the security context is present and true, "false" if present and false, and <NA> if not present. Here's an example minimal rules file that demonstrates the difference:
- list: k8s_audit_stages
items: ["ResponseComplete"]
- macro: kevt
condition: (jevt.value[/stage] in (k8s_audit_stages))
- macro: kcreate
condition: ka.verb=create
- macro: pod
condition: ka.target.resource=pods and not ka.target.subresource exists
- rule: Create Pod
desc: Detect any attempt to create a pod
condition: kevt and pod and kcreate
output: Pod created (user=%ka.user.name pod=%ka.resp.name ns=%ka.target.namespace images=%ka.req.pod.containers.image privileged=%ka.req.pod.containers.privileged)
priority: INFO
source: k8s_audit
tags: [k8s]
$ sudo falco -r ~/create_pod.yaml -e create_nginx_pod_no_secctx_1st_container_privileged_2nd_container.json
Wed Apr 7 10:13:33 2021: Falco version 0.27.0-84+8c9d4f4 (driver version 5c0b863ddade7a45568c0ac97d037422c9efb750)
Wed Apr 7 10:13:33 2021: Falco initialized with configuration file /etc/falco/falco.yaml
Wed Apr 7 10:13:33 2021: Loading rules from file /home/mstemm/create_pod.yaml:
Wed Apr 7 10:13:33 2021: Reading k8s audit events from file: /mnt/sf_mstemm/work/src/falco/test/trace_files/k8s_audit/create_nginx_pod_no_secctx_1st_container_privileged_2nd_container.json
Events detected: 1
Rule counts by severity:
INFO: 1
Triggered rules by rule name:
Create Pod: 1
Syscall event drop monitoring:
- event drop detected: 0 occurrences
- num times actions taken: 0
10:53:07.006844928: Notice Pod created (user=system:serviceaccount:kube-system:replicaset-controller pod=nginx-deployment-544b59f8b8-ffkxm ns=default images=(nginx,mysql:latest) privileged=<NA>)
$ sudo ./userspace/falco/falco -r ~/create_pod.yaml -e create_nginx_pod_no_secctx_1st_container_privileged_2nd_container.json
Wed Apr 7 10:14:09 2021: Falco version 0.27.0-84+8c9d4f4 (driver version 5c0b863ddade7a45568c0ac97d037422c9efb750)
Wed Apr 7 10:14:09 2021: Falco initialized with configuration file /etc/falco/falco.yaml
Wed Apr 7 10:14:09 2021: Loading rules from file /home/mstemm/create_pod.yaml:
Wed Apr 7 10:14:09 2021: Reading k8s audit events from file: /mnt/sf_mstemm/work/src/falco/test/trace_files/k8s_audit/create_nginx_pod_no_secctx_1st_container_privileged_2nd_container.json
Events detected: 1
Rule counts by severity:
INFO: 1
Triggered rules by rule name:
Create Pod: 1
Syscall event drop monitoring:
- event drop detected: 0 occurrences
- num times actions taken: 0
10:53:07.006844928: Notice Pod created (user=system:serviceaccount:kube-system:replicaset-controller pod=nginx-deployment-544b59f8b8-ffkxm ns=default images=(nginx,mysql:latest) privileged=(<NA>,true))
There was a problem hiding this comment.
Thanks for the exhaustive explanation Mark!
leodido
approved these changes
Apr 9, 2021
|
LGTM label has been added. Git tree hash: 4cee3e32a8b9449b756c20be6114da7bb74b21d3
|
|
/milestone 0.28.0 |
fntlnz
approved these changes
Apr 9, 2021
leogr
approved these changes
Apr 9, 2021
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fntlnz, leodido, leogr The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
jasondellaluce
added a commit
to falcosecurity/plugins
that referenced
this pull request
Apr 21, 2022
In the legacy implementation, list values used to include the value <NA> whenever a given field could not be extracted (see: falcosecurity/falco#1601). However, this makes the semantics of missing values ambigue and not coherent. Instead, here we just skip missing values. If a value is not found when extracted, the plugin framework is able to signal the "field is not present" info. For list values, missing values are simply skipped now. This works as expected with operators such as `in` and `intersects`, for which the <NA> string was ambiguous anyways. If a list field is accessed with an index argument, then the extracted value can either be a list with a single value (the one actually extracted at the positional index of the arg), or an empty string (which is interpreted by the framework as <NA> internally). Signed-off-by: Jason Dellaluce <[email protected]>
poiana
pushed a commit
to falcosecurity/plugins
that referenced
this pull request
Apr 22, 2022
In the legacy implementation, list values used to include the value <NA> whenever a given field could not be extracted (see: falcosecurity/falco#1601). However, this makes the semantics of missing values ambigue and not coherent. Instead, here we just skip missing values. If a value is not found when extracted, the plugin framework is able to signal the "field is not present" info. For list values, missing values are simply skipped now. This works as expected with operators such as `in` and `intersects`, for which the <NA> string was ambiguous anyways. If a list field is accessed with an index argument, then the extracted value can either be a list with a single value (the one actually extracted at the positional index of the arg), or an empty string (which is interpreted by the framework as <NA> internally). Signed-off-by: Jason Dellaluce <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind bug
Any specific area of the project related to this PR?
/area engine
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: