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..aeace3120 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 * @@ -296,6 +304,10 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { return fileName; } + /** + * @deprecated since 7.0.1, use {@link StringUtils#normalizeSpace(String)} instead + */ + @Deprecated protected String sanitizeNewlines(String before) { return before.replaceAll("\\R", "_"); } @@ -413,4 +425,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..b36e6e2f8 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 @@ -32,6 +32,8 @@ import java.nio.file.Path; import java.util.ArrayList; import java.util.List; +import static org.apache.commons.lang3.StringUtils.normalizeSpace; + /** * Multipart form data request adapter for Jakarta Commons FileUpload package. */ @@ -47,11 +49,11 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { prepareServletFileUpload(charset, Path.of(saveDir)); for (DiskFileItem item : servletFileUpload.parseRequest(request)) { - LOG.debug(() -> "Processing a form field: " + sanitizeNewlines(item.getFieldName())); + LOG.debug(() -> "Processing a form field: " + normalizeSpace(item.getFieldName())); if (item.isFormField()) { processNormalFormField(item, charset); } else { - LOG.debug(() -> "Processing a file: " + sanitizeNewlines(item.getFieldName())); + LOG.debug(() -> "Processing a file: " + normalizeSpace(item.getFieldName())); processFileField(item); } } @@ -77,6 +79,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 [%s] is rejected!".formatted(normalizeSpace(item.getFieldName()))); + return; + } + List values; String fieldName = item.getFieldName(); if (parameters.get(fieldName) != null) { @@ -98,9 +105,19 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { } protected void processFileField(DiskFileItem item) { + if (!isAccepted(item.getName())) { + LOG.warn(() -> "File name [%s] is not accepted".formatted(normalizeSpace(item.getName()))); + return; + } + + if (!isAccepted(item.getFieldName())) { + LOG.warn(() -> "Field name [%s] is not accepted".formatted(normalizeSpace(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())); + LOG.debug(() -> "No file has been uploaded for the field: " + normalizeSpace(item.getFieldName())); return; } @@ -112,7 +129,7 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { } if (item.isInMemory()) { - LOG.warn("Storing uploaded files just in memory isn't supported currently, skipping file: {}!", item.getName()); + LOG.warn(() -> "Storing uploaded files just in memory isn't supported currently, skipping file: %s!".formatted(normalizeSpace(item.getName()))); } else { UploadedFile uploadedFile = StrutsUploadedFile.Builder .create(item.getPath().toFile()) 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..7fb44f21f 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 @@ -42,6 +42,8 @@ import java.util.ArrayList; import java.util.List; import java.util.UUID; +import static org.apache.commons.lang3.StringUtils.normalizeSpace; + /** * Multi-part form data request adapter for Jakarta Commons FileUpload package that * leverages the streaming API rather than the traditional non-streaming API. @@ -71,10 +73,10 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { LOG.debug("Using Jakarta Stream API to process request"); servletFileUpload.getItemIterator(request).forEachRemaining(item -> { if (item.isFormField()) { - LOG.debug(() -> "Processing a form field: " + sanitizeNewlines(item.getFieldName())); + LOG.debug(() -> "Processing a form field: " + normalizeSpace(item.getFieldName())); processFileItemAsFormField(item); } else { - LOG.debug(() -> "Processing a file: " + sanitizeNewlines(item.getFieldName())); + LOG.debug(() -> "Processing a file: " + normalizeSpace(item.getFieldName())); processFileItemAsFileField(item, location); } }); @@ -114,6 +116,11 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { String fieldName = fileItemInput.getFieldName(); String fieldValue = readStream(fileItemInput.getInputStream()); + if (!isAccepted(fieldName)) { + LOG.warn(() -> "Form field [%s] is rejected!".formatted(normalizeSpace(fieldName))); + return; + } + if (exceedsMaxStringLength(fieldName, fieldValue)) { return; } @@ -141,7 +148,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { if (maxFiles != null && maxFiles == uploadedFiles.size()) { if (LOG.isDebugEnabled()) { LOG.debug("Cannot accept another file: {} as it will exceed max files: {}", - sanitizeNewlines(fileItemInput.getName()), maxFiles); + normalizeSpace(fileItemInput.getName()), maxFiles); } LocalizedMessage errorMessage = buildErrorMessage( FileUploadFileCountLimitException.class, @@ -160,7 +167,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { private void exceedsMaxSizeOfFiles(FileItemInput fileItemInput, File file, Long currentFilesSize) { if (LOG.isDebugEnabled()) { LOG.debug("File: {} of size: {} exceeds allowed max size: {}, actual size of already uploaded files: {}", - sanitizeNewlines(fileItemInput.getName()), file.length(), maxSizeOfFiles, currentFilesSize + normalizeSpace(fileItemInput.getName()), file.length(), maxSizeOfFiles, currentFilesSize ); } LocalizedMessage errorMessage = buildErrorMessage( @@ -174,7 +181,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { } if (!file.delete() && LOG.isWarnEnabled()) { LOG.warn("Cannot delete file: {} which exceeds maximum size: {} of all files!", - sanitizeNewlines(fileItemInput.getName()), maxSizeOfFiles); + normalizeSpace(fileItemInput.getName()), maxSizeOfFiles); } } @@ -187,7 +194,12 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { protected void processFileItemAsFileField(FileItemInput fileItemInput, Path location) throws IOException { // Skip file uploads that don't have a file name - meaning that no file was selected. if (fileItemInput.getName() == null || fileItemInput.getName().trim().isEmpty()) { - LOG.debug(() -> "No file has been uploaded for the field: " + sanitizeNewlines(fileItemInput.getFieldName())); + LOG.debug(() -> "No file has been uploaded for the field: " + normalizeSpace(fileItemInput.getFieldName())); + return; + } + + if (!isAccepted(fileItemInput.getName())) { + LOG.warn(() -> "File field [%s] rejected".formatted(normalizeSpace(fileItemInput.getName()))); return; } @@ -230,7 +242,9 @@ 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); + if (LOG.isDebugEnabled()) { + LOG.debug("Streaming file: {} using buffer size: {}", normalizeSpace(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");