Skip to content

Commit 1abe140

Browse files
jandro996devflow.devflow-routing-intake
jandro996
and
devflow.devflow-routing-intake
authored
Add server.request.body.filenames support for Tomcat and Netty 4.1 (#10973)
Add server.request.body.filenames instrumentation for Tomcat, Netty 4.1, and Liberty try/catch it to be safe refactor due to #10973 (comment) Address review comments: PartHelper, ParameterCollector safety, fix precedence bug Merge branch 'master' into alejandro.gonzalez/APPSEC-61873-2 Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
1 parent 30e798a commit 1abe140

File tree

11 files changed

+354
-18
lines changed

11 files changed

+354
-18
lines changed

‎dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/base/HttpServerTest.groovy‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,7 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
135135
ss.registerCallback(events.requestBodyStart(), callbacks.requestBodyStartCb)
136136
ss.registerCallback(events.requestBodyDone(), callbacks.requestBodyEndCb)
137137
ss.registerCallback(events.requestBodyProcessed(), callbacks.requestBodyObjectCb)
138+
ss.registerCallback(events.requestFilesFilenames(), callbacks.requestFilesFilenamesCb)
138139
ss.registerCallback(events.responseBody(), callbacks.responseBodyObjectCb)
139140
ss.registerCallback(events.responseStarted(), callbacks.responseStartedCb)
140141
ss.registerCallback(events.responseHeader(), callbacks.responseHeaderCb)
@@ -367,6 +368,10 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
367368
false
368369
}
369370

371+
boolean testBodyFilenames() {
372+
false
373+
}
374+
370375
boolean testBodyJson() {
371376
false
372377
}
@@ -1618,6 +1623,29 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
16181623
}
16191624
}
16201625

1626+
def 'test instrumentation gateway file upload filenames'() {
1627+
setup:
1628+
assumeTrue(testBodyFilenames())
1629+
RequestBody fileBody = RequestBody.create(MediaType.parse('application/octet-stream'), 'file content')
1630+
def body = new MultipartBody.Builder()
1631+
.setType(MultipartBody.FORM)
1632+
.addFormDataPart('file', 'evil.php', fileBody)
1633+
.build()
1634+
def httpRequest = request(BODY_MULTIPART, 'POST', body).build()
1635+
def response = client.newCall(httpRequest).execute()
1636+
1637+
when:
1638+
TEST_WRITER.waitForTraces(1)
1639+
1640+
then:
1641+
TEST_WRITER.get(0).any {
1642+
it.getTag('request.body.filenames') == "[evil.php]"
1643+
}
1644+
1645+
cleanup:
1646+
response.close()
1647+
}
1648+
16211649
def 'test instrumentation gateway json request body'() {
16221650
setup:
16231651
assumeTrue(testBodyJson())
@@ -2552,6 +2580,7 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
25522580
boolean responseHeadersInTags
25532581
boolean responseBodyTag
25542582
Object responseBody
2583+
List<String> uploadedFilenames
25552584
}
25562585

25572586
static final String stringOrEmpty(String string) {
@@ -2719,6 +2748,15 @@ abstract class HttpServerTest<SERVER> extends WithHttpServer<SERVER> {
27192748
}
27202749
} as BiFunction<RequestContext, Object, Flow<Void>>)
27212750

2751+
final BiFunction<RequestContext, List<String>, Flow<Void>> requestFilesFilenamesCb =
2752+
({
2753+
RequestContext rqCtxt, List<String> filenames ->
2754+
rqCtxt.traceSegment.setTagTop('request.body.filenames', filenames as String)
2755+
Context context = rqCtxt.getData(RequestContextSlot.APPSEC)
2756+
context.uploadedFilenames = filenames
2757+
Flow.ResultFlow.empty()
2758+
} as BiFunction<RequestContext, List<String>, Flow<Void>>)
2759+
27222760
final BiFunction<RequestContext, Object, Flow<Void>> responseBodyObjectCb =
27232761
({
27242762
RequestContext rqCtxt, Object obj ->

‎dd-java-agent/instrumentation/liberty/liberty-20.0/src/main/java/datadog/trace/instrumentation/liberty20/GetPartsInstrumentation.java‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
package datadog.trace.instrumentation.liberty20;
2+
3+
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
4+
import static datadog.trace.api.gateway.Events.EVENTS;
5+
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
6+
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
7+
import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
8+
9+
import com.google.auto.service.AutoService;
10+
import datadog.appsec.api.blocking.BlockingException;
11+
import datadog.trace.advice.ActiveRequestContext;
12+
import datadog.trace.advice.RequiresRequestContext;
13+
import datadog.trace.agent.tooling.Instrumenter;
14+
import datadog.trace.agent.tooling.InstrumenterModule;
15+
import datadog.trace.api.gateway.BlockResponseFunction;
16+
import datadog.trace.api.gateway.CallbackProvider;
17+
import datadog.trace.api.gateway.Flow;
18+
import datadog.trace.api.gateway.RequestContext;
19+
import datadog.trace.api.gateway.RequestContextSlot;
20+
import datadog.trace.bootstrap.instrumentation.api.AgentTracer;
21+
import java.util.Collection;
22+
import java.util.List;
23+
import java.util.function.BiFunction;
24+
import net.bytebuddy.asm.Advice;
25+
26+
@AutoService(InstrumenterModule.class)
27+
public class GetPartsInstrumentation extends InstrumenterModule.AppSec
28+
implements Instrumenter.ForKnownTypes, Instrumenter.HasMethodAdvice {
29+
30+
public GetPartsInstrumentation() {
31+
super("liberty");
32+
}
33+
34+
@Override
35+
public String[] knownMatchingTypes() {
36+
return new String[] {
37+
"com.ibm.ws.webcontainer.srt.SRTServletRequest",
38+
"com.ibm.ws.webcontainer31.srt.SRTServletRequest31",
39+
};
40+
}
41+
42+
@Override
43+
public String[] helperClassNames() {
44+
return new String[] {"datadog.trace.instrumentation.liberty20.PartHelper"};
45+
}
46+
47+
@Override
48+
public void methodAdvice(MethodTransformer transformer) {
49+
transformer.applyAdvice(
50+
isMethod().and(named("getParts")).and(isPublic()).and(takesArguments(0)),
51+
GetPartsInstrumentation.class.getName() + "$GetFilenamesAdvice");
52+
}
53+
54+
@RequiresRequestContext(RequestContextSlot.APPSEC)
55+
public static class GetFilenamesAdvice {
56+
@Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class)
57+
static void after(
58+
@Advice.Return Collection<?> parts,
59+
@ActiveRequestContext RequestContext reqCtx,
60+
@Advice.Thrown(readOnly = false) Throwable t) {
61+
if (t != null) {
62+
return;
63+
}
64+
List<String> filenames = PartHelper.extractFilenames(parts);
65+
if (filenames.isEmpty()) {
66+
return;
67+
}
68+
CallbackProvider cbp = AgentTracer.get().getCallbackProvider(RequestContextSlot.APPSEC);
69+
BiFunction<RequestContext, List<String>, Flow<Void>> callback =
70+
cbp.getCallback(EVENTS.requestFilesFilenames());
71+
if (callback == null) {
72+
return;
73+
}
74+
Flow<Void> flow = callback.apply(reqCtx, filenames);
75+
Flow.Action action = flow.getAction();
76+
if (action instanceof Flow.Action.RequestBlockingAction) {
77+
Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action;
78+
BlockResponseFunction brf = reqCtx.getBlockResponseFunction();
79+
if (brf != null) {
80+
brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba);
81+
if (t == null) {
82+
t = new BlockingException("Blocked request (multipart file upload)");
83+
reqCtx.getTraceSegment().effectivelyBlocked();
84+
}
85+
}
86+
}
87+
}
88+
}
89+
}

‎dd-java-agent/instrumentation/liberty/liberty-20.0/src/main/java/datadog/trace/instrumentation/liberty20/PartHelper.java‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
package datadog.trace.instrumentation.liberty20;
2+
3+
import java.lang.reflect.Method;
4+
import java.util.ArrayList;
5+
import java.util.Collection;
6+
import java.util.Collections;
7+
import java.util.List;
8+
9+
public class PartHelper {
10+
11+
public static List<String> extractFilenames(Collection<?> parts) {
12+
if (parts == null || parts.isEmpty()) {
13+
return Collections.emptyList();
14+
}
15+
Class<?> partClass = parts.iterator().next().getClass();
16+
Method getSubmittedFileName = resolveMethod(partClass, "getSubmittedFileName");
17+
Method getHeader = resolveMethod(partClass, "getHeader", String.class);
18+
19+
List<String> filenames = new ArrayList<>();
20+
for (Object part : parts) {
21+
String name = getSubmittedFilename(getSubmittedFileName, part);
22+
if (name == null) {
23+
name = getFilenameFromContentDisposition(getHeader, part);
24+
}
25+
if (name != null && !name.isEmpty()) {
26+
filenames.add(name);
27+
}
28+
}
29+
return filenames;
30+
}
31+
32+
private static Method resolveMethod(Class<?> clazz, String name, Class<?>... params) {
33+
try {
34+
return clazz.getMethod(name, params);
35+
} catch (Exception ignored) {
36+
return null;
37+
}
38+
}
39+
40+
private static String getSubmittedFilename(Method method, Object part) {
41+
if (method == null) {
42+
return null;
43+
}
44+
try {
45+
return (String) method.invoke(part);
46+
} catch (Exception ignored) {
47+
return null;
48+
}
49+
}
50+
51+
private static String getFilenameFromContentDisposition(Method getHeader, Object part) {
52+
if (getHeader == null) {
53+
return null;
54+
}
55+
try {
56+
String cd = (String) getHeader.invoke(part, "content-disposition");
57+
if (cd == null) {
58+
return null;
59+
}
60+
for (String tok : cd.split(";")) {
61+
tok = tok.trim();
62+
if (tok.startsWith("filename=")) {
63+
String name = tok.substring(9).trim();
64+
if (name.startsWith("\"") && name.endsWith("\"")) {
65+
name = name.substring(1, name.length() - 1);
66+
}
67+
return name;
68+
}
69+
}
70+
} catch (Exception ignored) {
71+
}
72+
return null;
73+
}
74+
}

‎dd-java-agent/instrumentation/liberty/liberty-20.0/src/test/groovy/datadog/trace/instrumentation/liberty20/Liberty20Test.groovy‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,11 @@ abstract class Liberty20Test extends HttpServerTest<Server> {
121121
true
122122
}
123123

