From 05cd538a22dccac30556a8564a078812ecaeaf8f Mon Sep 17 00:00:00 2001 From: Anil Mahtani <929854+Anilm3@users.noreply.github.com> Date: Mon, 4 Dec 2023 17:31:03 +0000 Subject: [PATCH 1/4] Always serialize schemas when available --- appsec/src/helper/client.cpp | 10 ---------- appsec/src/helper/client.hpp | 1 - appsec/src/helper/engine.cpp | 5 +---- appsec/src/helper/engine.hpp | 1 - appsec/src/helper/network/proto.hpp | 3 +-- appsec/src/helper/subscriber/base.hpp | 1 - appsec/src/helper/subscriber/waf.cpp | 27 ++++++++++++++++++++------- appsec/src/helper/subscriber/waf.hpp | 2 ++ appsec/tests/helper/broker_test.cpp | 3 +-- appsec/tests/helper/client_test.cpp | 2 +- appsec/tests/helper/waf_test.cpp | 5 +++-- 11 files changed, 29 insertions(+), 31 deletions(-) diff --git a/appsec/src/helper/client.cpp b/appsec/src/helper/client.cpp index aa15d8584a..d7f859e790 100644 --- a/appsec/src/helper/client.cpp +++ b/appsec/src/helper/client.cpp @@ -471,16 +471,6 @@ bool client::handle_command(network::request_shutdown::request &command) response->triggers = std::move(res->events); response->force_keep = res->force_keep; - for (const auto &[key, value] : res->schemas) { - std::string schema = value; - if (value.length() > max_plain_schema_allowed) { - auto encoded = compress(schema); - if (encoded) { - schema = base64_encode(encoded.value(), false); - } - } - response->meta.emplace(key, std::move(schema)); - } DD_STDLOG(DD_STDLOG_ATTACK_DETECTED); } else { diff --git a/appsec/src/helper/client.hpp b/appsec/src/helper/client.hpp index 9b72283fb0..2d5a8d040f 100644 --- a/appsec/src/helper/client.hpp +++ b/appsec/src/helper/client.hpp @@ -21,7 +21,6 @@ namespace dds { class client { public: // Below this limit the encoding+compression might result on a longer string - static constexpr int max_plain_schema_allowed = 260; client(std::shared_ptr service_manager, network::base_broker::ptr &&broker) : service_manager_(std::move(service_manager)), diff --git a/appsec/src/helper/engine.cpp b/appsec/src/helper/engine.cpp index 8501a54f84..ff6cc16e27 100644 --- a/appsec/src/helper/engine.cpp +++ b/appsec/src/helper/engine.cpp @@ -75,7 +75,6 @@ std::optional engine::context::publish(parameter &¶m) std::vector event_data; std::unordered_set event_actions; - std::map schemas; for (auto &sub : common_->subscribers) { auto it = listeners_.find(sub); @@ -89,7 +88,6 @@ std::optional engine::context::publish(parameter &¶m) std::make_move_iterator(event->data.begin()), std::make_move_iterator(event->data.end())); event_actions.merge(event->actions); - schemas.merge(event->schemas); } } catch (std::exception &e) { SPDLOG_ERROR("subscriber failed: {}", e.what()); @@ -100,8 +98,7 @@ std::optional engine::context::publish(parameter &¶m) return std::nullopt; } - dds::engine::result res{ - action_type::record, {}, std::move(event_data), std::move(schemas)}; + dds::engine::result res{action_type::record, {}, std::move(event_data)}; // Currently the only action the extension can perform is block if (!event_actions.empty()) { // The extension can only handle one action, so we pick the first one diff --git a/appsec/src/helper/engine.hpp b/appsec/src/helper/engine.hpp index 29964c0247..fc50b42881 100644 --- a/appsec/src/helper/engine.hpp +++ b/appsec/src/helper/engine.hpp @@ -50,7 +50,6 @@ class engine { action_type type; std::unordered_map parameters; std::vector events; - std::map schemas; bool force_keep; }; diff --git a/appsec/src/helper/network/proto.hpp b/appsec/src/helper/network/proto.hpp index dc38be1666..50d89424e6 100644 --- a/appsec/src/helper/network/proto.hpp +++ b/appsec/src/helper/network/proto.hpp @@ -280,10 +280,9 @@ struct request_shutdown { std::map meta; std::map metrics; - std::map schemas; MSGPACK_DEFINE( - verdict, parameters, triggers, force_keep, meta, metrics, schemas); + verdict, parameters, triggers, force_keep, meta, metrics); }; }; diff --git a/appsec/src/helper/subscriber/base.hpp b/appsec/src/helper/subscriber/base.hpp index 07e5ea76fa..2c4d93a31f 100644 --- a/appsec/src/helper/subscriber/base.hpp +++ b/appsec/src/helper/subscriber/base.hpp @@ -21,7 +21,6 @@ class subscriber { struct event { std::vector data; std::unordered_set actions; - std::map schemas; }; class listener { diff --git a/appsec/src/helper/subscriber/waf.cpp b/appsec/src/helper/subscriber/waf.cpp index e3157d3f11..8219ade445 100644 --- a/appsec/src/helper/subscriber/waf.cpp +++ b/appsec/src/helper/subscriber/waf.cpp @@ -3,8 +3,6 @@ // // This product includes software developed at Datadog // (https://www.datadoghq.com/). Copyright 2021 Datadog, Inc. -#include "../std_logging.hpp" -#include "ddwaf.h" #include #include #include @@ -18,7 +16,11 @@ #include #include "../json_helper.hpp" +#include "../std_logging.hpp" #include "../tags.hpp" +#include "base64.h" +#include "compression.hpp" +#include "ddwaf.h" #include "waf.hpp" namespace dds::waf { @@ -39,11 +41,6 @@ dds::subscriber::event format_waf_result(ddwaf_result &res) output.data.emplace_back(std::move(parameter_to_json(event))); } - const parameter_view schemas{res.derivatives}; - for (const auto &schema : schemas) { - output.schemas.emplace( - schema.key(), std::move(parameter_to_json(schema))); - } } catch (const std::exception &e) { SPDLOG_ERROR("failed to parse WAF output: {}", e.what()); } @@ -209,6 +206,11 @@ std::optional instance::listener::call( // NOLINTNEXTLINE total_runtime_ += res.total_runtime / 1000.0; + const parameter_view schemas{res.derivatives}; + for (const auto &schema : schemas) { + schemas_.emplace(schema.key(), std::move(parameter_to_json(schema))); + } + switch (code) { case DDWAF_MATCH: return format_waf_result(res); @@ -236,6 +238,17 @@ void instance::listener::get_meta_and_metrics( { meta[std::string(tag::event_rules_version)] = ruleset_version_; metrics[tag::waf_duration] = total_runtime_; + + for (const auto &[key, value] : schemas_) { + std::string schema = value; + if (value.length() > max_plain_schema_allowed) { + auto encoded = compress(schema); + if (encoded) { + schema = base64_encode(encoded.value(), false); + } + } + meta.emplace(key, std::move(schema)); + } } instance::instance(parameter &rule, std::map &meta, diff --git a/appsec/src/helper/subscriber/waf.hpp b/appsec/src/helper/subscriber/waf.hpp index b7d26f7ad5..508b20a11c 100644 --- a/appsec/src/helper/subscriber/waf.hpp +++ b/appsec/src/helper/subscriber/waf.hpp @@ -23,6 +23,7 @@ void initialise_logging(spdlog::level::level_enum level); class instance : public dds::subscriber { public: static constexpr int default_waf_timeout_us = 10000; + static constexpr int max_plain_schema_allowed = 260; using ptr = std::shared_ptr; class listener : public dds::subscriber::listener { @@ -46,6 +47,7 @@ class instance : public dds::subscriber { std::chrono::microseconds waf_timeout_; double total_runtime_{0.0}; std::string_view ruleset_version_; + std::map schemas_; }; // NOLINTNEXTLINE(google-runtime-references) diff --git a/appsec/tests/helper/broker_test.cpp b/appsec/tests/helper/broker_test.cpp index 13255a3418..b4e7bef2a6 100644 --- a/appsec/tests/helper/broker_test.cpp +++ b/appsec/tests/helper/broker_test.cpp @@ -166,7 +166,7 @@ TEST(BrokerTest, SendRequestShutdown) packer.pack_array(1); // Array of messages packer.pack_array(2); // First message pack_str(packer, "request_shutdown"); // Type - packer.pack_array(7); + packer.pack_array(6); pack_str(packer, "block"); packer.pack_map(2); pack_str(packer, "type"); @@ -179,7 +179,6 @@ TEST(BrokerTest, SendRequestShutdown) packer.pack_true(); // Force keep packer.pack_map(0); packer.pack_map(0); - packer.pack_map(0); const auto &expected_data = ss.str(); network::header_t h; diff --git a/appsec/tests/helper/client_test.cpp b/appsec/tests/helper/client_test.cpp index 4ed652f9ba..c672724247 100644 --- a/appsec/tests/helper/client_test.cpp +++ b/appsec/tests/helper/client_test.cpp @@ -2365,7 +2365,7 @@ TEST(ClientTest, SchemasOverTheLimitAreCompressed) int body_length = 0; int i = 0; // Lets generate a body which goes over max_plain_schema_allowed limit - while (body_length < client::max_plain_schema_allowed) { + while (body_length < waf::instance::max_plain_schema_allowed) { body.add(std::to_string(i), parameter::int64(i)); auto schema = parameter::array(); schema.add(parameter::int64(4)); diff --git a/appsec/tests/helper/waf_test.cpp b/appsec/tests/helper/waf_test.cpp index 63ece3cb2c..88debfccf8 100644 --- a/appsec/tests/helper/waf_test.cpp +++ b/appsec/tests/helper/waf_test.cpp @@ -323,7 +323,8 @@ TEST(WafTest, SchemasAreAdded) EXPECT_FALSE(doc.HasParseError()); EXPECT_TRUE(doc.IsObject()); - EXPECT_FALSE(res->schemas.empty()); - EXPECT_STREQ(res->schemas["_dd.appsec.s.arg2"].c_str(), "[8]"); + ctx->get_meta_and_metrics(meta, metrics); + EXPECT_FALSE(meta.empty()); + EXPECT_STREQ(meta["_dd.appsec.s.arg2"].c_str(), "[8]"); } } // namespace dds From 2f7d88a961c2ae03e112e3971f71d81b6d1e19da Mon Sep 17 00:00:00 2001 From: Anil Mahtani <929854+Anilm3@users.noreply.github.com> Date: Tue, 5 Dec 2023 09:27:40 +0000 Subject: [PATCH 2/4] Remove experimental API security config variable --- appsec/src/extension/commands/client_init.c | 17 ++++++++++++++--- appsec/src/extension/configuration.h | 1 - .../extension/api_security_env_variables.phpt | 3 --- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/appsec/src/extension/commands/client_init.c b/appsec/src/extension/commands/client_init.c index 35d856a6ba..8945ba0cfc 100644 --- a/appsec/src/extension/commands/client_init.c +++ b/appsec/src/extension/commands/client_init.c @@ -164,10 +164,21 @@ static dd_result _pack_command( mpack_start_map(w, 2); dd_mpack_write_lstr(w, "enabled"); - mpack_write_bool(w, get_global_DD_EXPERIMENTAL_API_SECURITY_ENABLED()); - dd_mpack_write_lstr(w, "sample_rate"); - mpack_write(w, get_global_DD_API_SECURITY_REQUEST_SAMPLE_RATE()); +#define MIN_SE_SAMPLE_RATE 0.0001 + + double se_sample_rate = get_global_DD_API_SECURITY_REQUEST_SAMPLE_RATE(); + if (se_sample_rate >= MIN_SE_SAMPLE_RATE) { + mpack_write_bool(w, true); + + dd_mpack_write_lstr(w, "sample_rate"); + mpack_write(w, se_sample_rate); + } else { + mpack_write_bool(w, false); + + dd_mpack_write_lstr(w, "sample_rate"); + mpack_write(w, 0.0); + } mpack_finish_map(w); diff --git a/appsec/src/extension/configuration.h b/appsec/src/extension/configuration.h index 11a165f783..d349fdbb92 100644 --- a/appsec/src/extension/configuration.h +++ b/appsec/src/extension/configuration.h @@ -62,7 +62,6 @@ extern bool runtime_config_first_init; CONFIG(CUSTOM(STRING), DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING, "safe", .parser = dd_parse_automated_user_events_tracking) \ CONFIG(STRING, DD_APPSEC_HTTP_BLOCKED_TEMPLATE_HTML, "") \ CONFIG(STRING, DD_APPSEC_HTTP_BLOCKED_TEMPLATE_JSON, "") \ - CONFIG(BOOL, DD_EXPERIMENTAL_API_SECURITY_ENABLED, "false") \ CONFIG(DOUBLE, DD_API_SECURITY_REQUEST_SAMPLE_RATE, "0.1") // clang-format on diff --git a/appsec/tests/extension/api_security_env_variables.phpt b/appsec/tests/extension/api_security_env_variables.phpt index 8cfffe0b70..f387cb6be5 100644 --- a/appsec/tests/extension/api_security_env_variables.phpt +++ b/appsec/tests/extension/api_security_env_variables.phpt @@ -1,13 +1,10 @@ --TEST-- Set and test API security ini settings --ENV-- -DD_EXPERIMENTAL_API_SECURITY_ENABLED=1 DD_API_SECURITY_REQUEST_SAMPLE_RATE=0.8 --FILE-- --EXPECTF-- -string(1) "1" string(3) "0.8" From dcffec7f29c9cd6c7883c64ebd9c4793c3a7502a Mon Sep 17 00:00:00 2001 From: Anil Mahtani <929854+Anilm3@users.noreply.github.com> Date: Tue, 5 Dec 2023 09:39:41 +0000 Subject: [PATCH 3/4] Fix test --- .../extension/rinit_rshutdown_basic.phpt | Bin 5557 -> 5558 bytes 1 file changed, 0 insertions(+), 0 deletions(-) diff --git a/appsec/tests/extension/rinit_rshutdown_basic.phpt b/appsec/tests/extension/rinit_rshutdown_basic.phpt index c7956b98e2e790c725778a3d80620248e2062fd2..0ed364bbeceec78e7df7ad5ce1aa43f09eb3abaf 100644 GIT binary patch delta 20 ccmdn0y-j<=WX{Pqxa1};VCUMri}N=(09$tmVE_OC delta 20 ccmdm{y;Xa|WX{P!e6o`luybwR&H0-f09989-v9sr From d38718d2828de7c0852e83fda7e91b84f30f2c23 Mon Sep 17 00:00:00 2001 From: Anil Mahtani <929854+Anilm3@users.noreply.github.com> Date: Tue, 5 Dec 2023 09:54:23 +0000 Subject: [PATCH 4/4] Add max schema limit --- appsec/src/helper/subscriber/waf.cpp | 5 ++++- appsec/src/helper/subscriber/waf.hpp | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/appsec/src/helper/subscriber/waf.cpp b/appsec/src/helper/subscriber/waf.cpp index 8219ade445..0f79ef9280 100644 --- a/appsec/src/helper/subscriber/waf.cpp +++ b/appsec/src/helper/subscriber/waf.cpp @@ -247,7 +247,10 @@ void instance::listener::get_meta_and_metrics( schema = base64_encode(encoded.value(), false); } } - meta.emplace(key, std::move(schema)); + + if (schema.length() <= max_schema_size) { + meta.emplace(key, std::move(schema)); + } } } diff --git a/appsec/src/helper/subscriber/waf.hpp b/appsec/src/helper/subscriber/waf.hpp index 508b20a11c..2f8b34305b 100644 --- a/appsec/src/helper/subscriber/waf.hpp +++ b/appsec/src/helper/subscriber/waf.hpp @@ -24,7 +24,7 @@ class instance : public dds::subscriber { public: static constexpr int default_waf_timeout_us = 10000; static constexpr int max_plain_schema_allowed = 260; - + static constexpr int max_schema_size = 25000; using ptr = std::shared_ptr; class listener : public dds::subscriber::listener { public: