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.

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

New unrestricted file upload size vulnerability (#351) #454

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,30 @@

import com.zaxxer.hikari.HikariDataSource;
import java.io.IOException;
import java.util.Arrays;
import java.util.List;
import java.util.Locale;
import java.util.Properties;
import javax.servlet.http.HttpServletRequest;
import javax.sql.DataSource;
import org.sasanlabs.internal.utility.LevelConstants;
import org.sasanlabs.service.vulnerability.fileupload.UnrestrictedFileUpload;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.boot.autoconfigure.jdbc.DataSourceProperties;
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.Primary;
import org.springframework.context.support.ReloadableResourceBundleMessageSource;
import org.springframework.core.annotation.Order;
import org.springframework.core.io.Resource;
import org.springframework.core.io.ResourceLoader;
import org.springframework.core.io.support.PathMatchingResourcePatternResolver;
import org.springframework.core.io.support.PropertiesLoaderUtils;
import org.springframework.jdbc.core.JdbcTemplate;
import org.springframework.web.multipart.MultipartResolver;
import org.springframework.web.multipart.commons.CommonsMultipartResolver;
import org.springframework.web.multipart.support.MultipartFilter;
import org.springframework.web.servlet.i18n.AcceptHeaderLocaleResolver;

/**
Expand All @@ -30,6 +39,9 @@ public class VulnerableAppConfiguration {
private static final String I18N_MESSAGE_FILE_LOCATION = "classpath:i18n/messages";
private static final String ATTACK_VECTOR_PAYLOAD_PROPERTY_FILES_LOCATION_PATTERN =
"classpath:/attackvectors/*.properties";
private static final List<String> MAX_FILE_UPLOAD_SIZE_OVERRIDE_PATHS =
Arrays.asList(
"/" + UnrestrictedFileUpload.CONTROLLER_PATH + "/" + LevelConstants.LEVEL_9);

/**
* Will Inject MessageBundle into messageSource bean.
Expand Down Expand Up @@ -123,4 +135,29 @@ public JdbcTemplate applicationJdbcTemplate(
@Qualifier("applicationDataSource") DataSource applicationDataSource) {
return new JdbcTemplate(applicationDataSource);
}

/**
* Customized MultipartFilter bean disables default max upload size for multipart files and
* their overall requests, for select paths. See {@link
* UnrestrictedFileUpload#getVulnerablePayloadLevel10()} for usage.
*/
@Bean
@Order(0)
public MultipartFilter multipartFilter() {
class MaxUploadSizeOverrideMultipartFilter extends MultipartFilter {
@Override
protected MultipartResolver lookupMultipartResolver(HttpServletRequest request) {
if (MAX_FILE_UPLOAD_SIZE_OVERRIDE_PATHS.contains(request.getServletPath())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way even simple url with CONTROLLER_PATH will match this filter. Does checking other way around be better?

Copy link
Contributor Author

@tkomlodi tkomlodi Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this comment. The servlet path must exactly match
"/" + UnrestrictedFileUpload.CONTROLLER_PATH + "/" + LevelConstants.LEVEL_10
for the condition to evaluate to true.

Copy link
Member

@preetkaran20 preetkaran20 Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are using here contains instead of equals and that is the reason for the question. we can make it opposite like if request.getServletPath().contains(MAX_FILE_UPLOAD_SIZE_OVERRIDE_PATHS). Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAX_FILE_UPLOAD_SIZE_OVERRIDE_PATHS is a list of string values. I set it up this way so later we can add to it if necessary. The check here is contains(), as it checks if there is a string in the list, which is equal to the path. In this context, this is really an "equals" check, with any of the values of the list. Which is, for now, just one possible value.
I can change this to be just a single value, but I believe, the behavior is already what you are suggesting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aauch ... My mistake.

CommonsMultipartResolver multipart = new CommonsMultipartResolver();
multipart.setMaxUploadSize(-1);
multipart.setMaxUploadSizePerFile(-1);
return multipart;
} else {
// returns default implementation
return lookupMultipartResolver();
}
}
};
return new MaxUploadSizeOverrideMultipartFilter();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,11 @@
*/
@VulnerableAppRestController(
descriptionLabel = "UNRESTRICTED_FILE_UPLOAD_VULNERABILITY",
value = "UnrestrictedFileUpload")
value = UnrestrictedFileUpload.CONTROLLER_PATH)
public class UnrestrictedFileUpload {
private Path root;
private Path contentDispositionRoot;

public static final String CONTROLLER_PATH = "UnrestrictedFileUpload";
private static final String STATIC_FILE_LOCATION = "upload";
static final String CONTENT_DISPOSITION_STATIC_FILE_LOCATION = "contentDispositionUpload";
private static final String BASE_PATH = "static";
Expand Down Expand Up @@ -101,7 +101,7 @@ public UnrestrictedFileUpload() throws IOException, URISyntaxException {
}
}

private static final ResponseEntity<GenericVulnerabilityResponseBean<String>>
private static ResponseEntity<GenericVulnerabilityResponseBean<String>>
genericFileUploadUtility(
Path root,
String fileName,
Expand All @@ -116,31 +116,24 @@ public UnrestrictedFileUpload() throws IOException, URISyntaxException {
root.resolve(fileName),
StandardCopyOption.REPLACE_EXISTING);
String uploadedFileLocation;
String input =
FrameworkConstants.VULNERABLE_APP
+ FrameworkConstants.SLASH
+ (isContentDisposition
? CONTENT_DISPOSITION_STATIC_FILE_LOCATION
: STATIC_FILE_LOCATION)
+ FrameworkConstants.SLASH
+ fileName;
if (htmlEncode) {
uploadedFileLocation =
StringEscapeUtils.escapeHtml4(
FrameworkConstants.VULNERABLE_APP
+ FrameworkConstants.SLASH
+ (isContentDisposition
? CONTENT_DISPOSITION_STATIC_FILE_LOCATION
: STATIC_FILE_LOCATION)
+ FrameworkConstants.SLASH
+ fileName);
uploadedFileLocation = StringEscapeUtils.escapeHtml4(input);
} else {
uploadedFileLocation =
FrameworkConstants.VULNERABLE_APP
+ FrameworkConstants.SLASH
+ (isContentDisposition
? CONTENT_DISPOSITION_STATIC_FILE_LOCATION
: STATIC_FILE_LOCATION)
+ FrameworkConstants.SLASH
+ fileName;
uploadedFileLocation = input;
}
return new ResponseEntity<GenericVulnerabilityResponseBean<String>>(
return new ResponseEntity<>(
new GenericVulnerabilityResponseBean<String>(uploadedFileLocation, true),
HttpStatus.OK);
}
return new ResponseEntity<GenericVulnerabilityResponseBean<String>>(
return new ResponseEntity<>(
new GenericVulnerabilityResponseBean<String>("Input is invalid", false),
HttpStatus.OK);
}
Expand Down Expand Up @@ -344,13 +337,39 @@ public ResponseEntity<GenericVulnerabilityResponseBean<String>> getVulnerablePay
value = LevelConstants.LEVEL_8,
htmlTemplate = "LEVEL_1/FileUpload",
requestMethod = RequestMethod.POST)
public ResponseEntity<GenericVulnerabilityResponseBean<String>> getVulnerablePayloadLevel9(
public ResponseEntity<GenericVulnerabilityResponseBean<String>> getVulnerablePayloadLevel8(
@RequestParam(REQUEST_PARAMETER) MultipartFile file)
throws ServiceApplicationException, IOException {
return genericFileUploadUtility(
contentDispositionRoot, file.getOriginalFilename(), () -> true, file, true, true);
}

@AttackVector(
vulnerabilityExposed = {
VulnerabilityType.UNCONTROLLED_RESOURCE_CONSUPTION,
VulnerabilityType.DENIAL_OF_SERVICE
},
description = "UNRESTRICTED_FILE_UPLOAD_UNCONTROLLED_RESOURCE_CONSUMPTION",
payload = "UNRESTRICTED_FILE_UPLOAD_PAYLOAD_LEVEL_9")
@AttackVector(
vulnerabilityExposed = {VulnerabilityType.PATH_TRAVERSAL},
description = "UNRESTRICTED_FILE_UPLOAD_NO_VALIDATION_FILE_NAME",
payload = "UNRESTRICTED_FILE_UPLOAD_PAYLOAD_LEVEL_8")
@VulnerableAppRequestMapping(
value = LevelConstants.LEVEL_9,
htmlTemplate = "LEVEL_1/FileUpload",
requestMethod = RequestMethod.POST)
public ResponseEntity<GenericVulnerabilityResponseBean<String>> getVulnerablePayloadLevel9(
@RequestParam(REQUEST_PARAMETER) MultipartFile file) throws IOException {
return genericFileUploadUtility(
root,
RANDOM.nextInt() + "_" + file.getOriginalFilename(),
() -> true,
file,
true,
false);
}

// I think below vulnerability is not exploitable. Need to check again after running Owasp
// ZAP FileUpload Addon.
@AttackVector(
Expand All @@ -361,11 +380,11 @@ public ResponseEntity<GenericVulnerabilityResponseBean<String>> getVulnerablePay
description =
"UNRESTRICTED_FILE_UPLOAD_IF_FILE_NAME_NOT_ENDS_WITH_.PNG_OR_.JPEG_CASE_INSENSITIVE")
@VulnerableAppRequestMapping(
value = LevelConstants.LEVEL_9,
value = LevelConstants.LEVEL_10,
variant = Variant.SECURE,
htmlTemplate = "LEVEL_1/FileUpload",
requestMethod = RequestMethod.POST)
public ResponseEntity<GenericVulnerabilityResponseBean<String>> getVulnerablePayloadLevel8(
public ResponseEntity<GenericVulnerabilityResponseBean<String>> getVulnerablePayloadLevel10(
@RequestParam(REQUEST_PARAMETER) MultipartFile file)
throws ServiceApplicationException, IOException {
String fileName = file.getOriginalFilename();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ public enum VulnerabilityType {
COMMAND_INJECTION(77, 31),

UNRESTRICTED_FILE_UPLOAD(434, null),
UNCONTROLLED_RESOURCE_CONSUPTION(400, null),
DENIAL_OF_SERVICE(730, 10),

OPEN_REDIRECT_3XX_STATUS_CODE(601, 38),

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,7 @@ UNRESTRICTED_FILE_UPLOAD_PAYLOAD_LEVEL_7=Hackers can execute code in a user's ma
The extension after the null byte is ignored and the file will be treated as .html. Now if we upload this file and access it, the file should get executed in the browser.
UNRESTRICTED_FILE_UPLOAD_PAYLOAD_LEVEL_8=A path traversal attack aims to overwrite files and directories that are stored outside the web root folder. By manipulating variables that reference files with "dot-dot-slash (../)" sequences and its variations or by using absolute file paths, it may be possible to change arbitrary files and directories stored on file system including application source code or configuration and critical system files.<br/>\
<b>How to exploit this level?</b><br/>\
This level is vulnerable for path traversal, so if you choose a filename like <code>../index.html</code> then the file will be stored in another directory than provided.
This level is vulnerable for path traversal, so if you choose a filename like <code>../index.html</code> then the file will be stored in another directory than provided.
UNRESTRICTED_FILE_UPLOAD_PAYLOAD_LEVEL_9=Uploading very large files may lead to denial of service attacks on file space or other web application functions that may be impacted by in-memory file manipulation. This could overload application memory or disk space causing temporary or permanent unavailability of the application.<br/>\
<b>How to exploit this level?</b><br/>\
Since there is no file size validation at this level, you can upload very large sized files. Uploaded files are stored in memory cumulatively, which may cause the application to run out of memory and become unavailable. In such cases, a java.lang.OutOfMemoryError may be thrown and logged.
2 changes: 1 addition & 1 deletion src/main/resources/i18n/messages.properties
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ UNRESTRICTED_FILE_UPLOAD_IF_NOT_HTML_NOT_HTM_FILE_EXTENSION_CASE_INSENSITIVE=All
UNRESTRICTED_FILE_UPLOAD_IF_FILE_NAME_NOT_CONTAINS_.PNG_OR_.JPEG_CASE_INSENSITIVE=Only file name is allowed if it contains case insensitive .jpeg or .png.
UNRESTRICTED_FILE_UPLOAD_IF_FILE_NAME_NOT_ENDS_WITH_.PNG_OR_.JPEG_CASE_INSENSITIVE_AND_FILE_NAMES_CONSIDERED_BEFORE_NULL_BYTE=Only file name is allowed if it ends with case insensitive .jpeg or .png and it is considered before Null Bytes only.
UNRESTRICTED_FILE_UPLOAD_IF_FILE_NAME_NOT_ENDS_WITH_.PNG_OR_.JPEG_CASE_INSENSITIVE=Only file name is allowed if it ends with case insensitive .jpeg or .png.

UNRESTRICTED_FILE_UPLOAD_UNCONTROLLED_RESOURCE_CONSUMPTION=Maximum uploaded file size is not limited.

# XXE Vulnerability
XXE_VULNERABILITY=An XML External Entity attack is a type of attack against an \
Expand Down
1 change: 1 addition & 0 deletions src/main/resources/i18n/messages_en_US.properties
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ Important Links:<br/>\
</ol>

#### Attack Vector Description
UNRESTRICTED_FILE_UPLOAD_UNCONTROLLED_RESOURCE_CONSUPTION=Maximum uploaded file size is not limited.
Copy link
Member

@preetkaran20 preetkaran20 Dec 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add this message.properties and if possible, you can add to other translations as well and please change key to UNRESTRICTED_FILE_UPLOAD_UNCONTROLLED_RESOURCE_CONSUMPTION.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add placeholders, but I don't speak the other languages, so can't add actual translations. I could do google translate, if that is acceptable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add just in message.properties

UNRESTRICTED_FILE_UPLOAD_NO_VALIDATION_FILE_NAME=There is no validation on uploaded file's name.
UNRESTRICTED_FILE_UPLOAD_IF_NOT_HTML_FILE_EXTENSION=All file extensions are allowed except .html extensions.
UNRESTRICTED_FILE_UPLOAD_IF_NOT_HTML_NOT_HTM_FILE_EXTENSION=All file extensions are allowed except .html and .htm extensions.
Expand Down
Loading