This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a commit to branch feature/WW-5501-exclude-s7
in repository https://gitbox.apache.org/repos/asf/struts.git
commit a10dc21f14c04d3a469275d37fc170bbed260978
Author: Lukasz Lenart <lukaszlen...@apache.org>
AuthorDate: Mon Dec 23 11:43:47 2024 +0100
WW-5501 Excludes malicious names
---
.../multipart/AbstractMultiPartRequest.java | 12 ++++
.../multipart/JakartaMultiPartRequest.java | 15 +++++
.../multipart/JakartaStreamMultiPartRequest.java | 5 ++
.../security/DefaultExcludedPatternsChecker.java | 3 +-
.../multipart/AbstractMultiPartRequestTest.java | 16 +++++-
.../multipart/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 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<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();
+ }
+
}
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..ffb3a48d5 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 name [{}] is not accepted",
item.getFieldName().replaceAll("[\\r\\n]", ""));
+ 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",
item.getName().replaceAll("[\\r\\n]", ""));
+ return;
+ }
+
+ if (!isAccepted(item.getFieldName())) {
+ LOG.warn("Field name [{}] is not accepted",
item.getFieldName().replaceAll("[\\r\\n]", ""));
+ 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..2c4ca9266 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 @@ public class JakartaStreamMultiPartRequest extends
AbstractMultiPartRequest {
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 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..6186208c1 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");
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<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");
