From 688162cae2fcfa0f45ecdfb44ff5c0ce70932ca2 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Fri, 10 Jan 2025 07:45:57 +0100 Subject: [PATCH] WW-5501 Only exclude malicious file names --- .../multipart/AbstractMultiPartRequest.java | 34 ++++++++++++------- .../multipart/JakartaMultiPartRequest.java | 6 ++-- .../JakartaStreamMultiPartRequest.java | 5 ++- .../ActionFileUploadInterceptorTest.java | 4 --- .../FileUploadInterceptorTest.java | 4 --- 5 files changed, 26 insertions(+), 27 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 ed013b7241..a0c865590c 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,7 +20,8 @@ import com.opensymphony.xwork2.LocaleProviderFactory; import com.opensymphony.xwork2.inject.Inject; -import com.opensymphony.xwork2.security.NotExcludedAcceptedPatternsChecker; +import com.opensymphony.xwork2.security.DefaultExcludedPatternsChecker; +import com.opensymphony.xwork2.security.ExcludedPatternsChecker; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.StrutsConstants; @@ -39,13 +40,15 @@ 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. */ public static final int BUFFER_SIZE = 10240; /** - * Internal list of raised errors to be passed to the the Struts2 framework. + * Internal list of raised errors to be passed to the Struts2 framework. */ protected List errors = new ArrayList<>(); @@ -80,7 +83,13 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { * Localization to be used regarding errors. */ protected Locale defaultLocale = Locale.ENGLISH; - private NotExcludedAcceptedPatternsChecker patternsChecker; + + private final ExcludedPatternsChecker patternsChecker; + + public AbstractMultiPartRequest() { + patternsChecker = new DefaultExcludedPatternsChecker(); + ((DefaultExcludedPatternsChecker) patternsChecker).setAdditionalExcludePatterns(EXCLUDED_FILE_PATTERN); + } /** * @param bufferSize Sets the buffer size to be used. @@ -123,14 +132,9 @@ 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. + * the Struts2 framework. */ protected void setLocale(HttpServletRequest request) { if (defaultLocale == null) { @@ -141,7 +145,7 @@ protected void setLocale(HttpServletRequest request) { /** * Build error message. * - * @param e the Throwable/Exception + * @param e the Throwable/Exception * @param args arguments * @return error message */ @@ -154,7 +158,7 @@ protected LocalizedMessage buildErrorMessage(Throwable e, Object[] args) { /* (non-Javadoc) * @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getErrors() - */ + */ public List getErrors() { return errors; } @@ -176,8 +180,12 @@ protected String getCanonicalName(final String originalFileName) { return fileName; } - 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(); } } 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 0367ac5e65..e78dea6eb4 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 @@ -115,12 +115,12 @@ protected void processUpload(HttpServletRequest request, String saveDir) throws protected void processFileField(FileItem item) { LOG.debug("Item is a file upload"); - if (!isAccepted(item.getName())) { + if (isExcluded(item.getName())) { LOG.warn("File name [{}] is not accepted", normalizeSpace(item.getName())); return; } - if (!isAccepted(item.getFieldName())) { + if (isExcluded(item.getFieldName())) { LOG.warn("Field name [{}] is not accepted", normalizeSpace(item.getFieldName())); return; } @@ -146,7 +146,7 @@ protected void processNormalFormField(FileItem item, String charset) throws Unsu try { LOG.debug("Item is a normal form field"); - if (!isAccepted(item.getFieldName())) { + if (isExcluded(item.getFieldName())) { LOG.warn("Form field name [{}] is not accepted", normalizeSpace(item.getFieldName())); return; } 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 9abe7a0112..757f56b49f 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 @@ -25,7 +25,6 @@ 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; @@ -315,7 +314,7 @@ protected void addFileSkippedError(String fileName, HttpServletRequest request) */ protected void processFileItemStreamAsFormField(FileItemStream itemStream) { String fieldName = itemStream.getFieldName(); - if (!isAccepted(fieldName)) { + if (isExcluded(fieldName)) { LOG.warn("Form field [{}] rejected!", normalizeSpace(fieldName)); return; } @@ -347,7 +346,7 @@ protected void processFileItemStreamAsFileField(FileItemStream itemStream, Strin return; } - if (!isAccepted(itemStream.getName())) { + if (isExcluded(itemStream.getName())) { LOG.warn("File field [{}] rejected", normalizeSpace(itemStream.getName())); return; } 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 819ec89e07..b9e64df8ff 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java @@ -767,15 +767,11 @@ private MultiPartRequestWrapper createMultipartRequest(HttpServletRequest req, i 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 4fd91ecd22..0df273321e 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java @@ -829,16 +829,12 @@ private MultiPartRequestWrapper createMultipartRequest(HttpServletRequest req, i 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()); }