[WIP] cleanup(sinsp)!: enforce some runtime checks and remove an extra space from evt.args
#1609
Conversation
Signed-off-by: Andrea Terzolo <[email protected]>
Signed-off-by: Andrea Terzolo <[email protected]>
Signed-off-by: Andrea Terzolo <[email protected]>
Signed-off-by: Andrea Terzolo <[email protected]>
Signed-off-by: Andrea Terzolo <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| @@ -1076,6 +1076,7 @@ uint8_t* sinsp_filter_check_event::extract(sinsp_evt *evt, OUT uint32_t* len, bo | |||
| case TYPE_CPU: | |||
| RETURN_EXTRACT_VAR(evt->m_cpuid); | |||
| case TYPE_ARGRAW: | |||
| evt->check_param_name_exists(m_argname); | |||
There was a problem hiding this comment.
these filter checks like TYPE_ARGRAW are very particular because they are related in some way to a specific event, for example evt.arg.uid is a valid filter-check for setuid events but not for execve events. So to do a proper validation during filter-check parsing time we should find a way to correlate the evt.type with these fields
There was a problem hiding this comment.
at the moment we crash at runtime which is not the ideal thing, but on the other side using wrong filter checks is not an ideal thing in the same way :/
There was a problem hiding this comment.
Moreover, we need to keep attention when we use these filter checks, for example, if the condition is evt.type in (open, openat,...) these kind of fields should be used because they could be valid for some events but not for others! For example, evt.arg.dirfd is valid for openat but not for open
| @@ -1159,6 +1157,10 @@ uint8_t* sinsp_filter_check_event::extract(sinsp_evt *evt, OUT uint32_t* len, bo | |||
| } | |||
| } | |||
|
|
|||
| if(!m_strstorage.empty()) | |||
There was a problem hiding this comment.
this was an extra space at the end of the string
There was a problem hiding this comment.
oh again some issues with string handling ...
Andreagit97
changed the title
evt.argsevt.args
|
Oh this is a real bummer, I wasn't aware Falco would just crash in such cases, re specifically the Edit @Andreagit97 you already opened the rules PR, on it! |
|
at the moment Falco doesn't crash if a field /hold |
|
Thanks for the clarification and also thanks for looking into it! |
|
@Andreagit97 could you rebase? I think we could start getting this over the finish line if you would like, I'll help as reviewer, thank you! |
|
i will try to address it this week! |
|
close in favor of #1791 |
What type of PR is this?
/kind cleanup
Any specific area of the project related to this PR?
/area driver-modern-bpf
/area libsinsp
/area tests
Does this PR require a change in the driver versions?
No
What this PR does / why we need it:
This PR does the followings:
evt.arg/evt.rawarg/avt.argsfieldscheck_param_name_exists/check_param_id_existsonevt.argthis should prevent our users from using non-existent fields withevt.arg.*. I noticed it in one of our default rulesNon sudo setuid, see it here Unexpected setuid call by non-sudo events contain no details #1630. we useexe_flags=%evt.arg.flagswhen the event issetuidbutsetuiddoesn't have aflagsarg. Please note this is a BREAKING CHANGE since now some broken rules will stop Falco at runtime... it would be amazing to stop it when parsing the filter-checks names but at the moment we don't have any event information at that point so we need to do that during extraction :/ BTW there is room for improvements in the futureWhich issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?: