[OAUTH-WG] WGLC review of OAuth 2.0 Security Best Current Practice by Mike Jones

[Mike Jones <[e-mail]>]

I did a complete read of draft-ietf-oauth-security-topics-13<https://tools.ietf.org/html/draft-ietf-oauth-security-topics-13>.  My review comments follow, divided into substantive and editorial sections.

SUBSTANTIVE

2. Attacker Model, (A1) - Attacker description (A1) actually describes two kinds of attacks - the Web Attackers attack at the start of the bullet and the phishing attack, whose description starts with "It must also be assumed that web attackers can lure the user".  Separate these two distinct attacks in the description, labelling the phishing attack (A2).

3.1 Protecting Redirect-Based Flows, paragraph 3 - Please state that the use of "nonce" as defined in OpenID Connect is another means of preventing CSRF attacks (similar to how state can be used for this).

3.1.1. Authorization Code Grant - This starts by saying that "Clients utilizing the authorization grant type MUST use PKCE" but later contradicts this MUST by saying "OpenID Connect clients MAY use the "nonce" parameter of the OpenID Connect ... for the same purpose".  At the very least, reword the second to say "Alternatively, OpenID Connect clients...", making it clear that MUST does not apply to Connect clients.  Or consider other editorial ways to make this distinction even clearer.

3.1.2. Implicit Grant - The statement "no viable mechanism exists to cryptographically bind access tokens issued in the authorization response to a certain client" isn't actually true.  Please describe how the OpenID Connect ID Token achieves this for the "id_token token" and "code id_token token" response types via the "at_hash" claim in this paragraph, and appropriately qualify the unconditional statement cited.

3.3. Access Token Privilege Restriction, last paragraph - This paragraph is essentially talking about placing audience restrictions on access tokens, but fails to use the standard "audience" term.  Please revise to explicitly describe utilizing audience restrictions for access tokens.

4.1.3. Proposed Countermeasures, second bullet - Please explain how "Clients MAY drop fragments" in such a way that the recommendation is actionable without having to go and read the reference.

4.3.2.  Access Token in Browser History - Strengthen the prohibition on passing access tokens in query parameters along the following lines: "Clients MUST NOT pass access tokens as a URI query parameter in the way described in Section 2.3 of [RFC 6750]."  (As background, note that the OpenID Certification testing software treats doing so as a FAILURE, preventing certification.)

4.4.1. Attack Description - Item 2 says "The attacker intercepts this request and changes the user's selection to "A-AS"."  Please expand this description to say *how* the attacker accomplishes this.  (Most readers of this spec will not be security experts, so this should be spelled out for them.)

4.4.1. Attack Description - Item 4 says "Now the attacker intercepts this response and changes the redirection such that the user is being redirected to H-AS."  Like the previous comment, please expand this description to say *how* the attacker accomplishes this.

4.4.1. Attack Description - The last bullet cites attacks on OpenID Connect.  Again, to make this description more accessible to readers, it would be useful to include a one-sentence description of each of the pertinent attacks here, saying how they work and what they accomplish.

4.5.1. Attack Description - Item 3 says "The attacker injects the stolen authorization code in the response of the authorization server to the legitimate client".  As per pervious comments, please expand this description to say *how* the attacker accomplishes this.

4.5.1. Attack Description, item 4 - Clearly say whether the client_id and client_secret used are those of the attacker's client or those of the client being attacked.  If the latter, say how these values for the client being attacked were obtained by the attacker.

4.5.3. Proposed Countermeasures - The draft uses the term "pre-warmed secrets" without defining the term or providing a citation for it.  Please do both.

4.7.1. Proposed Countermeasures - Change "Alternatively, PKCE provides CSRF protection" to "Alternatively, PKCE or the "nonce" provide CSRF protection".

4.7.1. Proposed Countermeasures - Change "If an authorization server does not support PKCE, "state" MUST be used for CSRF protection" to "If an authorization server does not support PKCE, "state" or "nonce" MUST be used for CSRF protection".

4.7.1. Proposed Countermeasures, last paragraph - Expand the oblique phrase "standard CSRF defenses" to give examples of such defenses and citations for them.  As currently written, this countermeasure won't be comprehensible or actionable by many readers.

