Skip to content

Match tainted objects with sources when checking unbounded vulnerabilities #6122

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 11, 2023
Merged

Match tainted objects with sources when checking unbounded vulnerabilities #6122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/SinkModuleBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import com.datadog.iast.model.Evidence;
import com.datadog.iast.model.Location;
import com.datadog.iast.model.Range;
import com.datadog.iast.model.Source;
import com.datadog.iast.model.Vulnerability;
import com.datadog.iast.model.VulnerabilityType;
import com.datadog.iast.model.VulnerabilityType.InjectionType;
Expand Down Expand Up @@ -57,7 +58,7 @@ protected SinkModuleBase(@Nonnull final Dependencies dependencies) {
if (!overheadController.consumeQuota(Operations.REPORT_VULNERABILITY, span)) {
return null;
}
final Evidence result = new Evidence(value.toString(), ranges);
final Evidence result = buildEvidence(value, ranges);
report(span, type, result);
return result;
}
Expand Down Expand Up @@ -119,7 +120,7 @@ protected SinkModuleBase(@Nonnull final Dependencies dependencies) {
return null;
}

final Evidence result = new Evidence(evidence, notMarkedRanges);
final Evidence result = buildEvidence(evidence, notMarkedRanges);
report(span, type, result);
return result;
}
Expand Down Expand Up @@ -161,7 +162,7 @@ protected SinkModuleBase(@Nonnull final Dependencies dependencies) {
if (notMarkedRanges == null || notMarkedRanges.length == 0) {
return null;
}
final Evidence result = new Evidence(evidence.toString(), notMarkedRanges);
final Evidence result = buildEvidence(evidence, notMarkedRanges);
report(span, type, result);
return result;
}
Expand All @@ -179,6 +180,24 @@ protected StackTraceElement getCurrentStackTrace() {
return stackWalker.walk(SinkModuleBase::findValidPackageForVulnerability);
}

protected Evidence buildEvidence(final Object value, final Range[] ranges) {
final Range unbound = Ranges.findUnbound(ranges);
if (unbound != null) {
final Source source = unbound.getSource();
if (source != null && source.getValue() != null) {
final String sourceValue = source.getValue();
final String evidenceValue = value.toString();
final int start = evidenceValue.indexOf(sourceValue);
if (start >= 0) {
return new Evidence(
evidenceValue,
new Range[] {new Range(start, sourceValue.length(), source, unbound.getMarks())});
}
}
}
return new Evidence(value instanceof String ? (String) value : value.toString(), ranges);
}

static StackTraceElement findValidPackageForVulnerability(
@Nonnull final Stream<StackTraceElement> stream) {
final StackTraceElement[] first = new StackTraceElement[1];
Expand Down
9 changes: 9 additions & 0 deletions dd-java-agent/agent-iast/src/main/java/com/datadog/iast/taint/Ranges.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,15 @@ public static Range[] forObject(final @Nonnull Source source, final int mark) {
return new Range[] {new Range(0, Integer.MAX_VALUE, source, mark)};
}

@Nullable
public static Range findUnbound(@Nonnull final Range[] ranges) {
if (ranges.length != 1) {
return null;
}
final Range range = ranges[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we iterate over all ranges instead of checking only the first one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay in the response, this PR only covers the case of unbounded tainted values that are created via:

  public static Range[] forObject(final @Nonnull Source source, final int mark) {
    return new Range[] {new Range(0, Integer.MAX_VALUE, source, mark)};
  }

There should not be other cases unless we have a bug of course.

return range.getStart() == 0 && range.getLength() == Integer.MAX_VALUE ? range : null;
}

public static void copyShift(
final @Nonnull Range[] src, final @Nonnull Range[] dst, final int dstPos, final int shift) {
copyShift(src, dst, dstPos, shift, src.length);
Expand Down
57 changes: 56 additions & 1 deletion dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/AbstractSinkModuleTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,37 @@
package com.datadog.iast.sink

import com.datadog.iast.IastModuleImplTestBase
import com.datadog.iast.IastRequestContext
import com.datadog.iast.model.Source
import com.datadog.iast.overhead.Operations
import com.datadog.iast.propagation.PropagationModuleImpl
import com.datadog.iast.taint.Ranges
import datadog.trace.api.gateway.RequestContext
import datadog.trace.api.gateway.RequestContextSlot
import datadog.trace.bootstrap.instrumentation.api.AgentSpan

class AbstractSinkModuleTest extends IastModuleImplTestBase {
import static com.datadog.iast.model.VulnerabilityType.SSRF
import static datadog.trace.api.iast.SourceTypes.REQUEST_PARAMETER_VALUE

class AbstractSinkModuleTest extends IastModuleImplTestBase {

final StackTraceElement ignoredPackageClassElement = element("org.springframework.Ignored")
final StackTraceElement notIgnoredPackageClassElement = element("datadog.smoketest.NotIgnored")
final StackTraceElement notInIastExclusionTrie = element("not.in.iast.exclusion.Class")

private IastRequestContext ctx
private AgentSpan span

void setup() {
ctx = new IastRequestContext()
final reqCtx = Mock(RequestContext) {
getData(RequestContextSlot.IAST) >> ctx
}
span = Mock(AgentSpan) {
getRequestContext() >> reqCtx
}
tracer.activeSpan() >> span
}

void 'filter ignored package element from stack'() {

Expand Down Expand Up @@ -43,6 +67,37 @@ class AbstractSinkModuleTest extends IastModuleImplTestBase {
result == expected
}

void 'test reporting evidence on objects'() {
given:
overheadController.consumeQuota(Operations.REPORT_VULNERABILITY, span) >> true
final sink = new SinkModuleBase(dependencies) {}
final propagation = new PropagationModuleImpl()
final input = new String(source.value)
ctx.getTaintedObjects().taint(input, Ranges.forCharSequence(input, source))

when:
propagation.taintIfTainted(toReport, input)
final evidence = sink.checkInjection(span, ctx, SSRF, toReport)

then:
evidence.ranges.length == 1
final range = evidence.ranges[0]
if (matches) {
final taintedEvidence = evidence.value.substring(range.start, range.start + range.length)
taintedEvidence == input
} else {
final taintedEvidence = evidence.value
taintedEvidence != input
}

where:
source | toReport | matches
new Source(REQUEST_PARAMETER_VALUE, 'url', 'datadog.com') | new URL('https://datadog.com/index.html') | true
new Source(REQUEST_PARAMETER_VALUE, 'url', 'datadog.com') | new URI('https://datadog.com/index.html') | true
new Source(REQUEST_PARAMETER_VALUE, 'url', 'datadog.com') | new URI('https://dAtAdOg.com/index.html') | false
new Source(REQUEST_PARAMETER_VALUE, 'url', 'datadog.com') | new URI('https://dAtAdOg.com/index.html') | false
}

private StackTraceElement element(final String declaringClass) {
return new StackTraceElement(declaringClass, "method", "fileName", 1)
}
Expand Down