ASVS
ASVS copied to clipboard
OWASP
I think the requirement 12.2.1 should be more explicit
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 https://github.com/hansphp/ASVS/commit/d1b32f2c60dab790e12f048e177b8bd4390e54da
I request a hearing for PR.
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?
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,
@hansphp - can you please clarify, what is the precise problem your proposal should solve?
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.
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".
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.
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"
??
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.
@SoftwareSinner I'm afraid I am not sure I understand why you want to make that change...
@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.
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.
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
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.
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.
"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.
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?
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.
For example, an image file should have properties such as width and height.
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.
@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.
I would suggest this check: Verify that files obtained from untrusted sources are validated to be of expected type based on the file's content, using methods such as checking file extensions, validating file magic numbers, and verifying MIME types.
As suggested in my closed issue https://github.com/OWASP/ASVS/issues/1623, we should have a requirement like the following. This is in agreement with some of above comments.
12.2.x (new) Verify that user-submitted filename extension matches content type. CWE-646
@ImanSharaf I am not sure what "verifying mime type" means. A mime type might come with a file sent a in request but that should probably be disregarded anyway.
@ImanSharaf @securitydave I also think we need to word this carefully to be clear about what we are checking and what we trust. So for example, our expectations we can trust and the format of the content of the file we can trust. Everything else such as file extension and mime type is untrusted data.
So @ImanSharaf, @securitydave @elarlang what do you think about the following?
| # | Description | L1 | L2 | L3 | CWE |
|---|---|---|---|---|---|
| 12.2.1 | [MODIFIED] Verify that contents of files being processed by the application are validated to match the expected type and that the file has the correct file extension, including but not limited to checking the initial "magic bytes" of the file. | ✓ | ✓ | 434 |
Related to previous comments:
- https://github.com/OWASP/ASVS/issues/1291#issuecomment-1212102771
- https://github.com/OWASP/ASVS/issues/1291#issuecomment-1212113067
- https://github.com/OWASP/ASVS/issues/1291#issuecomment-1245747794
Verify that contents of files being processed by the application are validated
If I read the requirement, for me it does not cover scenario "application allows users to upload files and later serves them" (without processing them on the server side). We can call accepting and later serving a file also processing, but there is assumption (which is mother of all ...) that everyone interpret it that way.
I don't have any wordsmithing proposals, just pointing out the problem.
So how about the following @elarlang:
| # | Description | L1 | L2 | L3 | CWE |
|---|---|---|---|---|---|
| 12.2.1 | [MODIFIED] Verify that contents of files being accepted by the application are validated to match the expected type and that the file has the correct file extension, including but not limited to checking the initial "magic bytes" of the file. | ✓ | ✓ | 434 |
I suggest the following reshuffle of the sentence: Verify that the contents of each file being accepted by the application are validated to match the expected type, including but not limited to checking the initial "magic bytes" of the file, and that the file has the correct file extension.
I suggest the following reshuffle of the sentence: Verify that the contents of each file being accepted by the application are validated to match the expected type, including but not limited to checking the initial "magic bytes" of the file, and that the file has the correct file extension.
So I worded this quite deliberately @securitydave but maybe it needs to be clearer.
To my mind the following process needs to take place.
- Have an expectation for the file types to be accepted.
- Verify that the file extension matches that expectation (although this can be spoofed)
- If 2 is successful, verify that the contents of the file matches the expected type from 2), including but not limited to checking magic bytes.
So what do you think about:
| # | 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 type and it validates that the contents of the file match that type, including but not limited to checking the initial "magic bytes". | ✓ | ✓ | 434 |
In PR #1681, @elarlang said:
it checks that the file extension of the file matches an expected type
instead of "expected type" maybe we should mention "allowed list of file extensions"? File extension and type is just not intuitive to put together,
So how about this @elarlang ?
[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".