Httpdir last call review of draft-ietf-oauth-browser-based-apps-22

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.

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:

> [...] 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.

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.

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.

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.)

Section 6.3.4.2.3 doesn't distinguish refresh tokens, which I would when it
comes to the fanciful solutions being speculated upon.

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).

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...

# Niggles

Please cite (and expand) on first use of PKCE, DPoP, and other jargon.

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.

"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.

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.

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.

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.

Received on Thursday, 30 January 2025 06:22:42 UTC