[tbb-bugs] #17965 [Tor Browser]: Isolate HPKP pinning to url bar domain

[Tor Bug Tracker & Wiki]

#17965: Isolate HPKP pinning to url bar domain
-------------------------------------------------+-------------------------
 Reporter:  mikeperry                            |          Owner:  tbb-
     Type:  defect                               |  team
 Priority:  High                                 |         Status:
Component:  Tor Browser                          |  needs_review
 Severity:  Normal                               |      Milestone:
 Keywords:  tbb-linkability,                     |        Version:
  TorBrowserTeam201601R                          |     Resolution:
Parent ID:                                       |  Actual Points:
  Sponsor:                                       |         Points:
-------------------------------------------------+-------------------------
Changes (by arthuredelstein):

 * status:  needs_revision => needs_review


Comment:

 Replying to [comment:11 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 at resource://gre/modules/commonjs/toolkit/loader.js ->
 resource://gre/modules/devtools/toolkit/webconsole/network-
 helper.js:621:20
 > NRL_getSecurityInfo at 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 at 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 URI. So I don't see any way to avoid modifying
 those methods, unfortunately. Do you have another approach in mind?

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/17965#comment:12>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online