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

Merge pull request #1184 from apache/fix/WW-5501-exclude-s7

Browse Source 
WW-5501 Only exclude malicious file names
This commit is contained in:
Lukasz Lenart
2025-01-16 10:19:15 +01:00
committed by GitHub
parent 71bca19c13 4279511c5b
commit fa60e1ceb2
7 changed files with 47 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files
core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java
+17 -9
View File
@@ -19,6 +19,8 @@
package org.apache.struts2.dispatcher.multipart;
import org.apache.struts2.inject.Inject;
import org.apache.struts2.security.DefaultExcludedPatternsChecker;
import org.apache.struts2.security.ExcludedPatternsChecker;
import jakarta.servlet.http.HttpServletRequest;
import org.apache.commons.fileupload2.core.FileUploadByteCountLimitException;
import org.apache.commons.fileupload2.core.FileUploadContentTypeException;
@@ -31,7 +33,6 @@ 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;
@@ -53,6 +54,8 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest {
private static final Logger LOG = LogManager.getLogger(AbstractMultiPartRequest.class);
private static final String EXCLUDED_FILE_PATTERN = ".*[<>&\"'|;\\\\/?*:]+.*|.*\\.\\..*";
/**
* Defines the internal buffer size used during streaming operations.
*/
@@ -108,7 +111,13 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest {
*/
protected Map<String, List<String>> parameters = new HashMap<>();
protected NotExcludedAcceptedPatternsChecker patternsChecker;
private final ExcludedPatternsChecker patternsChecker;
protected AbstractMultiPartRequest() {
patternsChecker = new DefaultExcludedPatternsChecker();
((DefaultExcludedPatternsChecker) patternsChecker).setAdditionalExcludePatterns(EXCLUDED_FILE_PATTERN);
}
/**
* @param bufferSize Sets the buffer size to be used.
@@ -183,11 +192,6 @@ 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
*
@@ -425,8 +429,12 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest {
}
}
protected boolean isAccepted(String fileName) {
return patternsChecker.isAllowed(fileName).isAllowed();
/**
* @param fileName file name to check
* @return true if the file name is excluded
*/
protected boolean isExcluded(String fileName) {
return patternsChecker.isExcluded(fileName).isExcluded();
}
}
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java
+3 -3
View File
@@ -79,7 +79,7 @@ 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())) {
if (isExcluded(item.getFieldName())) {
LOG.warn(() -> "Form field [%s] is rejected!".formatted(normalizeSpace(item.getFieldName())));
return;
}
@@ -105,12 +105,12 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
}
protected void processFileField(DiskFileItem item) {
if (!isAccepted(item.getName())) {
if (isExcluded(item.getName())) {
LOG.warn(() -> "File name [%s] is not accepted".formatted(normalizeSpace(item.getName())));
return;
}
if (!isAccepted(item.getFieldName())) {
if (isExcluded(item.getFieldName())) {
LOG.warn(() -> "Field name [%s] is not accepted".formatted(normalizeSpace(item.getFieldName())));
return;
}
core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java
+2 -2
View File
@@ -116,7 +116,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
String fieldName = fileItemInput.getFieldName();
String fieldValue = readStream(fileItemInput.getInputStream());
if (!isAccepted(fieldName)) {
if (isExcluded(fieldName)) {
LOG.warn(() -> "Form field [%s] is rejected!".formatted(normalizeSpace(fieldName)));
return;
}
@@ -198,7 +198,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
return;
}
if (!isAccepted(fileItemInput.getName())) {
if (isExcluded(fileItemInput.getName())) {
LOG.warn(() -> "File field [%s] rejected".formatted(normalizeSpace(fileItemInput.getName())));
return;
}
core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java
+22
View File
@@ -525,6 +525,28 @@ abstract class AbstractMultiPartRequestTest {
.isEmpty();
}
@Test
public void maliciousFilename() throws IOException {
String content = formFile("file1", "../test1.csv", "1,2,3,4") +
formField("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()
.hasSize(1);
assertThat(multiPart.getParameterNames().asIterator()).toIterable()
.containsOnly("param");
assertThat(multiPart.getFileNames("file1")).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
+1 -5
View File
@@ -18,15 +18,11 @@
*/
package org.apache.struts2.dispatcher.multipart;
import org.apache.struts2.security.DefaultNotExcludedAcceptedPatternsChecker;
public class JakartaMultiPartRequestTest extends AbstractMultiPartRequestTest {
@Override
protected AbstractMultiPartRequest createMultipartRequest() {
JakartaMultiPartRequest multiPartRequest = new JakartaMultiPartRequest();
multiPartRequest.setNotExcludedAllowedPatternsChecker(container.inject(DefaultNotExcludedAcceptedPatternsChecker.class));
return multiPartRequest;
return new JakartaMultiPartRequest();
}
}
core/src/test/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequestTest.java
+2 -5
View File
@@ -20,7 +20,6 @@ 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;
@@ -33,9 +32,7 @@ public class JakartaStreamMultiPartRequestTest extends AbstractMultiPartRequestT
@Override
protected AbstractMultiPartRequest createMultipartRequest() {
JakartaStreamMultiPartRequest multiPartRequest = new JakartaStreamMultiPartRequest();
multiPartRequest.setNotExcludedAllowedPatternsChecker(container.inject(DefaultNotExcludedAcceptedPatternsChecker.class));
return multiPartRequest;
return new JakartaStreamMultiPartRequest();
}
@Test
@@ -51,7 +48,7 @@ public class JakartaStreamMultiPartRequestTest extends AbstractMultiPartRequestT
// when
multiPart.setMaxSizeOfFiles("10");
multiPart.parse(mockRequest, tempDir.toString());
multiPart.parse(mockRequest, tempDir);
// then
assertThat(multiPart.uploadedFiles)
core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java
-3
View File
@@ -24,7 +24,6 @@ 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;
@@ -610,8 +609,6 @@ 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());
}