I think the requirement 12.2.1 should be more explicit

[hansphp]

Hello,
I think the requirement 12.2.1 should be more explicit.
I have noticed that many developers confuse content integrity with file signature.
I suggest the following hansphp@d1b32f2

I request a hearing for PR.

[elarlang]

Current requirement:

#	Description	L1	L2	L3	CWE
12.2.1	Verify that files obtained from untrusted sources are validated to be of expected type based on the file's content.		✓	✓	434

Point for the requirement: if you are waiting image files, then user can not upload any other formats.

Can you explain - how your recommendation improve the requirement or what exact problem it solves?

[RafaelGreen1]

Hi,
I think that @hansphp meant to say that the heading above the table doesn't match the table's content.
The heading is: V12.2 File Integrity
Thanks,
Rafael,

[elarlang]

Agree with that word, for me it's basically input validation issue here.

[elarlang]

@hansphp - can you please clarify, what is the precise problem your proposal should solve?

[hansphp]

Of course, Need to be more explicit for these cases:

Suppose we have this file:

Under specific configuration conditions it could be executed:

The above file complies with being of the correct extension, of the correct file type, but does not comply with the overall integrity of the file.

This is what this point is about and most file input validations are only restricted to validating extension and file type.

There are other more complex attacks such as PHP Wrapping, where a breach like this would allow LFI to be carried out.

[elarlang]

Ok, thank you for feedback.

For me the requirement text is covering this scenario clearly: "Verify that files obtained from untrusted sources are validated to be of expected type based on the file's content."

If we could start listing "for image files, check image size, for pdf check something else" - then we give examples, but the point for the requirement stays like it is at the moment.

Personally I would like to get rid of the phrase "untrusted".

[tghosth]

Hi @hansphp, I think I understand your point. To me it is about trying to be more sophisticated in how we verify that the file is really what it says it is.

What do you think about the following?

12.2.1:

Verify that files obtained from untrusted sources are validated to be of expected type based on the file's content. Ideally this should be more sophisticated than just checking the initial "magic bytes" of the file.

[SoftwareSinner]

This one has me scratching my head at times when reading it.

"Verify that files obtained from untrusted sources are validated to be of expected type based on the file's content."

I think maybe wording it as "Verify that files obtained from untrusted sources are validated to be of expected type or extension"

??

[elarlang]

I don't think we go in an improvement direction at the moment.

If someone says to me "do it a more sophisticated way", it does not say to me, what I actually need to do now.

... or extension

This is exactly the reason why this requirement exists - do not rely on the file extension.

The point for this requirement - the file actual content must be what you expect on the application side and a user can not upload javascript, php or whatever other files saying, that this is image by extension or by content-type.

First - how far we can go with that requirement? We have polyglot files - by every meaning correct image by content, but it can be still valid javascript or php file. But this part is more important how those files are accessed or served later.

If the application uses user-uploaded file name later on serving the file - it's important to have additional check, that for uploaded file - file content and file extension are matching. From serving the file perspective, it's covered by requirement V14.4.1:

