12.1.1 is not reasonable

[jmanico]

From https://github.com/OWASP/ASVS/blob/master/5.0/en/0x20-V12-Files-Resources.md

Most all providers allow large file uploads. This one needs to be dropped or revisited.

12.1.1	Verify that the application will not accept large files that could fill up storage or cause a denial of service.	✓	✓	✓	400

[elarlang]

You can drop "large files" or reword it, but the point should stay - one user should not occupy entire storage space.

[jmanico]

You can drop "large files" or reword it, but the point should stay - one user should not occupy entire storage space.

This is less of a problem with the cloud, and proper quota and quota is handled elsewhere. This is not a reasonable requirement and is too general and should, IMO, go away.

[elarlang]

As I have proved this to be an issue during pen-test cases and caused DoS attack with filled storage, I disagree and say - it is actual and valid requirement. Not everything in the world is/or will be moved to cloud.

And it is valid in cloud as well - there is auto-scale available for make more storage space available, but you need to pay for that (but this is more business logical issue).

[jmanico]

So what size is too big? Many services take giant multi-GB files. I again assert the problem is that you abused quota and filled space to the max in your test and that the file size itself was not the problem.

We NEED to support large files as developers, which is also my point.

[jmanico]

12.1.3 already talks about a file size quota. So there you go, you get your requirement. "large files" is ambiguous while "file size quota" is more relevant and covers your exact test case.

So again, I suggest we drop 12.1.1 since it's already COMPLETELY covered in 12.1.3

From https://github.com/OWASP/ASVS/blob/master/5.0/en/0x20-V12-Files-Resources.md

12.1.3	Verify that a file size quota and maximum number of files per user is enforced to ensure that a single user cannot fill up the storage with too many files, or excessively large files.		✓	✓	770

[elarlang]

Agree with those arguments on storage topic, those are covered with 12.1.3 and this part should be removed from 12.1.1 if the requirement itself stays.

But I want to be sure that we don't create some new not covered area here.

12.1.1 without storage occupation should be:
Verify that the application will not accept large files that could cause a denial of service. (+ in general application is not able to process).

If some application or service accepts "too large" file and dies when processing it, it is "a bit" problematic. I think we need to cover that case. It's basically userinput validation. It's a bit the same, like do not accept longer text-field values you can store to the database field.

One more thing to take in account or think through - current logic behind ZIP bomb's is a bit related that you don't allow to upload "too large" compressed files.
#799 (comment)

[jsulinski]

Another idea here would be dropping 12.1.1 and modifying 12.1.3 to read:

"Verify that file size and number restrictions are enforced per user to ensure that a single user cannot fill up storage or cause a denial of service."

It also might make sense to include this in L1 as it can be tested externally and is included in most pentests.

[elarlang]

I would keep storage and denial of service topics separately

[danielcuthbert]

Whilst we have changed how we feel about size, the requirement is still valid and if you do say accept 1gb uploads, they need to handled and processed in a way that doesn't kill the app or turn into a dos-like attack.

For example, what @jsulinski wrote is ideal

[elarlang]

I repeat, that for me it makes sense to keep those requirements separately

• 12.1.1 - test per one file, mostly doable with black-box testing, checks the application per one file upload
• 12.1.3 - is checking available resources from the configuration/setup and if you can not cause immediate damage/dos, it requires white-box testing.

And at the moment I would just remove the storage problem from 12.1.1
Verify that the application will not accept large files that could cause a denial of service.

Or just improve this requirement with the message, that application can accept however large files, but it must be able to handle/process them.

[tghosth]

12.1.1 needs to be focussed 100% on the risk of denial of service at processing time, not at storage time.

@set-reminder 3 weeks @tghosth to create a PR

[octo-reminder]

⏰ Reminder
Wednesday, January 18, 2023 12:00 AM (GMT+01:00)

@tghosth to create a PR

[elarlang]

Proposed:
Verify that the application will not accept large files that could cause a denial of service when they are processed.

I would like to see there something like "Verify that the application will not accept larger files than it can process and not causing denial of service attack."

The point to add / scenario to cover: by proposed requirement text, if application gets an error but without causing dos attack, it could be still ok.

[tghosth]

How about:

Verify that the application will not accept files which are larger than it can process without causing a loss of performance or availability (denial of service attack).

Otherwise, I am not sure what you mean by " if application gets an error but without causing dos attack, it could be still ok"

[elarlang]

I mean - if you try to upload some large file, program code just crashes. Server in general is ok, crash is affecting only this one user. But this one user can not upload the file he/she wants. It's not denial of service for other users, we can maybe watch it as denial of service for this one user.

Maybe just move brackets:
Verify that the application will not accept files which are larger than it can process (e. g without causing a loss of performance or denial of service attack).

[tghosth]

I mean - if you try to upload some large file, program code just crashes. Server in general is ok, crash is affecting only this one user. But this one user can not upload the file he/she wants. It's not denial of service for other users, we can maybe watch it as denial of service for this one user.

Maybe just move brackets: Verify that the application will not accept files which are larger than it can process (e. g without causing a loss of performance or denial of service attack).

I modified the PR #1514 on this basis

[elarlang]

I accepted and merged the PR, but reopened issue.

Do we want to reword it to "positive requirements" (what application should do) instead of "negative requirement" (what application should not do) like we have at the moment. If this is not needed, this issue can be closed.

[tghosth]

Great, point I opened #1519 which rewords it

[elarlang]

update merged, close

[octo-reminder]

🔔 @tghosth

@tghosth to create a PR