Preserve original types before passing data to the WAF (#7220)

Author: smola

dd-java-agent/appsec/src/main/java/com/datadog/appsec/event/data/ObjectIntrospection.java

@@ -1,7 +1,11 @@
 package com.datadog.appsec.event.data;
 
 import datadog.trace.api.Platform;
-import java.lang.reflect.*;
+import java.lang.reflect.Array;
+import java.lang.reflect.Field;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
@@ -32,38 +36,89 @@ public final class ObjectIntrospection {
   private ObjectIntrospection() {}
 
   /**
-   * Converts arbitrary objects to strings, maps and lists, by using reflection. This serves two
-   * main purposes: - the objects can be inspected by the appsec subsystem and passed to the WAF. -
-   * By creating new containers and not transforming only immutable objects like strings, the new
-   * object can be safely manipulated by the appsec subsystem without worrying about modifications
-   * in other threads.
+   * Converts arbitrary objects compatible with ddwaf_object. Possible types in the result are:
+   *
+   * <ul>
+   *   <li>Null
+   *   <li>Strings
+   *   <li>Boolean
+   *   <li>Byte, Short, Integer, Long (will be serialized as int64)
+   *   <li>Float, Double (will be serialized as float64)
+   *   <li>Maps with string keys
+   *   <li>Lists
+   * </ul>
+   *
+   * This serves two purposes:
+   *
+   * <ul>
+   *   <li>The objects can be inspected by the appsec subsystem and passed to the WAF.
+   *   <li>>By creating new containers and not transforming only immutable objects like strings, the
+   *       new object can be safely manipulated by the appsec subsystem without worrying about
+   *       modifications in other threads.
+   * </ul>
    *
    * <p>Certain instance fields are excluded. Right now, this includes metaClass fields in Groovy
    * objects and this$0 fields in inner classes.
    *
+   * <p>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
    * @return the converted object
    */
   public static Object convert(Object obj) {
-    return guardedConversion(obj, 0, new int[] {MAX_ELEMENTS});
+    return guardedConversion(obj, 0, new State());
   }
 
-  private static Object guardedConversion(Object obj, int depth, int[] elemsLeft) {
+  private static class State {
+    int elemsLeft = MAX_ELEMENTS;
+    int invalidKeyId;
+  }
+
+  private static Object guardedConversion(Object obj, int depth, State state) {
     try {
-      return doConversion(obj, depth, elemsLeft);
+      return doConversion(obj, depth, state);
     } catch (Throwable t) {
-      return "<error: " + t.getMessage() + ">";
+      // TODO: Use invalid object
+      return "error:" + t.getMessage();
+    }
+  }
+
+  private static String keyConversion(Object key, State state) {
+    state.elemsLeft--;
+    if (state.elemsLeft <= 0) {
+      return null;
+    }
+    if (key == null) {
+      return "null";
+    }
+    if (key instanceof String) {
+      return (String) key;
     }
+    if (key instanceof Number
+        || key instanceof Boolean
+        || key instanceof Character
+        || key instanceof CharSequence) {
+      return key.toString();
+    }
+    return "invalid_key:" + (++state.invalidKeyId);
   }
 
-  private static Object doConversion(Object obj, int depth, int[] elemsLeft) {
-    elemsLeft[0]--;
-    if (elemsLeft[0] <= 0 || obj == null || depth > MAX_DEPTH) {
+  private static Object doConversion(Object obj, int depth, State state) {
+    state.elemsLeft--;
+    if (state.elemsLeft <= 0 || obj == null || depth > MAX_DEPTH) {
       return null;
     }
 
-    // char sequences / numbers
-    if (obj instanceof CharSequence || obj instanceof Number) {
+    // strings, booleans and numbers are preserved
+    if (obj instanceof String || obj instanceof Boolean || obj instanceof Number) {
+      return obj;
+    }
+
+    // char sequences are transformed just in case they are not immutable,
+    // single char sequences are transformed to strings for ddwaf compatibility.
+    if (obj instanceof CharSequence || obj instanceof Character) {
       return obj.toString();
     }
 
@@ -72,12 +127,12 @@ private static Object doConversion(Object obj, int depth, int[] elemsLeft) {
       Map<Object, Object> newMap = new HashMap<>((int) Math.ceil(((Map) obj).size() / .75));
       for (Map.Entry<?, ?> e : ((Map<?, ?>) obj).entrySet()) {
         Object key = e.getKey();
-        Object newKey = guardedConversion(e.getKey(), depth + 1, elemsLeft);
+        Object newKey = keyConversion(e.getKey(), state);
         if (newKey == null && key != null) {
           // probably we're out of elements anyway
           continue;
         }
-        newMap.put(newKey, guardedConversion(e.getValue(), depth + 1, elemsLeft));
+        newMap.put(newKey, guardedConversion(e.getValue(), depth + 1, state));
       }
       return newMap;
     }
@@ -91,10 +146,10 @@ private static Object doConversion(Object obj, int depth, int[] elemsLeft) {
         newList = new ArrayList<>();
       }
       for (Object o : ((Iterable<?>) obj)) {
-        if (elemsLeft[0] <= 0) {
+        if (state.elemsLeft <= 0) {
           break;
         }
-        newList.add(guardedConversion(o, depth + 1, elemsLeft));
+        newList.add(guardedConversion(o, depth + 1, state));
       }
       return newList;
     }
@@ -104,8 +159,8 @@ private static Object doConversion(Object obj, int depth, int[] elemsLeft) {
     if (clazz.isArray()) {
       int length = Array.getLength(obj);
       List<Object> newList = new ArrayList<>(length);
-      for (int i = 0; i < length && elemsLeft[0] > 0; i++) {
-        newList.add(guardedConversion(Array.get(obj, i), depth + 1, elemsLeft));
+      for (int i = 0; i < length && state.elemsLeft > 0; i++) {
+        newList.add(guardedConversion(Array.get(obj, i), depth + 1, state));
       }
       return newList;
     }
@@ -122,7 +177,7 @@ private static Object doConversion(Object obj, int depth, int[] elemsLeft) {
     outer:
     for (Field[] fields : allFields) {
       for (Field f : fields) {
-        if (elemsLeft[0] <= 0) {
+        if (state.elemsLeft <= 0) {
           break outer;
         }
         if (Modifier.isStatic(f.getModifiers())) {
@@ -132,19 +187,21 @@ private static Object doConversion(Object obj, int depth, int[] elemsLeft) {
           continue;
         }
         String name = f.getName();
-        if (name.equals("this$0")) {
+        if (ignoredFieldName(name)) {
           continue;
         }
 
         if (setAccessible(f)) {
           try {
-            newMap.put(f.getName(), guardedConversion(f.get(obj), depth + 1, elemsLeft));
+            newMap.put(f.getName(), guardedConversion(f.get(obj), depth + 1, state));
           } catch (IllegalAccessException e) {
             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();
         }
       }
@@ -153,6 +210,17 @@ private static Object doConversion(Object obj, int depth, int[] elemsLeft) {
     return newMap;
   }
 
+  private static boolean ignoredFieldName(final String name) {
+    switch (name) {
+      case "this$0":
+      case "memoizedHashCode":
+      case "memoizedSize":
+        return true;
+      default:
+        return false;
+    }
+  }
+
   /**
    * Try to make field accessible
    *

dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/event/data/ObjectIntrospectionSpecification.groovy

@@ -1,35 +1,68 @@
 package com.datadog.appsec.event.data
 
-
 import spock.lang.Specification
 
 import java.nio.CharBuffer
 
 import static com.datadog.appsec.event.data.ObjectIntrospection.convert
 
 class ObjectIntrospectionSpecification extends Specification {
-  void 'char sequences are converted to strings'() {
-    setup:
-    def charBuffer = CharBuffer.allocate(5)
-    charBuffer.put('hello')
-    charBuffer.position(0)
 
+  void 'null is preserved'() {
     expect:
-    convert('hello') == 'hello'
-    convert(charBuffer) == 'hello'
+    convert(null) == null
   }
 
-  void 'numbers are converted to strings'() {
-    expect:
-    convert(5L) == '5'
-    convert(0.33G) == '0.33'
+  void 'type #type is preserved'() {
+    when:
+    def result = convert(input)
+
+    then:
+    input.getClass() == type
+    result.getClass() == type
+    result == input
+
+    where:
+    input        | type
+    'hello'      | String
+    true         | Boolean
+    (byte) 1     | Byte
+    (short) 1    | Short
+    1            | Integer
+    1L           | Long
+    1.0F         | Float
+    (double) 1.0 | Double
+    1G           | BigInteger
+    1.0G         | BigDecimal
+  }
+
+  void 'type #type is converted to string'() {
+    when:
+    def result = convert(input)
+
+    then:
+    type.isAssignableFrom(input.getClass())
+    result instanceof String
+    result == output
+
+    where:
+    input                     | type       || output
+    (char) 'a'                | Character  || 'a'
+    createCharBuffer('hello') | CharBuffer || 'hello'
+  }
+
+  static CharBuffer createCharBuffer(String s) {
+    def charBuffer = CharBuffer.allocate(s.length())
+    charBuffer.put(s)
+    charBuffer.position(0)
+    charBuffer
   }
 
   void 'iterables are converted to lists'() {
     setup:
     def iter = new Iterable() {
-        @Delegate Iterable delegate = ['a', 'b']
-      }
+      @Delegate Iterable delegate = ['a', 'b']
+    }
 
     expect:
     convert(iter) instanceof List
@@ -40,19 +73,25 @@ class ObjectIntrospectionSpecification extends Specification {
   void 'maps are converted to hash maps'() {
     setup:
     def map = new Map() {
-        @Delegate Map map = [a: 'b']
-      }
+      @Delegate Map map = [a: 'b']
+    }
 
     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']
   }
 
   void 'arrays are converted into lists'() {
     expect:
-    convert([6, 'b'] as Object[]) == ['6', 'b']
-    convert([1, 2] as int[]) == ['1', '2']
+    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]
   }
 
   @SuppressWarnings('UnusedPrivateField')
@@ -62,6 +101,7 @@ class ObjectIntrospectionSpecification extends Specification {
     private String a = 'b'
     private List l = [1, 2]
   }
+
   class ClassToBeConvertedExt extends ClassToBeConverted {
     @SuppressWarnings('UnusedPrivateField')
     private String c = 'd'
@@ -70,7 +110,26 @@ 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 ClassToBeConvertedExt()) == [c: 'd', a: 'b', l: [1, 2]]
+  }
+
+  class ProtobufLikeClass {
+    String c = 'd'
+    int memoizedHashCode = 1
+    int memoizedSize = 2
+  }
+
+  void 'some field names are ignored'() {
+    expect:
+    convert(new ProtobufLikeClass()) instanceof HashMap
+    convert(new ProtobufLikeClass()) == [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']
   }
 
   void 'max number of elements is honored'() {
@@ -81,7 +140,7 @@ class ObjectIntrospectionSpecification extends Specification {
     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
+    convert(m).size() == 127 // +1 for the map, 2 for each entry (key and value)
   }
 
   void 'max depth is honored — array version'() {
@@ -98,7 +157,6 @@ class ObjectIntrospectionSpecification extends Specification {
     depth == 21 // after max depth we have nulls
   }
 
-
   void 'max depth is honored — list version'() {
     setup:
     def list = []
@@ -127,18 +185,18 @@ class ObjectIntrospectionSpecification extends Specification {
     depth == 21 // after max depth we have nulls
   }
 
-  def 'conversion of an element throws'() {
+  void 'conversion of an element throws'() {
     setup:
     def cs = new CharSequence() {
-        @Delegate String s = ''
+      @Delegate String s = ''
 
-        @Override
-        String toString() {
-          throw new RuntimeException('my exception')
-        }
+      @Override
+      String toString() {
+        throw new RuntimeException('my exception')
       }
+    }
 
     expect:
-    convert([cs]) == ['<error: my exception>']
+    convert([cs]) == ['error:my exception']
   }
 }