Merge pull request #1184 from apache/fix/WW-5501-exclude-s7

WW-5501 Only exclude malicious file names
apache · Jan 16, 2025 · fa60e1c · fa60e1c
2 parents 71bca19 + 4279511
commit fa60e1c
Show file tree

Hide file tree

Showing 7 changed files with 47 additions and 27 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
@@ -19,6 +19,8 @@
 package org.apache.struts2.dispatcher.multipart;
 
 import org.apache.struts2.inject.Inject;
+import org.apache.struts2.security.DefaultExcludedPatternsChecker;
+import org.apache.struts2.security.ExcludedPatternsChecker;
 import jakarta.servlet.http.HttpServletRequest;
 import org.apache.commons.fileupload2.core.FileUploadByteCountLimitException;
 import org.apache.commons.fileupload2.core.FileUploadContentTypeException;
@@ -31,7 +33,6 @@
 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;
@@ -53,6 +54,8 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest {
 
     private static final Logger LOG = LogManager.getLogger(AbstractMultiPartRequest.class);
 
+    private static final String EXCLUDED_FILE_PATTERN = ".*[<>&\"'|;\\\\/?*:]+.*|.*\\.\\..*";
+
     /**
      * Defines the internal buffer size used during streaming operations.
      */
@@ -108,7 +111,13 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest {
      */
     protected Map<String, List<String>> parameters = new HashMap<>();
 
-    protected NotExcludedAcceptedPatternsChecker patternsChecker;
+
+    private final ExcludedPatternsChecker patternsChecker;
+
+    protected AbstractMultiPartRequest() {
+        patternsChecker = new DefaultExcludedPatternsChecker();
+        ((DefaultExcludedPatternsChecker) patternsChecker).setAdditionalExcludePatterns(EXCLUDED_FILE_PATTERN);
+    }
 
     /**
      * @param bufferSize Sets the buffer size to be used.
@@ -183,11 +192,6 @@ 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
      *
@@ -425,8 +429,12 @@ public void cleanUp() {
         }
     }
 
-    protected boolean isAccepted(String fileName) {
-        return patternsChecker.isAllowed(fileName).isAllowed();
+    /**
+     * @param fileName file name to check
+     * @return true if the file name is excluded
+     */
+    protected boolean isExcluded(String fileName) {
+        return patternsChecker.isExcluded(fileName).isExcluded();
     }
 
 }
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
@@ -79,7 +79,7 @@ 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())) {
+        if (isExcluded(item.getFieldName())) {
             LOG.warn(() -> "Form field [%s] is rejected!".formatted(normalizeSpace(item.getFieldName())));
             return;
         }
@@ -105,12 +105,12 @@ protected void processNormalFormField(DiskFileItem item, Charset charset) throws
     }
 
     protected void processFileField(DiskFileItem item) {
-        if (!isAccepted(item.getName())) {
+        if (isExcluded(item.getName())) {
             LOG.warn(() -> "File name [%s] is not accepted".formatted(normalizeSpace(item.getName())));
             return;
         }
 
-        if (!isAccepted(item.getFieldName())) {
+        if (isExcluded(item.getFieldName())) {
             LOG.warn(() -> "Field name [%s] is not accepted".formatted(normalizeSpace(item.getFieldName())));
             return;
         }

diff --git a/.../src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java b/.../src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java
@@ -116,7 +116,7 @@ protected void processFileItemAsFormField(FileItemInput fileItemInput) throws IO
         String fieldName = fileItemInput.getFieldName();
         String fieldValue = readStream(fileItemInput.getInputStream());
 
-        if (!isAccepted(fieldName)) {
+        if (isExcluded(fieldName)) {
             LOG.warn(() -> "Form field [%s] is rejected!".formatted(normalizeSpace(fieldName)));
             return;
         }
@@ -198,7 +198,7 @@ protected void processFileItemAsFileField(FileItemInput fileItemInput, Path loca
             return;
         }
 
-        if (!isAccepted(fileItemInput.getName())) {
+        if (isExcluded(fileItemInput.getName())) {
             LOG.warn(() -> "File field [%s] rejected".formatted(normalizeSpace(fileItemInput.getName())));
             return;
         }

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
@@ -525,6 +525,28 @@ public void maliciousFields() throws IOException {
                 .isEmpty();
     }
 
+    @Test
+    public void maliciousFilename() throws IOException {
+        String content = formFile("file1", "../test1.csv", "1,2,3,4") +
+                formField("param", "expression") +
+                endline + "--" + boundary + "--";
+
+        mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8));
+
+        assertThat(JakartaServletDiskFileUpload.isMultipartContent(mockRequest)).isTrue();
+
+        multiPart.parse(mockRequest, tempDir);
+
+        assertThat(multiPart.getErrors())
+                .isEmpty();
+
+        assertThat(multiPart.getParameterNames().asIterator()).toIterable()
+                .hasSize(1);
+        assertThat(multiPart.getParameterNames().asIterator()).toIterable()
+                .containsOnly("param");
+        assertThat(multiPart.getFileNames("file1")).isEmpty();
+    }
+
     protected String formFile(String fieldName, String filename, String content) {
         return endline +
                 "--" + boundary + endline +

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,15 +18,11 @@
  */
 package org.apache.struts2.dispatcher.multipart;
 
-import org.apache.struts2.security.DefaultNotExcludedAcceptedPatternsChecker;
-
 public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest {
 
     @Override
     protected AbstractMultiPartRequest createMultipartRequest() {
-        JakartaMultiPartRequest multiPartRequest = new JakartaMultiPartRequest();
-        multiPartRequest.setNotExcludedAllowedPatternsChecker(container.inject(DefaultNotExcludedAcceptedPatternsChecker.class));
-        return multiPartRequest;
+        return new JakartaMultiPartRequest();
     }
 
 }
diff --git a/.../test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java b/.../test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java
@@ -20,7 +20,6 @@
 
 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;
 
@@ -33,9 +32,7 @@ public class JakartaStreamMultiPartRequestTest extends AbstractMultiPartRequestT
 
     @Override
     protected AbstractMultiPartRequest createMultipartRequest() {
-        JakartaStreamMultiPartRequest multiPartRequest = new JakartaStreamMultiPartRequest();
-        multiPartRequest.setNotExcludedAllowedPatternsChecker(container.inject(DefaultNotExcludedAcceptedPatternsChecker.class));
-        return multiPartRequest;
+        return new JakartaStreamMultiPartRequest();
     }
 
     @Test
@@ -51,7 +48,7 @@ public void maxSizeOfFiles() throws IOException {
 
         // when
         multiPart.setMaxSizeOfFiles("10");
-        multiPart.parse(mockRequest, tempDir.toString());
+        multiPart.parse(mockRequest, tempDir);
 
         // then
         assertThat(multiPart.uploadedFiles)

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,7 +24,6 @@
 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;
@@ -610,8 +609,6 @@ 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());
     }