Opened 3 years ago

Closed 3 months ago

Last modified 3 months ago

#17965 closed defect (fixed)

Isolate HPKP and HSTS to url bar domain

Reported by: mikeperry Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-linkability, TorBrowserTeam201804, ff60-esr-will-have
Cc: gk, arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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).

Child Tickets

Change History (43)

comment:1 Changed 3 years ago by gk

Status: newneeds_information

What is the relationship of this ticket to #6458? 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?

Last edited 3 years ago by gk (previous) (diff)

comment:2 in reply to:  1 ; Changed 3 years ago by gk

Replying to gk:

What is the relationship of this ticket to #6458? 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. :)

Last edited 3 years ago by gk (previous) (diff)

comment:3 Changed 3 years ago by gk

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.

comment:4 in reply to:  2 Changed 3 years ago by gk

Status: needs_informationassigned

Replying to gk:

Replying to gk:

What is the relationship of this ticket to #6458? 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.

comment:5 Changed 3 years ago by arthuredelstein

Keywords: TorBrowserTeam201601R added; TorBrowserTeam201601 removed
Status: assignedneeds_review

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 to address this issue.

comment:6 in reply to:  5 ; Changed 3 years ago by mcs

Status: needs_reviewneeds_revision

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:

  1. Please change the uuid in netwerk/base/nsISiteSecurityService.idl.
  2. Add @param aIsolationKey comments where appropriate in that same file.
  3. Rename isolationKey to aIsolationKey in the declaration for getKeyPinsForHostname() to be consistent with most parameter declarations inside that same idl.
  4. It looks like there is an unnecessary change (line break added) inside nsSiteSecurityService::ProcessPKPHeader().
  5. In the commit message, mention that this fix depends on the fix for #13670.

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:

yield setPref("security.nocertdb", false);

comment:7 in reply to:  6 Changed 3 years ago by arthuredelstein

Replying 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

comment:8 Changed 3 years ago by arthuredelstein

Status: needs_revisionneeds_review

comment:9 Changed 3 years ago by 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.

comment:10 in reply to:  9 Changed 3 years ago by arthuredelstein

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

comment:11 Changed 3 years ago by gk

Status: needs_reviewneeds_revision

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 :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/toolkit/webconsole/network-helper.js :: NH_parseSecurityInfo :: line 621"  data: no]
Stack: NH_parseSecurityInfo@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/toolkit/webconsole/network-helper.js:621:20
NRL_getSecurityInfo@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/toolkit/webconsole/network-monitor.js:222:15
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:13
NRL_onStartRequest@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/toolkit/webconsole/network-monitor.js:207:4
Line: 621, column: 0

We might want to think about a different approach than "just" adding an additional parameter to nsISiteSecurityService methods.

Last edited 3 years ago by gk (previous) (diff)

comment:12 in reply to:  11 Changed 3 years ago by arthuredelstein

Status: needs_revisionneeds_review

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 :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/toolkit/webconsole/network-helper.js :: NH_parseSecurityInfo :: line 621"  data: no]
Stack: NH_parseSecurityInfo@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/toolkit/webconsole/network-helper.js:621:20
NRL_getSecurityInfo@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/toolkit/webconsole/network-monitor.js:222:15
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:13
NRL_onStartRequest@resource://gre/modules/commonjs/toolkit/loader.js -> resource://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?

Last edited 3 years ago by arthuredelstein (previous) (diff)

comment:13 Changed 3 years ago by gk

Keywords: TorBrowserTeam201602R added; TorBrowserTeam201601R removed

Carrying over review tickets.

comment:14 Changed 3 years ago by gk

Keywords: TorBrowserTeam201602 added; TorBrowserTeam201602R removed
Status: needs_reviewneeds_revision

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_revision because 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 no isSecureChannel() 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 nsISiteSecurityService2 containing 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 deprecating nsISiteSecurityService in favor of nsISiteSecurityService2. This would allow us getting rid of nsSIteSecruityService in 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).

comment:15 Changed 3 years ago by gk

Keywords: TorBrowserTeam201603 added; TorBrowserTeam201602 removed

comment:16 Changed 2 years ago by bugzilla

Looks like this is one of the main tickets for first-party isolation effort of Mozilla devs, and it's good to get it upstreamed, while their Target Milestone is mozilla52.

comment:17 Changed 2 years ago by arthuredelstein

Summary: Isolate HPKP pinning to url bar domainIsolate HPKP and HSTS to url bar domain

comment:19 Changed 2 years ago by arthuredelstein

