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

Clarify reasoning behind 13.2.5 #2472

Open
tghosth opened this issue Dec 16, 2024 · 13 comments
Open

Clarify reasoning behind 13.2.5 #2472

tghosth opened this issue Dec 16, 2024 · 13 comments
Assignees
Labels
4a) Waiting for another This issue is waiting for another issue to be resolved V13 _5.0 - prep This needs to be addressed to prepare 5.0

Comments

@tghosth
Copy link
Collaborator

tghosth commented Dec 16, 2024

This this is the problem to fix not to say 13.2.5 is not valid defense against pre-preflight. 13.2.5 is important requirement for CORS, if there is another meaning in another context, I prefer to make a separate requirement for the other context with a clear definition, what other risk it mitigates.

#876 (comment)

Well I would actually argue that 13.2.5 is defence in depth for CSRF because the correct fix is what is in 50.4.1. It is irrelevant for CORS because again even if you manage to send the request, you cannot read the response.

#876 (comment)

We should clarify the reason for 13.2.5

# Description L1 L2 L3 CWE
13.2.5 Verify that REST services explicitly check the incoming Content-Type to be the expected one, such as application/xml or application/json. 436
@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 V13 labels Dec 16, 2024
@elarlang
Copy link
Collaborator

Please also provide an explanation, what security risk the requirement solves outside of CORS context.

@tghosth
Copy link
Collaborator Author

tghosth commented Dec 16, 2024

Ok, the CWE is 436 "Interpretation Conflict". Based on the CWE it seems like the original intention of the requirement was to ensure that content which is received is processed by the correct interpreter and potentially also an expected interpreter.

Tracing back, it comes from ASVS 3.0, see this training slide:
image

There are various frameworks which will seamlessly handle a variety of incoming content types. If assumptions have been made that incoming content will be of a certain type, sending content of a different type may bypass controls which rely on those assumptions.

I think if nothing else the age of the requirement probably indicates that it was not intended to be related to CORS :)

@elarlang
Copy link
Collaborator

If it is such a generous problem, why is the requirement limited to REST services and precisely mentioning 2 certain content-types?

@elarlang
Copy link
Collaborator

elarlang commented Dec 16, 2024

If assumptions have been made that incoming content will be of a certain type, sending content of a different type may bypass controls which rely on those assumptions.

How does it make sense or how is the assumption a security requirement?

If you make a request and change the content-type to mismatch with the content, it will reach the functionality anyway.

Basically, you are saying, that for file upload we need to validate the file mime-type/content-type, and then it's ok to process it further? Take a look at requirement 12.2.1 to validate your idea for this interpretation.

@tghosth
Copy link
Collaborator Author

tghosth commented Dec 16, 2024

why is the requirement limited to REST services

Fair question and should potentially be wider

precisely mentioning 2 certain content-types

These are examples, not a closed list

If you make a request and change the content-type to mismatch with the content, it will reach the functionality anyway

No, the point is that if it expects to receive json and it receives xml or formurlencoded then it will reject the request. If it expects to receive multiple types then it needs to be capable of validating the content.

Basically, you are saying, that for file upload we need to validate the file mime-type/content-type, and then it's ok to process it further?

This is not about file upload, this is about a regular web request where the web server is processing the content

@elarlang
Copy link
Collaborator

No, the point is that if it expects to receive json and it receives xml or formurlencoded then it will reject the request. If it expects to receive multiple types then it needs to be capable of validating the content.

It's called input validation. Checking the content-type header does not serve the goal. I think you did not take parallel with file-upload seriously.

I reached my facepalm limit for this year.

@elarlang elarlang added the next meeting Filter for leaders label Dec 16, 2024
@tghosth
Copy link
Collaborator Author

tghosth commented Dec 18, 2024

I was thinking about this a little more @elarlang.

Maybe if we refine it to:

"Verify that REST services explicitly process received data based on the incoming Content-Type and will only accept expected content types. For example, a JSON API should not accept a request with an application/xml content type nor should it process a request with a text/plain content type as JSON."

@elarlang
Copy link
Collaborator

What problem it solves outside of CORS?

If the service expects JSON content and gets a valid JSON content but with the Content-Type: text/plain - what security risk it solves to not accept that?

If the service is accepting different contents and Content-Types, it just need to validate, that the sent "message body" is matching with defined Content-Type. As previously pointed parallel with file upload - the content must match with defined type. For me this is just basic input validation question.

@tghosth
Copy link
Collaborator Author

tghosth commented Dec 18, 2024

Ok so you think this should be merged into input validation or just removed as redundant?

@elarlang
Copy link
Collaborator

From CORS point of view it gets merged into "CSRF" requirement.

For standard input validation I don't see specific problem or vector to solve that is not covered by general input validation requirements.

@tghosth
Copy link
Collaborator Author

tghosth commented Dec 18, 2024

Opened #2483

@tghosth tghosth added 6) PR awaiting review and removed 1) Discussion ongoing Issue is opened and assigned but no clear proposal yet next meeting Filter for leaders labels Dec 18, 2024
@elarlang
Copy link
Collaborator

It gets merged into V50.4.1, will be solved with #2481

@elarlang elarlang added 4a) Waiting for another This issue is waiting for another issue to be resolved and removed 6) PR awaiting review labels Dec 18, 2024
@elarlang elarlang self-assigned this Dec 18, 2024
@jmanico
Copy link
Member

jmanico commented Dec 18, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4a) Waiting for another This issue is waiting for another issue to be resolved V13 _5.0 - prep This needs to be addressed to prepare 5.0
Projects
None yet
Development

No branches or pull requests

3 participants