Skip to content

Commit

Permalink
Merge pull request #1181 from apache/fix/WW-5501-exclude
Browse files Browse the repository at this point in the history
WW-5501 Only exclude malicious file names
  • Loading branch information
lukaszlenart authored Jan 16, 2025
2 parents c7a6daf + 688162c commit cba0db0
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<LocalizedMessage> errors = new ArrayList<>();

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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) {
Expand All @@ -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
*/
Expand All @@ -154,7 +158,7 @@ protected LocalizedMessage buildErrorMessage(Throwable e, Object[] args) {

/* (non-Javadoc)
* @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getErrors()
*/
*/
public List<LocalizedMessage> getErrors() {
return errors;
}
Expand All @@ -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();
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down

0 comments on commit cba0db0

Please sign in to comment.