-
Notifications
You must be signed in to change notification settings - Fork 424
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
feat: remove default usage of parse-arguments #4331
feat: remove default usage of parse-arguments #4331
Conversation
9b98630
to
3330712
Compare
10f7b81
to
f5a2cc5
Compare
f5a2cc5
to
a0de6c0
Compare
Co-authored-by: Geyslan Gregório <geyslan@gmail.com>
Removing default argument parsing will break rego signatures, whose implementation depends on parsed arguments. In order to keep the option of readability in REGO signatures, events will be parsed by default in the context of evaluating these kind of signatures. This will also ensure that they will not be broken depending on the selection of parse-arguments. Co-authored-by: Geyslan Gregório <geyslan@gmail.com>
Refactor 'event feeding to engine' logic to copy the event before parsing it, cloning its arguments.
a0de6c0
to
47451f5
Compare
/fast-forward |
Addendum: Analyze mode (requires events with raw args)
{"level":"info","ts":1728558831.3532736,"msg":"Signatures loaded","total":1,"signatures":["Anti-Debugging detected"]}
{"timestamp":1728622986623810048,"threadStartTime":1728622986623762297,"processorId":6,"processId":531875,"cgroupId":20475,"threadId":531875,"parentProcessId":531873,"hostProcessId":531875,"hostThreadId":531875,"hostParentProcessId":531873,"userId":1000,"mountNamespace":4026531841,"pidNamespace":4026531836,"processName":"strace","executable":{"path":""},"hostName":"hb0","containerId":"","container":{},"kubernetes":{},"eventId":"6000","eventName":"anti_debugging","argsNum":1,"returnValue":0,"syscall":"ptrace","stackAddresses":null,"contextFlags":{"containerStarted":false,"isCompat":false},"threadEntityId":2721268392,"processEntityId":2721268392,"parentEntityId":2984027261,"args":[{"name":"triggeredBy","type":"unknown","value":{"args":[{"name":"request","type":"long","value":0},{"name":"pid","type":"pid_t","value":0},{"name":"addr","type":"void*","value":0},{"name":"data","type":"void*","value":0}],"id":101,"name":"ptrace","returnValue":0}}],"metadata":{"Version":"1","Description":"A process used anti-debugging techniques to block a debugger. Malware use anti-debugging to stay invisible and inhibit analysis of their behavior.","Tags":null,"Properties":{"Category":"defense-evasion","Kubernetes_Technique":"","Severity":1,"Technique":"Debugger Evasion","external_id":"T1622","id":"attack-pattern--e4dc8c01-417f-458d-9ee0-bb0617c1b391","signatureID":"TRC-102","signatureName":"Anti-Debugging detected"}}} |
// Parse args here if the rule engine is NOT enabled (parsed there if it is). | ||
if t.config.Output.ParseArguments && !t.config.EngineConfig.Enabled { |
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.
Does this still hold?
If I understand correctly, parse arguments is no longer "always on" when the rule engine is enabled.
So shouldn't the condition be if t.config.Output.ParseArguments
only?
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.
TODO: #4368
if t.config.Output.ParseArguments { | ||
// shallow clone the event arguments before parsing them (new slice is created), | ||
// to keep the eventCopy with raw arguments. | ||
eventCopy.Args = slices.Clone(event.Args) | ||
|
||
err := t.parseArguments(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.
Why do we need to parse the arguments here anyway?
- If signatures are now using raw arguments - won't this be a bug?
- Shouldn't we leave data parsing to the sink stage?
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 agree with you, but we still need it here until the internal migration is finalised.
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.
Do we have a "todo" opened so we won't forget to remove it once finalised?
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.
TODO: #4368
Close: #2177
1. Explain what the PR does
47451f5 feat(cmd): disable arg parsing by default
cb48e1f fix: possible overwrite of Args on Derive stage
3cfe5c5 chore(ebpf): copy event before parsing it
0fce183 feat(rego): parse event arguments in sig
ec478cc feat(sigs): refactor to use nonparsed arguments
ddc219a chore(go.mod): bump sig helpers to 0b23713
3cfe5c5 chore(ebpf): copy event before parsing it
0fce183 feat(rego): parse event arguments in sig
ec478cc feat(sigs): refactor to use nonparsed arguments
2. Explain how to test it
-e ptrace -o json
-e ptrace -o json -o option:parse-arguments
-e ptrace,anti_debugging -o json
-e ptrace,anti_debugging -o json -o option:parse-arguments
-e clone -o json
-e clone -o json -o option:parse-arguments
Analyze mode (requires events with raw args)
-e ptrace -o json:output.json
analyze -e anti_debugging output.json
3. Other comments