user-submitted filename metadata (V12.3)

[Sjord]

ASVS/0x20-V12-Files-Resources.md at master · OWASP/ASVS · GitHub

#	Description	L1	L2	L3	CWE
12.3.1	Verify that user-submitted filename metadata is not used directly by system or framework filesystems and that a URL API is used to protect against path traversal.	✓	✓	✓	22
12.3.2	Verify that user-submitted filename metadata is validated or ignored to prevent the disclosure, creation, updating or removal of local files (LFI).	✓	✓	✓	73
12.3.3	Verify that user-submitted filename metadata is validated or ignored to prevent the disclosure or execution of remote files via Remote File Inclusion (RFI) or Server-side Request Forgery (SSRF) attacks.	✓	✓	✓	98
12.3.5	Verify that untrusted file metadata is not used directly with system API or libraries, to protect against OS command injection.	✓	✓	✓	78

I think "filename metadata" is incorrect here. It would be information about the filename, not the filename itself.

I think "filename metadata" should be either "filename" or "file metadata". I would prefer "file metadata", as that would include both the filename and content-type, and any other file information sent by the client.

[elarlang]

Oh, I have postponed to reorganize this section as I have some big tasks here opened already, but in this section are so many things which are incorrect or I just don't like (https://github.com/OWASP/ASVS/issues/1136#issuecomment-974680885). For a starter:

• user-submitted - unnecessary limitation, should be for every filename or metainfo an application processes
• in general we should just say - filename and metainfo are user input and we can use other requirements in V5 category
• 12.3.1 - no clear path traversal requirement in kind of covered in V5.2.2
• 12.3.2 - overlaps with 5.2.6, 5.3.9
• 12.3.3 - overlaps with 5.2.6, 5.3.9
• 12.3.5 - duplicate with 5.3.8

Related requirements:	#	Description	L1	L2	L3	CWE
5.2.2	Verify that unstructured data is sanitized to enforce safety measures such as allowed characters and length.	✓	✓	✓	138
5.2.6	Verify that the application protects against SSRF attacks, by validating or sanitizing untrusted data or HTTP file metadata, such as filenames and URL input fields, and uses allow lists of protocols, domains, paths and ports.	✓	✓	✓	918
5.3.8	Verify that the application protects against OS command injection and that operating system calls use parameterized OS queries or use contextual command line output encoding. (C4)	✓	✓	✓	78
5.3.9	Verify that the application protects against Local File Inclusion (LFI) or Remote File Inclusion (RFI) attacks.	✓	✓	✓	829

edit: update 2024-09-04. Requirement 5.3.9 is deleted as duplicate of 12.3.2, 12.3.3.

[elarlang]

I think this entire "#V12.3 File Execution" should be moved away from V12 - all other requirements in V12 are more related to handling user-uploaded or application generated files as content.

V12.6.1 is already waiting to be moved away but there is no clear-good category yet (#1343).

From V12.3:

• 12.3.1 path traversal - should be in validation + sanitization
• 12.3.2 local file - the point is the same with path traversal, it should be not possible to manipulate the path where the resource is created/read/deleted
• 12.3.3 - do we actually need this requirement? from rfi point of view, if 12.3.1 and 12.3.2 are done well, this problem can not exists. ssrf part is covered by 5.2.6
• 12.3.5 cmd injection - merge to 5.3.8 (I opened separate issue #1472 for that)
• 12.3.6 this is more malicous code execution from external sources, V10 category topic. merge to 10.3.2 (I opened separate issue #1471 for that)

[tghosth]

@set-reminder 3 weeks @tghosth to look at this

[octo-reminder[bot]]

⏰ Reminder Wednesday, January 18, 2023 12:00 AM (GMT+01:00)

@tghosth to look at this

[tghosth]

My interpretation is that 12.3.1 is talking about path traversal leading to arbitrary file download and 12.3.2 is talking about using a the filename provided to upload functionality to control where a file is stored, e.g. leading to deleting a sensitive file or a webshell. This seems to be supported by the relevant CWE descriptions.

Regardless I agree that the section needs rework.

[elarlang]

Category name for 12.3 is file execution - not file upload (which is separate category 12.1) or file download (which is separate category 12.5)

12.3.1 There are many ways how to reach to file download. For example access control problems - files stored in public folder or IDOR, but we don't duplicate V4 requirements in V12. The same way I think we should handle input validation / sanitization problem in V5 category.

12.3.2/12.3.3 as mentioning LFI and RFI are clear file execution requirements, which is impact. Base problem is still not validated path or url when resource is loaded. Abstractly the same problem as 12.3.1 - "arbitrary resource loading". Again there can be alternative ways how to reach to arbitrary resource loading - using again architecture problems, uploading PHP files (as LFI and RFI mostly are PHP related) to public folder - access it directly and effect is the same.

Should we get in some file processing requirements like handling zip files and then see, how we need to change/create categories for them:

• https://github.com/OWASP/ASVS/issues/1424
• https://github.com/OWASP/ASVS/issues/1222

[tghosth]

History for 12.3.3:

#	Description	L1	L2	L3	CWE
12.3.3	Verify that user-submitted filename metadata is validated or ignored to prevent the disclosure or execution of remote files via Remote File Inclusion (RFI) or Server-side Request Forgery (SSRF) attacks.	✓	✓	✓	98
12.3.3	Verify that user-submitted filename metadata is validated or ignored to prevent the disclosure or execution of remote files (RFI), which may also lead to SSRF.	✓	✓	✓	98
16.4	Verify that untrusted data is not used within inclusion, class loader, or reflection capabilities to prevent remote/local code execution vulnerabilities.	✓	✓	✓	tbd	tbd

(That last one is not 100% match)

[octo-reminder[bot]]

🔔 @tghosth

@tghosth to look at this

[tghosth]

This was tagged as V5 but my inclination is to keep it as V12 as it is specific to files and V5 is a little overloaded :)

[jmanico]

This thread got out of hand. I want to address the original posters comments.

1) I think "filename metadata" is incorrect here. It would be information about the filename, not the filename itself. 2) I think "filename metadata" should be either "filename" or "file metadata". I would prefer "file metadata", as that would include both the filename and content-type, and any other file information sent by the client.

I think these comments are spot on and need to be addressed. Here are my thoughts on how to address the OP and how to fix these problems.

12.3.1	Verify that the user-submitted filename or file metadata is not used directly when creating filepaths for stored files to protect against path traversal.	✓	✓	✓	22
12.3.2	Verify that user-submitted filenames or file metadata is validated or ignored to prevent the disclosure, creation, updating or removal of local files (LFI).	✓	✓	✓	73
12.3.3	Verify that user-submitted filenames or file metadata is validated or ignored to prevent the disclosure or execution of remote files via Remote File Inclusion (RFI) or Server-side Request Forgery (SSRF) attacks.	✓	✓	✓	98
12.3.5	Verify that user-submitted filenames or file metadata is not used directly with system API or libraries, to protect against OS command injection.	✓	✓	✓	78

I think these are all necessary to ensure that this section (v12) is complete to help folks build secure file uploaded features.

[elarlang]

note that 12.3.5 is removed as duplicate of 5.3.8

My previous comment (https://github.com/OWASP/ASVS/issues/1427#issuecomment-1363380134):

• 12.3.1 path traversal - should be in validation + sanitization
• 12.3.2 local file - the point is the same with path traversal, it should be not possible to manipulate the path where the resource is created/read/deleted
• 12.3.3 - do we actually need this requirement? from rfi point of view, if 12.3.1 and 12.3.2 are done well, this problem can not exists. ssrf part is covered by 5.2.6

Proposal from Jim is ok from wording perspective, but I question the need to have 3 separate requirements for the same problem in the program code with just having 3 different attack codes to test them.

• 12.3.1 - path traversal when writing a file
• 12.3.2 - non-validated/sanitized userinput that may cause path traversal and/or using full path for files for any operation with files
• 12.3.3 - the same as 12.3.2 but using full url's as input

Many situations in vulnerable program codes matches all 3 at the same time.

[jmanico]

Good call Elar. How about we merge these into one, something like:

12.3.1	Verify that the user-submitted filename or file metadata is not used directly when creating filepaths for stored files to protect against path traversal, SSRF, LFI and similar attacks that use filename and filename metadata from untrusted sources.	✓	✓	✓	22

[tghosth]

Merged looks great, @elarlang to do some wording fine tuning

[elarlang]

To also have positive requirement, just some example for the direction:

Verify that file operations are safe from manipulating the resource path or address with data from untrusted sources - using validation and sanitization to avoid path traversal, local- or remote file inclusion, and server-side request forgery attacks.

[jmanico]

I'd rather not suggest validation or sanitization (this is just too error prone with file operations) and instead just avoid user data for file operations all together. My suggestion:

Verify that file operations do not use user-submitted filenames or file metadata when creating filepaths to protect against path traversal, local or remote file inclusion (LFI/RFI), and server-side request forgery (SSRF) attacks, by preventing manipulation of resource paths with data from untrusted sources.

[elarlang]

I don't think it is a realistic suggestion and it is against widespread reality how the application handling uploaded file names (for example). If you upload an attachment to be sent via email, you could suggest that the uploaded file does not have the same name you just uploaded?

[jmanico]

You can upload a file and use an internal trusted name (like an ID) and still preserve the filename for display only. You do not ever need to use the filename for actual file-IO. This is a very common secure dev practice.

[elarlang]

Explanation makes sense, but as one valid option how to do that. If there is validation and sanitization that gives defense, we can not say it is wrong.

Let's say there is situation, that only letter and numbers are kept in the user input, with max length 50 symbols and stored as file name. Someone is watching ASVS requirement and points out that the application is not valid for ASVS...

[jmanico]

How about we suggest both?

Avoid using the user data OR validate/sanitize?

[elarlang]

Yes, this is ok.

[jmanico]

How about this for 12.3.1 (and we eliminate 12.3.2 and 12.3.3) (and 12.3.4 and 12.3.5 are already removed from v12)

Verify that file operations do not use user-submitted filenames or file metadata when creating filepaths to protect against path traversal, local or remote file inclusion (LFI/RFI), and server-side request forgery (SSRF) attacks, by preventing manipulation of resource paths with data from untrusted sources. If user-submitted filenames or file metadata must be used in file operations then use strict validation and sanitization.

[tghosth]

@elarlang to confirm on this