Apache/struts
1
0
Fork 0
mirror of https://github.com/apache/struts.git synced 2026-08-07 07:37:20 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #1157 from apache/feature/WW-5501-exclude-s7

Browse Source 
WW-5501 Excludes malicious names
This commit is contained in:
Lukasz Lenart
2024-12-23 15:09:05 +01:00
committed by GitHub
parent 0bcb6516bf bbdb38dedc
commit 36ca20d3dc
9 changed files with 169 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files
core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java
+16
View File
@@ -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<String, List<String>> 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();
}
}
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java
+21 -4
View File
@@ -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<String> 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())
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java
+21 -7
View File
@@ -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);
}
core/src/main/java/org/apache/struts2/security/DefaultExcludedPatternsChecker.java
+2 -1
View File
@@ -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"
};
core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java
+34 -1
View File
@@ -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 +
core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequestTest.java
+5 -1
View File
@@ -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;
}
}
core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java
+4 -1
View File
@@ -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
core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java
+65 -1
View File
@@ -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());
}
core/src/test/java/org/apache/struts2/security/DefaultExcludedPatternsCheckerTest.java
+1 -1
View File
@@ -93,7 +93,7 @@ public class DefaultExcludedPatternsCheckerTest extends XWorkTestCase {
public void testDefaultExcludePatterns() throws Exception {
// given
List<String> prefixes = Arrays.asList("#[0].%s", "[0].%s", "top.%s", "%{[0].%s}", "%{#[0].%s}", "%{top.%s}", "%{#top.%s}", "%{#%s}", "%{%s}", "#%s");
List<String> 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<String> inners = Arrays.asList("servletRequest", "servletResponse", "servletContext", "application", "session", "struts", "request", "response", "dojo", "parameters");
List<String> suffixes = Arrays.asList("['test']", "[\"test\"]", ".test");