Skip to content
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

new requirement - do not follow redirects by default for requests made by server-side components #2491

Closed
elarlang opened this issue Jan 2, 2025 · 8 comments · Fixed by #2539
Labels
6) PR awaiting review V10 V14 _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@elarlang
Copy link
Collaborator

elarlang commented Jan 2, 2025

I think we don't have addressed the problem, if server-side client makes request to external party and the external party redirects the request to "malicious URL".

Related requirement:

14.6.1 Verify that the web or application server is configured with an allowlist of resources or systems to which the server can send requests or load data or files from.

My proposal is to have requirement with the idea: server-side components that making requests must not follow redirects till is not intended for that use-case.

@elarlang elarlang added 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet _5.0 - prep This needs to be addressed to prepare 5.0 labels Jan 2, 2025
@tghosth tghosth added the V14 label Jan 2, 2025
@tghosth
Copy link
Collaborator

tghosth commented Jan 2, 2025

So I think 14.6.1 is more general and may not be logically aware of whether to follow redirects or not.

I think maybe we we want to work on the other requirement for SSRF:

# Description L1 L2 L3 CWE
5.2.6 [MODIFIED] Verify that the application protects against Server-side Request Forgery (SSRF) attacks, by validating 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

I would suggest a modification:

"Verify that the application protects against Server-side Request Forgery (SSRF) attacks, by validating untrusted data against an allowlist of protocols, domains, paths and ports and sanitizing potentially dangerous characters before using the data to call another service. The application should also be configured to not follow redirects when calling the other service unless this is specifically intended functionality."

@tghosth tghosth added the V5 Temporary label for grouping input validation, sanitization, encoding, escaping related requirements label Jan 2, 2025
@elarlang
Copy link
Collaborator Author

elarlang commented Jan 2, 2025

Although the note is valid here, requirement 5.2.6 is addressing user-input driven request forgery in the application but "do not follow redirects" is wider than user-input controlled calls.

Also, the defense against "do not follow redirect" is about configuration or defensive coding, but not input validation or sanitization. It is not a V5 topic.

@elarlang elarlang added the next meeting Filter for leaders label Jan 3, 2025
@tghosth
Copy link
Collaborator

tghosth commented Jan 16, 2025

Verify that where the application back-end makes calls to external URLs, it is configured to not follow redirects unless this is specifically intended functionality. This is relevant for any external URL and is not limited to a situation where the URL has been provided by an untrusted user.

This is the proposed wording, to be added to 14.6

Alternative option to edit existing 14.6.1:

[MODIFIED, MOVED FROM 12.6.1] Verify that the web or application server is configured with an allowlist of resources or systems to which the server can send requests or load data or files from, and that it is configured to not follow redirects unless this is specifically intended functionality.

@tghosth tghosth added 4) proposal for review Issue contains clear proposal for add/change something and removed next meeting Filter for leaders V5 Temporary label for grouping input validation, sanitization, encoding, escaping related requirements 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet labels Jan 16, 2025
@jmanico
Copy link
Member

jmanico commented Jan 16, 2025

I think @elarlang is spot on here. An allow list is validation, but if one of the items on your allow list redirects, that's only a problem if your server-side HTTP "fetch" class takes redirects, and to avoid this is about configuring something - and not validation.

So perhaps split the allow-list from the redirect? Especially when it comes to coding these are solved at different layers.

@elarlang
Copy link
Collaborator Author

Thinking more about it, I think we should go with a separate requirement and probably the best option is to use section "V10.4 Defensive Coding" for that as proposed by Josh during discussion.

14.6.1 can be done in some waf, proxy, os, application or whatever level, but the "no not follow redirects" is the question of the library that makes the request - those are different enough to test and to implement - so let's keep them separately.

@elarlang elarlang added the V10 label Jan 16, 2025
@tghosth
Copy link
Collaborator

tghosth commented Jan 19, 2025

Verify that where the application back-end makes calls to external URLs, it is configured to not follow redirects unless this is specifically intended functionality. This is relevant for any external URL and is not limited to a situation where the URL has been provided by an untrusted user.

So @elarlang, you are proposing this wording and to add on to 10.4?

@elarlang
Copy link
Collaborator Author

Yes, although I'm not sure we need this explanation part. But if someone thinks that it is good then it can stay like it is.

@elarlang elarlang added 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR and removed 4) proposal for review Issue contains clear proposal for add/change something labels Jan 19, 2025
@jmanico
Copy link
Member

jmanico commented Jan 19, 2025 via email

@elarlang elarlang linked a pull request Jan 20, 2025 that will close this issue
@elarlang elarlang added 6) PR awaiting review and removed 5) awaiting PR A proposal hs been accepted and reviewed and we are now waiting for a PR labels Jan 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6) PR awaiting review V10 V14 _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants