clarification needed for 12.4.1

elarlang commented 3 years ago

Verify that files obtained from untrusted sources are stored outside the web root, with limited permissions.

"spin-off" issue from https://github.com/OWASP/ASVS/issues/1065#issuecomment-944915831

I'm not too happy with this requirement. It feels a bit from old-style PHP crappy architecture, where file in public folder leads to RCE.

The main question is, should we just blindly disallow to store files in public folder in general, or make it a bit more flexible.

sometimes files are expected to stay in public folder, like "public" images (or we ask them to serve from another domain? is it reasonable?)
even if .php file is stored in public folder for "PHP application", it's important to avoid to execute it as PHP program code (by web-server configuration for example)

jmanico commented 3 years ago

I agree this needs work. Again, I like to delete confusing requirements. We do not need to be a complete standard, we need to be a clear one, is my opinion.

cmlh commented 3 years ago

We do not need to be a complete standard, we need to be a clear one, is my opinion.

Perhaps inserting an explanatory note would resolve the confusion @jmanico or something like the format below:

Boardline Requirement 1 1a. Conflicting Requirement 1a 1b. Conflicting Requirement 1b 1c. ...

jmanico commented 3 years ago

This is really old-school thinking. The situation is way more complex, I again suggest we just delete this requirement and move on.

elarlang commented 3 years ago

Take it as a placeholder, I'll come back to this one when looking entire V12.*

jmanico commented 2 years ago

I'd still like to delete it.

elarlang commented 2 years ago

proposal, delete as duplicate of 4.1.3: V4.1.3 Verify that the principle of least privilege exists - users should only be able to access functions, data files, URLs, controllers, services, and other resources, for which they possess specific authorization. This implies protection against spoofing and elevation of privilege.

jmanico commented 2 years ago

I agree with your proposal.

elarlang commented 2 years ago

12.4.1 is removed as duplicate of 4.1.3 (just one special case of authorization problems)

For fixing "decision time situation":

#	Description	L1	L2	L3	CWE
4.1.3	Verify that the principle of least privilege exists - users should only be able to access functions, data files, URLs, controllers, services, and other resources, for which they possess specific authorization. This implies protection against spoofing and elevation of privilege. (C7)	✓	✓	✓	285
12.4.1	Verify that files obtained from untrusted sources are stored outside the web root, with limited permissions.	✓	✓	✓	552

tghosth commented 1 year ago

Rise!GIF

tghosth commented 1 year ago

Sorry, @elarlang / @jmanico,

I am reopening this as I really don't agree that it is a duplicate of 4.1.3 and I don't think it should have been deleted.

This is the history of the requirement:

#	Description	L1	L2	L3
12.4.1	Verify that files obtained from untrusted sources are stored outside the web root, with limited permissions.	✓	✓	✓	552
12.4.1	[MODIFIED] Verify that files obtained from untrusted sources are stored outside the web root, with limited permissions.	✓	✓	✓	552
12.4.1	Verify that files obtained from untrusted sources are stored outside the web root, with limited permissions, preferably with strong validation.	✓	✓	✓	922
16.4.1	Verify that files obtained from untrusted sources are stored outside the web root, with limited permissions, preferably with strong validation.	✓	✓	✓	922
16.6	Verify that files obtained from untrusted sources are stored outside the web root, with limited permissions, preferably with strong validation.		✓	✓	3.0

Basically it comes from a 3.0 requirement and I agree that the permissions/validation part is weird and that the webroot part is a little 90s but I still think the "web root" part is important and I don't want it to get lost.

I would propose the following, (note the updated CWE):

#	Description	L1	L2	L3	CWE
12.4.1	[MODIFIED] Verify that files obtained from untrusted sources are stored outside the web root so that there is no risk of accidental execution.	✓	✓	✓	553

@elarlang @jmanico what do you think?

elarlang commented 1 year ago

Well, not that easy.

[MODIFIED] Verify that files obtained from untrusted sources are stored outside the web root so that there is no risk of accidental execution.

Authorization

12.4 is file storage category and was removed from that perspective.

From file execution perspective, it should be 12.3 requirement.

Client-Side execution

What about public content - like images, profile pictures, logos, some pdf files etc?

We have it kind of covered with othe requirement.

#	Description	L1	L2	L3	CWE
1.12.2	[MODIFIED] Verify that user-uploaded files - if required to be displayed or downloaded from the application - are served by either octet stream downloads, or from an unrelated domain, such as a cloud file storage bucket.		✓	✓	646
12.5.2	Verify that direct requests to uploaded files will never be executed as HTML/JavaScript content.	✓	✓	✓	434

I have made proposal to merge them: https://github.com/OWASP/ASVS/issues/1406

It would be wasting resources to serve static images via program code and may actually cause good DoS vectors.

But those are more client-side oriented.

Server-Side execution

Now the question to solve is - why it may happen, that users can upload as "public content" something, which is executed on the server side via HTTP request as program code.

I would call it business logic problem.

#	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

See also opened related issues: https://github.com/OWASP/ASVS/issues/1604

We also have another requirement to play with.

#	Description	L1	L2	L3	CWE
14.3.6	[GRAMMAR, MOVED FROM 12.5.1] Verify that the web tier is configured to serve only files with specific file extensions to prevent unintentional information and source code leakage. For example, backup files (e.g. .bak), temporary working files (e.g. .swp), compressed files (.zip, .tar.gz, etc.) and other extensions commonly used by editors should be blocked unless required.	✓	✓	✓	552

What we clearly don't have is (in whatever wording): Verify that files obtained from untrusted sources are not executed as program code when directly accessed with HTTP request.

tghosth commented 1 year ago

Ok, I am going to chalk this up for the rework of 12

jmanico commented 3 months ago

I like the merge proposal at https://github.com/OWASP/ASVS/issues/1406 and this is well under way
Also see that 12.4.1 is removed
12.4.1 [DELETED, DUPLICATE OF 4.1.3]

Can we close this out for now and move these issues to a new issue if needed?

tghosth commented 2 months ago

I'd like to keep this open until we are sure

jmanico commented 1 month ago

I re-read this and do agree that in the context of uploading files, we need to stage uploaded files in directories that are permissioned properly. This is a fundamental requirement to secure file upload.

I vote we revive 12.4.1 with a requirement like:

Verify that uploaded files from untrusted sources are being stored in a secure, non-web-accessible directory or other data store, with restrictive file system permissions to prevent unauthorized access or execution.

elarlang commented 1 month ago

Verify that files obtained from untrusted sources are not executed as program code when directly accessed with HTTP request.

It does not take into account public static files by their logical meaning, such as profile images,

