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

RESOLVED FIXED in Firefox 59

Status

defect
P2
normal
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: gaubugzilla, Assigned: mixedpuppy)

Tracking

(Blocks 1 bug)

Trunk
mozilla59
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

2 years ago
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.
Assignee

Comment 1

2 years ago
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)
Reporter

Comment 2

2 years ago
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)
Assignee

Comment 3

2 years ago
Posted 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-
Assignee

Comment 5

2 years ago
(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.
Assignee

Updated

2 years ago
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

Updated

2 years ago
Assignee: nobody → mixedpuppy
Flags: needinfo?(gaubugzilla)
Reporter

Comment 8

2 years ago
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)

Comment 9

2 years ago
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.
Assignee

Comment 10

2 years ago
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)
Reporter

Comment 11

2 years ago
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)

Updated

2 years ago
Priority: -- → P2
Assignee

Updated

2 years ago
Attachment #8941287 - Flags: review?(honzab.moz)
Assignee

Updated

2 years ago
Attachment #8929581 - Attachment is obsolete: true

Comment 13

Last year
mozreview-review
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+
Comment hidden (mozreview-request)

Comment 15

Last year
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9fe0fbef78ed
Fix timing of STS header processing for webextensions r=mayhemer

Comment 16

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/9fe0fbef78ed
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

Comment 17

Last year
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)
Reporter

Comment 19

Last year
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)
Reporter

Updated

Last year
Blocks: 1431043

Updated

Last year
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.