Add server.request.body.filenames AppSec address for commons-fileupload

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

@@ -129,6 +129,7 @@ public class GatewayBridge {
       new ConcurrentHashMap<>();
   private volatile DataSubscriberInfo execCmdSubInfo;
   private volatile DataSubscriberInfo shellCmdSubInfo;
+  private volatile DataSubscriberInfo requestFilesFilenamesSubInfo;
 
   public GatewayBridge(
       SubscriptionService subscriptionService,
@@ -201,6 +202,10 @@ public void init() {
       subscriptionService.registerCallback(
           EVENTS.requestBodyProcessed(), this::onRequestBodyProcessed);
     }
+    if (additionalIGEvents.contains(EVENTS.requestFilesFilenames())) {
+      subscriptionService.registerCallback(
+          EVENTS.requestFilesFilenames(), this::onRequestFilesFilenames);
+    }
   }
 
   /**
@@ -227,6 +232,7 @@ public void reset() {
     loginEventSubInfo.clear();
     execCmdSubInfo = null;
     shellCmdSubInfo = null;
+    requestFilesFilenamesSubInfo = null;
   }
 
   private Flow<Void> onUser(final RequestContext ctx_, final String user) {
@@ -542,6 +548,31 @@ private Flow<Void> onFileLoaded(RequestContext ctx_, String path) {
     }
   }
 
+  private Flow<Void> onRequestFilesFilenames(RequestContext ctx_, List<String> filenames) {
+    AppSecRequestContext ctx = ctx_.getData(RequestContextSlot.APPSEC);
+    if (ctx == null || filenames == null || filenames.isEmpty()) {
+      return NoopFlow.INSTANCE;
+    }
+    while (true) {
+      DataSubscriberInfo subInfo = requestFilesFilenamesSubInfo;
+      if (subInfo == null) {
+        subInfo = producerService.getDataSubscribers(KnownAddresses.REQUEST_FILES_FILENAMES);
+        requestFilesFilenamesSubInfo = subInfo;
+      }
+      if (subInfo == null || subInfo.isEmpty()) {
+        return NoopFlow.INSTANCE;
+      }
+      DataBundle bundle =
+          new SingletonDataBundle<>(KnownAddresses.REQUEST_FILES_FILENAMES, filenames);
+      try {
+        GatewayContext gwCtx = new GatewayContext(false);
+        return producerService.publishDataEvent(subInfo, ctx, bundle, gwCtx);
+      } catch (ExpiredSubscriberInfoException e) {
+        requestFilesFilenamesSubInfo = null;
+      }
+    }
+  }
+
   private Flow<Void> onDatabaseSqlQuery(RequestContext ctx_, String sql) {
     AppSecRequestContext ctx = ctx_.getData(RequestContextSlot.APPSEC);
     if (ctx == null) {
@@ -1399,6 +1430,8 @@ private static class IGAppSecEventDependencies {
           KnownAddresses.REQUEST_BODY_RAW, l(EVENTS.requestBodyStart(), EVENTS.requestBodyDone()));
       DATA_DEPENDENCIES.put(KnownAddresses.REQUEST_PATH_PARAMS, l(EVENTS.requestPathParams()));
       DATA_DEPENDENCIES.put(KnownAddresses.REQUEST_BODY_OBJECT, l(EVENTS.requestBodyProcessed()));
+      DATA_DEPENDENCIES.put(
+          KnownAddresses.REQUEST_FILES_FILENAMES, l(EVENTS.requestFilesFilenames()));
     }
 
     private static Collection<datadog.trace.api.gateway.EventType<?>> l(

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

@@ -121,6 +121,7 @@ class GatewayBridgeSpecification extends DDSpecification {
   BiFunction<RequestContext, HttpClientResponse, Flow<Void>> httpClientResponseCB
   BiFunction<RequestContext, Long, Flow<Void>> httpClientSamplingCB
   BiFunction<RequestContext, String, Flow<Void>> fileLoadedCB
+  BiFunction<RequestContext, List<String>, Flow<Void>> requestFilesFilenamesCB
   BiFunction<RequestContext, String, Flow<Void>> requestSessionCB
   BiFunction<RequestContext, String[], Flow<Void>> execCmdCB
   BiFunction<RequestContext, String, Flow<Void>> shellCmdCB
@@ -461,7 +462,7 @@ class GatewayBridgeSpecification extends DDSpecification {
 
   void callInitAndCaptureCBs() {
     // force all callbacks to be registered
-    _ * eventDispatcher.allSubscribedDataAddresses() >> [KnownAddresses.REQUEST_PATH_PARAMS, KnownAddresses.REQUEST_BODY_OBJECT]
+    _ * eventDispatcher.allSubscribedDataAddresses() >> [KnownAddresses.REQUEST_PATH_PARAMS, KnownAddresses.REQUEST_BODY_OBJECT, KnownAddresses.REQUEST_FILES_FILENAMES]
 
     1 * ig.registerCallback(EVENTS.requestStarted(), _) >> {
       requestStartedCB = it[1]; null
@@ -553,6 +554,9 @@ class GatewayBridgeSpecification extends DDSpecification {
     1 * ig.registerCallback(EVENTS.httpRoute(), _) >> {
       httpRouteCB = it[1]; null
     }
+    1 * ig.registerCallback(EVENTS.requestFilesFilenames(), _) >> {
+      requestFilesFilenamesCB = it[1]; null
+    }
     0 * ig.registerCallback(_, _)
 
     bridge.init()
@@ -1078,6 +1082,38 @@ class GatewayBridgeSpecification extends DDSpecification {
     gatewayContext.isRasp == true
   }
 
+  void 'process request files filenames'() {
+    setup:
+    final filenames = ['malicious.php', 'document.pdf']
+    eventDispatcher.getDataSubscribers({
+      KnownAddresses.REQUEST_FILES_FILENAMES in it
+    }) >> nonEmptyDsInfo
+    DataBundle bundle
+    GatewayContext gatewayContext
+
+    when:
+    Flow<?> flow = requestFilesFilenamesCB.apply(ctx, filenames)
+
+    then:
+    1 * eventDispatcher.publishDataEvent(nonEmptyDsInfo, ctx.data, _ as DataBundle, _ as GatewayContext) >> {
+      a, b, db, gw -> bundle = db; gatewayContext = gw; NoopFlow.INSTANCE
+    }
+    bundle.get(KnownAddresses.REQUEST_FILES_FILENAMES) == filenames
+    flow.result == null
+    flow.action == Flow.Action.Noop.INSTANCE
+    gatewayContext.isTransient == false
+    gatewayContext.isRasp == false
+  }
+
+  void 'process request files filenames with empty list returns noop'() {
+    when:
+    Flow<?> flow = requestFilesFilenamesCB.apply(ctx, [])
+
+    then:
+    flow == NoopFlow.INSTANCE
+    0 * eventDispatcher.publishDataEvent(*_)
+  }
+
   void 'process exec cmd'() {
     setup:
     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

@@ -0,0 +1,93 @@
+package datadog.trace.instrumentation.commons.fileupload;
+
+import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
+import static datadog.trace.api.gateway.Events.EVENTS;
+import static net.bytebuddy.matcher.ElementMatchers.isPublic;
+import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
+
+import com.google.auto.service.AutoService;
+import datadog.appsec.api.blocking.BlockingException;
+import datadog.trace.advice.ActiveRequestContext;
+import datadog.trace.advice.RequiresRequestContext;
+import datadog.trace.agent.tooling.Instrumenter;
+import datadog.trace.agent.tooling.InstrumenterModule;
+import datadog.trace.api.gateway.BlockResponseFunction;
+import datadog.trace.api.gateway.CallbackProvider;
+import datadog.trace.api.gateway.Flow;
+import datadog.trace.api.gateway.RequestContext;
+import datadog.trace.api.gateway.RequestContextSlot;
+import datadog.trace.bootstrap.instrumentation.api.AgentTracer;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.function.BiFunction;
+import net.bytebuddy.asm.Advice;
+import org.apache.commons.fileupload.FileItem;
+
+@AutoService(InstrumenterModule.class)
+public class CommonsFileUploadAppSecModule extends InstrumenterModule.AppSec
+    implements Instrumenter.ForSingleType, Instrumenter.HasMethodAdvice {
+
+  public CommonsFileUploadAppSecModule() {
+    super("commons-fileupload");
+  }
+
+  @Override
+  public String instrumentedType() {
+    return "org.apache.commons.fileupload.servlet.ServletFileUpload";
+  }
+
+  @Override
+  public void methodAdvice(MethodTransformer transformer) {
+    transformer.applyAdvice(
+        named("parseRequest")
+            .and(isPublic())
+            .and(takesArgument(0, named("javax.servlet.http.HttpServletRequest"))),
+        getClass().getName() + "$ParseRequestAdvice");
+  }
+
+  @RequiresRequestContext(RequestContextSlot.APPSEC)
+  public static class ParseRequestAdvice {
+    @Advice.OnMethodExit(suppress = Throwable.class, onThrowable = Throwable.class)
+    static void after(
+        @Advice.Return final List<FileItem> fileItems,
+        @ActiveRequestContext RequestContext reqCtx,
+        @Advice.Thrown(readOnly = false) Throwable t) {
+      if (t != null || fileItems == null || fileItems.isEmpty()) {
+        return;
+      }
+
+      CallbackProvider cbp = AgentTracer.get().getCallbackProvider(RequestContextSlot.APPSEC);
+      BiFunction<RequestContext, List<String>, Flow<Void>> callback =
+          cbp.getCallback(EVENTS.requestFilesFilenames());
+      if (callback == null) {
+        return;
+      }
+
+      List<String> filenames = new ArrayList<>();
+      for (FileItem fileItem : fileItems) {
+        if (fileItem.isFormField()) {
+          continue;
+        }
+        String name = fileItem.getName();
+        if (name != null && !name.isEmpty()) {
+          filenames.add(name);
+        }
+      }
+      if (filenames.isEmpty()) {
+        return;
+      }
+
+      Flow<Void> flow = callback.apply(reqCtx, filenames);
+      Flow.Action action = flow.getAction();
+      if (action instanceof Flow.Action.RequestBlockingAction) {
+        Flow.Action.RequestBlockingAction rba = (Flow.Action.RequestBlockingAction) action;
+        BlockResponseFunction brf = reqCtx.getBlockResponseFunction();
+        if (brf != null) {
+          brf.tryCommitBlockingResponse(reqCtx.getTraceSegment(), rba);
+          t = new BlockingException("Blocked request (multipart file upload)");
+          reqCtx.getTraceSegment().effectivelyBlocked();
+        }
+      }
+    }
+  }
+}

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

@@ -21,6 +21,9 @@ dependencies {
   implementation(group: 'com.fasterxml.jackson.core', name: 'jackson-databind', version: '2.6.0')
   implementation group: 'com.h2database', name: 'h2', version: '2.1.212'
 
+  // file upload
+  implementation group: 'commons-fileupload', name: 'commons-fileupload', version: '1.5'
+
   // ssrf
   implementation group: 'commons-httpclient', name: 'commons-httpclient', version: '2.0'
   implementation group: 'org.apache.httpcomponents', name: 'httpclient', version: '4.0'

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

@@ -20,7 +20,6 @@ com.fasterxml.jackson.core:jackson-databind:2.13.0=compileClasspath,runtimeClass
 com.fasterxml.jackson.datatype:jackson-datatype-jdk8:2.13.0=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
 com.fasterxml.jackson.datatype:jackson-datatype-jsr310:2.13.0=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
 com.fasterxml.jackson.module:jackson-module-parameter-names:2.13.0=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
-com.fasterxml.jackson:jackson-bom:2.13.0=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
 com.github.javaparser:javaparser-core:3.25.6=codenarc
 com.github.jnr:jffi:1.3.14=testRuntimeClasspath
 com.github.jnr:jnr-a64asm:1.0.0=testRuntimeClasspath
@@ -48,9 +47,9 @@ com.squareup.okio:okio:1.17.5=testCompileClasspath,testRuntimeClasspath
 com.squareup.okio:okio:1.6.0=compileClasspath,runtimeClasspath
 com.thoughtworks.qdox:qdox:1.12.1=codenarc
 commons-codec:commons-codec:1.3=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
-commons-fileupload:commons-fileupload:1.5=testCompileClasspath,testRuntimeClasspath
+commons-fileupload:commons-fileupload:1.5=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
 commons-httpclient:commons-httpclient:2.0=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
-commons-io:commons-io:2.11.0=testCompileClasspath,testRuntimeClasspath
+commons-io:commons-io:2.11.0=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
 commons-io:commons-io:2.20.0=spotbugs
 commons-lang:commons-lang:1.0.1=compileClasspath,runtimeClasspath,testCompileClasspath,testRuntimeClasspath
 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

@@ -4,8 +4,9 @@
 import java.lang.reflect.Field;
 import org.springframework.boot.SpringApplication;
 import org.springframework.boot.autoconfigure.SpringBootApplication;
+import org.springframework.boot.autoconfigure.web.servlet.MultipartAutoConfiguration;
 
-@SpringBootApplication
+@SpringBootApplication(exclude = MultipartAutoConfiguration.class)
 public class SpringbootApplication {
 
   public static void main(final String[] args) {

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

@@ -20,9 +20,15 @@
 import java.nio.file.Paths;
 import java.sql.Connection;
 import java.sql.DriverManager;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.Map;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpSession;
+import org.apache.commons.fileupload.FileItem;
+import org.apache.commons.fileupload.FileUploadException;
+import org.apache.commons.fileupload.disk.DiskFileItemFactory;
+import org.apache.commons.fileupload.servlet.ServletFileUpload;
 import org.apache.commons.httpclient.HttpClient;
 import org.apache.commons.httpclient.HttpMethod;
 import org.apache.commons.httpclient.methods.GetMethod;
@@ -272,6 +278,25 @@ public ResponseEntity<String> exceedResponseHeaders() {
     return new ResponseEntity<>("Custom headers added", headers, HttpStatus.OK);
   }
 
+  @PostMapping(value = "/upload", consumes = "multipart/form-data")
+  public ResponseEntity<String> upload(HttpServletRequest request) {
+    if (!ServletFileUpload.isMultipartContent(request)) {
+      return ResponseEntity.badRequest().body("Not a multipart request");
+    }
+    try {
+      List<FileItem> items = new ServletFileUpload(new DiskFileItemFactory()).parseRequest(request);
+      List<String> names = new ArrayList<>();
+      for (FileItem item : items) {
+        if (!item.isFormField()) {
+          names.add(item.getName());
+        }
+      }
+      return ResponseEntity.ok("Uploaded: " + names);
+    } catch (FileUploadException e) {
+      return ResponseEntity.status(500).body("Upload error: " + e.getMessage());
+    }
+  }
+
   @PostMapping("/waf-event-with-body")
   public String wafEventWithBody(@RequestBody String body) {
     return "EXECUTED";

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

@@ -1 +1,2 @@
 logging.level.root=WARN
+spring.servlet.multipart.enabled=false

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

@@ -210,6 +210,26 @@ class SpringBootSmokeTest extends AbstractAppSecServerSmokeTest {
           transformers: [],
           on_match    : ['block']
         ],
+        [
+          id          : '__test_file_upload_block',
+          name        : 'test rule to block on malicious file upload filename',
+          tags        : [
+            type      : 'unrestricted-file-upload',
+            category  : 'attack_attempt',
+            confidence: '1',
+          ],
+          conditions  : [
+            [
+              parameters: [
+                inputs: [[address: 'server.request.body.filenames']],
+                regex : '\\.(?:jsp|php|asp|aspx)$',
+              ],
+              operator  : 'match_regex',
+            ]
+          ],
+          transformers: [],
+          on_match    : ['block']
+        ],
         [
           id  : "apiA-100-001",
           name: "API 10 tag rule on request headers",
@@ -559,6 +579,38 @@ class SpringBootSmokeTest extends AbstractAppSecServerSmokeTest {
     }
   }
 
+  void 'block request based on malicious file upload filename'() {
+    when:
+    String url = "http://localhost:${httpPort}/upload"
+    def requestBody = new okhttp3.MultipartBody.Builder()
+      .setType(okhttp3.MultipartBody.FORM)
+      .addFormDataPart('file', 'exploit.jsp',
+      RequestBody.create(MediaType.parse('application/octet-stream'), 'webshell content'))
+      .build()
+    def request = new Request.Builder()
+      .url(url)
+      .post(requestBody)
+      .build()
+    def response = client.newCall(request).execute()
+    def responseBodyStr = response.body().string()
+
+    then:
+    responseBodyStr.contains("blocked")
+    response.code() == 403
+
+    when:
+    waitForTraceCount(1) == 1
+
+    then:
+    rootSpans.size() == 1
+    forEachRootSpanTrigger {
+      assert it['rule']['id'] == '__test_file_upload_block'
+    }
+    rootSpans.each {
+      assert it.meta.get('appsec.blocked') != null, 'appsec.blocked is not set'
+    }
+  }
+
   void 'rasp reports stacktrace on sql injection'() {
     when:
     String url = "http://localhost:${httpPort}/sqli/query?id=' OR 1=1 --"