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

From: Mike Jones <[email-masked]>
Date: Wed, 20 Nov 2019 03:40:34 +0000

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

---

From: Benjamin Kaduk <[email-masked]>
Date: Fri, 22 Nov 2019 22:46:12 -0800

> 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 sec…
[...]
> 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 ht…

I would go further and say that we should not document "best practices"
that involve non-standardized values.

> 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.

(ditto)

[...]
> 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".

The RFC Editor uses a "restrictive" vs. "non-restrictive" split for "that"
and "which" -- see https://www.rfc-editor.org/styleguide/tips/ .

-Ben (no hats)

---

From: Brian Campbell <[email-masked]>
Date: Mon, 25 Nov 2019 12:41:45 -0700

On Fri, Nov 22, 2019 at 11:46 PM Benjamin Kaduk <[email-masked]> wrote:

> > 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.
>
> (ditto)
>

The document has a number of occurrences similar to these where a
particular solution is discussed even though it's not been standardized
and/or isn't actually a recommendation of the document. Such discussions
can instructive and have valuable information. But I wonder if it might be
more appropriate to omit them from the BCP? When the document is read and
understood in its full context, I do think the scope and intent of such
discussions are made reasonably clear. However, they could be pretty easily
misunderstood by someone just reading individual subsections or from citing
parts of the document text without the larger context. I guess I'm
thinking/suggesting that it'd be better if the BCP only focused on the
actionable recommendations it is making. And omit background type
discussion of alternative approaches that didn't get used for whatever
reason. Or, if they do stay in the document, de-emphasize them even further
like maybe moving them into an appendix rather than the main body of the
doc.

-- 
_CONFIDENTIALITY NOTICE: This email may contain confidential and privileged 
material for the sole use of the intended recipient(s). Any review, use, 
distribution or disclosure by others is strictly prohibited.  If you have 
received this communication in error, please notify the sender immediately 
by e-mail and delete the message and any file attachments from your 
computer. Thank you._

---

From: Daniel Fett <[email-masked]>
Date: Mon, 25 Nov 2019 21:49:55 +0100

+1. We should only discuss solutions if we would be okay with people actually implementing them. (See also my feedback to Guido's review.)

-Daniel

-- 
Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.

---

From: Aaron Parecki <[email-masked]>
Date: Mon, 25 Nov 2019 12:55:08 -0800

+1, I'm only comfortable making recommendations in this BCP if they
are in fact, the best current practice. In my mind that means nothing
aspirational, only things that are well established and that people
can act on today.

----
Aaron Parecki
aaronparecki.com

On Mon, Nov 25, 2019 at 12:50 PM Daniel Fett <[email-masked]> wrote:
>> On Fri, Nov 22, 2019 at 11:46 PM Benjamin Kaduk <kaduk@mit..edu> wrote:
>>>
>> The document has a number of occurrences similar to these where a particular solution is discussed even though it's not been standardized and/or isn't actually a recommendation of the document. Such discussions can instructive and have valuable information. But I wonder if it might be more appropriate to omit them from the BCP? When the document is read and understood in its full context, I do think the scope and intent of such discussions are made reasonably clear. However, they could be pretty easily misunderstood by someone just reading individual subsections or from citing parts of the document text without the larger context. I guess I'm thinking/suggesting that it'd be better if the BCP only focused on the actionable recommendations it is making. And omit background type discussion of alternative approaches that didn't get used for whatever reason. Or, if they do stay in the document, de-emphasize them even further like maybe moving them into an appendix rather than the main body of the doc.
>>
>>
>> CONFIDENTIALITY NOTICE: This email may contain confidential and privileged material for the sole use of the intended recipient(s). Any review, use, distribution or disclosure by others is strictly prohibited..  If you have received this communication in error, please notify the sender immediately by e-mail and delete the message and any file attachments from your computer. Thank you.
>
>
> --
> _______________________________________________
> OAuth mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/oauth

---

From: Torsten Lodderstedt <[email-masked]>
Date: Mon, 25 Nov 2019 23:02:31 +0100

Parts of the text in section 4 capture discussions of potential solutions and reasons why we decided in favor of a certain solution. I think this will be useful in the future and it has already proven useful for me, e.g. in the recent discussions around PoP vs audience restriction.

> Am 25.11.2019 um 21:55 schrieb Aaron Parecki <[email-masked]>:
>

---

From: Daniel Fett <[email-masked]>
Date: Tue, 26 Nov 2019 08:17:51 +0100

> Parts of the text in section 4 capture discussions of potential solutions and reasons why we decided in favor of a certain solution. I think this will be useful in the future and it has already pro…

Then let's move these discussions to an appendix or a separate document.
I have the feeling that some sections have too many "could"s, "might"s
and "should"s for a normative document. Another point is that the
alternative solutions that we are discussing often have not been
analyzed as thoroughly as the recommended solutions (see, e.g., PKCE vs.
Code-bound State vs. Token binding for Code).

-Daniel

---

From: Torsten Lodderstedt <[email-masked]>
Date: Tue, 26 Nov 2019 11:34:24 +0100

Moving the discussions to an appendix sounds good.

> On 26. Nov 2019, at 08:17, Daniel Fett <[email-masked]> wrote:
> 
> Then let's move these discussions to an appendix or a separate document. I have the feeling that some sections have too many "could"s, "might"s and "should"s for a normative document. Another point is that the alternative solutions that we are discussing often have not been analyzed as thoroughly as the recommended solutions (see, e.g., PKCE vs. Code-bound State vs. Token binding for Code).
> 
> -Daniel
> 
>