Skip to content

Commit 49bc370

Browse files
jandro996devflow.devflow-routing-intake
jandro996
and
devflow.devflow-routing-intake
authored
Add server.request.body.filenames AppSec address for commons-fileupload (#10949)
Add server.request.body.filenames IG event and GatewayBridge wiring - Add REQUEST_FILES_FILENAMES_ID=30 event to Events.java with BiFunction<RequestContext, List<String>, Flow<Void>> callback type - Register case in InstrumentationGateway switch to wrap with try-catch - Wire GatewayBridge: conditional registration, handler, cache field, reset, and IGAppSecEventDependencies entry - Add unit tests in InstrumentationGatewayTest and GatewayBridgeSpecification tag: ai generated Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Add server.request.body.filenames support for commons-fileupload Instrument ServletFileUpload.parseRequest() to extract filenames from non-form-field FileItems and fire the requestFilesFilenames() IG event. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Add smoke test for malicious file upload filename blocking Certifies that the commons-fileupload instrumentation fires server.request.body.filenames and the WAF can block on it end-to-end: - Add /upload endpoint using ServletFileUpload.parseRequest() (mirrors client's fileupload.jsp pattern) - Disable Spring multipart auto-config so Commons FileUpload handles the request before Spring intercepts it - Add commons-fileupload:1.5 dependency to the smoke test app - Add __test_file_upload_block WAF rule matching .jsp/.php/.asp/.aspx filenames and block request based on malicious file upload filename test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Fix smoke test multipart: exclude MultipartAutoConfiguration Spring's MultipartAutoConfiguration was activating despite spring.servlet.multipart.enabled=false in application.properties, causing StandardServletMultipartResolver to consume the request InputStream before Commons FileUpload could read it. Explicitly exclude MultipartAutoConfiguration via @SpringBootApplication so the raw InputStream is available to ServletFileUpload.parseRequest(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Fix import ordering and named() matcher consistency - InstrumentationGateway.java: restore alphabetical import order (REQUEST_FILES_FILENAMES_ID belongs after REQUEST_ENDED_ID) - CommonsFileUploadAppSecModule.java: use NameMatchers.named instead of ElementMatchers.named, consistent with adjacent IAST instrumentation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Merge branch 'master' into alejandro.gonzalez/APPSEC-61873-1 spotless Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
1 parent e4c4150 commit 49bc370

12 files changed

Lines changed: 271 additions & 5 deletions

File tree

‎dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,7 @@ public class GatewayBridge {
129129
new ConcurrentHashMap<>();
130130
private volatile DataSubscriberInfo execCmdSubInfo;
131131
private volatile DataSubscriberInfo shellCmdSubInfo;
132+
private volatile DataSubscriberInfo requestFilesFilenamesSubInfo;
132133

133134
public GatewayBridge(
134135
SubscriptionService subscriptionService,
@@ -201,6 +202,10 @@ public void init() {
201202
subscriptionService.registerCallback(
202203
EVENTS.requestBodyProcessed(), this::onRequestBodyProcessed);
203204
}
205+
if (additionalIGEvents.contains(EVENTS.requestFilesFilenames())) {
206+
subscriptionService.registerCallback(
207+
EVENTS.requestFilesFilenames(), this::onRequestFilesFilenames);
208+
}
204209
}
205210

206211
/**
@@ -227,6 +232,7 @@ public void reset() {
227232
loginEventSubInfo.clear();
228233
execCmdSubInfo = null;
229234
shellCmdSubInfo = null;
235+
requestFilesFilenamesSubInfo = null;
230236
}
231237

232238
private Flow<Void> onUser(final RequestContext ctx_, final String user) {
@@ -542,6 +548,31 @@ private Flow<Void> onFileLoaded(RequestContext ctx_, String path) {
542548
}
543549
}
544550

551+
private Flow<Void> onRequestFilesFilenames(RequestContext ctx_, List<String> filenames) {
552+
AppSecRequestContext ctx = ctx_.getData(RequestContextSlot.APPSEC);
553+
if (ctx == null || filenames == null || filenames.isEmpty()) {
554+
return NoopFlow.INSTANCE;
555+
}
556+
while (true) {
557+
DataSubscriberInfo subInfo = requestFilesFilenamesSubInfo;
558+
if (subInfo == null) {
559+
subInfo = producerService.getDataSubscribers(KnownAddresses.REQUEST_FILES_FILENAMES);
560+
requestFilesFilenamesSubInfo = subInfo;
561+
}
562+
if (subInfo == null || subInfo.isEmpty()) {
563+
return NoopFlow.INSTANCE;
564+
}
565+
DataBundle bundle =
566+
new SingletonDataBundle<>(KnownAddresses.REQUEST_FILES_FILENAMES, filenames);
567+
try {
568+
GatewayContext gwCtx = new GatewayContext(false);
569+
return producerService.publishDataEvent(subInfo, ctx, bundle, gwCtx);
570+
} catch (ExpiredSubscriberInfoException e) {
571+
requestFilesFilenamesSubInfo = null;
572+
}
573+
}
574+
}
575+
545576
private Flow<Void> onDatabaseSqlQuery(RequestContext ctx_, String sql) {
546577
AppSecRequestContext ctx = ctx_.getData(RequestContextSlot.APPSEC);
547578
if (ctx == null) {
@@ -1399,6 +1430,8 @@ private static class IGAppSecEventDependencies {
13991430
KnownAddresses.REQUEST_BODY_RAW, l(EVENTS.requestBodyStart(), EVENTS.requestBodyDone()));
14001431
DATA_DEPENDENCIES.put(KnownAddresses.REQUEST_PATH_PARAMS, l(EVENTS.requestPathParams()));
14011432
DATA_DEPENDENCIES.put(KnownAddresses.REQUEST_BODY_OBJECT, l(EVENTS.requestBodyProcessed()));
1433+
DATA_DEPENDENCIES.put(
1434+
KnownAddresses.REQUEST_FILES_FILENAMES, l(EVENTS.requestFilesFilenames()));
14021435
}
14031436

14041437
private static Collection<datadog.trace.api.gateway.EventType<?>> l(

‎dd-java-agent/appsec/src/test/groovy/com/datadog/appsec/gateway/GatewayBridgeSpecification.groovy‎

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,7 @@ class GatewayBridgeSpecification extends DDSpecification {
121121
BiFunction<RequestContext, HttpClientResponse, Flow<Void>> httpClientResponseCB
122122
BiFunction<RequestContext, Long, Flow<Void>> httpClientSamplingCB
123123
BiFunction<RequestContext, String, Flow<Void>> fileLoadedCB
124+
BiFunction<RequestContext, List<String>, Flow<Void>> requestFilesFilenamesCB
124125
BiFunction<RequestContext, String, Flow<Void>> requestSessionCB
125126
BiFunction<RequestContext, String[], Flow<Void>> execCmdCB
126127
BiFunction<RequestContext, String, Flow<Void>> shellCmdCB
@@ -461,7 +462,7 @@ class GatewayBridgeSpecification extends DDSpecification {
461462

462463
void callInitAndCaptureCBs() {
463464
// force all callbacks to be registered
464-
_ * eventDispatcher.allSubscribedDataAddresses() >> [KnownAddresses.REQUEST_PATH_PARAMS, KnownAddresses.REQUEST_BODY_OBJECT]
465+
_ * eventDispatcher.allSubscribedDataAddresses() >> [KnownAddresses.REQUEST_PATH_PARAMS, KnownAddresses.REQUEST_BODY_OBJECT, KnownAddresses.REQUEST_FILES_FILENAMES]
465466

466467
1 * ig.registerCallback(EVENTS.requestStarted(), _) >> {
467468
requestStartedCB = it[1]; null
@@ -553,6 +554,9 @@ class GatewayBridgeSpecification extends DDSpecification {
553554
1 * ig.registerCallback(EVENTS.httpRoute(), _) >> {
554555
httpRouteCB = it[1]; null
555556
}
557+
1 * ig.registerCallback(EVENTS.requestFilesFilenames(), _) >> {
558+
requestFilesFilenamesCB = it[1]; null
559+
}
556560
0 * ig.registerCallback(_, _)
557561

558562
bridge.init()
@@ -1078,6 +1082,38 @@ class GatewayBridgeSpecification extends DDSpecification {
10781082
gatewayContext.isRasp == true
10791083
}
10801084

1085+
void 'process request files filenames'() {
1086+
setup:
1087+
final filenames = ['malicious.php', 'document.pdf']
1088+
eventDispatcher.getDataSubscribers({
1089+
KnownAddresses.REQUEST_FILES_FILENAMES in it
1090+
}) >> nonEmptyDsInfo
1091+
DataBundle bundle
1092+
GatewayContext gatewayContext
1093+
1094+
when:
1095+
Flow<?> flow = requestFilesFilenamesCB.apply(ctx, filenames)
1096+
1097+
then:
1098+
1 * eventDispatcher.publishDataEvent(nonEmptyDsInfo, ctx.data, _ as DataBundle, _ as GatewayContext) >> {
1099+
a, b, db, gw -> bundle = db; gatewayContext = gw; NoopFlow.INSTANCE
1100+
}
1101+
bundle.get(KnownAddresses.REQUEST_FILES_FILENAMES) == filenames
1102+
flow.result == null
1103+
flow.action == Flow.Action.Noop.INSTANCE
1104+
gatewayContext.isTransient == false
1105+
gatewayContext.isRasp == false
1106+
}
1107+
1108+
void 'process request files filenames with empty list returns noop'() {
1109+
when:
1110+
Flow<?> flow = requestFilesFilenamesCB.apply(ctx, [])
1111+
1112+
then:
1113+
flow == NoopFlow.INSTANCE
1114+
0 * eventDispatcher.publishDataEvent(*_)
1115+
}
1116+
10811117
void 'process exec cmd'() {
10821118
setup:
10831119
final cmd = ['/bin/../usr/bin/reboot', '-f'] as String[]

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

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
package datadog.trace.instrumentation.commons.fileupload;
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.isPublic;
6+
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
7+
8+
import com.google.auto.service.AutoService;
9+
import datadog.appsec.api.blocking.BlockingException;
10+
import datadog.trace.advice.ActiveRequestContext;
11+
import datadog.trace.advice.RequiresRequestContext;
12+
import datadog.trace.agent.tooling.Instrumenter;
13+
import datadog.trace.agent.tooling.InstrumenterModule;
14+
import datadog.trace.api.gateway.BlockResponseFunction;
15+
import datadog.trace.api.gateway.CallbackProvider;
16+
import datadog.trace.api.gateway.Flow;
17+
import datadog.trace.api.gateway.RequestContext;
18+
import datadog.trace.api.gateway.RequestContextSlot;
19+
import datadog.trace.bootstrap.instrumentation.api.AgentTracer;
20+
import java.util.ArrayList;
21+
import java.util.List;
22+
import java.util.function.BiFunction;
23+
import net.bytebuddy.asm.Advice;
24+
import org.apache.commons.fileupload.FileItem;
25+
26+
@AutoService(InstrumenterModule.class)
27+
public class CommonsFileUploadAppSecModule extends InstrumenterModule.AppSec
28+
implements Instrumenter.ForSingleType, Instrumenter.HasMethodAdvice {
29+
30+
public CommonsFileUploadAppSecModule() {
31+
super("commons-fileupload");
32+
}
33+
34+
@Override
35+
public String instrumentedType() {
36+
return "org.apache.commons.fileupload.servlet.ServletFileUpload";
37+
}
38+
39+
@Override
40+
public void methodAdvice(MethodTransformer transformer) {
41+
transformer.applyAdvice(
42+
named("parseRequest")
43+
.and(isPublic())
44+
.and(takesArgument(0, named("javax.servlet.http.HttpServletRequest"))),
45+
getClass().getName() + "$ParseRequestAdvice");
46+
}
47+
48+
@RequiresRequestContext(RequestContextSlot.APPSEC)
49+
public static class ParseRequestAdvice {
50+
@Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class)
51+
static void after(
52+
@Advice.Return final List<FileItem> fileItems,
53+
@ActiveRequestContext RequestContext reqCtx,
54+
@Advice.Thrown(readOnly = false) Throwable t) {
55+
if (t != null || fileItems == null || fileItems.isEmpty()) {
56+
return;
57+
}
58+
59+
CallbackProvider cbp = AgentTracer.get().getCallbackProvider(RequestContextSlot.APPSEC);
60+
BiFunction<RequestContext, List<String>, Flow<Void>> callback =
61+
cbp.getCallback(EVENTS.requestFilesFilenames());
62+
if (callback == null) {
63+
return;
64+
}
65+
66+
List<String> filenames = new ArrayList<>();
67+
for (FileItem fileItem : fileItems) {
68+
if (fileItem.isFormField()) {
69+
continue;
70+
}
71+
String name = fileItem.getName();
72+
if (name != null && !name.isEmpty()) {
73+
filenames.add(name);
74+
}
75+
}
76+
if (filenames.isEmpty()) {
77+
return;
78+
}
79+
80+
Flow<Void> flow = callback.apply(reqCtx, filenames);
81+
Flow.Action action = flow.getAction();
82+
if (action instanceof Flow.Action.RequestBlockingAction) {
83+
Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action;
84+
BlockResponseFunction brf = reqCtx.getBlockResponseFunction();
85+
if (brf != null) {
86+
brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba);
87+
t = new BlockingException("Blocked request (multipart file upload)");
88+
reqCtx.getTraceSegment().effectivelyBlocked();
89+
}
90+
}
91+
}
92+
}
93+
}

‎dd-smoke-tests/appsec/springboot/build.gradle‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,9 @@ dependencies {
2121
implementation(group: 'com.fasterxml.jackson.core', name: 'jackson-databind', version: '2.6.0')
2222
implementation group: 'com.h2database', name: 'h2', version: '2.1.212'
2323

24+
// file upload
25+
implementation group: 'commons-fileupload', name: 'commons-fileupload', version: '1.5'
26+
2427
// ssrf
2528
implementation group: 'commons-httpclient', name: 'commons-httpclient', version: '2.0'
2629
implementation group: 'org.apache.httpcomponents', name: 'httpclient', version: '4.0'

‎dd-smoke-tests/appsec/springboot/gradle.lockfile‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ com.fasterxml.jackson.core:jackson-databind:2.13.0=compileClasspath,runtimeClass
2020
com.fasterxml.jackson.datatype:jackson-datatype-jdk8:2.13.0=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
2121
com.fasterxml.jackson.datatype:jackson-datatype-jsr310:2.13.0=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
2222
com.fasterxml.jackson.module:jackson-module-parameter-names:2.13.0=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
23-
com.fasterxml.jackson:jackson-bom:2.13.0=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
2423
com.github.javaparser:javaparser-core:3.25.6=codenarc
2524
com.github.jnr:jffi:1.3.14=testRuntimeClasspath
2625
com.github.jnr:jnr-a64asm:1.0.0=testRuntimeClasspath
@@ -48,9 +47,9 @@ com.squareup.okio:okio:1.17.5=testCompileClasspath,testRuntimeClasspath
4847
com.squareup.okio:okio:1.6.0=compileClasspath,runtimeClasspath
4948
com.thoughtworks.qdox:qdox:1.12.1=codenarc
5049
commons-codec:commons-codec:1.3=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
51-
commons-fileupload:commons-fileupload:1.5=testCompileClasspath,testRuntimeClasspath
50+
commons-fileupload:commons-fileupload:1.5=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
5251
commons-httpclient:commons-httpclient:2.0=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
53-
commons-io:commons-io:2.11.0=testCompileClasspath,testRuntimeClasspath
52+
commons-io:commons-io:2.11.0=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
5453
commons-io:commons-io:2.20.0=spotbugs
5554
commons-lang:commons-lang:1.0.1=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
5655
commons-logging:commons-logging:1.1.1=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath

‎dd-smoke-tests/appsec/springboot/src/main/java/datadog/smoketest/appsec/springboot/SpringbootApplication.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,9 @@
44
import java.lang.reflect.Field;
55
import org.springframework.boot.SpringApplication;
66
import org.springframework.boot.autoconfigure.SpringBootApplication;
7+
import org.springframework.boot.autoconfigure.web.servlet.MultipartAutoConfiguration;
78

8-
@SpringBootApplication
9+
@SpringBootApplication(exclude = MultipartAutoConfiguration.class)
910
public class SpringbootApplication {
1011

1112
public static void main(final String[] args) {

‎dd-smoke-tests/appsec/springboot/src/main/java/datadog/smoketest/appsec/springboot/controller/WebController.java‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,15 @@
2020
import java.nio.file.Paths;
2121
import java.sql.Connection;
2222
import java.sql.DriverManager;
23+
import java.util.ArrayList;
24+
import java.util.List;
2325
import java.util.Map;
2426
import javax.servlet.http.HttpServletRequest;
2527
import javax.servlet.http.HttpSession;
28+
import org.apache.commons.fileupload.FileItem;
29+
import org.apache.commons.fileupload.FileUploadException;
30+
import org.apache.commons.fileupload.disk.DiskFileItemFactory;
31+
import org.apache.commons.fileupload.servlet.ServletFileUpload;
2632
import org.apache.commons.httpclient.HttpClient;
2733
import org.apache.commons.httpclient.HttpMethod;
2834
import org.apache.commons.httpclient.methods.GetMethod;
@@ -272,6 +278,25 @@ public ResponseEntity<String> exceedResponseHeaders() {
272278
return new ResponseEntity<>("Custom headers added", headers, HttpStatus.OK);
273279
}
274280

281+
@PostMapping(value = "/upload", consumes = "multipart/form-data")
282+
public ResponseEntity<String> upload(HttpServletRequest request) {
283+
if (!ServletFileUpload.isMultipartContent(request)) {
284+
return ResponseEntity.badRequest().body("Not a multipart request");
285+
}
286+
try {
287+
List<FileItem> items = new ServletFileUpload(new DiskFileItemFactory()).parseRequest(request);
288+
List<String> names = new ArrayList<>();
289+
for (FileItem item : items) {
290+
if (!item.isFormField()) {
291+
names.add(item.getName());
292+
}
293+
}
294+
return ResponseEntity.ok("Uploaded: " + names);
295+
} catch (FileUploadException e) {
296+
return ResponseEntity.status(500).body("Upload error: " + e.getMessage());
297+
}
298+
}
299+
275300
@PostMapping("/waf-event-with-body")
276301
public String wafEventWithBody(@RequestBody String body) {
277302
return "EXECUTED";

‎dd-smoke-tests/appsec/springboot/src/main/resources/application.properties‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
11
logging.level.root=WARN
2+
spring.servlet.multipart.enabled=false

‎dd-smoke-tests/appsec/springboot/src/test/groovy/datadog/smoketest/appsec/SpringBootSmokeTest.groovy‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,26 @@ class SpringBootSmokeTest extends AbstractAppSecServerSmokeTest {
210210
transformers: [],
211211
on_match : ['block']
212212
],
213+
[
214+
id : '__test_file_upload_block',
215+
name : 'test rule to block on malicious file upload filename',
216+
tags : [
217+
type : 'unrestricted-file-upload',
218+
category : 'attack_attempt',
219+
confidence: '1',
220+
],
221+
conditions : [
222+
[
223+
parameters: [
224+
inputs: [[address: 'server.request.body.filenames']],
225+
regex : '\\.(?:jsp|php|asp|aspx)$',
226+
],
227+
operator : 'match_regex',
228+
]
229+
],
230+
transformers: [],
231+
on_match : ['block']
232+
],
213233
[
214234
id : "apiA-100-001",
215235
name: "API 10 tag rule on request headers",
@@ -559,6 +579,38 @@ class SpringBootSmokeTest extends AbstractAppSecServerSmokeTest {
559579
}
560580
}
561581

582+
void 'block request based on malicious file upload filename'() {
583+
when:
584+
String url = "http://localhost:${httpPort}/upload"
585+
def requestBody = new okhttp3.MultipartBody.Builder()
586+
.setType(okhttp3.MultipartBody.FORM)
587+
.addFormDataPart('file', 'exploit.jsp',
588+
RequestBody.create(MediaType.parse('application/octet-stream'), 'webshell content'))
589+
.build()
590+
def request = new Request.Builder()
591+
.url(url)
592+
.post(requestBody)
593+
.build()
594+
def response = client.newCall(request).execute()
595+
def responseBodyStr = response.body().string()
596+
597+
then:
598+
responseBodyStr.contains("blocked")
599+
response.code() == 403
600+
601+
when:
602+
waitForTraceCount(1) == 1
603+
604+
then:
605+
rootSpans.size() == 1
606+
forEachRootSpanTrigger {
607+
assert it['rule']['id'] == '__test_file_upload_block'
608+
}
609+
rootSpans.each {
610+
assert it.meta.get('appsec.blocked') != null, 'appsec.blocked is not set'
611+
}
612+
}
613+
562614
void 'rasp reports stacktrace on sql injection'() {
563615
when:
564616
String url = "http://localhost:${httpPort}/sqli/query?id=' OR 1=1 --"

0 commit comments

Comments
 (0)