diff --git a/appsec/helper-rust/src/telemetry/tel_aware_logger.rs b/appsec/helper-rust/src/telemetry/tel_aware_logger.rs index 866c1303db..dc0d6e7f24 100644 --- a/appsec/helper-rust/src/telemetry/tel_aware_logger.rs +++ b/appsec/helper-rust/src/telemetry/tel_aware_logger.rs @@ -100,7 +100,7 @@ fn submit_error_to_telemetry(record: &Record) { tags.add("module", module); } - let message = format!("{}", record.args()); + let message = redact_waf_strings(&format!("{}", record.args())).into_owned(); let location = if let (Some(module), Some(line)) = (record.module_path(), record.line()) { Cow::Owned(format!("{}:{}", module, line)) @@ -154,6 +154,53 @@ impl<'kvs> VisitSource<'kvs> for BacktraceExtractor { } } +/// Replace every `WafString("…")` span with `WafString("")`. +/// +/// The Debug impl of libddwaf `WafString` escapes `"` as `\"` and `\` as `\\` +/// inside the delimiters, so the closing `")` can only be an unescaped `"` +/// followed by `)`. Returns `Cow::Borrowed` when no replacement is needed. +pub(crate) fn redact_waf_strings(msg: &str) -> Cow<'_, str> { + const OPEN: &str = "WafString(\""; + const REPLACEMENT: &str = "WafString(\"\")"; + + if !msg.contains(OPEN) { + return Cow::Borrowed(msg); + } + + let mut out = String::with_capacity(msg.len()); + let mut rest = msg; + + while let Some(open_at) = rest.find(OPEN) { + let content = &rest[open_at + OPEN.len()..]; + let Some(end) = find_waf_string_end(content) else { + break; + }; + out.push_str(&rest[..open_at]); + out.push_str(REPLACEMENT); + rest = &content[end..]; + } + + out.push_str(rest); + Cow::Owned(out) +} + +/// Given the bytes right after the opening `WafString("`, return the offset +/// just past the closing `")`, treating `\\` and `\"` as escapes inside the +/// quoted content. Returns `None` if the string is unterminated. +fn find_waf_string_end(content: &str) -> Option { + let bytes = content.as_bytes(); + let mut i = 0; + while i < bytes.len() { + match bytes[i] { + b'\\' if i + 1 < bytes.len() => i += 2, // skip escaped char + b'"' if bytes.get(i + 1) == Some(&b')') => return Some(i + 2), + b'"' => return None, // bare `"` not followed by `)` — malformed + _ => i += 1, + } + } + None +} + fn extract_anyhow_backtrace(record: &Record) -> Option { let mut extractor = BacktraceExtractor { backtrace: None }; let _ = record.key_values().visit(&mut extractor); @@ -300,6 +347,61 @@ mod tests { assert!(extracted.is_none()); } + #[test] + fn test_redact_waf_strings_no_match_is_borrowed() { + let input = "no waf data here, just text"; + let out = redact_waf_strings(input); + assert!(matches!(out, Cow::Borrowed(_))); + assert_eq!(out, input); + } + + #[test] + fn test_redact_waf_strings_single() { + let input = r#"error: WafString("Mozilla/5.0 secret")"#; + let out = redact_waf_strings(input); + assert_eq!(out, r#"error: WafString("")"#); + } + + #[test] + fn test_redact_waf_strings_escaped_quote_not_treated_as_close() { + let input = r#"WafString("say \"hi\"")"#; + let out = redact_waf_strings(input); + assert_eq!(out, r#"WafString("")"#); + } + + #[test] + fn test_redact_waf_strings_escaped_backslash_before_close() { + let input = r#"WafString("trailing backslash\\")"#; + let out = redact_waf_strings(input); + assert_eq!(out, r#"WafString("")"#); + } + + #[test] + fn test_primary_delegate_receives_unredacted_error_message() { + let logs = Arc::new(Mutex::new(Vec::new())); + let primary = Box::new(TestLogger { logs: logs.clone() }); + let composite = TelemetryAwareLogger::new(primary); + + let record = log::Record::builder() + .args(format_args!( + r#"error in request loop: unexpected command WafString("ORIGINAL_SECRET")"# + )) + .level(Level::Error) + .build(); + + composite.log(&record); + + let captured = logs.lock().unwrap(); + assert_eq!(captured.len(), 1); + // Delegate must see the original, unredacted message. + assert!( + captured[0].contains("ORIGINAL_SECRET"), + "delegate did not receive the original message, got: {}", + captured[0] + ); + assert!(!captured[0].contains("")); + } + #[test] fn test_extract_anyhow_backtrace_with_other_keys() { use log::kv::{self, ToValue}; diff --git a/appsec/src/extension/ddappsec.c b/appsec/src/extension/ddappsec.c index 23987bfdfd..e839b60dd9 100644 --- a/appsec/src/extension/ddappsec.c +++ b/appsec/src/extension/ddappsec.c @@ -621,12 +621,16 @@ ZEND_ARG_TYPE_INFO(0, data, IS_ARRAY, 0) ZEND_END_ARG_INFO() // clang-format off -static const zend_function_entry testing_functions[] = { +// Available under either DD_APPSEC_TESTING or DD_APPSEC_TESTING_INVALID_COMMAND +static const zend_function_entry testing_request_control_functions[] = { ZEND_RAW_FENTRY(DD_TESTING_NS "rinit", PHP_FN(datadog_appsec_testing_rinit), void_ret_bool_arginfo, 0, NULL, NULL) ZEND_RAW_FENTRY(DD_TESTING_NS "rshutdown", PHP_FN(datadog_appsec_testing_rshutdown), void_ret_bool_arginfo, 0, NULL, NULL) + ZEND_RAW_FENTRY(DD_TESTING_NS "request_exec", PHP_FN(datadog_appsec_testing_request_exec), request_exec_arginfo, 0, NULL, NULL) + PHP_FE_END +}; +static const zend_function_entry testing_functions[] = { ZEND_RAW_FENTRY(DD_TESTING_NS "helper_mgr_acquire_conn", PHP_FN(datadog_appsec_testing_helper_mgr_acquire_conn), void_ret_bool_arginfo, 0, NULL, NULL) ZEND_RAW_FENTRY(DD_TESTING_NS "stop_for_debugger", PHP_FN(datadog_appsec_testing_stop_for_debugger), void_ret_bool_arginfo, 0, NULL, NULL) - ZEND_RAW_FENTRY(DD_TESTING_NS "request_exec", PHP_FN(datadog_appsec_testing_request_exec), request_exec_arginfo, 0, NULL, NULL) PHP_FE_END }; static const zend_function_entry testing_invalid_command_functions[] = { @@ -637,13 +641,18 @@ static const zend_function_entry testing_invalid_command_functions[] = { static void _register_testing_objects(void) { - if (get_global_DD_APPSEC_TESTING_INVALID_COMMAND()) { + bool invalid_command = get_global_DD_APPSEC_TESTING_INVALID_COMMAND(); + bool full_testing = get_global_DD_APPSEC_TESTING(); + + if (invalid_command) { dd_phpobj_reg_funcs(testing_invalid_command_functions); } - if (!get_global_DD_APPSEC_TESTING()) { - return; + if (invalid_command || full_testing) { + dd_phpobj_reg_funcs(testing_request_control_functions); } - dd_phpobj_reg_funcs(testing_functions); + if (full_testing) { + dd_phpobj_reg_funcs(testing_functions); + } } diff --git a/appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/docker/AppSecContainer.groovy b/appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/docker/AppSecContainer.groovy index 759073cb78..e5e36b1fb8 100644 --- a/appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/docker/AppSecContainer.groovy +++ b/appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/docker/AppSecContainer.groovy @@ -410,6 +410,16 @@ class AppSecContainer> extends GenericContain traceFromRequest(req, HttpResponse.BodyHandlers.ofInputStream(), doWithConn, ignoreOtherRequests) } + Trace traceFromRequest(String path, + HttpResponse.BodyHandler bodyHandler, + @ClosureParams(value = FromString, + options = 'java.net.http.HttpResponse') + Closure doWithResp = null, + boolean ignoreOtherRequests = false) { + HttpRequest req = buildReq(path).GET().build() + traceFromRequest(req, bodyHandler, doWithResp, ignoreOtherRequests) + } + @CompileStatic(TypeCheckingMode.SKIP) Trace traceFromRequest(HttpRequest req, HttpResponse.BodyHandler bodyHandler, diff --git a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/TelemetryTests.groovy b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/TelemetryTests.groovy index 99811deabb..7e04e211ad 100644 --- a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/TelemetryTests.groovy +++ b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/TelemetryTests.groovy @@ -1049,33 +1049,17 @@ class TelemetryTests { } /** - * RFC-1012: appsec.waf.requests must include the rate_limited boolean tag. - * The rate limiter must only be consulted when the WAF triggered (waf_keep=true), - * matching C++ semantics: `event.keep && limiter_.allow()`. Clean requests must - * not consume a limiter slot. - * - * This test verifies: - * 1. Clean requests do not consume the limiter slot. - * 2. The first attack request gets rate_limited:false (slot was preserved). - * 3. The second attack request gets rate_limited:true (slot now exhausted). + * Verifies that WafString(...) contents in helper error messages are redacted before + * being submitted to telemetry, while the local helper file log retains the original + * unredacted contents. * - * Only applies to the Rust helper. Ordered last because it restarts php-fpm - * with a different rate limit configuration. + * The scenario triggers `unexpected command {:?}` in the helper request loop by + * sending a request_exec when the helper is waiting for request_init. */ @Test - @Order(20) - void 'waf requests rate_limited tag is emitted'() { - Assumptions.assumeTrue(System.getProperty('USE_HELPER_RUST') != null, - 'rate_limited tag is only implemented on the Rust helper') - - // Restart php-fpm with a rate limit of 1 trace/sec. - org.testcontainers.containers.Container.ExecResult res = CONTAINER.execInContainer( - 'bash', '-c', - '''kill -9 `pgrep php-fpm`; - export DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS=1; - export DD_APPSEC_TRACE_RATE_LIMIT=1; - php-fpm -y /etc/php-fpm.conf -c /etc/php/php-rc.ini''') - assert res.exitCode == 0 + @Order(12) + void 'telemetry log redacts WafString contents'() { + Assumptions.assumeTrue(System.getProperty('USE_HELPER_RUST') != null) Supplier requestSup = CONTAINER.applyRemoteConfig(RC_TARGET, [ 'datadog/2/ASM_FEATURES/asm_features_activation/config': [ @@ -1083,65 +1067,53 @@ class TelemetryTests { ] ]) - // First request: starts helper. May or may not be covered by appsec depending on RC timing. - CONTAINER.traceFromRequest('/hello.php') { HttpResponse resp -> + // warm-up request to start helper and apply RC + Trace trace = CONTAINER.traceFromRequest('/hello.php') { HttpResponse resp -> assert resp.statusCode() == 200 } + assert trace.traceId != null assert requestSup.get() != null - // Several clean requests: these must NOT consume the rate-limiter slot. - // With correct behavior (matching C++), the limiter is only consulted when - // the WAF triggered, so clean requests leave the slot untouched. - for (int i = 0; i < 3; i++) { - CONTAINER.traceFromRequest('/hello.php') { HttpResponse resp -> - assert resp.statusCode() == 200 - } + // another covered request to ensure the connection is established + trace = CONTAINER.traceFromRequest('/hello.php') { HttpResponse resp -> + assert resp.statusCode() == 200 } + assert trace.traceId != null - // Send a burst of attack requests. With rate_limit=1, all requests within the - // same second share a single slot: the first gets rate_limited:false and the - // rest get rate_limited:true. Sending several increases the chance that at - // least two land within the same second even under CI timing variance. - HttpRequest attackReq = CONTAINER.buildReq('/hello.php') - .header('User-Agent', 'Arachni/v1').GET().build() - for (int i = 0; i < 5; i++) { - CONTAINER.traceFromRequest(attackReq, ofString()) { HttpResponse resp -> - assert resp.body().size() > 0 - } + // trigger the error path with a known sentinel value embedded as a WafString + CONTAINER.traceFromRequest('/send_request_exec_before_init.php', ofString()) { + HttpResponse resp -> assert resp.statusCode() == 200 } - TelemetryHelpers.Metric wafReqRateLimited - TelemetryHelpers.Metric wafReqNotRateLimited - - TelemetryHelpers.waitForMetrics(CONTAINER, 30) { List messages -> - def allSeries = messages.collectMany { it.series } - // Both must be attack requests (rule_triggered:true) to verify the limiter - // only fires for WAF-triggered requests, not clean ones. - wafReqRateLimited = allSeries.find { - it.name == 'waf.requests' && - 'rule_triggered:true' in it.tags && - 'rate_limited:true' in it.tags - } - wafReqNotRateLimited = allSeries.find { - it.name == 'waf.requests' && - 'rule_triggered:true' in it.tags && - 'rate_limited:false' in it.tags + def messages = TelemetryHelpers.waitForLogs(CONTAINER, 30) { List logs -> + def relevantLogs = logs.collectMany { + it.logs.findAll { + it.tags?.contains('log_type:helper::logged_error') && + it.message?.contains('error in request loop') + } } - wafReqRateLimited != null && wafReqNotRateLimited != null + !relevantLogs.empty + }.collectMany { it.logs } + + def errorLog = messages.find { + it.tags?.contains('log_type:helper::logged_error') && + it.message?.contains('error in request loop') } + assert errorLog != null : "Expected to find an 'error in request loop' telemetry log. " + + "All logs: ${messages.collect { [tags: it.tags, message: it.message] }}" - assert wafReqNotRateLimited != null - assert wafReqNotRateLimited.namespace == 'appsec' - assert wafReqNotRateLimited.type == 'count' - assert 'rule_triggered:true' in wafReqNotRateLimited.tags - assert 'rate_limited:false' in wafReqNotRateLimited.tags + // The telemetry log must NOT leak the original WafString payload + assert !errorLog.message.contains('APPSEC_REDACT_TEST_SENTINEL_XYZ') : + "Telemetry log leaked unredacted WafString contents: ${errorLog.message}" - assert wafReqRateLimited != null - assert wafReqRateLimited.namespace == 'appsec' - assert wafReqRateLimited.type == 'count' - assert wafReqRateLimited.points[0][1] >= 1.0 - assert 'rule_triggered:true' in wafReqRateLimited.tags - assert 'rate_limited:true' in wafReqRateLimited.tags + // And the WafString(...) span must have been replaced with the redacted marker + assert errorLog.message.contains('WafString("")') : + "Telemetry log is missing redacted WafString marker; got: ${errorLog.message}" + + // The local helper file log must still contain the original unredacted contents + def helperLog = CONTAINER.execInContainer('bash', '-c', 'cat /tmp/logs/helper.log').stdout + assert helperLog.contains('APPSEC_REDACT_TEST_SENTINEL_XYZ') : + "Expected helper.log to contain the original (unredacted) WafString contents" } /** @@ -1157,7 +1129,7 @@ class TelemetryTests { * Only applies to the Rust helper. */ @Test - @Order(12) + @Order(13) void 'rasp rule match has block tag'() { Assumptions.assumeTrue(System.getProperty('USE_HELPER_RUST') != null, 'block tag on rasp.rule.match is only implemented on the Rust helper') @@ -1249,6 +1221,103 @@ class TelemetryTests { } } + + /** + * RFC-1012: appsec.waf.requests must include the rate_limited boolean tag. + * The rate limiter must only be consulted when the WAF triggered (waf_keep=true), + * matching C++ semantics: `event.keep && limiter_.allow()`. Clean requests must + * not consume a limiter slot. + * + * This test verifies: + * 1. Clean requests do not consume the limiter slot. + * 2. The first attack request gets rate_limited:false (slot was preserved). + * 3. The second attack request gets rate_limited:true (slot now exhausted). + * + * Only applies to the Rust helper. Ordered last because it restarts php-fpm + * with a different rate limit configuration. + */ + @Test + @Order(20) + void 'waf requests rate_limited tag is emitted'() { + Assumptions.assumeTrue(System.getProperty('USE_HELPER_RUST') != null, + 'rate_limited tag is only implemented on the Rust helper') + + // Restart php-fpm with a rate limit of 1 trace/sec. + org.testcontainers.containers.Container.ExecResult res = CONTAINER.execInContainer( + 'bash', '-c', + '''kill -9 `pgrep php-fpm`; + export DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS=1; + export DD_APPSEC_TRACE_RATE_LIMIT=1; + php-fpm -y /etc/php-fpm.conf -c /etc/php/php-rc.ini''') + assert res.exitCode == 0 + + Supplier requestSup = CONTAINER.applyRemoteConfig(RC_TARGET, [ + 'datadog/2/ASM_FEATURES/asm_features_activation/config': [ + asm: [enabled: true] + ] + ]) + + // First request: starts helper. May or may not be covered by appsec depending on RC timing. + CONTAINER.traceFromRequest('/hello.php') { HttpResponse resp -> + assert resp.statusCode() == 200 + } + assert requestSup.get() != null + + // Several clean requests: these must NOT consume the rate-limiter slot. + // With correct behavior (matching C++), the limiter is only consulted when + // the WAF triggered, so clean requests leave the slot untouched. + for (int i = 0; i < 3; i++) { + CONTAINER.traceFromRequest('/hello.php') { HttpResponse resp -> + assert resp.statusCode() == 200 + } + } + + // Send a burst of attack requests. With rate_limit=1, all requests within the + // same second share a single slot: the first gets rate_limited:false and the + // rest get rate_limited:true. Sending several increases the chance that at + // least two land within the same second even under CI timing variance. + HttpRequest attackReq = CONTAINER.buildReq('/hello.php') + .header('User-Agent', 'Arachni/v1').GET().build() + for (int i = 0; i < 5; i++) { + CONTAINER.traceFromRequest(attackReq, ofString()) { HttpResponse resp -> + assert resp.body().size() > 0 + } + } + + TelemetryHelpers.Metric wafReqRateLimited + TelemetryHelpers.Metric wafReqNotRateLimited + + TelemetryHelpers.waitForMetrics(CONTAINER, 30) { List messages -> + def allSeries = messages.collectMany { it.series } + // Both must be attack requests (rule_triggered:true) to verify the limiter + // only fires for WAF-triggered requests, not clean ones. + wafReqRateLimited = allSeries.find { + it.name == 'waf.requests' && + 'rule_triggered:true' in it.tags && + 'rate_limited:true' in it.tags + } + wafReqNotRateLimited = allSeries.find { + it.name == 'waf.requests' && + 'rule_triggered:true' in it.tags && + 'rate_limited:false' in it.tags + } + wafReqRateLimited != null && wafReqNotRateLimited != null + } + + assert wafReqNotRateLimited != null + assert wafReqNotRateLimited.namespace == 'appsec' + assert wafReqNotRateLimited.type == 'count' + assert 'rule_triggered:true' in wafReqNotRateLimited.tags + assert 'rate_limited:false' in wafReqNotRateLimited.tags + + assert wafReqRateLimited != null + assert wafReqRateLimited.namespace == 'appsec' + assert wafReqRateLimited.type == 'count' + assert wafReqRateLimited.points[0][1] >= 1.0 + assert 'rule_triggered:true' in wafReqRateLimited.tags + assert 'rate_limited:true' in wafReqRateLimited.tags + } + @Test @Order(21) void 'waf config errors emitted with action init for bad init rules'() { diff --git a/appsec/tests/integration/src/test/www/base/public/send_request_exec_before_init.php b/appsec/tests/integration/src/test/www/base/public/send_request_exec_before_init.php new file mode 100644 index 0000000000..62a19d5ce6 --- /dev/null +++ b/appsec/tests/integration/src/test/www/base/public/send_request_exec_before_init.php @@ -0,0 +1,22 @@ + [ + 'x-test' => 'APPSEC_REDACT_TEST_SENTINEL_XYZ', + ], +]); + +header('Content-Type: text/plain'); +echo 'done';