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.

Sign up for GitHub

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

Jump to bottom

[WIP] cleanup(sinsp)!: enforce some runtime checks and remove an extra space from evt.args #1609

Closed
wants to merge 5 commits into from
Closed

[WIP] cleanup(sinsp)!: enforce some runtime checks and remove an extra space from evt.args #1609

wants to merge 5 commits into from

Conversation

Andreagit97
Copy link
Member

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:

  • add a test for UNIX sockets in sinsp
  • add some tests for evt.arg/evt.rawarg/avt.args fields
  • enforce some runtime checks check_param_name_exists/check_param_id_exists on evt.arg this should prevent our users from using non-existent fields with evt.arg.* . I noticed it in one of our default rules Non sudo setuid, see it here Unexpected setuid call by non-sudo events contain no details #1630. we use exe_flags=%evt.arg.flags when the event is setuid but setuid doesn't have a flags arg. 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 future

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

cleanup(sinsp)!: enforce some runtime checks and remove an extra space from `evt.args`

@Andreagit97
style(modern_bpf): reword a comment
dd65717 
Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>
@Andreagit97
new(tests): add a test for unix sockets
2af68fe 
Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>
@Andreagit97
fix(sinsp): remove an extra " "
dc57b7e 
Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>
@Andreagit97
update(sinsp)!: add some validation checks on evt. filters
ea5cb97 
Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>
@Andreagit97
update(tests): rework a test on user id
ea29c47 
Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>
@poiana
Copy link
Contributor

poiana commented Jan 5, 2024

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana added the size/XL label Jan 5, 2024
@Andreagit97 Andreagit97 added this to the 0.15.0 milestone Jan 5, 2024
Andreagit97
Andreagit97 commented Jan 5, 2024
View reviewed changes
userspace/libsinsp/sinsp_filtercheck_event.cpp
@@ -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);
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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 :/

Copy link
Member Author

Choose a reason for hiding this comment

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

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

userspace/libsinsp/sinsp_filtercheck_event.cpp
@@ -1159,6 +1157,10 @@ uint8_t* sinsp_filter_check_event::extract(sinsp_evt *evt, OUT uint32_t* len, bo
}
}

if(!m_strstorage.empty())
Copy link
Member Author

Choose a reason for hiding this comment

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

this was an extra space at the end of the string

Copy link
Contributor

Choose a reason for hiding this comment

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

oh again some issues with string handling ...

@Andreagit97 Andreagit97 changed the title cleanup(sinsp)!: enforce some runtime checks and remove an extra space from evt.args [WIP] cleanup(sinsp)!: enforce some runtime checks and remove an extra space from evt.args Jan 5, 2024
@incertum
Copy link
Contributor

incertum commented Jan 5, 2024 •
edited
Loading

Oh this is a real bummer, I wasn't aware Falco would just crash in such cases, re specifically the exe_flags=%evt.arg.flags besides renaming it to flags we should adjust all upstream rules to not cause crashes and only use fields that are valid. We should still fix it here, but at least that way we fix things for Falco 0.37 and the next rules release. I can take care of that PR.

Edit @Andreagit97 you already opened the rules PR, on it!

@Andreagit97
Copy link
Member Author

at the moment Falco doesn't crash if a field evt.arg.* doesn't exist for an event, so we are fine. The 2 checks implemented here check_param_name_exists/check_param_id_exists cannot work with actual Falco because we have several rules in which we use evt.arg.* but the field is not defined for all the events in the condition (see falcosecurity/rules#214) so it would cause random crashes at runtime :/
I will detach the tests in another PR and probably close this one! until we solve the above issue (if we want to solve it) we cannot implement this strict validation unfortunately

/hold

@incertum
Copy link
Contributor

incertum commented Jan 5, 2024

Thanks for the clarification and also thanks for looking into it!

@incertum
Copy link
Contributor

incertum commented Feb 6, 2024

@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!

@Andreagit97
Copy link
Member Author

i will try to address it this week!

@Andreagit97 Andreagit97 modified the milestones: 0.15.0, 0.16.0 Mar 11, 2024
@Andreagit97 Andreagit97 mentioned this pull request Apr 15, 2024
Merged
@Andreagit97
Copy link
Member Author

close in favor of #1791

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@incertum incertum incertum left review comments

@hbrueckner hbrueckner Awaiting requested review from hbrueckner

@LucaGuerra LucaGuerra Awaiting requested review from LucaGuerra

Assignees
No one assigned
Labels
approved area/driver-modern-bpf area/libsinsp area/tests dco-signoff: yes do-not-merge/hold do-not-merge/work-in-progress kind/cleanup release-note size/XL
Projects
None yet
Milestone
0.16.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Andreagit97 @poiana @incertum