See bug 1253006. Arthur has a patch in https://trac.torproject.org/projects/tor/ticket/17965, but it inevitably breaks addons. Many addons use nsISiteSecuirtyService::isSecureURI(), but it provides no information about the first party domain. I'm not sure how we can proceed here.

(In reply to Jonathan Hao [:jhao] from comment #0) > See bug 1253006. > > Arthur has a patch in https://trac.torproject.org/projects/tor/ticket/17965, > but it inevitably breaks addons. Many addons use > nsISiteSecuirtyService::isSecureURI(), but it provides no information about > the first party domain. I'm not sure how we can proceed here. It seems like you could address this by just leaving the old API for now, so that the add-ons can keep using it, and adding a new API that does what you want (which actually gets called by gecko-internal stuff). Is this not possible?

Yes, that's possible. One concern is that if container or first party isolation pref is on, some addons code might still access the HSTS cache of the default origin attributes, but using that information to upgrade connections in other origin attributes.

But it's still better than doing nothing. Once we have the new API, I will make a pull request to HTTPS-Everywhere, which is the only addon that affects Tor Browser.

MozReview-Commit-ID: 193BRSxZ7PO

Comment on attachment 8824018 [details] [diff] [review] Isolate the HSTS and HPKP cache by first party domain. (WIP) Review of attachment 8824018 [details] [diff] [review]: ----------------------------------------------------------------- I want to ask for advice, so I uploaded this WIP patch. ::: dom/security/nsMixedContentBlocker.cpp @@ +843,5 @@ > nsCOMPtr<nsISiteSecurityService> sss = > do_GetService(NS_SSSERVICE_CONTRACTID, &rv); > NS_ENSURE_SUCCESS(rv, rv); > rv = sss->IsSecureURI(nsISiteSecurityService::HEADER_HSTS, aContentLocation, > + 0, originAttrs, &cached, &hsts); Hi Christophe. We need to pass origin attributes to IsSecureURI to make HSTS aware of them. In this WIP patch, I get them from the requestPrincipal, but afaik sometimes the request principal doesn't exist or doesn't have the correct origin attributes. How should I get correct origin attributes from the parameter of ShouldLoad()?

(In reply to Jonathan Hao [:jhao] from comment #5) > Hi Christophe. We need to pass origin attributes to IsSecureURI to make > HSTS aware of them. In this WIP patch, I get them from the > requestPrincipal, but afaik sometimes the request principal doesn't exist or > doesn't have the correct origin attributes. How should I get correct origin > attributes from the parameter of ShouldLoad()? The principal you want to use is not the argument aRequestPrincipal, it's the principal that is queried from the requesting context [1]. If I remember correctly aReqeustingPrincipal is null in quite so many cases unless we wanna pass and expandedPrincipal within that argument. Let me know if that does not fix your problem. [1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#604

Try using the principal of the requestingContext. If there is no requestingContext, I guess you could fall back to requestingPrincipal. Though I think you may get inconsistent behavior: * sometimes requesitngPrincipal is null * sometimes it is the triggeringPrincipal, as defined by loadInfo * sometimes it is the loadingPrincipal, as defined by loadInfo (http://searchfox.org/mozilla-central/rev/568e68ade5c6e3d29abba1d1bc25fe87fa1da962/dom/security/nsMixedContentBlocker.cpp#312 - we may actually want to change this code)

Thanks, Christoph and Tanvi. This version can compile but it makes the APIs not scriptable.

Actually, taking a closer look, would the requestingLocation URI have Origin Attributes in its suffix? If so, that looks like the thing to use. It comes from "principal" and if principal doesn't exist, it is set to nsIURI* aRequestingLocation. I don't recall if all URIs have the origin suffix with origin attributes or not.

I realized that if we isolate the HSTS and HPKP storage, we need to provide ForgetAboutSite.jsm a way to clear a domain's storage of all origin attributes. Therefore, we're blocked by bug 1290529. I'll take that bug first. [1] http://searchfox.org/mozilla-central/rev/0f254a30d684796bcc8b6e2a102a0095d25842bb/toolkit/forgetaboutsite/ForgetAboutSite.jsm#206

This one is rebased but hasn't deal with js yet.

Almost done, but it seems to still isolate the storage by userContextId even though I made it not to. I'll need to fix that.

Comment on attachment 8829739 [details] Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (ForgetAboutSite) https://reviewboard.mozilla.org/r/106740/#review108022 ::: security/manager/ssl/tests/unit/test_forget_about_site_security_headers.js:124 (Diff revision 1) > + let originAttributesList = [ > + {}, > + { userContextId: 1}, > + { firstPartyDomain: "foo.com" }, > + { userContextId: 1, firstPartyDomain: "foo.com" }, > + ]; I'm not sure what this array is for. The loops loop over it but never use the contents.

(In reply to Dave Townsend [:mossop] from comment #17) > Comment on attachment 8829739 [details] > Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain > (ForgetAboutSite) > > https://reviewboard.mozilla.org/r/106740/#review108022 > > ::: > security/manager/ssl/tests/unit/test_forget_about_site_security_headers.js: > 124 > (Diff revision 1) > > + let originAttributesList = [ > > + {}, > > + { userContextId: 1}, > > + { firstPartyDomain: "foo.com" }, > > + { userContextId: 1, firstPartyDomain: "foo.com" }, > > + ]; > > I'm not sure what this array is for. The loops loop over it but never use > the contents. Sorry, my bad. The origin attributes were supposed to be passed into processHeader() and isSecureURI().

Comment on attachment 8829736 [details] Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (ForgetAboutSite) https://reviewboard.mozilla.org/r/106734/#review108712 This is impressive, and as far as I can tell is almost entirely correct (the main issue is a few areas where we should have error checks - see below). More generally, though, I'm wondering what you think about first removing some unnecessary methods from nsISiteSecurityService before making these changes? In particular, unsafeProcessHeader isn't necessary if we convert the one gtest that uses it to an xpcshell test. Also, IsSecureHost is redundant when we have IsSecureURI, so maybe we can remove the former? Let me know what you think about that - it would be a non-trivial amount of work, I think. ::: security/manager/ssl/nsISiteSecurityService.idl:110 (Diff revision 2) > * @param aSourceURI the URI of the resource with the HTTP header. > * @param aHeader the HTTP response header specifying security data. > * @param aSSLStatus the SSLStatus of the current channel. > * @param aFlags options for this request as defined in nsISocketProvider: > * NO_PERMANENT_STORAGE > + * @param aOriginAttributes the origin attributes in question. Maybe instead of "in question" we could say something like "... that isolate this origin" or something for all of this documentation (we should also add that we don't actually isolate by userContextID) ::: security/manager/ssl/nsISiteSecurityService.idl:147 (Diff revision 2) > /** > * Same as processHeader but without checking for the security properties > * of the connection. Use ONLY for testing. > */ > + [binaryname(UnsafeProcessHeader)] > + void unsafeProcessHeaderNative( As far as I can tell, unsafeProcessHeader is only used in a gtest that really could be an xpcshell test (in which case we wouldn't need unsafeProcessHeader because we can use FakeSSLStatus). I realize it's a fair bit more work, but I think it would be beneficial to remove unsafeProcessHeader before we make the changes in this bug. ::: security/manager/ssl/nsISiteSecurityService.idl:180 (Diff revision 2) > * @param aFlags options for this request as defined in nsISocketProvider: > * NO_PERMANENT_STORAGE > + * @param aOriginAttributes the origin attributes in question. > */ > + [binaryname(RemoveStateScriptable), implicit_jscontext, optional_argc] > void removeState(in uint32_t aType, removeState isn't currently called from C++ (unless I'm just not finding it), so perhaps we could get away with not defining a native version of it? (or is that not possible?) ::: security/manager/ssl/nsSiteSecurityService.h:50 (Diff revision 2) > /** > * SiteHPKPState: A utility class that encodes/decodes a string describing > * the public key pins of a site. > * HPKP state consists of: > * - Hostname (nsCString) > + * - OriginAttributes (OriginAttributes) nit: the first "OriginAttributes" should probably be "Origin attributes" ::: security/manager/ssl/nsSiteSecurityService.cpp:434 (Diff revision 2) > -SetStorageKey(nsAutoCString& storageKey, const nsACString& hostname, uint32_t aType) > +SetStorageKey(nsAutoCString& storageKey, const nsACString& hostname, > + uint32_t aType, const OriginAttributes& aOriginAttributes) > { > storageKey = hostname; > + > + // Don't isolate by userContextId. We should document this in nsISiteSecurityService.idl. ::: security/manager/ssl/nsSiteSecurityService.cpp:598 (Diff revision 2) > + if (aArgc > 0) { > + // OriginAttributes were passed in. > + if (!aOriginAttributes.isObject()) { > + return NS_ERROR_INVALID_ARG; > + } > + originAttributes.Init(aCx, aOriginAttributes); If I understand correctly, we need to test for and handle failure here (I think all of the *Scriptable functions have this issue). ::: security/manager/ssl/nsSiteSecurityService.cpp:1606 (Diff revision 2) > } > > NS_ENSURE_ARG_POINTER(aResult); > NS_ENSURE_ARG_POINTER(aSha256Pins); > + OriginAttributes originAttributes; > + if (aArgc >= 2) { The other aArgc checks use just '>', so I think we should be consistent here. ::: security/manager/tools/getHSTSPreloadList.js:122 (Diff revision 2) > try { > var uri = Services.io.newURI("https://" + host.name); > var sslStatus = securityInfo.QueryInterface(Ci.nsISSLStatusProvider) > .SSLStatus; > gSSService.processHeader(Ci.nsISiteSecurityService.HEADER_HSTS, > - uri, header, sslStatus, 0, maxAge, > + uri, header, sslStatus, 0, {}, maxAge, Looks like security/manager/ssl/tests/unit/test_sts_ipv4_ipv6.js will need a similar update.

Comment on attachment 8829739 [details] Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (ForgetAboutSite) https://reviewboard.mozilla.org/r/106740/#review108746 LGTM. ::: security/manager/ssl/tests/unit/test_forget_about_site_security_headers.js:115 (Diff revision 2) > "example.org", 0), > "example.org should still be HSTS"); > }); > + > +// Test the case of processing HSTS and HPKP headers for a.pinning2.example.com > +// with multiple originAttributes, using "Forget About Site" on example.com, and Maybe s/multiple/various/

Comment on attachment 8829739 [details] Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (ForgetAboutSite) https://reviewboard.mozilla.org/r/106740/#review109008

Comment on attachment 8829738 [details] Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (Necko) https://reviewboard.mozilla.org/r/106738/#review109330 I think Tanvi should review this, not me. ::: netwerk/protocol/http/HSTSPrimerListener.cpp:231 (Diff revision 2) > - rv = sss->IsSecureURI(nsISiteSecurityService::HEADER_HSTS, uri, 0, &cached, &hsts); > + > + OriginAttributes originAttributes; > + NS_GetOriginAttributes(aRequestChannel, originAttributes); > + > + rv = sss->IsSecureURI(nsISiteSecurityService::HEADER_HSTS, uri, 0, > + originAttributes,&cached, &hsts); nit space after comma

Comment on attachment 8829737 [details] Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (DOM/DocShell) https://reviewboard.mozilla.org/r/106736/#review109836 Ask tanvi or somebody else to review the nsMixedContentBlocker code. ::: dom/security/nsMixedContentBlocker.cpp:837 (Diff revision 2) > nsresult stateRV = securityUI->GetState(&state); > > + OriginAttributes originAttributes; > + if (principal) { > + originAttributes.Inherit(principal->OriginAttributesRef()); > + } else if (aRequestPrincipal) { It seems we should not use aRequestPrincipal here. But I prefer somebody else to review this. I'm not familiar with this code.

Comment on attachment 8829736 [details] Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (ForgetAboutSite) https://reviewboard.mozilla.org/r/106734/#review109948 Looks good, modulo the comments below and what keeler has already mentioned. ::: security/manager/ssl/PublicKeyPinningService.h:34 (Diff revision 2) > */ > static nsresult ChainHasValidPins(const UniqueCERTCertList& certList, > const char* hostname, > mozilla::pkix::Time time, > bool enforceTestMode, > + const OriginAttributes& originAttributes, Please forward declare `OriginAttributes` or include the appropriate header. Same anywhere else this isn't done. ::: security/manager/ssl/nsISiteSecurityService.idl:290 (Diff revision 2) > * @param aSha256Pins array of hashed key fingerprints (SHA-256, base64) > * @param aIsPreload are these key pins for a preload entry? (false by > * default) > + * @param aOriginAttributes the origin attributes in question. > */ > + Nit: Unnecessary blank line. ::: security/manager/ssl/nsSiteSecurityService.cpp:22 (Diff revision 2) > #include "mozilla/Logging.h" > #include "mozilla/Preferences.h" > #include "nsArrayEnumerator.h" > #include "nsCOMArray.h" > #include "nsCRTGlue.h" > +#include "nsIScriptSecurityManager.h" Nit: This files seems to use case-sensitive include order, so this should sort under `nsISSLStatus.h` instead. ::: security/manager/ssl/nsSiteSecurityService.cpp:429 (Diff revision 2) > > return NS_OK; > } > > static void > -SetStorageKey(nsAutoCString& storageKey, const nsACString& hostname, uint32_t aType) > +SetStorageKey(nsAutoCString& storageKey, const nsACString& hostname, If you feel like it, it would be nice to make `storageKey` the last param instead of the first, and mark it as `/*out*/`, to match convention. ::: security/manager/ssl/tests/unit/test_sss_originAttributes.js:47 (Diff revision 2) > +let sslStatus = new FakeSSLStatus(constructCertFromFile( > + "test_pinning_dynamic/a.pinning2.example.com-pinningroot.pem")); > + > +// Check if originAttributes1 and originAttributes2 are isolated with respect > +// to HSTS/HPKP storage. > +function doTest(originAttributes1, originAttributes2, aShouldShare) { Nit: Just `shouldShare` in JS code. ::: security/manager/ssl/tests/unit/test_sss_originAttributes.js:57 (Diff revision 2) > + ok(sss.isSecureURI(type, uri, 0, originAttributes1), > + "Same origin attributes should be set"); > + equal(sss.isSecureURI(type, uri, 0, originAttributes2), aShouldShare, > + "Different origin attributes should be set if and only if " + > + "aShouldShare is true"); Nit: "origin attributes should be set" to me sounds a bit weird for what we're testing here... Maybe something like "URI should be secure given original origin attributes" and "URI should be secure given different origin attributes if and only if [...]"? Same thing elsewhere. ::: security/manager/ssl/tests/unit/test_sss_originAttributes.js:87 (Diff revision 2) > + equal(sss.isSecureHost(Ci.nsISiteSecurityService.HEADER_HPKP, host, 0, > + originAttributes2), > + aShouldShare, "Different origin attributes should not be set"); > + sss.clearAll(); > + ok(!sss.isSecureHost(Ci.nsISiteSecurityService.HEADER_HPKP, host, 0, > + originAttributes1), Nit: Indentation here is off. ::: security/manager/ssl/tests/unit/test_sss_originAttributes.js:88 (Diff revision 2) > + originAttributes2), > + aShouldShare, "Different origin attributes should not be set"); > + sss.clearAll(); > + ok(!sss.isSecureHost(Ci.nsISiteSecurityService.HEADER_HPKP, host, 0, > + originAttributes1), > + "Same origin attributes should cleared"); Nit: Grammar here seems off.

(In reply to Dana Keeler [:keeler] (use needinfo?) from comment #23) > Comment on attachment 8829736 [details] > Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (PSM) > > https://reviewboard.mozilla.org/r/106734/#review108712 > > This is impressive, and as far as I can tell is almost entirely correct (the > main issue is a few areas where we should have error checks - see below). Thanks, although some credits should go to Arthur because some of the patch is actually his work, and it reminds me that I should acknowledge that in the commit message. > More generally, though, I'm wondering what you think about first removing > some unnecessary methods from nsISiteSecurityService before making these > changes? In particular, unsafeProcessHeader isn't necessary if we convert > the one gtest that uses it to an xpcshell test. Also, IsSecureHost is > redundant when we have IsSecureURI, so maybe we can remove the former? Let > me know what you think about that - it would be a non-trivial amount of > work, I think. I could but it would break a few addons. Not much though, 8 for isSecureHost and 2 for unsafeProcessHeader. https://dxr.mozilla.org/addons/search?q=isSecureHost&redirect=false https://dxr.mozilla.org/addons/search?q=unsafeProcessHeader&redirect=false Do you think we should still remove them?

Comment on attachment 8829736 [details] Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (ForgetAboutSite) https://reviewboard.mozilla.org/r/106734/#review110686 As far as I can tell this is the same patch I already reviewed?

(In reply to Jonathan Hao [:jhao] from comment #29) ... > > More generally, though, I'm wondering what you think about first removing > > some unnecessary methods from nsISiteSecurityService before making these > > changes? In particular, unsafeProcessHeader isn't necessary if we convert > > the one gtest that uses it to an xpcshell test. Also, IsSecureHost is > > redundant when we have IsSecureURI, so maybe we can remove the former? Let > > me know what you think about that - it would be a non-trivial amount of > > work, I think. > > I could but it would break a few addons. Not much though, 8 for > isSecureHost and 2 for unsafeProcessHeader. > https://dxr.mozilla.org/addons/search?q=isSecureHost&redirect=false > https://dxr.mozilla.org/addons/search?q=unsafeProcessHeader&redirect=false > > Do you think we should still remove them? The isSecureHost instances can be replaced with isSecureURI. If any add-ons really need unsafeProcessHeader, they can use processHeader with a fake SSL status like our tests do: https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/security/manager/ssl/tests/unit/head_psm.js#668 Since this bug will add some complexity to nsISiteSecurityService/nsSiteSecurityService, I think removing some unnecessary aspects beforehand would be beneficial. (To be clear, though, this should be done in a separate bug.)

Rebased to bug 1336867.

Comment on attachment 8833258 [details] Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (PSM) https://reviewboard.mozilla.org/r/109504/#review113114 It looks like this needs some further work due to Bug 1336867, so I've only skimmed the changes for now. (Also, you might want to figure out how to ensure your workflow creates the "MozReview-Commit-ID:" line in your commit messages - I imagine it would've helped with making sure the interdiff keeps working even with a different commit order.) ::: security/manager/ssl/tests/unit/test_sss_originAttributes.js:79 (Diff revision 2) > + } > + // Set HPKP for originAttributes1. > + sss.setKeyPins(host, false, Date.now() + 1234567890, 2, > + [NON_ISSUED_KEY_HASH, PINNING_ROOT_KEY_HASH], false, > + originAttributes1); > + ok(sss.isSecureHost(Ci.nsISiteSecurityService.HEADER_HPKP, host, 0, Just as a gentle reminder, this needs to be updated to `isSecureURI()` instead. Same elsewhere. ::: security/manager/ssl/tests/unit/test_sss_originAttributes.js:110 (Diff revision 2) > + try { > + action(); > + ok(false, "Should get an error with invalid origin attributes"); > + } catch(e) { > + ok(true, "Should get an error with invalid origin attributes"); > + } Let's use `throws()` instead: ```js throws(action, /NS_ERROR_ILLEGAL_VALUE/, "Should get an error with invalid origin attributes"); ``` ::: security/manager/ssl/tests/unit/test_sss_originAttributes.js:120 (Diff revision 2) > + sss.setKeyPins(host, false, Date.now() + 1234567890, 2, > + [NON_ISSUED_KEY_HASH, PINNING_ROOT_KEY_HASH], false, > + originAttributes1); > + ok(false, "Should get an error with invalid origin attributes"); > + } catch(e) { > + ok(true, "Should get an error with invalid origin attributes"); We can and should use `throws()` here - the try block actually fails here due to a `ReferenceError`, and not because `setKeyPins()` is rejecting invalid input. ::: security/manager/ssl/tests/unit/test_sss_originAttributes.js:122 (Diff revision 2) > + } > + > + try { > + sss.setKeyPins(host, false, Date.now() + 1234567890, 2, > + [NON_ISSUED_KEY_HASH, PINNING_ROOT_KEY_HASH], false, > + originAttributes1); `originAttributes1` isn't defined in this function. I recommend you run `./mach eslint security/` to catch issues like this in the future.

Hi Christoph, could you review those two patches? Tanvi asked me to look for your help. Thanks.

(In reply to :Cykesiopka from comment #42) > Comment on attachment 8833258 [details] > Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (PSM) > > https://reviewboard.mozilla.org/r/109504/#review113114 > > It looks like this needs some further work due to Bug 1336867, so I've only > skimmed the changes for now. > > (Also, you might want to figure out how to ensure your workflow creates the > "MozReview-Commit-ID:" line in your commit messages - I imagine it would've > helped with making sure the interdiff keeps working even with a different > commit order.) You're right. My work flow involves git-mozreview and stgit. It turns out that `stg new` doesn't trigger the hook that mozreview configured to create the MozReview-Commit-ID. Therefore, I forked the stgit repo yesterday and patched it. https://github.com/johnathan79717/stgit/commit/3df569afb1599c4d2130b358848f76a569c51db1 > ::: security/manager/ssl/tests/unit/test_sss_originAttributes.js:122 > (Diff revision 2) > > + } > > + > > + try { > > + sss.setKeyPins(host, false, Date.now() + 1234567890, 2, > > + [NON_ISSUED_KEY_HASH, PINNING_ROOT_KEY_HASH], false, > > + originAttributes1); > > `originAttributes1` isn't defined in this function. > I recommend you run `./mach eslint security/` to catch issues like this in > the future. I keep forgetting to do eslint checks :( And that's why bug 1336867 was backed out. I'll make sure this is done after I change any js files from now on.

(In reply to Jonathan Hao [:jhao] from comment #48) > (In reply to :Cykesiopka from comment #42) > > Comment on attachment 8833258 [details] > > Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (PSM) > > > > https://reviewboard.mozilla.org/r/109504/#review113114 > > > > It looks like this needs some further work due to Bug 1336867, so I've only > > skimmed the changes for now. > > > > (Also, you might want to figure out how to ensure your workflow creates the > > "MozReview-Commit-ID:" line in your commit messages - I imagine it would've > > helped with making sure the interdiff keeps working even with a different > > commit order.) > > You're right. My work flow involves git-mozreview and stgit. It turns out > that `stg new` doesn't trigger the hook that mozreview configured to create > the MozReview-Commit-ID. Therefore, I forked the stgit repo yesterday and > patched it. > https://github.com/johnathan79717/stgit/commit/ > 3df569afb1599c4d2130b358848f76a569c51db1 Here's a more general approach to make `stgit new` trigger commit-msg hook : https://github.com/terinjokes/stgit/compare/master...johnathan79717:0649c45

Comment on attachment 8833258 [details] Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (PSM) https://reviewboard.mozilla.org/r/109504/#review113866 Awesome! r=me with comments addressed. ::: security/manager/ssl/nsISiteSecurityService.idl:111 (Diff revision 3) > * @param aHeader the HTTP response header specifying security data. > * @param aSSLStatus the SSLStatus of the current channel. > * @param aFlags options for this request as defined in nsISocketProvider: > * NO_PERMANENT_STORAGE > + * @param aOriginAttributes the origin attributes that isolate this origin, > + * but we don't isolate by userContextId. I would phrase this more as a paranthetical: "(note that this implementation does not isolate by userContextId, because ...)" and then fill in the reasoning briefly. ::: security/manager/ssl/nsISiteSecurityService.idl:121 (Diff revision 3) > * @return NS_OK if it succeeds > * NS_ERROR_FAILURE if it can't be parsed > * NS_SUCCESS_LOSS_OF_INSIGNIFICANT_DATA > * if there are unrecognized tokens in the header. > */ > + [binaryname(ProcessHeader)] Do we need a noscript tag on this as well? ::: security/manager/ssl/nsISiteSecurityService.idl:179 (Diff revision 3) > + * @param aOriginAttributes the origin attributes that isolate this origin, > + * but we don't isolate by userContextId. > * @param aCached true if we have cached information regarding whether or not > * the host is HSTS, false otherwise. > */ > + [binaryname(IsSecureURI)] Same here - noscript? ::: security/manager/ssl/nsISiteSecurityService.idl:211 (Diff revision 3) > * > * @param aHostname the hostname (punycode) to be queried about > * @param evalTime the time at which the pins should be valid. This is in > mozilla::pkix::Time which uses internally seconds since 0 AD. > + * @param aOriginAttributes the origin attributes that isolate this origin, > + * but we don't isolate by userContextId. Same comment about the documentation here. ::: security/manager/ssl/nsISiteSecurityService.idl:235 (Diff revision 3) > * @param aExpires the time this pin should expire (millis since epoch) > * @param aPinCount number of keys being pinnned > * @param aSha256Pins array of hashed key fingerprints (SHA-256, base64) > * @param aIsPreload are these key pins for a preload entry? (false by > * default) > + * @param aOriginAttributes the origin attributes that isolate this origin, Same documentation comment. ::: security/manager/ssl/nsSiteSecurityService.h:50 (Diff revision 3) > /** > * SiteHPKPState: A utility class that encodes/decodes a string describing > * the public key pins of a site. > * HPKP state consists of: > * - Hostname (nsCString) > + * - OriginAttributes (OriginAttributes) nit: the first "OriginAttributes" should probably be "Origin attributes", since it describes what it is (whereas the second one describes its type) ::: security/manager/ssl/nsSiteSecurityService.cpp:153 (Diff revision 3) > > +NS_IMETHODIMP > +SiteHSTSState::GetOriginAttributes(JSContext* aCx, > + JS::MutableHandle<JS::Value> aOriginAttributes) > +{ > + if (NS_WARN_IF(!ToJSValue(aCx, mOriginAttributes, aOriginAttributes))) { I'm not sure the NS_WARN_IF part is useful here. ::: security/manager/ssl/nsSiteSecurityService.cpp:332 (Diff revision 3) > > +NS_IMETHODIMP > +SiteHPKPState::GetOriginAttributes(JSContext* aCx, > + JS::MutableHandle<JS::Value> aOriginAttributes) > +{ > + if (NS_WARN_IF(!ToJSValue(aCx, mOriginAttributes, aOriginAttributes))) { Same here.

Comment on attachment 8829736 [details] Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (ForgetAboutSite) https://reviewboard.mozilla.org/r/106734/#review113870 r=me with updates as of bug 1336867. ::: security/manager/ssl/tests/unit/test_forget_about_site_security_headers.js:113 (Diff revision 6) > +// HPKP for any originAttributes any longer. Also test that unrelated sites > +// don't also get removed. > +add_task(function* () { > + let originAttributesList = [ > + {}, > + { userContextId: 1}, nit: add a space before the closing '}' ::: security/manager/ssl/tests/unit/test_forget_about_site_security_headers.js:124 (Diff revision 6) > + sslStatus, 0, originAttributes); > + sss.processHeader(Ci.nsISiteSecurityService.HEADER_HPKP, uri, > + GOOD_MAX_AGE + VALID_PIN + BACKUP_PIN, sslStatus, 0, > + originAttributes); > + > + Assert.ok(sss.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS, isSecureURI now, right? :)

Comment on attachment 8833260 [details] Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (Necko) https://reviewboard.mozilla.org/r/109508/#review114058 Hey Jonathan, I am willing to r+ that patch, just wanted to make sure we don't forget to account for SyncAttributesWithPrivateBrowsing, see detailed comments below. ::: netwerk/base/nsNetUtil.cpp:2236 (Diff revision 4) > + OriginAttributes originAttributes; > + if (aLoadInfo) { > + originAttributes = aLoadInfo->GetOriginAttributes(); > + } else if (aChannelResultPrincipal) { > + originAttributes.Inherit(aChannelResultPrincipal->OriginAttributesRef()); > + } In the other cases you call NS_GetOriginAttributes which internally calls |aAttributes.SyncAttributesWithPrivateBrowsing(NS_UsePrivateBrowsing(aChannel));| [1]. Would it make sense to rewrite NS_ShouldSecureUpgrade to take a channel instead of aURI, aLoadInfo, aChannelResultPrincipal, ...? I think you could query all of those arguments within NS_GetOriginAttributes, right? Alternatively, do we have to account for SyncAttributesWithPrivateBrowsing? [1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.cpp#1260 ::: netwerk/protocol/http/nsHttpHandler.cpp:2270 (Diff revision 4) > + if (aPrincipal) { > + originAttributes.Inherit(aPrincipal->OriginAttributesRef()); > + } else if (loadContext) { > + loadContext->GetOriginAttributes(originAttributes); > + originAttributes.StripAttributes(OriginAttributes::STRIP_ADDON_ID); > + } Ah, here we also don't have a channel so we can't call NS_GetOriginAttributes, but I suppose we already have accounted for private browsing once we have the origin attributes within the principal, right?

Comment on attachment 8833259 [details] Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (DOM/DocShell) https://reviewboard.mozilla.org/r/109506/#review114062 That looks good to me, just address my nits please. r=me ::: dom/ipc/ContentParent.h:826 (Diff revision 4) > nsTArray<uint8_t>&& aKeyHandle, > nsTArray<uint8_t>* aSignature) override; > > virtual mozilla::ipc::IPCResult RecvIsSecureURI(const uint32_t& aType, const URIParams& aURI, > - const uint32_t& aFlags, bool* aIsSecureURI) override; > + const uint32_t& aFlags, > + const OriginAttributes& originAttributes, nit: please prefix arguments: aOriginAttributes. ::: dom/ipc/ContentParent.h:832 (Diff revision 4) > + bool* aIsSecureURI) override; > > virtual mozilla::ipc::IPCResult RecvAccumulateMixedContentHSTS(const URIParams& aURI, > const bool& aActive, > - const bool& aHSTSPriming) override; > + const bool& aHSTSPriming, > + const OriginAttributes& originAttributes) override; same nit, please prefix arguments with a ::: dom/ipc/ContentParent.cpp:3351 (Diff revision 4) > > mozilla::ipc::IPCResult > ContentParent::RecvIsSecureURI(const uint32_t& type, > const URIParams& uri, > const uint32_t& flags, > + const OriginAttributes& originAttributes, nit, prefix with a ::: dom/ipc/PContent.ipdl:778 (Diff revision 4) > sync NSSU2FTokenSign(uint8_t[] application, uint8_t[] challenge, > uint8_t[] keyHandle) > returns (uint8_t[] signature); > > - sync IsSecureURI(uint32_t type, URIParams uri, uint32_t flags) > + sync IsSecureURI(uint32_t type, URIParams uri, uint32_t flags, > + OriginAttributes originAttributes) nit: prefix with a and everywhere

Comment on attachment 8833260 [details] Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (Necko) https://reviewboard.mozilla.org/r/109508/#review114058 > In the other cases you call NS_GetOriginAttributes which internally calls > |aAttributes.SyncAttributesWithPrivateBrowsing(NS_UsePrivateBrowsing(aChannel));| [1]. Would it make sense to rewrite NS_ShouldSecureUpgrade to take a channel instead of aURI, aLoadInfo, aChannelResultPrincipal, ...? > I think you could query all of those arguments within NS_GetOriginAttributes, right? > > Alternatively, do we have to account for SyncAttributesWithPrivateBrowsing? > > [1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsNetUtil.cpp#1260 How about getting the origin attributes from the channel and passing them to NS_ShouldSecureUpgrade? > Ah, here we also don't have a channel so we can't call NS_GetOriginAttributes, but I suppose we already have accounted for private browsing once we have the origin attributes within the principal, right? I think you're right. Besides, this chunk of code was already there at line 2308.

Comment on attachment 8833260 [details] Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (Necko) https://reviewboard.mozilla.org/r/109508/#review114456 Passing OriginAttributes explicitly to ShouldSecureUpgrade works for me. Thanks for fixing. r=me

Comment on attachment 8833258 [details] Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (PSM) https://reviewboard.mozilla.org/r/109504/#review114536 Looks good with comments addressed. ::: security/manager/ssl/nsISiteSecurityService.idl:111 (Diff revision 4) > + * (note that this implementation does not isolate > + * by userContextId because of the risk of man-in- > + * the-middle attacks before trust-on-second-use > + * happens. Nit: The closing ) is missing here and below. ::: security/manager/ssl/nsSiteSecurityService.cpp:603 (Diff revision 4) > + } > + } > + return RemoveStateInternal(aType, aURI, aFlags, originAttributes); > +} > + > +NS_IMETHODIMP This should be `nsresult` instead. It would also be nice to move entire method above the other `RemoveStateInternal()`, so it's easier to see what the difference between the two are. ::: security/manager/ssl/nsSiteSecurityService.cpp:1656 (Diff revision 4) > continue; > } > > - nsCString hostname( > + nsCString origin( > StringHead(item.key(), item.key().Length() - keySuffix.Length())); > + nsCString hostname; Nit: I imagine hostnames can typically fit within 64 chars, so let's use `nsAutoCString` instead. ::: security/manager/ssl/tests/unit/test_pinning_header_parsing.js:53 (Diff revision 4) > gSSService.processHeader(Ci.nsISiteSecurityService.HEADER_HPKP, uri, > validPinValue, sslStatus, 0); > } > try { > gSSService.processHeader(Ci.nsISiteSecurityService.HEADER_HPKP, uri, > - pinValue, sslStatus, 0, maxAge); > + pinValue, sslStatus, 0, {}, maxAge); test_sts_parser.js needs similar fixes as well.

Comment on attachment 8833259 [details] Bug 1323644 - Isolate the HSTS and HPKP storage by first party domain (DOM/DocShell) https://reviewboard.mozilla.org/r/109506/#review114850

Pushed by jhao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9f8bb076c706 Isolate the HSTS and HPKP storage by first party domain (PSM) r=Cykesiopka,keeler https://hg.mozilla.org/integration/autoland/rev/1e40c75bc052 Isolate the HSTS and HPKP storage by first party domain (DOM/DocShell) r=baku,ckerschb https://hg.mozilla.org/integration/autoland/rev/821c93716882 Isolate the HSTS and HPKP storage by first party domain (Necko) r=ckerschb https://hg.mozilla.org/integration/autoland/rev/fe6f2e7b9aa1 Isolate the HSTS and HPKP storage by first party domain (ForgetAboutSite) r=keeler,mossop

https://hg.mozilla.org/mozilla-central/rev/9f8bb076c706 https://hg.mozilla.org/mozilla-central/rev/1e40c75bc052 https://hg.mozilla.org/mozilla-central/rev/821c93716882 https://hg.mozilla.org/mozilla-central/rev/fe6f2e7b9aa1

Well done, Jonathan! This is a great progress! Thank you for the effort and thanks for all the reviewers!

(In reply to Ethan Tseng [:ethan] from comment #75) > Well done, Jonathan! This is a great progress! > Thank you for the effort and thanks for all the reviewers! Agreed! Awesome work and much appreciated.

(In reply to Jonathan Hao [:jhao] from comment #3) > But it's still better than doing nothing. Once we have the new API, I will > make a pull request to HTTPS-Everywhere, which is the only addon that > affects Tor Browser. HTTPS Everywhere should not be affected since we do not access the HSTS nor HPKP cache.

(In reply to William Budington from comment #77) > (In reply to Jonathan Hao [:jhao] from comment #3) > > But it's still better than doing nothing. Once we have the new API, I will > > make a pull request to HTTPS-Everywhere, which is the only addon that > > affects Tor Browser. > > HTTPS Everywhere should not be affected since we do not access the HSTS nor > HPKP cache. Does HTTPS Everywhere uses nsSiteSecurityService::IsSecureURI()? It reads the internal HSTS and HPKP cache information.

There is one instance of this in the code, but I'm pretty sure it's a no-op in the context it's used and can be safely removed. I'll do this before next release.

This has been removed as of https://github.com/EFForg/https-everywhere/commit/2daf85c5208b7e8bc988e068e4be66520ab6f07e and will be part of the 5.2.15 release.