Skip to content

Commit d8dc1f2

Browse files
authored
fix addVulnerabilityStackTrace in reportVulnerability when multiple vulnerabilities share location (#8412)
If multiple vulnerabilities share the same location, it is not necessary to generate the stack trace again, as they should share it
1 parent e0242cf commit d8dc1f2

File tree

2 files changed

+77
-1
lines changed

2 files changed

+77
-1
lines changed

dd-java-agent/agent-iast/src/main/java/com/datadog/iast/Reporter.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,9 @@ private void reportVulnerability(
7777
final VulnerabilityBatch batch = getOrCreateVulnerabilityBatch(span);
7878
if (batch != null) {
7979
batch.add(vulnerability);
80-
if (Config.get().isIastStackTraceEnabled() && batch.getVulnerabilities() != null) {
80+
if (Config.get().isIastStackTraceEnabled()
81+
&& batch.getVulnerabilities() != null
82+
&& vulnerability.getLocation().getStackId() == null) {
8183
String stackId =
8284
addVulnerabilityStackTrace(span, String.valueOf(batch.getVulnerabilities().size()));
8385
if (stackId != null) {

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

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -533,6 +533,80 @@ class ReporterTest extends DDSpecification {
533533
batch.vulnerabilities.size() == 2
534534
}
535535

536+
void 'two cookie vulnerabilities that share location share stackId and only generate one stack'() {
537+
given:
538+
final Reporter reporter = new Reporter()
539+
final traceSegment = Mock(TraceSegment)
540+
final ctx = new IastRequestContext(noOpTaintedObjects())
541+
final reqCtx = Mock(RequestContext)
542+
final spanId = 123456
543+
VulnerabilityBatch batch = null
544+
Map stackTraceBatch = new ConcurrentHashMap()
545+
546+
final span = Stub(AgentSpan)
547+
span.getRequestContext() >> reqCtx
548+
span.getSpanId() >> spanId
549+
550+
final location = Location.forSpanAndStack(span, new StackTraceElement("foo", "foo", "foo", 1))
551+
552+
final v1 = new Vulnerability(
553+
VulnerabilityType.INSECURE_COOKIE,
554+
location,
555+
new Evidence("JSESSIONID")
556+
)
557+
final v2 = new Vulnerability(
558+
VulnerabilityType.NO_SAMESITE_COOKIE,
559+
location,
560+
new Evidence("JSESSIONID")
561+
)
562+
563+
when:
564+
reporter.report(span, v1)
565+
reporter.report(span, v2)
566+
567+
then:
568+
1 * reqCtx.getData(RequestContextSlot.IAST) >> ctx
569+
2 * reqCtx.getTraceSegment() >> traceSegment
570+
// first vulnerability
571+
1 * traceSegment.getDataTop('iast') >> null
572+
1 * traceSegment.setDataTop('iast', _) >> { batch = it[1] as VulnerabilityBatch }
573+
1 * reqCtx.getOrCreateMetaStructTop('_dd.stack', _) >> { stackTraceBatch }
574+
// second vulnerability
575+
1 * traceSegment.getDataTop('iast') >> { return batch } // second vulnerability
576+
JSONAssert.assertEquals('''{
577+
"vulnerabilities": [
578+
{
579+
"evidence": { "value":"JSESSIONID" },
580+
"hash":1156210466,
581+
"location": {
582+
"spanId":123456,
583+
"line":1,
584+
"path":"foo",
585+
"method": "foo",
586+
"stackId":"1"
587+
},
588+
"type":"INSECURE_COOKIE"
589+
},
590+
{
591+
"evidence": {"value":"JSESSIONID"},
592+
"hash":1090504969,
593+
"location": {
594+
"spanId":123456,
595+
"line":1,
596+
"path":"foo",
597+
"method": "foo",
598+
"stackId":"1"
599+
},
600+
"type":"NO_SAMESITE_COOKIE"
601+
}
602+
]
603+
}''', batch.toString(), true)
604+
1 * traceSegment.setTagTop('asm.keep', true)
605+
1 * traceSegment.setTagTop('_dd.p.ts', ProductTraceSource.ASM)
606+
assert stackTraceBatch.get("vulnerability").size() == 1
607+
0 * _
608+
}
609+
536610
private AgentSpan spanWithBatch(final VulnerabilityBatch batch) {
537611
final traceSegment = Mock(TraceSegment) {
538612
getDataTop('iast') >> batch

0 commit comments

Comments
 (0)