WW-5501 Excludes malicious names

apache · Dec 23, 2024 · 76cd96a · 76cd96a
1 parent 0bcb651
commit 76cd96a
Show file tree

Hide file tree

Showing 9 changed files with 124 additions and 6 deletions.
diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java
@@ -31,6 +31,7 @@
 import org.apache.logging.log4j.Logger;
 import org.apache.struts2.StrutsConstants;
 import org.apache.struts2.dispatcher.LocalizedMessage;
+import org.apache.struts2.security.NotExcludedAcceptedPatternsChecker;
 
 import java.io.IOException;
 import java.nio.charset.Charset;
@@ -107,6 +108,8 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest {
      */
     protected Map<String, List<String>> parameters = new HashMap<>();
 
+    protected NotExcludedAcceptedPatternsChecker patternsChecker;
+
     /**
      * @param bufferSize Sets the buffer size to be used.
      */
@@ -180,6 +183,11 @@ protected Charset readCharsetEncoding(HttpServletRequest request) {
         return Charset.forName(charsetStr);
     }
 
+    @Inject
+    public void setNotExcludedAllowedPatternsChecker(NotExcludedAcceptedPatternsChecker patternsChecker) {
+        this.patternsChecker = patternsChecker;
+    }
+
     /**
      * Creates an instance of {@link JakartaServletDiskFileUpload} used by the parser to extract uploaded files
      *
@@ -413,4 +421,8 @@ public void cleanUp() {
         }
     }
 
+    protected boolean isAccepted(String fileName) {
+        return patternsChecker.isAllowed(fileName).isAllowed();
+    }
+
 }
diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java
@@ -77,6 +77,11 @@ protected JakartaServletDiskFileUpload createJakartaFileUpload(Charset charset,
     protected void processNormalFormField(DiskFileItem item, Charset charset) throws IOException {
         LOG.debug("Item: {} is a normal form field", item.getName());
 
+        if (!isAccepted(item.getFieldName())) {
+            LOG.warn("Form field name [{}] is not accepted", sanitizeNewlines(item.getFieldName()));
+            return;
+        }
+
         List<String> values;
         String fieldName = item.getFieldName();
         if (parameters.get(fieldName) != null) {
@@ -98,6 +103,16 @@ protected void processNormalFormField(DiskFileItem item, Charset charset) throws
     }
 
     protected void processFileField(DiskFileItem item) {
+        if (!isAccepted(item.getName())) {
+            LOG.warn("File name [{}] is not accepted", sanitizeNewlines(item.getName()));
+            return;
+        }
+
+        if (!isAccepted(item.getFieldName())) {
+            LOG.warn("Field name [{}] is not accepted", sanitizeNewlines(item.getFieldName()));
+            return;
+        }
+
         // Skip file uploads that don't have a file name - meaning that no file was selected.
         if (item.getName() == null || item.getName().trim().isEmpty()) {
             LOG.debug(() -> "No file has been uploaded for the field: " + sanitizeNewlines(item.getFieldName()));

diff --git a/.../src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java b/.../src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java
@@ -191,6 +191,11 @@ protected void processFileItemAsFileField(FileItemInput fileItemInput, Path loca
             return;
         }
 
+        if (!isAccepted(fileItemInput.getName())) {
+            LOG.warn("File field [{}] rejected", fileItemInput.getName());
+            return;
+        }
+
         if (exceedsMaxFiles(fileItemInput)) {
             return;
         }

diff --git a/core/src/main/java/org/apache/struts2/security/DefaultExcludedPatternsChecker.java b/core/src/main/java/org/apache/struts2/security/DefaultExcludedPatternsChecker.java
@@ -36,7 +36,8 @@ public class DefaultExcludedPatternsChecker implements ExcludedPatternsChecker {
     private static final Logger LOG = LogManager.getLogger(DefaultExcludedPatternsChecker.class);
 
     public static final String[] EXCLUDED_PATTERNS = {
-            "(^|\\%\\{)((#?)(top(\\.|\\['|\\[\")|\\[\\d\\]\\.)?)(dojo|struts|session|request|response|application|servlet(Request|Response|Context)|parameters|context|_memberAccess)(\\.|\\[).*",
+            "(^|\\%\\{)(#?top\\.)[^\\s]*",
+            "(^|\\%\\{)((#?)(\\[\\d\\]\\.)?)(dojo|struts|session|request|response|application|servlet(Request|Response|Context)|parameters|context|_memberAccess)(\\.|\\[).*",
             ".*(^|\\.|\\[|\\'|\"|get)class(\\(\\.|\\[|\\'|\").*",
             "actionErrors|actionMessages|fieldErrors"
     };

diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java
@@ -19,7 +19,13 @@
 package org.apache.struts2.dispatcher.multipart;
 
 import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload;
+import org.apache.struts2.config.Configuration;
+import org.apache.struts2.config.ConfigurationManager;
+import org.apache.struts2.dispatcher.Dispatcher;
 import org.apache.struts2.dispatcher.LocalizedMessage;
+import org.apache.struts2.inject.Container;
+import org.apache.struts2.util.StrutsTestCaseHelper;
+import org.apache.struts2.views.jsp.StrutsMockServletContext;
 import org.assertj.core.api.InstanceOfAssertFactories;
 import org.junit.After;
 import org.junit.Before;
@@ -31,6 +37,7 @@
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -47,6 +54,7 @@ abstract class AbstractMultiPartRequestTest {
     protected final String endline = "\r\n";
 
     protected AbstractMultiPartRequest multiPart;
+    protected Container container;
 
     abstract protected AbstractMultiPartRequest createMultipartRequest();
 
@@ -59,7 +67,13 @@ public static void beforeClass() throws IOException {
     }
 
     @Before
-    public void before() {
+    public void before() throws Exception {
+        StrutsMockServletContext servletContext = new StrutsMockServletContext();
+        Dispatcher dispatcher = StrutsTestCaseHelper.initDispatcher(servletContext, Collections.emptyMap());
+        ConfigurationManager configurationManager = dispatcher.getConfigurationManager();
+        Configuration configuration = configurationManager.getConfiguration();
+        container = configuration.getContainer();
+
         mockRequest = new MockHttpServletRequest();
         mockRequest.setCharacterEncoding(StandardCharsets.UTF_8.name());
         mockRequest.setMethod("post");

diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java
@@ -18,11 +18,15 @@
  */
 package org.apache.struts2.dispatcher.multipart;
 
+import org.apache.struts2.security.DefaultNotExcludedAcceptedPatternsChecker;
+
 public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest {
 
     @Override
     protected AbstractMultiPartRequest createMultipartRequest() {
-        return new JakartaMultiPartRequest();
+        JakartaMultiPartRequest multiPartRequest = new JakartaMultiPartRequest();
+        multiPartRequest.setNotExcludedAllowedPatternsChecker(container.inject(DefaultNotExcludedAcceptedPatternsChecker.class));
+        return multiPartRequest;
     }
 
 }
diff --git a/.../test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java b/.../test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java
@@ -20,6 +20,7 @@
 
 import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload;
 import org.apache.struts2.dispatcher.LocalizedMessage;
+import org.apache.struts2.security.DefaultNotExcludedAcceptedPatternsChecker;
 import org.assertj.core.api.InstanceOfAssertFactories;
 import org.junit.Test;
 
@@ -32,7 +33,9 @@ public class JakartaStreamMultiPartRequestTest extends AbstractMultiPartRequestT
 
     @Override
     protected AbstractMultiPartRequest createMultipartRequest() {
-        return new JakartaStreamMultiPartRequest();
+        JakartaStreamMultiPartRequest multiPartRequest = new JakartaStreamMultiPartRequest();
+        multiPartRequest.setNotExcludedAllowedPatternsChecker(container.inject(DefaultNotExcludedAcceptedPatternsChecker.class));
+        return multiPartRequest;
     }
 
     @Test

diff --git a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java
@@ -24,6 +24,7 @@
 import org.apache.struts2.ValidationAwareSupport;
 import org.apache.struts2.mock.MockActionInvocation;
 import org.apache.struts2.mock.MockActionProxy;
+import org.apache.struts2.security.DefaultNotExcludedAcceptedPatternsChecker;
 import org.apache.struts2.util.ClassLoaderUtil;
 import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload;
 import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletFileUpload;
@@ -514,11 +515,71 @@ public void testMultipartRequestLocalizedError() throws Exception {
         assertTrue(msg.startsWith("Der Request übertraf die maximal erlaubte Größe"));
     }
 
+    public void testUnacceptedFieldName() throws Exception {
+        request.setCharacterEncoding(StandardCharsets.UTF_8.name());
+        request.setMethod("post");
+        request.addHeader("Content-type", "multipart/form-data; boundary=---1234");
+
+        // inspired by the unit tests for jakarta commons fileupload
+        String content = ("-----1234\r\n" +
+            "Content-Disposition: form-data; name=\"top.file\"; filename=\"deleteme.txt\"\r\n" +
+            "Content-Type: text/html\r\n" +
+            "\r\n" +
+            "Unit test of ActionFileUploadInterceptor" +
+            "\r\n" +
+            "-----1234--\r\n");
+        request.setContent(content.getBytes(StandardCharsets.US_ASCII));
+
+        MyFileUploadAction action = container.inject(MyFileUploadAction.class);
+
+        MockActionInvocation mai = new MockActionInvocation();
+        mai.setAction(action);
+        mai.setResultCode("success");
+        mai.setInvocationContext(ActionContext.getContext());
+        ActionContext.getContext()
+            .withServletRequest(createMultipartRequestMaxSize(2000));
+
+        interceptor.intercept(mai);
+
+        assertFalse(action.hasActionErrors());
+        assertNull(action.getUploadFiles());
+    }
+
+    public void testUnacceptedFileName() throws Exception {
+        request.setCharacterEncoding(StandardCharsets.UTF_8.name());
+        request.setMethod("post");
+        request.addHeader("Content-type", "multipart/form-data; boundary=---1234");
+
+        // inspired by the unit tests for jakarta commons fileupload
+        String content = ("-----1234\r\n" +
+            "Content-Disposition: form-data; name=\"file\"; filename=\"../deleteme.txt\"\r\n" +
+            "Content-Type: text/html\r\n" +
+            "\r\n" +
+            "Unit test of ActionFileUploadInterceptor" +
+            "\r\n" +
+            "-----1234--\r\n");
+        request.setContent(content.getBytes(StandardCharsets.US_ASCII));
+
+        MyFileUploadAction action = container.inject(MyFileUploadAction.class);
+
+        MockActionInvocation mai = new MockActionInvocation();
+        mai.setAction(action);
+        mai.setResultCode("success");
+        mai.setInvocationContext(ActionContext.getContext());
+        ActionContext.getContext()
+            .withServletRequest(createMultipartRequestMaxSize(2000));
+
+        interceptor.intercept(mai);
+
+        assertFalse(action.hasActionErrors());
+        assertNull(action.getUploadFiles());
+    }
+
     private String encodeTextFile(String filename, String contentType, String content) {
         return endline +
                 "--" + boundary +
                 endline +
-                "Content-Disposition: form-data; name=\"" + "file" + "\"; filename=\"" + filename +
+                "Content-Disposition: form-data; name=\"" + "file" + "\"; filename=\"" + filename + "\"" +
                 endline +
                 "Content-Type: " + contentType +
                 endline +
@@ -549,6 +610,9 @@ private MultiPartRequestWrapper createMultipartRequest(int maxsize, int maxfiles
         jak.setMaxFiles(String.valueOf(maxfiles));
         jak.setMaxStringLength(String.valueOf(maxStringLength));
         jak.setDefaultEncoding(StandardCharsets.UTF_8.name());
+        DefaultNotExcludedAcceptedPatternsChecker patternsChecker = container.inject(DefaultNotExcludedAcceptedPatternsChecker.class);
+        jak.setNotExcludedAllowedPatternsChecker(patternsChecker);
+
         return new MultiPartRequestWrapper(jak, request, tempDir.getAbsolutePath(), new DefaultLocaleProvider());
     }
 

diff --git a/core/src/test/java/org/apache/struts2/security/DefaultExcludedPatternsCheckerTest.java b/core/src/test/java/org/apache/struts2/security/DefaultExcludedPatternsCheckerTest.java
@@ -93,7 +93,7 @@ public void testHardcodedPatterns() throws Exception {
 
     public void testDefaultExcludePatterns() throws Exception {
         // given
-        List<String> prefixes = Arrays.asList("#[0].%s", "[0].%s", "top.%s", "%{[0].%s}", "%{#[0].%s}", "%{top.%s}", "%{#top.%s}", "%{#%s}", "%{%s}", "#%s");
+        List<String> prefixes = Arrays.asList("#[0].%s", "[0].%s", "top.%s", "%{[0].%s}", "%{#[0].%s}", "%{top.%s}", "%{#top.%s}", "%{#%s}", "%{%s}", "#%s", "top.param", "%{top.request}", "#top.param");
         List<String> inners = Arrays.asList("servletRequest", "servletResponse", "servletContext", "application", "session", "struts", "request", "response", "dojo", "parameters");
         List<String> suffixes = Arrays.asList("['test']", "[\"test\"]", ".test");
-Original file line number
+Diff line change
@@ Expand Up @@
                 return;
             }
+            if (!isAccepted(fileItemInput.getName())) {
+                LOG.warn("File field [{}] rejected", fileItemInput.getName());
+                return;
+            }
             if (exceedsMaxFiles(fileItemInput)) {
                 return;
             }
@@ Expand Down @@