[OAUTH-WG] Additional WGLC review of OAuth 2.0 Security Best Current Practice by an AAD developer

[Mike Jones <[e-mail]>]

Please also add these WGLC comments that a Microsoft Azure Active Directory (AAD) developer asked me to convey:


  1.  In 4.12, "Authorization servers MUST determine based on their risk assessment whether to issue refresh tokens to a certain client [...]" I'm not sure what this requirement requires in practice. AAD issues refresh_tokens to all clients upon request and user consent and applies different lifetime policies to different clients. We also routinely make risk assessments about all manner of things. Does AAD thereby comply with this guideline? Reading the whole paragraph, I think the paragraph is trying to encourage OAuth clients which use a RT when the RT is returned but use auth codes when the RT is not returned. That's fine, but the current text comes off as imposing a vague requirement on authorization servers. Edits inline - "Authorization servers MUST MAY dynamically determine based on their risk assessment whether to issue refresh tokens to a certain client.  If the authorization server decides not to issue refresh tokens, the client may SHOULD refresh access tokens by utilizing other grant types, such as the authorization code grant type.  In such a case, the authorization server may utilize cookies and persistent grants to optimize the user experience."


  1.  In 4.12, the heading says "Authorization server MUST utilize one of these methods to detect refresh token replay for public clients" - however, it doesn't quite specify why refresh token replay is bad. The second paragraph described the goal more precisely ("If an attacker is able to exfiltrate and successfully replay [RTs]") - it's refresh token exfiltration that we MUST mitigate, not refresh token replay. Browser based apps that desire multiple access token should be able to use sender-constrained refresh tokens on multiple simultaneous HTTP requests - and some AAD apps do exactly this today. In practice, it's also difficult and undesirable to lock access to MSAL's RT cache - individual reads and writes are serialized, but { read, http request, write } events are not serialized. Here, just replace "to detect refresh token replay" with "to mitigate refresh token exfiltration" and then under "refresh token rotation," explain that such ASes using RT rotation to mitigate refresh token exfiltration MUST also forbid / prevent RT replay.


  1.  In 4.12, *Sender-constrained refresh tokens:* - sender-constrained tokens are absolutely valuable from a security perspective. However, without authenticating the sender, it's not necessarily an adequate protection for XSS. Some sender-constrained implementations (such as AAD's WIP spec for RTs) use un-attested keys and permit silent rebinding. Indeed, I think this will be common in browser based apps - attested keys have privacy implications leading to undesirable UX and rebinding prompts also lead to undesirable UX. An XSS attacker in those scenarios could likely silently rebind to an attacker-controlled key. It'd be good for the spec to acknowledge these aspects of sender-constrained tokens and make recommendations.
     *   It's these things that make us wary of moving from the implicit flow to RTs. The damage from exfiltrated RTs is much more severe, but the mitigations are less-specified by the BCPs, and thus seem less well-understood. Frankly, I think each method of mitigation is worth of its own subsection with more discussion - 4.12.1, 4.12.2, etc.


  1.  In 4.12, *Refresh token rotation:* - in addition to Mike Jones' review comment. "The authorization server cannot determine which party submitted the invalid refresh token, but it can revoke the active refresh token" is the guidance given around revoking stolen RTs. I think this guidance should use an RFC2119 word in place of "can" - and that the guidance should be stronger, e.g. "SHOULD" or "MUST", w.r.t revoking the active refresh token. Rotating RTs doesn't do much any good as mitigation if the stolen RT remains valid.


  1.  In 4.12, we'd like to see fixed-expiry RTs of ~1 day added as "one of the methods to mitigate RT exfiltration for public clients" that ASes MUST utilize one of. This is what we're implementing; we think it's sound; and we'd like to see the draft say so, to remove all doubt.


                                                       -- Mike