124+
@Override
125+
boolean testBodyFilenames() {
126+
true
127+
}
128+
124129
@Override
125130
boolean testSessionId() {
126131
true

‎dd-java-agent/instrumentation/liberty/liberty-23.0/src/test/groovy/datadog/trace/instrumentation/liberty23/Liberty23Test.groovy‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,11 @@ abstract class Liberty23Test extends HttpServerTest<Server> {
111111
true
112112
}
113113

114+
@Override
115+
boolean testBodyFilenames() {
116+
true
117+
}
118+
114119
@Override
115120
boolean testSessionId() {
116121
true

‎dd-java-agent/instrumentation/netty/netty-4.1/src/main/java/datadog/trace/instrumentation/netty41/HttpPostRequestDecoderInstrumentation.java‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import datadog.trace.bootstrap.instrumentation.api.AgentTracer;
2121
import io.netty.handler.codec.http.EmptyHttpHeaders;
2222
import io.netty.handler.codec.http.multipart.Attribute;
23+
import io.netty.handler.codec.http.multipart.FileUpload;
2324
import io.netty.handler.codec.http.multipart.InterfaceHttpData;
2425
import io.netty.handler.codec.http.multipart.InterfaceHttpPostRequestDecoder;
2526
import java.io.IOException;
@@ -98,6 +99,7 @@ static void after(
9899
RuntimeException exc = null;
99100

100101
Map<String, List<String>> attributes = new LinkedHashMap<>();
102+
List<String> filenames = new ArrayList<>();
101103
for (InterfaceHttpData data : thiz.getBodyHttpDatas()) {
102104
if (data.getHttpDataType() == InterfaceHttpData.HttpDataType.Attribute) {
103105
String name = data.getName();
@@ -111,6 +113,11 @@ static void after(
111113
} catch (IOException e) {
112114
exc = new UndeclaredThrowableException(e);
113115
}
116+
} else if (data.getHttpDataType() == InterfaceHttpData.HttpDataType.FileUpload) {
117+
String filename = ((FileUpload) data).getFilename();
118+
if (filename != null && !filename.isEmpty()) {
119+
filenames.add(filename);
120+
}
114121
}
115122
}
116123

@@ -125,6 +132,24 @@ static void after(
125132
thr = new BlockingException("Blocked request (multipart/urlencoded post data)");
126133
}
127134

135+
if (!filenames.isEmpty()) {
136+
BiFunction<RequestContext, List<String>, Flow<Void>> filenamesCb =
137+
cbp.getCallback(EVENTS.requestFilesFilenames());
138+
if (filenamesCb != null) {
139+
Flow<Void> filenamesFlow = filenamesCb.apply(requestContext, filenames);
140+
Flow.Action filenamesAction = filenamesFlow.getAction();
141+
if (thr == null && filenamesAction instanceof Flow.Action.RequestBlockingAction) {
142+
Flow.Action.RequestBlockingAction rba =
143+
(Flow.Action.RequestBlockingAction) filenamesAction;
144+
BlockResponseFunction brf = requestContext.getBlockResponseFunction();
145+
if (brf != null) {
146+
brf.tryCommitBlockingResponse(requestContext.getTraceSegment(), rba);
147+
}
148+
thr = new BlockingException("Blocked request (multipart file upload)");
149+
}
150+
}
151+
}
152+
128153
if (exc != null) {
129154
// for it to be logged
130155
throw exc;

‎dd-java-agent/instrumentation/ratpack-1.5/src/test/groovy/server/RatpackHttpServerTest.groovy‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,11 @@ class RatpackHttpServerTest extends HttpServerTest<EmbeddedApp> {
7272
true
7373
}
7474

75+
@Override
76+
boolean testBodyFilenames() {
77+
true
78+
}
79+
7580
@Override
7681
boolean testBodyJson() {
7782
true

‎dd-java-agent/instrumentation/servlet/javax-servlet/javax-servlet-3.0/src/test/groovy/TomcatServlet3Test.groovy‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,11 @@ class TomcatServlet3TestSync extends TomcatServlet3Test {
365365
true
366366
}
367367

368+
@Override
369+
boolean testBodyFilenames() {
370+
true
371+
}
372+
368373
@Override
369374
Class<Servlet> servlet() {
370375
TestServlet3.Sync

‎dd-java-agent/instrumentation/tomcat/tomcat-5.5/src/latestDepTest/groovy/TomcatServletTest.groovy‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,11 @@ class TomcatServletTest extends AbstractServletTest<Tomcat, Context> {
5959
true
6060
}
6161

62+
@Override
63+
boolean testBodyFilenames() {
64+
true
65+
}
66+
6267
@Override
6368
boolean testBlockingOnResponse() {
6469
true

0 commit comments

Comments
 (0)