From 84afd9269a5c866f3b462f552611a260f0cfe4ed Mon Sep 17 00:00:00 2001 From: Mark Stemm Date: Wed, 16 Oct 2024 13:44:56 -0700 Subject: [PATCH 1/3] Add a source to rule_update_info It's possible that someone might want to override a property for a non-syscall rule source. To assist in this, decode any source property for rules with append/override and save it in the rule_update_info object. For the source property only, the value for source can be empty e.g. 'source: ' or an empty string e.g. 'source: ""'. Both of those are considered valid but result in an empty source. A later change will ensure that the sources match up when appending/redefining/overriding/enabling. Signed-off-by: Mark Stemm --- userspace/engine/rule_loader.h | 1 + userspace/engine/rule_loader_reader.cpp | 40 +++++++++++++++++++++---- userspace/engine/rule_loader_reader.h | 5 ++++ 3 files changed, 41 insertions(+), 5 deletions(-) diff --git a/userspace/engine/rule_loader.h b/userspace/engine/rule_loader.h index 4a270e4932f..622066e9253 100644 --- a/userspace/engine/rule_loader.h +++ b/userspace/engine/rule_loader.h @@ -488,6 +488,7 @@ struct rule_update_info { context cond_ctx; std::string name; std::optional cond; + std::string source; std::optional output; std::optional desc; std::optional> tags; diff --git a/userspace/engine/rule_loader_reader.cpp b/userspace/engine/rule_loader_reader.cpp index 146eed35400..960e3340d55 100644 --- a/userspace/engine/rule_loader_reader.cpp +++ b/userspace/engine/rule_loader_reader.cpp @@ -53,7 +53,8 @@ static void decode_val_generic(const YAML::Node& item, const char* key, T& out, const rule_loader::context& ctx, - bool optional) { + bool optional, + bool can_be_empty) { const YAML::Node& val = item[key]; if(!val.IsDefined() && optional) { @@ -61,10 +62,19 @@ static void decode_val_generic(const YAML::Node& item, } THROW(!val.IsDefined(), std::string("Item has no mapping for key '") + key + "'", ctx); + if(val.IsNull() && can_be_empty) { + return; + } + THROW(val.IsNull(), std::string("Mapping for key '") + key + "' is empty", ctx); rule_loader::context valctx(val, rule_loader::context::VALUE_FOR, key, ctx); THROW(!val.IsScalar(), "Value is not a scalar value", valctx); + + if(val.Scalar().empty() && can_be_empty) { + return; + } + THROW(val.Scalar().empty(), "Value must be non-empty", valctx); THROW(!YAML::convert::decode(val, out), "Can't decode YAML scalar value", valctx); @@ -75,9 +85,10 @@ static void decode_val_generic(const YAML::Node& item, const char* key, std::optional& out, const rule_loader::context& ctx, - bool optional) { + bool optional, + bool can_be_empty) { T decoded; - decode_val_generic(item, key, decoded, ctx, optional); + decode_val_generic(item, key, decoded, ctx, optional, can_be_empty); out = decoded; } @@ -87,8 +98,9 @@ void rule_loader::reader::decode_val(const YAML::Node& item, T& out, const rule_loader::context& ctx) { bool optional = false; + bool can_be_empty = false; - decode_val_generic(item, key, out, ctx, optional); + decode_val_generic(item, key, out, ctx, optional, can_be_empty); } template void rule_loader::reader::decode_val(const YAML::Node& item, @@ -102,8 +114,20 @@ void rule_loader::reader::decode_optional_val(const YAML::Node& item, T& out, const rule_loader::context& ctx) { bool optional = true; + bool can_be_empty = false; - decode_val_generic(item, key, out, ctx, optional); + decode_val_generic(item, key, out, ctx, optional, can_be_empty); +} + +template +void rule_loader::reader::decode_optional_empty_val(const YAML::Node& item, + const char* key, + T& out, + const rule_loader::context& ctx) { + bool optional = true; + bool can_be_empty = true; + + decode_val_generic(item, key, out, ctx, optional, can_be_empty); } template void rule_loader::reader::decode_optional_val( @@ -591,6 +615,9 @@ void rule_loader::reader::read_item(rule_loader::configuration& cfg, rule_loader::context ctx(item, rule_loader::context::RULE, name, parent); + std::string source = ""; + decode_optional_empty_val(item, "source", source, ctx); + bool has_append_flag = false; decode_optional_val(item, "append", has_append_flag, ctx); if(has_append_flag) { @@ -648,6 +675,7 @@ void rule_loader::reader::read_item(rule_loader::configuration& cfg, "append", "condition", ctx)) { + v.source = source; decode_val(item, "condition", v.cond, ctx); } @@ -682,6 +710,7 @@ void rule_loader::reader::read_item(rule_loader::configuration& cfg, "replace", "condition", ctx)) { + v.source = source; decode_val(item, "condition", v.cond, ctx); } @@ -765,6 +794,7 @@ void rule_loader::reader::read_item(rule_loader::configuration& cfg, } else if(has_append_flag) { rule_loader::rule_update_info v(ctx); v.name = name; + v.source = source; if(item["condition"].IsDefined()) { v.cond_ctx = rule_loader::context(item["condition"], diff --git a/userspace/engine/rule_loader_reader.h b/userspace/engine/rule_loader_reader.h index 3edfded67f8..f99c23188a8 100644 --- a/userspace/engine/rule_loader_reader.h +++ b/userspace/engine/rule_loader_reader.h @@ -66,6 +66,11 @@ class reader { const char* key, T& out, const rule_loader::context& ctx); + template + static void decode_optional_empty_val(const YAML::Node& item, + const char* key, + T& out, + const rule_loader::context& ctx); protected: virtual void read_item(rule_loader::configuration& cfg, From f62b77f93f0505e4ee2d5e44d2066077e9556514 Mon Sep 17 00:00:00 2001 From: Mark Stemm Date: Wed, 16 Oct 2024 13:45:21 -0700 Subject: [PATCH 2/3] When overriding rules, ensure that the sources match In places where a second rule definition might replace, append to, or replace items from a base rule, ensure that the source of the second rule definiton matches the first. This already existed for defines, but for other changes. There was a bug where a second definition might exist for a different source, but the additional rule was used anyway. This now returns the same error for these other changes e.g. "Rule has been re-defined..." as define. Signed-off-by: Mark Stemm --- userspace/engine/rule_loader_collector.cpp | 23 ++++++++++++++++------ userspace/engine/rule_loader_collector.h | 3 +++ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/userspace/engine/rule_loader_collector.cpp b/userspace/engine/rule_loader_collector.cpp index 18103950db2..3d26b3e72eb 100644 --- a/userspace/engine/rule_loader_collector.cpp +++ b/userspace/engine/rule_loader_collector.cpp @@ -182,10 +182,8 @@ void rule_loader::collector::append(configuration& cfg, macro_info& info) { } void rule_loader::collector::define(configuration& cfg, rule_info& info) { - const auto* prev = m_rule_infos.at(info.name); - THROW(prev && prev->source != info.source, - "Rule has been re-defined with a different source", - info.ctx); + auto prev = find_prev_rule(info); + (void)prev; const auto* source = cfg.sources.at(info.source); if(!source) { @@ -205,7 +203,7 @@ void rule_loader::collector::define(configuration& cfg, rule_info& info) { } void rule_loader::collector::append(configuration& cfg, rule_update_info& info) { - auto prev = m_rule_infos.at(info.name); + auto prev = find_prev_rule(info); THROW(!prev, ERROR_NO_PREVIOUS_RULE_APPEND, info.ctx); THROW(!info.has_any_value(), @@ -275,7 +273,7 @@ void rule_loader::collector::append(configuration& cfg, rule_update_info& info) } void rule_loader::collector::selective_replace(configuration& cfg, rule_update_info& info) { - auto prev = m_rule_infos.at(info.name); + auto prev = find_prev_rule(info); THROW(!prev, ERROR_NO_PREVIOUS_RULE_REPLACE, info.ctx); THROW(!info.has_any_value(), @@ -330,6 +328,19 @@ void rule_loader::collector::selective_replace(configuration& cfg, rule_update_i replace_info(prev, info, m_cur_index++); } +template +rule_loader::rule_info* rule_loader::collector::find_prev_rule(ruleInfo& info) { + auto ret = m_rule_infos.at(info.name); + + // Throw an error if both the original rule and current rule + // have the same name and explicitly have different sources. + THROW(ret && (ret->source != "" && info.source != "" && ret->source != info.source), + "Rule has been re-defined with a different source", + info.ctx); + + return ret; +} + void rule_loader::collector::enable(configuration& cfg, rule_info& info) { auto prev = m_rule_infos.at(info.name); THROW(!prev, "Rule has 'enabled' key but no rule by that name already exists", info.ctx); diff --git a/userspace/engine/rule_loader_collector.h b/userspace/engine/rule_loader_collector.h index 6abf862961c..d995f6c54d0 100644 --- a/userspace/engine/rule_loader_collector.h +++ b/userspace/engine/rule_loader_collector.h @@ -97,6 +97,9 @@ class collector { virtual void selective_replace(configuration& cfg, rule_update_info& info); private: + template + rule_info* find_prev_rule(ruleInfo& info); + uint32_t m_cur_index; indexed_vector m_rule_infos; indexed_vector m_macro_infos; From 0f2ae346d70746268a91edef4784b2d8b6efebce Mon Sep 17 00:00:00 2001 From: Mark Stemm Date: Wed, 16 Oct 2024 13:45:56 -0700 Subject: [PATCH 3/3] Add tests for mismatched sources and append Add additional unit tests to verify that rule loading fails when a second rules object has a different source but the name of an existing rules object. Also add tests for additional rules having an empty source. Signed-off-by: Mark Stemm --- unit_tests/engine/test_rule_loader.cpp | 105 +++++++++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/unit_tests/engine/test_rule_loader.cpp b/unit_tests/engine/test_rule_loader.cpp index 8feadd2c592..83618c60781 100644 --- a/unit_tests/engine/test_rule_loader.cpp +++ b/unit_tests/engine/test_rule_loader.cpp @@ -1222,3 +1222,108 @@ TEST_F(test_falco_engine, exceptions_fields_transformer_space_quoted) { EXPECT_EQ(get_compiled_rule_condition("test_rule"), "(evt.type = open and not tolower(proc.name) = test)"); } + +TEST_F(test_falco_engine, redefine_rule_different_source) { + auto rules_content = R"END( +- rule: LD_PRELOAD trick + desc: Some desc + condition: ka.verb = GET + output: some output + priority: INFO + source: k8s_audit + +- rule: LD_PRELOAD trick + desc: Some desc + condition: and 1 = 2 + output: Some output + priority: INFO + source: syscall +)END"; + + ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); + ASSERT_TRUE(check_error_message("Rule has been re-defined with a different source")); +} + +TEST_F(test_falco_engine, append_across_sources) { + auto rules_content = R"END( +- rule: LD_PRELOAD trick + desc: Some desc + condition: ka.verb = GET + output: some output + priority: INFO + source: k8s_audit + +- rule: LD_PRELOAD trick + desc: Some desc + condition: and 1 = 2 + output: Some output + priority: INFO + source: syscall + append: true +)END"; + + ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); + ASSERT_TRUE(check_error_message("Rule has been re-defined with a different source")); +} + +TEST_F(test_falco_engine, selective_replace_across_sources) { + auto rules_content = R"END( +- rule: LD_PRELOAD trick + desc: Some desc + condition: ka.verb = GET + output: some output + priority: INFO + source: k8s_audit + +- rule: LD_PRELOAD trick + condition: 1 = 2 + override: + condition: replace + source: syscall +)END"; + + ASSERT_FALSE(load_rules(rules_content, "rules.yaml")); + ASSERT_TRUE(check_error_message("Rule has been re-defined with a different source")); +} + +TEST_F(test_falco_engine, empty_source_addl_rule) { + auto rules_content = R"END( +- rule: LD_PRELOAD trick + desc: Some desc + condition: evt.type=execve + output: some output + priority: INFO + source: syscall + +- rule: LD_PRELOAD trick + desc: Some desc + condition: and proc.name=apache + output: Some output + priority: INFO + source: + append: true +)END"; + + EXPECT_TRUE(load_rules(rules_content, "rules.yaml")); +} + +TEST_F(test_falco_engine, empty_string_source_addl_rule) { + auto rules_content = R"END( +- rule: LD_PRELOAD trick + desc: Some desc + condition: evt.type=execve + output: some output + priority: INFO + source: syscall + +- rule: LD_PRELOAD trick + desc: Some desc + condition: and proc.name=apache + output: Some output + priority: INFO + source: "" + append: true +)END"; + + EXPECT_TRUE(load_rules(rules_content, "rules.yaml")); +}