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

Consideration of Pixel Flood Attack Vulnerability in File Upload Functionality #1740

Closed
ImanSharaf opened this issue Sep 29, 2023 · 19 comments · Fixed by #1803
Closed

Consideration of Pixel Flood Attack Vulnerability in File Upload Functionality #1740

ImanSharaf opened this issue Sep 29, 2023 · 19 comments · Fixed by #1803
Assignees
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 6) PR awaiting review V12 _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@ImanSharaf
Copy link
Collaborator

I would like to bring to attention a potential vulnerability related to file upload functionality, specifically concerning the handling of image files. A simple yet impactful attack vector, known as the Pixel Flood Attack, can be executed whenever there is a file upload functionality that accepts images. In a Pixel Flood Attack, an attacker attempts to upload an image file with an exceedingly large pixel size, which could result in significant server resource consumption. This, in turn, may lead the application to crash, creating a scenario akin to an application-level denial of service (DoS).

Modern applications frequently leverage third-party libraries to process large images and convert them into smaller, more manageable sizes, thus conserving storage and processing power. However, some of these libraries might be susceptible to the Pixel Flood Attack, potentially leading to resource exhaustion or an Application-Level DoS attack.

This vulnerability could be of particular concern in scenarios where applications have not implemented adequate checks on the dimensions of the uploaded image files. Given the rise in the utilization of third-party image processing libraries, it's imperative to ensure that they are resilient against such attack vectors.

@elarlang
Copy link
Collaborator

elarlang commented Sep 30, 2023

Good one, I think there are other ways to achieve it also, like pbm images.

Now the good question is, should we have separate requirement for images to "check the dimensions to be in expected range" or use some other requirement to cover it.

We have close requirement for this.

# Description L1 L2 L3 CWE
12.2.1 [MODIFIED] Verify that when the application is accepting a file, it checks that the file extension of the file matches an expected file extension and that it validates that the contents of the file match the type represented by that extension, including but not limited to checking the initial "magic bytes". 434

As separate requirement it should belong to 12.1, as there are some archive content validation requirements, so validating something image specific fits there as well.

# Description L1 L2 L3 CWE
12.1.2 Verify that the application checks compressed files (e.g. zip, gz, docx, odt) against maximum allowed uncompressed size and against maximum number of files before uncompressing the file. 409
12.1.4 [ADDED] Verify that the application does not allow uploading compressed files containing symlinks unless this is specifically required (in which case it will be necessary to enforce an allow list of the files that can be symlinked to). 61

@elarlang elarlang added the 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet label Oct 2, 2023
@tghosth
Copy link
Collaborator

tghosth commented Oct 9, 2023

I agree this should be in a general file checking requirement. @ImanSharaf can you suggest where to add it?

@tghosth tghosth added the _5.0 - prep This needs to be addressed to prepare 5.0 label Oct 9, 2023
@elarlang
Copy link
Collaborator

elarlang commented Oct 9, 2023

Verify that when the application is accepting a file, it checks that the file extension of the file matches an expected file extension and that it validates that the contents of the file match the type represented by that extension, including but not limited to checking the initial "magic bytes".

To point out more clearly: ... including but not limited to checking the initial "magic bytes".

So, if I find this vulnerability, requirement 12.2.1 is something to use for reporting. The question is - should be put some special hilight on the attack vector in the 12.2.1 or in some new separate requirement.

@tghosth
Copy link
Collaborator

tghosth commented Oct 22, 2023

So I don't think 12.2.1 applies in this case because @ImanSharaf is talking about a situation where the filetype is as expected but it effectively contains a denial of service payload.

As such, @ImanSharaf it would be good to get your perspective on the specific requirement to be added and where?

@elarlang
Copy link
Collaborator

elarlang commented Oct 22, 2023

If it will be separate requirement, we can look current 12.1.2 as a direction.

# Description L1 L2 L3 CWE
12.1.2 Verify that the application checks compressed files (e.g. zip, gz, docx, odt) against maximum allowed uncompressed size and against maximum number of files before uncompressing the file. 409

One more question to answer - should we watch as malicious content in general and handle them like viruses, or should we have requirement only to protect an application - if you going to create thumbnails and load image to the memory based on dimensions described by user, then validate the maximum allowed dimensions.

@tghosth
Copy link
Collaborator

tghosth commented Oct 23, 2023

If it will be separate requirement, we can look current 12.1.2 as a direction.

I think there is a theoretical risk in trying to cover all the possible bad things that can happen in an uploaded file, it may be there is no limit to the possibilities.

On the otherhand, in the same way as we specifically talk about zipped files, images are also highly common with some common risks.

