Skip to content

Conversation

@panva
Copy link
Member

@panva panva commented Jan 16, 2024

A followup review with suggestions as requested at IETF 118

@panva panva marked this pull request as ready for review February 9, 2024 10:16
@panva panva requested a review from aaronpk as a code owner February 9, 2024 10:16
Copy link
Member

@aaronpk aaronpk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see suggestions inline

@panva panva requested a review from aaronpk February 13, 2024 07:29
@aaronpk aaronpk merged commit 6f12ac0 into oauth-wg:main Feb 13, 2024
@panva panva deleted the filip-review branch February 13, 2024 18:28
Copy link
Contributor

@philippederyck philippederyck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @panva, thanks for the detailed review!

@aaronpk I am aware that this is merged already, but I added a bunch of comments that I believe need addressing. Let me know if you need any more input from me.

### Acquisition and Extraction of New Tokens {#payload-new-flow}

In this advanced attack scenario, the attacker completely disregards any tokens that the application has already obtained. Instead, the attacker takes advantage of the ability to run malicious code in the application's origin. With that ability, the attacker can inject a hidden iframe and launch a silent Authorization Code flow. This silent flow will reuse the user's existing session with the authorization server and result in the issuing of a new, independent set of tokens. This scenario consists of the following steps:
In this advanced attack scenario, the attacker completely disregards any tokens that the application has already obtained. Instead, the attacker takes advantage of the ability to run malicious code in the application's execution context. With that ability, the attacker can inject a hidden iframe and launch a silent Authorization Code flow. This silent flow will reuse the user's existing session with the authorization server and result in the issuing of a new, independent set of tokens. This scenario consists of the following steps:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe that the consistent rewording to execution context is accurate in this context. It is true that the code runs in the execution context, but the underlying security model of the browser is based on origins (Same-Origin Policy). When two execution contexts have the same origin and have a direct reference to each other, they are not isolated (e.g., a parent window and a frame).

I strongly recommend to stick with origin. Alternatively, we should add a note to explain this context.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explaining either one of the two that you think is more appropriate is definitely needed. I don't think origin is any better to execution context without an explanation of what we mean by it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworded to clarify what it means.

however, that the browser will attempt to GET or POST to the API endpoint before
knowing any CORS policy; it simply hides the succeeding or failing result from
for these endpoints should be a wildcard origin or more restrictive. Note,
however, that the browser will typically do a "preflight" request to check to see
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct. The browser only uses preflights for CORS-safelisted requests (i.e., anything that cannot be sent without modifying a request through JS). A simple GET or POST without custom headers or without a custom content type will not require a preflight.

Copy link
Member Author

@panva panva Feb 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A simple request won't get a preflight, right, feel free to reword the conditions which are currently summarized as "typically".

It needs to be mentioned (at least here) that when one uses DPoP there are no more simple requests. Having had written not one browser based client I am no longer envisioning typical operation to be without preflights.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your point of view, but vanilla OAuth requests with form encoding would be simple requests. Reworded the text to add the necessary context.

during preflight when it has no access to the actual request's body or headers and then when
the actual request is made decide to restrict the response after it was able to check the request
was made from an expected origin based on the data in the request, e.g. based on pre-registered
origins of the Browser-based OAuth 2.0 client.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am assuming you are hinting at a dynamic CORS configuration where the AS uses the client ID to lookup a configuration and determine if this origin is legitimate? If that is indeed true, I don't think enforcement through CORS would be a correct implementation technique. I consider it application-level logic, akin to checking a redirect URI during an OAuth flow. As such, I would handle this as an application-level error instead of an invalid CORS request.

I am not objecting against this, but I believe it needs a bit more explaining when it is left in. This will not be obvious to someone without proper context.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not enforced through CORS, the request would still fail with a proper oauth error, but it is through CORS that we ensure even these error responses don't get read from unintended origins.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the scenario would be as follows:

The AS has client 1 with origin A and client B with origin B. Upon handling a preflight, it sees a request coming from origin B, so it allows the request to go through. Upon handling the request, the AS sees that client 1 does not have origin B approved, so it issues an error message. However, additionally, it should not set the CORS header that allows origin B to read this response.

Is that a correct description? If so, I think we need to add this context in the document to make this point clear. We probably also need to make this a SHOULD requirement for the AS (as I don't think we can make it a MUST)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's currently a Note and I think that's enough to give guidance. I wouldn't make this normative in any way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I have rephrased this to include a bit more details on an example.

philippederyck added a commit to philippederyck/OAUTHWG-oauth-browser-based-apps that referenced this pull request Feb 25, 2024
Reworded the text based on the discussion in [PR oauth-wg#30](oauth-wg#30)
philippederyck added a commit to philippederyck/OAUTHWG-oauth-browser-based-apps that referenced this pull request Feb 25, 2024
Reworded the text based on the discussion in [PR oauth-wg#30](oauth-wg#30)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants