From 414bafb53778d81849e6e00d370df980ee0c2a0b Mon Sep 17 00:00:00 2001 From: Anil Mahtani <929854+Anilm3@users.noreply.github.com> Date: Mon, 9 Oct 2023 21:16:12 +0100 Subject: [PATCH] Fix collection cache ephemerality --- CMakeLists.txt | 1 - src/collection.cpp | 15 +++++++++++---- src/collection.hpp | 11 +++++------ src/expression.cpp | 5 ++--- src/expression.hpp | 29 +++++++++++++---------------- src/rule.cpp | 31 ------------------------------- src/rule.hpp | 15 ++++++++++++++- tests/rule_test.cpp | 7 +++++++ 8 files changed, 52 insertions(+), 62 deletions(-) delete mode 100644 src/rule.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index a7938cab8..936799ef9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -76,7 +76,6 @@ set(LIBDDWAF_SOURCE ${libddwaf_SOURCE_DIR}/src/object_store.cpp ${libddwaf_SOURCE_DIR}/src/collection.cpp ${libddwaf_SOURCE_DIR}/src/expression.cpp - ${libddwaf_SOURCE_DIR}/src/rule.cpp ${libddwaf_SOURCE_DIR}/src/ruleset_info.cpp ${libddwaf_SOURCE_DIR}/src/ip_utils.cpp ${libddwaf_SOURCE_DIR}/src/processor.cpp diff --git a/src/collection.cpp b/src/collection.cpp index 65af20ca0..d2d34e9f3 100644 --- a/src/collection.cpp +++ b/src/collection.cpp @@ -83,16 +83,23 @@ void base_collection::match(memory::vector &events, const object ddwaf::timer &deadline) const { if (cache.result >= Derived::type()) { - return; + // If the result was cached but ephemeral, clear it. Note that this is + // just a workaround taking advantage of the order of evaluation of + // collections. Collections might be removed in the future altogether. + if (cache.result == Derived::type() && cache.ephemeral) { + cache.result = collection_type::none; + cache.ephemeral = false; + } else { + return; + } } for (auto *rule : rules_) { auto event = match_rule(rule, store, cache.rule_cache, rules_to_exclude, objects_to_exclude, dynamic_matchers, deadline); if (event.has_value()) { - if (!event->ephemeral) { - cache.result = Derived::type(); - } + cache.result = Derived::type(); + cache.ephemeral = event->ephemeral; events.emplace_back(std::move(*event)); DDWAF_DEBUG("Found event on rule %s", rule->get_id().c_str()); diff --git a/src/collection.hpp b/src/collection.hpp index c907e12e7..c3945bf20 100644 --- a/src/collection.hpp +++ b/src/collection.hpp @@ -26,11 +26,11 @@ enum class collection_type : uint8_t { none = 0, regular = 1, priority = 2 }; // how they interact with the cache, e.g. a priority collection will try to match even if there has // already been a match in a regular collection. -// The collection cache is shared by both priority and regular collections, -// this ensures that regular collections for which there is an equivalent -// priority collection of the same type, aren't processed when the respective -// priority collection has already had a match. +// The collection cache is shared by both priority and regular collections, this ensures that +// regular collections for which there is an equivalent priority collection of the same type, +// aren't processed when the respective priority collection has already had a match. struct collection_cache { + bool ephemeral{false}; collection_type result{collection_type::none}; memory::unordered_map rule_cache; }; @@ -49,8 +49,7 @@ template class base_collection { void insert(const std::shared_ptr &rule) { rules_.emplace_back(rule.get()); } - void match(memory::vector &events /* output */, const object_store &store, - collection_cache &cache, + void match(memory::vector &events, const object_store &store, collection_cache &cache, const memory::unordered_map &rules_to_exclude, const memory::unordered_map &objects_to_exclude, const std::unordered_map> &dynamic_matchers, diff --git a/src/expression.cpp b/src/expression.cpp index 4d58a8fd5..711c59553 100644 --- a/src/expression.cpp +++ b/src/expression.cpp @@ -118,7 +118,6 @@ const matcher::base *expression::evaluator::get_matcher(const condition &cond) c return it->second.get(); } -// NOLINTNEXTLINE(misc-no-recursion) expression::eval_result expression::evaluator::eval_condition( const condition &cond, condition::cache_type &cache) { @@ -183,7 +182,7 @@ expression::eval_result expression::evaluator::eval() continue; } - auto [res, ephemeral] = eval_condition(*cond, cond_cache); + auto [res, ephemeral] = eval_condition(cond, cond_cache); if (!res) { return {false, false}; } @@ -224,7 +223,7 @@ void expression_builder::add_target(std::string name, std::vector k target.source = source; auto &cond = conditions_.back(); - cond->targets.emplace_back(std::move(target)); + cond.targets.emplace_back(std::move(target)); } } // namespace ddwaf diff --git a/src/expression.hpp b/src/expression.hpp index 68e03fcd0..f541041ae 100644 --- a/src/expression.hpp +++ b/src/expression.hpp @@ -77,7 +77,7 @@ class expression { ddwaf::timer &deadline; const ddwaf::object_limits &limits; - const std::vector> &conditions; + const std::vector &conditions; const object_store &store; const exclusion::object_set &objects_excluded; const std::unordered_map> &dynamic_matchers; @@ -86,8 +86,7 @@ class expression { expression() = default; - explicit expression( - std::vector> &&conditions, ddwaf::object_limits limits = {}) + explicit expression(std::vector &&conditions, ddwaf::object_limits limits = {}) : limits_(limits), conditions_(std::move(conditions)) {} @@ -99,9 +98,7 @@ class expression { void get_addresses(std::unordered_map &addresses) const { for (const auto &cond : conditions_) { - for (const auto &target : cond->targets) { - addresses.emplace(target.root, target.name); - } + for (const auto &target : cond.targets) { addresses.emplace(target.root, target.name); } } } @@ -125,7 +122,7 @@ class expression { protected: ddwaf::object_limits limits_; - std::vector> conditions_; + std::vector conditions_; }; class expression_builder { @@ -136,37 +133,37 @@ class expression_builder { conditions_.reserve(num_conditions); } - void start_condition() { conditions_.emplace_back(std::make_unique()); } + void start_condition() { conditions_.emplace_back(expression::condition{}); } template void start_condition(Args... args) { - auto cond = std::make_unique(); - cond->matcher = std::make_unique(std::forward(args)...); + expression::condition cond; + cond.matcher = std::make_unique(std::forward(args)...); conditions_.emplace_back(std::move(cond)); } void start_condition(std::string data_id) { - auto cond = std::make_unique(); - cond->data_id = std::move(data_id); + expression::condition cond; + cond.data_id = std::move(data_id); conditions_.emplace_back(std::move(cond)); } void set_data_id(std::string data_id) { auto &cond = conditions_.back(); - cond->data_id = std::move(data_id); + cond.data_id = std::move(data_id); } template void set_matcher(Args... args) { auto &cond = conditions_.back(); - cond->matcher = std::make_unique(args...); + cond.matcher = std::make_unique(args...); } void set_matcher(std::unique_ptr &&matcher) { auto &cond = conditions_.back(); - cond->matcher = std::move(matcher); + cond.matcher = std::move(matcher); } void add_target(std::string name, std::vector key_path = {}, @@ -180,7 +177,7 @@ class expression_builder { protected: ddwaf::object_limits limits_{}; - std::vector> conditions_{}; + std::vector conditions_{}; }; } // namespace ddwaf diff --git a/src/rule.cpp b/src/rule.cpp deleted file mode 100644 index ee255fcf1..000000000 --- a/src/rule.cpp +++ /dev/null @@ -1,31 +0,0 @@ -// Unless explicitly stated otherwise all files in this repository are -// dual-licensed under the Apache-2.0 License or BSD-3-Clause License. -// -// This product includes software developed at Datadog (https://www.datadoghq.com/). -// Copyright 2021 Datadog, Inc. -#include - -#include "log.hpp" -#include "rule.hpp" - -namespace ddwaf { - -std::optional rule::match(const object_store &store, cache_type &cache, - const exclusion::object_set &objects_excluded, - const std::unordered_map> &dynamic_matchers, - ddwaf::timer &deadline) const -{ - if (expression::get_result(cache)) { - // An event was already produced, so we skip the rule - return std::nullopt; - } - - auto res = expr_->eval(cache, store, objects_excluded, dynamic_matchers, deadline); - if (!res.outcome) { - return std::nullopt; - } - - return {ddwaf::event{this, expression::get_matches(cache), res.ephemeral}}; -} - -} // namespace ddwaf diff --git a/src/rule.hpp b/src/rule.hpp index 07a39d620..dccde42bc 100644 --- a/src/rule.hpp +++ b/src/rule.hpp @@ -65,7 +65,20 @@ class rule { virtual std::optional match(const object_store &store, cache_type &cache, const exclusion::object_set &objects_excluded, const std::unordered_map> &dynamic_matchers, - ddwaf::timer &deadline) const; + ddwaf::timer &deadline) const + { + if (expression::get_result(cache)) { + // An event was already produced, so we skip the rule + return std::nullopt; + } + + auto res = expr_->eval(cache, store, objects_excluded, dynamic_matchers, deadline); + if (!res.outcome) { + return std::nullopt; + } + + return {ddwaf::event{this, expression::get_matches(cache), res.ephemeral}}; + } [[nodiscard]] bool is_enabled() const { return enabled_; } void toggle(bool value) { enabled_ = value; } diff --git a/tests/rule_test.cpp b/tests/rule_test.cpp index 358fc295c..e8ea267c5 100644 --- a/tests/rule_test.cpp +++ b/tests/rule_test.cpp @@ -15,6 +15,7 @@ using namespace ddwaf; using namespace std::literals; namespace { + TEST(TestRule, Match) { expression_builder builder(1); @@ -49,6 +50,7 @@ TEST(TestRule, Match) std::vector expected_actions{"update", "block", "passlist"}; EXPECT_EQ(event->rule->get_actions(), expected_actions); EXPECT_EQ(event->matches.size(), 1); + EXPECT_FALSE(event->ephemeral); auto &match = event->matches[0]; EXPECT_STREQ(match.resolved.c_str(), "192.168.0.1"); @@ -57,6 +59,7 @@ TEST(TestRule, Match) EXPECT_STREQ(match.operator_value.data(), ""); EXPECT_STREQ(match.address.data(), "http.client_ip"); EXPECT_TRUE(match.key_path.empty()); + EXPECT_FALSE(match.ephemeral); } { @@ -67,6 +70,8 @@ TEST(TestRule, Match) EXPECT_FALSE(event.has_value()); } + EXPECT_TRUE(cache.result); + ddwaf_object_free(&root); } @@ -108,6 +113,8 @@ TEST(TestRule, EphemeralMatch) EXPECT_TRUE(event->ephemeral); } + EXPECT_FALSE(cache.result); + ddwaf_object_free(&root); }