I think the problem to address (written in https://github.com/OWASP/ASVS/issues/1087#issuecomment-1568391839) is:

What we clearly don't have is (in whatever wording): Verify that files obtained from untrusted sources are not executed as program code when directly accessed with HTTP request.

jmanico commented 1 month ago

Good point. How about:

Verify that uploaded files from untrusted sources are being stored in a secure, non-web-accessible directory or other data store, with restrictive file system permissions to prevent unauthorized access or execution. If files like profile images must be stored in a public directory, ensure they are not executable or contain malware.

elarlang commented 1 month ago

If files like profile images must be stored in a public directory, ensure they are not executable

"executable" here is not that clear I guess, you can execute file if you have such permissions. My guess is that the original requirement comes from PHP era - there files are "executed" with direct HTTP request, I'm not sure that execute permissions are required for this or file-read permissions for a web server (like Apahce) is enough?

... or contain malware.

This is covered by 12.4.2: Verify that files obtained from untrusted sources are scanned by antivirus scanners to prevent upload and serving of known malicious content.

jmanico commented 1 month ago

Profile images are generally safe if you deploy them in a read only directory. The problems of the past were that an image might be a gif, but it’s really a shell script can be executed if it’s delivered with executable file permission. Actually directory needs executable permissions to diverse the directory. It’s just the file itself that only needs to be read only.

I still think it’s sensible if you’re uploading an image that you want to be in a public web directory there’s no reason for the file to be anything other than read only. The problem specifically with gif’s have been relatively recent within the last couple of years.

jmanico commented 1 month ago

How about:

_Verify that uploaded files from untrusted sources are being stored in a secure, non-web-accessible directory or other data store, with restrictive file system permissions to prevent unauthorized access or execution. If files like profile images must be stored in a public directory, ensure the files permissions are read only._

tghosth commented 1 month ago

@elarlang to provide an updated option

elarlang commented 1 month ago

I re-read it, and I think it is not clear, what is the problem to solve with this proposed requirement?

So the re-usable template for everyone to be used in every issue: "To prevent /attack, vulnerability/ Verify that / what need to be done to solve or avoid the problem /"

jmanico commented 1 month ago

It’s to prevent any uploaded code from being executed on the server, typically a form of command injection via malformed images and other uploaded files that can be executed as code. There is a rich history of this form of attack.

elarlang commented 1 month ago

If malformed image is executed, it can be 2 reason:

problem in image processing library (known vuln, not up to date library) or the way it is called (command injection)
image itself is being executed as program code, e. g. so called polyglots (we have requirement 14.3.6 to limit the file extensions being allowed)

So, as concluded before, the only "problem to solve" is that "Verify that files uploaded or generated by untrusted input stored in public folder are not executed as program code when accessed directly with HTTP request".

jmanico commented 1 month ago

The problem with the image library (which has dozens of historical examples) should be handled in requirement to keep components updated.

image itself is being executed as program code, e. g. so called polyglots (we have requirement 14.3.6 to limit the file extensions being allowed)

The whole point of file permission limitations is that this class of attack allowed even files with .gif extensions (or in the distant past wmf files and other file types), and with magic bytes claiming to be gif to still be executable ik certain cases. This is why you want to set the file permissions of uploaded files that you publically deploy to be read only. It’s a very basic and easy to implement control that fully shuts this category of attacks down.

Gifs, ICO, SVG and others have had this problem.

Or let me ask another question. Would you want an uploaded file of any type that you stage up in a public directory to be permissioned as executable? This seems like a very bad idea to me.

elarlang commented 1 month ago

What and why is going to execute it?

jmanico commented 1 month ago

Why? Why do attackers attack? I don’t understand the question.

There is a lot of research on uploaded images being executable historically. For a wide variety of reasons depending on the web server type, the app server type, and the image type.

So are you suggesting that staging up uploaded files in a public directory with file permissions being set to executable to be an acceptable practice? Really?

elarlang commented 1 month ago

Kind of "here we go again" with demagogy.

If you allow user to upload (edit: a php) file to public folder that is executed when directly accessed with an HTTP request, it does not require to have "executable" permissions. Read-only is enough.

And if it comes to "executable", then something - some process must execute it. This was my question, what process and why it can execute the file?

jmanico commented 1 month ago

Kind of "here we go again" with demagogy.

This is very insulting when I am in good faith trying for to help. Please stick to the technical conversation. Personal attacks help no one.

And if it comes to "executable", then something - some process must execute it. This was my question, what process and why it can execute the file? Since its in a public directly any user can navigate to that file. JS files can impact node servers. Gif-Jar’s impact Java servers. There are numberous situations again depending on the server type.

Setting piblically available uploaded files to be read only (depending on the server and other factors) shuts down:

Remote Code Execution (RCE)
Web shell attacks
File inclusion vulnerabilities (LFI/RFI)
Privilege escalation
Denial of Service (DoS) through malicious scripts
Soe forms of Cross-site scripting (XSS) and other injection-based attacks

And even if other ASVS items (like file validation) address these issues, setting uploaded publically staged files to read only - is a fairly standard secure coding practice.

elarlang commented 1 month ago

For the mirror.

So are you suggesting that staging up uploaded files in a public directory with file permissions being set to executable to be an acceptable practice? Really?

Setting piblically available uploaded files to be read only (depending on the server and other factors) shuts down:

Sorry, but I would say that you don't know what you are talking about. For example, LFI only requires reading permission.

jmanico commented 1 month ago

Sorry, but I would say that you don't know what you are talking about. For example, LFI only requires reading permission.

Again with the personal attacks….

I undetand LFI Elar. But read only permissions reduces the overall likelihood of file upload problems in this category. As mentioned above, this is not just about LFI.

elarlang commented 1 month ago

Let's come back to this one.

So are you suggesting that staging up uploaded files in a public directory with file permissions being set to executable to be an acceptable practice? Really?

Did I say anything towards that? No. It is your attempt to "assign" this to me, and then saying I'm attacking you :)

There is no reason to add executable permissions to uploaded files, but only removing executable permissions from files that are stored in public folders, but otherwise being directly handled by the web server (classical php solution), do not solve ANY of your listed problems. So, your proposed solution does not solve the problem (avoid file getting executed) and I think my conclusion out of that is aligned.

jmanico commented 1 month ago

There is no reason to add executable permissions to uploaded files, but only removing executable permissions from files that are stored in public folders, but otherwise being directly handled by the web server (classical php solution), do not solve ANY of your listed problems. So, your proposed solution does not solve the problem (avoid file getting executed) and I think my conclusion out of that is aligned.

So can you think of any situation where read only vs executable for public uploaded files helps reduce the risk of any web based attack or do you think this is just an unreasonable defense to suggest?

I’m not trying to lead you on, I am genuinely asking.

This is a control I’ve been asked by security engineers to implement for 20+ years. I wonder if all this time it was not necessary.

elarlang commented 1 month ago

I think the answer was already in quoted text:

There is no reason to add executable permissions to uploaded files, but only removing executable permissions from files that are stored in public folders, /.../, do not solve ANY of your listed problems.

tghosth commented 1 month ago

So I feel like this thread got a bit confused.

@elarlang suggested above the following requirement (I made a minor wording change):

Verify that files uploaded or generated by untrusted input which are stored in a public folder are not executable as program code when accessed directly by an end user.

@jmanico is there anything missing from that from your perspective?

jmanico commented 1 month ago

This works for me.

elarlang commented 1 month ago

One may argue that it disallows executing client-side javascript, so maybe we limit it to the server-side program code?

Verify that files uploaded or generated by untrusted input which are stored in a public folder are not executable as server-side program code when accessed directly by an end user.

jmanico commented 1 month ago

I like your suggestion even better, @elarlang - nice ending to this. Thank you.

tghosth commented 1 month ago

One may argue that it disallows executing client-side javascript, so maybe we limit it to the server-side program code?

Verify that files uploaded or generated by untrusted input which are stored in a public folder are not executable as server-side program code when accessed directly by an end user.

Great, let's get this PRed in :) @elarlang

randomstuff commented 1 month ago

Verify that files uploaded or generated by untrusted input which are stored in a public folder are not executable as server-side program code when accessed directly by an end user.

One may argue that it disallows executing client-side javascript, so maybe we limit it to the server-side program code?

Actually I understood it mainly the other way :) i.e. if the user uploads an HML or SVG file, you want to make sure that this application won't get executed on the client-side (browser).

