Validation Error when using a macro as definition of an overwritten macro. #706
Comments
|
Me and @leodido debugged this for a while this morning. We've been able to reproduce this thanks for reporting this @natalysheinin ! The reason why this does not work as expected is because in the default We crafted a minimum reproducible example to test it: falco_rules.yaml - macro: never_true
condition: (evt.num=0)
- macro: open_write
condition: (evt.type=open or evt.type=openat) and evt.is_open_write=true and fd.typechar='f' and fd.num>=0
- macro: allowed_clear_log_files
condition: (never_true)
- macro: consider_all_chmods
condition: (never_true)
- rule: Clear Log Activities
desc: Detect clearing of critical log files
condition: >
open_write and
not allowed_clear_log_files
output: >
Log files were tampered (user=%user.name command=%proc.cmdline file=%fd.name container_id=%container.id image=%container.image.repository)
priority:
WARNING
tags: [file, mitre_defense_evasion]falco_rules.local.yaml That set of rules with that order will output the same error you are reporting: ./build/userspace/falco/falco -c falco.yaml --validate rules/falco_rules.yaml --validate rules/falco_rules.local.yaml
Fri Jul 5 12:22:39 2019: Validating rules file(s):
Fri Jul 5 12:22:39 2019: rules/falco_rules.yaml
Fri Jul 5 12:22:39 2019: rules/falco_rules.local.yaml
Fri Jul 5 12:22:39 2019: Runtime error: Error loading rules: ...ts/falcosecurity/falco/userspace/engine/lua/compiler.lua:74: attempt to index a nil value. Exiting.After that, if we swap the order of That's what we observed with this reproducible example but it's actually the same if you just swap the order in the upstream Here's the - macro: never_true
condition: (evt.num=0)
- macro: open_write
condition: (evt.type=open or evt.type=openat) and evt.is_open_write=true and fd.typechar='f' and fd.num>=0
- macro: consider_all_chmods
condition: (never_true)
- macro: allowed_clear_log_files
condition: (never_true)
- rule: Clear Log Activities
desc: Detect clearing of critical log files
condition: >
open_write and
not allowed_clear_log_files
output: >
Log files were tampered (user=%user.name command=%proc.cmdline file=%fd.name container_id=%container.id image=%container.image.repository)
priority:
WARNING
tags: [file, mitre_defense_evasion]
Here's the result: ./build/userspace/falco/falco -c falco.yaml --validate rules/falco_rules.yaml --validate rules/falco_rules.local.yaml
Fri Jul 5 12:25:22 2019: Validating rules file(s):
Fri Jul 5 12:25:22 2019: rules/falco_rules.yaml
Fri Jul 5 12:25:22 2019: rules/falco_rules.local.yaml
Fri Jul 5 12:25:22 2019: Ok |
|
/area rules |
|
While swapping the macro definitions in the rules file works, it cannot be considered a fix for this bug, for 2 reasons:
So we think that, in order to truly fix this bug, the way the |
|
This is great! I'm glad you were able to easily replicate this scenario. So now I have another (similar) scenario. Take for instance the exact same default rule file as you mentioned: falco_rules.yaml However, now falco_rules.local.yaml When I run my validation, once again, a configuration error (similar to above): |
|
Yes @natalysheinin, unfortunately this is due to the fact that And this is very correlated to the original problem you reported. Also this is happening because all the processing (particularly the creation of the macros tree) that |
|
@leodido Thanks for clarifying that, it is extremely useful to know when the macro tree processing occurs. Do you have any recommendations for how to approach solving this from the user-end for now?
|
The workaround I'd use right now would be putting everything in one file (making sure that Clearly the edits to the creation and the processing of the macros tree needs to fix also this case. |
|
For the sake of completeness, this is the generated AST for the last rules that @natalysheinin sent in this comment click me to see the ast dumpThe |
|
Here's some background about how the objects are maintained and how it can cause this bug. When the objects are read they're maintained in an ordered array. The ordered array is necessary to assure that appends are applied in the right order. I think the problem is that the macro allowed_clear_log_files replaces the instance of allowed_clear_log_files in that array (in this case, third). Since it's third, the macro reference I think the fix is to be more liberal about using macros defined later in the file when expanding macro references. We'll have to make sure that semantics about ordering still apply wrt appends and overrides. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
We still want to fix this. |
|
@kris-nova is going to start looking at this. It would be good to discuss on the upcoming repo planning call to get an idea of how long it might take to fix. |
|
/priority high |
|
/reopen
L.
…On Thu, Jan 28, 2021 at 9:57 AM poiana ***@***.***> wrote:
@poiana <https://github.com/poiana>: Closing this issue.
In response to this
<#706 (comment)>
:
Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.
Provide feedback via https://github.com/falcosecurity/community.
/close
Instructions for interacting with me using PR comments are available here
<https://git.k8s.io/community/contributors/guide/pull-requests.md>. If
you have questions or suggestions related to my behavior, please file an
issue against the kubernetes/test-infra
<https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:>
repository.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#706 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA5J43FLDQYD6KRWWX5PC3S4ERHRANCNFSM4H6AIRSA>
.
|
|
@leodido: Reopened 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. |
|
Rotten issues close after 30d of inactivity. Reopen the issue with Mark the issue as fresh with Provide feedback via https://github.com/falcosecurity/community. |
|
@poiana: 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. |
|
/reopen /cc @leodido |
|
@leogr: Reopened 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. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh with Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with Provide feedback via https://github.com/falcosecurity/community. /lifecycle stale |
|
/remove-lifecycle stale |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh with Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with Provide feedback via https://github.com/falcosecurity/community. /lifecycle stale |
|
/remove-lifecycle stale |
|
Here are some hacks that appear to work. I would suggest refactoring this (and the rest of the --- a/userspace/engine/lua/rule_loader.lua
+++ b/userspace/engine/lua/rule_loader.lua
@@ -917,17 +917,35 @@ function load_rules(rules_content,
state.lists[v['list']] = {["items"] = items, ["used"] = false}
end
- for _, name in ipairs(state.ordered_macro_names) do
-
- local v = state.macros_by_name[name]
-
- local status, ast = compiler.compile_macro(v['condition'], state.macros, state.lists)
-
- if status == false then
- return false, nil, nil, build_error_with_context(v['context'], ast), warnings
+ -- Ugly hackery so we push macros that do not compile to the back for
+ -- later reprocessing. Sometimes macros aren't expanded yet, so we'll
+ -- retry them later. Stop/error once we've failed for an entire
+ -- iteration.
+ local v, ast
+ local idx, changed
+ local macros_to_try = {}
+ local macros_to_retry = {}
+ for idx = 1, #state.ordered_macro_names do
+ macros_to_try[idx] = state.ordered_macro_names[idx]
+ end
+ repeat
+ changed = false
+ for idx = 1, #macros_to_try do
+ local name = macros_to_try[idx]
+ v = state.macros_by_name[name]
+ status, ast = compiler.compile_macro(v['condition'], state.macros, state.lists)
+ if status == false then
+ macros_to_retry[#macros_to_retry + 1] = name
+ else
+ state.macros[v['macro']] = {["ast"] = ast.filter.value, ["used"] = false}
+ changed = true
+ end
end
-
- state.macros[v['macro']] = {["ast"] = ast.filter.value, ["used"] = false}
+ macros_to_try = macros_to_retry
+ macros_to_retry = {}
+ until #macros_to_try == 0 or changed == false
+ if #macros_to_try ~= 0 then
+ return false, nil, nil, build_error_with_context(v['context'], ast), warnings
end
for _, name in ipairs(state.ordered_rule_names) doReplaces: for _, name in ipairs(state.ordered_macro_names) do
local v = state.macros_by_name[name]
local status, ast = compiler.compile_macro(v['condition'], state.macros, state.lists)
if status == false then
return false, nil, nil, build_error_with_context(v['context'], ast), warnings
end
state.macros[v['macro']] = {["ast"] = ast.filter.value, ["used"] = false}
endwith: -- Ugly hackery so we push macros that do not compile to the back for
-- later reprocessing. Sometimes macros aren't expanded yet, so we'll
-- retry them later. Stop/error once we've failed for an entire
-- iteration.
local v, ast
local idx, changed
local macros_to_try = {}
local macros_to_retry = {}
for idx = 1, #state.ordered_macro_names do
macros_to_try[idx] = state.ordered_macro_names[idx]
end
repeat
changed = false
for idx = 1, #macros_to_try do
local name = macros_to_try[idx]
v = state.macros_by_name[name]
status, ast = compiler.compile_macro(v['condition'], state.macros, state.lists)
if status == false then
macros_to_retry[#macros_to_retry + 1] = name
else
state.macros[v['macro']] = {["ast"] = ast.filter.value, ["used"] = false}
changed = true
end
end
macros_to_try = macros_to_retry
macros_to_retry = {}
until #macros_to_try == 0 or changed == false
if #macros_to_try ~= 0 then
return false, nil, nil, build_error_with_context(v['context'], ast), warnings
end |
|
Hey @wdoekes |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh with Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with Provide feedback via https://github.com/falcosecurity/community. /lifecycle stale |
|
/remove-lifecycle stale |
What happened / Replication Steps:
Given a macro that is defined in the default ruleset and for the purposes of being concrete let's use
Clear Log Activities:falco_rules.local.yaml, for example:When I run my validation script:
Everything succeeds 🎉
falco_rules.local.yamlusing another macro, for example:I get the following error message:
Runtime error: Error loading rules: /usr/share/falco/lua/compiler.lua:65: Undefined macro 'consider_all_chmods' used in filter.. Exiting.(It is important to note that
consider_all_chmodsis in defined in the default ruleset, so I am not sure why it is failing here.Anything else we need to know?:
Environment:
falco --version): falco 0.14.0cat /etc/os-release):uname -a):The text was updated successfully, but these errors were encountered: