-
Notifications
You must be signed in to change notification settings - Fork 902
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
[BUG] Solving overlapping syscall names issues #2443
Comments
Intention is to re-iterate aspects Jason already mentioned as well as provide a different view / summary of falcosecurity/libs#911 in order to derive a more concrete / unambiguous plan. 2023-03-15: Edited the setuid, setuid32 Row based on community call conclusion.
Re syscall extraction from each rule (this means evt.type corresponds to a system call event) ... do we continue to use |
Thanks @incertum!
I think removing PPME_GENERIC_ would be too much of a structural change for this effort, plus in my opinion having it it's the right approach to support all the syscalls that don't have a specific filler in our drivers.
My proposal is for to rename the current |
Glad to hear we keep PPME_GENERIC_ thank you!
Works! Re implementation details: The new |
That's right, we'll still rely on the syscall names when dealing with generic events. |
I'm late to the party...
Agree with @jasondellaluce here, removing
Yep the end logic should be exactly the same, I would change just the name since it is quite misleading.
Yeah I like the idea of parity if we need to add just one specular API, it's ok, but I would avoid adding tons of new APIs just to have specular methods, I would implement new methods only when we have a real need |
Community call summary 2023-03-15: No objection, we proceed with this proposal. In addition, we clarified that these changes accomplish a smoother and clearer end user experience. Tracking libs event types <-> syscall refactors:
!!! We decided to keep the overlap for cases like setuid, setuid32 (for now) as it is merely an architecture distinction, see also @Andreagit97 comment in falcosecurity/libs#911 (comment) Additional Action Items:
|
/milestone 0.35.0 |
…names deprecated A major refactor of Falco now exposes each syscall Falco's libs supports to the end user :) At the same time we deprecate overlapping syscall names, see falcosecurity/falco#2443 (comment) Official support starts with Falco 0.35.0 Co-authored-by: Jason Dellaluce <jasondellaluce@gmail.com> Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it> Co-authored-by: Federico Di Pierro <nierro92@gmail.com> Signed-off-by: incertum <melissa.kilby.oss@gmail.com>
…names deprecated A major refactor of Falco now exposes each syscall Falco's libs supports to the end user :) At the same time we deprecate overlapping syscall names, see falcosecurity/falco#2443 (comment) Official support starts with Falco 0.35.0 Co-authored-by: Jason Dellaluce <jasondellaluce@gmail.com> Co-authored-by: Andrea Terzolo <andrea.terzolo@polito.it> Co-authored-by: Federico Di Pierro <nierro92@gmail.com> Signed-off-by: incertum <melissa.kilby.oss@gmail.com>
All items completed, @jasondellaluce feel free to close this issue. |
/close 🥳 |
@jasondellaluce: Closing this issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This acts both as a bug report and as a solution proposal. Solving this problem inevitably involves some breaking changes in the Falco UX, so the goal here is to find an agreement on the direction we want to go for.
Problem
In Falco rules, events can be matches by type through the
evt.type
filter field, both in rules' conditions and outputs (e.g.evt.type=open
,evt.type in (openat, close)
, etc). The string values extracted through event type withevt.type
come right from libsinsp, libscap and the drivers, and as for today have two distinct origins:PPME_GENERIC_E
andPPM_GENERIC_X
(never supported by Falco as of the latest stable version, but to-be-supported in Falco 0.35+, and available to other libs consumers), the name information associated with thePPM_SC
enumeration, which is our platform/architecture-agnostic representation of syscalls and linux-kernel eventsIn the libraries, we currently have an design flaw where some syscalls (each with their own distinct platform-agnostic PPM_SC representation) are mapped to generate the same event at the kernel-level in the drivers. The end result is that a given
evt.type
comparison would unexpectedly match more than one syscall. For example,accept4
andaccept
are mapped to both produce the same event, and so the filterevt.type=accept
matches bothaccept4
andaccept
syscalls at runtime. This can be ambiguous and misleading.This issue is tracked in detail in falcosecurity/libs#911.
Secondary problems
Since starting the development for the syscall selection feature (#2371, #2373), Falco started relying on the
PPM_SC
constants to configure the kernel drivers about which syscalls need to be collected at runtime. Considering the problem above, we have some secondary issues:event_set_to_names
andnames_to_sc_set
) libs#915. The workaround so far consists in using "event names" (related to the event table) instead of "syscalls names" (related to actual syscalls), which works for now but is dirty, ambiguous, and suboptimal for our libsinsp API stability goals.evt.type=accept
would cause the selection of more than one event, thus losing control and granularity. This also happens when selecting rules through the newly-introducedbase_syscalls
set ([New Feature]: User input optionbase_syscalls
overriding default sinsp state enforcement #2373).Proposed solution
The discussions involving me, @Andreagit97, @FedeDP, and @incertum converged in two proposals:
event_names_to_sc_set(...)
) (will be used by Falco)sc_names_to_sc_set(...)
)Breaking changes
Solving this issue will involve a breaking change in the Falco rules. Considering the example of a rule with a condition containing
evt.type = accept
, we would now need to change it intoevt.type in (accept, accept4)
, because after the fix theaccept4
syscall would have its own event. Not doing so would causeaccept4
syscall events to be ignored by the rules. This applies for all the events listed in falcosecurity/libs#911, however from my personal researchaccept
seems to be the only used one in rulesets (including our default one).In all cases, the migration plan consists only of changes such as
evt.type = accept
->evt.type in (accept, accept4)
.Action items
cc @falcosecurity/falco-maintainers @falcosecurity/libs-maintainers
I'm opening this issue for everyone's visibility and because we need to agree on:
Looking forward to having feedback! 🤗 If there will be no contrary opinions on those points, we'll proceed with the proposed solution.
The text was updated successfully, but these errors were encountered: