diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/model/Evidence.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/model/Evidence.java index c4dc17d27e5..ab6d91ecbb4 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/model/Evidence.java +++ b/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 context; diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/model/Range.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/model/Range.java index d3e2790c1d3..ce570588da8 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/model/Range.java +++ b/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); + } } diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/model/Source.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/model/Source.java index 502685cfc27..e4020973114 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/model/Source.java +++ b/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,6 +52,10 @@ 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; @@ -58,7 +63,10 @@ private String asString(@Nullable final Object target) { 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; } } diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/PropagationModuleImpl.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/PropagationModuleImpl.java index 59cb3556087..e0d1668d6fd 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/PropagationModuleImpl.java +++ b/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()); diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/model/EvidenceTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/model/EvidenceTest.groovy index 8cd5c9485e7..d21d573f6ec 100644 --- a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/model/EvidenceTest.groovy +++ b/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 + } }