Deduplicate SSRF requirements

[tghosth]

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]

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]

Do you agree with my 5.2.6 change?

[elarlang]

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]

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]

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

[tghosth]

Two problems with 5.2.6:

• I read it but I found talking about "HTTP file metadata, such as filenames and URL input fields," confusing and unnecessary.
• I don't want this to be confused with input validation

[jmanico]

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

[tghosth]

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

[jmanico]

Possibly, but if I was trying to fix SSRF I wish I had one section that referenced all SSRF controls. I think in general some of the categorization we are doing makes consuming ASVS tough. I would suggest that if we put this into the input validation section, we still point to it from a dedicated SSRF section - and do this cross referencing for all of ASVS so its consumable per topic without jumping around. This is a big ask, I know.

[tghosth]

Ultimately this is doing both validation and sanitization so we can probably leave it in the section where it is and I will PR using "validation" instead of "checking".

@set-reminder 2 days @tghosth to open a PR

[octo-reminder]

⏰ Reminder
Saturday, September 28, 2024 12:00 AM (GMT+02:00)

@tghosth to open a PR

[octo-reminder]

🔔 @tghosth

@tghosth to open a PR