-
-
Notifications
You must be signed in to change notification settings - Fork 662
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
I think the requirement 12.2.1 should be more explicit #1291
Comments
Current requirement:
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, |
Agree with that word, for me it's basically input validation issue here. |
@hansphp - can you please clarify, what is the precise problem your proposal should solve? |
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:
|
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.
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:
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: And now we are close to opposite phrase: 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"): |
Hey @elarlang 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. |
And Fileinfo looks at the magic bytes.
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 |
@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
|
I disagree. File content and file extension need to match for a legit file. |
Can you give some examples? Opposite examples:
|
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. |
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
|
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. |
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)
|
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.
|
I agree with that part - from functionality perspective it does not make sense ....
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. |
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: ***@***.***>
|
I wrote my concern here: #1291 (comment)
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:
So, I accept the PR and let's see what the future brings with this one. edit: @tghosth , please update the PR |
@elarlang ok so I have updated the PR, if you are comfortable enough with it, please merge it. |
If we want to develop the requirement further, then - there is "that" 5 times in the requirement. |
What is wrong with that? |
Nothing, just a bit repetitive. Pointed out as potential for improvement, not as a party stopper. |
Ok, that's cool, Elar. |
Actually we do need a grammar change
|
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 |
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 |
Looks good, want to open a PR @jmanico ? |
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.
The text was updated successfully, but these errors were encountered: