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

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

Browse Source 
WW-5501 Exclude malicious names
This commit is contained in:
Lukasz Lenart
2024-12-23 15:08:58 +01:00
committed by GitHub
parent f5688e680b 3ea126388c
commit c8febc2050
7 changed files with 195 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files
core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java
+1
View File
@@ -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"
core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java
+11
View File
@@ -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();
}
}
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java
+21 -7
View File
@@ -44,6 +44,8 @@ import java.util.List;
import java.util.Map;
import java.util.Set;
import static org.apache.commons.lang3.StringUtils.normalizeSpace;
/**
* Multipart form data request adapter for Jakarta Commons Fileupload package.
*/
@@ -100,7 +102,7 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
protected void processUpload(HttpServletRequest request, String saveDir) throws FileUploadException, UnsupportedEncodingException {
if (ServletFileUpload.isMultipartContent(request)) {
for (FileItem item : parseRequest(request, saveDir)) {
LOG.debug("Found file item: [{}]", sanitizeNewlines(item.getFieldName()));
LOG.debug("Found file item: [{}]", normalizeSpace(item.getFieldName()));
if (item.isFormField()) {
processNormalFormField(item, request.getCharacterEncoding());
} else {
@@ -113,9 +115,19 @@ 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", normalizeSpace(item.getName()));
return;
}
if (!isAccepted(item.getFieldName())) {
LOG.warn("Field name [{}] is not accepted", 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;
}
@@ -134,6 +146,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", normalizeSpace(item.getFieldName()));
return;
}
List<String> values;
if (params.get(item.getFieldName()) != null) {
values = params.get(item.getFieldName());
@@ -143,7 +160,7 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
long size = item.getSize();
if (maxStringLength != null && size > maxStringLength) {
LOG.debug("Form field {} of size {} bytes exceeds limit of {}.", sanitizeNewlines(item.getFieldName()), size, maxStringLength);
LOG.debug("Form field [{}] of size [{}] bytes exceeds limit of [{}].", normalizeSpace(item.getFieldName()), size, maxStringLength);
String errorKey = "struts.messages.upload.error.parameter.too.long";
LocalizedMessage localizedMessage = new LocalizedMessage(this.getClass(), errorKey, null,
new Object[]{item.getFieldName(), maxStringLength, size});
@@ -359,7 +376,7 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
for (String name : names) {
List<FileItem> items = files.get(name);
for (FileItem item : items) {
LOG.debug("Removing file {} {}", name, item);
LOG.debug("Removing file [{}]", normalizeSpace(name));
if (!item.isInMemory()) {
item.delete();
}
@@ -367,7 +384,4 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
}
}
private String sanitizeNewlines(String before) {
return before.replaceAll("[\n\r]", "_");
}
}
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java
+21 -7
View File
@@ -25,6 +25,7 @@ import org.apache.commons.fileupload.FileUploadBase.FileSizeLimitExceededExcepti
import org.apache.commons.fileupload.FileUploadException;
import org.apache.commons.fileupload.servlet.ServletFileUpload;
import org.apache.commons.fileupload.util.Streams;
import org.apache.commons.lang3.StringUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.struts2.dispatcher.LocalizedMessage;
@@ -45,6 +46,8 @@ import java.util.List;
import java.util.Map;
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.
@@ -77,7 +80,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
File file = fileInfo.getFile();
LOG.debug("Deleting file '{}'.", file.getName());
if (!file.delete()) {
LOG.warn("There was a problem attempting to delete file '{}'.", file.getName());
LOG.warn("There was a problem attempting to delete file [{}].", file.getName());
}
}
}
@@ -252,7 +255,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
// prevent processing file field item if request size not allowed.
if (!requestSizePermitted) {
addFileSkippedError(itemStream.getName(), request);
LOG.debug("Skipped stream '{}', request maximum size ({}) exceeded.", itemStream.getName(), maxSize);
LOG.debug("Skipped stream [{}], request maximum size ({}) exceeded.", normalizeSpace(itemStream.getName()), maxSize);
continue;
}
@@ -296,7 +299,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
* @param request the servlet request
*/
protected void addFileSkippedError(String fileName, HttpServletRequest request) {
String exceptionMessage = "Skipped file " + fileName + "; request size limit exceeded.";
String exceptionMessage = "Skipped file " + normalizeSpace(fileName) + "; request size limit exceeded.";
long allowedMaxSize = maxSize != null ? maxSize : -1;
FileSizeLimitExceededException exception = new FileUploadBase.FileSizeLimitExceededException(exceptionMessage, getRequestSize(request), allowedMaxSize);
LocalizedMessage message = buildErrorMessage(exception, new Object[]{fileName, getRequestSize(request), allowedMaxSize});
@@ -312,6 +315,10 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
*/
protected void processFileItemStreamAsFormField(FileItemStream itemStream) {
String fieldName = itemStream.getFieldName();
if (!isAccepted(fieldName)) {
LOG.warn("Form field [{}] rejected!", normalizeSpace(fieldName));
return;
}
try {
List<String> values;
String fieldValue = Streams.asString(itemStream.openStream());
@@ -323,7 +330,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
}
values.add(fieldValue);
} catch (IOException e) {
LOG.warn("Failed to handle form field '{}'.", fieldName, e);
LOG.warn("Failed to handle form field [{}]", normalizeSpace(fieldName), e);
}
}
@@ -336,7 +343,12 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
protected void processFileItemStreamAsFileField(FileItemStream itemStream, String location) {
// Skip file uploads that don't have a file name - meaning that no file was selected.
if (itemStream.getName() == null || itemStream.getName().trim().isEmpty()) {
LOG.debug("No file has been uploaded for the field: {}", itemStream.getFieldName());
LOG.debug("No file has been uploaded for the field: {}", normalizeSpace(itemStream.getFieldName()));
return;
}
if (!isAccepted(itemStream.getName())) {
LOG.warn("File field [{}] rejected", normalizeSpace(itemStream.getName()));
return;
}
@@ -353,7 +365,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
try {
file.delete();
} catch (SecurityException se) {
LOG.warn("Failed to delete '{}' due to security exception above.", file.getName(), se);
LOG.warn("Failed to delete [{}] due to security exception above.", normalizeSpace(file.getName()), se);
}
}
}
@@ -385,7 +397,9 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
}
File file = File.createTempFile(prefix + "_", suffix, new File(location));
LOG.debug("Creating temporary file '{}' (originally '{}').", file.getName(), fileName);
if (LOG.isDebugEnabled()) {
LOG.debug("Creating temporary file [{}] (originally [{}]).", file.getName(), normalizeSpace(fileName));
}
return file;
}
core/src/test/java/com/opensymphony/xwork2/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");
List<String> inners = Arrays.asList("servletRequest", "servletResponse", "servletContext", "application", "session", "struts", "request", "response", "dojo", "parameters");
List<String> suffixes = Arrays.asList("['test']", "[\"test\"]", ".test");
core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java
+70 -3
View File
@@ -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());
}
core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java
+70 -3
View File
@@ -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());
}