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

tkomlodi · 2023-11-17T22:22:50Z

As proposed earlier, I added a new level 10 endpoint to UnrestrictedFileUpload which accepts unlimited sized files. These files are stored in-memory on the heap to allow triggering an out-of-memory condition.

VulnerableAppConfiguration was updated to remove the default file size limit for the new endpoint only.

Two new maven dependencies were added:

commons-fileupload: facilitates the configuration change
spring-test: allows mocking multipart files in the new unit test

I didn't add the new labels to the non-English i18n files. Let me know if that is required.

The unit test does not test the actual enforcement (or lack of) the file size limit. That could only be done with an integration test which I did not want to introduce. I manually tested that the limit is removed from the new endpoint but it still applies to all the other ones.

This change contains the prior fix for #449 (#453). This is the only way I can run the application in my environment. I hope that, if that is merged first, it won't cause any issues. However, I'm not 100% sure how github will react.

Thanks!

…rittable by application SasanLabs#449.

… size. SasanLabs#351

preetkaran20 · 2023-11-21T19:36:51Z

src/main/java/org/sasanlabs/configuration/VulnerableAppConfiguration.java

+    @Bean
+    @Order(0)
+    public MultipartFilter multipartFilter() {
+        class CustomMF extends MultipartFilter {


Please rename as per the working of the filter.

preetkaran20 · 2023-11-21T19:38:31Z

src/main/java/org/sasanlabs/configuration/VulnerableAppConfiguration.java

+        class CustomMF extends MultipartFilter {
+            @Override
+            protected MultipartResolver lookupMultipartResolver(HttpServletRequest request) {
+                if (MAX_FILE_UPLOAD_SIZE_OVERRIDE_PATHS.contains(request.getServletPath())) {


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

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.

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?

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.

aauch ... My mistake.

preetkaran20 · 2023-11-21T20:13:45Z

build.gradle

@@ -129,6 +129,8 @@ dependencies {
    // https://mvnrepository.com/artifact/org.assertj/assertj-core
    testImplementation group: 'org.assertj', name: 'assertj-core', version: '3.17.2'

+    testImplementation group: 'org.springframework', name: 'spring-test'


do we really need this? we are not using spring context in the code. I think this will make the project heavier

For mocking multipart file, I think mockito should help by mocking the interface and it will dynamically create the implementation and that way you can return the values by using Mockito.when ... then construct.

Good point. I introduced spring-test when I was trying to make the integration test work. When I moved on from that, I didn't think of simply going back to mockito.

preetkaran20 · 2023-11-21T20:15:42Z

src/main/java/org/sasanlabs/service/vulnerability/fileupload/UnrestrictedFileUpload.java

@@ -92,10 +96,10 @@ public UnrestrictedFileUpload() throws IOException, URISyntaxException {
                    "If you are running vulnerableApp as a Jar then UnrestrictedFileUpload will not work. "
                            + "For more information: https://github.com/SasanLabs/VulnerableApp/issues/255",
                    e);
-            if (root != null) {
+            if (root == null || !root.toFile().exists()) {


will we be able to retrieve the files from temp location after this change?

Good question. What I aimed for and tested is that my change does not impact the original behavior. E.g. I tested the app running as a jar, and also running with ./gradlew bootRun. (I cannot use docker in my current setup.) In both cases the behavior stayed unchanged. However, I'm not 100% clear on how this is supposed to work. I don't see a way in the existing code to return files saved to a temp root location. When the root location stays a static location within the published web file structure, this gets served as all the other static files. However, when root gets modified to a tmp location, there is no way to load saved files from there. PreflightController does not have logic to serve files from a temp root.

I didn't enter a bug for this as I'm not 100% sure that I'm not misreading the code and this wasn't the focus of my work. But there might be an existing bug in there. My change should not impact this either way, the original logic should be unchanged.

preetkaran20

@tkomlodi thanks a lot for this change. Amazing work !!!!.

requested a few changes, please incorporated and will merge this change very soon.

…oved spring-test dependency.

preetkaran20 · 2023-12-03T19:00:23Z

src/main/resources/i18n/messages_en_US.properties

@@ -114,6 +114,7 @@ Important Links:<br/>\
 </ol>

 #### Attack Vector Description
+UNRESTRICTED_FILE_UPLOAD_UNCONTROLLED_RESOURCE_CONSUPTION=Maximum uploaded file size is not limited.


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.

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.

add just in message.properties

preetkaran20 · 2023-12-03T19:20:52Z

src/main/java/org/sasanlabs/service/vulnerability/fileupload/UnrestrictedFileUpload.java

+        // stores files in heap memory indefinitely to allow triggering OutOfMemoryError
+        heapMemoryFileStore.add(file.getBytes());
+        return new ResponseEntity<GenericVulnerabilityResponseBean<String>>(
+                new GenericVulnerabilityResponseBean<String>("File accepted.", true),


we should return the path of file instead of returning File Accepted like we are doing this for other levels.

preetkaran20 · 2023-12-03T19:21:36Z

src/main/java/org/sasanlabs/service/vulnerability/fileupload/UnrestrictedFileUpload.java

+                VulnerabilityType.DENIAL_OF_SERVICE
+            },
+            description = "UNRESTRICTED_FILE_UPLOAD_UNCONTROLLED_RESOURCE_CONSUPTION",
+            payload = "UNRESTRICTED_FILE_UPLOAD_PAYLOAD_LEVEL_10")


can you please make this level as 9 and the current level 9 as level 10 so that we can give users a gradual experience with increasing complexity.

preetkaran20 · 2023-12-03T19:22:40Z

src/main/java/org/sasanlabs/service/vulnerability/fileupload/UnrestrictedFileUpload.java

 public class UnrestrictedFileUpload {
    private Path root;
    private Path contentDispositionRoot;
+    private List<byte[]> heapMemoryFileStore = new ArrayList<>();


Instead of this, shall we use the current filesystem like we are doing in all other levels?

I decided to store large files in memory and not on the file system for two reasons:

The integrity of the system that the application runs on. If these files are stored on the file system, they can cause the runtime environment to run out disk space and become corrupted. If it is a VM, it can become unusable depending on the setup. If it is a laptop, like in my case, it could also cause some serious unrecoverable issues. If the files are stored in memory, a simple application restart will reclaim the used memory without the possibility of system corruption.

It takes a much smaller file size to cause a DOS condition. Usually the available memory to an application is less than available disk space. It is also easier to confirm this by simply looking at the error logs of the application server. If the disk runs out of space, it is possible that no error logs will be generated due to this.

I can make large uploaded files downloadable, if this is something desirable. One can just access these files directly from memory and stream them to the client.

With all that in mind, if you still prefer writing these files to the disk, just let me know and I'll update the code accordingly.
Thanks

If that is the case then put in the temp folder instead and then we also need to look for files in temp folder for downloading. However, for this PR i would suggest to go ahead with storing in filesystem and take temp folder related change as enhancement or in a different issue.

preetkaran20 · 2023-12-03T19:23:30Z

src/main/java/org/sasanlabs/service/vulnerability/fileupload/UnrestrictedFileUpload.java

+    public ResponseEntity<GenericVulnerabilityResponseBean<String>> getVulnerablePayloadLevel10(
+            @RequestParam(REQUEST_PARAMETER) MultipartFile file) throws IOException {
+        // stores files in heap memory indefinitely to allow triggering OutOfMemoryError
+        heapMemoryFileStore.add(file.getBytes());


I would suggest using

genericFileUploadUtility( root, RANDOM.nextInt() + "_" + file.getOriginalFilename(), validator, file, true, false);

please check the other levels for mechanism of storing files

preetkaran20 · 2023-12-03T19:23:53Z

src/main/java/org/sasanlabs/service/vulnerability/fileupload/UnrestrictedFileUpload.java

+                HttpStatus.OK);
+    }
+
+    List<byte[]> getStoredFiles() {


This is not needed.

For handling SasanLabs/VulnerableApp#454, we need to enable unrestriction in facade app to work in docker environment.

preetkaran20

Thanks a lot for the PR. merging it now.

codecov-commenter · 2023-12-18T04:18:44Z

Codecov Report

Attention: 44 lines in your changes are missing coverage. Please review.

Comparison is base (a867b67) 23.15% compared to head (b7adbc2) 23.08%.
Report is 1 commits behind head on master.

Files	Patch %	Lines
...Injection/UnionBasedSQLInjectionVulnerability.java	27.27%	24 Missing ⚠️
...labs/configuration/VulnerableAppConfiguration.java	0.00%	10 Missing ⚠️
...lnerability/fileupload/UnrestrictedFileUpload.java	0.00%	8 Missing ⚠️
...ice/vulnerability/sqlInjection/CarInformation.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #454      +/-   ##
============================================
- Coverage     23.15%   23.08%   -0.08%     
- Complexity      206      207       +1     
============================================
  Files            55       55              
  Lines          1831     1880      +49     
  Branches        196      196              
============================================
+ Hits            424      434      +10     
- Misses         1365     1404      +39     
  Partials         42       42

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

preetkaran20 · 2023-12-18T04:19:37Z

Thanks @tkomlodi for the PR and presenting Idea for fixing this issue.

tkomlodi added 2 commits November 14, 2023 10:05

Fixed file upload tmp directory creation when root directory is not w…

6ce0de9

…rittable by application SasanLabs#449.

Added new UnrestrectedFileUpload vulnerability with unrestricted file…

daa993c

… size. SasanLabs#351

preetkaran20 reviewed Nov 21, 2023

View reviewed changes

PR feedback updates: renamed custom MultipartResolver class name, rem…

d1db58f

…oved spring-test dependency.

preetkaran20 reviewed Dec 3, 2023

View reviewed changes

preetkaran20 requested changes Dec 3, 2023

View reviewed changes

preetkaran20 added a commit to SasanLabs/VulnerableApp-facade that referenced this pull request Dec 3, 2023

Update nginx for handling unrestricted file upload

992fa8f

For handling SasanLabs/VulnerableApp#454, we need to enable unrestriction in facade app to work in docker environment.

preetkaran20 mentioned this pull request Dec 3, 2023

Update nginx for handling unrestricted file upload SasanLabs/VulnerableApp-facade#99

Merged

preetkaran20 added a commit to SasanLabs/VulnerableApp-facade that referenced this pull request Dec 3, 2023

Update nginx for handling unrestricted file upload (#99)

4319501

For handling SasanLabs/VulnerableApp#454, we need to enable unrestriction in facade app to work in docker environment.

preetkaran20 mentioned this pull request Dec 3, 2023

UnrestrictedFileUpload: uploaded file is not accessible when root directory "/" is not writable #449

Closed

Karan Preet Singh Sasan added 2 commits December 17, 2023 19:45

Merge branch 'master' of https://github.com/SasanLabs/VulnerableApp

10dd208

Adding storage of bigger files in disk

b7adbc2

preetkaran20 approved these changes Dec 18, 2023

View reviewed changes

preetkaran20 merged commit 60c924b into SasanLabs:master Dec 18, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tkomlodi commented Nov 17, 2023

preetkaran20 Nov 21, 2023

preetkaran20 Nov 21, 2023

tkomlodi Nov 27, 2023 •

edited

Loading

preetkaran20 Nov 29, 2023 •

edited

Loading

tkomlodi Dec 11, 2023

preetkaran20 Dec 18, 2023

preetkaran20 Nov 21, 2023

preetkaran20 Nov 21, 2023

tkomlodi Nov 27, 2023

preetkaran20 Nov 21, 2023

tkomlodi Nov 27, 2023

preetkaran20 left a comment

preetkaran20 Dec 3, 2023 •

edited

Loading

tkomlodi Dec 11, 2023

preetkaran20 Dec 18, 2023

preetkaran20 Dec 3, 2023

preetkaran20 Dec 3, 2023

preetkaran20 Dec 3, 2023

tkomlodi Dec 4, 2023

preetkaran20 Dec 4, 2023

preetkaran20 Dec 3, 2023

preetkaran20 Dec 3, 2023

preetkaran20 Dec 3, 2023

preetkaran20 left a comment

codecov-commenter commented Dec 18, 2023

preetkaran20 commented Dec 18, 2023

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

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

Conversation

tkomlodi commented Nov 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkomlodi Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

preetkaran20 Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

preetkaran20 left a comment

Choose a reason for hiding this comment

preetkaran20 Dec 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

preetkaran20 left a comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 18, 2023

Codecov Report

preetkaran20 commented Dec 18, 2023

tkomlodi Nov 27, 2023 •

edited

Loading

preetkaran20 Nov 29, 2023 •

edited

Loading

preetkaran20 Dec 3, 2023 •

edited

Loading