OWASP / ASVS

Application Security Verification Standard
Creative Commons Attribution Share Alike 4.0 International
2.7k stars 660 forks source link

Deduplicate SSRF requirements #2115

Open tghosth opened 2 hours ago

tghosth commented 2 hours ago

We currently have the following requirements which refer to SSRF:

# Description L1 L2 L3 CWE
5.2.6 [GRAMMAR] 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 an allowlist of protocols, domains, paths and ports. 918
5.5.5 [MODIFIED, MOVED FROM 13.1.1, LEVEL L1 > L2] Verify that different parsers used in the application for the same data type (e.g. JSON parsers, XML parsers, URL parsers), perform parsing in a consistent way and use the same character encoding mechanism to avoid issues such as JSON Interoperability vulnerabilities or different URI or file parsing behavior being exploited in Remote File Inclusion (RFI) or Server-side Request Forgery (SSRF) attacks. 436
12.3.1 [MODIFIED, MERGED FROM 12.3.2, 12.3.3, 5.3.9] Verify that file operations avoid using user-submitted filenames or file metadata when creating file paths to protect against path traversal, local or remote file inclusion (LFI, RFI), and server-side request forgery (SSRF) attacks. Instead, use internal, trusted data for file I/O. If user-submitted filenames or file metadata must be used, strict validation and sanitization must be applied. 73

I think that requirements 5.5.5 and 12.3.1 are referring to specific protection mechanisms which are required which have the potential side effect of SSRF as such, I think they are ok as they are.

I think that 5.2.6 needs to be very slightly clarified to focus on the risk of putting input directly into a URL being accessed.

I would say:

# Description L1 L2 L3 CWE
5.2.6 [MODIFIED] Verify that the application protects against SSRF attacks, by checking untrusted data against an allowlist of protocols, domains, paths and ports and sanitizing potentially dangerous characters before using the data to call another service. 918

Thoughts @jmanico @elarlang

elarlang commented 2 hours ago

5.2.6 "URL operations" vs 12.3.1 "file operations" (if file operation does not have required protection it may lead to SSRF that is equal to not protected URL operation)

5.5.5 is about parsers and is a bit different topic.

For me, everything works as those are (it does not mean those can not be improved)

tghosth commented 2 hours ago

Do you agree with my 5.2.6 change?

elarlang commented 2 hours ago

What is the meaning of "checking" here? What is the action and what is the outcome? I think the previous "validation or sanitizing" was more precise.

The rest of the requirement is probably just moving things around.

tghosth commented 1 hour ago

So maybe matching rather than checking, I don't like validating as it sounds like input validation and I don't like sanitization as I don't think it is exactly what is being done

elarlang commented 1 hour ago

Let's go other way - what is the problem with current 5.2.6 you try to solve?

tghosth commented 37 minutes ago

Two problems with 5.2.6:

jmanico commented 26 minutes ago

The term I typically use is “allowlist validation”. It’s definitely a form of validation, IMO.

tghosth commented 24 minutes ago

So does it need to move into the input validation section?