See also discussion in #6458.

comment:20 Changed 17 months ago by gk

Keywords: TorBrowserTeam201704R added; TorBrowserTeam201603 removed
Status: needs_revisionneeds_review

This landed in https://bugzilla.mozilla.org/show_bug.cgi?id=1323644. There are follow-up bugs that are mainly done. Part of the backport is in https://github.com/arthuredelstein/tor-browser/commits/21340+5. A part is still missing (see: comment:21:ticket:21340). Putting this up for review.

comment:21 Changed 17 months ago by gk

I applied the patches to tor-browser-52.1.0esr-7.0-2. Compiling on all three platforms works. Testing and looking at the resulting SiteSecurityServiceState.txt does not show any difference (e.g. domain isolation key or something) to "normal" SiteSecurityServiceState.txt files. Not sure if that is intended or not but that's a thing to keep in mind during a proper review. (My assumption was that there actually should be such a key saved. How else would code running in a new session know which domain those entries were keyed to?)

comment:22 Changed 17 months ago by gk

Keywords: TorBrowserTeam201705R added; TorBrowserTeam201704R removed

Moving review tickets to May.

comment:23 Changed 16 months ago by gk

Keywords: TorBrowserTeam201706R added; TorBrowserTeam201705R removed

Moving review tickets to 201706

comment:24 Changed 15 months ago by gk

Keywords: TorBrowserTeam201707 added

Moving Tickets to July 2017.

comment:25 Changed 15 months ago by gk

Keywords: TorBrowserTeam201707R added; TorBrowserTeam201706R removed

Moving the review tickets to July 2017 as well.

comment:26 Changed 15 months ago by gk

Keywords: TorBrowserTeam201707 removed

comment:27 Changed 14 months ago by gk

Keywords: TorBrowserTeam201707 added; TorBrowserTeam201707R removed
Status: needs_reviewneeds_revision

Okay, I spent quite some time on the review today. We need to rebase at least taking https://bugzilla.mozilla.org/show_bug.cgi?id=1350599 into account it seems (given the patches in 21340+5). Arthur could you do that, so that we get a clean branch I can look at again? FWIW: https://bugzilla.mozilla.org/show_bug.cgi?id=1345612 might be relevant as well.

comment:28 Changed 14 months ago by gk

Keywords: TorBrowserTeam201708 added; TorBrowserTeam201707 removed

Moving our Tickets to August.

comment:29 Changed 13 months ago by gk

Keywords: TorBrowserTeam201709 added; TorBrowserTeam201708 removed

Items for September 2017.

comment:30 Changed 12 months ago by gk

Keywords: TorBrowserTeam201710 added; TorBrowserTeam201709 removed

Moving to October.

comment:31 Changed 11 months ago by gk

Keywords: TorBrowserTeam201711 added; TorBrowserTeam201710 removed

Moving tickets over to November.

comment:32 Changed 10 months ago by gk

Moving tickets to December 2017

comment:33 Changed 10 months ago by gk

Keywords: TorBrowserTeam201712 added; TorBrowserTeam201711 removed

Moving tickets to December 2017, for realz.

comment:34 Changed 8 months ago by gk

Keywords: TorBrowserTeam201801 added; TorBrowserTeam201712 removed

Moving tickets to 2018.

comment:35 Changed 8 months ago by gk

Keywords: TorBrowserTeam201802 added; TorBrowserTeam201801 removed

Moving tickets to Feb

comment:36 Changed 7 months ago by gk

Keywords: TorBrowserTeam201803 added; TorBrowserTeam201802 removed

Adding to our March plate.

comment:37 Changed 6 months ago by gk

FWIW: Apple claims tracking via HSTS is seen in the wild and they have implemented an own defense: https://webkit.org/blog/8146/protecting-against-hsts-abuse/

comment:38 Changed 6 months ago by cypherpunks

Why does improving Tor Browser have a lower prio than upstreaming?

comment:39 Changed 5 months ago by gk

Keywords: TorBrowserTeam201804 added; TorBrowserTeam201803 removed

Moving our tickets to April.

comment:40 Changed 5 months ago by gk

Priority: HighMedium

comment:41 Changed 4 months ago by gk

Keywords: ff60-esr-will-have added

comment:42 Changed 3 months ago by gk

Resolution: fixed
Status: needs_revisionclosed

That's available starting with Tor Browser 8.0a9.

comment:43 Changed 3 months ago by cypherpunks

Yay!

Note: See TracTickets for help on using tickets.