As such, I think we should have a separate requirement, similar to 12.1.2 about this issue.

One more question to answer - should we watch as malicious content in general and handle them like viruses, or should we have requirement only to protect an application - if you going to create thumbnails and load image to the memory based on dimensions described by user, then validate the maximum allowed dimensions.

We already have 12.4.2 to cover this:

# Description L1 L2 L3 CWE
12.4.2 Verify that files obtained from untrusted sources are scanned by antivirus scanners to prevent upload and serving of known malicious content. 509

@elarlang
Copy link
Collaborator

Yes I know we have separate requirement for viruses, my question was - should we watch the pixel flooded image as virus as well? It's clearly malicious content.

And the zip bomb (12.1.2)? And symlinked zip (12.1.4)? It makes sense to put them to one subcategory?

@tghosth
Copy link
Collaborator

tghosth commented Oct 23, 2023

Same category, maybe. Some requirement, no

@elarlang
Copy link
Collaborator

Clarification:

  • On topic (the issue) - new separate requirement.
  • sidetopic - zip bomb, zip symlink, viruses and pixel flood - can be put to the same subcategory as separate requirements as those all are clearly malicious inputs

@tghosth
Copy link
Collaborator

tghosth commented Oct 23, 2023

On topic (the issue) - new separate requirement.

I would suggest:

12.2.x Verify that the application enforces a maximum pixel size for uploaded images to prevent pixel flood attacks.

Maybe CWE 400?

L1

sidetopic - zip bomb, zip symlink, viruses and pixel flood - can be put to the same subcategory as separate requirements as those all are clearly malicious inputs

Opened #1769

@elarlang
Copy link
Collaborator

12.2.x Verify that the application enforces a maximum pixel size for uploaded images to prevent pixel flood attacks.

What means "enforces" in this context?

@tghosth
Copy link
Collaborator

tghosth commented Oct 24, 2023

enforces = uses input validation on images with a pixel size larger than the maximum allowed to either reject them or reduce the pixel size down to the maximum

@elarlang
Copy link
Collaborator

or reduce the pixel size down to the maximum

but pixel flood attack is addressing exactly this functionality, no?

Maybe we should watch this potential pixel flood image as zip bomb or virus content - clearly malicious, do not proceed?

@ImanSharaf
Copy link
Collaborator Author

An antivirus doesn't detect that as a malicious file!

@ImanSharaf
Copy link
Collaborator Author

On topic (the issue) - new separate requirement.

I would suggest:

12.2.x Verify that the application enforces a maximum pixel size for uploaded images to prevent pixel flood attacks.

Maybe CWE 400?

L1

sidetopic - zip bomb, zip symlink, viruses and pixel flood - can be put to the same subcategory as separate requirements as those all are clearly malicious inputs

Opened #1769

I am good with this.

@elarlang
Copy link
Collaborator

An antivirus doesn't detect that as a malicious file!

I wrote: Maybe we should watch ...

@elarlang elarlang added the next meeting Filter for leaders label Nov 29, 2023
@tghosth
Copy link
Collaborator

tghosth commented Nov 30, 2023

On topic (the issue) - new separate requirement.

I would suggest:

12.2.x Verify that the application enforces a maximum pixel size for uploaded images to prevent pixel flood attacks.

Maybe CWE 400?

L1

sidetopic - zip bomb, zip symlink, viruses and pixel flood - can be put to the same subcategory as separate requirements as those all are clearly malicious inputs

Opened #1769

Following discussion with @elarlang,

12.2.x Verify that the application blocks uploaded images with a pixel size larger than the maximum allowed, to prevent pixel flood attacks.

@tghosth
Copy link
Collaborator

tghosth commented Nov 30, 2023

@ImanSharaf are you ok with that as well

@tghosth tghosth removed the next meeting Filter for leaders label Nov 30, 2023
@elarlang elarlang added the V12 label Dec 6, 2023
@elarlang elarlang added the 2) Awaiting response Awaiting a response from the original poster label Dec 6, 2023
elarlang added a commit that referenced this issue Dec 14, 2023
@elarlang elarlang linked a pull request Dec 14, 2023 that will close this issue
@elarlang
Copy link
Collaborator

So, I took:

and created PR: #1803

If @ImanSharaf has some improvement ideas, we can reopen the issue.

Category and section may be changed in the future - #1769

@elarlang elarlang added 6) PR awaiting review and removed 2) Awaiting response Awaiting a response from the original poster labels Dec 14, 2023
tghosth pushed a commit that referenced this issue Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1) Discussion ongoing Issue is opened and assigned but no clear proposal yet 6) PR awaiting review V12 _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants