From 552f7eba77245b6ac65df714fec7f1c1643a4e9a 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 | 12 +++- .../DefaultExcludedPatternsChecker.java | 3 +- .../AbstractMultiPartRequestTest.java | 35 +++++++++- .../JakartaMultiPartRequestTest.java | 6 +- .../JakartaStreamMultiPartRequestTest.java | 5 +- .../ActionFileUploadInterceptorTest.java | 66 ++++++++++++++++++- .../DefaultExcludedPatternsCheckerTest.java | 2 +- 9 files changed, 149 insertions(+), 7 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 ccb25e053..38852a2f2 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.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; @@ -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 @@ 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 * @@ -413,4 +421,8 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { } } + 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 3a76adc40..dd7428656 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 @@ 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())) { + LOG.warn("Form field [{}] is rejected!", sanitizeNewlines(item.getFieldName())); + return; + } + List values; String fieldName = item.getFieldName(); if (parameters.get(fieldName) != null) { @@ -98,6 +103,16 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { } 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 cebd60250..50544d493 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 @@ -114,6 +114,11 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { String fieldName = fileItemInput.getFieldName(); String fieldValue = readStream(fileItemInput.getInputStream()); + if (!isAccepted(fieldName)) { + LOG.warn("Form field [{}] is rejected!", sanitizeNewlines(fieldName)); + return; + } + if (exceedsMaxStringLength(fieldName, fieldValue)) { return; } @@ -191,6 +196,11 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { return; } + if (!isAccepted(fileItemInput.getName())) { + LOG.warn("File field [{}] rejected", sanitizeNewlines(fileItemInput.getName())); + return; + } + if (exceedsMaxFiles(fileItemInput)) { return; } @@ -230,7 +240,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { InputStream input = fileItemInput.getInputStream(); try (OutputStream output = new BufferedOutputStream(Files.newOutputStream(file.toPath()), bufferSize)) { byte[] buffer = new byte[bufferSize]; - LOG.debug("Streaming file: {} using buffer size: {}", fileItemInput.getName(), bufferSize); + LOG.debug("Streaming file: {} using buffer size: {}", sanitizeNewlines(fileItemInput.getName()), bufferSize); for (int length; ((length = input.read(buffer)) > 0); ) { output.write(buffer, 0, length); } 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 824d98c58..6b7f5eed4 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 22625cdf9..942ffe01a 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.File; 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 @@ abstract class AbstractMultiPartRequestTest { } @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"); @@ -492,6 +506,25 @@ abstract class AbstractMultiPartRequestTest { .containsExactly("struts.messages.upload.error.FileUploadException"); } + @Test + public void maliciousFields() throws IOException { + String content = formFile("file1", "test1.csv", "1,2,3,4") + + formField("top.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() + .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 781b7fbd0..efa0ff9b0 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 aa54316f5..532be8a38 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 @@ 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; @@ -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 93e52ab11..9aca62717 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.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; @@ -514,11 +515,71 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { 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 @@ 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()); } 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 1f549e011..06889302a 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 class DefaultExcludedPatternsCheckerTest extends XWorkTestCase { 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");