4.8.1.1. Metadata - This section suggests the use of a "resource_servers" metadata value.  This isn't defined by RFC 8414 nor do I see it the IANA OAuth Authorization Server Metadata registry at https://www.iana.org/assignments/oauth-parameters/oauth-parameters.xhtml#authorization-server-metadata.  Is this something that's been standardized elsewhere?  If so, please add a citation.  If not, please clearly say that this metadata value is not standardized, and is therefore unlikely to be interoperable.

4.8.1.1. Metadata - This section suggests the use a "access_token_resource_server" token response value.  Please likewise clearly say that this parameter isn't a standard.

4.8.1.2. Sender-Constrained Access Tokens - List DPoP as one of the possibilities for demonstrating proof of possession.

4.8.1.3. Audience Restricted Access Tokens, next to last paragraph - When you list MTLS, also please state that the MTLS key can be used as a correlation handle, since it is the same for connections to all sites.

4.12. Refresh Token Protection - The draft says "refresh tokens belonging to the same grant may share a common id".  Augment this description to say what this ID is and where it is located, and if there's a pertinent standard or other reference for this ID, please include a citation.


EDITORIAL

1. Introduction - The wording "and, as the foundation of OpenID Connect [OpenID<https://tools.ietf.org/html/draft-ietf-oauth-security-topics-13#ref-OpenID>], identity providing" is awkward.  Change to "and as the basis for federated login using OpenID Connect [OpenID]".

1. Introduction - Change "While OAuth was used in a variety of scenarios and different kinds of deployments, the following challenges could be observed" to "While OAuth is used in a variety of scenarios and different kinds of deployments, the following challenges can be observed".

1. Introduction, bullet 1 - Add citations for the first uses of the terms "CSRF" and "referer header".

Many locations - You have misspelled the "referer" header name as "referrer" throughout the document.  (Yes, "referrer" is the correct English spelling.  The header name uses an English misspelling.)

1. Introduction, last paragraph - Change "such as [RFC7591<https://tools.ietf.org/html/rfc7591>] and [RFC8414<https://tools.ietf.org/html/rfc8414>]" to "such as the OAuth 2.0 Dynamic Client Registration Protocol [RFC7591<https://tools.ietf.org/html/rfc7591>] and OAuth 2.0 Authorization Server Metadata [RFC8414<https://tools.ietf.org/html/rfc8414>]".

1. Introduction, last paragraph - Delete "As a challenge to the community", as it doesn't add any value to the exposition.

2. Attacker Model, (A3) - Add a citation for the first use of the term "mix-up attack" (and consider deleting "so-called").

3.1.1. Authorization Code Grant - Change "authorization grant type" to "authorization code grant type".

3.1.2. Implicit Grant - The canonical order of composite response types is alphabetical.  Therefore, please change "token id_token" to "id_token token" and "code token id_token" to "code id_token token".

3.1.2. Implicit Grant, last paragraph - Add ", such as the "code id_token" response type" at the end of the first sentence.

3.3. Access Token Privilege Restriction, last paragraph - This paragraph is essentially talking about placing audience restrictions on access tokens, but fails to use the standard "audience" term.  Please revise to explicitly describe including audience restrictions for access tokens.

3.4. Resource Owner Password Grant - Include citations where you currently say (WebCrypto, WebAuthn).

4.1. Insufficient Redirect URI Validation - Add citations for the "Several successful attacks..." mentioned.

4.8.1.2. Sender-Constrained Access Tokens - Delete "so-called".

Many locations - You use the term "id", which should always be spelled "ID".  (The English word "id" has a completely different meaning than "ID".)  Be sure to also change instances of "ids" to "IDs".

4.8.1.3. Audience Restricted Access Tokens - Delete "basically".

Many locations - The draft often uses the word "which" when you mean "that".  For instance, in 4.9.2, please change "which could" to "that could" and in 4.11 change "which are" to "that are".  There's lots of places to look up the difference in meaning, but a rule of thumb that's usually right is that if you don't have a comma before the "which", it should probably be "that".

4.12. Refresh Token Protection - Change "legit" to "legitimate".

May locations - The spelling of ID Token is inconsistent in the draft - sometimes using "token" and sometimes using "Token".  I would suggest consistently using the spelling from OpenID Connect, which is "ID Token".

5.  Acknowledgements - If you choose to add me to the acknowledgements, please add me as "Michael B. Jones".  (Mike Jones is way too common a name.)

Thanks for taking these comments into account.

                                                       -- Mike