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

Rule order is not preserved between rules that have an event type and those that don't #354

Closed
mstemm opened this Issue Apr 18, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@mstemm
Contributor

mstemm commented Apr 18, 2018

It's possible to have rules that don't explicitly name an event type, or have conditions that make it difficult to map to a specific set of event types. These rules run for all event types. This compares to most rules that name an event type, which are stored in a list of rules indexed by event type.

There isn't anything that preserves rule order between the two collections, so it's possible that a rule that appears later in a rules file and runs for all event types will be considered before a rule that appears earlier in a rules file runs for a specific event type.

Here's an example:

- rule: open_below_tmp
  desc: An open event below /tmp
  condition: evt.type=open and fd.directory=/tmp
  output: some other output
  priority: INFO

- rule: related_to_tmp
  desc: Any action related to /tmp/
  condition: fd.directory=/tmp
  output: some output
  priority: INFO

In this case, related_to_tmp will run first even though it appears second in the file.

The fix is to keep track of overall rule order and preserve it when comparing rules in the two collections.

mstemm added a commit to draios/sysdig that referenced this issue Apr 18, 2018

Preserve order between catchall & other filters
Previously, catchall filters were always matched against an event first,
followed by any filters specificially linked to that event's type. This
didn't preserve the order of filters as they appeared in the file,
though.

Instead, assign a filter order for each filter and walk the catchall and
evttype-specific filters in order, trying to match each against the
event.

This fixes falcosecurity/falco#354.

mstemm added a commit that referenced this issue Apr 18, 2018

Add test for preserving rule order
Test the fix for #354. A rules
file has a event-specific rule first and a catchall rule second. Without
the changes in draios/sysdig#1103, the first
rule does not match the event.
@mattpag

This comment has been minimized.

Contributor

mattpag commented Apr 19, 2018

What's the goal of this fix? Is there a reason why falco must match the rules in the order in which they appear in the file?
Asking because if there's not and the goal is performance optimization, why can't we try to automatically reorder the rules to first match the rules that names event types and only after the "catchall" ones, instead of relying on the order in the rules file?

PS - I cannot find the test rules file catchall_order.yaml, maybe you forgot to add it to the commit?

mstemm added a commit that referenced this issue Apr 19, 2018

Add test for preserving rule order
Test the fix for #354. A rules
file has a event-specific rule first and a catchall rule second. Without
the changes in draios/sysdig#1103, the first
rule does not match the event.
@mstemm

This comment has been minimized.

Contributor

mstemm commented Apr 19, 2018

I think generally preserving rule order is useful as you can ensure that earlier rules will match an event first.

I ran into this bug when I had a rules file that first checked filesystem paths related to an open, and later checked system calls. I wanted the filesystem paths rule to take precedence over the system calls rule, so I put it earlier in the file. However, because the system calls rule wasn't specifically tied to any particular system call (its condition was something like and not evt.type in (...), it was put in the catchall category and always was evaluated first.

When I tried to open a file below a disallowed path, the system call rule triggered instead of the filesystem rule.

Thanks about catchall_order.yaml, I added it to the PR.

@mattpag

This comment has been minimized.

Contributor

mattpag commented Apr 19, 2018

Oh ok now I got it, it's a rule precedence issue.
I agree with your solution then. Thanks for the explanation!

mstemm added a commit to draios/sysdig that referenced this issue Apr 19, 2018

Preserve order between catchall & other filters (#1103)
Previously, catchall filters were always matched against an event first,
followed by any filters specificially linked to that event's type. This
didn't preserve the order of filters as they appeared in the file,
though.

Instead, assign a filter order for each filter and walk the catchall and
evttype-specific filters in order, trying to match each against the
event.

This fixes draios/falco#354.

mstemm added a commit that referenced this issue Apr 19, 2018

Add tests catchall order (#355)
* Only check whole rule names when matching counts

Tweak the regex so a rule my_great_rule doesn't pick up event counts for
a rule "great_rule: nnn".

* Add ability to skip evttype warnings for rules

A new attribute warn_evttypes, if present, suppresses printing warnings
related to a rule not matching any event type. Useful if you have a rule
where not including an event type is intentional.

* Add test for preserving rule order

Test the fix for #354. A rules
file has a event-specific rule first and a catchall rule second. Without
the changes in draios/sysdig#1103, the first
rule does not match the event.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment