From af399f87d0fda8453501e50535fe1462af5826e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Gonz=C3=A1lez=20Garc=C3=ADa?= Date: Tue, 17 Mar 2026 13:46:38 +0100 Subject: [PATCH] Fix ObjectIntrospection exposing JDK internal toString() to the WAF (#10820) Fix ObjectIntrospection exposing JDK internal toString() to the WAF wip fix test for all jdks Avoid log classes new approach test new approach test change to .trie WIP Merge branch 'master' into alejandro.gonzalez/APPSEC-61693 Co-authored-by: devflow.devflow-routing-intake --- dd-java-agent/appsec/build.gradle | 9 ++ .../event/data/ObjectIntrospection.java | 13 +-- .../data/introspection_excluded_types.trie | 15 ++++ .../ObjectIntrospectionSpecification.groovy | 88 +++++++++++++++++-- 4 files changed, 114 insertions(+), 11 deletions(-) create mode 100644 dd-java-agent/appsec/src/main/resources/com/datadog/appsec/event/data/introspection_excluded_types.trie diff --git a/dd-java-agent/appsec/build.gradle b/dd-java-agent/appsec/build.gradle index c44a4f3311d..2d1fb0abffc 100644 --- a/dd-java-agent/appsec/build.gradle +++ b/dd-java-agent/appsec/build.gradle @@ -9,6 +9,7 @@ plugins { apply from: "$rootDir/gradle/java.gradle" apply from: "$rootDir/gradle/version.gradle" +apply from: "$rootDir/gradle/tries.gradle" dependencies { api libs.slf4j @@ -29,6 +30,14 @@ dependencies { testImplementation libs.jackson.databind } +tasks.named("compileJava", JavaCompile) { + dependsOn("generateClassNameTries") +} + +tasks.named("sourcesJar", Jar) { + dependsOn("generateClassNameTries") +} + tasks.named("shadowJar", ShadowJar) { exclude '**/*-dbgsym.zip' dependencies deps.excludeShared diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/event/data/ObjectIntrospection.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/event/data/ObjectIntrospection.java index 9169e2f2e08..92d19e0a848 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/event/data/ObjectIntrospection.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/event/data/ObjectIntrospection.java @@ -287,7 +287,7 @@ private static Object doConversion(Object obj, int depth, State state) { if (Modifier.isStatic(f.getModifiers())) { continue; } - if (f.getType().getName().equals("groovy.lang.MetaClass")) { + if (IntrospectionExcludedTypesTrie.apply(f.getType().getName()) >= 1) { continue; } String name = f.getName(); @@ -302,12 +302,13 @@ private static Object doConversion(Object obj, int depth, State state) { log.error("Unable to get field value", e); // TODO: Use invalid object } - } else { - // One of fields is inaccessible, might be it's Strongly Encapsulated Internal class - // consider it as integral object without introspection - // TODO: Use invalid object - return obj.toString(); } + // This field is inaccessible (Strongly Encapsulated Internal class on Java 9+). + // Skip it and continue with the remaining fields — other accessible fields on the + // same object may still contain useful data for WAF inspection. Do NOT call + // obj.toString() here: JDK internal toString() representations (e.g. + // "class java.lang.Object") can match legitimate WAF phrase_match rules and + // produce false positives (e.g. crs-944-130 java_code_injection). } } diff --git a/dd-java-agent/appsec/src/main/resources/com/datadog/appsec/event/data/introspection_excluded_types.trie b/dd-java-agent/appsec/src/main/resources/com/datadog/appsec/event/data/introspection_excluded_types.trie new file mode 100644 index 00000000000..8a5a564f594 --- /dev/null +++ b/dd-java-agent/appsec/src/main/resources/com/datadog/appsec/event/data/introspection_excluded_types.trie @@ -0,0 +1,15 @@ +# Generates 'IntrospectionExcludedTypesTrie.java' + +# Field types excluded from ObjectIntrospection to avoid deep/cyclic traversals +# that could trigger WAF false positives (e.g. crs-944-130 java_code_injection). +# 1 = exclude this field type + +# -------- Groovy -------- +1 groovy.lang.MetaClass + +# -------- Logging frameworks -------- +1 ch.qos.logback.* +1 java.util.logging.Logger +1 org.apache.commons.logging.Log +1 org.apache.logging.* +1 org.slf4j.* diff --git a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/event/data/ObjectIntrospectionSpecification.groovy b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/event/data/ObjectIntrospectionSpecification.groovy index f945244cf03..108cdb32229 100644 --- a/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/event/data/ObjectIntrospectionSpecification.groovy +++ b/dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/event/data/ObjectIntrospectionSpecification.groovy @@ -164,7 +164,9 @@ class ObjectIntrospectionSpecification extends DDSpecification { void 'max number of elements is honored'() { setup: def m = [:] - 128.times { m[it] = 'b' } + 128.times { + m[it] = 'b' + } when: def result1 = convert([['a'] * 255], ctx)[0] @@ -184,7 +186,9 @@ class ObjectIntrospectionSpecification extends DDSpecification { // Build a nested array 22 levels deep Object[] objArray = new Object[1] def p = objArray - 22.times { p = p[0] = new Object[1] } + 22.times { + p = p[0] = new Object[1] + } when: // Invoke conversion with context @@ -208,7 +212,9 @@ class ObjectIntrospectionSpecification extends DDSpecification { // Build a nested list 22 levels deep def list = [] def p = list - 22.times { p << []; p = p[0] } + 22.times { + p << []; p = p[0] + } when: // Invoke conversion with context @@ -232,7 +238,9 @@ class ObjectIntrospectionSpecification extends DDSpecification { // Build a nested map 22 levels deep under key 'a' def map = [:] def p = map - 22.times { p['a'] = [:]; p = p['a'] } + 22.times { + p['a'] = [:]; p = p['a'] + } when: // Invoke conversion with context @@ -413,7 +421,9 @@ class ObjectIntrospectionSpecification extends DDSpecification { setup: // Create deeply nested JSON final json = JsonOutput.toJson( - (1..(MAX_DEPTH + 1)).inject([:], { result, i -> [("child_$i".toString()) : result] }) + (1..(MAX_DEPTH + 1)).inject([:], { + result, i -> [("child_$i".toString()) : result] + }) ) when: @@ -478,6 +488,74 @@ class ObjectIntrospectionSpecification extends DDSpecification { MAPPER.readTree('"unicode: \\u0041"') || 'unicode: A' } + void 'logging framework fields are excluded from introspection'() { + given: '''Reproduces the false positive triggered by crs-944-130 (java_code_injection). + A request body DTO had an instance field of a concrete logging type. ObjectIntrospection + traversed the logger internals 19 levels deep (SLF4J → Log4jLogger → core.Logger → + privateConfig → loggerConfig → appenders → appenderArray → appender → manager → + layout → objectWriter → _config → _base → _typeFactory → _typeCache → _map → + invalid_key:9 → _elementType → _class), eventually calling + java.lang.Class.toString() = "class java.lang.object" which phrase-matched the WAF rule. + The field in the real trace was declared as org.slf4j.Logger (runtime: Log4jLogger), + but the trie also covers org.apache.logging.slf4j.Log4jLogger directly via + org.apache.logging.* in case the field is declared as the concrete class.''' + def input = new DtoWithLogger() + + when: + def result = convert(input, ctx) as Map + + then: + result['userId'] == 'user123' + result['payload'] == 'data' + !result.containsKey('logger') + } + + static class DtoWithLogger { + String userId = 'user123' + // Declared as the concrete Logback type (ch.qos.logback.classic.Logger) to mirror the + // real scenario where org.apache.logging.slf4j.Log4jLogger is the runtime instance of + // a field declared as the concrete logger class rather than the SLF4J interface. + // Both are covered by the trie (ch.qos.logback.* and org.apache.logging.*). + ch.qos.logback.classic.Logger logger = (ch.qos.logback.classic.Logger) org.slf4j.LoggerFactory.getLogger('test') + String payload = 'data' + } + + void 'objects with inaccessible JDK fields skip those fields rather than expose toString()'() { + given: '''An object with two fields: one normal, one holding a java.lang.ref.SoftReference. + java.lang.ref is NOT opened in the test JVM (only java.lang and java.util are), + so trySetAccessible() returns false for SoftReference's own fields on Java 9+. + + Bug: ObjectIntrospection fell back to obj.toString() when any field was inaccessible, + exposing internal JDK string representations to the WAF and discarding all other + accessible fields on the same object. For example, java.lang.Class.toString() produces + "class java.lang.Object" which matches WAF phrase_match rule crs-944-130 + (java_code_injection) — a false positive causing a CPU spike. + + Fix: skip inaccessible fields (continue) instead of aborting the whole object. + Accessible fields on the same object are still reported to the WAF.''' + def input = new WrapperWithSoftRef() + + when: + def result = convert(input, ctx) as Map + + then: + // The accessible field 'name' must be preserved regardless of JVM version + result['name'] == 'test' + // The inaccessible-field object must NOT expose toString() to the WAF. + // On JDK 8 and JDK 9-15 (--illegal-access=permit default), java.lang.ref fields are + // accessible so result['ref'] is a non-empty Map. On JDK 16+ strict module enforcement, + // result['ref'] is an empty Map [:] since all Reference fields are inaccessible. + // Before fix (any version where fields are inaccessible): result['ref'] is a String + // e.g. "java.lang.ref.SoftReference@..." — a false WAF positive. + // After fix: result['ref'] is always a Map, never a String. + result['ref'] instanceof Map + } + + static class WrapperWithSoftRef { + String name = 'test' + java.lang.ref.SoftReference ref = new java.lang.ref.SoftReference<>("test") + } + void 'iterable json objects'() { setup: final map = [name: 'This is just a test', list: [1, 2, 3, 4, 5]]