Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WW-5501 Exclude malicious names #1156

Merged
merged 2 commits into from
Dec 23, 2024
Merged

WW-5501 Exclude malicious names #1156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
09993e3
WW-5501 Exclude malicious names
lukaszlenart Dec 23, 2024
3ea1263
WW-5501 Uses StringUtils.normalizeSpace instead of sanitizeNewlines
lukaszlenart Dec 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
11 changes: 11 additions & 0 deletions core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

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;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -121,6 +123,11 @@ public void setLocaleProviderFactory(LocaleProviderFactory localeProviderFactory
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.
Expand Down Expand Up @@ -169,4 +176,8 @@ protected String getCanonicalName(final String originalFileName) {
return fileName;
}

protected boolean isAccepted(String fileName) {
return patternsChecker.isAllowed(fileName).isAllowed();
}

}
28 changes: 21 additions & 7 deletions core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@
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.
*/
Expand Down Expand Up @@ -100,7 +102,7 @@ public void parse(HttpServletRequest request, String saveDir) throws IOException
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 {
Expand All @@ -113,9 +115,19 @@ protected void processUpload(HttpServletRequest request, String saveDir) throws
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;
}

Expand All @@ -134,6 +146,11 @@ protected void processNormalFormField(FileItem item, String charset) throws Unsu
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());
Expand All @@ -143,7 +160,7 @@ protected void processNormalFormField(FileItem item, String charset) throws Unsu

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});
Expand Down Expand Up @@ -359,15 +376,12 @@ public void cleanUp() {
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();
}
}
}
}

private String sanitizeNewlines(String before) {
return before.replaceAll("[\n\r]", "_");
}
}
28 changes: 21 additions & 7 deletions .../src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
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;
Expand All @@ -45,6 +46,8 @@
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.
Expand Down Expand Up @@ -77,7 +80,7 @@ public void cleanUp() {
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());
}
}
}
Expand Down Expand Up @@ -252,7 +255,7 @@ protected void processUpload(HttpServletRequest request, String saveDir) throws
// 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;
}

Expand Down Expand Up @@ -296,7 +299,7 @@ protected long getRequestSize(HttpServletRequest request) {
* @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});
Expand All @@ -312,6 +315,10 @@ protected void addFileSkippedError(String fileName, HttpServletRequest request)
*/
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());
Expand All @@ -323,7 +330,7 @@ protected void processFileItemStreamAsFormField(FileItemStream itemStream) {
}
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);
}
}

Expand All @@ -336,7 +343,12 @@ protected void processFileItemStreamAsFormField(FileItemStream itemStream) {
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;
}

Expand All @@ -353,7 +365,7 @@ protected void processFileItemStreamAsFileField(FileItemStream itemStream, Strin
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);
}
}
}
Expand Down Expand Up @@ -385,7 +397,9 @@ protected File createTemporaryFile(String fileName, String location) throws IOEx
}

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;
}

Expand Down
2 changes: 1 addition & 1 deletion core/src/test/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsCheckerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ public void testHardcodedPatterns() throws Exception {

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");

Expand Down
73 changes: 70 additions & 3 deletions core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@
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;
Expand Down Expand Up @@ -663,6 +666,68 @@ public void testMultipartRequestLocalizedError() throws Exception {
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" +
"--" +
Expand All @@ -672,7 +737,7 @@ private String encodeTextFile(String filename, String contentType, String conten
"file" +
"\"; filename=\"" +
filename +
"\r\n" +
"\"\r\n" +
"Content-Type: " +
contentType +
"\r\n" +
Expand All @@ -697,18 +762,20 @@ private MultiPartRequestWrapper createMultipartRequestMaxStringLength(HttpServle
}

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());
}

Expand Down
Loading
Loading