WW-5501 Only exclude malicious file names

apache · Jan 16, 2025 · 3d3b076 · 3d3b076
1 parent b5d94a1
commit 3d3b076
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 16 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;
+
+    public 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/interceptor/ActionFileUploadInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java
@@ -610,8 +610,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());
     }