Skip to content

Commit fd7f750

Browse files
MariovidoMariovido
authored
Fix progagation for Untrusted Deserialization vulnerability (#7374)
This improves the smoke tests and add new instrumentations to ensure the propagation.
1 parent 594a2a4 commit fd7f750

File tree

25 files changed

+1440
-5
lines changed

25 files changed

+1440
-5
lines changed

‎dd-java-agent/instrumentation/commons-fileupload/build.gradle‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,11 @@ dependencies {
77
testImplementation group: 'org.apache.commons', name: 'commons-fileupload2', version: '2.0.0-M1'
88
testImplementation group: 'org.apache.tomcat.embed', name: 'tomcat-embed-core', version: '7.0.0'
99

10+
compileOnly group: 'commons-fileupload', name: 'commons-fileupload', version: '1.5'
11+
testImplementation group: 'commons-fileupload', name: 'commons-fileupload', version: '1.5'
12+
testImplementation group: 'javax.servlet', name: 'javax.servlet-api', version: '3.1.0'
1013

1114
testRuntimeOnly project(':dd-java-agent:instrumentation:iast-instrumenter')
1215
latestDepTestImplementation group: 'org.apache.commons', name: 'commons-fileupload2', version: '+'
16+
latestDepTestImplementation group: 'commons-fileupload', name: 'commons-fileupload', version: '1.+'
1317
}

‎dd-java-agent/instrumentation/commons-fileupload/src/main/java/datadog/trace/instrumentation/commons/fileupload/FileItemInstrumenter.java‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
package datadog.trace.instrumentation.commons.fileupload;
2+
3+
import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface;
4+
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
5+
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
6+
import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
7+
8+
import com.google.auto.service.AutoService;
9+
import datadog.trace.advice.ActiveRequestContext;
10+
import datadog.trace.advice.RequiresRequestContext;
11+
import datadog.trace.agent.tooling.Instrumenter;
12+
import datadog.trace.agent.tooling.InstrumenterModule;
13+
import datadog.trace.api.gateway.RequestContext;
14+
import datadog.trace.api.gateway.RequestContextSlot;
15+
import datadog.trace.api.iast.IastContext;
16+
import datadog.trace.api.iast.InstrumentationBridge;
17+
import datadog.trace.api.iast.propagation.PropagationModule;
18+
import java.io.InputStream;
19+
import net.bytebuddy.asm.Advice;
20+
import net.bytebuddy.description.type.TypeDescription;
21+
import net.bytebuddy.matcher.ElementMatcher;
22+
import org.apache.commons.fileupload.FileItem;
23+
24+
@AutoService(InstrumenterModule.class)
25+
public class FileItemInstrumenter extends InstrumenterModule.Iast
26+
implements Instrumenter.ForTypeHierarchy {
27+
28+
public FileItemInstrumenter() {
29+
super("commons-fileupload", "fileitem");
30+
}
31+
32+
@Override
33+
public String hierarchyMarkerType() {
34+
return "org.apache.commons.fileupload.FileItem";
35+
}
36+
37+
@Override
38+
public ElementMatcher<TypeDescription> hierarchyMatcher() {
39+
return implementsInterface(named(hierarchyMarkerType()));
40+
}
41+
42+
@Override
43+
public void methodAdvice(MethodTransformer transformer) {
44+
transformer.applyAdvice(
45+
named("getInputStream").and(isPublic()).and(takesArguments(0)),
46+
getClass().getName() + "$GetInputStreamAdvice");
47+
}
48+
49+
@RequiresRequestContext(RequestContextSlot.IAST)
50+
public static class GetInputStreamAdvice {
51+
@Advice.OnMethodExit(suppress = Throwable.class)
52+
public static void onExit(
53+
@Advice.Return final InputStream inputStream,
54+
@Advice.This final FileItem self,
55+
@ActiveRequestContext RequestContext reqCtx) {
56+
final PropagationModule module = InstrumentationBridge.PROPAGATION;
57+
if (module != null) {
58+
IastContext ctx = reqCtx.getData(RequestContextSlot.IAST);
59+
module.taintObjectIfTainted(ctx, inputStream, self);
60+
}
61+
}
62+
}
63+
}

‎dd-java-agent/instrumentation/commons-fileupload/src/main/java/datadog/trace/instrumentation/commons/fileupload/FileItemIteratorInstrumenter.java‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
package datadog.trace.instrumentation.commons.fileupload;
2+
3+
import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface;
4+
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
5+
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
6+
import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
7+
8+
import com.google.auto.service.AutoService;
9+
import datadog.trace.advice.ActiveRequestContext;
10+
import datadog.trace.advice.RequiresRequestContext;
11+
import datadog.trace.agent.tooling.Instrumenter;
12+
import datadog.trace.agent.tooling.InstrumenterModule;
13+
import datadog.trace.api.gateway.RequestContext;
14+
import datadog.trace.api.gateway.RequestContextSlot;
15+
import datadog.trace.api.iast.IastContext;
16+
import datadog.trace.api.iast.InstrumentationBridge;
17+
import datadog.trace.api.iast.propagation.PropagationModule;
18+
import net.bytebuddy.asm.Advice;
19+
import net.bytebuddy.description.type.TypeDescription;
20+
import net.bytebuddy.matcher.ElementMatcher;
21+
import org.apache.commons.fileupload.FileItemIterator;
22+
import org.apache.commons.fileupload.FileItemStream;
23+
24+
@AutoService(InstrumenterModule.class)
25+
public class FileItemIteratorInstrumenter extends InstrumenterModule.Iast
26+
implements Instrumenter.ForTypeHierarchy {
27+
28+
public FileItemIteratorInstrumenter() {
29+
super("commons-fileupload", "fileitemiterator");
30+
}
31+
32+
@Override
33+
public String hierarchyMarkerType() {
34+
return "org.apache.commons.fileupload.FileItemIterator";
35+
}
36+
37+
@Override
38+
public ElementMatcher<TypeDescription> hierarchyMatcher() {
39+
return implementsInterface(named(hierarchyMarkerType()));
40+
}
41+
42+
@Override
43+
public void methodAdvice(MethodTransformer transformer) {
44+
transformer.applyAdvice(
45+
named("next").and(isPublic()).and(takesArguments(0)), getClass().getName() + "$NextAdvice");
46+
}
47+
48+
@RequiresRequestContext(RequestContextSlot.IAST)
49+
public static class NextAdvice {
50+
@Advice.OnMethodExit(suppress = Throwable.class)
51+
public static void onExit(
52+
@Advice.Return final FileItemStream fileItemStream,
53+
@Advice.This final FileItemIterator self,
54+
@ActiveRequestContext RequestContext reqCtx) {
55+
final PropagationModule module = InstrumentationBridge.PROPAGATION;
56+
if (module != null) {
57+
IastContext ctx = reqCtx.getData(RequestContextSlot.IAST);
58+
module.taintObjectIfTainted(ctx, fileItemStream, self);
59+
}
60+
}
61+
}
62+
}

‎dd-java-agent/instrumentation/commons-fileupload/src/main/java/datadog/trace/instrumentation/commons/fileupload/FileItemStreamInstrumenter.java‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
package datadog.trace.instrumentation.commons.fileupload;
2+
3+
import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface;
4+
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
5+
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
6+
import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
7+
8+
import com.google.auto.service.AutoService;
9+
import datadog.trace.advice.ActiveRequestContext;
10+
import datadog.trace.advice.RequiresRequestContext;
11+
import datadog.trace.agent.tooling.Instrumenter;
12+
import datadog.trace.agent.tooling.InstrumenterModule;
13+
import datadog.trace.api.gateway.RequestContext;
14+
import datadog.trace.api.gateway.RequestContextSlot;
15+
import datadog.trace.api.iast.IastContext;
16+
import datadog.trace.api.iast.InstrumentationBridge;
17+
import datadog.trace.api.iast.propagation.PropagationModule;
18+
import java.io.InputStream;
19+
import net.bytebuddy.asm.Advice;
20+
import net.bytebuddy.description.type.TypeDescription;
21+
import net.bytebuddy.matcher.ElementMatcher;
22+
import org.apache.commons.fileupload.FileItemStream;
23+
24+
@AutoService(InstrumenterModule.class)
25+
public class FileItemStreamInstrumenter extends InstrumenterModule.Iast
26+
implements Instrumenter.ForTypeHierarchy {
27+
28+
public FileItemStreamInstrumenter() {
29+
super("commons-fileupload", "fileitemstream");
30+
}
31+
32+
@Override
33+
public String hierarchyMarkerType() {
34+
return "org.apache.commons.fileupload.FileItemStream";
35+
}
36+
37+
@Override
38+
public ElementMatcher<TypeDescription> hierarchyMatcher() {
39+
return implementsInterface(named(hierarchyMarkerType()));
40+
}
41+
42+
@Override
43+
public void methodAdvice(MethodTransformer transformer) {
44+
transformer.applyAdvice(
45+
named("openStream").and(isPublic()).and(takesArguments(0)),
46+
getClass().getName() + "$OpenStreamAdvice");
47+
}
48+
49+
@RequiresRequestContext(RequestContextSlot.IAST)
50+
public static class OpenStreamAdvice {
51+
@Advice.OnMethodExit(suppress = Throwable.class)
52+
public static void onExit(
53+
@Advice.Return final InputStream inputStream,
54+
@Advice.This final FileItemStream self,
55+
@ActiveRequestContext RequestContext reqCtx) {
56+
final PropagationModule module = InstrumentationBridge.PROPAGATION;
57+
if (module != null) {
58+
IastContext ctx = reqCtx.getData(RequestContextSlot.IAST);
59+
module.taintObjectIfTainted(ctx, inputStream, self);
60+
}
61+
}
62+
}
63+
}

‎dd-java-agent/instrumentation/commons-fileupload/src/main/java/datadog/trace/instrumentation/commons/fileupload/ServletFileUploadInstrumenter.java‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
package datadog.trace.instrumentation.commons.fileupload;
2+
3+
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
4+
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
5+
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
6+
7+
import com.google.auto.service.AutoService;
8+
import datadog.trace.advice.ActiveRequestContext;
9+
import datadog.trace.advice.RequiresRequestContext;
10+
import datadog.trace.agent.tooling.Instrumenter;
11+
import datadog.trace.agent.tooling.InstrumenterModule;
12+
import datadog.trace.api.gateway.RequestContext;
13+
import datadog.trace.api.gateway.RequestContextSlot;
14+
import datadog.trace.api.iast.IastContext;
15+
import datadog.trace.api.iast.InstrumentationBridge;
16+
import datadog.trace.api.iast.Source;
17+
import datadog.trace.api.iast.SourceTypes;
18+
import datadog.trace.api.iast.propagation.PropagationModule;
19+
import java.util.List;
20+
import java.util.Map;
21+
import net.bytebuddy.asm.Advice;
22+
import org.apache.commons.fileupload.FileItem;
23+
import org.apache.commons.fileupload.FileItemIterator;
24+
25+
@AutoService(InstrumenterModule.class)
26+
public class ServletFileUploadInstrumenter extends InstrumenterModule.Iast
27+
implements Instrumenter.ForSingleType {
28+
29+
public ServletFileUploadInstrumenter() {
30+
super("commons-fileupload", "servlet");
31+
}
32+
33+
@Override
34+
public String instrumentedType() {
35+
return "org.apache.commons.fileupload.servlet.ServletFileUpload";
36+
}
37+
38+
@Override
39+
public void methodAdvice(MethodTransformer transformer) {
40+
transformer.applyAdvice(
41+
named("parseRequest")
42+
.and(isPublic())
43+
.and(takesArgument(0, named("javax.servlet.http.HttpServletRequest"))),
44+
getClass().getName() + "$ParseRequestAdvice");
45+
transformer.applyAdvice(
46+
named("parseParameterMap")
47+
.and(isPublic())
48+
.and(takesArgument(0, named("javax.servlet.http.HttpServletRequest"))),
49+
getClass().getName() + "$ParseParameterMapAdvice");
50+
transformer.applyAdvice(
51+
named("getItemIterator")
52+
.and(isPublic())
53+
.and(takesArgument(0, named("javax.servlet.http.HttpServletRequest"))),
54+
getClass().getName() + "$GetItemIteratorAdvice");
55+
}
56+
57+
@RequiresRequestContext(RequestContextSlot.IAST)
58+
public static class ParseRequestAdvice {
59+
@Advice.OnMethodExit(suppress = Throwable.class)
60+
@Source(SourceTypes.REQUEST_MULTIPART_PARAMETER)
61+
public static void onExit(
62+
@Advice.Return final List<FileItem> fileItems,
63+
@ActiveRequestContext RequestContext reqCtx) {
64+
final PropagationModule module = InstrumentationBridge.PROPAGATION;
65+
if (module != null) {
66+
IastContext ctx = reqCtx.getData(RequestContextSlot.IAST);
67+
for (final FileItem fileItem : fileItems) {
68+
module.taintObject(ctx, fileItem, SourceTypes.REQUEST_MULTIPART_PARAMETER);
69+
}
70+
}
71+
}
72+
}
73+
74+
@RequiresRequestContext(RequestContextSlot.IAST)
75+
public static class ParseParameterMapAdvice {
76+
@Advice.OnMethodExit(suppress = Throwable.class)
77+
@Source(SourceTypes.REQUEST_MULTIPART_PARAMETER)
78+
public static void onExit(
79+
@Advice.Return final Map<String, List<FileItem>> parameterMap,
80+
@ActiveRequestContext RequestContext reqCtx) {
81+
final PropagationModule module = InstrumentationBridge.PROPAGATION;
82+
if (module != null) {
83+
IastContext ctx = reqCtx.getData(RequestContextSlot.IAST);
84+
for (List<FileItem> fileItems : parameterMap.values()) {
85+
for (FileItem fileItem : fileItems) {
86+
module.taintObject(ctx, fileItem, SourceTypes.REQUEST_MULTIPART_PARAMETER);
87+
}
88+
}
89+
}
90+
}
91+
}
92+
93+
@RequiresRequestContext(RequestContextSlot.IAST)
94+
public static class GetItemIteratorAdvice {
95+
@Advice.OnMethodExit(suppress = Throwable.class)
96+
@Source(SourceTypes.REQUEST_MULTIPART_PARAMETER)
97+
public static void onExit(
98+
@Advice.Return final FileItemIterator fileItemIterator,
99+
@ActiveRequestContext RequestContext reqCtx) {
100+
final PropagationModule module = InstrumentationBridge.PROPAGATION;
101+
if (module != null) {
102+
IastContext ctx = reqCtx.getData(RequestContextSlot.IAST);
103+
module.taintObject(ctx, fileItemIterator, SourceTypes.REQUEST_MULTIPART_PARAMETER);
104+
}
105+
}
106+
}
107+
}

‎dd-java-agent/instrumentation/commons-fileupload/src/test/groovy/FileItemInstrumenterTest.groovy‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
import datadog.trace.agent.test.AgentTestRunner
2+
import datadog.trace.api.iast.IastContext
3+
import datadog.trace.api.iast.InstrumentationBridge
4+
import datadog.trace.api.iast.propagation.PropagationModule
5+
import datadog.trace.bootstrap.instrumentation.api.AgentTracer
6+
import datadog.trace.bootstrap.instrumentation.api.TagContext
7+
import foo.bar.smoketest.MockFileItem
8+
9+
class FileItemInstrumenterTest extends AgentTestRunner {
10+
11+
private Object iastCtx
12+
13+
@Override
14+
protected void configurePreAgent() {
15+
injectSysConfig('dd.iast.enabled', 'true')
16+
}
17+
18+
@Override
19+
void setup() {
20+
iastCtx = Stub(IastContext)
21+
}
22+
23+
@Override
24+
void cleanup() {
25+
InstrumentationBridge.clearIastModules()
26+
}
27+
28+
void 'test commons fileupload FileItem getInputStream'() {
29+
given:
30+
final module = Mock(PropagationModule)
31+
InstrumentationBridge.registerIastModule(module)
32+
final inputStream = new ByteArrayInputStream('inputStream'.getBytes())
33+
final fileItem = new MockFileItem('fileItemName', inputStream)
34+
35+
when:
36+
runUnderIastTrace { fileItem.getInputStream() }
37+
38+
then:
39+
1 * module.taintObjectIfTainted(iastCtx, inputStream, fileItem)
40+
0 * _
41+
}
42+
43+
protected <E> E runUnderIastTrace(Closure<E> cl) {
44+
final ddctx = new TagContext().withRequestContextDataIast(iastCtx)
45+
final span = TEST_TRACER.startSpan("test", "test-iast-span", ddctx)
46+
try {
47+
return AgentTracer.activateSpan(span).withCloseable(cl)
48+
} finally {
49+
span.finish()
50+
}
51+
}
52+
}

0 commit comments

Comments
 (0)