Closed Bug 1418275 Opened 4 years ago Closed 4 years ago

webRequest.onHeadersReceived fires too late to affect HSTS and other mechanisms

Categories

(WebExtensions :: Request Handling, defect, P2)

defect

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: ecfbugzilla, Assigned: mixedpuppy)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

I am once again trying to port Enforce Encryption to Web Extensions. The port does the following in the onHeadersReceived handler:

      let headers = details.responseHeaders || [];
      headers.push({
        name: "Strict-Transport-Security",
        value: "max-age=31536000000"
      });
      return {responseHeaders: headers};

This code works in Chrome, in Firefox (tested in 59.0a1 2017-11-16 nightly) it has no effect however. Looking into the source code, onBeforeReceived is mapped to http-on-examine-response observer notification. This notification is sent out too late however, see https://dxr.mozilla.org/mozilla-central/rev/a3f183201f7f183c263d554bfb15fbf0b0ed2ea4/netwerk/protocol/http/nsHttpChannel.cpp#2365 - STS processing is already done at that point. Headers relevant to caching are also processed earlier, so that extensions have no chance of changing caching behavior either.
I can't tell what you're talking about.

a) there is no onBeforeReceived in webRequest
b) onHeadersReceived happens when headers are received from the server, HSTS happened long before that.

Can you clarify what problem you're having?
Flags: needinfo?(gaubugzilla)
a) I meant onHeadersReceived of course (have it correctly two times out of three ;).
b) Processing of the Strict-Transport-Security header *received* from the server happens right before onHeadersReceived. If you look at my link to nsHttpChannel - it's there, eleven lines above triggering http-on-examine-response.

The purpose of Enforce Encryption extension is enabling HSTS even when the server doesn't - yet adding that header in onHeadersReceived doesn't work (in Firefox, not in Chrome) because the code looking for the Strict-Transport-Security header already executed. So did code deciding whether the request should be cached, but that's not an issue for my extension.
Flags: needinfo?(gaubugzilla)
Attached patch stsAfterExamineResponse (obsolete) — Splinter Review
Ok, looks like we should be able to handle sts after the notification (I cannot see any reason not to).  Honza, do you see anything wrong with this change?
Attachment #8929581 - Flags: feedback?(honzab.moz)
Comment on attachment 8929581 [details] [diff] [review]
stsAfterExamineResponse

Review of attachment 8929581 [details] [diff] [review]:
-----------------------------------------------------------------

Please keep this code inside nsHttpChannel::ProcessResponse and move it just under gHttpHandler->OnExamineResponse.  

looking forward for reports that HSTS and HPKP database was not updated before this notification has happened..  if OnExamineResponse handler opens new channels to the same origin and expects them to upgrade according the just received hsts headers then there will be no update on those new channels - there are code paths doing the upgrade check from inside asyncopen (hence before OnExamineResponse returns and the headers are processed).

hard to say which of the two is a bigger evil but I presume you may also want to remove the headers (not sure why tho)?

At least this change must be very clearly and visibly documented!

I would also like to summarize why you need this.  thanks.
Attachment #8929581 - Flags: feedback?(honzab.moz) → feedback-
(In reply to Honza Bambas (:mayhemer) from comment #4)
> Comment on attachment 8929581 [details] [diff] [review]
> stsAfterExamineResponse
> 
> Review of attachment 8929581 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please keep this code inside nsHttpChannel::ProcessResponse and move it just
> under gHttpHandler->OnExamineResponse.  

I moved it to where it is so that async handling post-OnExamineResponse will work.  Webextensions wont have any benefit or ability to make use of it otherwise.

> looking forward for reports that HSTS and HPKP database was not updated
> before this notification has happened..  if OnExamineResponse handler opens
> new channels to the same origin and expects them to upgrade according the
> just received hsts headers then there will be no update on those new
> channels - there are code paths doing the upgrade check from inside
> asyncopen (hence before OnExamineResponse returns and the headers are
> processed).

Does firefox internals do these things?  If so, maybe we just need a new notification so existing consumers are not affected, but we can get the timing characteristics we prefer for webextensions.

> hard to say which of the two is a bigger evil but I presume you may also
> want to remove the headers (not sure why tho)?

Yes, that would otherwise already be supported.

> At least this change must be very clearly and visibly documented!
> 
> I would also like to summarize why you need this.  thanks.

That is covered in comment 0.  Chrome compatibility and the ability to update STS, the latter being used by ad blockers/privacy addons/etc.
Flags: needinfo?(honzab.moz)
(In reply to Shane Caraveo (:mixedpuppy) from comment #5)
> (In reply to Honza Bambas (:mayhemer) from comment #4)
> > Comment on attachment 8929581 [details] [diff] [review]
> > stsAfterExamineResponse
> > 
> > Review of attachment 8929581 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Please keep this code inside nsHttpChannel::ProcessResponse and move it just
> > under gHttpHandler->OnExamineResponse.  
> 
> I moved it to where it is so that async handling post-OnExamineResponse will
> work.  Webextensions wont have any benefit or ability to make use of it
> otherwise.

Aha, good point.  I will recheck then.

> Does firefox internals do these things?  

No.

> If so, maybe we just need a new
> notification so existing consumers are not affected, but we can get the
> timing characteristics we prefer for webextensions.

Let's leave it for now (process after onexamine) also because adding more and more of these wide-global notifications is a huge perf issue.
Flags: needinfo?(honzab.moz)
Comment on attachment 8929581 [details] [diff] [review]
stsAfterExamineResponse

Review of attachment 8929581 [details] [diff] [review]:
-----------------------------------------------------------------

f+ based on comment 5
Attachment #8929581 - Flags: feedback- → feedback+
Assignee: nobody → mixedpuppy
Flags: needinfo?(gaubugzilla)
I'm not sure what you need my info on. The change looks good to me if that's what you are asking - STS is being processed after http-on-examine-response. No change for caching headers but that isn't something I personally care about.
Flags: needinfo?(gaubugzilla)
IMHO it should be possible to add or remove any and all response headers through extensions before they are processed at all (e.h. STS, Caching, Content Type, Cookies, Content Length, or any others).

I didn't dig into the code to check if caching is the only thing processed beforehand, but it might be worth changing while you're at it.
Sorry Wladimir, I meant to ask you to outline a couple concrete examples that I can turn into tests. 

Caching will be dealt with separately.
Flags: needinfo?(gaubugzilla)
I pushed my current code to the Enforce Encryption repository, you can see the relevant code path under https://github.com/palant/enforceencryption/blob/0864c35b12adf126ee754244419e396ebd4ae8c7/data/background/background.js#L190 - hopefully that's something you can use. This code emits a request to the current tab's URL, ideally one that never hits the network (cache: "force-cache"). It also adds a webRequest.onHeadersReceived listener that adds Strict-Transport-Security: max-age=31536000000 header to the response to this request. Finally, it calls checkStatus() via updateStatus() - this function sends a request to the plain HTTP version of the same URL and uses webRequest.onBeforeSendHeaders to verify that it is automatically rewritten into HTTPS. All of that works correctly in Chrome.
Flags: needinfo?(gaubugzilla)
Priority: -- → P2
Attachment #8941287 - Flags: review?(honzab.moz)
Attachment #8929581 - Attachment is obsolete: true
Comment on attachment 8941287 [details]
Bug 1418275 Fix timing of STS header processing for webextensions

https://reviewboard.mozilla.org/r/211560/#review217554

::: commit-message-e4de6:1
(Diff revision 1)
> +Bug 1418275 Fix timing of security header processing for webextensions

a bit richer description is needed
Attachment #8941287 - Flags: review?(honzab.moz) → review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9fe0fbef78ed
Fix timing of STS header processing for webextensions r=mayhemer
https://hg.mozilla.org/mozilla-central/rev/9fe0fbef78ed
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(mixedpuppy)
This has unit testing.  Would like to hear from Wladimir how it works our for him.
Flags: qe-verify-
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(gaubugzilla)
The bad news is: Enforce Encryption still doesn't work. The reason is that the request in question is cached and processing is different there. If I specify `cache: "no-cache"` for this request then it works. Having this request hit the network is really undesirable however. Guess we need another bug.
Flags: needinfo?(gaubugzilla)
Blocks: 1431043
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.