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

[Daniel Fett <[e-mail]>]

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]