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

Remaining reqs in section 5.1 seem like they don't belong. #2487

Open
tghosth opened this issue Dec 26, 2024 · 10 comments
Open

Remaining reqs in section 5.1 seem like they don't belong. #2487

tghosth opened this issue Dec 26, 2024 · 10 comments
Assignees
Labels
3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos next meeting Filter for leaders V5 Temporary label for grouping input validation, sanitization, encoding, escaping related requirements _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@tghosth
Copy link
Collaborator

tghosth commented Dec 26, 2024

These requirements are not really input validation but it is not clear where they should go.

# Description Level Suggested action
5.1.5 [MODIFIED, SPLIT TO 50.8.1] Verify that the application will only automatically redirect the user to a different URL directly from an application URL where the destination appears on an allowlist. L1 or L1>L2 Leave here but open a separate issue to discuss location
5.1.6 [ADDED] Verify that the application validates that user-controlled input in HTTP request header fields does not exceed the server's maximum header field size limit (usually 4kB or 8kB) to prevent client-based denial of service attacks. L2 Leave here but open a separate issue to discuss location

Previous thoughts:

@elarlang:

V5.1.5 is not just a technical input validation - it relies on logic and a defined list to define, what is a trusted and allowed destination.

V5.1.6 it is a technical check, but during a building the header. It may involve more than one input or other logic - just that the combined length that is used in one header field value can not exceed defined limit. It is a question of program code logic and fits to defensive coding.

(apologies in advance if I missed anything)

@tghosth:

5.1.5 sounds like classic "accept or reject it" validation so I think this should stay put.

5.1.6 is not language specific but rather a conceptual question of how you incorporate untrusted content into a header. I still think it is more about input than logic but I am not strongly opinionated.

@tghosth tghosth 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 V5 Temporary label for grouping input validation, sanitization, encoding, escaping related requirements labels Dec 26, 2024
@jmanico
Copy link
Member

jmanico commented Jan 1, 2025

5.1.5 looks like validation to me. It's typically called "allow list validation" and this kind of validation does in fact almost always provide a needed security benefit when protecting redirects.

5.1.6 is just doing a size limit check, a form of validation. Depending on the underlying framework, a programmer doing this manually may or may not provide a major security benefit.

But these both seem like security validation to me.

@elarlang
Copy link
Collaborator

elarlang commented Jan 3, 2025

5.1.6 can be also widened to building a request URL (to an API) - it may happen in the background without processing directly the request initiated by the user - so it is not direct user input validation. Probably we need to change the 5.1.6.

@jmanico
Copy link
Member

jmanico commented Jan 3, 2025

Especially since 5.1.6 is something a developer rarely needs to deal with, I agree on moving it out of validation, Elar.

@tghosth
Copy link
Collaborator Author

tghosth commented Jan 5, 2025

For 5.1.5, I think the closest sections are either "V5.2 Sanitization and Sandboxing" because it fits into the theme of only using untrusted content after we are sure that it is safe.

For 5.1.6, I would welcome a suggestion from @elarlang on how to alter the requirement and where it should go.

@tghosth tghosth added 3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos next meeting Filter for leaders and removed 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet labels Jan 5, 2025
@elarlang elarlang self-assigned this Jan 16, 2025
@elarlang
Copy link
Collaborator

Proposal for 5.1.6 - update the text with the meaning (needs grammas/language/wording review/update) below and move it to V10.4 Defensive coding.

Verify that if the application builds an HTTP request it has validation, sanitization, or other situation handling in place not to create longer URI (e. g. API calls) or HTTP request header fields (e. g. Authorization, Cookie) than all components for the communication can accept. It includes values provided for the browser to avoid client-based denial of service attacks.

@tghosth
Copy link
Collaborator Author

tghosth commented Jan 22, 2025

What do you think about:

"Verify that the application uses validation, sanitization, or other mechanisms to avoid creating URIs (such as for API calls) or HTTP request header fields (such as Authorization or Cookie), as part of an HTTP request, which are too long to be accepted by the receiving components. The application is otherwise exposed to client-based denial of service attacks."

I didn't understand what you meant by "It includes values provided for the browser"

@elarlang
Copy link
Collaborator

First part - I have an error reading the as part of an HTTP request in this part of the sentence. Maybe it is just my problem :) Can it be in the beginning or at the end? Otherwise the first impression when reading it is "avoid creating URIs or HTTP request header fields as part of an HTTP request".


Second part

Initial:

It includes values provided for the browser to avoid client-based denial of service attacks.

Updated:

The application is otherwise exposed to client-based denial of service attacks.

Updated version has different meaning - initial has the goal to highlight one possible impact, the updated version declares or defines this as one and only possible impact

@tghosth
Copy link
Collaborator Author

tghosth commented Jan 26, 2025

"Verify that, within HTTP requests, the application uses validation, sanitization, or other mechanisms to avoid creating URIs (such as for API calls) or HTTP request header fields (such as Authorization or Cookie), which are too long to be accepted by the receiving component. This will help prevent attacks such as client-based denial of service."

@elarlang better?

@elarlang
Copy link
Collaborator

First part is better.

I'm concerned that the last part can be still interpreted as the requirements is valid and addressed only for the "client-based denial of service". If we can not send it as a clear message, as this is just one option or outcome, is better to not send it at all.

@tghosth
Copy link
Collaborator Author

tghosth commented Jan 26, 2025

"Such as" clearly sets this out as an example.

We need to explain the reason for this so if you don't want my proposal then maybe try and reword what you wrote before: "It includes values provided for the browser to avoid client-based denial of service attacks"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3) awaiting proposal There is some discussion in issue and reach to some results but it's not concluded with clear propos next meeting Filter for leaders V5 Temporary label for grouping input validation, sanitization, encoding, escaping related requirements _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

No branches or pull requests

3 participants