From 4d7418998aad6ba55975bc2922ef17eb540d253e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Andr=C3=A9=20dos=20Santos=20Lopes?= Date: Thu, 28 May 2026 21:53:01 +0100 Subject: [PATCH] Prevent request_exec being issued between requests --- appsec/src/extension/ddappsec.c | 6 +++ appsec/src/extension/request_lifecycle.c | 22 ++++++++++ appsec/src/extension/request_lifecycle.h | 1 + .../php/integration/RoadRunnerTests.groovy | 44 +++++++++++++++++++ .../_handlers/src/PostRespondLfiHandler.php | 21 +++++++++ .../src/test/www/roadrunner/worker.php | 6 +++ 6 files changed, 100 insertions(+) create mode 100644 appsec/tests/integration/src/test/www/_handlers/src/PostRespondLfiHandler.php diff --git a/appsec/src/extension/ddappsec.c b/appsec/src/extension/ddappsec.c index e839b60dd95..9c2ef9c2f42 100644 --- a/appsec/src/extension/ddappsec.c +++ b/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; diff --git a/appsec/src/extension/request_lifecycle.c b/appsec/src/extension/request_lifecycle.c index 0e14c3de991..4d9cb27088f 100644 --- a/appsec/src/extension/request_lifecycle.c +++ b/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; } diff --git a/appsec/src/extension/request_lifecycle.h b/appsec/src/extension/request_lifecycle.h index 55262b6509e..6a01d27b576 100644 --- a/appsec/src/extension/request_lifecycle.h +++ b/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); diff --git a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RoadRunnerTests.groovy b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RoadRunnerTests.groovy index d8d27f00eef..51ade591d40 100644 --- a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RoadRunnerTests.groovy +++ b/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 resp -> + assert resp.statusCode() == 200 + } + + // Follow-up request verifies the connection is still usable. + CONTAINER.traceFromRequest('/') { HttpResponse 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') + } } diff --git a/appsec/tests/integration/src/test/www/_handlers/src/PostRespondLfiHandler.php b/appsec/tests/integration/src/test/www/_handlers/src/PostRespondLfiHandler.php new file mode 100644 index 00000000000..fd692b889f4 --- /dev/null +++ b/appsec/tests/integration/src/test/www/_handlers/src/PostRespondLfiHandler.php @@ -0,0 +1,21 @@ + 'text/plain'], 'OK'); + } +} diff --git a/appsec/tests/integration/src/test/www/roadrunner/worker.php b/appsec/tests/integration/src/test/www/roadrunner/worker.php index de9b32c3c2c..36ea37179b0 100644 --- a/appsec/tests/integration/src/test/www/roadrunner/worker.php +++ b/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']); + } }