1574174 - fetch() returns Response objects with mutable headers

Status: VERIFIED FIXED

(Core :: DOM: Networking, defect, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

Step 6 here:

https://fetch.spec.whatwg.org/#dom-global-fetch

Requires that the Response returned from fetch() have immutable headers. Currently, though, firefox returns a Response object with mutable headers instead.

STR:

1. Load https://example.com
2. Open devtools
3. In console evaluate:

var r = await fetch('/');
r.headers.set('content-type', 'foo');

or for opaque responses:

var r = await fetch('https://code-cache-mime-type.glitch.me/client-no-code-cache.js', { mode: 'no-cors' });
r.headers.set('content-type', 'foo');

Setting the header should throw, but both succeed in firefox nightly for me.

I found this while investigating a similar issue with Cache API:

https://github.com/w3c/ServiceWorker/issues/1456

Ironically firefox restores the correct guard out of Cache API unlike chrome/safari, but fails to get the initial immutable guard set so the outcome is the same.

Marking a security bug because I'm unsure how big of a problem this is. Anne, please adjust as necessary.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

As I understand it, the risks we'd be concerned about here are that a bad actor could attempt to mutate an opaque response in a fashion that would enable its cross-origin contents to be exposed to the attacking origin. For example, by causing an image to be interpreted as JS by forging its content-type and returned to a script SW request. Or by attempting to make it look like a non-cors response was actually a CORS response.

Given our consistent wrapping of InternalResponse objects across all response tainting types (even basic!) where we copy the headers across, and that our consumption of headers is always of the UnfilteredHeaders()[1] which pierces/bypasses the wrapped InternalResponse, it's not clear that this could be exploited at an implementation level. At a spec level, I would think the "you can lie to yourself" security model for ServiceWorkers means this isn't a problem, but I also agree Anne is the right person to make the true determination!

1: https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla3dom16InternalResponse17UnfilteredHeadersEv&redirect=false

Comment 2

[Anne (:annevk)]

Those are the attacks I would be worried about as well. So if in a SW I get a non-basic response A out of the cache and then modify its headers, I technically modify A', its wrapper. And then when I pass A to a document, that document will only look at A, not A' to determine the contents. Is that even true if the document uses fetch()? There's no way where you can expose more headers of A by setting Access-Control-Expose-Headers on A' and such?

The model in the specification is such that when A is wrapped with A', A' cannot be modified, so it's not entirely clear to me what would happen when that's not the case.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:nhi, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Set responseObject to the result of creating a Response object, given response, "immutable", and relevantRealm.

I think all we need to do is call SetGuard immutable here.
Sunil, do you want to take this?

Comment 5

[Sunil Mayya]

Sure Valentin, I will check this.

Comment 6

[Sunil Mayya]

Attachment: Bug 1574174 - add immutable guard for fetch response headers. r=#necko,edenchuang

Comment 7

[Sunil Mayya]

Comment on attachment 9303713 [details]
Bug 1574174 - add immutable guard for fetch response headers. r=#necko,edenchuang

As this is security bug, I could not get this landed by adding the check-in flag in phabricator. Hence, requesting to land this bug. I have confirmed with Daniel Veditz [:dveditz] that a sec approval is not needed for this as it is just a sec-audit bug.

Comment 8

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Comment on attachment 9303713 [details]
Bug 1574174 - add immutable guard for fetch response headers. r=#necko,edenchuang

Done.
https://hg.mozilla.org/integration/autoland/rev/fcb447986ee1be3457e44c8b43167a193788f48e

Comment 9

[Sunil Mayya]

Thanks Valentin!

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

add immutable guard for fetch response headers. r=edenchuang,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/fcb447986ee1be3457e44c8b43167a193788f48e
https://hg.mozilla.org/mozilla-central/rev/fcb447986ee1

Comment 11

[Virgil Sangerean]

For validation, we attempted to reproduce the issue on 102.0, 100.0 and 96.0 Firefox on Windows 10 X64 but we could not reproduce it.
While running
"var r = await fetch('/');
r.headers.set('content-type', 'foo'); "
or
"var r = await fetch('https://code-cache-mime-type.glitch.me/client-no-code-cache.js', { mode: 'no-cors' });
r.headers.set('content-type', 'foo');"
in devtools console for the "http://example.com/" website we just got "undefined" as a response.

What should be the expected result for these commands?
Is there anything else that needs to be done to reproduce the issue?
Thank you!

Comment 12

[Sunil Mayya]

(In reply to Virgil Sangerean from comment #11)

For validation, we attempted to reproduce the issue on 102.0, 100.0 and 96.0 Firefox on Windows 10 X64 but we could not reproduce it.
While running
"var r = await fetch('/');
r.headers.set('content-type', 'foo'); "
or
"var r = await fetch('https://code-cache-mime-type.glitch.me/client-no-code-cache.js', { mode: 'no-cors' });
r.headers.set('content-type', 'foo');"
in devtools console for the "http://example.com/" website we just got "undefined" as a response.

What should be the expected result for these commands?
Is there anything else that needs to be done to reproduce the issue?
Thank you!

Thanks for checking this. The problem the bug reported is that we should not be able to modify the response headers received from fetch.
Hence, the following statements should throw:

var r = await fetch('/');
r.headers.set('content-type', 'foo'); 

Additionally, if you query for the header you should NOT get the value you have set you can verify this with a get command.

r.headers.get('content-type'); 

Before the fix the above statement would return "foo". After the fix you would see an exception when setting the response header.

Comment 13

[Virgil Sangerean]

Thank you for your help and clarifications!

Reproduced issue on 102.0 and 108.0 Firefox.

Issue is no longer reproducible on Nightly from 6th of December 2022. (109.0a1).

var r = await fetch('/');
r.headers.set('content-type', 'foo');
Uncaught (in promise) TypeError: Headers.set: Headers are immutable and cannot be modified.
<anonymous> debugger eval code:5
async* debugger eval code:6
debugger eval code:5:20
r.headers.get('content-type');
"text/html; charset=UTF-8"