From b4aa5e50f2e8c7520a750f13cc786598c569ba1f Mon Sep 17 00:00:00 2001 From: Alejandro Estringana Ruiz Date: Fri, 3 Jan 2025 12:38:06 +0100 Subject: [PATCH 01/13] Implement SSRF --- .../Filesystem/FilesystemIntegration.php | 25 ++++++----- .../Filesystem/FilesystemTest.php | 45 ++++++++++++++++--- 2 files changed, 53 insertions(+), 17 deletions(-) diff --git a/src/DDTrace/Integrations/Filesystem/FilesystemIntegration.php b/src/DDTrace/Integrations/Filesystem/FilesystemIntegration.php index 528cc894c2..2f3365f0a3 100644 --- a/src/DDTrace/Integrations/Filesystem/FilesystemIntegration.php +++ b/src/DDTrace/Integrations/Filesystem/FilesystemIntegration.php @@ -51,20 +51,21 @@ public function init(): int private static function preHook($variant) { return static function (HookData $hook) use ($variant) { - if (count($hook->args) == 0 || !is_string($hook->args[0])) { - return; - } + if (count($hook->args) == 0 || !is_string($hook->args[0])) { + return; + } - $protocol = []; - if ( - preg_match('/^[a-z]+\:\/\//', $hook->args[0], $protocol) && - isset($protocol[0]) && - $protocol[0] !== 'file') - { - return; - } + $protocol = []; + $uri_parsed = preg_match('/^([a-z]+)\:\/\//', $hook->args[0], $protocol); + $protocol = isset($protocol[1]) ? $protocol[1]: ""; - \datadog\appsec\push_address("server.io.fs.file", $hook->args[0], true); + if (empty($protocol) || $protocol === 'file') { + \datadog\appsec\push_address("server.io.fs.file", $hook->args[0], true); + } + + if (in_array($variant, ['file_get_contents', 'fopen']) && (empty($protocol) || $protocol === 'http')) { + \datadog\appsec\push_address("server.io.net.url", $hook->args[0], true); + } }; } diff --git a/tests/Integrations/Filesystem/FilesystemTest.php b/tests/Integrations/Filesystem/FilesystemTest.php index ed7cbdcb96..bf84f6386a 100644 --- a/tests/Integrations/Filesystem/FilesystemTest.php +++ b/tests/Integrations/Filesystem/FilesystemTest.php @@ -15,14 +15,23 @@ protected static function getAppIndexScript() return __DIR__ . '/index.php'; } - protected function assertEvent(string $value, $traces) + protected static function getEnvs() + { + return array_merge(parent::getEnvs(), [ + 'DD_APPSEC_RASP_ENABLED' => true + ]); + } + + protected function assertEvent(string $value, $traces, $also_ssrf = false) { $events = AppsecStatus::getInstance()->getEvents(); - $this->assertEquals(1, count($events)); + $this->assertEquals($also_ssrf ? 2 : 1, count($events)); $this->assertEquals($value, $events[0]["server.io.fs.file"]); + if ($also_ssrf) { + $this->assertEquals($value, $events[1]["server.io.net.url"]); + } $this->assertEquals('push_address', $events[0]['eventName']); $this->assertTrue($events[0]['rasp']); - $this->assertGreaterThanOrEqual(0.0, $traces[0][0]['metrics']['_dd.appsec.rasp.duration_ext']); } public function testFileGetContents() @@ -32,7 +41,33 @@ public function testFileGetContents() TestCase::assertSame('OK', $response); }); - $this->assertEvent('./index.php', $traces); + $this->assertEvent('./index.php', $traces, true); + } + + public function testFileProtocol() + { + $file = __DIR__ . '/index.php'; + $traces = $this->tracesFromWebRequest(function () use ($file) { + $response = $this->call(GetSpec::create('Root', '/?function=fopen&path=file://'.$file)); + TestCase::assertSame('OK', $response); + }); + + $this->assertEvent('file://'. $file, $traces); + } + + public function testHttpProtocol() + { + $file = 'http://example.com'; + $traces = $this->tracesFromWebRequest(function () use ($file) { + $response = $this->call(GetSpec::create('Root', '/?function=fopen&path='.$file)); + TestCase::assertSame('OK', $response); + }); + + $events = AppsecStatus::getInstance()->getEvents(); + $this->assertEquals(1, count($events)); + $this->assertEquals($file, $events[0]["server.io.net.url"]); + $this->assertEquals('push_address', $events[0]['eventName']); + $this->assertTrue($events[0]['rasp']); } public function testFilePutContents() @@ -50,7 +85,7 @@ public function testFopen() $response = $this->call(GetSpec::create('Root', '/?function=fopen&path=./index.php')); TestCase::assertSame('OK', $response); }); - $this->assertEvent('./index.php', $traces); + $this->assertEvent('./index.php', $traces, true); } public function testReadFile() From 8d9f549fc9e6583af0cd70b0027bd39e5af6a57f Mon Sep 17 00:00:00 2001 From: Alejandro Estringana Ruiz Date: Fri, 3 Jan 2025 12:43:13 +0100 Subject: [PATCH 02/13] Add SSRF capability --- .../src/helper/remote_config/listeners/engine_listener.hpp | 2 +- appsec/src/helper/remote_config/product.hpp | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/appsec/src/helper/remote_config/listeners/engine_listener.hpp b/appsec/src/helper/remote_config/listeners/engine_listener.hpp index 95a2f4b34f..aee64d31a1 100644 --- a/appsec/src/helper/remote_config/listeners/engine_listener.hpp +++ b/appsec/src/helper/remote_config/listeners/engine_listener.hpp @@ -34,7 +34,7 @@ class engine_listener : public listener_base { [[nodiscard]] std::unordered_set get_supported_products() override { return {known_products::ASM, known_products::ASM_DD, - known_products::ASM_DATA, known_products::ASM_RASP_LFI}; + known_products::ASM_DATA, known_products::ASM_RASP_LFI, known_products::ASM_RASP_SSRF}; } protected: diff --git a/appsec/src/helper/remote_config/product.hpp b/appsec/src/helper/remote_config/product.hpp index 74036e40dd..d405118448 100644 --- a/appsec/src/helper/remote_config/product.hpp +++ b/appsec/src/helper/remote_config/product.hpp @@ -31,6 +31,8 @@ struct known_products { std::string_view{"ASM_FEATURES"}}; static inline constexpr product ASM_RASP_LFI{ std::string_view{"ASM_RASP_LFI"}}; + static inline constexpr product ASM_RASP_SSRF{ + std::string_view{"ASM_RASP_SSRF"}}; static inline constexpr product UNKNOWN{std::string_view{"UNKOWN"}}; static product for_name(std::string_view name) @@ -50,6 +52,9 @@ struct known_products { if (name == ASM_RASP_LFI.name()) { return ASM_RASP_LFI; } + if (name == ASM_RASP_SSRF.name()) { + return ASM_RASP_SSRF; + } return UNKNOWN; } From 68db50a7dae563b24ba67203f0daa5228e07faf7 Mon Sep 17 00:00:00 2001 From: Alejandro Estringana Ruiz Date: Tue, 7 Jan 2025 17:47:43 +0100 Subject: [PATCH 03/13] Refactor push_address to cope with multiple addresses at once --- appsec/src/extension/ddappsec.c | 43 ++++++++++------ .../tests/extension/actions_handling_01.phpt | 4 +- appsec/tests/extension/push_params_block.phpt | 6 +-- appsec/tests/extension/push_params_ok_01.phpt | 4 +- appsec/tests/extension/push_params_ok_02.phpt | 4 +- appsec/tests/extension/push_params_ok_03.phpt | 4 +- appsec/tests/extension/push_params_ok_04.phpt | 4 +- appsec/tests/extension/push_params_ok_05.phpt | 4 +- appsec/tests/extension/push_params_ok_06.phpt | 4 +- appsec/tests/extension/push_params_ok_07.phpt | 49 +++++++++++++++++++ .../extension/push_params_redirect_01.phpt | 6 +-- .../tests/extension/report_backtrace_05.phpt | 4 +- .../Filesystem/FilesystemIntegration.php | 15 ++++-- .../Laravel/LaravelIntegration.php | 4 +- .../Symfony/SymfonyIntegration.php | 4 +- .../WordPress/WordPressIntegration.php | 4 +- tests/Appsec/Mock.php | 11 ++--- .../Filesystem/FilesystemTest.php | 13 ++--- 18 files changed, 130 insertions(+), 57 deletions(-) create mode 100644 appsec/tests/extension/push_params_ok_07.phpt diff --git a/appsec/src/extension/ddappsec.c b/appsec/src/extension/ddappsec.c index f455abafa8..0c88d9b692 100644 --- a/appsec/src/extension/ddappsec.c +++ b/appsec/src/extension/ddappsec.c @@ -126,7 +126,7 @@ static void ddappsec_sort_modules(void *base, size_t count, size_t siz, // Reorder ddappsec to ensure it's always after ddtrace for (Bucket *module = base, *end = module + count, *ddappsec_module = NULL; - module < end; ++module) { + module < end; ++module) { zend_module_entry *m = (zend_module_entry *)Z_PTR(module->val); if (m->name == ddappsec_module_entry.name) { ddappsec_module = module; @@ -474,7 +474,7 @@ static PHP_FUNCTION(datadog_appsec_testing_request_exec) RETURN_TRUE; } -static PHP_FUNCTION(datadog_appsec_push_address) +static PHP_FUNCTION(datadog_appsec_push_addresses) { struct timespec start; struct timespec end; @@ -482,15 +482,14 @@ static PHP_FUNCTION(datadog_appsec_push_address) long elapsed = 0; UNUSED(return_value); if (!DDAPPSEC_G(active)) { - mlog(dd_log_debug, "Trying to access to push_address " + mlog(dd_log_debug, "Trying to access to push_addresses " "function while appsec is disabled"); return; } - zend_string *key = NULL; - zval *value = NULL; + HashTable *addresses = NULL; bool rasp = false; - if (zend_parse_parameters(ZEND_NUM_ARGS(), "Sz|b", &key, &value, &rasp) == + if (zend_parse_parameters(ZEND_NUM_ARGS(), "h|b", &addresses, &rasp) == FAILURE) { RETURN_FALSE; } @@ -499,16 +498,32 @@ static PHP_FUNCTION(datadog_appsec_push_address) return; } + if (addresses == NULL) { + return; + } + + zend_array *parameters_arr = + zend_new_array(zend_hash_num_elements(addresses)); zval parameters_zv; - zend_array *parameters_arr = zend_new_array(1); ZVAL_ARR(¶meters_zv, parameters_arr); - zend_hash_add(Z_ARRVAL(parameters_zv), key, value); - Z_TRY_ADDREF_P(value); + + zend_string *key; + zval *value; + ZEND_HASH_FOREACH_STR_KEY_VAL(addresses, key, value) + { + if (!key) { + continue; + } + + zend_hash_add(Z_ARRVAL(parameters_zv), key, value); + Z_TRY_ADDREF_P(value); + } + ZEND_HASH_FOREACH_END(); dd_conn *conn = dd_helper_mgr_cur_conn(); if (conn == NULL) { zval_ptr_dtor(¶meters_zv); - mlog_g(dd_log_debug, "No connection; skipping push_address"); + mlog_g(dd_log_debug, "No connection; skipping push_addresses"); return; } @@ -549,16 +564,16 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(request_exec_arginfo, 0, 1, _IS_BOOL, 0) ZEND_ARG_INFO(0, "data") ZEND_END_ARG_INFO() -ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(push_address_arginfo, 0, 0, IS_VOID, 1) -ZEND_ARG_INFO(0, key) -ZEND_ARG_INFO(0, value) +ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX( + push_addresses_arginfo, 0, 0, IS_VOID, 1) +ZEND_ARG_INFO(0, addresses) ZEND_ARG_INFO(0, rasp) ZEND_END_ARG_INFO() // clang-format off static const zend_function_entry functions[] = { ZEND_RAW_FENTRY(DD_APPSEC_NS "is_enabled", PHP_FN(datadog_appsec_is_enabled), void_ret_bool_arginfo, 0, NULL, NULL) - ZEND_RAW_FENTRY(DD_APPSEC_NS "push_address", PHP_FN(datadog_appsec_push_address), push_address_arginfo, 0, NULL, NULL) + ZEND_RAW_FENTRY(DD_APPSEC_NS "push_addresses", PHP_FN(datadog_appsec_push_addresses), push_addresses_arginfo, 0, NULL, NULL) PHP_FE_END }; static const zend_function_entry testing_functions[] = { diff --git a/appsec/tests/extension/actions_handling_01.phpt b/appsec/tests/extension/actions_handling_01.phpt index 3b162f5e5b..479c36ea79 100644 --- a/appsec/tests/extension/actions_handling_01.phpt +++ b/appsec/tests/extension/actions_handling_01.phpt @@ -6,7 +6,7 @@ datadog.appsec.enabled=1 --FILE-- "params", "more" => "parameters"]); +push_addresses(["server.request.path_params" => ["some" => "params", "more" => "parameters"]]); var_dump(rshutdown()); var_dump($helper->get_command("request_exec")); diff --git a/appsec/tests/extension/push_params_block.phpt b/appsec/tests/extension/push_params_block.phpt index 0a33c41b48..6cc767be24 100644 --- a/appsec/tests/extension/push_params_block.phpt +++ b/appsec/tests/extension/push_params_block.phpt @@ -6,7 +6,7 @@ datadog.appsec.enabled=1 --FILE-- "params", "more" => "parameters"]); +push_addresses(["server.request.path_params" => ["some" => "params", "more" => "parameters"]]); var_dump("THIS SHOULD NOT GET IN THE OUTPUT"); @@ -26,4 +26,4 @@ Status: 404 Not Found Content-type: application/json --EXPECTF-- {"errors": [{"title": "You've been blocked", "detail": "Sorry, you cannot access this page. Please contact the customer service team. Security provided by Datadog."}]} -Warning: datadog\appsec\push_address(): Datadog blocked the request and presented a static error page in %s on line %d +Warning: datadog\appsec\push_addresses(): Datadog blocked the request and presented a static error page in %s on line %d diff --git a/appsec/tests/extension/push_params_ok_01.phpt b/appsec/tests/extension/push_params_ok_01.phpt index 3addad4649..2d3ed8a668 100644 --- a/appsec/tests/extension/push_params_ok_01.phpt +++ b/appsec/tests/extension/push_params_ok_01.phpt @@ -6,7 +6,7 @@ datadog.appsec.enabled=1 --FILE-- "params", "more" => "parameters"]); +push_addresses(["server.request.path_params" => ["some" => "params", "more" => "parameters"]]); var_dump(rshutdown()); var_dump($helper->get_command("request_exec")); diff --git a/appsec/tests/extension/push_params_ok_02.phpt b/appsec/tests/extension/push_params_ok_02.phpt index 822385d004..5e367eab2c 100644 --- a/appsec/tests/extension/push_params_ok_02.phpt +++ b/appsec/tests/extension/push_params_ok_02.phpt @@ -6,7 +6,7 @@ datadog.appsec.enabled=1 --FILE-- "some string"]); var_dump(rshutdown()); var_dump($helper->get_command("request_exec")); diff --git a/appsec/tests/extension/push_params_ok_03.phpt b/appsec/tests/extension/push_params_ok_03.phpt index b840c936a4..c9853e4d3e 100644 --- a/appsec/tests/extension/push_params_ok_03.phpt +++ b/appsec/tests/extension/push_params_ok_03.phpt @@ -6,7 +6,7 @@ datadog.appsec.enabled=1 --FILE-- 1234]); var_dump(rshutdown()); var_dump($helper->get_command("request_exec")); diff --git a/appsec/tests/extension/push_params_ok_04.phpt b/appsec/tests/extension/push_params_ok_04.phpt index e2d95c606d..ad1479d6a5 100644 --- a/appsec/tests/extension/push_params_ok_04.phpt +++ b/appsec/tests/extension/push_params_ok_04.phpt @@ -7,7 +7,7 @@ datadog.appsec.rasp_enabled=1 --FILE-- 1234], $is_rasp); var_dump(rshutdown()); print_r(root_span_get_metrics()); diff --git a/appsec/tests/extension/push_params_ok_05.phpt b/appsec/tests/extension/push_params_ok_05.phpt index c09b292346..baafbff8e7 100644 --- a/appsec/tests/extension/push_params_ok_05.phpt +++ b/appsec/tests/extension/push_params_ok_05.phpt @@ -8,7 +8,7 @@ DD_APPSEC_RASP_ENABLED=false --FILE-- 1234], $is_rasp); var_dump(rshutdown()); print_r(root_span_get_metrics()); diff --git a/appsec/tests/extension/push_params_ok_06.phpt b/appsec/tests/extension/push_params_ok_06.phpt index 8cf52e5553..40184d61f8 100644 --- a/appsec/tests/extension/push_params_ok_06.phpt +++ b/appsec/tests/extension/push_params_ok_06.phpt @@ -6,7 +6,7 @@ datadog.appsec.enabled=1 --FILE-- 1234], $is_rasp); var_dump(rshutdown()); print_r(root_span_get_metrics()); diff --git a/appsec/tests/extension/push_params_ok_07.phpt b/appsec/tests/extension/push_params_ok_07.phpt new file mode 100644 index 0000000000..fd3fe2c93f --- /dev/null +++ b/appsec/tests/extension/push_params_ok_07.phpt @@ -0,0 +1,49 @@ +--TEST-- +Multiple addresses can be sent at once +--INI-- +extension=ddtrace.so +datadog.appsec.enabled=1 +--FILE-- + ["some" => "params", "more" => "parameters"], "some.other" => 12345]); +var_dump(rshutdown()); + +var_dump($helper->get_command("request_exec")); + +?> +--EXPECTF-- +bool(true) +bool(true) +array(2) { + [0]=> + string(12) "request_exec" + [1]=> + array(2) { + [0]=> + bool(false) + [1]=> + array(2) { + ["server.request.path_params"]=> + array(2) { + ["some"]=> + string(6) "params" + ["more"]=> + string(10) "parameters" + } + ["some.other"]=> + int(12345) + } + } +} diff --git a/appsec/tests/extension/push_params_redirect_01.phpt b/appsec/tests/extension/push_params_redirect_01.phpt index 93fe6858d8..8289d4b3dc 100644 --- a/appsec/tests/extension/push_params_redirect_01.phpt +++ b/appsec/tests/extension/push_params_redirect_01.phpt @@ -6,7 +6,7 @@ datadog.appsec.enabled=1 --FILE-- "params", "more" => "parameters"]); +push_addresses(["server.request.path_params" => ["some" => "params", "more" => "parameters"]]); var_dump("THIS SHOULD NOT GET IN THE OUTPUT"); @@ -25,4 +25,4 @@ var_dump("THIS SHOULD NOT GET IN THE OUTPUT"); Status: 303 See Other Content-type: text/html; charset=UTF-8 --EXPECTF-- -Warning: datadog\appsec\push_address(): Datadog blocked the request and attempted a redirection to https://datadoghq.com in %s on line %d +Warning: datadog\appsec\push_addresses(): Datadog blocked the request and attempted a redirection to https://datadoghq.com in %s on line %d diff --git a/appsec/tests/extension/report_backtrace_05.phpt b/appsec/tests/extension/report_backtrace_05.phpt index 5f24dc18d6..f30e6ffeff 100644 --- a/appsec/tests/extension/report_backtrace_05.phpt +++ b/appsec/tests/extension/report_backtrace_05.phpt @@ -9,7 +9,7 @@ datadog.appsec.enabled=1 --FILE-- "params", "more" => "parameters"]); + push_addresses(["irrelevant" => ["some" => "params", "more" => "parameters"]]); } function one($param01) diff --git a/src/DDTrace/Integrations/Filesystem/FilesystemIntegration.php b/src/DDTrace/Integrations/Filesystem/FilesystemIntegration.php index 2f3365f0a3..c8a7205721 100644 --- a/src/DDTrace/Integrations/Filesystem/FilesystemIntegration.php +++ b/src/DDTrace/Integrations/Filesystem/FilesystemIntegration.php @@ -16,7 +16,7 @@ public function init(): int return Integration::NOT_LOADED; } - if (!function_exists('datadog\appsec\push_address')) { + if (!function_exists('datadog\appsec\push_addresses')) { //Dont load Appsec wrappers is not available return Integration::NOT_LOADED; } @@ -59,13 +59,22 @@ private static function preHook($variant) $uri_parsed = preg_match('/^([a-z]+)\:\/\//', $hook->args[0], $protocol); $protocol = isset($protocol[1]) ? $protocol[1]: ""; + $addresses = []; + if (empty($protocol) || $protocol === 'file') { - \datadog\appsec\push_address("server.io.fs.file", $hook->args[0], true); + $addresses["server.io.fs.file"] = $hook->args[0]; } if (in_array($variant, ['file_get_contents', 'fopen']) && (empty($protocol) || $protocol === 'http')) { - \datadog\appsec\push_address("server.io.net.url", $hook->args[0], true); + $addresses["server.io.net.url"] = $hook->args[0]; + } + + if (empty($addresses)) { + return; + } + + \datadog\appsec\push_addresses($addresses, true); }; } diff --git a/src/DDTrace/Integrations/Laravel/LaravelIntegration.php b/src/DDTrace/Integrations/Laravel/LaravelIntegration.php index 7fd635d445..59f069caa3 100644 --- a/src/DDTrace/Integrations/Laravel/LaravelIntegration.php +++ b/src/DDTrace/Integrations/Laravel/LaravelIntegration.php @@ -136,10 +136,10 @@ function ($This, $scope, $args, $route) use ($integration) { if (\method_exists($route, 'uri')) { $rootSpan->meta[Tag::HTTP_ROUTE] = $route->uri(); } - if (\method_exists($route, 'parameters') && function_exists('\datadog\appsec\push_address')) { + if (\method_exists($route, 'parameters') && function_exists('\datadog\appsec\push_addresses')) { $parameters = $route->parameters(); if (count($parameters) > 0) { - \datadog\appsec\push_address("server.request.path_params", $parameters); + \datadog\appsec\push_addresses(["server.request.path_params" => $parameters]); } } $rootSpan->meta[Tag::HTTP_METHOD] = $request->method(); diff --git a/src/DDTrace/Integrations/Symfony/SymfonyIntegration.php b/src/DDTrace/Integrations/Symfony/SymfonyIntegration.php index b5d685aa91..07c14acf3d 100644 --- a/src/DDTrace/Integrations/Symfony/SymfonyIntegration.php +++ b/src/DDTrace/Integrations/Symfony/SymfonyIntegration.php @@ -401,8 +401,8 @@ function (SpanData $span, $args, $response) use ($integration) { $parameters = $request->get('_route_params'); if (!empty($parameters) && is_array($parameters) && - function_exists('\datadog\appsec\push_address')) { - \datadog\appsec\push_address("server.request.path_params", $parameters); + function_exists('\datadog\appsec\push_addresses')) { + \datadog\appsec\push_addresses(["server.request.path_params" => $parameters]); } $route = $request->get('_route'); diff --git a/src/DDTrace/Integrations/WordPress/WordPressIntegration.php b/src/DDTrace/Integrations/WordPress/WordPressIntegration.php index ce8a00b1be..1b53520d42 100644 --- a/src/DDTrace/Integrations/WordPress/WordPressIntegration.php +++ b/src/DDTrace/Integrations/WordPress/WordPressIntegration.php @@ -55,12 +55,12 @@ public function init(): int \DDTrace\hook_method('WP', 'main', null, function ($This, $scope, $args) { if (\property_exists($This, 'did_permalink') && $This->did_permalink === true) { - if (function_exists('\datadog\appsec\push_address') && + if (function_exists('\datadog\appsec\push_addresses') && \property_exists($This, 'query_vars') && function_exists('is_404') && is_404() === false) { $parameters = $This->query_vars; if (count($parameters) > 0) { - \datadog\appsec\push_address("server.request.path_params", $parameters); + \datadog\appsec\push_addresses(["server.request.path_params" => $parameters]); } } } diff --git a/tests/Appsec/Mock.php b/tests/Appsec/Mock.php index 99b7876c89..f817a1ed7a 100644 --- a/tests/Appsec/Mock.php +++ b/tests/Appsec/Mock.php @@ -205,16 +205,15 @@ function track_user_signup_event($userId, $metadata) } } -if (!function_exists('datadog\appsec\push_address')) { +if (!function_exists('datadog\appsec\push_addresses')) { /** * This function is exposed by appsec but here we are mocking it for tests * @param array $params */ - function push_address($key, $value, $rasp = false) - { - if (!appsecMockEnabled()) { - return; + function push_addresses($addresses, $rasp = false) { + if(!appsecMockEnabled()) { + return; } - AppsecStatus::getInstance()->addEvent(['rasp' => $rasp, $key => $value], 'push_address'); + AppsecStatus::getInstance()->addEvent(['rasp' => $rasp, $addresses], 'push_addresses'); } } diff --git a/tests/Integrations/Filesystem/FilesystemTest.php b/tests/Integrations/Filesystem/FilesystemTest.php index bf84f6386a..0f0b9b679b 100644 --- a/tests/Integrations/Filesystem/FilesystemTest.php +++ b/tests/Integrations/Filesystem/FilesystemTest.php @@ -25,12 +25,13 @@ protected static function getEnvs() protected function assertEvent(string $value, $traces, $also_ssrf = false) { $events = AppsecStatus::getInstance()->getEvents(); - $this->assertEquals($also_ssrf ? 2 : 1, count($events)); - $this->assertEquals($value, $events[0]["server.io.fs.file"]); + $this->assertEquals(1, count($events)); + $this->assertEquals($also_ssrf ? 2 : 1, count($events[0][0])); + $this->assertEquals($value, $events[0][0]["server.io.fs.file"]); if ($also_ssrf) { - $this->assertEquals($value, $events[1]["server.io.net.url"]); + $this->assertEquals($value, $events[0][0]["server.io.net.url"]); } - $this->assertEquals('push_address', $events[0]['eventName']); + $this->assertEquals('push_addresses', $events[0]['eventName']); $this->assertTrue($events[0]['rasp']); } @@ -65,8 +66,8 @@ public function testHttpProtocol() $events = AppsecStatus::getInstance()->getEvents(); $this->assertEquals(1, count($events)); - $this->assertEquals($file, $events[0]["server.io.net.url"]); - $this->assertEquals('push_address', $events[0]['eventName']); + $this->assertEquals($file, $events[0][0]["server.io.net.url"]); + $this->assertEquals('push_addresses', $events[0]['eventName']); $this->assertTrue($events[0]['rasp']); } From eeda697de879bc30c1340b856b205e2109c18945 Mon Sep 17 00:00:00 2001 From: Alejandro Estringana Ruiz Date: Wed, 8 Jan 2025 11:14:17 +0100 Subject: [PATCH 04/13] Fix failing tests --- tests/Appsec/Mock.php | 2 +- tests/Integrations/Laravel/PathParamsTestSuite.php | 14 +++++++------- tests/Integrations/Symfony/PathParamsTestSuite.php | 14 +++++++------- .../Integrations/WordPress/PathParamsTestSuite.php | 14 +++++++------- 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/tests/Appsec/Mock.php b/tests/Appsec/Mock.php index f817a1ed7a..ae5006a7d1 100644 --- a/tests/Appsec/Mock.php +++ b/tests/Appsec/Mock.php @@ -77,7 +77,7 @@ public function getEvents(array $names = [], array $addresses = []) foreach ($events as $event) { $new = json_decode($event['event'], true); if (empty($names) || in_array($new['eventName'], $names) && - (empty($addresses) || !empty(array_intersect($addresses, array_keys($new))))) { + (empty($addresses) || !empty(array_intersect($addresses, array_keys($new[0]))))) { $result[] = $new; } } diff --git a/tests/Integrations/Laravel/PathParamsTestSuite.php b/tests/Integrations/Laravel/PathParamsTestSuite.php index f6ffdc2005..4c77ed1d45 100644 --- a/tests/Integrations/Laravel/PathParamsTestSuite.php +++ b/tests/Integrations/Laravel/PathParamsTestSuite.php @@ -15,10 +15,10 @@ public function testDynamicRouteWithAllParametersGiven() $this->call( GetSpec::create('Call to dynamic route', "/dynamic_route/$param01/static/$param02") ); - $events = AppsecStatus::getInstance()->getEvents(['push_address'], ['server.request.path_params']); + $events = AppsecStatus::getInstance()->getEvents(['push_addresses'], ['server.request.path_params']); $this->assertEquals(1, count($events)); - $this->assertEquals($param01, $events[0]["server.request.path_params"]['param01']); - $this->assertEquals($param02, $events[0]["server.request.path_params"]['param02']); + $this->assertEquals($param01, $events[0][0]["server.request.path_params"]['param01']); + $this->assertEquals($param02, $events[0][0]["server.request.path_params"]['param02']); } public function testDynamicRouteWithOptionalParametersNotGiven() @@ -27,10 +27,10 @@ public function testDynamicRouteWithOptionalParametersNotGiven() $this->call( GetSpec::create('Call to dynamic route', "/dynamic_route/$param01/static") ); - $events = AppsecStatus::getInstance()->getEvents(['push_address'], ['server.request.path_params']); + $events = AppsecStatus::getInstance()->getEvents(['push_addresses'], ['server.request.path_params']); $this->assertEquals(1, count($events)); - $this->assertCount(1, $events[0]["server.request.path_params"]); - $this->assertEquals($param01, $events[0]["server.request.path_params"]['param01']); + $this->assertCount(1, $events[0][0]["server.request.path_params"]); + $this->assertEquals($param01, $events[0][0]["server.request.path_params"]['param01']); } public function testStaticRouteDoesNotGenerateEvent() @@ -38,7 +38,7 @@ public function testStaticRouteDoesNotGenerateEvent() $this->call( GetSpec::create('Call to static route', "/simple") ); - $events = AppsecStatus::getInstance()->getEvents(['push_address'], ['server.request.path_params']); + $events = AppsecStatus::getInstance()->getEvents(['push_addresses'], ['server.request.path_params']); $this->assertEquals(0, count($events)); } } diff --git a/tests/Integrations/Symfony/PathParamsTestSuite.php b/tests/Integrations/Symfony/PathParamsTestSuite.php index a6cf11161e..d3ff51c280 100644 --- a/tests/Integrations/Symfony/PathParamsTestSuite.php +++ b/tests/Integrations/Symfony/PathParamsTestSuite.php @@ -17,26 +17,26 @@ public function testDynamicRouteWithOptionalsFilled() $param01 = 'first_param'; $param02 = 'second_param'; $this->call(GetSpec::create('dynamic', "/dynamic_route/$param01/$param02")); - $events = AppsecStatus::getInstance()->getEvents(['push_address'], ['server.request.path_params']); + $events = AppsecStatus::getInstance()->getEvents(['push_addresses'], ['server.request.path_params']); $this->assertEquals(1, count($events)); - $this->assertEquals($param01, $events[0]["server.request.path_params"]['param01']); - $this->assertEquals($param02, $events[0]["server.request.path_params"]['param02']); + $this->assertEquals($param01, $events[0][0]["server.request.path_params"]['param01']); + $this->assertEquals($param02, $events[0][0]["server.request.path_params"]['param02']); } public function testDynamicRouteWithOptionalsNotFilled() { $param01 = 'first_param'; $this->call(GetSpec::create('dynamic', "/dynamic_route/$param01")); - $events = AppsecStatus::getInstance()->getEvents(['push_address'], ['server.request.path_params']); + $events = AppsecStatus::getInstance()->getEvents(['push_addresses'], ['server.request.path_params']); $this->assertEquals(1, count($events)); - $this->assertEquals($param01, $events[0]["server.request.path_params"]['param01']); - $this->assertEmpty($events[0]["server.request.path_params"]['param02']); + $this->assertEquals($param01, $events[0][0]["server.request.path_params"]['param01']); + $this->assertEmpty($events[0][0]["server.request.path_params"]['param02']); } public function testStaticRoute() { $this->call(GetSpec::create('static', "/simple")); - $events = AppsecStatus::getInstance()->getEvents(['push_address'], ['server.request.path_params']); + $events = AppsecStatus::getInstance()->getEvents(['push_addresses'], ['server.request.path_params']); $this->assertEquals(0, count($events)); } } diff --git a/tests/Integrations/WordPress/PathParamsTestSuite.php b/tests/Integrations/WordPress/PathParamsTestSuite.php index a3a5520c6d..8b75c43e37 100644 --- a/tests/Integrations/WordPress/PathParamsTestSuite.php +++ b/tests/Integrations/WordPress/PathParamsTestSuite.php @@ -17,9 +17,9 @@ public function testPost() ) ); - $events = AppsecStatus::getInstance()->getEvents(['push_address'], ['server.request.path_params']); + $events = AppsecStatus::getInstance()->getEvents(['push_addresses'], ['server.request.path_params']); $this->assertEquals(1, count($events)); - $this->assertEquals('hello-world', $events[0]["server.request.path_params"]['name']); + $this->assertEquals('hello-world', $events[0][0]["server.request.path_params"]['name']); } public function testCategory() @@ -31,9 +31,9 @@ public function testCategory() ) ); - $events = AppsecStatus::getInstance()->getEvents(['push_address'], ['server.request.path_params']); + $events = AppsecStatus::getInstance()->getEvents(['push_addresses'], ['server.request.path_params']); $this->assertEquals(1, count($events)); - $this->assertEquals('uncategorized', $events[0]["server.request.path_params"]['category_name']); + $this->assertEquals('uncategorized', $events[0][0]["server.request.path_params"]['category_name']); } public function testAuthor() @@ -45,9 +45,9 @@ public function testAuthor() ) ); - $events = AppsecStatus::getInstance()->getEvents(['push_address'], ['server.request.path_params']); + $events = AppsecStatus::getInstance()->getEvents(['push_addresses'], ['server.request.path_params']); $this->assertEquals(1, count($events)); - $this->assertEquals('test', $events[0]["server.request.path_params"]['author_name']); + $this->assertEquals('test', $events[0][0]["server.request.path_params"]['author_name']); } public function testNonExistingPost() @@ -59,7 +59,7 @@ public function testNonExistingPost() ) ); - $events = AppsecStatus::getInstance()->getEvents(['push_address'], ['server.request.path_params']); + $events = AppsecStatus::getInstance()->getEvents(['push_addresses'], ['server.request.path_params']); $this->assertEquals(0, count($events)); } } From e3276608e05d080a352f427118d8440304e4b9ed Mon Sep 17 00:00:00 2001 From: Alejandro Estringana Ruiz Date: Thu, 9 Jan 2025 11:43:56 +0100 Subject: [PATCH 05/13] Address comments from PR --- appsec/src/extension/ddappsec.c | 4 --- .../Filesystem/FilesystemIntegration.php | 3 +- .../Filesystem/FilesystemTest.php | 36 ++++++++++++++++--- tests/Integrations/Filesystem/index.php | 6 ++-- 4 files changed, 36 insertions(+), 13 deletions(-) diff --git a/appsec/src/extension/ddappsec.c b/appsec/src/extension/ddappsec.c index 0c88d9b692..e823156e9a 100644 --- a/appsec/src/extension/ddappsec.c +++ b/appsec/src/extension/ddappsec.c @@ -498,10 +498,6 @@ static PHP_FUNCTION(datadog_appsec_push_addresses) return; } - if (addresses == NULL) { - return; - } - zend_array *parameters_arr = zend_new_array(zend_hash_num_elements(addresses)); zval parameters_zv; diff --git a/src/DDTrace/Integrations/Filesystem/FilesystemIntegration.php b/src/DDTrace/Integrations/Filesystem/FilesystemIntegration.php index c8a7205721..51874f90a1 100644 --- a/src/DDTrace/Integrations/Filesystem/FilesystemIntegration.php +++ b/src/DDTrace/Integrations/Filesystem/FilesystemIntegration.php @@ -65,7 +65,8 @@ private static function preHook($variant) $addresses["server.io.fs.file"] = $hook->args[0]; } - if (in_array($variant, ['file_get_contents', 'fopen']) && (empty($protocol) || $protocol === 'http')) { + if (in_array($variant, ['file_get_contents', 'fopen']) && + (empty($protocol) || in_array($protocol, ['http', 'https', 'ftp', 'ftps']))) { $addresses["server.io.net.url"] = $hook->args[0]; } diff --git a/tests/Integrations/Filesystem/FilesystemTest.php b/tests/Integrations/Filesystem/FilesystemTest.php index 0f0b9b679b..f5a60c1186 100644 --- a/tests/Integrations/Filesystem/FilesystemTest.php +++ b/tests/Integrations/Filesystem/FilesystemTest.php @@ -56,21 +56,47 @@ public function testFileProtocol() $this->assertEvent('file://'. $file, $traces); } - public function testHttpProtocol() + public function raspProtocols() { - $file = 'http://example.com'; - $traces = $this->tracesFromWebRequest(function () use ($file) { - $response = $this->call(GetSpec::create('Root', '/?function=fopen&path='.$file)); + return [ + ['http'], + ['https'], + ['ftp'], + ['ftps'] + ]; + } + + /** + * @dataProvider raspProtocols + */ + public function testRaspProtocols($protocol) + { + $url = $protocol.'://example.com'; + $traces = $this->tracesFromWebRequest(function () use ($url) { + $response = $this->call(GetSpec::create('Root', '/?function=fopen&path='.$url)); TestCase::assertSame('OK', $response); }); $events = AppsecStatus::getInstance()->getEvents(); $this->assertEquals(1, count($events)); - $this->assertEquals($file, $events[0][0]["server.io.net.url"]); + $this->assertEquals(1, count($events[0][0])); + $this->assertEquals($url, $events[0][0]["server.io.net.url"]); $this->assertEquals('push_addresses', $events[0]['eventName']); $this->assertTrue($events[0]['rasp']); } + public function testInvalidProtocol() + { + $url = 'bad://example.com'; + $traces = $this->tracesFromWebRequest(function () use ($url) { + $response = $this->call(GetSpec::create('Root', '/?function=fopen&path='.$url)); + TestCase::assertSame('OK', $response); + }); + + $events = AppsecStatus::getInstance()->getEvents(); + $this->assertEquals(0, count($events)); + } + public function testFilePutContents() { $traces = $this->tracesFromWebRequest(function () { diff --git a/tests/Integrations/Filesystem/index.php b/tests/Integrations/Filesystem/index.php index 6e371a1630..1082573dfa 100644 --- a/tests/Integrations/Filesystem/index.php +++ b/tests/Integrations/Filesystem/index.php @@ -7,13 +7,13 @@ switch ($function) { case 'file_put_contents': - file_put_contents($path, 'some content'); + @file_put_contents($path, 'some content'); break; case 'fopen': - fopen($path, 'r'); + @fopen($path, 'r'); break; default: - $function($path); + @$function($path); break; } From 27869d4f968d80e9ba0b1ed4e98d9c3c98679cce Mon Sep 17 00:00:00 2001 From: Alejandro Estringana Ruiz Date: Thu, 9 Jan 2025 12:57:19 +0100 Subject: [PATCH 06/13] Set rasp capabilities properly --- .../remote_config/listeners/engine_listener.hpp | 2 +- appsec/src/helper/remote_config/product.hpp | 11 ----------- components-rs/remote_config.rs | 2 ++ 3 files changed, 3 insertions(+), 12 deletions(-) diff --git a/appsec/src/helper/remote_config/listeners/engine_listener.hpp b/appsec/src/helper/remote_config/listeners/engine_listener.hpp index aee64d31a1..d58828bf35 100644 --- a/appsec/src/helper/remote_config/listeners/engine_listener.hpp +++ b/appsec/src/helper/remote_config/listeners/engine_listener.hpp @@ -34,7 +34,7 @@ class engine_listener : public listener_base { [[nodiscard]] std::unordered_set get_supported_products() override { return {known_products::ASM, known_products::ASM_DD, - known_products::ASM_DATA, known_products::ASM_RASP_LFI, known_products::ASM_RASP_SSRF}; + known_products::ASM_DATA}; } protected: diff --git a/appsec/src/helper/remote_config/product.hpp b/appsec/src/helper/remote_config/product.hpp index d405118448..07085394f6 100644 --- a/appsec/src/helper/remote_config/product.hpp +++ b/appsec/src/helper/remote_config/product.hpp @@ -29,10 +29,6 @@ struct known_products { static inline constexpr product ASM_DATA{std::string_view{"ASM_DATA"}}; static inline constexpr product ASM_FEATURES{ std::string_view{"ASM_FEATURES"}}; - static inline constexpr product ASM_RASP_LFI{ - std::string_view{"ASM_RASP_LFI"}}; - static inline constexpr product ASM_RASP_SSRF{ - std::string_view{"ASM_RASP_SSRF"}}; static inline constexpr product UNKNOWN{std::string_view{"UNKOWN"}}; static product for_name(std::string_view name) @@ -49,13 +45,6 @@ struct known_products { if (name == ASM_FEATURES.name()) { return ASM_FEATURES; } - if (name == ASM_RASP_LFI.name()) { - return ASM_RASP_LFI; - } - if (name == ASM_RASP_SSRF.name()) { - return ASM_RASP_SSRF; - } - return UNKNOWN; } }; diff --git a/components-rs/remote_config.rs b/components-rs/remote_config.rs index 97551fb26f..7070e1db1b 100644 --- a/components-rs/remote_config.rs +++ b/components-rs/remote_config.rs @@ -120,6 +120,8 @@ pub unsafe extern "C" fn ddog_init_remote_config( RemoteConfigCapabilities::AsmCustomRules, RemoteConfigCapabilities::AsmCustomBlockingResponse, RemoteConfigCapabilities::AsmTrustedIps, + RemoteConfigCapabilities::AsmRaspLfi, + RemoteConfigCapabilities::AsmRaspSsrf, ] .iter() .for_each(|c| DDTRACE_REMOTE_CONFIG_CAPABILITIES.push(*c)); From c95b186dfd08ddb801d3a86ce44da341d81ed0e6 Mon Sep 17 00:00:00 2001 From: Alejandro Estringana Ruiz Date: Wed, 22 Jan 2025 17:31:53 +0100 Subject: [PATCH 07/13] Add SSRF integration tests --- .../appsec/php/integration/CommonTests.groovy | 54 +++++++++++++++++-- .../php/integration/RemoteConfigTests.groovy | 2 + .../integration/src/test/waf/recommended.json | 48 +++++++++++++++++ .../src/test/www/base/public/ssrf.php | 28 ++++++++++ 4 files changed, 129 insertions(+), 3 deletions(-) create mode 100644 appsec/tests/integration/src/test/www/base/public/ssrf.php diff --git a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/CommonTests.groovy b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/CommonTests.groovy index 28d0dc315e..f7f4e9368d 100644 --- a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/CommonTests.groovy +++ b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/CommonTests.groovy @@ -275,7 +275,7 @@ trait CommonTests { assert exploit.frames[2].line == 15 } - static Stream getTestData() { + static Stream getTestLfiData() { return Arrays.stream(new Arguments[]{ Arguments.of("file_put_contents", "/tmp/dummy", 9), Arguments.of("readfile", "/tmp/dummy", 15), @@ -285,8 +285,8 @@ trait CommonTests { } @ParameterizedTest - @MethodSource("getTestData") - void 'file_put_contents generates LFI signal'(String target_function, String path, Integer line) { + @MethodSource("getTestLfiData") + void 'filesystem functions generate LFI signal'(String target_function, String path, Integer line) { HttpRequest req = container.buildReq('/filesystem.php?function='+target_function+"&path="+path).GET().build() def trace = container.traceFromRequest(req, ofString()) { HttpResponse re -> assert re.statusCode() == 200 @@ -528,4 +528,52 @@ trait CommonTests { throw new AssertionError("Module has STATIC_TLS flag: $res.stdout") } } + + static Stream getTestSsrfData() { + return Arrays.stream(new Arguments[]{ + Arguments.of("file_get_contents", 12), + Arguments.of("fopen", 9), + }); + } + + @ParameterizedTest + @MethodSource("getTestSsrfData") + void 'filesystem functions generate SSRF signal'(String target_function, Integer line) { + HttpRequest req = container.buildReq('/ssrf.php?function='+target_function+"&domain=169.254.169.254").GET().build() + def trace = container.traceFromRequest(req, ofString()) { HttpResponse re -> + assert re.statusCode() == 200 + assert re.body().contains('OK') + } + + Span span = trace.first() + + assert span.metrics."_dd.appsec.enabled" == 1.0d + assert span.metrics."_dd.appsec.waf.duration" > 0.0d + assert span.meta."_dd.appsec.event_rules.version" != '' + + InputStream stream = new ByteArrayInputStream( span.meta_struct."_dd.stack".decodeBase64() ) + MessageUnpacker unpacker = MessagePack.newDefaultUnpacker(stream) + List stacks = [] + stacks << MsgpackHelper.unpackSingle(unpacker) + Object exploit = stacks.first().exploit.first() + + assert exploit.language == "php" + assert exploit.id ==~ /^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$/ + assert exploit.frames[0].file == "ssrf.php" + assert exploit.frames[0].function == target_function + assert exploit.frames[0].id == 1 + assert exploit.frames[0].line == line + assert exploit.frames[1].file == "ssrf.php" + assert exploit.frames[1].function == "one" + assert exploit.frames[1].id == 2 + assert exploit.frames[1].line == 18 + assert exploit.frames[2].file == "ssrf.php" + assert exploit.frames[2].function == "two" + assert exploit.frames[2].id == 3 + assert exploit.frames[2].line == 22 + assert exploit.frames[3].file == "ssrf.php" + assert exploit.frames[3].function == "three" + assert exploit.frames[3].id == 4 + assert exploit.frames[3].line == 25 + } } diff --git a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RemoteConfigTests.groovy b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RemoteConfigTests.groovy index ed8036751d..1d788f02aa 100644 --- a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RemoteConfigTests.groovy +++ b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RemoteConfigTests.groovy @@ -84,6 +84,8 @@ class RemoteConfigTests { Capability.ASM_CUSTOM_RULES, Capability.ASM_CUSTOM_BLOCKING_RESPONSE, Capability.ASM_TRUSTED_IPS, + Capability.ASM_RASP_LFI, + Capability.ASM_RASP_SSRF, ].each { assert it in capSet } doReq.call(403) diff --git a/appsec/tests/integration/src/test/waf/recommended.json b/appsec/tests/integration/src/test/waf/recommended.json index 6d60f3236e..5309a0d333 100644 --- a/appsec/tests/integration/src/test/waf/recommended.json +++ b/appsec/tests/integration/src/test/waf/recommended.json @@ -85,6 +85,54 @@ "stack_trace" ] }, + { + "id": "rasp-934-100", + "name": "Server-side request forgery exploit", + "tags": { + "type": "ssrf", + "category": "vulnerability_trigger", + "cwe": "918", + "capec": "1000/225/115/664", + "confidence": "1", + "module": "rasp" + }, + "conditions": [ + { + "parameters": { + "resource": [ + { + "address": "server.io.net.url" + } + ], + "params": [ + { + "address": "server.request.query" + }, + { + "address": "server.request.body" + }, + { + "address": "server.request.path_params" + }, + { + "address": "grpc.server.request.message" + }, + { + "address": "graphql.server.all_resolvers" + }, + { + "address": "graphql.server.resolver" + } + ] + }, + "operator": "ssrf_detector" + } + ], + "transformers": [], + "on_match": [ + "stack_trace" + ] + }, { "id": "blk-001-003", "name": "Block User Addresses", diff --git a/appsec/tests/integration/src/test/www/base/public/ssrf.php b/appsec/tests/integration/src/test/www/base/public/ssrf.php new file mode 100644 index 0000000000..52c49a5752 --- /dev/null +++ b/appsec/tests/integration/src/test/www/base/public/ssrf.php @@ -0,0 +1,28 @@ + Date: Thu, 23 Jan 2025 11:45:46 +0100 Subject: [PATCH 08/13] Update push_address to push_addresses --- appsec/tests/extension/push_params_block_02.phpt | 4 ++-- appsec/tests/extension/push_params_block_03.phpt | 4 ++-- appsec/tests/extension/push_params_redirect_02.phpt | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/appsec/tests/extension/push_params_block_02.phpt b/appsec/tests/extension/push_params_block_02.phpt index a11c052a8f..b499024bb1 100644 --- a/appsec/tests/extension/push_params_block_02.phpt +++ b/appsec/tests/extension/push_params_block_02.phpt @@ -6,7 +6,7 @@ datadog.appsec.enabled=1 --FILE-- "params", "more" => "parameters"]); + push_addresses(["server.request.path_params", ["some" => "params", "more" => "parameters"]]); var_dump("This should be executed"); }; } diff --git a/appsec/tests/extension/push_params_block_03.phpt b/appsec/tests/extension/push_params_block_03.phpt index eba1d26d4a..848a494a3b 100644 --- a/appsec/tests/extension/push_params_block_03.phpt +++ b/appsec/tests/extension/push_params_block_03.phpt @@ -6,7 +6,7 @@ datadog.appsec.enabled=1 --FILE-- "params", "more" => "parameters"]); + push_addresses(["server.request.path_params", ["some" => "params", "more" => "parameters"]]); var_dump("This should be executed"); }; } diff --git a/appsec/tests/extension/push_params_redirect_02.phpt b/appsec/tests/extension/push_params_redirect_02.phpt index e169294abf..0bfd5272a0 100644 --- a/appsec/tests/extension/push_params_redirect_02.phpt +++ b/appsec/tests/extension/push_params_redirect_02.phpt @@ -6,7 +6,7 @@ datadog.appsec.enabled=1 --FILE-- "params", "more" => "parameters"]); + push_addresses(["server.request.path_params", ["some" => "params", "more" => "parameters"]]); var_dump("This should be executed"); }; } From 76a43d0e3d947eac9de6e38ffd720795c92fa7df Mon Sep 17 00:00:00 2001 From: Alejandro Estringana Ruiz Date: Thu, 23 Jan 2025 13:12:25 +0100 Subject: [PATCH 09/13] Revert format change as it is not clang valid --- appsec/src/extension/ddappsec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/appsec/src/extension/ddappsec.c b/appsec/src/extension/ddappsec.c index e823156e9a..69a338ba72 100644 --- a/appsec/src/extension/ddappsec.c +++ b/appsec/src/extension/ddappsec.c @@ -126,7 +126,7 @@ static void ddappsec_sort_modules(void *base, size_t count, size_t siz, // Reorder ddappsec to ensure it's always after ddtrace for (Bucket *module = base, *end = module + count, *ddappsec_module = NULL; - module < end; ++module) { + module < end; ++module) { zend_module_entry *m = (zend_module_entry *)Z_PTR(module->val); if (m->name == ddappsec_module_entry.name) { ddappsec_module = module; From 1ee08fa44834de44e555ccad405b146d55aa3f21 Mon Sep 17 00:00:00 2001 From: Alejandro Estringana Ruiz Date: Fri, 24 Jan 2025 13:04:18 +0100 Subject: [PATCH 10/13] Avoid copy addresses array --- appsec/src/extension/ddappsec.c | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/appsec/src/extension/ddappsec.c b/appsec/src/extension/ddappsec.c index 69a338ba72..ab4e4ec27e 100644 --- a/appsec/src/extension/ddappsec.c +++ b/appsec/src/extension/ddappsec.c @@ -498,23 +498,9 @@ static PHP_FUNCTION(datadog_appsec_push_addresses) return; } - zend_array *parameters_arr = - zend_new_array(zend_hash_num_elements(addresses)); zval parameters_zv; - ZVAL_ARR(¶meters_zv, parameters_arr); - - zend_string *key; - zval *value; - ZEND_HASH_FOREACH_STR_KEY_VAL(addresses, key, value) - { - if (!key) { - continue; - } - - zend_hash_add(Z_ARRVAL(parameters_zv), key, value); - Z_TRY_ADDREF_P(value); - } - ZEND_HASH_FOREACH_END(); + ZVAL_ARR(¶meters_zv, addresses); + Z_TRY_ADDREF_P(¶meters_zv); dd_conn *conn = dd_helper_mgr_cur_conn(); if (conn == NULL) { From b9f23a42f1feebcccd22eee697082d23ee1ca579 Mon Sep 17 00:00:00 2001 From: Alejandro Estringana Ruiz Date: Tue, 28 Jan 2025 13:06:09 +0100 Subject: [PATCH 11/13] Amend lfi protocol --- .../Filesystem/FilesystemIntegration.php | 3 +- .../Filesystem/FilesystemTest.php | 97 +++++++++---------- 2 files changed, 48 insertions(+), 52 deletions(-) diff --git a/src/DDTrace/Integrations/Filesystem/FilesystemIntegration.php b/src/DDTrace/Integrations/Filesystem/FilesystemIntegration.php index 51874f90a1..73fa64bd79 100644 --- a/src/DDTrace/Integrations/Filesystem/FilesystemIntegration.php +++ b/src/DDTrace/Integrations/Filesystem/FilesystemIntegration.php @@ -66,9 +66,8 @@ private static function preHook($variant) } if (in_array($variant, ['file_get_contents', 'fopen']) && - (empty($protocol) || in_array($protocol, ['http', 'https', 'ftp', 'ftps']))) { + in_array($protocol, ['http', 'https', 'ftp', 'ftps'])) { $addresses["server.io.net.url"] = $hook->args[0]; - } if (empty($addresses)) { diff --git a/tests/Integrations/Filesystem/FilesystemTest.php b/tests/Integrations/Filesystem/FilesystemTest.php index f5a60c1186..0617e2e431 100644 --- a/tests/Integrations/Filesystem/FilesystemTest.php +++ b/tests/Integrations/Filesystem/FilesystemTest.php @@ -22,41 +22,18 @@ protected static function getEnvs() ]); } - protected function assertEvent(string $value, $traces, $also_ssrf = false) + protected function assertEvent(string $value, $traces, $ssrf = false) { $events = AppsecStatus::getInstance()->getEvents(); $this->assertEquals(1, count($events)); - $this->assertEquals($also_ssrf ? 2 : 1, count($events[0][0])); - $this->assertEquals($value, $events[0][0]["server.io.fs.file"]); - if ($also_ssrf) { - $this->assertEquals($value, $events[0][0]["server.io.net.url"]); - } + $this->assertEquals(1, count($events[0][0])); + $key = !$ssrf ? "server.io.fs.file" : "server.io.net.url"; + $this->assertEquals($value, $events[0][0][$key]); $this->assertEquals('push_addresses', $events[0]['eventName']); $this->assertTrue($events[0]['rasp']); } - public function testFileGetContents() - { - $traces = $this->tracesFromWebRequest(function () { - $response = $this->call(GetSpec::create('Root', '/?function=file_get_contents&path=./index.php')); - TestCase::assertSame('OK', $response); - }); - - $this->assertEvent('./index.php', $traces, true); - } - - public function testFileProtocol() - { - $file = __DIR__ . '/index.php'; - $traces = $this->tracesFromWebRequest(function () use ($file) { - $response = $this->call(GetSpec::create('Root', '/?function=fopen&path=file://'.$file)); - TestCase::assertSame('OK', $response); - }); - - $this->assertEvent('file://'. $file, $traces); - } - - public function raspProtocols() + public function ssrfProtocols() { return [ ['http'], @@ -67,9 +44,9 @@ public function raspProtocols() } /** - * @dataProvider raspProtocols + * @dataProvider ssrfProtocols */ - public function testRaspProtocols($protocol) + public function testSsrfProtocols($protocol) { $url = $protocol.'://example.com'; $traces = $this->tracesFromWebRequest(function () use ($url) { @@ -77,12 +54,7 @@ public function testRaspProtocols($protocol) TestCase::assertSame('OK', $response); }); - $events = AppsecStatus::getInstance()->getEvents(); - $this->assertEquals(1, count($events)); - $this->assertEquals(1, count($events[0][0])); - $this->assertEquals($url, $events[0][0]["server.io.net.url"]); - $this->assertEquals('push_addresses', $events[0]['eventName']); - $this->assertTrue($events[0]['rasp']); + $this->assertEvent($url, $traces, true); } public function testInvalidProtocol() @@ -97,30 +69,55 @@ public function testInvalidProtocol() $this->assertEquals(0, count($events)); } - public function testFilePutContents() + public function wrappedFunctions() { - $traces = $this->tracesFromWebRequest(function () { - $response = $this->call(GetSpec::create('Root', '/?function=file_put_contents&path=./somefile')); - TestCase::assertSame('OK', $response); + return [ + ['file_get_contents', 'ssrf' => true], + ['file_put_contents', 'ssrf' => false], + ['fopen', 'ssrf' => true], + ['readfile', 'ssrf' => false], + ]; + } + + /** + * @dataProvider wrappedFunctions + */ + public function testNoProtocol($targetFunction, $ssrf) + { + $traces = $this->tracesFromWebRequest(function () use($targetFunction) { + $response = $this->call(GetSpec::create('Root', '/?function='.$targetFunction.'&path=./somefile')); + + TestCase::assertSame('OK', str_replace('some content', '', $response)); }); - $this->assertEvent('./somefile', $traces); + $this->assertEvent('./somefile', $traces, false); } - public function testFopen() + /** + * @dataProvider wrappedFunctions + */ + public function testWithFileProtocol($targetFunction, $ssrf) { - $traces = $this->tracesFromWebRequest(function () { - $response = $this->call(GetSpec::create('Root', '/?function=fopen&path=./index.php')); + $traces = $this->tracesFromWebRequest(function () use($targetFunction) { + $response = $this->call(GetSpec::create('Root', '/?function='.$targetFunction.'&path=file://somefile')); TestCase::assertSame('OK', $response); }); - $this->assertEvent('./index.php', $traces, true); + $this->assertEvent('file://somefile', $traces, false); } - public function testReadFile() + /** + * @dataProvider wrappedFunctions + */ + public function testWithHttpProtocol($targetFunction, $ssrf) { - $traces = $this->tracesFromWebRequest(function () { - $response = $this->call(GetSpec::create('Root', '/?function=readfile&path=./dummy')); - TestCase::assertSame("Dummy file content\nOK", $response); + $traces = $this->tracesFromWebRequest(function () use($targetFunction) { + $response = $this->call(GetSpec::create('Root', '/?function='.$targetFunction.'&path=http://some.url')); + TestCase::assertSame('OK', $response); }); - $this->assertEvent('./dummy', $traces); + $events = AppsecStatus::getInstance()->getEvents(); + if ($ssrf) { + $this->assertEvent('http://some.url', $traces, $ssrf); + } else { //Only lfi and non valid protocol + $this->assertEquals(0, count(AppsecStatus::getInstance()->getEvents())); + } } } From 0f40d6d72579678bb046ca3dc84c0c5a4fd1eaf0 Mon Sep 17 00:00:00 2001 From: Alejandro Estringana Ruiz Date: Tue, 28 Jan 2025 13:19:35 +0100 Subject: [PATCH 12/13] Amend issue with ref counting --- appsec/src/extension/ddappsec.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/appsec/src/extension/ddappsec.c b/appsec/src/extension/ddappsec.c index ab4e4ec27e..72a5d012b7 100644 --- a/appsec/src/extension/ddappsec.c +++ b/appsec/src/extension/ddappsec.c @@ -500,7 +500,6 @@ static PHP_FUNCTION(datadog_appsec_push_addresses) zval parameters_zv; ZVAL_ARR(¶meters_zv, addresses); - Z_TRY_ADDREF_P(¶meters_zv); dd_conn *conn = dd_helper_mgr_cur_conn(); if (conn == NULL) { @@ -510,7 +509,6 @@ static PHP_FUNCTION(datadog_appsec_push_addresses) } dd_result res = dd_request_exec(conn, ¶meters_zv, rasp); - zval_ptr_dtor(¶meters_zv); if (rasp) { clock_gettime(CLOCK_MONOTONIC_RAW, &end); From 335c02b6824cd38a63fd5beedd906c5c9678c7c7 Mon Sep 17 00:00:00 2001 From: Alejandro Estringana Ruiz Date: Wed, 29 Jan 2025 10:14:40 +0100 Subject: [PATCH 13/13] Refactor push_address function --- appsec/src/extension/ddappsec.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/appsec/src/extension/ddappsec.c b/appsec/src/extension/ddappsec.c index 72a5d012b7..8040ef00d3 100644 --- a/appsec/src/extension/ddappsec.c +++ b/appsec/src/extension/ddappsec.c @@ -487,28 +487,28 @@ static PHP_FUNCTION(datadog_appsec_push_addresses) return; } - HashTable *addresses = NULL; + zval *addresses = NULL; bool rasp = false; - if (zend_parse_parameters(ZEND_NUM_ARGS(), "h|b", &addresses, &rasp) == + if (zend_parse_parameters(ZEND_NUM_ARGS(), "z|b", &addresses, &rasp) == FAILURE) { RETURN_FALSE; } + if (Z_TYPE_P(addresses) != IS_ARRAY) { + RETURN_FALSE; + } + if (rasp && !get_global_DD_APPSEC_RASP_ENABLED()) { return; } - zval parameters_zv; - ZVAL_ARR(¶meters_zv, addresses); - dd_conn *conn = dd_helper_mgr_cur_conn(); if (conn == NULL) { - zval_ptr_dtor(¶meters_zv); mlog_g(dd_log_debug, "No connection; skipping push_addresses"); return; } - dd_result res = dd_request_exec(conn, ¶meters_zv, rasp); + dd_result res = dd_request_exec(conn, addresses, rasp); if (rasp) { clock_gettime(CLOCK_MONOTONIC_RAW, &end);