-
-
Notifications
You must be signed in to change notification settings - Fork 390
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
Changes from all commits
6ce0de9
daa993c
d1db58f
10dd208
b7adbc2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -114,6 +114,7 @@ Important Links:<br/>\ | |
</ol> | ||
|
||
#### Attack Vector Description | ||
UNRESTRICTED_FILE_UPLOAD_UNCONTROLLED_RESOURCE_CONSUPTION=Maximum uploaded file size is not limited. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aauch ... My mistake.