From e5d420f38750ed09d16205ada9cbef5c96c47fbb Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Wed, 14 May 2025 15:39:50 +0200 Subject: [PATCH 1/2] wip --- .../event/data/ObjectIntrospection.java | 85 ++++++-- .../datadog/appsec/gateway/GatewayBridge.java | 4 +- .../ObjectIntrospectionSpecification.groovy | 185 ++++++++++++++---- 3 files changed, 220 insertions(+), 54 deletions(-) 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 cf8e93b059f..d62e770c39f 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 @@ -1,6 +1,8 @@ package com.datadog.appsec.event.data; +import com.datadog.appsec.gateway.AppSecRequestContext; import datadog.trace.api.Platform; +import datadog.trace.api.telemetry.WafMetricCollector; import java.lang.reflect.Array; import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; @@ -16,6 +18,7 @@ public final class ObjectIntrospection { private static final int MAX_DEPTH = 20; private static final int MAX_ELEMENTS = 256; + private static final int MAX_STRING_LENGTH = 4096; private static final Logger log = LoggerFactory.getLogger(ObjectIntrospection.class); private static final Method trySetAccessible; @@ -35,6 +38,25 @@ public final class ObjectIntrospection { private ObjectIntrospection() {} + /** Functional interface to receive truncation flags. */ + @FunctionalInterface + public interface TruncationCallback { + + /** + * Called when the object is converted. + * + * @param requestContext the request context + * @param stringTooLong true if a string was truncated + * @param listMapTooLarge true if a list or map was truncated + * @param objectTooDeep true if an object was too deep + */ + void onTruncation( + AppSecRequestContext requestContext, + boolean stringTooLong, + boolean listMapTooLarge, + boolean objectTooDeep); + } + /** * Converts arbitrary objects compatible with ddwaf_object. Possible types in the result are: * @@ -60,20 +82,32 @@ private ObjectIntrospection() {} *

Certain instance fields are excluded. Right now, this includes metaClass fields in Groovy * objects and this$0 fields in inner classes. * - *

Only string values are preserved. Numbers or booleans are removed, since we do not expect - * rules to detect malicious payloads in these types. An exception to this are map keys, which are - * always converted to strings. - * * @param obj an arbitrary object + * @param requestContext the request context * @return the converted object */ - public static Object convert(Object obj) { - return guardedConversion(obj, 0, new State()); + public static Object convert(Object obj, AppSecRequestContext requestContext) { + State state = new State(requestContext); + Object converted = guardedConversion(obj, 0, state); + if (state.stringTooLong || state.listMapTooLarge || state.objectTooDeep) { + requestContext.setWafTruncated(); + WafMetricCollector.get() + .wafInputTruncated(state.stringTooLong, state.listMapTooLarge, state.objectTooDeep); + } + return converted; } private static class State { int elemsLeft = MAX_ELEMENTS; int invalidKeyId; + boolean objectTooDeep = false; + boolean listMapTooLarge = false; + boolean stringTooLong = false; + AppSecRequestContext requestContext; + + private State(AppSecRequestContext requestContext) { + this.requestContext = requestContext; + } } private static Object guardedConversion(Object obj, int depth, State state) { @@ -94,31 +128,48 @@ private static String keyConversion(Object key, State state) { return "null"; } if (key instanceof String) { - return (String) key; + return checkStringLength((String) key, state); } if (key instanceof Number || key instanceof Boolean || key instanceof Character || key instanceof CharSequence) { - return key.toString(); + return checkStringLength(key.toString(), state); } return "invalid_key:" + (++state.invalidKeyId); } private static Object doConversion(Object obj, int depth, State state) { + if (obj == null) { + return null; + } state.elemsLeft--; - if (state.elemsLeft <= 0 || obj == null || depth > MAX_DEPTH) { + if (state.elemsLeft <= 0) { + state.listMapTooLarge = true; return null; } - // strings, booleans and numbers are preserved - if (obj instanceof String || obj instanceof Boolean || obj instanceof Number) { + if (depth > MAX_DEPTH) { + state.objectTooDeep = true; + return null; + } + + // booleans and numbers are preserved + if (obj instanceof Boolean || obj instanceof Number) { return obj; } + // strings are preserved, but we need to check the length + if (obj instanceof String) { + return checkStringLength((String) obj, state); + } + // char sequences are transformed just in case they are not immutable, + if (obj instanceof CharSequence) { + return checkStringLength(obj.toString(), state); + } // single char sequences are transformed to strings for ddwaf compatibility. - if (obj instanceof CharSequence || obj instanceof Character) { + if (obj instanceof Character) { return obj.toString(); } @@ -147,6 +198,7 @@ private static Object doConversion(Object obj, int depth, State state) { } for (Object o : ((Iterable) obj)) { if (state.elemsLeft <= 0) { + state.listMapTooLarge = true; break; } newList.add(guardedConversion(o, depth + 1, state)); @@ -178,6 +230,7 @@ private static Object doConversion(Object obj, int depth, State state) { for (Field[] fields : allFields) { for (Field f : fields) { if (state.elemsLeft <= 0) { + state.listMapTooLarge = true; break outer; } if (Modifier.isStatic(f.getModifiers())) { @@ -239,4 +292,12 @@ private static boolean setAccessible(Field field) { return false; } } + + private static String checkStringLength(final String str, final State state) { + if (str.length() > MAX_STRING_LENGTH) { + state.stringTooLong = true; + return str.substring(0, MAX_STRING_LENGTH); + } + return str; + } } diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java index 7d29e6ee8b0..5864cc57d63 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java @@ -471,7 +471,7 @@ private Flow onGrpcServerRequestMessage(RequestContext ctx_, Object obj) { if (subInfo == null || subInfo.isEmpty()) { return NoopFlow.INSTANCE; } - Object convObj = ObjectIntrospection.convert(obj); + Object convObj = ObjectIntrospection.convert(obj, ctx); DataBundle bundle = new SingletonDataBundle<>(KnownAddresses.GRPC_SERVER_REQUEST_MESSAGE, convObj); try { @@ -573,7 +573,7 @@ private Flow onRequestBodyProcessed(RequestContext ctx_, Object obj) { } DataBundle bundle = new SingletonDataBundle<>( - KnownAddresses.REQUEST_BODY_OBJECT, ObjectIntrospection.convert(obj)); + KnownAddresses.REQUEST_BODY_OBJECT, ObjectIntrospection.convert(obj, ctx)); try { GatewayContext gwCtx = new GatewayContext(false); return producerService.publishDataEvent(subInfo, ctx, bundle, gwCtx); 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 64c65db410d..0910e636d7d 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 @@ -1,21 +1,39 @@ package com.datadog.appsec.event.data -import spock.lang.Specification +import com.datadog.appsec.gateway.AppSecRequestContext +import datadog.trace.api.telemetry.WafMetricCollector +import datadog.trace.test.util.DDSpecification +import spock.lang.Shared import java.nio.CharBuffer import static com.datadog.appsec.event.data.ObjectIntrospection.convert -class ObjectIntrospectionSpecification extends Specification { +class ObjectIntrospectionSpecification extends DDSpecification { + + @Shared + protected static final ORIGINAL_METRIC_COLLECTOR = WafMetricCollector.get() + + AppSecRequestContext ctx = Mock(AppSecRequestContext) + + WafMetricCollector wafMetricCollector = Mock(WafMetricCollector) + + void setup() { + WafMetricCollector.INSTANCE = wafMetricCollector + } + + void cleanup() { + WafMetricCollector.INSTANCE = ORIGINAL_METRIC_COLLECTOR + } void 'null is preserved'() { expect: - convert(null) == null + convert(null, ctx) == null } void 'type #type is preserved'() { when: - def result = convert(input) + def result = convert(input, ctx) then: input.getClass() == type @@ -38,7 +56,7 @@ class ObjectIntrospectionSpecification extends Specification { void 'type #type is converted to string'() { when: - def result = convert(input) + def result = convert(input, ctx) then: type.isAssignableFrom(input.getClass()) @@ -65,9 +83,9 @@ class ObjectIntrospectionSpecification extends Specification { } expect: - convert(iter) instanceof List - convert(iter) == ['a', 'b'] - convert(['a', 'b']) == ['a', 'b'] + convert(iter, ctx) instanceof List + convert(iter, ctx) == ['a', 'b'] + convert(['a', 'b'], ctx) == ['a', 'b'] } void 'maps are converted to hash maps'() { @@ -77,21 +95,21 @@ class ObjectIntrospectionSpecification extends Specification { } expect: - convert(map) instanceof HashMap - convert(map) == [a: 'b'] - convert([(6): 'b']) == ['6': 'b'] - convert([(null): 'b']) == ['null': 'b'] - convert([(true): 'b']) == ['true': 'b'] - convert([('a' as Character): 'b']) == ['a': 'b'] - convert([(createCharBuffer('a')): 'b']) == ['a': 'b'] + convert(map, ctx) instanceof HashMap + convert(map, ctx) == [a: 'b'] + convert([(6): 'b'], ctx) == ['6': 'b'] + convert([(null): 'b'], ctx) == ['null': 'b'] + convert([(true): 'b'], ctx) == ['true': 'b'] + convert([('a' as Character): 'b'], ctx) == ['a': 'b'] + convert([(createCharBuffer('a')): 'b'], ctx) == ['a': 'b'] } void 'arrays are converted into lists'() { expect: - convert([6, 'b'] as Object[]) == [6, 'b'] - convert([null, null] as Object[]) == [null, null] - convert([1, 2] as int[]) == [1 as int, 2 as int] - convert([1, 2] as byte[]) == [1 as byte, 2 as byte] + convert([6, 'b'] as Object[], ctx) == [6, 'b'] + convert([null, null] as Object[], ctx) == [null, null] + convert([1, 2] as int[], ctx) == [1 as int, 2 as int] + convert([1, 2] as byte[], ctx) == [1 as byte, 2 as byte] } @SuppressWarnings('UnusedPrivateField') @@ -109,8 +127,8 @@ class ObjectIntrospectionSpecification extends Specification { void 'other objects are converted into hash maps'() { expect: - convert(new ClassToBeConverted()) instanceof HashMap - convert(new ClassToBeConvertedExt()) == [c: 'd', a: 'b', l: [1, 2]] + convert(new ClassToBeConverted(), ctx) instanceof HashMap + convert(new ClassToBeConvertedExt(), ctx) == [c: 'd', a: 'b', l: [1, 2]] } class ProtobufLikeClass { @@ -121,15 +139,15 @@ class ObjectIntrospectionSpecification extends Specification { void 'some field names are ignored'() { expect: - convert(new ProtobufLikeClass()) instanceof HashMap - convert(new ProtobufLikeClass()) == [c: 'd'] + convert(new ProtobufLikeClass(), ctx) instanceof HashMap + convert(new ProtobufLikeClass(), ctx) == [c: 'd'] } void 'invalid keys are converted to special strings'() { expect: - convert(Collections.singletonMap(new ClassToBeConverted(), 'a')) == ['invalid_key:1': 'a'] - convert([new ClassToBeConverted(): 'a', new ClassToBeConverted(): 'b']) == ['invalid_key:1': 'a', 'invalid_key:2': 'b'] - convert(Collections.singletonMap([1, 2], 'a')) == ['invalid_key:1': 'a'] + convert(Collections.singletonMap(new ClassToBeConverted(), 'a'), ctx) == ['invalid_key:1': 'a'] + convert([new ClassToBeConverted(): 'a', new ClassToBeConverted(): 'b'], ctx) == ['invalid_key:1': 'a', 'invalid_key:2': 'b'] + convert(Collections.singletonMap([1, 2], 'a'), ctx) == ['invalid_key:1': 'a'] } void 'max number of elements is honored'() { @@ -137,52 +155,139 @@ class ObjectIntrospectionSpecification extends Specification { def m = [:] 128.times { m[it] = 'b' } - expect: - convert([['a'] * 255])[0].size() == 254 // +2 for the lists - convert([['a'] * 255 as String[]])[0].size() == 254 // +2 for the lists - convert(m).size() == 127 // +1 for the map, 2 for each entry (key and value) + when: + def result1 = convert([['a'] * 255], ctx)[0] + def result2 = convert([['a'] * 255 as String[]], ctx)[0] + def result3 = convert(m, ctx) + + then: + result1.size() == 254 // +2 for the lists + result2.size() == 254 // +2 for the lists + result3.size() == 127 // +1 for the map, 2 for each entry (key and value) + 2 * ctx.setWafTruncated() + 2 * wafMetricCollector.wafInputTruncated(false, true, false) + } void 'max depth is honored — array version'() { setup: + // 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] } - expect: + when: + // Invoke conversion with context + def result = convert(objArray, ctx) + + then: + // Traverse converted arrays to count actual depth int depth = 0 - for (p = convert(objArray); p != null; p = p[0]) { + for (p = result; p != null; p = p[0]) { depth++ } depth == 21 // after max depth we have nulls + + // Should record a truncation due to depth + 1 * ctx.setWafTruncated() + 1 * wafMetricCollector.wafInputTruncated(false, false, true) } void 'max depth is honored — list version'() { setup: + // Build a nested list 22 levels deep def list = [] def p = list - 22.times {p << []; p = p[0] } + 22.times { p << []; p = p[0] } - expect: + when: + // Invoke conversion with context + def result = convert(list, ctx) + + then: + // Traverse converted lists to count actual depth int depth = 0 - for (p = convert(list); p != null; p = p[0]) { + for (p = result; p != null; p = p[0]) { depth++ } depth == 21 // after max depth we have nulls + + // Should record a truncation due to depth + 1 * ctx.setWafTruncated() + 1 * wafMetricCollector.wafInputTruncated(false, false, true) } def 'max depth is honored — map version'() { setup: + // 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'] } - expect: + when: + // Invoke conversion with context + def result = convert(map, ctx) + + then: + // Traverse converted maps to count actual depth int depth = 0 - for (p = convert(map); p != null; p = p['a']) { + for (p = result; p != null; p = p['a']) { depth++ } depth == 21 // after max depth we have nulls + + // Should record a truncation due to depth + 1 * ctx.setWafTruncated() + 1 * wafMetricCollector.wafInputTruncated(false, false, true) + } + + void 'truncate long #typeName to 4096 chars and set truncation flag'() { + setup: + def longInput = rawInput + + when: + def converted = convert(longInput, ctx) + + then: + // Should always produce a String of exactly 4096 chars + converted instanceof String + converted.length() == 4096 + + // Should record a truncation due to string length + 1 * ctx.setWafTruncated() + 1 * wafMetricCollector.wafInputTruncated(true, false, false) + + where: + typeName | rawInput + 'String' | 'A' * 5000 + 'StringBuilder' | new StringBuilder('B' * 6000) + } + + void 'truncate long #typeName when used as map key to 4096 chars and set truncation flag'() { + setup: + // Build a map whose only key is a very long string/CharSequence + def longKey = rawKey + def inputMap = [(longKey): 'value'] + + when: + // convert returns Pair + def converted = convert(inputMap, ctx) + + then: + // Extract the single truncated key + def truncatedKey = converted.keySet().iterator().next() as String + + // Key must be exactly 4096 characters + truncatedKey.length() == 4096 + + 1 * ctx.setWafTruncated() + 1 * wafMetricCollector.wafInputTruncated(true, false, false) + + + where: + typeName | rawKey + 'String' | 'A' * 5000 + 'StringBuilder' | new StringBuilder('B' * 6000) } void 'conversion of an element throws'() { @@ -197,6 +302,6 @@ class ObjectIntrospectionSpecification extends Specification { } expect: - convert([cs]) == ['error:my exception'] + convert([cs], ctx) == ['error:my exception'] } } From 1f37e49e47e0daff88f6e48137278a1ef23bee7d Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Wed, 14 May 2025 15:48:03 +0200 Subject: [PATCH 2/2] wip --- .../event/data/ObjectIntrospection.java | 19 ------------------- 1 file changed, 19 deletions(-) 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 d62e770c39f..dbb235304f4 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 @@ -38,25 +38,6 @@ public final class ObjectIntrospection { private ObjectIntrospection() {} - /** Functional interface to receive truncation flags. */ - @FunctionalInterface - public interface TruncationCallback { - - /** - * Called when the object is converted. - * - * @param requestContext the request context - * @param stringTooLong true if a string was truncated - * @param listMapTooLarge true if a list or map was truncated - * @param objectTooDeep true if an object was too deep - */ - void onTruncation( - AppSecRequestContext requestContext, - boolean stringTooLong, - boolean listMapTooLarge, - boolean objectTooDeep); - } - /** * Converts arbitrary objects compatible with ddwaf_object. Possible types in the result are: *