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

WW-5501 Excludes malicious names

Browse Source
This commit is contained in:
Lukasz Lenart
2024-12-23 11:43:47 +01:00
parent 0bcb6516bf
commit 552f7eba77
9 changed files with 149 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files
core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java
+12
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
*
@@ -413,4 +421,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
+15
View File
@@ -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<String> 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()));
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java
+11 -1
View File
@@ -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);
}
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");