Re: [OAUTH-WG] Review of OAuth 2.0 for Browser-Based Apps

[Aaron Parecki <[e-mail]>]

Thank you for the thorough review! My responses and feedback is below.
I've gone ahead and published a new draft based on this feedback so
that the edits can be read in context. It is now available here:

https://tools.ietf.org/html/draft-ietf-oauth-browser-based-apps-05

I've made all "EDITORIAL" changes as suggested with the exception of the below:

> Authorization Server Mix-Up Mitigation – Please also reference draft-ietf-oauth-mix-up-mitigation here.

It appears that draft expired in January 2017, so it feels strange to
reference an outdated draft. If that document is picked up again and
shows signs that it is on track to publication I'd be happy to
reference it.

I'm going to avoid getting into the "ADDITIONAL OVERALL ISSUES" in
this thread to keep it manageable. I will follow up with those
specific points in a new thread later.

As for the "SUBSTANTIVE" list, I've made most of the changes as
suggested, with a few exceptions. These are noted below.

> 1. Introduction – The first sentence says that this spec is about applications running entirely in a browser.  Many browser-based applications also use helper services.  Please broaden the scope to encompass both patterns.

Changed "running entirely in a browser" to "executing in a browser".

> 2.  Overview – Change “This was the principal motivation” to “This was one of the motivations”.

Done

> 3.  Terminology – In the “OAuth” definition, also reference RFC 6750.

Done

> 4.  Overview – Change “the current best practice for browser-based applications is to use OAuth 2.0 authorization code flow with PKCE” to “the current best practice for browser-based applications is to use either OAuth 2.0 authorization code flow with PKCE or OpenID Connect <xref target="OpenID.Core"/>”.  This parallels the same recommendation in the forthcoming OAuth Security BCP specification.

This feels too broad. It ends up sounding like it's okay to use the
implicit flow to get an access token if you're also "using OpenID
Connect". I would rather reference a specific grant type / response
mode in OpenID Connect similar to how this sentence references
specifically authorization code with PKCE. Any suggestions?

> 4.  Overview – Change “Protect themselves against CSRF attacks by using the OAuth 2.0 state parameter to carry one-time use CSRF tokens, or by ensuring the authorization server supports PKCE” to “Protect themselves against CSRF attacks by using the OAuth 2.0 state parameter or the OpenID Connect nonce parameter to carry one-time use CSRF tokens or by ensuring the authorization server supports PKCE”.

Done

> 4.  Overview – The intended meaning of this text isn’t clear: “Register one or more redirect URIs, and not vary the redirect URI per authorization request”.  Do you mean to include a registered redirect_uri value in each authorization request?  This needs to be clarified.

The intent is to prevent dynamic redirect URIs such as varying the
query string component. I will update this to:

"Register one or more redirect URIs, and use only exact registered
redirect URIs in authorization requests"

> 4.  Overview – Change “Support the PKCE extension” to “Support the PKCE extension or OpenID Connect”.

The Security BCP already requires that authorization servers MUST
support PKCE. I don't think it makes sense for this BCP to allow
supporting OpenID Connect *instead of* PKCE, which is what the text
above would do.

> 5.  First-Party Applications – The third paragraph says that “first-party applications using OAuth or OpenID Connect MUST use the authorization code flow”.  OpenID Connect can be safely used with other response_type values than simply “code”.  For instance, the “code id_token” value is just as safe.  Please generalize this language when describing the use of OpenID Connect to recognize that other response_type value may continue to be safely used.

The intent here is to require a redirect-based flow rather than for
example the password grant. I will update this to:

"To conform to this best practice, first-party applications using
OAuth or OpenID Connect MUST use a redirect-based flow (such as the
OAuth Authorization Code flow) as described later in this document."

> 6.2.  JavaScript Applications with a Backend – The sentence “The Application Server utilizes own session with the browser to store the access token” raises a question with no obvious answer:  Where is the specification recommending or suggesting that the access token be stored in this case?  Please clarify.

I've updated this text to:

"The Application Server stores this access token itself and
establishes its own cookie-based session with the Browser application.
The Application Server can store the access token either server-side,
or in the cookie itself."

