1648635 - Changes from channel.setResponseHeader are persisted in the (disk) cache

Status: NEW

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

Description

[Rob Wu [:robwu]]

When a server's response can be cached, any internal calls to channel.setResponseHeader also end up in the cache.
This is bad, because extensions can change headers (via the webRequest API), and the expectation is that the header is not cached. By caching client-side header modifications, two bugs occur:

• extensions are unable to read the original response headers from the "cached" "server response"
• modifications from extensions persist even after an extension is uninstalled.

Ideally, client-side header modifications should be stored separately from the server's actual response.

Here are unit tests that confirms the current (wrong) behavior (tests from bug 1376932):
https://searchfox.org/mozilla-central/rev/cfaa250d14e344834932de4c2eed0061701654da/toolkit/components/extensions/test/xpcshell/test_ext_webRequest_cached.js#125-311

STR for manual testing (tested with 77 and Nightly 79.0a1 20200624093107 ):

1. Create a server (e.g. netcat) at localhost to listen at port 12345.
2. Open a privileged console (e.g. global JS console or console in a privileged about:-page such as about:config) and run the following snippet. It will add CORS headers once:

var { WebRequest } = ChromeUtils.import("resource://gre/modules/WebRequest.jsm");
WebRequest.onHeadersReceived.addListener(function listener({responseHeaders}) {
  responseHeaders = [...responseHeaders, { name: "Access-Control-Allow-Origin", value: "*" }];
  console.log("Rewritten response headers", responseHeaders);
  setTimeout(() => { WebRequest.onHeadersReceived.removeListener(listener); }, 1000);
  return {responseHeaders};
}, {
  urls: new MatchPatternSet(["http://localhost/dummyreq*"]),
}, ["blocking", "responseHeaders"]);

3. Visit example.com, open the console and run:

await fetch("http://localhost:12345/dummyreq");

4. The server will get a request for /dummyreq. Respond with:

HTTP/1.1 200 OK
Cache-Control: max-age=600
ETag: testmytag
Content-Length: 1

x

5. Console from step 3 should show that the request has succeeded.
6. Repeat step 3.

Expected:

• The server should not get a request (because of the cache).
• The fetch() request should fail with a CORS error (because the original response did not have CORS headers).

Actual:

• The server should not get a request (because of the cache, as expected).
• BAD: The fetch() request succeeded (i.e. CORS modification from the server was cached).
• BAD: ProfD/cache2/entries/ contains a file WITH modifications (search for file containing "testmytag").
  • BAD (separate bug): This entry is NOT listed in about:cache

(side note: The devtools shows the response from the server, without client-side modifications - this is bug 1554345).

Comment 1

[Rob Wu [:robwu]]

:mayhemer Do you know what is happening?

This bug has been in the back of my head for a while, and I started to explore it to check whether the proposed fix for bug 1645204 is safe. Fortunately, modified response headers are somehow not persisted when channel.redirectTo is used. Still, I want to know why, and how this bug can be fixed to prevent malicious extensions from poisoning the cache.

Comment 2

[Honza Bambas (:mayhemer)]

