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.

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

Allow append of new exceptions to rules #1768

Closed
wants to merge 10 commits into from
47 changes: 43 additions & 4 deletions test/falco_tests_exceptions.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -177,10 +177,18 @@ trace_files: !mux
trace_file: trace_files/cat_write.scap

rule_exception_append_item_not_in_rule:
exit_status: 0
stderr_contains: |+
1 warnings:
Rule My Rule with append=true: no set of fields matching name ex2
exit_status: 1
stdout_is: |+
1 errors:
Rule exception new item ex2: must have fields property with a list of fields
---
- rule: My Rule
exceptions:
- name: ex2
values:
- [apache, /tmp]
append: true
---
validate_rules_file:
- rules/exceptions/append_item_not_in_rule.yaml
trace_file: trace_files/cat_write.scap
Expand Down Expand Up @@ -311,4 +319,35 @@ trace_files: !mux
- rules/exceptions/rule_exception_single_field_append.yaml
trace_file: trace_files/cat_write.scap

rule_exception_new_single_field_append:
detect: False
detect_level: WARNING
rules_file:
- rules/exceptions/rule_exception_new_single_field_append.yaml
trace_file: trace_files/cat_write.scap

rule_exception_new_second_field_append:
detect: False
detect_level: WARNING
rules_file:
- rules/exceptions/rule_exception_new_second_field_append.yaml
trace_file: trace_files/cat_write.scap

rule_exception_new_append_no_field:
exit_status: 1
stdout_is: |+
1 errors:
Rule exception new item proc_cmdline: must have fields property with a list of fields
---
- rule: Open From Cat
exceptions:
- name: proc_cmdline
comps: in
values:
- "cat /dev/null"
append: true
---
validate_rules_file:
- rules/exceptions/rule_exception_new_no_field_append.yaml
trace_file: trace_files/cat_write.scap

31 changes: 31 additions & 0 deletions test/rules/exceptions/rule_exception_new_no_field_append.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#
# Copyright (C) 2021 The Falco Authors.
#
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

- rule: Open From Cat
desc: A process named cat does an open
condition: evt.type=open and proc.name=cat
output: "An open was seen (command=%proc.cmdline)"
priority: WARNING

- rule: Open From Cat
exceptions:
- name: proc_cmdline
comps: in
values:
- "cat /dev/null"
append: true

39 changes: 39 additions & 0 deletions test/rules/exceptions/rule_exception_new_second_field_append.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
#
# Copyright (C) 2021 The Falco Authors.
#
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

- rule: Open From Cat
desc: A process named cat does an open
condition: evt.type=open and proc.name=cat
output: "An open was seen (command=%proc.cmdline)"
exceptions:
- name: proc_cmdline
fields: proc.cmdline
comps: in
values:
- cat /dev/zero
priority: WARNING

- rule: Open From Cat
exceptions:
- name: proc_cmdline_2
fields: proc.cmdline
comps: in
values:
- "cat /dev/null"
append: true


33 changes: 33 additions & 0 deletions test/rules/exceptions/rule_exception_new_single_field_append.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#
# Copyright (C) 2021 The Falco Authors.
#
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

- rule: Open From Cat
desc: A process named cat does an open
condition: evt.type=open and proc.name=cat
output: "An open was seen (command=%proc.cmdline)"
priority: WARNING

- rule: Open From Cat
exceptions:
- name: proc_cmdline
fields: proc.cmdline
comps: in
values:
- "cat /dev/null"
append: true


87 changes: 62 additions & 25 deletions userspace/engine/lua/rule_loader.lua
Original file line number Diff line number Diff line change
Expand Up @@ -592,41 +592,78 @@ function load_rules_doc(rules_mgr, doc, load_state)
if next(v['exceptions']) ~= nil then

for _, eitem in ipairs(v['exceptions']) do
local name = eitem['name']
local fields = eitem['fields']
local comps = eitem['comps']

if name == nil then
if eitem['name'] == nil then
return false, build_error_with_context(v['context'], "Rule exception item must have name property"), warnings
end

