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.

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

tetragon: harden actions a bit #3279

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

tetragon: harden actions a bit #3279

wants to merge 10 commits into from

Conversation

olsajiri
Copy link
Contributor

@olsajiri olsajiri commented Jan 7, 2025

when execve_map is full, create local process record in kprobe context and use it
to do the filter and execute actions

on user space side such process is tracked as 'unknown'

{
  "process_kprobe": {
    "process": {
      "binary": "unknown"
    },
    "parent": {
      "binary": "unknown"
    },
    "function_name": "__x64_sys_symlinkat",
    "args": [
      {
        "string_arg": "/tmp/passwd"
      },
      {
        "int_arg": -100
      },
      {
        "string_arg": "/tmp/link"
      }
    ],
    "action": "KPROBE_ACTION_OVERRIDE",
    "policy_name": "sys-symlink-passwd",
    "return_action": "KPROBE_ACTION_POST"
  },
  "node_name": "ubuntu-24",
  "time": "2025-01-21T10:56:52.279276243Z"
}

@olsajiri olsajiri added the release-note/minor This PR introduces a minor user-visible change label Jan 7, 2025
@olsajiri olsajiri force-pushed the pr/olsajiri/harden branch 3 times, most recently from 7b664dd to ab3513b Compare January 7, 2025 13:31
Comment on lines 109 to 110
threads := readFileDefault("/proc/sys/kernel/threads-max", 32768)
ExecveMap.SetMaxEntries(int(threads))
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this? It seems it will make the size of the execve_map even larger as threads-max is often well above 32K. This will take significant space while running threads-max threads is pretty rare nop?

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 tried this as mitigation for https://github.com/isovalent/security/issues/88 .. I think we need some combination of this change (with some reasonable size for execve_map) and other ways mentioned in the issue

Copy link
Member

Choose a reason for hiding this comment

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

ah I see, maybe this is one of the cases where NO_PREALLOC could help so that we can dynamically size to a very large map. But I could see how this can lead to memory issues in the future. That's not an easy problem :/

@olsajiri olsajiri force-pushed the pr/olsajiri/harden branch 2 times, most recently from c64e7e4 to fcfa0a9 Compare January 13, 2025 14:28
Copy link

netlify bot commented Jan 13, 2025

Deploy Preview for tetragon ready!

Name Link
🔨 Latest commit 6922611
🔍 Latest deploy log https://app.netlify.com/sites/tetragon/deploys/67a0e5c46b3e55000898b260
😎 Deploy Preview https://deploy-preview-3279--tetragon.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -416,4 +419,6 @@ func AddFlags(flags *pflag.FlagSet) {
flags.Int(KeyEventCacheRetryDelay, defaults.DefaultEventCacheRetryDelay, "Delay in seconds between event cache retries")

flags.Bool(KeyCompatibilitySyscall64SizeType, false, "syscall64 type will produce output of type size (compatibility flag, will be removed in v1.4)")

flags.String(KeyExecveMapEntries, "", "Set entries for execve_map table (default 32768)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have the default from the actual default value here? If we ever change it, the change should be reflected in help.

@olsajiri olsajiri force-pushed the pr/olsajiri/harden branch 7 times, most recently from db51eba to 44fdaa4 Compare January 21, 2025 11:17
Adding Maps array to loader LoadOpts object as additional source
of user maps for loader maps resolving.

This will allow program loader to use user maps pinned to another
program and will ease up maps sharing for sensors in following
changes.

At the moment for simplicity we will pass all the sensors maps,
so we filter out user maps before using the array.

Signed-off-by: Jiri Olsa <[email protected]>
Passing sensors maps to program loader so the loader
has access to all sensor maps.

Signed-off-by: Jiri Olsa <[email protected]>
Now that the loader can access all sensor maps we no longer need
to pin user maps to programs.

Also for static maps we can add new program.MapUserFrom interface
where it's enough just to pass map pointer to create its user map.

Signed-off-by: Jiri Olsa <[email protected]>
Adding owner info when printing map object, like:
  Map{Name:m1 PinPath:m1 Owner: false}

Signed-off-by: Jiri Olsa <[email protected]>
Adding support to allow to setup execve_map max entries,
so we can control the size of this map.

Signed-off-by: Jiri Olsa <[email protected]>
Adding execve-map-entries option to setup entries of execve_map map.

Signed-off-by: Jiri Olsa <[email protected]>
so filter tail call can change it

Signed-off-by: Jiri Olsa <[email protected]>
@olsajiri olsajiri changed the title tetragon: Setup execve_map max entries tetragon: harden actions a bit Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor This PR introduces a minor user-visible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants