Thanks for your thorough review Martin! We've published version 23 of the draft addressing your feedback.
Notes on your feedback are inline. You can also see the specific feedback and commits by following this GitHub issue:
On Thu, Jan 30, 2025 at 6:22 AM Martin Thomson via Datatracker <noreply@xxxxxxxx> wrote:
Reviewer: Martin Thomson
Review result: Ready with Nits
Hey,
I've been tasked with looking at this document for the HTTP directorate, for
which I would just look at Section 6.1.3.2 and say "yup, that's how you do
cookies right; even if the prohibition on domain seems unnecessary, it's not a
useful feature anyway". But I wanted to read the document to see if you had
other HTTP uses in mind. And it was an interesting perspective on how browsers
work.
For the most part, this a pretty good document and I see no problems with it
proceeding. This review is provided in the hopes that it can be made slightly
better.
Thank you! I hope it has been made slightly better from this.
The underlying premise here is that your browser app will run malicious code.
Which is pretty challenging, because you have given away everything at that
point. Game over, man.
Of course, there's a nice little save hidden in Section 5.1.4: proxying
requests via an active browser is out of scope. At first blush this seems like
a cop-out, but I really appreciate the thoroughness of this documentation of
how to limit the harm. There's a bit more in Section 5.2.3 about how this
attack might not always confer all the powers that an attacker might want.
>From my perspective, it might be good to be clearer up front about what can and
cannot be achieved here. Put differently, a little more clarity about the
threat model up front might help. That is something like an applicability
statement might help:
We've added a variation of this paragraph: https://www.ietf.org/archive/id/draft-ietf-oauth-browser-based-apps-23.html#section-5
> [...] malicious _javascript_ code has the same privileges as the legitimate
application code. All JS applications are exposed to this risk in some degree.
Applications might also obtain OAuth tokens that confers authorization that are
necessary to their functioning. In combination, this effectively gives
compromised code the ability to use that authorization for malicious ends > >
Though the risk of attacker abuse of authorization is unavoidable, there are
ways to limit the extent to which a compromised application can abuse that
authorization. For instance, this access might be limited to times when the
application is in active use by ensuring that tokens cannot be obtained by an
attacker, by limiting the type of tokens that might be obtained, or by binding
the tokens to the application.
Beyond that, I have only quibbles. And a few niggles.
# Quibbles
I found the use of "client" to be a little confused at times. It wasn't always
crisp about whether this was a person or a browser application. Take Section
6.3.3.2, which says: "the authorization server SHOULD NOT process authorization
requests automatically without user consent or interaction, except when the
identity of the client can be assured". That's a person. In the previous
section, "end-user" is used to refer to a person and "client" is the code that
runs the browser. Presumably in this case "client" refers to the identity of
the entity that is asking for authorization.
"Client" is an OAuth term that refers to the application, not the end user. "Identity of the client" is a phrase from RFC6749. We expanded this to "client application" to hopefully not confuse this with the end user here.
This in Section 6.3.4.2.1 makes no sense to me: "A practical implementation
pattern can use a Web Worker [WebWorker] to isolate the refresh token, and
provide the application with the access token making requests to resource
servers." I can't work out how this would help at all.
We added a sentence to clarify what this is meant to protect, which is only preventing the attacker from using the application's refresh token:
None of this section makes sense to me. The last paragraph says something
about a "perfect storage mechanism", which implies that some set of criteria
have been established whereby we could deem some storage "perfect". This
extends to Section 8, which also references some platonic ideal.
We've updated all mentions of "perfect storage" to phrases like "...even a token storage mechanism that completely isolates the tokens from the attacker..." Hopefully this is clearer.
The advice about DPoP in Section 6.3.4.2.2 is gold. Could this be made more
prominent? (I don't like WebCrypto in terms of its design, but the basic
capability here is definitely useful.)
I actually don't think it would be helpful to emphasize DPoP more than it already is, since the context of this section is to explain which of the previously mentioned threats are solved by each technique, and using DPoP still leaves open some attacks.
Section 6.3.4.2.3 doesn't distinguish refresh tokens, which I would when it
comes to the fanciful solutions being speculated upon.
There isn't anything particularly unique about refresh tokens here. I added an explicit mention of refresh tokens to hopefully make that more clear.
I struggled with Section 7.4.1.1. The point about an unregistered service
worker remaining resident is distracting. The point that the codes will be
visible to the compromised code when a new browsing context is created is key,
but the immediate impression was that it was OK because the resident worker
would still block requests. Then, the presumption was that the token would be
exposed to the application somehow. That's true for implicit grants, which use
URLs, and when the client receives tokens directly. But I don't understand how
it would be possible of tokens are provided using cookies in the way you
recommend. In that case, cookies are handled by the browser and cannot be read
by script, malicious or otherwise. The text says that the token can be read
from a frame URL, which suggests maybe this really is considering implicit
grants (that's a terrible name, by the way, there's nothing at all implicit
about it, I guess it's a legacy thing: a bad name is forever).
This isn't about the implicit flow at all, it's about the attacker starting their own authorization code flow. And yes I agree "implicit" is a terrible name.
The premise of Section 8.3 seems wrong to me. It's probably a good idea to
state this explicitly: the reason that it might be better to store refresh
tokens in a web worker is that the functionality of the worker can be limited
in scope, which makes the overall vulnerability to attack less. That's sort of
true, but it presumes a bunch of things, not all of which necessarily hold. In
general, the browser security model places mainline code and workers (and
worklets) in the same security domain. It's possible that attacks haven't
evolved these capabilities to date, but workers can be started with compromised
code created by compromised main code, which means that workers can be induced
to execute bad code if the main code is compromised. Sure, you need to replace
the worker ahead of when a refresh occurs, but that seems well within the
attacker capabilities described. You can maybe curtail some of this with CSP,
but I've already mentioned that...
I tried to rephrase this a bit because the intent is not what you're describing. This section is trying to explain why token storage in a web worker still doesn't actually solve everything.
# Niggles
Please cite (and expand) on first use of PKCE, DPoP, and other jargon.
I did a pass through the document and updated all of these with definitions and citations.
Generally, I found the lack of mention for CSP concerning; it's an core web
security mechanism, but it only gets mention in passing. For instance, it's a
pretty effective defence against the attacks described in S5.2.3, in some ways
more so than CORS.
We added a chunk to section 5 that lists out several things such as CSP that should be considered first:
"section 11.4 of [RFC9449]" -- assuming use of kramdown-rfc, you should change
the source to "{{Section 11.4 of !RFC9449}}" to get that linked up. That's a
problem throughout.
Thanks, I had no idea this was a thing. Fixed throughout.
In Section 8.5, please consider de-emphasis of local storage. Browsers have
been trying to move people off the jank-inducing API for more than a decade
now. Anything that uses storage needs to be asynchronous, local storage isn't.
I added a mention of localStorage being synchronous.
In Section 8.6, this is not always true: "browsers ultimately store data in
plain text on the filesystem". I would instead say that you cannot rely on
encryption at rest.
Of course you're correct. This has been changed to reference "encryption at rest".
Your citations of the HTML spec isn't what WHATWG recommend. Please ask them
to make #webstorage stable (see https://whatwg.org/working-mode#anchors) and
cite the latest.
Thanks, I've made the request and updated the references: https://github.com/oauth-wg/oauth-browser-based-apps/issues/85
-- last-call mailing list -- last-call@xxxxxxxx To unsubscribe send an email to last-call-leave@xxxxxxxx