-- You can't append exception fields or comps to a rule
if fields ~= nil then
return false, build_error_with_context(v['context'], "Can not append exception fields to existing rule, only values"), warnings
end
-- Seperate case when a exception name is not found
-- This means that a new exception is being appended

if comps ~= nil then
return false, build_error_with_context(v['context'], "Can not append exception comps to existing rule, only values"), warnings
local new_exception = true
for _, rex_item in ipairs(state.rules_by_name[v['rule']]['exceptions']) do
if rex_item['name'] == eitem['name'] then
new_exception = false
break
end
end

-- You can append values. They are added to the
-- corresponding name, if it exists. If no
-- exception with that name exists, add a
-- warning.
if eitem['values'] ~= nil then
local found=false
for _, reitem in ipairs(state.rules_by_name[v['rule']]['exceptions']) do
if reitem['name'] == eitem['name'] then
found=true
for _, values in ipairs(eitem['values']) do
reitem['values'][#reitem['values'] + 1] = values
if new_exception then
local exceptions = state.rules_by_name[v['rule']]['exceptions']
if exceptions == nil then
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there's any need for this check given that the rule object always has exceptions--it's added on line 538. If the check on line 538 were not there, this would not actually work, as the exceptions variable would end up pointing to a local table and not the table that's a part of the rule object.

exceptions = {}
end

if eitem['fields'] == nil then
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to use the fields/comps variables set at the top of this block, right?

return false, build_error_with_context(v['context'], "Rule exception new item "..eitem['name']..": must have fields property with a list of fields"), warnings
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try to use the local variables defined on lines 595 instead of accessing eitem['name'], eitem['fields'], etc? The prior version wasn't entirely consistent but did try to use those local variables, to make the code a bit easier to read.

If you prefer accessing the name, fields, etc. via eitem['xxx'] instead, that's also ok, but let's be consistent and remove the definitions on line 595-597.

end
if eitem['values'] == nil then
return false, build_error_with_context(v['context'], "Rule exception new item "..eitem['name']..": must have values property with a list of values"), warnings
end

local valid, err
if type(eitem['fields']) == "table" then
valid, err = validate_exception_item_multi_fields(rules_mgr, v['source'], eitem, v['context'])
else
valid, err = validate_exception_item_single_field(rules_mgr, v['source'], eitem, v['context'])
end

if valid == false then
return valid, err
end

-- Insert the complete exception object
exceptions[#exceptions+1] = eitem
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you need to add the exceptions back to the original rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The line 639 is supposed to add the entire exception to the original rule. I have tested this specific insertion to the original table on a lua playground. Will update it if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right--tables are references, so the local exceptions variable still points to the same table which is a part of the rule. See my earlier comment about assigning the exceptions variable, though.

else
-- Appends to existing exception here
-- You can't append exception fields or comps to an existing rule exception
if eitem['fields'] ~= nil then
return false, build_error_with_context(v['context'], "Can not append exception fields to existing rule, only values"), warnings
Copy link
Contributor

Choose a reason for hiding this comment

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

The indentation looks okay in my editor, but doesn't seem okay here. I wouldn't go crazy trying to fix it, but if you can align the indent with whatever is being used by the other parts of this file it would help with readability.

end

if eitem['comps'] ~= nil then
return false, build_error_with_context(v['context'], "Can not append exception comps to existing rule, only values"), warnings
end

-- You can append values. They are added to the
-- corresponding name, if it exists. If no
-- exception with that name exists, add a
-- warning.
if eitem['values'] ~= nil then
local found=false
for _, reitem in ipairs(state.rules_by_name[v['rule']]['exceptions']) do
if reitem['name'] == eitem['name'] then
found=true
for _, values in ipairs(eitem['values']) do
reitem['values'][#reitem['values'] + 1] = values
end
end
end
end

if found == false then
warnings[#warnings + 1] = "Rule "..v['rule'].." with append=true: no set of fields matching name "..eitem['name']
end
if found == false then
warnings[#warnings + 1] = "Rule "..v['rule'].." with append=true: no set of fields matching name "..eitem['name']
end
end
end
end
end
Expand Down