This is a typical vector for client-side JS injection for example when you let users upload images:

1) malicious user uploads SVG file containing malicious JavaScript code; 2) the malicious SVG is exposed/available in the application origin; 3) malicious user redirects you to this SVG file; 4) SVG file is executed in your browser (including JavaScript !) and an use your session.

elarlang commented 1 month ago

For this scenario we have those:

V50.5 Unintended Content Interpretation

#	Description	L1	L2	L3	CWE
50.5.1	[GRAMMAR, MOVED FROM 12.5.2] Verify that direct requests to uploaded files will never be executed as HTML and JavaScript content.	✓	✓	✓	434
50.5.2	[MODIFIED, MOVED FROM 1.12.2] Verify that user-uploaded files - if required to be displayed or downloaded from the application - are served by either octet stream downloads, or from an unrelated domain, such as a cloud file storage bucket.		✓	✓	646
50.5.3	[ADDED, DEPRECATES 14.4.2] Verify that security controls are in place to prevent browsers from rendering content or functionality in HTTP responses in an incorrect context (e.g., when an API or other resource is loaded directly). Possible controls could include: not serving the content unless headers indicate it is the correct context, Content-Security-Policy: sandbox, Content-Disposition: attachment, etc.	✓	✓	✓

Related issue to merge 50.5.1 and 50.5.2: https://github.com/OWASP/ASVS/issues/1406

OWASP / ASVS