Isolate HPKP and HSTS to url bar domain
HPKP pinning (where an HTTP header can list a key to pin) may enable third party tracking if an adversary creates multiple certificates for many domains.
HPKP is already memory-only. In normal Firefox, it is saved to disk in the same location as HSTS is.
We should isolate HPKP to the url bar domain, and verify that it and HSTS are cleared on New Identity (I believe they are).
Activity
What is the relationship of this ticket to #6458 (moved)? I thought we should deal with both issues in the latter (see my comment:11:ticket:6458) Is there a reason you want to split HPKP off?
Trac:
Status: new to needs_information-
Replying to gk:
What is the relationship of this ticket to #6458 (moved)? I thought we should deal with both issues in the latter (see my comment:11:ticket:6458) Is there a reason you want to split HPKP off?
Oh, I am asking here as HSTS seems to creep into your description of this ticket. :)
-
And, for the record, see https://zyan.scripts.mit.edu/presentations/toorcon2015.pdf slides 21ff. where an attack scenario is described in more detail that should be moot with binding HPKP state to the URL bar domain.
-
Replying to gk:
Replying to gk:
What is the relationship of this ticket to #6458 (moved)? I thought we should deal with both issues in the latter (see my comment:11:ticket:6458) Is there a reason you want to split HPKP off?
Oh, I am asking here as HSTS seems to creep into your description of this ticket. :)
(Answering my question(s) myself): After thinking a bit more about it it seems reasonable to not deal with both features in the same ticket as they (and the linkability attacks allowed by them) are different enough and unrelated.
Trac:
Status: needs_information to assigned Here is a branch that isolates both HSTS and HPKP.
https://github.com/arthuredelstein/tor-browser/commits/17965+1
The same mechanism is used to store both HSTS and HPKP state, so I isolate both HSTS and HPKP in the first patch. Note that I left out isolation for SpeculativeConnect for now, because we have it disabled, and otherwise the patch would be substantially larger and more complicated.
The second patch in this branch provides a regression test for HSTS isolation. I still need to write a regression test for HPKP isolation.
Unfortunately, I discovered that mochitests fail to load https sites when our "security.nocertdb" pref is enabled. So to run this test, one needs to temporarily set that pref to false in
browser/app/profile/000-tor-browser.js. I opened a #18087 (moved) to address this issue.Trac:
Status: assigned to needs_review
Keywords: TorBrowserTeam201601 deleted, TorBrowserTeam201601R added
Replying to arthuredelstein:
Here is a branch that isolates both HSTS and HPKP.
https://github.com/arthuredelstein/tor-browser/commits/17965+1
The same mechanism is used to store both HSTS and HPKP state, so I isolate both HSTS and HPKP in the first patch. Note that I left out isolation for SpeculativeConnect for now, because we have it disabled, and otherwise the patch would be substantially larger and more complicated.
Kathy and I reviewed the patch. It is unfortunate that so many places need to be touched in order to pass the isolationKey through, but we do not have a better idea. We have a few minor comments:
- Please change the uuid in netwerk/base/nsISiteSecurityService.idl.
- Add @param aIsolationKey comments where appropriate in that same file.
- Rename isolationKey to aIsolationKey in the declaration for getKeyPinsForHostname() to be consistent with most parameter declarations inside that same idl.
- It looks like there is an unnecessary change (line break added) inside nsSiteSecurityService::ProcessPKPHeader().
- In the commit message, mention that this fix depends on the fix for #13670 (moved).
The second patch in this branch provides a regression test for HSTS isolation. I still need to write a regression test for HPKP isolation.
This looks OK. Please remove this line from bug17965_tab.html: console.log("hi"); Please add a comment to this line in test_tor_bug17965.html that mentions that it is needed due to #18087 (moved): yield setPref("security.nocertdb", false);
Trac:
Status: needs_review to needs_revisionReplying to mcs:
Kathy and I reviewed the patch.
Thank you for the review! I have taken all your suggestions. Here is the new version of the branch (two patches):
https://github.com/arthuredelstein/tor-browser/commits/17965+2
Replying to mcs:
The revised changes look OK. Our only nit is that you missed adding the @param comment for several method declarations in the idl file.
Thanks for noticing. Here's the new version with this fixed (two patches):
https://github.com/arthuredelstein/tor-browser/commits/17965+3
-
I did not look much at the patch yet but decided to try some test bundles with it. It breaks at least HTTPS-E and it seems in a way that sites like facebook.com are not working anymore. In the error console I get:
NS_ERROR_XPC_NOT_ENOUGH_ARGS: Not enough arguments [nsISiteSecurityService.isSecureURI] HTTPS.js:43:0Without HTTPS-E it is loading but still there are issues visible:
Handler function NRL_getSecurityInfo threw an exception: [Exception... "Not enough arguments [nsISiteSecurityService.isSecureHost]" nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)" location: "JS frame :: re[/gre/modules/commonjs/toolkit/loader.js](/gre/modules/commonjs/toolkit/loader.js) -> re[/gre/modules/devtools/toolkit/webconsole/network-helper.js](/gre/modules/devtools/toolkit/webconsole/network-helper.js) :: NH_parseSecurityInfo :: line 621" data: no] Stack: NH_parseSecurityInfo@re[/gre/modules/commonjs/toolkit/loader.js](/gre/modules/commonjs/toolkit/loader.js) -> re[/gre/modules/devtools/toolkit/webconsole/network-helper.js:621:20](/gre/modules/devtools/toolkit/webconsole/network-helper.js:621:20) NRL_getSecurityInfo@re[/gre/modules/commonjs/toolkit/loader.js](/gre/modules/commonjs/toolkit/loader.js) -> re[/gre/modules/devtools/toolkit/webconsole/network-monitor.js:222:15](/gre/modules/devtools/toolkit/webconsole/network-monitor.js:222:15) makeInfallible/<@re[/gre/modules/commonjs/toolkit/loader.js](/gre/modules/commonjs/toolkit/loader.js) -> re[/gre/modules/devtools/DevToolsUtils.js:82:13](/gre/modules/devtools/DevToolsUtils.js:82:13) NRL_onStartRequest@re[/gre/modules/commonjs/toolkit/loader.js](/gre/modules/commonjs/toolkit/loader.js) -> re[/gre/modules/devtools/toolkit/webconsole/network-monitor.js:207:4](/gre/modules/devtools/toolkit/webconsole/network-monitor.js:207:4) Line: 621, column: 0We might want to think about a different approach than "just" adding an additional parameter to nsISiteSecurityService methods.
Trac:
Status: needs_review to needs_revision Replying to gk:
I did not look much at the patch yet but decided to try some test bundles with it. It breaks at least HTTPS-E and it seems in a way that sites like facebook.com are not working anymore. In the error console I get: {{{ NS_ERROR_XPC_NOT_ENOUGH_ARGS: Not enough arguments [nsISiteSecurityService.isSecureURI] HTTPS.js:43:0 }}} Without HTTPS-E it is loading but still there are issues visible: {{{ Handler function NRL_getSecurityInfo threw an exception: [Exception... "Not enough arguments [nsISiteSecurityService.isSecureHost]" nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)" location: "JS frame :: re/gre/modules/commonjs/toolkit/loader.js -> re/gre/modules/devtools/toolkit/webconsole/network-helper.js :: NH_parseSecurityInfo :: line 621" data: no] Stack: NH_parseSecurityInfo@re/gre/modules/commonjs/toolkit/loader.js -> re/gre/modules/devtools/toolkit/webconsole/network-helper.js:621:20 NRL_getSecurityInfo@re/gre/modules/commonjs/toolkit/loader.js -> re/gre/modules/devtools/toolkit/webconsole/network-monitor.js:222:15 makeInfallible/<@re/gre/modules/commonjs/toolkit/loader.js -> re/gre/modules/devtools/DevToolsUtils.js:82:13 NRL_onStartRequest@re/gre/modules/commonjs/toolkit/loader.js -> re/gre/modules/devtools/toolkit/webconsole/network-monitor.js:207:4 Line: 621, column: 0 }}}
Thanks for catching these issues. I've attempted to fix these problems with nsISiteSecurityService and also tried to fix all the unit tests that are broken by these changes. Here is the new tor-browser branch (three patches): https://github.com/arthuredelstein/tor-browser/commits/17965+4 and here is a patch for https-everywhere to handle the changes in nsISiteSecurityService: https://github.com/arthuredelstein/https-everywhere/commit/17965
We might want to think about a different approach than "just" adding an additional parameter to nsISiteSecurityService methods.
I think the problem is that the existing nsISiteSecurityService methods assume that there is a simple mapping of URIs to HSTS and HPKP state, regardless of first-party host. So I don't see any way to avoid modifying those methods, unfortunately. Do you have another approach in mind?
Trac:
Status: needs_revision to needs_review-
Do we have any numbers on how many extensions are actually using the methods in nsSiteSecurityService? I fear there are a bunch and it seems that already one is enough to make the whole Tor Browser unusable.
This is in
needs_revisionbecause I think the approach does not work, especially if we think about upstreaming that patch (apart from the fact that the HTTPS-E patch is either wrong because HTTPS-E is used to a great deal outside of the Tor Browser context, too (and there is noisSecureChannel()available) or not sufficient as we would need to patch HTTPS-E for us during the bundling step).So, what about this: we introduce an
nsISiteSecurityService2containing the changes we want and then we make sure that callers from a non-chrome context + chrome context we control (i.e. browser chrome) are using that. That would leave the extensions unbroken. I guess given the things extensions can already do and that we need to trust them anyway the HSTS/HPKP bits do not matter much for now. This idea would probably make it easier for us to get our patch upstreamed as nothing existing would break + it would outline a proper way forward: Mozilla could start deprecatingnsISiteSecurityServicein favor ofnsISiteSecurityService2. This would allow us getting rid ofnsSIteSecruityServicein extensions as well eventually.Another thing we could do is try to to talk to some Mozilla devs about whether they know a better solution that they would merge (instead).
Trac:
Keywords: TorBrowserTeam201602R deleted, TorBrowserTeam201602 added
Status: needs_review to needs_revision See also discussion in #6458 (moved).