Re: [OAUTH-WG] Shepherd Review of draft-ietf-oauth-security-topics-23

[Daniel Fett <[e-mail]>]

Hi Axel,

I would be happy to see OAuth move away from state as a CSRF protection 
mechanism in the future, but there is not too much to be gained from 
relying solely on PKCE right now. The main advantage is that relying on 
PKCE incentivizes clients to properly manage the session state in a 
cookie instead of relying on a parameter. Beyond that, there's a small 
reduction in effort required by the client, messages will be smaller 
messages, etc.. The disadvantage is that a client puts CSRF protection 
in the hands of the AS. Therefore, we chose the wording "ensure" to say 
that the client has be sure that the AS actually implements PKCE 
correctly before relying on it. What that means in the concrete instance 
is up to the client.

Likewise, to your second point, I do not see enough of an advantage to 
RECOMMEND relying solely on PKCE for CSRF protection.

The main intention here is to open the door to rely on PKCE, e.g., in 
closed ecosystems, ecosystems with in-depth conformance testing, 
first-party applications and similar. This also helps to avoid a lot of 
convoluted language telling client developers how to properly choose, 
track, and check state values in profiles such as FAPI (and the pitfalls 
when interpreting that language).

-Daniel

Am 02.01.24 um 10:31 schrieb [e-mail]:
>
> Hi,
>
> sorry for being late in the game.
>
> I am not too happy with this section:
>
> "Clients that have ensured that the authorization server supports 
> Proof Key for Code Exchange (PKCE, [RFC7636 
> <https://oauthstuff.github.io/draft-ietf-oauth-security-topics/draft-ietf-oauth-security-topics.html#RFC7636>]) 
> MAY rely on the CSRF protection provided by PKCE."
>
>  1. Maybe a minor point that is due to not being a native speaker, but
>     the verb "ensure" seems too strong.
>     If the AZ states in its metadata, that it supports PKCE than this
>     is "ensurance" enough, right? The client does not have to "ensure"
>     the support by actually testing compliance, right?
>     I suggest rephrasing that to "*If the authorization server
>     **states in its meta-data support for Proof Key for Code Exchange*
>     (PKCE, [RFC7636
>     <https://oauthstuff.github.io/draft-ietf-oauth-security-topics/draft-ietf-oauth-security-topics.html#RFC7636>])
>     the client MAY rely on the CSRF protection provided by PKCE."
>  2. I suggest changing the "MAY" into a recommendation for all
>     OAuth2-based protocols. OIDC flows can easily support PKCE, and
>     new clients SHOULD use PKCE, I think.
>     Suggestion:
>     "If the authorization server states in its meta-data support for
>     Proof Key for Code Exchange (PKCE, [RFC7636
>     <https://oauthstuff.github.io/draft-ietf-oauth-security-topics/draft-ietf-oauth-security-topics.html#RFC7636>]),
>     it is RECOMMENDED the clientrelies on the CSRF protection provided
>     by PKCE."
>
> Kind regards
>
> Axel
>
> *From: *OAuth <[e-mail]> on behalf of Daniel Fett 
> <fett=[e-mail]>
> *Date: *Thursday, 28. December 2023 at 14:38
> *To: *[e-mail] <[e-mail]>
> *Subject: *Re: [OAUTH-WG] Shepherd Review of 
> draft-ietf-oauth-security-topics-23
>
> Hi Hannes,
>
> thanks again for your feedback! It is incorporated in the editor's 
> copy now.
>
> - 
> https://oauthstuff.github.io/draft-ietf-oauth-security-topics/draft-ietf-oauth-security-topics.html
>
> - Diff to published version: 
> https://author-tools.ietf.org/api/iddiff?doc_1=draft-ietf-oauth-security-topics&url_2=https://oauthstuff.github.io/draft-ietf-oauth-security-topics/draft-ietf-oauth-security-topics.txt 
> <https://author-tools.ietf.org/api/iddiff?doc_1=draft-ietf-oauth-security-topics&url_2=https://oauthstuff.github.io/draft-ietf-oauth-security-topics/draft-ietf-oauth-security-topics.txt>
>
> I plan to publish the next version once we have resolved the 
> discussion points from Roman's AD review.
>
> -Daniel
>
> Am 04.10.23 um 15:41 schrieb Tschofenig, Hannes:
>
>     Hi all,
>
>     here are some comments as part of my shepherd review of the OAuth
>     Security BCP.
>
>     First, I want to send a big "Thanks" to everyone in the group for
>     the work on this document and to the authors in particular. It has
>     taken us a while to come up with such an impressive list of
>     security recommendations for OAuth 2.0.
>
>     At this point in time my review comments can only be minor given
>     the amount of feedback this documents has already received.
>
>     Here are a few remarks.
>
>     I believe we should indicate that the specification updates other
>     OAuth RFCs. The obvious documents it updates are RFC 6749, RFC
>     6750 and RFC 6819.
>
>     You can set these "updates" in the template you are using.
>
>     In Section 1 you say:
>
>     "
>
>     It does not supplant the security advice given in
>
>        [RFC6749], [RFC6750], and [RFC6819], but complements those
>     documents.
>
>     "
>
>     In the subsequent paragraph you state that you "depreciate some
>     modes of operation".
>
>     I believe you are need to be clear about what you are doing in
>     relationship to these prior documents. It might also be useful to
>     say something about OAuth 2.1.
>
>     Expand abbreviations on first use. Example: "AS" and "PKCE" in
>     Section 2.1. The AS abbreviation is only expanded later in Section
>     3. Decide whether you want to use abbreviations or not. You mix
>     them throughout the document without no reasons.
>
>     Listing the abbreviations in Section 1.2 may also be useful. There
>     are not that many abbreviations anyway.
>
>     I have wording suggestions for this paragraph:
>
>     FROM:
>
>     "
>
>        Authorization servers SHOULD use client authentication if possible.
>
>        It is RECOMMENDED to use asymmetric (public-key based) methods for
>
>        client authentication such as mTLS [RFC8705] or using signed JWTs
>
>        ("Private Key JWT") in accordance with [RFC7521] and [RFC7523] (in
>
>        [OpenID.Core] defined as the client authentication method
>
>     private_key_jwt).  When such methods for client authentication are
>
>        used, authorization servers do not need to store sensitive
>     symmetric
>
>        keys, making these methods more robust against a number of attacks.
>
>     "
>
>     TO:
>
>     "
>
>        Authorization servers SHOULD enforce client authentication, if
>     possible.
>
>        It is RECOMMENDED to use asymmetric cryptography for
>
>        client authentication, such as mTLS [RFC8705] or using signed JWTs
>
>        ("Private Key JWT"), in accordance with [RFC7521] and [RFC7523] (in
>
>        [OpenID.Core] defined as the client authentication method
>
>     private_key_jwt).  When asymmetric cryptography for client
>     authentication is
>
>        used, authorization servers do not need to store sensitive
>     symmetric
>
>        keys, making client authentication more robust against leakage
>     of keys.
>
>     "
>
>     (Note: For the reader it is always better if they are told what
>     attacks
>
>     are prevented rather than saying "a number of attacks". You don't
>     want the reader
>
>     to guess what you mean.)
>
>     Section 2 is a summary of what follows in Section 4. Maybe you can
>     make this explicit
>
>     either in the title of Section 2 or in the first paragraph of
>     Section 2.
>
>     Section 3.
>
>     You write:
>
>     "
>
>        These attackers conform to the attacker model that was used in
>     formal
>
>        analysis efforts for OAuth [arXiv.1601.01229].  This is a minimal
>
>        attacker model.  Implementers MUST take into account all possible
>
>        types of attackers in the environment in which their OAuth
>
>     implementations are expected to run.
>
>     "
>
>     When you say "these attackers" please clarify which attackers you
>     are talking about.
>
>     Prior to this paragraph you have just spoken about various forms
>     of network attackers.
>
>     Just before that you talked about network and web attackers.
>
>     Then, you introduce more attackers and you keep talking about
>     "this attacker model" and
>
>     "these attackers". Make it easier for the reader by referring
>     explictly which attackers
>
>     you are talking about in a specific paragraph.
>
>     Then, you conclude the section with a hint that there is an even
>     stronger attacker model.
>
>     As a reader I might want to know what this stronger attacker model
>     looks like and why you
>
>     do not consider it in this document.
>
>     Section 4.1.1:
>
>     You write:
>
>     "
>
>     Note: Vulnerabilities of this kind can also exist if the
>
>        authorization server handles wildcards properly.
>
>     "
>
>     I believe you are saying that the vulnerabilities caused by
>     incorrect redirect URI validation parsing when you refer to "this
>     kind".
>
>     I would also remove the "note"
>
>     Section 4.1.3:
>
>     You write:
>
>     "
>
>        *  Servers on which callbacks are hosted MUST NOT expose open
>
>     redirectors (see Section 4.11).
>
>     "
>
>     Are you talking about authorization servers (which is what was
>     referenced in the paragraph before)?
>
>     Section 4.10.1: Sender-constrained Access Tokens
>
>     The text gives the reader the impression that the token binding
>     would be an option for developers to use.
>
>     I don't think that this is the case. I am particularly referring
>     to this sentence:
>
>     "
>
>        *  *DPoP* ([I-D.ietf-oauth-dpop]): DPoP (Demonstration of Proof-of-
>
>           Possession at the Application Layer) outlines an
>     application-level
>
>     sender-constraining for access and refresh tokens that can be used
>
>           in cases where neither mTLS nor OAuth Token Binding (see below)
>
>           are available.
>
>     "
>
>     I would change it to:
>
>     "
>
>        *  *DPoP* ([I-D.ietf-oauth-dpop]): DPoP (Demonstration of Proof-of-
>
>           Possession at the Application Layer) outlines an
>     application-level
>
>     sender-constraining for access and refresh tokens that can be used
>
>           in cases where mTLS is not available.
>
>     "
>
>     I would then remove the subsequent text talking about old, expired
>     drafts.
>
>     Alternatively, you could move the text to the appendix.
>
>     Section 4.10.2: Audience Restricted Access Tokens
>
>     In the text you say:
>
>     "
>
>        Audience restriction essentially restricts access tokens to a
>
>        particular resource server.  The authorization server
>     associates the
>
>        access token with the particular resource server and the resource
>
>        server SHOULD verify the intended audience.
>
>     "
>
>     You have to put a MUST here. If the resource server does not check
>     the audience
>
>     restriction when using audience restricted access tokens then you
>     obviously do not
>
>     get the value from it. It is like using DPOP and not using the
>     proof-of-possession.
>
>     Likewise the SHOULD language in this sentence is also questionable:
>
>     "
>
>     The client SHOULD tell the authorization server the intended resource
>
>        server.  The proposed mechanism [RFC8707] could be used or by
>
>        encoding the information in the scope value.
>
>     "
>
>     If the client does not tell the authorization server what the
>     intended resource server
>
>     is then how should the authorization server know (unless in a very
>     limited setup).
>
>     Also the reference to RFC 8707 is a bit weak. We standardized
>     resource indicators: why not
>
>     recommend using it?
>
>     Section 4.10.3: The section heading is "Discussion: Preventing
>     Leakage via Metadata".
>
>     The content of the section is not really a discussion but rather a
>     description of why
>
>     this path has not been taken. I wonder whether it would be better
>     to move this section
>
>     to the appendix and then start the text by explaining why other
>     solutions have been used instead of this approach.
>
>     Section 4.11: I would put the definition about what an "open
>     redirector" is into the terminology section since you
>
>     are using the term already in earlier sections. Here is the
>     definition:
>
>     "
>
>     An open redirector is an endpoint that forwards a user’s
>
>        browser to an arbitrary URI obtained from a query parameter.
>
>     "
>
>     Typos/Wording:
>
>     FROM:
>
>     "
>
>     Afterwards, the updated the OAuth attacker model is presented.
>
>     "
>
>     TO:
>
>     "
>
>     Afterwards, the updated OAuth attacker model is presented.
>
>     "
>
>     Section 4.1:
>
>     "... wild ."
>
>              ^
>
>     Consider using the guidelines for inclusive language:
>
>     https://www.rfc-editor.org/part2/#inclusive_language
>     <https://www.rfc-editor.org/part2/#inclusive_language>
>
>     For example, "If the attacker is able to ... , **he** will
>     directly get access to ..."
>
>     Another example is "whitelisted".
>
>     Section 4.1.2: a wording suggestion.
>
>     FROM:
>
>     "
>
>     The attack
>
>        described here combines this behavior with the client as an open
>
>        redirector (see Section 4.11.1) in order to get access to access
>
>        tokens.
>
>     "
>
>     TO:
>
>     "
>
>     The attack
>
>        described here combines this behavior with the client as an open
>
>        redirector (see Section 4.11.1) to obtain access tokens.
>
>     "
>
>     Section 4.7.1: word missing
>
>     FROM:
>
>     "
>
>     PKCE provides robust protection against CSRF attacks even in presence
>
>        of an that can read the authorization response (see Attacker A3 in
>
>        Section 3).
>
>     "
>
>     TO:
>
>     "
>
>     PKCE provides robust protection against CSRF attacks even in presence
>
>        of an attacker that can read the authorization response (see
>     Attacker A3 in
>
>        Section 3).
>
>     "
>
>     Section 4.18.2: capitalization
>
>     FROM:
>
>     "
>
>     Wildcard origins like "*" in postMessage MUST not be used as
>
>        attackers can use them to leak a victim's in-browser message to
>
>        malicious origins.
>
>     "
>
>     TO:
>
>     "
>
>     Wildcard origins like "*" in postMessage MUST NOT be used as
>
>        attackers can use them to leak a victim's in-browser message to
>
>        malicious origins.
>
>     "
>
>     You might also want to replace the short title
>     "oauth-security-topics" (which can be found on each page) with
>     something like "OAuth 2.0 Security BCP".
>
>     Ciao
>
>     Hanns
>
>
>
>     _______________________________________________
>
>     OAuth mailing list
>
>     [e-mail]
>
>     https://www.ietf.org/mailman/listinfo/oauth
>
> -- 
> Please use my new email address:[e-mail]
>
> _______________________________________________
> OAuth mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/oauth