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

Process execute failed #4233

Merged
merged 2 commits into from
Sep 5, 2024
Merged

Conversation

OriGlassman
Copy link
Collaborator

1. Explain what the PR does

"Replace me with make check-pr output"

2. Explain how to test it

3. Other comments

@CLAassistant
Copy link

CLAassistant commented Aug 6, 2024

CLA assistant check
All committers have signed the CLA.

@yanivagman
Copy link
Collaborator

Since this PR currently have some issues and we want to proceed with removing sys_enter/exit dependency from this event as well, I added it to my generic kprobes PR at #4256.
If and when this PR will be fixed we can consider replacing the approach.

@yanivagman
Copy link
Collaborator

Eventually picked #4259

@geyslan
Copy link
Member

geyslan commented Aug 26, 2024

@OriGlassman was this resolved by #4259?

@OriGlassman OriGlassman force-pushed the process_execute_failed branch 2 times, most recently from ae0e6d8 to 335479b Compare September 2, 2024 12:47
Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

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

I've tested in three scenarios:

checking ./tests/e2e-inst-signatures/scripts/process_execute_failed.sh

19:55:04:856921  1000   process_execute  94695   94695   -8               process_execute_failed    dirfd: <nil>, flags: <nil>, pathname: /tmp/test.sh, binary.path: /tmp/test.sh, binary.device_id: 44, binary.inode_number: 1861, binary.ctime: 1725494781586232082, binary.inode_mode: 33261, interpreter_path: /tmp/test.sh, stdin_type: 8192, stdin_path: /dev/null, kernel_invoked: 0, argv: [/tmp/test.sh], envp: <nil>

checking a failed execveat:

19:52:46:050970  1000   fake             92599   92599   -14              process_execute_failed    dirfd: 69, flags: 0, pathname: <nil>, binary.path: <nil>, binary.device_id: <nil>, binary.inode_number: <nil>, binary.ctime: <nil>, binary.inode_mode: <nil>, interpreter_path: <nil>, stdin_type: <nil>, stdin_path: <nil>, kernel_invoked: <nil>, argv: [], envp: <nil>

without filter (mostly with only pathname and argv):

19:47:58:827702  1000   code             86452   86452   -2               process_execute_failed    dirfd: <nil>, flags: <nil>, pathname: /home/gg/.goenv/versions/1.22.4/bin/docker, binary.path: <nil>, binary.device_id: <nil>, binary.inode_number: <nil>, binary.ctime: <nil>, binary.inode_mode: <nil>, interpreter_path: <nil>, stdin_type: <nil>, stdin_path: <nil>, kernel_invoked: <nil>, argv: [docker context ls --format {{json .}}], envp: <nil>
...
19:47:58:827727  1000   code             86452   86452   -2               process_execute_failed    dirfd: <nil>, flags: <nil>, pathname: /home/gg/.goenv/bin/docker, binary.path: <nil>, binary.device_id: <nil>, binary.inode_number: <nil>, binary.ctime: <nil>, binary.inode_mode: <nil>, interpreter_path: <nil>, stdin_type: <nil>, stdin_path: <nil>, kernel_invoked: <nil>, argv: [docker context ls --format {{json .}}], envp: <nil>

LGTM overall. I've approved but there's some putting to be considered.

pkg/ebpf/c/tracee.bpf.c Show resolved Hide resolved
Comment on lines +120 to +122
newEvent.Args[parse.ArgIndex(newEvent.Args, "argv")] = execInfo.args[parse.ArgIndex(execInfo.args, "argv")]
newEvent.Args[parse.ArgIndex(newEvent.Args, "envp")] = execInfo.args[parse.ArgIndex(execInfo.args, "envp")]

Copy link
Member

Choose a reason for hiding this comment

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

If index is not found by ArgIndex, this will crash with -1 as index.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed. But the whole logic assumes that tracee fills all arguments, even ones that are not set from kernelspace so there must be argv and envp

events.ModuleLoad: pb.EventId_module_load,
events.ModuleFree: pb.EventId_module_free,
events.ExecuteFinished: pb.EventId_execute_finished,
events.ProcessExecuteFailedInternal: pb.EventId_security_bprm_creds_for_exec,
Copy link
Member

Choose a reason for hiding this comment

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

I suppose that as SecurityBprmCredsForExec was removed as an event (remaining as a probe), this relation should be renamed as well.

Run make -f builder/Makefile.protoc protoc-run to update event.pb.go.

Insert in this PR the commit related to gprc bump (temporarily). So before merge it can be dropped and cherry-picked into a next PR. @rscampos know better about this proceedings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ran that - it didn't seem to change tracee.go, rather api/v1beta1/event.pb.go

@geyslan geyslan merged commit d1eaeef into aquasecurity:main Sep 5, 2024
30 checks passed
@geyslan
Copy link
Member

geyslan commented Sep 5, 2024

@rscampos v0.22.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants