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.

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.

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.

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:

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:

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:

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?

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

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