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

Thanks for your thorough review Martin! We've published version 23 of the
draft addressing your feedback.

https://www.ietf.org/archive/id/draft-ietf-oauth-browser-based-apps-23.html

Notes on your feedback are inline. You can also see the specific feedback
and commits by following this GitHub issue:

https://github.com/oauth-wg/oauth-browser-based-apps/issues/73


On Thu, Jan 30, 2025 at 6:22 AM Martin Thomson via Datatracker <
noreply@ietf.org> 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:
https://www.ietf.org/archive/id/draft-ietf-oauth-browser-based-apps-23.html#section-6.3.4.2.1


>
> 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:
https://www.ietf.org/archive/id/draft-ietf-oauth-browser-based-apps-23.html#section-5


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