From 76cd96ab33e10b5ce63545505301b45a15adfc19 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 23 Dec 2024 11:43:47 +0100 Subject: [PATCH] WW-5501 Excludes malicious names --- .../multipart/AbstractMultiPartRequest.java | 12 ++++ .../multipart/JakartaMultiPartRequest.java | 15 +++++ .../JakartaStreamMultiPartRequest.java | 5 ++ .../DefaultExcludedPatternsChecker.java | 3 +- .../AbstractMultiPartRequestTest.java | 16 ++++- .../JakartaMultiPartRequestTest.java | 6 +- .../JakartaStreamMultiPartRequestTest.java | 5 +- .../ActionFileUploadInterceptorTest.java | 66 ++++++++++++++++++- .../DefaultExcludedPatternsCheckerTest.java | 2 +- 9 files changed, 124 insertions(+), 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 index ccb25e0535..38852a2f23 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 @@ -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> 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 index 3a76adc40e..0743e9a65d 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 @@ -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 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/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java index cebd60250f..2c4ca92661 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 @@ -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 index 824d98c58a..6b7f5eed49 100644 --- 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 index 22625cdf9d..6186208c1a 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 @@ -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 index 781b7fbd0e..efa0ff9b03 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,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/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java index aa54316f5a..532be8a385 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,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 index 93e52ab11e..9aca62717b 100644 --- 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 index 1f549e0110..06889302a5 100644 --- 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 prefixes = Arrays.asList("#[0].%s", "[0].%s", "top.%s", "%{[0].%s}", "%{#[0].%s}", "%{top.%s}", "%{#top.%s}", "%{#%s}", "%{%s}", "#%s"); + List 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 inners = Arrays.asList("servletRequest", "servletResponse", "servletContext", "application", "session", "struts", "request", "response", "dojo", "parameters"); List suffixes = Arrays.asList("['test']", "[\"test\"]", ".test");