(In reply to Rob Wu [:robwu] from comment #1)

:mayhemer Do you know what is happening?

Modifying an actual response head(ers) from the server is made directly in the structure we keep the response. And it is later put to the cache as the only one source of the response. I think we deliberately store headers to the cache after all "http-on-*" notifications have been done to allow the persistence - it's a wanted behavior.

This bug has been in the back of my head for a while, and I started to explore it to check whether the proposed fix for bug 1645204 is safe. Fortunately, modified response headers are somehow not persisted when channel.redirectTo is used. Still, I want to know why, and how this bug can be fixed to prevent malicious extensions from poisoning the cache.

redirectTo is called before we read the cached response head and we thus modify only the temporary response head we create in redirectTo (with the patch).

As we never receive a response from the server, we never update the cache entry on disk. We never get to the point of calling InitCacheEntry -> AddCacheEntryHeaders,
https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla3net13nsHttpChannel14InitCacheEntryEv&redirect=false
https://searchfox.org/mozilla-central/rev/cfaa250d14e344834932de4c2eed0061701654da/netwerk/protocol/http/nsHttpChannel.cpp#5558

Comment 3

[Rob Wu [:robwu]]

(In reply to Honza Bambas (:mayhemer) from comment #2)

(In reply to Rob Wu [:robwu] from comment #1)

:mayhemer Do you know what is happening?

Modifying an actual response head(ers) from the server is made directly in the structure we keep the response. And it is later put to the cache as the only one source of the response. I think we deliberately store headers to the cache after all "http-on-*" notifications have been done to allow the persistence - it's a wanted behavior.

Caching the changes isn't an issue if the modifications are always the same, but as extensions always get a chance to modify the response (via "http-on-examine-cached-response" and "http-on-examine-merged-response"), it is not desirable to always cache such headers (especially not if the extension is uninstalled).

Is it possible to have the ability to modify response headers without updating the cache? From some experimentation with WebRequest.jsm, I can see that .setResponseHeader on a response from the cache doesn't update the cached entry. This is what I also like to see for fresh responses from the server, whether it's going to be cached or not.

modified response headers are somehow not persisted when channel.redirectTo is used. Still, I want to know why, and how this bug can be fixed to prevent malicious extensions from poisoning the cache.

redirectTo is called before we read the cached response head and we thus modify only the temporary response head we create in redirectTo (with the patch).

redirectTo can also be called while handling http-on-examine-response (aka onHeadersReceived in webRequest). My wild guess is that the cache isn't populated when the request is redirected with redirectTo, possibly due to the special way that redirectTo is handled (with several hooks in the channel implementation).

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Rob Wu [:robwu] from comment #3)

(In reply to Honza Bambas (:mayhemer) from comment #2)

(In reply to Rob Wu [:robwu] from comment #1)

:mayhemer Do you know what is happening?

Modifying an actual response head(ers) from the server is made directly in the structure we keep the response. And it is later put to the cache as the only one source of the response. I think we deliberately store headers to the cache after all "http-on-*" notifications have been done to allow the persistence - it's a wanted behavior.

Caching the changes isn't an issue if the modifications are always the same, but as extensions always get a chance to modify the response (via "http-on-examine-cached-response" and "http-on-examine-merged-response"), it is not desirable to always cache such headers (especially not if the extension is uninstalled).

Is it possible to have the ability to modify response headers without updating the cache?

As I said, it depends on when we call InitCacheEntry, if the modification of headers happens before, it will be stored. If after, it likely won't. There is no explicit handling of this behavior right now and it would mean some architectural rethink.

From some experimentation with WebRequest.jsm, I can see that .setResponseHeader on a response from the cache doesn't update the cached entry. This is what I also like to see for fresh responses from the server, whether it's going to be cached or not.

As above, InitCacheEntry is probably called before the modification.

modified response headers are somehow not persisted when channel.redirectTo is used. Still, I want to know why, and how this bug can be fixed to prevent malicious extensions from poisoning the cache.

redirectTo is called before we read the cached response head and we thus modify only the temporary response head we create in redirectTo (with the patch).

redirectTo can also be called while handling http-on-examine-response (aka onHeadersReceived in webRequest). My wild guess is that the cache isn't populated when the request is redirected with redirectTo, possibly due to the special way that redirectTo is handled (with several hooks in the channel implementation).

on-examine is called before InitCacheEntry. So the modifications will be cached.

---

It's hard to decide when we want the header modifications be or not be cached. If extensions do that, it may be good to make the modification visible only to consumers of the response but not store them permanently. But there can be cases we want to store them, tho I don't have an example by hand. One could be Set-Cookie and WWW-/Proxy-Authenticate headers which we convert values of.

Also note that we do store the original headers separately from updated headers, but I don't recall exactly how it works for new headers. I would have to take a look.

To conclude: I'd like you to come up with some specification and rational what the behavior should be.

Comment 5

[Rob Wu [:robwu]]

It's hard to decide when we want the header modifications be or not be cached. If extensions do that, it may be good to make the modification visible only to consumers of the response but not store them permanently. But there can be cases we want to store them, tho I don't have an example by hand. One could be Set-Cookie and WWW-/Proxy-Authenticate headers which we convert values of.

Also note that we do store the original headers separately from updated headers, but I don't recall exactly how it works for new headers. I would have to take a look.

(I think that I found what you meant, see below at HeaderVariety)

I'd like you to come up with some specification and rational what the behavior should be.

I would like "external" changes via setResponseHeader to not be stored in the cache. At a high level:

Data producer (server or cache)
-> produces data
-> headers parsed and processed
-> store in cache if needed (without extension modifications if it happened before)
-> extension modifications
-> other header consumers (cookies, CORS, CSP, etc.)
-> data consumed by client (e.g. rendering or fetch).

I can imagine the "store in cache" part to run after "extension modifications" in practice (which is why this bug happens). Re-ordering calls sounds like too much work, so I was thinking of keeping track of whether a response header originated from the network/network internals or externally (e.g. extensions). Consumers will see the same as before, but the cache will ignore external header entries.
Currently, cache-related header modifications from extensions do affect the caching behavior. If it makes the implementation easier, I'm fine with dropping support for the functionality, because this ability to influence caching behavior is not documented, and Chrome does explicitly not support this.

While digging in the source, I found the HeaderVariety enum that seems to relate to what I just described, except it was meant to distinguish header data on the wire from header data after parsing by Firefox internals (and possibly the source of bug 1554345?):

  enum HeaderVariety {
    eVarietyUnknown,
    // Used only for request header.
    eVarietyRequestOverride,
    eVarietyRequestDefault,

    // Used only for response header.
    eVarietyResponseNetOriginalAndResponse, <--
    eVarietyResponseNetOriginal,
    eVarietyResponse

I'm thinking of adding eVariertyResponseOverride. This behaves like eVarietyResponse in almost every case, except it is ignored by the cache.

An alternative implementation is to copy and store mResponseHead before extensions are going to change it, and use that copy in nsHttpChannel::AddCacheEntryHeaders or DoAddCacheEntryHeaders. This would only work if there are no significant changes to mResponseHead between http-on-examine-response and storing in the cache. I think that the other approach looks more viable.

(In reply to Honza Bambas (:mayhemer) from comment #4)
(In reply to Rob Wu [:robwu] from comment #3)

redirectTo can also be called while handling http-on-examine-response (aka onHeadersReceived in webRequest). My wild guess is that the cache isn't populated when the request is redirected with redirectTo, possibly due to the special way that redirectTo is handled (with several hooks in the channel implementation).

on-examine is called before InitCacheEntry. So the modifications will be cached.

This does not match my observation when I test it. To rule out any influences from WebRequest.jsm or its async code, I did the following from a privileged console:

if (!lis) {
  var lis = {observe() {}};
  Services.obs.addObserver(lis, "http-on-examine-response")
}
lis.observe = (subject, topic, data) => {
  let channel = subject.QueryInterface(Ci.nsIHttpChannel);
  if (!channel.URI.spec.includes('xxx')) return;
  console.log(`Handling ${channel.URI.spec}`);
  Services.obs.removeObserver(lis, topic);
  lis = null;

  channel.setResponseHeader("Access-Control-Allow-Origin", "*", false);
  channel.redirectTo(Services.io.newURI("https://cors-anywhere.herokuapp.com/example.com"));
};

Start netcat (nc -l 12345). Now from example.com, run fetch("http://localhost:12345/xxx1") and respond with:

HTTP/1.1 200 OK
Cache-Control: max-age=600
ETag: testmytag
Content-Length: 1

x

The http-on-examine-response observer will unregister itself, add CORS headers once and then redirect to a CORS-enabled resource. The request will succeed.
When fetch() is run again, the server will receive the request. This shows that the response wasn't stored in the cache when redirectTo is used.

Comment 6

[Daniel Veditz [:dveditz]]

Does this need to be a hidden security bug? The problems from both sides seem clear, but not easy to reconcile. Meanwhile although it may lead to some extensions doing the wrong thing in some circumstances, it's not in itself exploitable.

Comment 7

[Rob Wu [:robwu]]

This specific part can be a nuisance, but not a true security issue:

[header] modifications from extensions persist even after an extension is uninstalled.

There isn't security-sensitive discussion on this bug, so I don't object to making this bug public.

Comment 8

[Honza Bambas (:mayhemer)]

I think we can open this bug. This is related to bug 1376932 quite a lot. That particular bug is quite confusing for me as only a test for the problem has landed.

(In reply to Rob Wu [:robwu] from comment #5)

on-examine is called before InitCacheEntry. So the modifications will be cached.

This does not match my observation when I test it.

Yes, because we handle the redirectTo sooner (in between). But when redirectTo is not used, we will cache the modified headers.

Comment 9

[Honza Bambas (:mayhemer)]

Dragana, can you please remind me of which variety is exactly for what?

  enum HeaderVariety {
    eVarietyUnknown,
    // Used only for request header.
    eVarietyRequestOverride,
    eVarietyRequestDefault,

    // Used only for response header.
    eVarietyResponseNetOriginalAndResponse, <--
    eVarietyResponseNetOriginal,
    eVarietyResponse
  };

Comment 10

[Honza Bambas (:mayhemer)]

Ah, the comment is just above the enum! Good, now I recall how this works. I assume we internally use eVarietyResponse + eVarietyResponseNetOriginalAndResponse and devtools get eVarietyResponseNetOriginal + eVarietyResponseNetOriginalAndResponse, right?

We can't add eVariertyResponseOverride because it would actually be exactly the same as eVarietyResponse (the naming is kinda confusing).

Comment 11

[Honza Bambas (:mayhemer)]

According priority of this bug, I can see as a P2, because we live with it for a long time.

Other problem is that not only extensions can modify the response headers of the channel. Hence we need to extend the header modification API on the channel to have an explicit distinction if it can or must not be cached - then we could possibly think again about the override (or with some better name given like "DontCache") variety for a header. Extensions would use the dont-cache variant, while internal consumers would use the cache-at-will variant of the API.

Makes sense?

Comment 12

[Shane Caraveo (:mixedpuppy)]

(In reply to Honza Bambas (:mayhemer) from comment #11)

Makes sense?

That seems reasonable.

Comment 13

[Nhi Nguyen (:nhi)]

please see comment #7 and comment #8

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Hi Shane, do you think we still need this for webextensions?

Comment 15

[Shane Caraveo (:mixedpuppy)]

Passing ni? to Rob