> 6.2.  JavaScript Applications with a Backend – The last paragraph talks about exposing the access token to the end user but fails to describe the perceived risk or threat.  If there are such risks or threats, please be explicit about what they are and their mitigations.

I thought I remembered seeing an attack described where the user can
extract an access token from an application and use it for things the
developers didn't intend. I can't find a reference to that right now
though. Anyone have an idea of where that might be? If we can't come
up with something to point to about that, I'm inclined to drop this
sentence.

> 6.3.  JavaScript Applications without a Backend – This sentence tells a false and incomplete story “The application then runs in the browser, and is considered a public client since it has no ability to be issued a client secret.”  Public clients can definitely be issued Client ID/Client Secret pairs using OAuth Dynamic Client Registration [RFC 7591].  Please revise this description accordingly.  (Likewise, the following paragraph says that “The JavaScript app is then responsible for storing the access token securely using appropriate browser APIs.  If this is safe, then storing the dynamically issued Client ID/Client Secret pair should be safe to do so in the same manner.)

This phrasing could use some work, but the point still stands. An
application deployed this way has no mechanism to securely
authenticate with the authorization server, which is the RFC6749
definition of a confidential client. Dynamic client registration
doesn't solve this, because the registration endpoint would have to be
publicly available and then anyone could register a client so there's
no way to tell whether this particular javascript application is the
"real" one. I don't see any way that an application in this
architecture could be treated as a confidential client. I will
rephrase this to the below:

"In this architecture, the JavaScript code is first loaded from a
static web host into the browser (A), and the application then runs in
the browser. This application is considered a public client, since
there is no way to issue it a client secret and there is no other
secure client authentication mechanism available in the browser."

> 7.1.  Initiating the Authorization Request from a Browser-Based Application – As in other places where the text says that PKCE must be used, please revise this instance to say that either PKCE or OpenID Connect must be used – just as is done in the forthcoming Security BCP.

Again, saying "use OpenID Connect" is not specific enough. One of the
things this BCP is trying to capture is the advice of only issuing
access tokens at the token endpoint with an authorization code. Since
OpenID Connect defines a few additional ways of issuing access tokens,
it would need to get more specific about what part of OpenID Connect
is okay here.

Also, in the case that an application is not using OpenID Connect at
all, only using OAuth, it needs to be clear still that PKCE is
required, and that simply adding some unspecified part of OpenID
Connect doesn't magically solve the problems with the implicit flow.

Can you provide more specific guidance on when it's okay to not use
PKCE? For example using certain response type or response mode
combinations, and what parts of ID token validation must be followed
in order to safely ignore PKCE?

> 7.1.  Initiating the Authorization Request from a Browser-Based Application – As in other places where the spec says that the “state” parameter must be used, revise this instance to say that either the OAuth “state” parameter or the OpenID Connect “nonce” parameter must be used to prevent the applicable attacks.

I've changed this paragraph to the below to better match the Security
BCP, and to match what was discussed during IETF 106:

"Browser-based apps MUST prevent CSRF attacks against their redirect
URI. This can be
accomplished by any of the below:

* using PKCE, and confirming that the authorization server supports PKCE
* if the application is using OpenID Connect, by using the OpenID
Connect "nonce" parameter
* using a unique value for the OAuth 2.0 "state" parameter"

This section then references the Security BCP.

> 9.2.  Client Authentication – The second paragraph talks about secrets that are statically included and warns against them, which is good.  But please also add text describing how OAuth Dynamic Client Registration [RFC 7591] can be used to safely statically create these secrets.

Do we know of anyone doing this in the wild with SPAs right now? This
feels like it has limited use at best, since dynamic registration
doesn't actually authenticate the client for public clients. You could
use the client secret instead of PKCE to prevent stolen authorization
codes, but since PKCE also solves that problem, it doesn't seem very
useful to recommend this.

> 9.3.  Client Impersonation – Add text to the first paragraph describing how the use of the OpenID Connect “id_token_hint” parameter can be used to satisfy the requirement that the identity of the client can be assured.

