This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a commit to branch feature/WW-5501-exclude
in repository https://gitbox.apache.org/repos/asf/struts.git
commit c537d2c48dafaf05e8e9c5302c740487c7238335
Author: Lukasz Lenart <lukaszlen...@apache.org>
AuthorDate: Mon Dec 23 11:00:29 2024 +0100
WW-5501 Exclude malicious names
---
.../security/DefaultExcludedPatternsChecker.java | 1 +
.../multipart/AbstractMultiPartRequest.java | 11 ++++
.../multipart/JakartaMultiPartRequest.java | 15 +++++
.../multipart/JakartaStreamMultiPartRequest.java | 9 +++
.../DefaultExcludedPatternsCheckerTest.java | 2 +-
.../ActionFileUploadInterceptorTest.java | 73 +++++++++++++++++++++-
.../interceptor/FileUploadInterceptorTest.java | 73 +++++++++++++++++++++-
7 files changed, 177 insertions(+), 7 deletions(-)
diff --git
a/core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java
b/core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java
index cf425d67c..fc96bb0f9 100644
---
a/core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java
+++
b/core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java
@@ -36,6 +36,7 @@ public class DefaultExcludedPatternsChecker implements
ExcludedPatternsChecker {
private static final Logger LOG =
LogManager.getLogger(DefaultExcludedPatternsChecker.class);
public static final String[] EXCLUDED_PATTERNS = {
+ "(^|\\%\\{)(#?top\\.)[^\\s]*",
"(^|\\%\\{)((#?)(top(\\.|\\['|\\[\")|\\[\\d\\]\\.)?)(dojo|struts|session|request|response|application|servlet(Request|Response|Context)|parameters|context|_memberAccess)(\\.|\\[).*",
".*(^|\\.|\\[|\\'|\"|get)class(\\(\\.|\\[|\\'|\").*",
"actionErrors|actionMessages|fieldErrors"
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 23d879ba4..ed013b724 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
@@ -20,6 +20,7 @@ package org.apache.struts2.dispatcher.multipart;
import com.opensymphony.xwork2.LocaleProviderFactory;
import com.opensymphony.xwork2.inject.Inject;
+import com.opensymphony.xwork2.security.NotExcludedAcceptedPatternsChecker;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.struts2.StrutsConstants;
@@ -79,6 +80,7 @@ public abstract class AbstractMultiPartRequest implements
MultiPartRequest {
* Localization to be used regarding errors.
*/
protected Locale defaultLocale = Locale.ENGLISH;
+ private NotExcludedAcceptedPatternsChecker patternsChecker;
/**
* @param bufferSize Sets the buffer size to be used.
@@ -121,6 +123,11 @@ public abstract class AbstractMultiPartRequest implements
MultiPartRequest {
defaultLocale =
localeProviderFactory.createLocaleProvider().getLocale();
}
+ @Inject
+ public void
setNotExcludedAllowedPatternsChecker(NotExcludedAcceptedPatternsChecker
patternsChecker) {
+ this.patternsChecker = patternsChecker;
+ }
+
/**
* @param request Inspect the servlet request and set the locale if one
wasn't provided by
* the Struts2 framework.
@@ -169,4 +176,8 @@ public abstract class AbstractMultiPartRequest implements
MultiPartRequest {
return fileName;
}
+ 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 c2cc07dbb..eafd332f1 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
@@ -113,6 +113,16 @@ public class JakartaMultiPartRequest extends
AbstractMultiPartRequest {
protected void processFileField(FileItem item) {
LOG.debug("Item is a file upload");
+ if (!isAccepted(item.getName())) {
+ LOG.warn("File name [{}] is not accepted", item.getName());
+ return;
+ }
+
+ if (!isAccepted(item.getFieldName())) {
+ LOG.warn("Field name [{}] is not accepted", 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()));
@@ -134,6 +144,11 @@ public class JakartaMultiPartRequest extends
AbstractMultiPartRequest {
try {
LOG.debug("Item is a normal form field");
+ if (!isAccepted(item.getFieldName())) {
+ LOG.warn("Form field name [{}] is not accepted",
item.getFieldName());
+ return;
+ }
+
List<String> values;
if (params.get(item.getFieldName()) != null) {
values = params.get(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 3985e0f52..e20e0f145 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
@@ -312,6 +312,10 @@ public class JakartaStreamMultiPartRequest extends
AbstractMultiPartRequest {
*/
protected void processFileItemStreamAsFormField(FileItemStream itemStream)
{
String fieldName = itemStream.getFieldName();
+ if (!isAccepted(fieldName)) {
+ LOG.warn("Form field [{}] rejected!", fieldName);
+ return;
+ }
try {
List<String> values;
String fieldValue = Streams.asString(itemStream.openStream());
@@ -340,6 +344,11 @@ public class JakartaStreamMultiPartRequest extends
AbstractMultiPartRequest {
return;
}
+ if (!isAccepted(itemStream.getName())) {
+ LOG.warn("File field [{}] rejected", itemStream.getName());
+ return;
+ }
+
File file = null;
try {
// Create the temporary upload file.
diff --git
a/core/src/test/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsCheckerTest.java
b/core/src/test/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsCheckerTest.java
index 738def5d6..114354dd5 100644
---
a/core/src/test/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsCheckerTest.java
+++
b/core/src/test/java/com/opensymphony/xwork2/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");
List<String> inners = Arrays.asList("servletRequest",
"servletResponse", "servletContext", "application", "session", "struts",
"request", "response", "dojo", "parameters");
List<String> suffixes = Arrays.asList("['test']", "[\"test\"]",
".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 81aa122ed..819ec89e0 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,9 @@ import com.opensymphony.xwork2.DefaultLocaleProvider;
import com.opensymphony.xwork2.ValidationAwareSupport;
import com.opensymphony.xwork2.mock.MockActionInvocation;
import com.opensymphony.xwork2.mock.MockActionProxy;
+import com.opensymphony.xwork2.security.DefaultAcceptedPatternsChecker;
+import com.opensymphony.xwork2.security.DefaultExcludedPatternsChecker;
+import
com.opensymphony.xwork2.security.DefaultNotExcludedAcceptedPatternsChecker;
import com.opensymphony.xwork2.util.ClassLoaderUtil;
import org.apache.commons.fileupload.servlet.ServletFileUpload;
import org.apache.struts2.ServletActionContext;
@@ -663,6 +666,68 @@ public class ActionFileUploadInterceptorTest extends
StrutsInternalTestCase {
assertTrue(msg.startsWith("Der Request übertraf die maximal erlaubte
Größe"));
}
+ public void testUnacceptedFieldName() throws Exception {
+ MockHttpServletRequest req = new MockHttpServletRequest();
+ req.setCharacterEncoding(StandardCharsets.UTF_8.name());
+ req.setMethod("post");
+ req.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");
+ req.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(req, 2000));
+
+ interceptor.intercept(mai);
+
+ assertFalse(action.hasActionErrors());
+ assertNull(action.getUploadFiles());
+ }
+
+ public void testUnacceptedFileName() throws Exception {
+ MockHttpServletRequest req = new MockHttpServletRequest();
+ req.setCharacterEncoding(StandardCharsets.UTF_8.name());
+ req.setMethod("post");
+ req.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");
+ req.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(req, 2000));
+
+ interceptor.intercept(mai);
+
+ assertFalse(action.hasActionErrors());
+ assertNull(action.getUploadFiles());
+ }
+
private String encodeTextFile(String filename, String contentType, String
content) {
return "\r\n" +
"--" +
@@ -672,7 +737,7 @@ public class ActionFileUploadInterceptorTest extends
StrutsInternalTestCase {
"file" +
"\"; filename=\"" +
filename +
- "\r\n" +
+ "\"\r\n" +
"Content-Type: " +
contentType +
"\r\n" +
@@ -697,18 +762,20 @@ public class ActionFileUploadInterceptorTest extends
StrutsInternalTestCase {
}
private MultiPartRequestWrapper createMultipartRequest(HttpServletRequest
req, int maxsize, int maxfilesize, int maxfiles, int maxStringLength) {
-
JakartaMultiPartRequest jak = new JakartaMultiPartRequest();
jak.setMaxSize(String.valueOf(maxsize));
jak.setMaxFileSize(String.valueOf(maxfilesize));
jak.setMaxFiles(String.valueOf(maxfiles));
jak.setMaxStringLength(String.valueOf(maxStringLength));
+ DefaultNotExcludedAcceptedPatternsChecker patternsChecker =
container.inject(DefaultNotExcludedAcceptedPatternsChecker.class);
+ jak.setNotExcludedAllowedPatternsChecker(patternsChecker);
return new MultiPartRequestWrapper(jak, req,
tempDir.getAbsolutePath(), new DefaultLocaleProvider());
}
private MultiPartRequestWrapper
createMultipartRequestNoMaxParamsSet(HttpServletRequest req) {
-
JakartaMultiPartRequest jak = new JakartaMultiPartRequest();
+ DefaultNotExcludedAcceptedPatternsChecker patternsChecker =
container.inject(DefaultNotExcludedAcceptedPatternsChecker.class);
+ jak.setNotExcludedAllowedPatternsChecker(patternsChecker);
return new MultiPartRequestWrapper(jak, req,
tempDir.getAbsolutePath(), new DefaultLocaleProvider());
}
diff --git
a/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java
b/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java
index 36fad5847..4fd91ecd2 100644
---
a/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java
+++
b/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java
@@ -23,6 +23,7 @@ import com.opensymphony.xwork2.ActionSupport;
import com.opensymphony.xwork2.DefaultLocaleProvider;
import com.opensymphony.xwork2.ValidationAwareSupport;
import com.opensymphony.xwork2.mock.MockActionInvocation;
+import
com.opensymphony.xwork2.security.DefaultNotExcludedAcceptedPatternsChecker;
import com.opensymphony.xwork2.util.ClassLoaderUtil;
import org.apache.commons.fileupload.servlet.ServletFileUpload;
import org.apache.struts2.ServletActionContext;
@@ -727,6 +728,68 @@ public class FileUploadInterceptorTest extends
StrutsInternalTestCase {
assertTrue(msg.startsWith("Der Request übertraf die maximal erlaubte
Größe"));
}
+ public void testUnacceptedFieldName() throws Exception {
+ MockHttpServletRequest req = new MockHttpServletRequest();
+ req.setCharacterEncoding(StandardCharsets.UTF_8.name());
+ req.setMethod("post");
+ req.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");
+ req.setContent(content.getBytes(StandardCharsets.US_ASCII));
+
+ ActionFileUploadInterceptorTest.MyFileUploadAction action =
container.inject(ActionFileUploadInterceptorTest.MyFileUploadAction.class);
+
+ MockActionInvocation mai = new MockActionInvocation();
+ mai.setAction(action);
+ mai.setResultCode("success");
+ mai.setInvocationContext(ActionContext.getContext());
+ ActionContext.getContext()
+ .withServletRequest(createMultipartRequestMaxSize(req, 2000));
+
+ interceptor.intercept(mai);
+
+ assertFalse(action.hasActionErrors());
+ assertNull(action.getUploadFiles());
+ }
+
+ public void testUnacceptedFileName() throws Exception {
+ MockHttpServletRequest req = new MockHttpServletRequest();
+ req.setCharacterEncoding(StandardCharsets.UTF_8.name());
+ req.setMethod("post");
+ req.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");
+ req.setContent(content.getBytes(StandardCharsets.US_ASCII));
+
+ ActionFileUploadInterceptorTest.MyFileUploadAction action =
container.inject(ActionFileUploadInterceptorTest.MyFileUploadAction.class);
+
+ MockActionInvocation mai = new MockActionInvocation();
+ mai.setAction(action);
+ mai.setResultCode("success");
+ mai.setInvocationContext(ActionContext.getContext());
+ ActionContext.getContext()
+ .withServletRequest(createMultipartRequestMaxSize(req, 2000));
+
+ interceptor.intercept(mai);
+
+ assertFalse(action.hasActionErrors());
+ assertNull(action.getUploadFiles());
+ }
+
private String encodeTextFile(String filename, String contentType, String
content) {
return "\r\n" +
"--" +
@@ -736,7 +799,7 @@ public class FileUploadInterceptorTest extends
StrutsInternalTestCase {
"file" +
"\"; filename=\"" +
filename +
- "\r\n" +
+ "\"\r\n" +
"Content-Type: " +
contentType +
"\r\n" +
@@ -761,18 +824,22 @@ public class FileUploadInterceptorTest extends
StrutsInternalTestCase {
}
private MultiPartRequestWrapper createMultipartRequest(HttpServletRequest
req, int maxsize, int maxfilesize, int maxfiles, int maxStringLength) {
-
JakartaMultiPartRequest jak = new JakartaMultiPartRequest();
jak.setMaxSize(String.valueOf(maxsize));
jak.setMaxFileSize(String.valueOf(maxfilesize));
jak.setMaxFiles(String.valueOf(maxfiles));
jak.setMaxStringLength(String.valueOf(maxStringLength));
+ DefaultNotExcludedAcceptedPatternsChecker patternsChecker =
container.inject(DefaultNotExcludedAcceptedPatternsChecker.class);
+ jak.setNotExcludedAllowedPatternsChecker(patternsChecker);
+
return new MultiPartRequestWrapper(jak, req,
tempDir.getAbsolutePath(), new DefaultLocaleProvider());
}
private MultiPartRequestWrapper
createMultipartRequestNoMaxParamsSet(HttpServletRequest req) {
-
JakartaMultiPartRequest jak = new JakartaMultiPartRequest();
+ DefaultNotExcludedAcceptedPatternsChecker patternsChecker =
container.inject(DefaultNotExcludedAcceptedPatternsChecker.class);
+ jak.setNotExcludedAllowedPatternsChecker(patternsChecker);
+
return new MultiPartRequestWrapper(jak, req,
tempDir.getAbsolutePath(), new DefaultLocaleProvider());
}