Verify that every HTTP response contains a Content-Type header. Also specify a safe character set (e.g., UTF-8, ISO-8859-1) if the content types are text/*, /+xml and application/xml. Content must match with the provided Content-Type header.

The same applies, when serving the file, user-provided content-type value is used.

We can put this to the requirement, like be sure that file extension, content-type (mime-type) and file actual content are matching each other. But.. if the application just uses file content and it is in expected/allowed list then it does not matter what was the file extension or mime-type on file upload.

[tghosth]

@SoftwareSinner I'm afraid I am not sure I understand why you want to make that change...

[tghosth]

@elarlang I agree that how a file is served later is important but as you say we have a requirement to cover that.

I also think that there is value in enforcing controls around files when they are received. I take your point about the fact that it is not so actionable.

12.2.1:

Verify that files obtained from untrusted sources are validated to be of expected type based on the file's content, including but not limited to checking the initial "magic bytes" of the file.

Do you think this is clearer that we would like them to do more without making too much of a judgement? Clearly we cannot give more details to satisfy every case.

[elarlang]

What is the actual problem we are solving here?

Initial proposal for improvement was phrase:
"Not just checking the file signature. For example, an image file should have properties such as width and height."

And now we are close to opposite phrase:
including but not limited to checking the initial "magic bytes" of the file.

I don't mind to have the last proposal, but I don't mind also to keep the requirement like it is. If you give an example as "magic bytes", I think in practice it will be taken as the main test-case here and at the end it works exactly opposite direction against first proposal.

[tghosth]

I think the aim is to clarify what we mean but file content. If that ends up just being "magic bytes" then I think that is ok as it clarifies the sort of direction we are looking for. Do you think that is ok? @elarlang

[elarlang]

Ok, let's go with last proposal (Verify that files obtained from untrusted sources are validated to be of expected type based on the file's content, including but not limited to checking the initial "magic bytes" of the file.).

Like I mentioned, personally I would like to get rid of the phrase "untrusted" - from the application side, everything should be handled as untrusted.

My proposal (we can use also "all files" instead of "files"):
Verify that files are validated to be of expected type based on the file's content, including but not limited to checking the initial "magic bytes" of the file.

[tghosth]

Hey @elarlang
I understand dropping "untrusted sources" but I think we still need to be specific so how about:

Verify that files being processed by the application are validated to be of expected type based on the file's content, including but not limited to checking the initial "magic bytes" of the file.

[elarlang]

"being processed by the application" is also unnecessary limitation. For (business logic) integrity, the application must validate files also in case, when the application does not process them but only serving them for (for example backoffice) users.

[tghosth]

We need a way to be clear about the sort of files we are talking about here. We aren't just talking about every file involved in the application like code files. How do we specify this?

[elarlang]

Ok, got it. Probably we need to give an example for this "being processed by the application", that just accepting files as "user input" and later just serving them is already part of it.

[Sjord]

For example, an image file should have properties such as width and height.

PHP docs disagree:

Do not use getimagesize() to check that a given file is a valid image. Use a purpose-built solution such as the Fileinfo extension instead.

And Fileinfo looks at the magic bytes.

Verify that files obtained from untrusted sources are validated to be of expected type based on the file's content.

I think this is fine. For me "untrusted sources" means user input. We could say as much to make it clearer, but I think it's fine.

Checking the file's content only goes so far. I think checking magic bytes is fine. Actually validating whether something is an image can be pretty hard to do securely, especially if the validating image processor is different than the destination image processor. Furthermore, it is possible to put malicious code in valid images and documents, so this does not protect against <?php tags in your uploads.

[tghosth]

@elarlang what would you add to this requirement to give an example so as to make it clearer? I am not sure what you are suggesting

Verify that files being processed by the application are validated to be of expected type based on the file's content, including but not limited to checking the initial "magic bytes" of the file.

[jmanico]

When the application processes the file content (eg import, convert images, convert to pdf etc), it only requires correct content - file extension and content-type does not matter.

I disagree. File content and file extension need to match for a legit file.

[elarlang]

When the application processes the file content (eg import, convert images, convert to pdf etc), it only requires correct content - file extension and content-type does not matter.

I disagree. File content and file extension need to match for a legit file.

Can you give some examples?

Opposite examples:

• if you upload CSV content and an application imports it, it does not matter what is the file extension or content-type (in good use-case they are anyway duplicates) - an application just reads the content
• if you upload an image and the application makes its own resized version of it and the file name is not used and stored, then it does not matter what was file extension or content-type

[tghosth]

It might not matter but I think Jim's point is that it may be suspicious if a file is provided without an extension so maybe that would be considered a warning sign.

[jmanico]

There are files that claim to be a .gif that are really a script file that can be run as a shell script. In this case - a common attack - the file extension matches the magic bytes of the file, but the contents are not a valid .gif and do not match the file type. Any time contents do not match the file type, that file should be discarded. - Jim

[jmanico]

It might not matter but I think Jim's point is that it may be suspicious if a file is provided without an extension so maybe that would be considered a warning sign.

My intention is an uploaded multi-part form, the file name extension must match the magic bytes and the file contents. They must all be congruent for a valid file upload.

[elarlang]

I repeat the context - if the application processes the content but never uses the file name and/or provided content-type. Your provided scenario does not contain risk here - if the content is not a valid image (or even it's polyglot) and the application converts other format of images from that, there is no attack scenario from the file extension or content-type.

What I don't like here is exactly this - basically false negative reports - "in ASVS there is a requirement for that but it does not contain any risk in reality".

But like I said in #1291 (comment)

I'm ok with this one, but there is some potential to make it easier.

[jmanico]

ASVS is also to help *developers* build more secure applications if you do not understand from an attackers perspective. There is no good reason to accept a file upload when the file extension, is one exists, does not match the file magic bytes and content. It’s a (very basic) part of file upload input validation. It’s also helps detect malicious content if other controls like anti virus, which is faulty, misses a bad file.

[elarlang]

There is no good reason to accept a file upload when the file extension, is one exists, does not match the file magic bytes and content. It’s a (very basic) part of file upload input validation. It’s also helps detect malicious content if other controls like anti virus, which is faulty, misses a bad file.

I agree with that part - from functionality perspective it does not make sense ....

ASVS is also to help developers build more secure applications if you do not understand from an attackers perspective.

For educating developers we probably have other projects like cheat sheet series. ASVS stands for Application Security Verification Standard and we should not have requirements like "Verify that your code makes sense even if there is no risk in that situation". Malicious content and virus checks are different topic from that requirement perspective.

[jmanico]

What is wrong with verifying that the file extension, magic bytes and content all match? What is that a bad control to verify? Is this something that you struggle to test for? Is it to hard for you as a pentester?Message ID: ***@***.***>

[elarlang]

I wrote my concern here: #1291 (comment)

What I don't like here is exactly this - basically false negative reports - "in ASVS there is a requirement for that but it does not contain any risk in reality".

What I don't like here that service or product owners, who are ordering pen-testing/security assesment/however you call it service and they will have "noise" there: "you have huge problem because the application accepts valid content x with file extension Y". But the application does not use the file extension and "incorrect value" does not cause any security risk in practice.

So. This is my concern and this is what I don't like at the moment. At the same time I don't know how to improve it with keeping it still short and understandable. And also, with black-box pen-testing is not maybe detectable, do application uses file extension (or content-type) from user or not.

Like I wrote in the beginning of this extra-discussion round:

I'm ok with this one, but there is some potential to make it easier.

So, I accept the PR and let's see what the future brings with this one.

edit: @tghosth , please update the PR

[tghosth]

@elarlang ok so I have updated the PR, if you are comfortable enough with it, please merge it.

[elarlang]

If we want to develop the requirement further, then - there is "that" 5 times in the requirement.

[jmanico]

What is wrong with that?

[elarlang]

Nothing, just a bit repetitive. Pointed out as potential for improvement, not as a party stopper.

[jmanico]

Ok, that's cool, Elar.

[jmanico]

Actually we do need a grammar change

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

[jmanico]

Change suggestion with less "That"

12.2.1 [MODIFIED] Verify that when the application accepts a file, it checks if the file extension matches an expected file extension and validates that the contents correspond to the type represented by the extension. This includes, but is not limited to, checking the initial 'magic bytes'. ✓ ✓ 434

[jmanico]

Any may I also suggest:

12.2.1 [MODIFIED] Verify that when the application accepts a file, it checks if the file extension matches an expected file extension and validates that the contents correspond to the type represented by the extension. This includes, but is not limited to, checking the initial 'magic bytes', performing image re-writing, and using specialized libraries for file content validation. ✓ ✓ 434

[tghosth]

Looks good, want to open a PR @jmanico ?

[jmanico]

PR is in, @tghosth #2009