1431521 - no HSTS processing happens for cached http requests

Status: NEW

(Core :: Networking: HTTP, enhancement, P2)

Description

[Shane Caraveo (:mixedpuppy)]

Extensions that enforce secure connections are unable to do so without hitting the network (making a priming call) because HSTS has no effect on cached requests.  See bug 1431043 for a webextension example.  

There are probably two parts to this.  One is figuring out having HSTS work with the cache, the second (bug 1431043) is ensuring the timing of this happens appropriately so webextensions can make use of it.

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

@mayhemer can you take a look at it?

Comment 2

[Honza Bambas (:mayhemer)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #0)
> Extensions that enforce secure connections are unable to do so without
> hitting the network (making a priming call) 

priming has been entirely removed from the code base in bug 1424917, few days ago.

> because HSTS has no effect on
> cached requests.  

I think I first need to understand what you mean with "because HSTS has no effect on cached requests."


> See bug 1431043 for a webextension example.  
> 
> There are probably two parts to this.  One is figuring out having HSTS work
> with the cache, the second (bug 1431043) is ensuring the timing of this
> happens appropriately so webextensions can make use of it.

as I understand (maybe wrong), the problem here is that TTL of the network HSTS response header data is shorter than freshness lifetime of a cache entry with HSTS response headers we are trying to load here, or not?

if HSTS has expired, but the cache entry is still fresh enough to load from the http cache, then we can't just re-set the HSTS database as if we have just loaded the content again from the network.  that is a nonsense and actually a security issue as well as not conforming rfc6797.

I think what we should rather do is to revalidate the cached entry with the server again and check if HSTS is still applied.  in other words, crop the freshness lifetime of a cache entry to correlate with HSTS ttl for the given domain.

Comment 3

[Wladimir Palant]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> priming has been entirely removed from the code base in bug 1424917, few
> days ago.

This was ambiguous in the description, the bug isn't about HSTS Priming at all. It's rather about extensions such as my Enforce Encryption that want to enable HSTS for specific websites. The only way for extensions to do this is triggering a request and adding HSTS headers to the response. Obviously, that request would ideally be served from the cache, so that side-effects are avoided. That works fine with Chrome, but Firefox won't consider HSTS headers on cached responses.

Comment 4

[Honza Bambas (:mayhemer)]

Hmm.. then I don't think this should do anything with HTTP cache.  We should rather force processing of the synthetically added response headers as if they were coming from the network.  Either just the modified or all the response headers merged.

This is more a 'synthetization' thing than a cache processing thing.

Ben, Shane, what do you think?

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I don't really know anything about this, unfortunately.  The webextension modification of headers is not really related to service worker response synthesis.

Comment 6

[Shane Caraveo (:mixedpuppy)]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #0)
> > Extensions that enforce secure connections are unable to do so without
> > hitting the network (making a priming call) 
> 
> priming has been entirely removed from the code base in bug 1424917, few
> days ago.

But the sts headers are still respected, correct?  By making a priming call I'm not talking about the priming code in firefox, but rather that the extension has to make a request, it then modifies the headers during http-on-examine-response (onHeadersReceived in WebRequest).

> I think I first need to understand what you mean with "because HSTS has no
> effect on cached requests."

I think it is a timing related issue just like bug 1418275, but I'm pretty uncomfortable touching the cache code in httpchannel. Essentially IIUC, sts headers set during http-on-examine-response should work whether the response is from the network or from the cache.  Right now they only work on network requests, cache handling still happens prior to http-on-examine-response.

Comment 8

[Shane Caraveo (:mixedpuppy)]

ISTM that something similar to the cached header merging on a 206[1] could be done after http-on-examine-response, is that a possible solution?

[1] https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#3307-3318

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #8)

Please use perma links, always.

We miss a notification when we are about to start loading a cached response that is used w/o any origin-server re-validation (fresh enough), so no 304 or 206 response triggering the "merged" notification.

That would need to be relatively carefully designed to be added.  I can add it as a workweek topic we are about to have soon.

Comment 10

[Honza Bambas (:mayhemer)]

So, looking more into the code reveals we already have http-on-examine-cached-response callback.  But it's called in an asynchronous way and it's not handling any modifications to the response made by the callback:

https://searchfox.org/mozilla-central/rev/8837610b6c999451435695e800f38d4acbc0a644/netwerk/protocol/http/nsHttpChannel.cpp#742

https://searchfox.org/mozilla-central/rev/8837610b6c999451435695e800f38d4acbc0a644/netwerk/protocol/http/nsHttpChannel.cpp#8502-8506

According https://bugzilla.mozilla.org/show_bug.cgi?id=449198#c9 - c11 it seems like Boris wanted to make this async.  These days it's unlikely this is about to be called from asyncOpen directly.  Hence I believe we can make it sync and add processing of headers as after network response examination callback execution.

Boris, any objections?  

The major change would be to call http-on-examine-cached-response even when ReadFromCache later fails.  If ContinueConnect() is called synchronously from inside AsyncOpen (highly unlikely these days, I don't even know if there is a possible code path for it) and ReadFromCache fails, there will be no  OnStartRequest/OnStopRequest with the error, just asyncOpen will throw.

Comment 11

[Shane Caraveo (:mixedpuppy)]

When you say "Hence I believe we can make it sync" what exactly does that mean?  Calls into webextensions have be be handled asynchronously.

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #11)
> When you say "Hence I believe we can make it sync" what exactly does that
> mean?  Calls into webextensions have be be handled asynchronously.

Good point.  

What I mean is the following, right now the code works like this:

(we are on the main thread)
- dispatch an event to the main thread to trigger the "http-on-examine-cached-response" notification asynchronously
- tell the cache to start reading the cached content
- if we successfully instructed the cache to read, the following happens, each as a separate async event:
  - call "http-on-examine-cached-response" observer, no response header processing
  - get OnStartRequest
  - get OnDataAvailable (possibly multiple times)
  - get OnStopRequest
- if instructing the cache to read failed, we revoke the event to trigger "http-on-examine-cached-response" and exit


And I wanted to change it to something like this:
(all synchronously, during one main thread event loop)
- call "http-on-examine-cached-response" directly
- process any added headers (STS, probably cookies, etc, subject to discussion)
- instruct the cache to read the cached content


If you say "Calls into webextensions have be be handled asynchronously." does it mean to handle suspend as in bug 1321528 and bug 1418275, or something else?

Comment 13

[Shane Caraveo (:mixedpuppy)]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #11)

> If you say "Calls into webextensions have be be handled asynchronously."
> does it mean to handle suspend as in bug 1321528 and bug 1418275, or
> something else?

Yes, like bug 1321528 and bug 1345893.  WebRequest.jsm will suspend the channel prior to waiting for an async result from the extension code.  So httpchannel needs to wait on suspendcount for any observer notification.  IIRC There are a couple that still do not, bug 1407384, such as one in DoAuthRetry.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Boris, any objections?  

Basically, I want http-on-examine-response and http-on-examine-cached-response to have the same (or as close as possible) behavior from the point of view of the necko API consumer.  That's all I was after in bug 449198.

Comment 15

[Honza Bambas (:mayhemer)]

Discussed during a necko work week.  we can do an extra processing of HSTS headers in HttpBaseChannel::SetResponseHeader.  that would nicely fulfill the requirement for you and not break anything else (hopefully.)

Comment 16

[Kershaw Chang [:kershaw]]

Hi Shane,

I am not sure I fully understand this problem. Could you provide me a webextension sample (or a xpcshell test) that shows this issue?
So I can try to debug myself and figure out a solution for this.

Thanks.

Comment 17

[Shane Caraveo (:mixedpuppy)]

I'm declaring need-info bankruptcy and resetting old ni? on me. If input is still needed on this issue and it is engineering specific for the webextensions team, ni? lgreco, or ni? me again.