Report rasp rule used on wrapped functions

DataDog · Jan 29, 2025 · 0627472 · 0627472
1 parent 6300ec5
commit 0627472
Show file tree

Hide file tree

Showing 18 changed files with 42 additions and 55 deletions.
diff --git a/appsec/src/extension/commands/request_exec.c b/appsec/src/extension/commands/request_exec.c
@@ -15,7 +15,7 @@
 
 struct ctx {
     struct req_info req_info; // dd_command_proc_resp_verd_span_data expect it
-    dd_rasp_rule rasp_rule;
+    zend_string *nullable rasp_rule;
     zval *nonnull data;
 };
 
@@ -30,7 +30,8 @@ static const dd_command_spec _spec = {
     .config_features_cb = dd_command_process_config_features_unexpected,
 };
 
-dd_result dd_request_exec(dd_conn *nonnull conn, zval *nonnull data, unsigned rasp_rule)
+dd_result dd_request_exec(
+    dd_conn *nonnull conn, zval *nonnull data, zend_string *nullable rasp_rule)
 {
     if (Z_TYPE_P(data) != IS_ARRAY) {
         mlog(dd_log_debug, "Invalid data provided to command request_exec, "
@@ -48,7 +49,7 @@ static dd_result _pack_command(mpack_writer_t *nonnull w, void *nonnull _ctx)
     assert(_ctx != NULL);
     struct ctx *ctx = _ctx;
 
-    mpack_write(w, ctx->rasp_rule);
+    dd_mpack_write_nullable_zstr(w, ctx->rasp_rule);
     dd_mpack_write_zval(w, ctx->data);
 
     return dd_success;

diff --git a/appsec/src/extension/commands/request_exec.h b/appsec/src/extension/commands/request_exec.h
@@ -9,4 +9,5 @@
 #include <SAPI.h>
 #include <php.h>
 
-dd_result dd_request_exec(dd_conn *nonnull conn, zval *nonnull data, unsigned rasp_rule);
+dd_result dd_request_exec(
+    dd_conn *nonnull conn, zval *nonnull data, zend_string *nullable rasp_rule);
diff --git a/appsec/src/extension/ddappsec.c b/appsec/src/extension/ddappsec.c
@@ -488,8 +488,8 @@ static PHP_FUNCTION(datadog_appsec_push_addresses)
     }
 
     zval *addresses = NULL;
-    long rasp_rule = dd_rasp_rule_none;
-    if (zend_parse_parameters(ZEND_NUM_ARGS(), "z|l", &addresses, &rasp_rule) ==
+    zend_string *rasp_rule = NULL;
+    if (zend_parse_parameters(ZEND_NUM_ARGS(), "z|S", &addresses, &rasp_rule) ==
         FAILURE) {
         RETURN_FALSE;
     }
@@ -498,11 +498,7 @@ static PHP_FUNCTION(datadog_appsec_push_addresses)
         RETURN_FALSE;
     }
 
-    if (rasp_rule != dd_rasp_rule_lfi && rasp_rule != dd_rasp_rule_ssrf) {
-        rasp_rule = dd_rasp_rule_none;
-    }
-
-    if (rasp_rule != dd_rasp_rule_none &&
+    if (rasp_rule && ZSTR_LEN(rasp_rule) > 0 &&
         !get_global_DD_APPSEC_RASP_ENABLED()) {
         return;
     }
@@ -515,7 +511,7 @@ static PHP_FUNCTION(datadog_appsec_push_addresses)
 
     dd_result res = dd_request_exec(conn, addresses, rasp_rule);
 
-    if (rasp_rule > dd_rasp_rule_none) {
+    if (rasp_rule && ZSTR_LEN(rasp_rule) > 0) {
         clock_gettime(CLOCK_MONOTONIC_RAW, &end);
         elapsed =
             ((int64_t)end.tv_sec - (int64_t)start.tv_sec) *
@@ -575,16 +571,6 @@ static void _register_testing_objects()
 {
     dd_phpobj_reg_funcs(functions);
 
-#    define _REG_RASP_CONST(php_name, value)                                    \
-        do {                                                                   \
-            char v[] = "datadog\\appsec\\rasp\\" php_name;       \
-            dd_phpobj_reg_long_const(                                          \
-                v, sizeof(v) - 1, value, CONST_CS | CONST_PERSISTENT);         \
-        } while (0)
-
-    _REG_RASP_CONST("LFI", dd_rasp_rule_lfi);
-    _REG_RASP_CONST("SSRF", dd_rasp_rule_ssrf);
-
     if (!get_global_DD_APPSEC_TESTING()) {
         return;
     }

diff --git a/appsec/src/extension/ddappsec.h b/appsec/src/extension/ddappsec.h
@@ -67,10 +67,4 @@ int dd_appsec_rshutdown(bool ignore_verdict);
 
 #define PHP_DDAPPSEC_EXTNAME "ddappsec"
 
-typedef enum {
-    dd_rasp_rule_none = 0,
-    dd_rasp_rule_lfi,
-    dd_rasp_rule_ssrf,
-} dd_rasp_rule;
-
 #endif // DDAPPSEC_H
diff --git a/appsec/tests/extension/actions_handling_01.phpt b/appsec/tests/extension/actions_handling_01.phpt
@@ -32,7 +32,7 @@ array(2) {
   [1]=>
   array(2) {
     [0]=>
-    int(0)
+    string(0) ""
     [1]=>
     array(1) {
       ["server.request.path_params"]=>

diff --git a/appsec/tests/extension/push_params_ok_01.phpt b/appsec/tests/extension/push_params_ok_01.phpt
@@ -32,7 +32,7 @@ array(2) {
   [1]=>
   array(2) {
     [0]=>
-    int(0)
+    string(0) ""
     [1]=>
     array(1) {
       ["server.request.path_params"]=>

diff --git a/appsec/tests/extension/push_params_ok_02.phpt b/appsec/tests/extension/push_params_ok_02.phpt
@@ -32,7 +32,7 @@ array(2) {
   [1]=>
   array(2) {
     [0]=>
-    int(0)
+    string(0) ""
     [1]=>
     array(1) {
       ["server.request.path_params"]=>

diff --git a/appsec/tests/extension/push_params_ok_03.phpt b/appsec/tests/extension/push_params_ok_03.phpt
@@ -32,7 +32,7 @@ array(2) {
   [1]=>
   array(2) {
     [0]=>
-    int(0)
+    string(0) ""
     [1]=>
     array(1) {
       ["server.request.path_params"]=>

diff --git a/appsec/tests/extension/push_params_ok_04.phpt b/appsec/tests/extension/push_params_ok_04.phpt
@@ -18,7 +18,7 @@ $helper = Helper::createInitedRun([
 ]);
 
 var_dump(rinit());
-push_addresses(["server.request.path_params" => 1234], \datadog\appsec\rasp\LFI);
+push_addresses(["server.request.path_params" => 1234], "lfi");
 var_dump(rshutdown());
 print_r(root_span_get_metrics());
 
@@ -40,7 +40,7 @@ array(2) {
   [1]=>
   array(2) {
     [0]=>
-    int(1)
+    string(3) "lfi"
     [1]=>
     array(1) {
       ["server.request.path_params"]=>

diff --git a/appsec/tests/extension/push_params_ok_05.phpt b/appsec/tests/extension/push_params_ok_05.phpt
@@ -18,7 +18,7 @@ $helper = Helper::createInitedRun([
 ]);
 
 var_dump(rinit());
-push_addresses(["server.request.path_params" => 1234], \datadog\appsec\rasp\LFI);
+push_addresses(["server.request.path_params" => 1234], "lfi");
 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
@@ -16,7 +16,7 @@ $helper = Helper::createInitedRun([
 ]);
 
 var_dump(rinit());
-push_addresses(["server.request.path_params" => 1234], \datadog\appsec\rasp\LFI);
+push_addresses(["server.request.path_params" => 1234], "lfi");
 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
@@ -32,7 +32,7 @@ array(2) {
   [1]=>
   array(2) {
     [0]=>
-    int(0)
+    string(0) ""
     [1]=>
     array(2) {
       ["server.request.path_params"]=>

diff --git a/appsec/tests/extension/push_params_ok_08.phpt b/appsec/tests/extension/push_params_ok_08.phpt
@@ -18,7 +18,7 @@ $helper = Helper::createInitedRun([
 ]);
 
 var_dump(rinit());
-push_addresses(["server.request.path_params" => ["some" => "params", "more" => "parameters"]], \datadog\appsec\rasp\LFI);
+push_addresses(["server.request.path_params" => ["some" => "params", "more" => "parameters"]], "lfi");
 var_dump(rshutdown());
 
 var_dump($helper->get_command("request_exec"));
@@ -33,7 +33,7 @@ array(2) {
   [1]=>
   array(2) {
     [0]=>
-    int(1)
+    string(3) "lfi"
     [1]=>
     array(1) {
       ["server.request.path_params"]=>

diff --git a/appsec/tests/extension/push_params_ok_09.phpt b/appsec/tests/extension/push_params_ok_09.phpt
@@ -18,7 +18,7 @@ $helper = Helper::createInitedRun([
 ]);
 
 var_dump(rinit());
-push_addresses(["server.request.path_params" => ["some" => "params", "more" => "parameters"]], \datadog\appsec\rasp\SSRF);
+push_addresses(["server.request.path_params" => ["some" => "params", "more" => "parameters"]], "ssrf");
 var_dump(rshutdown());
 
 var_dump($helper->get_command("request_exec"));
@@ -33,7 +33,7 @@ array(2) {
   [1]=>
   array(2) {
     [0]=>
-    int(2)
+    string(4) "ssrf"
     [1]=>
     array(1) {
       ["server.request.path_params"]=>

diff --git a/appsec/tests/extension/request_exec.phpt b/appsec/tests/extension/request_exec.phpt
@@ -47,7 +47,7 @@ array(2) {
   [1]=>
   array(2) {
     [0]=>
-    int(0)
+    string(0) ""
     [1]=>
     array(3) {
       ["key 01"]=>

diff --git a/src/DDTrace/Integrations/Filesystem/FilesystemIntegration.php b/src/DDTrace/Integrations/Filesystem/FilesystemIntegration.php
@@ -60,21 +60,24 @@ private static function preHook($variant)
            $protocol = isset($protocol[1]) ? $protocol[1]: "";
 
            $addresses = [];
+           $rule = "";
 
            if (empty($protocol) || $protocol === 'file') {
                 $addresses["server.io.fs.file"] = $hook->args[0];
+                $rule = "lfi";
            }
 
            if (in_array($variant, ['file_get_contents', 'fopen']) &&
            in_array($protocol, ['http', 'https', 'ftp', 'ftps'])) {
                 $addresses["server.io.net.url"] = $hook->args[0];
+                $rule = "ssrf";
            }
 
             if (empty($addresses)) {
                 return;
             }
 
-            \datadog\appsec\push_addresses($addresses, true);
+            \datadog\appsec\push_addresses($addresses, $rule);
         };
     }
 

diff --git a/tests/Appsec/Mock.php b/tests/Appsec/Mock.php
@@ -210,10 +210,10 @@ function track_user_signup_event($userId, $metadata)
      * This function is exposed by appsec but here we are mocking it for tests
      * @param array $params
      */
-    function push_addresses($addresses, $rasp = false) {
+    function push_addresses($addresses, $rasp = "") {
         if(!appsecMockEnabled()) {
            return;
         }
-        AppsecStatus::getInstance()->addEvent(['rasp' => $rasp, $addresses], 'push_addresses');
+        AppsecStatus::getInstance()->addEvent(['rasp_rule' => $rasp, $addresses], 'push_addresses');
     }
 }
diff --git a/tests/Integrations/Filesystem/FilesystemTest.php b/tests/Integrations/Filesystem/FilesystemTest.php
@@ -22,15 +22,15 @@ protected static function getEnvs()
             ]);
         }
 
-    protected function assertEvent(string $value, $traces, $ssrf = false)
+    protected function assertEvent(string $value, $traces, string $rasp_rule)
     {
        $events = AppsecStatus::getInstance()->getEvents();
        $this->assertEquals(1, count($events));
        $this->assertEquals(1, count($events[0][0]));
-       $key = !$ssrf ? "server.io.fs.file" : "server.io.net.url";
+       $key = $rasp_rule == "lfi" ? "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']);
+       $this->assertEquals($rasp_rule, $events[0]['rasp_rule']);
     }
 
     public function ssrfProtocols()
@@ -54,7 +54,7 @@ public function testSsrfProtocols($protocol)
             TestCase::assertSame('OK', $response);
         });
 
-       $this->assertEvent($url, $traces, true);
+       $this->assertEvent($url, $traces, "ssrf");
     }
 
     public function testInvalidProtocol()
@@ -80,19 +80,21 @@ public function wrappedFunctions()
     }
 
     /**
+    * With no protocol all default to files system wrapper and therefore lfi
     * @dataProvider wrappedFunctions
     */
     public function testNoProtocol($targetFunction, $ssrf)
     {
         $traces = $this->tracesFromWebRequest(function () use($targetFunction) {
             $response = $this->call(GetSpec::create('Root', '/?function='.$targetFunction.'&path=./somefile'));
-
+            //The str_replace replace is because the content of the file is sent to the output on some functions only
             TestCase::assertSame('OK', str_replace('some content', '', $response));
         });
-       $this->assertEvent('./somefile', $traces, false);
+       $this->assertEvent('./somefile', $traces, "lfi");
     }
 
     /**
+    * With file protocol always use LFI
     * @dataProvider wrappedFunctions
     */
     public function testWithFileProtocol($targetFunction, $ssrf)
@@ -101,10 +103,11 @@ public function testWithFileProtocol($targetFunction, $ssrf)
             $response = $this->call(GetSpec::create('Root', '/?function='.$targetFunction.'&path=file://somefile'));
             TestCase::assertSame('OK', $response);
         });
-       $this->assertEvent('file://somefile', $traces, false);
+       $this->assertEvent('file://somefile', $traces, "lfi");
     }
 
     /**
+    * HTTP protocol is valid for SSRF
     * @dataProvider wrappedFunctions
     */
     public function testWithHttpProtocol($targetFunction, $ssrf)
@@ -113,10 +116,9 @@ public function testWithHttpProtocol($targetFunction, $ssrf)
             $response = $this->call(GetSpec::create('Root', '/?function='.$targetFunction.'&path=http://some.url'));
             TestCase::assertSame('OK', $response);
         });
-        $events = AppsecStatus::getInstance()->getEvents();
         if ($ssrf) {
-            $this->assertEvent('http://some.url', $traces, $ssrf);
-        } else { //Only lfi and non valid protocol
+            $this->assertEvent('http://some.url', $traces, "ssrf");
+        } else {
             $this->assertEquals(0, count(AppsecStatus::getInstance()->getEvents()));
         }
     }