Prevent request_exec being issued between requests

appsec/cmake/ddtrace.cmake

@@ -20,22 +20,22 @@ add_custom_target(libdatadog_stamp
         "-DLIBDATADOG=${LIBDATADOG_DIR}"
         "-DSTAMP_FILE=${LIBDATADOG_STAMP_FILE}"
         -P "${CMAKE_CURRENT_LIST_DIR}/update_rust_stamp.cmake"
-    BYPRODUCT ${LIBDATADOG_STAMP_FILE}
+    BYPRODUCTS ${LIBDATADOG_STAMP_FILE}
 )
 
 if(${CMAKE_SYSTEM_NAME} STREQUAL "Linux")
 set(EXPORTS_FILE "${CMAKE_BINARY_DIR}/ddtrace_exports.version")
 add_custom_target(ddtrace_exports
     COMMAND bash -c "{ echo -e '{\\nglobal:'; sed 's/$/;/' '${CMAKE_SOURCE_DIR}'/../datadog.sym; echo -e 'local:\\n*;\\n};'; } > '${EXPORTS_FILE}'"
-    BYPRODUCT ${EXPORTS_FILE}
+    BYPRODUCTS ${EXPORTS_FILE}
     DEPENDS ${CMAKE_SOURCE_DIR}/../datadog.sym
     VERBATIM
 )
 elseif(APPLE)
 set(EXPORTS_FILE "${CMAKE_BINARY_DIR}/datadog_exports.sym")
 add_custom_target(ddtrace_exports
     COMMAND sed "s/^/_/" "${CMAKE_SOURCE_DIR}/../datadog.sym" > "${EXPORTS_FILE}"
-    BYPRODUCT ${EXPORTS_FILE}
+    BYPRODUCTS ${EXPORTS_FILE}
     DEPENDS ${CMAKE_SOURCE_DIR}/../datadog.sym
 )
 endif()

appsec/cmake/extension.cmake

@@ -68,7 +68,8 @@ if(ZAI_INCLUDE_DIRS)
 endif()
 target_link_libraries(extension PRIVATE zai)
 
-target_link_libraries(extension PRIVATE mpack PhpConfig zai rapidjson_appsec libxml2_static PCRE2::pcre2)
+find_package(Threads REQUIRED)
+target_link_libraries(extension PRIVATE mpack PhpConfig zai rapidjson_appsec libxml2_static PCRE2::pcre2 Threads::Threads)
 target_include_directories(extension PRIVATE ${EXT_ROOT_INCLUDES})
 
 # gnu unique prevents shared libraries from being unloaded from memory by dlclose

appsec/cmake/helper_rust.cmake

@@ -8,7 +8,7 @@ add_custom_target(helper-rust-stamp
         "-DHELPER_RUST_DIR=${HELPER_RUST_DIR}"
         "-DSTAMP_FILE=${HELPER_RUST_STAMP_FILE}"
         -P "${CMAKE_CURRENT_LIST_DIR}/update_helper_rust_stamp.cmake"
-    BYPRODUCT ${HELPER_RUST_STAMP_FILE}
+    BYPRODUCTS ${HELPER_RUST_STAMP_FILE}
 )
 
 set(CARGO_BUILD_CMD "cargo build")

appsec/src/extension/commands_helpers.c

@@ -179,7 +179,7 @@ static dd_result _dd_command_exec(dd_conn *nonnull conn,
                 "Response message for %.*s does not have the expected form",
                 NAME_L);
 
-            return dd_error;
+            return dd_network;
         }
         if (res != dd_success && res != dd_should_block &&
             res != dd_should_redirect && res != dd_should_record) {

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/ddappsec.h

@@ -38,6 +38,7 @@ ZEND_BEGIN_MODULE_GLOBALS(ddappsec)
     bool to_be_configured : 1;
 
     bool skip_rshutdown : 1;
+    // used to avoid a bailout during request shutdown
     bool during_request_shutdown : 1;
 ZEND_END_MODULE_GLOBALS(ddappsec)
 // clang-format on

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);
     }
 
@@ -265,7 +283,49 @@ static zend_array *nullable _do_request_begin(
     return spec;
 }
 
+static void _do_req_lifecycle_rshutdown(bool ignore_verdict, bool force);
+
 void dd_req_lifecycle_rshutdown(bool ignore_verdict, bool force)
+{
+    // temporarily remove the memory limit to avoid bailouts
+    // and as a safety net do a try-catch
+    __auto_type orig_mem_limit = PG(memory_limit);
+    zend_set_memory_limit((size_t)Z_L(-1) >> 1);
+
+    zend_try { _do_req_lifecycle_rshutdown(ignore_verdict, force); }
+    zend_catch
+    {
+        if (PG(last_error_message)) {
+#if PHP_VERSION_ID < 80000
+            const char *last_err = PG(last_error_message);
+#else
+            const char *last_err = ZSTR_VAL(PG(last_error_message));
+#endif
+            mlog_g(dd_log_error,
+                "Bailout in request shutdown; disconnecting from helper: %s",
+                last_err);
+        } else {
+            mlog_g(dd_log_error,
+                "Bailout in request shutdown; disconnecting from helper");
+        }
+        _reset_globals();
+        dd_helper_close_conn(); // note: not completely bailout-safe,
+                                // but should be fine with the raised mem limit
+    }
+    zend_end_try();
+
+#if PHP_VERSION_ID >= 80000 && PHP_VERSION_ID < 80100
+    // PHP 8.0: zend_set_memory_limit returns void
+    zend_set_memory_limit(orig_mem_limit);
+#else
+    if (zend_set_memory_limit(orig_mem_limit) == FAILURE) {
+        mlog(
+            dd_log_error, "Failed to restore memory limit in request shutdown");
+    }
+#endif
+}
+
+static void _do_req_lifecycle_rshutdown(bool ignore_verdict, bool force)
 {
     if (DDAPPSEC_G(enabled) == APPSEC_FULLY_DISABLED) {
         mlog_g(dd_log_debug, "Skipping all request shutdown actions because "
@@ -419,6 +479,7 @@ static zend_array *_do_request_finish_user_req(bool ignore_verdict,
     return spec;
 }
 
+// Should be bailout-safe -- means, in particular, no emalloc allocations
 static void _reset_globals(void)
 {
     _set_cur_span(NULL);
@@ -442,6 +503,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 +534,11 @@ static void _set_cur_span(zend_object *nullable span)
     }
 }
 
+bool dd_req_lifecycle_is_active(void)
+{
+    return _between_init_shutdown_msgs && DDAPPSEC_G(active);
+}
+
 zend_object *nullable dd_req_lifecycle_get_cur_span(void)
 {
     return _cur_req_span;
@@ -581,6 +648,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/src/extension/user_tracking.c

@@ -228,6 +228,12 @@ void dd_find_and_apply_verdict_for_user(zend_string *nullable user_id,
         return;
     }
 
+    if (!dd_req_lifecycle_is_active()) {
+        mlog_g(dd_log_info,
+            "Not running inside a tracked request; skipping user verdict");
+        return;
+    }
+
     dd_conn *conn = dd_helper_mgr_cur_conn();
     if (conn == NULL) {
         mlog(dd_log_debug, "No connection; unable to check user");

appsec/tests/extension/http_client_ip_generation_01.phpt

@@ -22,10 +22,10 @@ include __DIR__ . '/inc/mock_helper.php';
 
 $helper = Helper::createInitedRun([
     response_list(
-            response_request_init(['record', ['{"found":"attack"}','{"another":"attack"}']])
+            response_request_init([[['record', []]], ['{"found":"attack"}','{"another":"attack"}']])
    ),
    response_list(
-       response_request_shutdown(['record', ['{"yet another":"attack"}'], ["rshutdown_tag" => "rshutdown_value"], ["rshutdown_metric" => 2.1]])
+       response_request_shutdown([[['record', []]], ['{"yet another":"attack"}'], false, [], ["rshutdown_tag" => "rshutdown_value"], ["rshutdown_metric" => 2.1]])
     ),
 ], ['continuous' => true]);
 

appsec/tests/extension/rinit_fail_malformed_resp.phpt

@@ -8,20 +8,16 @@ use function datadog\appsec\testing\{rinit,rshutdown,backoff_status,is_connected
 
 include __DIR__ . '/inc/mock_helper.php';
 
-// respond correctly to client_init and request_shutdown, but not request_init
-$obj = new ArrayObject();
+// The malformed request_init response triggers dd_network, closing the connection.
+// rshutdown won't communicate with the helper.
 $helper = Helper::createInitedRun([
     response_list(
         response_request_init([['foo' => 'ok']])
     ),
-    response_list(
-        response_request_shutdown([[['ok', []]], $obj, $obj])
-    )
 ]);
 
 echo "rinit:\n";
 var_dump(rinit());
-// connection wasn't closed because this was no dd_network error. rshutdown will succeed
 echo "rshutdown:\n";
 var_dump(rshutdown());
 echo "is connected:\n";
@@ -37,10 +33,10 @@ bool(true)
 rshutdown:
 bool(true)
 is connected:
-bool(true)
+bool(false)
 array(2) {
   ["failed_count"]=>
-  int(0)
+  int(1)
   ["next_retry"]=>
-  float(0)
+  float(%f)
 }

appsec/tests/extension/user_tracking_block_from_set_user.phpt

@@ -2,6 +2,8 @@
 Block from DDTrace\set_user
 --INI--
 extension=ddtrace.so
+--ENV--
+DD_APPSEC_ENABLED=1
 --FILE--
 <?php
 use function datadog\appsec\testing\root_span_get_meta;

appsec/tests/extension/user_tracking_do_nothing_from_set_user.phpt

@@ -2,6 +2,8 @@
 Don't block or redirect from DDTrace\set_user
 --INI--
 extension=ddtrace.so
+--ENV--
+DD_APPSEC_ENABLED=1
 --FILE--
 <?php
 use function datadog\appsec\testing\root_span_get_meta;

appsec/tests/extension/user_tracking_redirect_from_set_user.phpt

@@ -2,6 +2,8 @@
 Redirect from DDTrace\set_user
 --INI--
 extension=ddtrace.so
+--ENV--
+DD_APPSEC_ENABLED=1
 --FILE--
 <?php
 use function datadog\appsec\testing\root_span_get_meta;

appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/docker/LogFile.groovy

@@ -0,0 +1,49 @@
+package com.datadog.appsec.php.docker
+
+/**
+ * A log file inside an {@link AppSecContainer}. Supports marking the current
+ * end-of-file position and then reading only what was appended since the mark,
+ * which is the usual way a test isolates the log output of a single action.
+ *
+ * <pre>
+ *   def lf = new LogFile(container, 'helper.log')
+ *   lf.markEndPos()
+ *   ... do something that writes to the log ...
+ *   lf.linesSinceMark().any { it.contains('boom') }
+ * </pre>
+ *
+ * A {@code name} without a leading {@code /} is resolved under {@link #LOG_DIR}.
+ */
+class LogFile {
+    static final String LOG_DIR = '/tmp/logs'
+
+    private final AppSecContainer container
+    final String path
+    private long markPos = 0
+
+    LogFile(AppSecContainer container, String name) {
+        this.container = container
+        this.path = name.startsWith('/') ? name : "${LOG_DIR}/${name}"
+    }
+
+    /** Current size of the log in bytes (0 if it does not exist yet). */
+    long size() {
+        container.execInContainer('bash', '-c', "wc -c < ${path} 2>/dev/null || echo 0".toString())
+                .stdout.trim() as long
+    }
+
+    /** Record the current end position; subsequent reads start from here. */
+    void markEndPos() {
+        markPos = size()
+    }
+
+    /** Raw text appended since the last {@link #markEndPos()}. */
+    String getTextSinceMark() {
+        container.execInContainer('bash', '-c', "tail -c +${markPos + 1} ${path}".toString()).stdout
+    }
+
+    /** Lines appended since the last {@link #markEndPos()}. */
+    List<String> getLinesSinceMark() {
+        getTextSinceMark().readLines()
+    }
+}