Extract path from linux_binprm in security_bprm_check

[dwindsor]

Is there an existing issue for this?

• I have searched the existing issues

Is your feature request related to a problem?

No response

Describe the feature you would like

With respect to TOCTOU issues, the most atomic way to block execution of a file is via the LSM hook security_bprm_check. This function is executed immediately before the kernel launches a new program, during its search for a handler for the format of the file being executed.

In order to write a TracingPolicy using the path provided to security_bprm_check, we need to add support to Tetragon for extracting the path from struct linux_binprm, the only argument to security_bprm_check.

Testing

The following new process_kprobe was generated by running sample-exec with the following TracingPolicy:

dave@dev:~$ /usr/bin/sample-exec
* Before executing /usr/bin/id
/usr/bin/sample-exec: 4: /usr/bin/id: Operation not permitted
* After executing /usr/bin/id

New process_kprobe for security_bprm_check events

{
  "process_kprobe": {
    "process": {
      "exec_id": "OjI3ODc0MDQ4Mzc3NTczMjoxMDAxODc5",
      "pid": 1001879,
      "uid": 1000,
      "cwd": "/home/dave",
      "binary": "/usr/bin/sample-exec",
      "arguments": "/usr/bin/sample-exec",
      "flags": "execve",
      "start_time": "2024-01-11T23:27:18.871818530Z",
      "auid": 1000,
      "parent_exec_id": "OjI3ODc0MDQ4MzIwMjM0OToxMDAxODc4",
      "refcnt": 1,
      "tid": 1001879
    },
    "parent": {
      "exec_id": "OjI3ODc0MDQ4MzIwMjM0OToxMDAxODc4",
      "pid": 1001878,
      "uid": 1000,
      "cwd": "/home/dave",
      "binary": "/usr/bin/sample-exec",
      "arguments": "/usr/bin/sample-exec",
      "flags": "execve clone",
      "start_time": "2024-01-11T23:27:18.871244532Z",
      "auid": 1000,
      "parent_exec_id": "OjI3NTY3MDM5MDAwMDAwMDo5ODU1MzI=",
      "tid": 1001878
    },
    "function_name": "security_bprm_check",
    "args": [
      {
        "linux_binprm_arg": {
          "path": "/usr/bin/id"
        }
      }
    ],
    "action": "KPROBE_ACTION_OVERRIDE",
    "policy_name": "sample-no-exec-id",
    "return_action": "KPROBE_ACTION_POST"
  },
  "time": "2024-01-11T23:27:18.871925631Z"
}

TracingPolicy

apiVersion: cilium.io/v1alpha1
kind: TracingPolicy
metadata:
  name: "sample-no-exec-id"
spec:
  kprobes:
  - call: "security_bprm_check"
    syscall: false
    args:
    - index: 0
      type: "linux_binprm"
    selectors:
      - matchBinaries:
        - operator: "In"
          values:
            - "/usr/bin/sample-exec"
        matchArgs:
          - index: 0
            operator: "Equal"
            values:
              - "/usr/bin/id"
        matchActions:
        - action: Override
          argError: -1

sample-exec

#!/bin/sh

echo "* Before executing /usr/bin/id"
/usr/bin/id
echo "* After executing /usr/bin/id"

Describe your proposed solution

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

[mtardy]

Hey, thanks for the issue and the PR, that's quite some work.

First, out of curiosity, I wanted to ask why you need security_bprm_check and cannot use security_bprm_creds_from_file. I really don't know the security implication of using one of another and you can already match on file on the second one argument of from_file.

Then, regardless of the above point, it's still interesting to add the linux_binprm as it's a big part of the LSM hooks for files. I was just wondering if it's okay to consider the whole structure as a file and just compare it against it. I mean the linux_binprm struct contains more information that might be useful to retrieve and compare against in TracingPolicies, is it okay to simplify it to a file?

[tixxdz]