I am not familiar with how the id_token_hint provides assurance of the
client identity. If the id_token was obtained via any front-channel
mechanism, then it is at risk of being stolen, and then it can't be
considered client authentication. Additionally, merely using the
id_token in a GET request means it's susceptible to all the same
problems we are avoiding with exposing access tokens in URLs, so again
it doesn't seem useful as client authentication.

> 9.3.  Client Impersonation – The wording of this phrase is ambiguous:  “If authorization servers restrict redirect URIs to a fixed set of absolute HTTPS URIs without wildcard domains, paths, or query string components…”  It could be interpreted with the “without” applying to “paths or query string components”, meaning that no path or query string components may be present at all.  (I think you meant only that wildcard values not be present, but that’s not the meaning of the phrase, as written.)

Correct, this was supposed to say that wildcard values for any of
those three components are not allowed. I will rephrase it to:

"...a fixed set of absolute HTTPS URIs, preventing the use of wildcard
domains, wildcard paths, or wildcard query string components..."

> 9.4.  Cross-Site Request Forgery Protections – As in other places where the spec says that the “state” parameter must be used, revise this instance to say that either the OAuth “state” parameter or the OpenID Connect “nonce” parameter must be used to prevent the applicable attacks.

I've replaced this section with the below:

"Clients MUST prevent Cross-Site Request Forgery (CSRF) attacks
against their redirect URI.
Clients can accomplish this by either ensuring the authorization server supports
PKCE and relying on the CSRF protection that PKCE provides, or using the "state"
parameter to carry one-time-use CSRF tokens as described in
{{auth_code_request}},
or if the client is also an OpenID Connect client, using the OpenID
Connect "nonce" parameter.

See Section 2.1 of [oauth-security-topics] for additional details."

> 9.8.  OAuth Implicit Grant Authorization Flow – The paragraph ends with “not all of which have sufficient mitigation strategies”.  It would be useful to be more specific here, saying which attacks do not have sufficient mitigation strategies.

The following sections with headers beginning with "Threat" describe
the attacks, I've updated the text to hopefully clarify that. I've
also added a new header structure here which should help make that
relationship between the paragraphs more apparent.

> 9.8.5.  Countermeasures – The phrase “using the authorization code with PKCE avoids these attacks” doesn’t say what attacks are being referred to.  Please revise so that the attacks in question are unambiguously referenced.

I've updated this section along with the change above.

> Appendix A.  Server Support Checklist – In bullet 3, revise to say that either PKCE or OpenID Connect is required, as per other locations in the specification.

This does not match what the Security BCP says:

"Authorization servers MUST support PKCE"

----
Aaron Parecki
aaronparecki.com
@aaronpk





