Prevent request_exec being issued between requests

appsec/src/extension/ddappsec.c

@@ -573,6 +573,12 @@ PHP_FUNCTION(datadog_appsec_push_addresses)
         RETURN_FALSE;
     }
 
+    if (!dd_req_lifecycle_is_active()) {
+        mlog_g(dd_log_info,
+            "Not running inside a tracked request; skipping push_addresses");
+        RETURN_FALSE;
+    }
+
     zval *addresses;
     zend_string *rasp_rule = NULL;
     zend_string *rule_variant = NULL;

appsec/src/extension/request_lifecycle.c

@@ -49,6 +49,7 @@ static THREAD_LOCAL_ON_ZTS zend_object *nullable _cur_req_span;
 static THREAD_LOCAL_ON_ZTS zend_array *nullable _superglob_equiv;
 static THREAD_LOCAL_ON_ZTS zend_string *nullable _client_ip;
 static THREAD_LOCAL_ON_ZTS zval _blocking_function;
+static THREAD_LOCAL_ON_ZTS bool _between_init_shutdown_msgs;
 static THREAD_LOCAL_ON_ZTS bool _shutdown_done_on_commit;
 static THREAD_LOCAL_ON_ZTS bool _empty_service_or_env;
 static THREAD_LOCAL_ON_ZTS bool _request_blocked;
@@ -186,6 +187,17 @@ static bool _rem_cfg_path_changed(bool ignore_empty /* called from rinit */)
     return true;
 }
 
+// Whether the result of dd_request_init means the helper has entered its
+// in-request (inner) loop and a request_shutdown will follow for this request.
+// The helper enters the inner loop for any WAF verdict (clean, record, block or
+// redirect); only a communication failure or other error leaves it in the outer
+// loop with no matching shutdown.
+static bool _req_init_started_request(dd_result res)
+{
+    return res == dd_success || res == dd_should_record ||
+           res == dd_should_block || res == dd_should_redirect;
+}
+
 static zend_array *nullable _do_request_begin(
     zval *nullable rbe_zv /* needs free */)
 {
@@ -231,12 +243,18 @@ static zend_array *nullable _do_request_begin(
         if (res == dd_success && DDAPPSEC_G(active)) {
             struct timespec start = dd_monotime_start();
             res = dd_request_init(conn, &req_info);
+            if (_req_init_started_request(res)) {
+                _between_init_shutdown_msgs = true;
+            }
             dd_duration_waf_ext_account(&start);
         }
     } else if (DDAPPSEC_G(active)) {
         // request_init
         struct timespec start = dd_monotime_start();
         res = dd_request_init(conn, &req_info);
+        if (_req_init_started_request(res)) {
+            _between_init_shutdown_msgs = true;
+        }
         dd_duration_waf_ext_account(&start);
     }
 
@@ -442,6 +460,7 @@ static void _reset_globals(void)
     }
     ZVAL_UNDEF(&_blocking_function);
 
+    _between_init_shutdown_msgs = false;
     _shutdown_done_on_commit = false;
     _request_blocked = false;
     dd_tags_rshutdown();
@@ -472,6 +491,8 @@ static void _set_cur_span(zend_object *nullable span)
     }
 }
 
+bool dd_req_lifecycle_is_active(void) { return _between_init_shutdown_msgs; }
+
 zend_object *nullable dd_req_lifecycle_get_cur_span(void)
 {
     return _cur_req_span;
@@ -581,6 +602,7 @@ static zend_array *nullable _response_commit(
     }
 
     _shutdown_done_on_commit = true;
+    _between_init_shutdown_msgs = false;
 
     return res;
 }

appsec/src/extension/request_lifecycle.h

@@ -22,5 +22,6 @@ enum request_stage {
 zend_array *nullable dd_req_lifecycle_abort(enum request_stage stage,
     dd_result result, struct block_params *nonnull block_params);
 
+bool dd_req_lifecycle_is_active(void);
 zend_object *nullable dd_req_lifecycle_get_cur_span(void);
 zend_string *nullable dd_req_lifecycle_get_client_ip(void);

appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RoadRunnerTests.groovy

@@ -3,16 +3,22 @@ package com.datadog.appsec.php.integration
 import com.datadog.appsec.php.docker.AppSecContainer
 import com.datadog.appsec.php.docker.FailOnUnmatchedTraces
 import com.datadog.appsec.php.docker.InspectContainerHelper
+import groovy.util.logging.Slf4j
+import org.junit.jupiter.api.Assumptions
 import org.junit.jupiter.api.BeforeAll
+import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.condition.EnabledIf
 import org.testcontainers.junit.jupiter.Container
 import org.testcontainers.junit.jupiter.Testcontainers
 
+import java.net.http.HttpResponse
+
 import static com.datadog.appsec.php.integration.TestParams.getPhpVersion
 import static com.datadog.appsec.php.integration.TestParams.getVariant
 import static com.datadog.appsec.php.integration.TestParams.phpVersionAtLeast
 
 @Testcontainers
+@Slf4j
 @EnabledIf('isExpectedVersion')
 class RoadRunnerTests implements WorkerStrategyTests {
     static boolean expectedVersion = phpVersionAtLeast('7.4') && !variant.contains('zts')
@@ -46,4 +52,42 @@ class RoadRunnerTests implements WorkerStrategyTests {
             Thread.sleep(500)
         }
     }
+
+    /**
+     * Regression test for the AppSec helper "unexpected command RequestExec" bug:
+     * RequestExec sent not between a request init and a request shutdown.
+     */
+    @Test
+    void 'no unexpected RequestExec in outer loop after post-respond fopen'() {
+        Assumptions.assumeTrue(System.getProperty('USE_HELPER_RUST') != null,
+                'This bug only manifests on the Rust helper (strict outer/inner loop state machine).')
+
+        long logOffset = (CONTAINER.execInContainer('bash', '-c',
+                'wc -c < /tmp/logs/helper.log').stdout.trim() as long)
+
+        // PostRespondLfiHandler sets a callback that calls fopen('../etc/passwd')
+        // after respond() returns. By that point, request_shutdown has been sent
+        // via the response_committed hook. If push_addresses() still reaches the
+        // helper (socket open, active=true), it sends RequestExec into the outer loop.
+        CONTAINER.traceFromRequest('/post-respond-lfi') { HttpResponse<InputStream> resp ->
+            assert resp.statusCode() == 200
+        }
+
+        // Follow-up request verifies the connection is still usable.
+        CONTAINER.traceFromRequest('/') { HttpResponse<InputStream> resp ->
+            assert resp.statusCode() == 200
+        }
+
+        String helperLog = CONTAINER.execInContainer('bash', '-c',
+                "tail -c +${logOffset + 1} /tmp/logs/helper.log").stdout
+
+        log.info("Helper log since offset:\n{}", helperLog)
+
+        assert !helperLog.contains('unexpected command RequestExec') :
+                "Helper received RequestExec in outer loop. " +
+                "Relevant log:\n" +
+                helperLog.readLines().findAll {
+                    it.contains('unexpected command') || it.contains('error in request loop')
+                }.join('\n')
+    }
 }

appsec/tests/integration/src/test/www/_handlers/src/PostRespondLfiHandler.php

@@ -0,0 +1,21 @@
+<?php
+
+namespace App;
+
+use Nyholm\Psr7\Response;
+use Psr\Http\Message\ResponseInterface;
+use Psr\Http\Message\ServerRequestInterface;
+
+class PostRespondLfiHandler
+{
+    public function handle(ServerRequestInterface $req): ResponseInterface
+    {
+        // Schedule fopen('../etc/passwd') to run after respond() returns.
+        // worker.php consumes this callback after respond(), which is after
+        // ddappsec's request_shutdown has been sent.
+        $GLOBALS['_rr_post_respond'] = static function () {
+            @fopen('../etc/passwd', 'r');
+        };
+        return new Response(200, ['Content-Type' => 'text/plain'], 'OK');
+    }
+}

appsec/tests/integration/src/test/www/roadrunner/worker.php

@@ -18,6 +18,7 @@
 $router->addRoute('/', new \App\HomePageHandler());
 $router->addRoute('/json', new \App\JsonHandler());
 $router->addRoute('/xml', new \App\XmlHandler());
+$router->addRoute('/post-respond-lfi', new \App\PostRespondLfiHandler());
 
 while ($req = $httpWorker->waitRequest()) {
     /** @var \Spiral\RoadRunner\Http\Request $req */
@@ -47,4 +48,9 @@
         );
     }
 //    \dd_trace_close_all_spans_and_flush();
+    // Post-respond hook: fires after request_shutdown has been sent inside respond().
+    if (isset($GLOBALS['_rr_post_respond'])) {
+        ($GLOBALS['_rr_post_respond'])();
+        unset($GLOBALS['_rr_post_respond']);
+    }
 }