[OAUTH-WG] AD Review of draft-ietf-oauth-security-topics-24

[Roman Danyliw <[e-mail]>]

Hi!

I performed an AD review of draft-ietf-oauth-security-topics-24.  Thank you for taking the time to document many years of operational deployment experience.  My feedback is below:

From idnits:
** All documents that are called out as being updated in the meta-data need to be mentioned in the abstract:

  -- The draft header indicates that this document updates RFC6750, but the
     abstract doesn't seem to mention this, which it should.

  -- The draft header indicates that this document updates RFC6749, but the
     abstract doesn't seem to mention this, which it should.

  -- The draft header indicates that this document updates RFC6819, but the
     abstract doesn't seem to mention this, which it should.

** Replaced RFC7231 with RFC9110
  -- Obsolete informational reference (is this intentional?): RFC 7231
     (Obsoleted by RFC 9110)

** I haven’t done the detailed review, but how much of this document applies to OAuth 2.1?  If it is significant, should it be mentioned?

** I struggled to understand what was mandatory in the mix of RFC2119 keywords.
(a) Section 1 says “Nonetheless, it is RECOMMENDED that implementers upgrade their implementations and ecosystems when feasible.”

(b) Section 3 says “OAuth MUST ensure that the authorization of the resource owner (RO) (with a user agent) at the authorization server (AS) and the subsequent usage of the access token at the resource server (RS) is protected at least against the following attackers:”

(c) Section 3 later says “Implementers MUST take into account all possible types of attackers in the environment in which their OAuth    implementations are expected to run.”

(b) and (c) seem to be making strong statements mandatory requirements for high-level threats.  However, (a) seems to suggest that conformance is only recommended.  Furthermore, Section 4.* helpfully provides very extensive guidance on threat mitigation but there are a many “SHOULDs” with few rationales on why a given practice is not mandatory.  I recommend reviewing the “SHOULDs” in Section 4.* and confirming they can’t be MUSTs or explaining why the not.


** Section 1.  Editorial.
   OAuth 2.0 ("OAuth"
   in the following)

This parenthetical is a fragment.  Should it be “referred to as simply “OAuth” in this document”?

** Section 2.1
   When an OAuth client can interact with more than one authorization
   server, a defense against mix-up attacks (see Section 4.4) is
   REQUIRED.  To this end, clients SHOULD
         …

   In the absence of these options, clients MAY instead use ...

The text is clear on saying some defense from a mix-up attack is needed.  What happens if the client cannot use the methods prescribed by the SHOULD and MAY in this text?

** Section 2.1.1
   Clients MUST prevent authorization code injection attacks (see
   Section 4.5) and misuse of authorization codes using one of the
   following options:

What approach should a confidential client take of PKCE is not used?  It is only recommended in the subsequent bulleted list.

** Section 2.2.

   The privileges associated with an access token SHOULD be restricted
   to the minimum required for the particular application or use case.

Under what circumstances should access tokens not be restricted?  Can this be documented?

** Section 2.3
   In particular, access tokens SHOULD be restricted to certain resource
   servers (audience restriction), preferably to a single resource
   server.

Is there anyway to refine this text to make the interplay of the SHOULD and “preferably” clearer?

** Section 2.4
  and authentication processes that require multiple
   steps can be hard or impossible.

Is this difficulty due to complexity?  It would be helpful to refine why it is “hard”.

** Section 2.6.
   It is RECOMMENDED to use end-to-end TLS between the client and the
   resource server.  If TLS traffic needs to be terminated at an
   intermediary, refer to Section 4.13 for further security advice.

Is there further TLS guidance to provide?  Could BCP 195 be used (RFC9325)?

** Section 3.
     It must also be assumed that web attackers can lure the user to
      open arbitrary attacker-chosen URIs at any time.

Is “open” needed here?  Isn’t it just arbitrary URIs?

** Section 3.
Implementers MUST take into account all possible
   types of attackers in the environment in which their OAuth
   implementations are expected to run.  Previous attacks on OAuth have
   shown that OAuth deployments SHOULD in particular consider the
   following, stronger attackers in addition to those listed above:

The mix of MUST in the first sentence and SHOULD in the second is confusing.  The first sentence requires taking all possible attacks into consideration.  However, the second sentence then uses the weaker SHOULD for specific attacks that seem in-scope of the first sentence.

** Section 4.1

   Several
   successful attacks exploiting flaws in the pattern matching
   implementation or concrete configurations have been observed in the
   wild .

Could these be cited?

** Section 4.1.1.  Editorial.  These examples are very helpful.  For the inline URLs, consider putting them in quotes to improve readability.

** Section 4.5.3.  Editorial.
   There are two good technical solutions to achieve this goal, outlined
   in the following.

Recommend restating the goal.

** Section 4.7.1
   *  If state is used for carrying application state, and integrity of
      its contents is a concern, clients MUST protect state against
      tampering and swapping.  This can be achieved by binding the
      contents of state to the browser session and/or signed/encrypted
      state values as discussed in the now-expired draft
      [I-D.bradley-oauth-jwt-encoded-state].

Is [I-D.bradley-oauth-jwt-encoded-state] the only way this can be achieved?  If not, s/This can be achieved/An example of how this can be achieved/.  Otherwise, there is a normative dependency created on an expired draft.

** Section 4.7.1.  Editorial. s/but AS MAY/but the AS MAY/

** Section 4.8.1
      the client has set the parameter
       code_challenge=sha256(abc)

-- Isn’t it base64(sha265(abc))?

-- Recommend citing that this is S256 per Section 4.2 of RFC7636 otherwise IETF LC or IESG review feedback will likely ask for a SHA256 citation.

** Section 4.8.2.  Editorial?
   Therefore, ASs MUST take precautions against this threat.

Is that the same things as saying this attack MUST be mitigated in some way?

** Section 4.9.2.
   If the attacker were able to gain full control, including shell
   access, all controls can be circumvented and all resources can be
   accessed.

What is the link to “shell access”?

** Section 4.9.2.
   *  The resource server MUST treat access tokens like any other
      credentials.  It is considered good practice to not log them and
      not store them in plain text.

Consider tightening up the language here.  There are a multitude of credentials with varied practices.  Recommend being specific on the desired OAuth token behavior.

** Section 4.10.2.
   The proposed mechanism [RFC8707] could be used or by
   encoding the information in the scope value.

-- What is “proposed” about RFC8707?

-- Please provide a reference to the scope value (Section 3.3 of RFC6749)

** Section 4.12.  Typo. s/unambigiously/unambiguously/

** Section 4.14.2
   Authorization servers SHOULD determine, based on a risk assessment,
   whether to issue refresh tokens to a certain client.

How can the decision to issue refresh tokens selectively be ambiguous?  Either they are issued or not.  Why isn’t this a MUST?

** Section 4.18.2.  Formally cite that these are Javascript (?) examples.

Regards,
Roman