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

Handle zero-value types for unavailable fields - ArgMeta #4336

Merged
merged 3 commits into from
Oct 8, 2024

Conversation

geyslan
Copy link
Member

@geyslan geyslan commented Oct 4, 2024

Close: #4335

1. Explain what the PR does

2f31471 fix(events): setuid16 arg name typo
4737625 fix: handle zero-value types for unavailable fields
41eeb29 chore(go.mod): bump types to latest version

2f31471 fix(events): setuid16 arg name typo

'git log --grep old_old_uid_t' in Linux kernel code didn't return any
results.

4737625 fix: handle zero-value types for unavailable fields

Some event arguments, such as "interpreter_*" from sched_process_exec,
were not being populated because the kernel might not always provide
them. This was causing errors during the gRPC proto conversion, as
the values couldn't be asserted - they were nil.

With this change, ArgMeta now holds the Zero value of the type, ensuring
that assignment is always possible without requiring additional parsing
in the pipeline.

This also might help parsing since the Zero field will always have
value, making it easier to assert its type instead of checking for
Name field (string).

2. Explain how to test it

3. Other comments

@geyslan
Copy link
Member Author

geyslan commented Oct 4, 2024

@trvll do you mind testing?

go.mod Outdated Show resolved Hide resolved
@geyslan geyslan force-pushed the 4335-fix-missing-values branch 2 times, most recently from 8a96e92 to 1cc78f0 Compare October 4, 2024 23:31
@NDStrahilevitz
Copy link
Collaborator

NDStrahilevitz commented Oct 7, 2024

I'll review this thoroughly tomorrow.
From a quick glance I have these to say:

  1. We should use the effort of type conversion here and already move to hardcoded ArgType enums. These should be used instead of the strings. (We can then add a timestamp argument type and have it autonormalized in the decode stage as I've hinted at in register normalizeTimeArg processor only when proctree is on #4332 (comment))
  2. I believe we're handling this issue or a similar one through the argument helper here: https://github.com/aquasecurity/tracee/blob/main/signatures/helpers/arguments_helpers.go#L35. What is the difference in issue? I think further elaboration in the resolved issue would have helped clear the difference.

@geyslan
Copy link
Member Author

geyslan commented Oct 7, 2024

I believe we're handling this issue or a similar one through the argument helper here: https://github.com/aquasecurity/tracee/blob/main/signatures/helpers/arguments_helpers.go#L35.

I don't believe so, GetTraceeArgumentByName only checks for nil and errs. We don't want to err in the consumer end since the event is fine.

What is the difference in issue? I think further elaboration in the resolved issue would have helped clear the difference.

The Arg transmitted via grpc arrives lacking the Type since it's nil, then an incomplete Event Arg. When that end tries to convert it back to an event, it fails. Take a look at #4335.

This assures that when an arg field not filled (whatever reason), it sets it with the correspondent zero value.

We should use the effort of type conversion here and already move to hardcoded ArgType enums.

I believe we should indeed, in a chore PR, since it will require to change types and the whole core events fields.

Copy link
Collaborator

@NDStrahilevitz NDStrahilevitz left a comment

Choose a reason for hiding this comment

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

LGTM. Just please add the comment documentation i've requested.

pkg/events/definition_group.go Show resolved Hide resolved
types/trace/trace.go Outdated Show resolved Hide resolved
Some event arguments, such as "interpreter_*" from sched_process_exec,
were not being populated because the kernel might not always provide
them. This was causing errors during the gRPC proto conversion, as
the values couldn't be asserted - they were nil.

With this change, ArgMeta now holds the Zero value of the type, ensuring
that assignment is always possible without requiring additional parsing
in the pipeline.

This also might help parsing since the Zero field will always have
value, making it easier to assert its type instead of checking for
Name field (string).
'git log --grep old_old_uid_t' in Linux kernel code didn't return any
results.
@geyslan
Copy link
Member Author

geyslan commented Oct 8, 2024

/fast-forward

@github-actions github-actions bot merged commit 2f31471 into aquasecurity:main Oct 8, 2024
26 checks passed
@geyslan geyslan added the candidate/v0.22.3 Candidate to be cherry-picked or backported into v0.22.3 release label Oct 8, 2024
@geyslan geyslan removed the candidate/v0.22.3 Candidate to be cherry-picked or backported into v0.22.3 release label Oct 14, 2024
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.

Event sched_process_exec is sending ArgMeta with nil value in gRPC
3 participants