Create new ranges for vulns to prevent GC issues

dd-java-agent/agent-iast/src/main/java/com/datadog/iast/model/Evidence.java

@@ -29,7 +29,7 @@ public Evidence(final String value) {
 
   public Evidence(@Nonnull final String value, @Nullable final Range[] ranges) {
     this.value = value;
-    this.ranges = ranges;
+    this.ranges = consolidate(ranges);
   }
 
   @Nonnull
@@ -62,6 +62,23 @@ public int hashCode() {
     return result;
   }
 
+  /**
+   * This method ensures that once a vulnerability has been found, all of it range names and values
+   * are strongly reachable preventing the GC from freeing them before the vul is reported. The
+   * newly created ranges have a lifespan equal to the target vulnerability.
+   */
+  @Nullable
+  private static Range[] consolidate(@Nullable final Range[] ranges) {
+    if (ranges == null || ranges.length == 0) {
+      return ranges;
+    }
+    final Range[] result = new Range[ranges.length];
+    for (int i = 0; i < ranges.length; i++) {
+      result[i] = ranges[i].consolidate();
+    }
+    return result;
+  }
+
   public static class Context {
 
     private final Map<String, Object> context;

dd-java-agent/agent-iast/src/main/java/com/datadog/iast/model/Range.java

@@ -82,4 +82,13 @@ public Range shift(final int offset) {
   public boolean isMarked(final int mark) {
     return (marks & mark) != NOT_MARKED;
   }
+
+  /** Creates a version of the range without weak references to be used in vulnerabilities */
+  public Range consolidate() {
+    if (!source.isReference()) {
+      return this;
+    }
+    return new Range(
+        start, length, new Source(source.getOrigin(), source.getName(), source.getValue()), marks);
+  }
 }

dd-java-agent/agent-iast/src/main/java/com/datadog/iast/model/Source.java

@@ -5,6 +5,7 @@
 import com.datadog.iast.model.json.SourceTypeString;
 import datadog.trace.api.iast.SourceTypes;
 import datadog.trace.api.iast.Taintable;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import java.lang.ref.Reference;
 import java.util.Objects;
 import java.util.StringJoiner;
@@ -51,14 +52,21 @@ public String getValue() {
     return asString(value);
   }
 
+  @SuppressFBWarnings(
+      value = "DM_STRING_CTOR",
+      justification = "New string instance requires constructor")
+  @SuppressWarnings("StringOperationCanBeSimplified")
   @Nullable
   private String asString(@Nullable final Object target) {
     Object value = target;
     if (value == PROPAGATION_PLACEHOLDER) {
       value = null;
     } else if (value instanceof Reference) {
       value = ((Reference<?>) value).get();
-      if (value == null) {
+      if (value != null) {
+        // avoid exposing internal weak reference to the outer world
+        value = new String(value.toString());
+      } else {
         value = GARBAGE_COLLECTED_REF;
         if (!gcReported) {
           gcReported = true;
@@ -96,10 +104,14 @@ public int hashCode() {
     return Objects.hash(origin, getName(), getValue());
   }
 
-  public Source attachValue(final CharSequence result) {
+  public Source attachValue(final Object newValue) {
     if (value != PROPAGATION_PLACEHOLDER) {
       return this;
     }
-    return new Source(origin, name, result);
+    return new Source(origin, name, newValue);
+  }
+
+  public boolean isReference() {
+    return name instanceof Reference || value instanceof Reference;
   }
 }

dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/PropagationModuleImpl.java

@@ -780,7 +780,7 @@ private static void internalTaint(
       ((Taintable) value).$$DD$setSource(source);
     } else {
       if (value instanceof CharSequence) {
-        source = source.attachValue((CharSequence) value);
+        source = attachSourceValue(source, (CharSequence) value);
         to.taint(value, Ranges.forCharSequence((CharSequence) value, source, mark));
       } else {
         to.taint(value, Ranges.forObject(source, mark));
@@ -796,7 +796,7 @@ private static void internalTaint(
     if (source == null) {
       return;
     }
-    source = source.attachValue(value);
+    source = attachSourceValue(source, value);
     to.taint(value, Ranges.forCharSequence(value, source, mark));
   }
 
@@ -865,6 +865,11 @@ private static Range[] markRanges(@Nonnull final Range[] ranges, final int mark)
     return result;
   }
 
+  private static Source attachSourceValue(final Source source, final CharSequence value) {
+    final Object newValue = sourceReference(value, value, true);
+    return newValue == null ? source : source.attachValue(newValue);
+  }
+
   private static Range[] attachSourceValue(
       @Nonnull final Range[] ranges, @Nonnull final CharSequence value) {
     // unbound sources can only occur when there's a single range in the array
@@ -873,7 +878,7 @@ private static Range[] attachSourceValue(
     }
     final Range range = ranges[0];
     final Source source = range.getSource();
-    final Source newSource = range.getSource().attachValue(value);
+    final Source newSource = attachSourceValue(source, value);
     return newSource == source
         ? ranges
         : Ranges.forCharSequence(value, newSource, range.getMarks());

dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/model/EvidenceTest.groovy

@@ -1,6 +1,14 @@
 package com.datadog.iast.model
 
+import com.datadog.iast.model.json.VulnerabilityEncoding
+import datadog.trace.api.iast.SourceTypes
 import datadog.trace.test.util.DDSpecification
+import groovy.json.JsonSlurper
+
+import java.lang.ref.Reference
+import java.lang.ref.WeakReference
+
+import static datadog.trace.api.iast.VulnerabilityMarks.NOT_MARKED
 
 class EvidenceTest extends DDSpecification {
 
@@ -27,4 +35,36 @@ class EvidenceTest extends DDSpecification {
     then:
     override
   }
+
+  void 'test that evidences are consolidated preventing the GC from clearing data'() {
+    given:
+    final String name = "Hello world!"
+    final Reference<?> ref = new WeakReference<>(name)
+
+    when:
+    final source = new Source(SourceTypes.REQUEST_PARAMETER_NAME, ref, "a random value")
+    final batch = vulnBatchForSource(source)
+
+    then:
+    source.getName() == name
+    final parsedBefore = new JsonSlurper().parseText(VulnerabilityEncoding.toJson(batch))
+    parsedBefore["sources"][0]["name"] == name
+
+    when:
+    ref.clear()
+
+    then:
+    source.getName() == Source.GARBAGE_COLLECTED_REF
+    final parsedAfter = new JsonSlurper().parseText(VulnerabilityEncoding.toJson(batch))
+    parsedAfter["sources"][0]["name"] == name
+  }
+
+  private static VulnerabilityBatch vulnBatchForSource(final Source source) {
+    final location = Location.forClassAndMethodAndLine(EvidenceTest.name, 'vulnBatchForSource', 69)
+    final evidence = new Evidence("test", [new Range(0, 4, source, NOT_MARKED)] as Range[])
+    final vuln = new Vulnerability(VulnerabilityType.INSECURE_COOKIE, location, evidence)
+    final batch = new VulnerabilityBatch()
+    batch.add(vuln)
+    return batch
+  }
 }