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 index aeace3120..cceef2cab 100644 --- 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.LogManager; 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> 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 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { 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 abstract class AbstractMultiPartRequest implements MultiPartRequest { } } - 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 index b36e6e2f8..737ba23f8 100644 --- 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 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { 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 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { } 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/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java index 7fb44f21f..2d2b67c68 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java @@ -116,7 +116,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { 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 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { 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 index 942ffe01a..29143f7ef 100644 --- 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 @@ abstract class AbstractMultiPartRequestTest { .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 index efa0ff9b0..781b7fbd0 100644 --- 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/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java index 532be8a38..7a8ea19d9 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java @@ -20,7 +20,6 @@ package org.apache.struts2.dispatcher.multipart; 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 class JakartaStreamMultiPartRequestTest extends AbstractMultiPartRequestT // 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 index 9aca62717..ab0132351 100644 --- 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.locale.DefaultLocaleProvider; 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 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { 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()); }