Hey, thanks for the issue and the PR, that's quite some work.

First, out of curiosity, I wanted to ask why you need security_bprm_check and cannot use security_bprm_creds_from_file. I really don't know the security implication of using one of another and you can already match on file on the second one argument of from_file.

This is answered here #1986 (comment) , the https://github.com/cilium/tetragon/blob/main/examples/tracingpolicy/process-exec/process-exec-elf-begin.yaml we have only triggers for flat and elf binaries where there are scripts script.sh misc binaries etc

Then, regardless of the above point, it's still interesting to add the linux_binprm as it's a big part of the LSM hooks for files. I was just wondering if it's okay to consider the whole structure as a file and just compare it against it. I mean the linux_binprm struct contains more information that might be useful to retrieve and compare against in TracingPolicies, is it okay to simplify it to a file?

Also answered in same response, linux_binprm in that PR does include path which we should abstract or around file of binprm, so we keep flexibility for the future , there are still lot of other usecases to do with binprm

[dwindsor]

Hey, thanks for the issue and the PR, that's quite some work.
First, out of curiosity, I wanted to ask why you need security_bprm_check and cannot use security_bprm_creds_from_file. I really don't know the security implication of using one of another and you can already match on file on the second one argument of from_file.

security_bprm_check is called after security_bprm_creds_from_file, so it has access to additional state not available at the earlier call site.

For example, bprm_creds_from_file is called at the start of begin_new_exec. After this call, additional state gets encoded into the struct linux_binprm that wouldn't otherwise be available.

This is answered here #1986 (comment) , the https://github.com/cilium/tetragon/blob/main/examples/tracingpolicy/process-exec/process-exec-elf-begin.yaml we have only triggers for flat and elf binaries where there are scripts script.sh misc binaries etc

Then, regardless of the above point, it's still interesting to add the linux_binprm as it's a big part of the LSM hooks for files. I was just wondering if it's okay to consider the whole structure as a file and just compare it against it. I mean the linux_binprm struct contains more information that might be useful to retrieve and compare against in TracingPolicies, is it okay to simplify it to a file?

Also answered in same response, linux_binprm in that PR does include path which we should abstract or around file of binprm, so we keep flexibility for the future , there are still lot of other usecases to do with binprm

Agreed. I think for now we only want to extract the path, so maybe we wait until we have other users of something other than the path in linux_binprm before adding this abstraction?

[tixxdz]

Hey, thanks for the issue and the PR, that's quite some work.
First, out of curiosity, I wanted to ask why you need security_bprm_check and cannot use security_bprm_creds_from_file. I really don't know the security implication of using one of another and you can already match on file on the second one argument of from_file.

security_bprm_check is called after security_bprm_creds_from_file, so it has access to additional state not available at the earlier call site.

For example, bprm_creds_from_file is called at the start of begin_new_exec. After this call, additional state gets encoded into the struct linux_binprm that wouldn't otherwise be available.

This is answered here #1986 (comment) , the https://github.com/cilium/tetragon/blob/main/examples/tracingpolicy/process-exec/process-exec-elf-begin.yaml we have only triggers for flat and elf binaries where there are scripts script.sh misc binaries etc

Then, regardless of the above point, it's still interesting to add the linux_binprm as it's a big part of the LSM hooks for files. I was just wondering if it's okay to consider the whole structure as a file and just compare it against it. I mean the linux_binprm struct contains more information that might be useful to retrieve and compare against in TracingPolicies, is it okay to simplify it to a file?

Also answered in same response, linux_binprm in that PR does include path which we should abstract or around file of binprm, so we keep flexibility for the future , there are still lot of other usecases to do with binprm

Agreed. I think for now we only want to extract the path, so maybe we wait until we have other users of something other than the path in linux_binprm before adding this abstraction?

All right! we may add it before next release or after release depends, but yeh all good ;-)

[mtardy]

Done by #1986.