On Fri, Feb 21, 2020 at 4:48 PM Mike Jones <[e-mail]> wrote:
>
> I did a cover-to-cover read of draft-ietf-oauth-browser-based-apps-04.  My review comments follow in three sections:  substantive comments on the text, substantive overall issues to consider (provided by Microsoft product engineers), and editorial issues.
>
>
>
> SUBSTANTIVE
>
>
>
> 1. Introduction – The first sentence says that this spec is about applications running entirely in a browser.  Many browser-based applications also use helper services.  Please broaden the scope to encompass both patterns.
>
>
>
> 2.  Overview – Change “This was the principal motivation” to “This was one of the motivations”.
>
>
>
> 3.  Terminology – In the “OAuth” definition, also reference RFC 6750.
>
>
>
> 4.  Overview – Change “the current best practice for browser-based applications is to use OAuth 2.0 authorization code flow with PKCE” to “the current best practice for browser-based applications is to use either OAuth 2.0 authorization code flow with PKCE or OpenID Connect <xref target="OpenID.Core"/>”.  This parallels the same recommendation in the forthcoming OAuth Security BCP specification.
>
>
>
> 4.  Overview – Change “Use the OAuth 2.0 authorization code flow with the PKCE extension” to  “Use either the OAuth 2.0 authorization code flow with the PKCE extension or OpenID Connect”.
>
>
>
> 4.  Overview – Change “Protect themselves against CSRF attacks by using the OAuth 2.0 state parameter to carry one-time use CSRF tokens, or by ensuring the authorization server supports PKCE” to “Protect themselves against CSRF attacks by using the OAuth 2.0 state parameter or the OpenID Connect nonce parameter to carry one-time use CSRF tokens or by ensuring the authorization server supports PKCE”.
>
>
>
> 4.  Overview – The intended meaning of this text isn’t clear: “Register one or more redirect URIs, and not vary the redirect URI per authorization request”.  Do you mean to include a registered redirect_uri value in each authorization request?  This needs to be clarified.
>
>
>
> 4.  Overview – Change “Support the PKCE extension” to “Support the PKCE extension or OpenID Connect”.
>
>
>
> 5.  First-Party Applications – The third paragraph says that “first-party applications using OAuth or OpenID Connect MUST use the authorization code flow”.  OpenID Connect can be safely used with other response_type values than simply “code”.  For instance, the “code id_token” value is just as safe.  Please generalize this language when describing the use of OpenID Connect to recognize that other response_type value may continue to be safely used.
>
>
>
> 6.2.  JavaScript Applications with a Backend – The sentence “The Application Server utilizes own session with the browser to store the access token” raises a question with no obvious answer:  Where is the specification recommending or suggesting that the access token be stored in this case?  Please clarify.
>
>
>
> 6.2.  JavaScript Applications with a Backend – The last paragraph talks about exposing the access token to the end user but fails to describe the perceived risk or threat.  If there are such risks or threats, please be explicit about what they are and their mitigations.
>
>
>
> 6.3.  JavaScript Applications without a Backend – This sentence tells a false and incomplete story “The application then runs in the browser, and is considered a public client since it has no ability to be issued a client secret.”  Public clients can definitely be issued Client ID/Client Secret pairs using OAuth Dynamic Client Registration [RFC 7591].  Please revise this description accordingly.  (Likewise, the following paragraph says that “The JavaScript app is then responsible for storing the access token securely using appropriate browser APIs.  If this is safe, then storing the dynamically issued Client ID/Client Secret pair should be safe to do so in the same manner.)
>
>
>
> 7.1.  Initiating the Authorization Request from a Browser-Based Application – As in other places where the text says that PKCE must be used, please revise this instance to say that either PKCE or OpenID Connect must be used – just as is done in the forthcoming Security BCP.
>
>
>
> 7.1.  Initiating the Authorization Request from a Browser-Based Application – As in other places where the spec says that the “state” parameter must be used, revise this instance to say that either the OAuth “state” parameter or the OpenID Connect “nonce” parameter must be used to prevent the applicable attacks.
>
>
>
> 9.2.  Client Authentication – The second paragraph talks about secrets that are statically included and warns against them, which is good.  But please also add text describing how OAuth Dynamic Client Registration [RFC 7591] can be used to safely statically create these secrets.
>
>
>
> 9.3.  Client Impersonation – Add text to the first paragraph describing how the use of the OpenID Connect “id_token_hint” parameter can be used to satisfy the requirement that the identity of the client can be assured.
>
>
>
> 9.3.  Client Impersonation – The wording of this phrase is ambiguous:  “If authorization servers restrict redirect URIs to a fixed set of absolute HTTPS URIs without wildcard domains, paths, or query string components…”  It could be interpreted with the “without” applying to “paths or query string components”, meaning that no path or query string components may be present at all.  (I think you meant only that wildcard values not be present, but that’s not the meaning of the phrase, as written.)
>
>
>
> 9.4.  Cross-Site Request Forgery Protections – As in other places where the spec says that the “state” parameter must be used, revise this instance to say that either the OAuth “state” parameter or the OpenID Connect “nonce” parameter must be used to prevent the applicable attacks.
>
>
>
> 9.8.  OAuth Implicit Grant Authorization Flow – The paragraph ends with “not all of which have sufficient mitigation strategies”.  It would be useful to be more specific here, saying which attacks do not have sufficient mitigation strategies.
>
>
>
> 9.8.5.  Countermeasures – The phrase “using the authorization code with PKCE avoids these attacks” doesn’t say what attacks are being referred to.  Please revise so that the attacks in question are unambiguously referenced.
>
>
>
> Appendix A.  Server Support Checklist – In bullet 3, revise to say that either PKCE or OpenID Connect is required, as per other locations in the specification.
>
>
>
> ADDITIONAL OVERALL ISSUES FROM MICROSOFT ENGINEERS
>
>
>
> These issues should be considered in the context of this document.  For these, I’m requesting working group discussions – not specific resolutions at this time.
>
> There seems to be a lot of focus on exact redirect URI validation for addressing the biggest risk (open redirect), but while certainly absolutely critical, it frankly ignores the reality of how apps integrate with authorization servers these days a bit. The pattern we are seeing is that a lot of applications have simply shifted to including their own redirect URIs in 'state' parameters. Even our own services use this pattern as cookie space is quite limited. We are seeing a lot of security issues and there are no real solutions in any of the BCP documents or RFCs.
> In Section 4, the spec requires exact matching of redirect URIs. The OAuth Security BCP mentions that there are alternatives to exact redirect URI matching “As an alternative to exact redirect URI matching, the AS could also authenticate clients, e.g., using [I-D.ietf-oauth-jwsreq].” While I don’t think client authentication is suitable for browser based apps, accepting signed requests via OpenID Connect’s “request_uri” parameter would be a suitable alternative pattern to exact redirect URI checking. For consistency, I think both docs should have the same recommendation for redirect URI matching.
> Considering the above, we probably don't have many more good options for fragment flows. That's one of the reasons why I still think hybrid patterns have a place, but we need mechanisms to protect against code injection. Unfortunately, the security-topics document dismisses solutions that we could make work (code-bound state, nonce) in favor of solutions that don't help (PKCE). This might be outside of this immediate discussion about the implicit flow, but standards need to evolve to better mitigate that risk.
> The big problem with giving SPAs refresh tokens is that refresh tokens provide offline access – they live on after the user signs out – including when an attacker steals them. For JavaScript SPAs, where XSS is a big concern, and where access is only intended to be provided for an online user, these refresh tokens represent an unnecessary security liability. Disabling CORS on /token forces an implementation pattern without refresh tokens for web apps. So today, we have a tradeoff between using the implicit flow and accepting attacks around browser history and generically injected scripts with limited impact duration OR using the two legged flow and accept attacks with specifically injected scripts with unlimited duration, plus a little perf/cogs hit.
>
>
>
> EDITORIAL
>
>
>
> 1.  Introduction – Change “native apps as well as browser-based apps” to “native apps and browser-based apps”.
>
>
>
> 4.  Overview – Change “OAuth 2.0 RFC 6749” to “OAuth 2.0 <xref target="RFC6749"/> <xref target="RFC6750"/>”.
>
>
>
> 4.  Overview – Change “the flow assures that” to “the flow ensures that”.
>
>
>
> 5.  First-Party Applications – The first sentence of the second paragraph has no verb.
>
>
>
> 5.  First-Party Applications – The fifth paragraph starts with “By using the Authorization Code flow and redirecting the user to the authorization server”.  I believe that this is continuing the message in the (single-sentence) fourth paragraph.  The exposition will be less confusing if these paragraphs are merged.
>
>
>
> 6.  Application Architecture Patterns – The bulleted list is not in the same order as the corresponding subsections.  Please reorder them to make them parallel.
>
>
>
> 6.2.  JavaScript Applications with a Backend – This section references the OWASP Foundation as an organization.  It would be far more useful to instead list the documents containing the specific applicable recommendations.  Please revise accordingly.
>
>
>
> 9.5.  Authorization Server Mix-Up Mitigation – Please also reference draft-ietf-oauth-mix-up-mitigation here.
>
>
>
> 9.8.3.  Threat: Manipulation of Scripts – Please change “is far greater” to “may be amplified” to avoid an overreaching statement.  Likewise, delete the word “very” later in the paragraph.
>
>
>
> 9.9.  Additional Security Considerations – This section references the OWASP Foundation as an organization.  It would be far more useful to instead list the documents containing the specific applicable recommendations.  Please revise accordingly.
>
>
>
> 11.1.  Normative References – These references are missing URLs:  CSP2, Fetch, oauth-security-topics.
>
>
>
> 11.1.  Informative References – This reference is missing a URL:  HTML.
>
>
>
>                                                                 -- Mike
>
>