-
Notifications
You must be signed in to change notification settings - Fork 434
feat: collect environment variables in execve events #3797
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the PR, i think this is super useful ❤️
- could we do something similar to https://github.com/cilium/tetragon/pull/1296/files#diff-de32ec7437bcdafd9a6442b53e938fbf4a100cc12b014b28412e0e1a3146e510R264 and set the
environment_variables
field only forprocess_exec
events to avoid increasing sizes of other events? - probably makes sense to put this behind a feature flag (
--enable-environment-variables
or something). some users might not want to expose environment variables as tetragon events, as they might contain sensitive information. - alternatively, make it configurable which environment variables get exported, like
--export-environment-variables=LD_FLAGS,HTTP_PROXY
. then default to an empty list, which effectively disables the feature, and maybe--export-environment-variables=*
to export all the environment variables 💭
I also think exposing a feature flag for this is a good idea. Although BPF code is likely not too much overhead it also isn't free so if folks don't want to include it I think its easier to just have a big switch. Even with the flag having filters would be nice to have. But, if with the flag I think we can do filters as a follow up. |
@michi-covalent @jrfastab do you think it makes sense to parse them into a map as well then? |
06b43e0
to
45b9c8c
Compare
029f7ef
to
5bb052b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the change, looks good
please split the generated changes in separate commits, I left some other comments
// EnvironmentVariables is a string map of unprocessed environment variables | ||
// that were passed to the process. This is not stored into the process, | ||
// but is used to construct the ProcessExec event. | ||
environmentVariables string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont understand the command, what you mean by 'unprocessed environment variables' and 'this is not stored in the process' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to binary properties above, it's stored as part of the ProcessExec
, but not Process
@@ -229,6 +302,7 @@ event_execve(struct exec_ctx_struct *ctx) | |||
p->size += read_path(ctx, event, filename); | |||
p->size += read_args(ctx, event); | |||
p->size += read_cwd(ctx, p); | |||
p->size += read_envs(ctx, event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we have the flag to enable this, but the bpf and parsing code is unconditional, right?
I think it should be behind some config check, and I wonder we could put it in tg_conf_map ? not sure..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could move the feature flag handling completely into the bpf code, i like that, could do if you green-light patching tg_conf_map
@slntopp could you also please update the docs? and if possible it'd be great to have some filtering possibility, probably passed in as and options like --environment-variables="VAR1,VAR2,!VAR3,.." or any better way you could think of ;-) thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
protobuf & command line flag LGTM ❤️
c5e2821
to
91fbbf3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the change, left some comments and some notes:
- provide full commit subject and message for 2nd commit
- keep just 1 Signed-off-by in 1st commit together with description of the changes
- the change in execParse needs tests, please add
- CI fails on existing execParse tests
- you still read the environment unconditionally
// The content of 'args' is uncertain. For safety, assume no reliable args/envs. | ||
// Alternatively, one might try to parse 'args' if BPF guarantees payload structure despite filename error. | ||
// For now, if EventErrorFilename is set, we won't attempt to parse args/envs from this buffer. | ||
args = nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like separate fix, please move it to separate commit
1c7d28f
to
0d067ad
Compare
@@ -10,6 +10,7 @@ type TetragonConf struct { | |||
TgCgrpHierarchy uint32 `align:"tg_cgrp_hierarchy"` // Tetragon Cgroup tracking hierarchy ID | |||
TgCgrpSubsysIdx uint32 `align:"tg_cgrp_subsys_idx"` // Tracking Cgroup css idx at compile time | |||
TgCgrpLevel uint32 `align:"tg_cgrp_level"` // Tetragon cgroup level | |||
EnvVarsEnabled uint64 `align:"env_vars_enabled"` // Whether to read environment variables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has to be uint64
due to the Golang's structs alignment
0d067ad
to
76c1cdb
Compare
This patch adds: 1. Reads environment variables from the process memory during execve events. 2. Adds a new field `envs` to the `ExecveEvent` message in the Tetragon API. 3. Updates the BPF code to collect these environment variables. 4. Updates the Go API to include the new `envs` field in the `ExecveEvent` struct. 5. Adds `--enable-process-environment-variables` flag to conditionally read env vars with ProcessExec events Partially resolves cilium#2648 Signed-off-by: Mikita Iwanowski <[email protected]>
- protobuf - cli doc Signed-off-by: Mikita Iwanowski <[email protected]>
Handle potentially corrupted buffer data in execParse Signed-off-by: Mikita Iwanowski <[email protected]>
76c1cdb
to
538029d
Compare
Hey @olsajiri which docs or other patches are prio to complete this patch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaving some more comments, plus CI is failing, thanks
read_envs(void *ctx, struct msg_execve_event *event) | ||
{ | ||
__s32 key = 0; | ||
struct tetragon_conf *conf = map_lookup_elem(&tg_conf_map, &key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add empty line between declarations and the code
return 0; | ||
|
||
struct task_struct *task = (struct task_struct *)get_current_task(); | ||
struct msg_process *p = &event->process; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stuck varibles declarations together at the top of the function
return total_event_bytes_written; | ||
|
||
if (envs_data_len < BUFFER && envs_data_len < remaining_space_in_event_buf) { | ||
__u32 bytes_to_copy = envs_data_len & 0x3ff; /* BUFFER - 1 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add empty line between declarations and the code
probe_read(&env_start_addr, sizeof(env_start_addr), _(&mm->env_start)); | ||
probe_read(&env_end_addr, sizeof(env_end_addr), _(&mm->env_end)); | ||
|
||
if (!env_start_addr || !env_end_addr || env_start_addr >= env_end_addr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't use {} for conditions with oneline code paths
@@ -144,6 +144,9 @@ func execParse(reader *bytes.Reader) (processapi.MsgProcess, bool, error) { | |||
proc.Filename = strutils.UTF8FromBPFBytes(args[:]) | |||
args = nil | |||
} | |||
} else if exec.Flags&api.EventErrorFilename != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be in commit together with the hunk below, no?
also I think this commit should go in before the env changes
diff --git a/pkg/sensors/exec/exec_linux.go b/pkg/sensors/exec/exec_linux.go
index 47a6e3fe24ae..5e9d1912a129 100644
--- a/pkg/sensors/exec/exec_linux.go
+++ b/pkg/sensors/exec/exec_linux.go
@@ -139,41 +139,131 @@ func execParse(reader *bytes.Reader) (processapi.MsgProcess, bool, error) {
if n != -1 {
proc.Filename = strutils.UTF8FromBPFBytes(args[:n])
args = args[n+1:]
+ } else {
+ // Filename not null-terminated or buffer consumed
+ proc.Filename = strutils.UTF8FromBPFBytes(args[:])
+ args = nil
}
}
var argsPayload []byte | ||
var envsPayload []byte | ||
|
||
// --- Robust args/envs split logic --- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change is too big and scary on very crucial code
if you need to change current args retrieval to fit in the environment stuff,
please do that in multiple incremental changes, so we can be sure we don't break things
@@ -126,6 +127,84 @@ read_args(void *ctx, struct msg_execve_event *event) | |||
return size; | |||
} | |||
|
|||
FUNC_INLINE __u32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please split ebpf and golang change in separate commits (ebpf goes first)
probably in here https://github.com/cilium/tetragon/blob/main/docs/content/en/docs/concepts/events.md
ok |
This patch adds:
envs
to theExecveEvent
message in the Tetragon API.envs
field in theExecveEvent
struct.Partially resolves #2648
Description
We're trying to develop policies that would detect the malicious use of the
LD_FLAGS
andHTTP_PROXY
environment variables, this PR brings in the patch that would enable it.Example Event
Example event:
Click to expand