This is one of the bugs from Tor Browser that we plan to put in Gecko. https://torpat.ch/13670

The attached WIP patch from the Tor Browser appears to make changes to the NSS library. Hopefully with origin attributes we won't need to make any changes to NSS. Franziskus, will you check this out and see if we're going to be able to do something like what this patch changes? The patch adds isolation to OCSP results caching.

Please note that the (In reply to Dave Huseby [:huseby] from comment #1) > The attached WIP patch from the Tor Browser appears to make changes to the > NSS library. Hopefully with origin attributes we won't need to make any > changes to NSS. Franziskus, will you check this out and see if we're going > to be able to do something like what this patch changes? The patch adds > isolation to OCSP results caching. Please not that this version of the patch is out of date, and the current Tor Browser patch is at https://torpat.ch/13670

(In reply to Dave Huseby [:huseby] from comment #1) > The attached WIP patch from the Tor Browser appears to make changes to the > NSS library. Hopefully with origin attributes we won't need to make any > changes to NSS. Franziskus, will you check this out and see if we're going > to be able to do something like what this patch changes? The patch adds > isolation to OCSP results caching. First note that the patch doesn't apply anymore because bug 1004149 landed (also the new version at https://torpat.ch/13670). To the NSS changes: While I don't see a problem with the changes in general it changes the signature of SEC_HttpRequest_CreateFcn in ocspt.h and thus will be considered API breaking. It should be possible to work around this, but it's a change that we'd probably have to discuss (with one of the module owners).

Not sure what I'm flagged for here, so removing my needinfo. For containers, we will not be separating OCSP responses by user context id. Are OCSP response particularly fingerprintable?

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #3) > it changes the signature of SEC_HttpRequest_CreateFcn in ocspt.h and thus > will be considered API breaking. It should be possible to work around this, > but it's a change that we'd probably have to discuss (with one of the module > owners). No "probably" about it, such changes always require module owner approval anywhere in Gecko. NSS owners are unlikely to allow such a change to an existing interface. They may allow the creation of a new parallel method. We guarantee that applications using the exported APIs will remain compatible with future versions of those libraries. https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/Introduction_to_Network_Security_Services Because of the way Linux systems drop in upgraded "system" versions of NSS that means binary compatibility, not just compatibility if recompiled against new headers.

(In reply to Daniel Veditz [:dveditz] from comment #5) > (In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #3) > No "probably" about it, such changes always require module owner approval > anywhere in Gecko. right, the probably was more for the discussion ;) > They may allow the creation of a new parallel method. That would've been my suggestion. @Dave/Jonathan: let me know when you need any help on moving forward here.

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #6) > (In reply to Daniel Veditz [:dveditz] from comment #5) > > (In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #3) > > No "probably" about it, such changes always require module owner approval > > anywhere in Gecko. > > right, the probably was more for the discussion ;) > > > They may allow the creation of a new parallel method. > > That would've been my suggestion. > @Dave/Jonathan: let me know when you need any help on moving forward here. I thought about making the extra parameter an optional one, so it doesn't break APIs. But this would be odd because originally the last parameter was an out parameter. An alternative is to move the original createFcn code to an internal method that takes isolationKey, and it can be called by the original API method. Franziskus, is this acceptible to you?

(In reply to Tanvi Vyas - unavailable until 9/06 [:tanvi] from comment #4) > Not sure what I'm flagged for here, so removing my needinfo. For > containers, we will not be separating OCSP responses by user context id. Hi Tanvi, I just need a quick confirm before I start. Would it be a problem if we separate OCSP by origin attributes, and consequently, by user context id?

(In reply to Jonathan Hao [:jhao] from comment #7) > I thought about making the extra parameter an optional one, so it doesn't > break APIs. But this would be odd because originally the last parameter was > an out parameter. > > An alternative is to move the original createFcn code to an internal method > that takes isolationKey, and it can be called by the original API method. This sounds like the way to go. Abstract the functionality in an internal function and add a new API. Do you do a patch against NSS?

We discussed this over email, and it sounds like we don't want OCSP isolation for Containers. For First Party Domain, it is still unclear to me if there are benefits to OCSP isolation. Since the OCSP requests are going from the browser to the CA with no credentials, reaching out to the CA more frequently (for each container) just means that we are leaking data to the CA about the sites users are visiting more frequently.

Comment on attachment 8741269 [details] [diff] [review] WIP patch from Tor Browser Review of attachment 8741269 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/socket/nsSOCKSIOLayer.cpp @@ +1388,4 @@ > const char *host, > int32_t port, > nsIProxyInfo *proxy, > + const char *isolationKey, Arthur, is this argument redundant?

And how do you test whether OCSP is isolated?

(In reply to Jonathan Hao [:jhao] from comment #17) > Comment on attachment 8741269 [details] [diff] [review] > WIP patch from Tor Browser > > Review of attachment 8741269 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: netwerk/socket/nsSOCKSIOLayer.cpp > @@ +1388,4 @@ > > const char *host, > > int32_t port, > > nsIProxyInfo *proxy, > > + const char *isolationKey, > > Arthur, is this argument redundant? Yes, it looks to me like the argument was added there by mistake. It should probably be removed in the corresponding header as well. (In reply to Jonathan Hao [:jhao] from comment #18) > And how do you test whether OCSP is isolated? Unfortunately we don't have an automated test. If that is something you are interested in working on, that would be extremely helpful to us. I imagine it should be possible to create an observer for the OCSP requests and check (1) that the channel has the correct associated firstParty and (2) that the OCSP request is repeated for the same third-party https:// address if the first party changes.

(In reply to Arthur Edelstein [:arthuredelstein] from comment #19) > Unfortunately we don't have an automated test. If that is something you are > interested in working on, that would be extremely helpful to us. > > I imagine it should be possible to create an observer for the OCSP requests > and check (1) that the channel has the correct associated firstParty and (2) > that the OCSP request is repeated for the same third-party https:// address > if the first party changes. I guess manual tests should be enough at the moment. Automated tests can wait after we uplift other patches. I thought I'd ran into build failures like in https://trac.torproject.org/projects/tor/ticket/13670#comment:21, but it builds on my linux desktop. Perhaps we don't build libpkix on linux now. I split the patches as in https://trac.torproject.org/projects/tor/ticket/13670#comment:33. I'll quote Arthur here so that these patches will be easier to understand. ========================================================================================================== In case it's useful to understanding this patch, the code path of the isolation key string is as follows: The browser is making an original HTTPS request using an nsHttpChannel. nsHttpChannel::BeginConnect() calls GetFirstPartyIsolationURI and passes it to the nsHttpConnectionInfo constructor ​(1). Soon, nsHalfOpenSocket::SetupStreams, called by HttpConnectionManager, gets the isolation key using nsHttpConnectionInfo->GetIsolationKey() and passes it to a new nsSocketTransport instance ​(2). nsSocketTransport::BuildSocket passes it to nsISocketProvider::NewSocket and nsISocketProvider::AddToSocket ​(3). The three implementations of these latter two methods in turn pass the isolationKey to nsSSLIOLayerNewSocket and nsSSLIOLayerAddToSocket ​(4), which in turn assign the isolationKey to a new TransportSecurityInfoinstance ​(5). The socket now needs to be verified: SSLServerCertVerificationJob AuthCertificate is called, gets the isolationKey from the TransportSecurityInfo instance ​(6). The isolationKey is then passed to CertVerifier::VerifySSLServerCert ​(7), to CertVerifier::VerifyCert ​(8), and then to CertVerifier::MozillaPKIXVerifyCert ​(9), the last of which instantiates an NSSCertDBTrustDomain containing the isolation key ​(10). When NSSCertDBTrustDomain::CheckRevocation is called, it passes the isolationKey to OCSPCache::Get, OCSPCache::Put and OCSPRequestor::DoOCSPRequest() ​(11). Up to this point, all code has simply been passing the isolationKey down from the original http channel to the OCSP code. Now OCSPCache.Get() and OCSPCache.Put() call OCSPCache::CertIDHash, which uses the isolationKey in its generation of the hash. ​(12). The DoOCSPRequest call stores the isolation key in new nsNSSHttpRequestSession instance (in a createFcn call). ​(13) The nsNSSHttpRequestSession instance creates a new nsHttpDownloadEvent, which uses the nsNSSHttpRequestSession's isolation key to set the document URI for a new nsHttpChannel for the OCSP request ​(14). The channel's document URI set to ensure that when GetFirstPartyIsolationURI is later on the channel by torbutton's domain isolator, the same first party URI will be returned as that for the original HTTPS channel ​(16).

Hi, Jonathan -- thanks for taking this on and separating out the components. The pieces look fine to me, although, as they come from my Tor Browser patch, I probably shouldn't be reviewing it. ;)

Comment on attachment 8797527 [details] [diff] [review] Part 1: Add isolation key to socket transport. Review of attachment 8797527 [details] [diff] [review]: ----------------------------------------------------------------- A necko peer should review this.

Comment on attachment 8797528 [details] [diff] [review] Part 2: Pass isolationKey to socket provider. Review of attachment 8797528 [details] [diff] [review]: ----------------------------------------------------------------- Same here.

Comment on attachment 8797527 [details] [diff] [review] Part 1: Add isolation key to socket transport. Hi Honza, would you review these two patches, please?

Unless I'm missing something, this series of patches would enable this by default (i.e. I can't find anything that depends on the value of "privacy.firstparty.isolate"). My concern is this would introduce significant latency in loading 3rd party https resources, and thus I don't think we should enable this by default on vanilla Firefox. I'm also concerned by the lack of tests. Please see the preexisting OCSP tests in https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/

Comment on attachment 8797529 [details] [diff] [review] Part 3: Store the isolationKey in TransportSecurityInfo. Review of attachment 8797529 [details] [diff] [review]: ----------------------------------------------------------------- (I'm clearing this review for now, but I'll post what comments I have so far.) ::: security/manager/ssl/TransportSecurityInfo.cpp @@ +100,5 @@ > > +nsresult > +TransportSecurityInfo::SetIsolationKey(const char* isolationKey) > +{ > + mIsolationKey.Adopt(isolationKey ? NS_strdup(isolationKey) : 0); `mIsolationKey.Assign(isolationKey);` should do what we want here. ::: security/manager/ssl/TransportSecurityInfo.h @@ +102,5 @@ > nsString &result); > > int32_t mPort; > nsXPIDLCString mHostName; > + nsXPIDLCString mIsolationKey; Other than following preexisting convention here, is there a reason to use nsXPIDLCString? I think just nsCString should work.

Comment on attachment 8797531 [details] [diff] [review] Part 5: Double key OCSP cache. Review of attachment 8797531 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/certverifier/OCSPCache.cpp @@ +79,5 @@ > if (rv != SECSuccess) { > return rv; > } > + if (aIsolationKey) { > + rv = PK11_DigestOp(context.get(), (const unsigned char*) aIsolationKey, In PSM, we're using BitwiseCast instead of c-style casts. ::: security/manager/ssl/tests/gtest/OCSPCacheTest.cpp @@ -50,1 @@ > Time time) We should add some tests here that the double-keying does what we expect.

Comment on attachment 8797532 [details] [diff] [review] Part 6: Do OCSP request using isolation key. Review of attachment 8797532 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/nsNSSCallbacks.cpp @@ +110,5 @@ > > chan->SetLoadFlags(nsIRequest::LOAD_ANONYMOUS | > nsIChannel::LOAD_BYPASS_SERVICE_WORKER); > > + // If we have an isolation key, use it as the URI for this channel. I don't understand the purpose of doing this. OCSP requests are already anonymous, so why do we need to do this?

Also, it would be great to use mozreview instead of splinter: https://reviewboard.mozilla.org/

(In reply to David Keeler [:keeler] (use needinfo?) from comment #28) > Comment on attachment 8797532 [details] [diff] [review] > Part 6: Do OCSP request using isolation key. > > Review of attachment 8797532 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: security/manager/ssl/nsNSSCallbacks.cpp > @@ +110,5 @@ > > > > chan->SetLoadFlags(nsIRequest::LOAD_ANONYMOUS | > > nsIChannel::LOAD_BYPASS_SERVICE_WORKER); > > > > + // If we have an isolation key, use it as the URI for this channel. > > I don't understand the purpose of doing this. OCSP requests are already > anonymous, so why do we need to do this? This came from Tor Browser's hacky approach to associating the first party with the channel. This part should be replaced by something that ensures that the OCSP request channel has the correct first party (in origin attributes) associated with it.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #25) > Unless I'm missing something, this series of patches would enable this by > default (i.e. I can't find anything that depends on the value of > "privacy.firstparty.isolate"). It looks like part of the original patch is missing. (Sorry I didn't notice before.) In the Tor Browser patch, https://torpat.ch/13670.2, nsHttpChannel::BeginConnect() is patched to add the first party to the new nsHttpConnectionInfo object. In the new Firefox approach, I believe the first party should be obtained from the nsHttpChannel's origin attributes, instead of using Tor Browser's method. > My concern is this would introduce > significant latency in loading 3rd party https resources, and thus I don't > think we should enable this by default on vanilla Firefox. Yes, when "privacy.firstparty.isolate" is turned off (the default), the firstParty string in the nsHttpChannel's origin attributes will always be empty and thus result in no change from the existing behavior.

Please ensure that this bug isolates OCSP only when firstpartydomain is enabled. Add tests to show that it does not isolate OCSP for containers. Thanks!

(In reply to Arthur Edelstein [:arthuredelstein] from comment #31) > (In reply to David Keeler [:keeler] (use needinfo?) from comment #25) > > Unless I'm missing something, this series of patches would enable this by > > default (i.e. I can't find anything that depends on the value of > > "privacy.firstparty.isolate"). > > It looks like part of the original patch is missing. (Sorry I didn't notice > before.) In the Tor Browser patch, https://torpat.ch/13670.2, > nsHttpChannel::BeginConnect() is patched to add the first party to the new > nsHttpConnectionInfo object. In the new Firefox approach, I believe the > first party should be obtained from the nsHttpChannel's origin attributes, > instead of using Tor Browser's method. We already pass nsHttpChannel's origin attributes to nsHttpConnectionInfo in bug 1283319. > > My concern is this would introduce > > significant latency in loading 3rd party https resources, and thus I don't > > think we should enable this by default on vanilla Firefox. > > Yes, when "privacy.firstparty.isolate" is turned off (the default), the > firstParty string in the nsHttpChannel's origin attributes will always be > empty and thus result in no change from the existing behavior. That's correct. (In reply to David Keeler [:keeler] (use needinfo?) from comment #25) > Unless I'm missing something, this series of patches would enable this by > default (i.e. I can't find anything that depends on the value of > "privacy.firstparty.isolate"). My concern is this would introduce > significant latency in loading 3rd party https resources, and thus I don't > think we should enable this by default on vanilla Firefox. I'm also > concerned by the lack of tests. Please see the preexisting OCSP tests in > https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/ > unit/ Thanks, David. I'll add some tests and use mozreview next time.

Comment on attachment 8797527 [details] [diff] [review] Part 1: Add isolation key to socket transport. Review of attachment 8797527 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsISocketTransport.idl @@ +44,5 @@ > */ > readonly attribute long port; > > /** > + * The isolation key for first party isolation. add a comment how this is used. also it seems that it must be set before the socket transport is built (which generally means before it starts to connect). the setter should IMO throw after that point (or any point the value has already been internally used for something). ::: netwerk/base/nsSocketTransport2.cpp @@ +1133,5 @@ > // on an explicit alternate service host name > const char *host = mOriginHost.get(); > int32_t port = (int32_t) mOriginPort; > nsCOMPtr<nsIProxyInfo> proxyInfo = mProxyInfo; > + const char *isolationKey = mIsolationKey.IsEmpty() ? nullptr : mIsolationKey.get(); if you want to split your patch that much (6 tiny patches is crumbling IMHO ;)) then you should move this line to the part 2 patch. here it looks unused, in part 2 it seems like falling off sky. anyway, a folded (complete) patch would be good to overview easily the whole picture. ::: netwerk/base/nsSocketTransport2.h @@ +302,5 @@ > bool mProxyTransparentResolvesHost; > bool mHttpsProxy; > uint32_t mConnectionFlags; > + > + nsCString mIsolationKey; add a comment what this is, where from it's taken and why it is here ::: netwerk/protocol/http/TunnelUtils.cpp @@ +1593,5 @@ > FWD_TS(SetConnectionFlags, uint32_t); > FWD_TS_PTR(GetRecvBufferSize, uint32_t); > FWD_TS(SetRecvBufferSize, uint32_t); > +FWD_TS(SetIsolationKey, const nsACString&); > +FWD_TS(GetIsolationKey, nsACString&); sounds like this should be in a different patch too.. ::: netwerk/protocol/http/nsHttpConnectionInfo.h @@ +103,5 @@ > } > > const char *Username() const { return mUsername.get(); } > nsProxyInfo *ProxyInfo() const { return mProxyInfo; } > + const char *IsolationKey() const { return NS_ConvertUTF16toUTF8(mOriginAttributes.mFirstPartyDomain).get(); } this is definitely wrong. your are returning reference to a stack or heap memory that is invalid on return from the method. according the code in NeckoOriginAttributes::InheritFromDocShellToNecko mOriginAttributes.mFirstPartyDomain is asciiHost, just converted to utf16 (what a waste!!), so converting here back is OK you simply must return nsCString (honestly, who is using char* these days?)

Comment on attachment 8797528 [details] [diff] [review] Part 2: Pass isolationKey to socket provider. Review of attachment 8797528 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/socket/nsISocketProvider.idl @@ +39,5 @@ > void newSocket(in long aFamily, > in string aHost, > in long aPort, > in nsIProxyInfo aProxy, > + in string aIsolationKey, use AString or (preferably) DOMString, or is there a reason not to? @@ +59,5 @@ > void addToSocket(in long aFamily, > in string aHost, > in long aPort, > in nsIProxyInfo aProxy, > + in string aIsolationKey, here as well

Hi Honza, thanks for your review. I merged those two patches and did mostly what you said in the new mozreview request except I use ACString in nsISocketProvider.idl because DOMString is utf16. Please review again, thanks.

Comment on attachment 8800213 [details] Bug 1264562 - Part 2: Test firstPartyDomain in test_ocsp_caching.js https://reviewboard.mozilla.org/r/85204/#review84598 Looks good (just a few comments). ::: security/manager/ssl/tests/unit/head_psm.js:326 (Diff revision 3) > * @param {Function} aWithSecurityInfo > * A callback function that takes an nsITransportSecurityInfo, which is called > * after the TLS handshake succeeds. > * @param {Function} aAfterStreamOpen > * A callback function that is called with the nsISocketTransport once the > * output stream is ready. nit: add documentation for aIsolationKey ::: security/manager/ssl/tests/unit/test_ocsp_caching.js:229 (Diff revision 3) > + // This test makes sure that OCSP cache are isolation by isolationKey. > + > + // A failure to retrieve an OCSP response will result in an error entry being > + // added to the cache. > + add_ocsp_test("ocsp-stapling-none.example.com", PRErrorCodeSuccess, > + [respondWithError], Rather than responding with an error and caching that, I think a more representative test would be to respond with a good response (which should result in the same behavior). ::: security/manager/ssl/tests/unit/test_ocsp_caching.js:230 (Diff revision 3) > + > + // A failure to retrieve an OCSP response will result in an error entry being > + // added to the cache. > + add_ocsp_test("ocsp-stapling-none.example.com", PRErrorCodeSuccess, > + [respondWithError], > + "No stapled response -> a fetch should have been attempted", Might be good to include the isolation key in the message here (and in the others).

Comment on attachment 8801074 [details] Bug 1264562 - Part 3: Store the firstPartyDomain in TransportSecurityInfo (adapted from Tor Browser patch 13670) https://reviewboard.mozilla.org/r/85882/#review84624 LGTM. ::: security/manager/ssl/TransportSecurityInfo.h:65 (Diff revision 1) > > int32_t GetPort() const { return mPort; } > nsresult GetPort(int32_t *aPort); > nsresult SetPort(int32_t aPort); > > + const char * GetIsolationKeyRaw() const { return mIsolationKey.get(); } nit: for new code, we want the '*' (or '&' as appropriate) to be next to the type, so: "const char* GetIsolationKeyRaw...") (I realize this file has a mix of styles, but still)

Comment on attachment 8801075 [details] Bug 1264562 - Part 4: Instantiates an NSSCertDBTrustDomain containing the first party domain (adapted from Tor Browser patch #13670) https://reviewboard.mozilla.org/r/85884/#review84626 r=me with comments addressed. ::: security/certverifier/CertVerifier.h:106 (Diff revision 1) > CERTCertificate* cert, > SECCertificateUsage usage, > mozilla::pkix::Time time, > void* pinArg, > const char* hostname, > + const char* isolationKey, This should have some documentation. Also, this parameter really seems optional in this case, so let's move it to below the sctsFromTLS parameter (and annotate it similarly, and have it = nullptr) ::: security/certverifier/CertVerifier.cpp:587 (Diff revision 1) > mOCSPCache, pinArg, ocspGETConfig, > mCertShortLifetimeInDays, > mPinningMode, keySizeOptions[i], > ValidityCheckingMode::CheckingOff, > sha1ModeConfigurations[j], > - mNetscapeStepUpPolicy, builtChain, > + mNetscapeStepUpPolicy, isolationKey, builtChain, nit: watch out for lines >80 characters ::: security/certverifier/NSSCertDBTrustDomain.h:187 (Diff revision 1) > CertVerifier::PinningMode mPinningMode; > const unsigned int mMinRSABits; > ValidityCheckingMode mValidityCheckingMode; > CertVerifier::SHA1Mode mSHA1Mode; > NetscapeStepUpPolicy mNetscapeStepUpPolicy; > + nsCString mIsolationKey; Let's just make this a non-owning const char* like mHostname. ::: security/manager/ssl/nsNSSIOLayer.cpp:458 (Diff revision 1) > nullptr, // stapledOCSPResponse > nullptr, // sctsFromTLSExtension > mozilla::pkix::Now(), > nullptr, // pinarg > hostnameFlat.get(), > + nullptr, // isolationKey I had a thought while looking at this - are we certain the network layer won't join connections where the isolation keys are different?

Comment on attachment 8801076 [details] Bug 1264562 - Part 5: Double key OCSP cache with firstPartyDomain (adapted from Tor Browser patch #13670) https://reviewboard.mozilla.org/r/85886/#review84686 This looks good, but the documentation needs to be updated to reflect these changes. I'd like to look at that, so clearing review for now. ::: security/certverifier/NSSCertDBTrustDomain.cpp:697 (Diff revision 1) > rv == Success || > rv == Result::ERROR_REVOKED_CERTIFICATE || > rv == Result::ERROR_OCSP_UNKNOWN_CERT) { > MOZ_LOG(gCertVerifierLog, LogLevel::Debug, > ("NSSCertDBTrustDomain: caching OCSP response")); > - Result putRV = mOCSPCache.Put(certID, rv, thisUpdate, validThrough); > + Result putRV = mOCSPCache.Put(certID, mIsolationKey.get(), rv, thisUpdate, validThrough); nit: long line ::: security/certverifier/OCSPCache.h:62 (Diff revision 1) > // Returns true if the status of the given certificate (issued by the given > // issuer) is in the cache, and false otherwise. > // If it is in the cache, returns by reference the error code of the cached > // status and the time through which the status is considered trustworthy. > bool Get(const mozilla::pkix::CertID& aCertID, > + const char* aIsolationKey, Let's add a bit of documentation on the effect of the isolation key. ::: security/certverifier/OCSPCache.h:103 (Diff revision 1) > > mozilla::pkix::Result mResult; > mozilla::pkix::Time mThisUpdate; > mozilla::pkix::Time mValidThrough; > // The SHA-384 hash of the concatenation of the DER encodings of the > // issuer name and issuer key, followed by the serial number. nit: update comment ::: security/certverifier/OCSPCache.cpp:44 (Diff revision 1) > namespace mozilla { namespace psm { > > // Let derIssuer be the DER encoding of the issuer of aCert. > // Let derPublicKey be the DER encoding of the public key of aIssuerCert. > // Let serialNumber be the bytes of the serial number of aCert. > // The value calculated is SHA384(derIssuer || derPublicKey || serialNumber). nit: update comment ::: security/certverifier/OCSPCache.cpp:149 (Diff revision 1) > } > return false; > } > > static inline void > LogWithCertID(const char* aMessage, const CertID& aCertID) Let's update this to also log the isolation key. ::: security/certverifier/OCSPCache.cpp:186 (Diff revision 1) > MakeMostRecentlyUsed(index, lock); > return true; > } > > Result > -OCSPCache::Put(const CertID& aCertID, Result aResult, > +OCSPCache::Put(const CertID& aCertID, nit: if more than one argument can fit on each line, that would be preferred

Thanks, David. I fixed what you commented in part 5 for you to review again. I'll fixed the nits in part 2 to part 4 later. Also, as I was writing the comments, I thought I should rename all isolationKey to firstPartyDomain. I thought the code will be clearer this way.

Comment on attachment 8801076 [details] Bug 1264562 - Part 5: Double key OCSP cache with firstPartyDomain (adapted from Tor Browser patch #13670) https://reviewboard.mozilla.org/r/85886/#review85294 Honestly I liked "isolationKey" better, since we could in theory use that more broadly (e.g. the OCSP cache isn't separated into private/non-private browsing contexts, so there's potentially a privacy concern there). However, overloading the parameter in that manner might be too clever for our own good, so it's probably not a big deal until (if) we ever decide to do that. In reviewing this again, I realized there was an issue with how this calculated the hashes of the entries. See the large comment below. ::: security/certverifier/NSSCertDBTrustDomain.cpp:697 (Diff revision 2) > rv == Success || > rv == Result::ERROR_REVOKED_CERTIFICATE || > rv == Result::ERROR_OCSP_UNKNOWN_CERT) { > MOZ_LOG(gCertVerifierLog, LogLevel::Debug, > ("NSSCertDBTrustDomain: caching OCSP response")); > - Result putRV = mOCSPCache.Put(certID, rv, thisUpdate, validThrough); > + Result putRV = mOCSPCache.Put(certID, mFirstPartyDomain.get(), rv, thisUpdate, nit: watch out for lines >80 characters like this ::: security/certverifier/OCSPCache.h:107 (Diff revision 2) > > mozilla::pkix::Result mResult; > mozilla::pkix::Time mThisUpdate; > mozilla::pkix::Time mValidThrough; > // The SHA-384 hash of the concatenation of the DER encodings of the > - // issuer name and issuer key, followed by the serial number. > + // issuer name and issuer key, followed by the serial number and the When updating the implementation of the hashing function, update this comment too, please (see below). ::: security/certverifier/OCSPCache.cpp:48 (Diff revision 2) > // Let serialNumber be the bytes of the serial number of aCert. > -// The value calculated is SHA384(derIssuer || derPublicKey || serialNumber). > +// The value calculated is SHA384(derIssuer || derPublicKey || serialNumber || > +// aFirstPartyDomain). > +// The first party domain is only non-empty when "privacy.firstParty.isolate" > +// is enabled, in order to isolate OCSP cache by first party. > // Because the DER encodings include the length of the data encoded, Now that I look at this closer, we've lost the property that there don't exist tuples (derIssuerA, derPublicKeyA, serialNumberA, firstPartyDomainA) and (derIssuerB, derPublicKeyB, serialNumberB, firstPartyDomainB) where the hashes of the concatenations won't collide (without each part being equal). The problem is that the serial number and first party domain parts aren't length-prefixed, so you could find a collision by re-using the issuer and public key and having two (serial, first party domain) pairs like (0x61616161, "aa.com") and (0x6161, "aaaa.com), which would result in the same hash. To fix this, it would probably be easiest to just adopt a simple length-prefixing scheme for these last two fields (you could use DER, but that's more complicated than we need, really). You could do something like pick a maximum length (e.g. 65535), reject inputs longer than that, store the length of each field in two bytes (in network order or something else well-defined), and then have the hash be: SHA384(derIssuer || derPublicKey || 2-byte length of serialNumber || serialNumber || 2-byte length of firstPartyDomain || firstPartyDomain) ::: security/certverifier/OCSPCache.cpp:56 (Diff revision 2) > // each triplet results in the same string of bytes but where each part in A is > // not equal to its counterpart in B. This is important because as a result it > // is computationally infeasible to find collisions that would subvert this > // cache (given that SHA384 is a cryptographically-secure hash function). > static SECStatus > -CertIDHash(SHA384Buffer& buf, const CertID& certID) > +CertIDHash(SHA384Buffer& buf, const CertID& certID, const char* aFirstPartyDomain) nit: I realize this file is all over the place with respect to parameter naming conventions, but let's at least be locally consistent (so for example, this function doesn't use the 'a' prefix) (and don't worry about functions where it's already mixed) ::: security/certverifier/OCSPCache.cpp:154 (Diff revision 2) > > static inline void > -LogWithCertID(const char* aMessage, const CertID& aCertID) > +LogWithCertID(const char* aMessage, const CertID& aCertID, > + const char* aFirstPartyDomain) > { > MOZ_LOG(gCertVerifierLog, LogLevel::Debug, (aMessage, &aCertID)); Need to pass aFirstPartyDomain along to MOZ_LOG as well.

Comment on attachment 8801076 [details] Bug 1264562 - Part 5: Double key OCSP cache with firstPartyDomain (adapted from Tor Browser patch #13670) https://reviewboard.mozilla.org/r/85886/#review85294 > Now that I look at this closer, we've lost the property that there don't exist tuples (derIssuerA, derPublicKeyA, serialNumberA, firstPartyDomainA) and (derIssuerB, derPublicKeyB, serialNumberB, firstPartyDomainB) where the hashes of the concatenations won't collide (without each part being equal). The problem is that the serial number and first party domain parts aren't length-prefixed, so you could find a collision by re-using the issuer and public key and having two (serial, first party domain) pairs like (0x61616161, "aa.com") and (0x6161, "aaaa.com), which would result in the same hash. > > To fix this, it would probably be easiest to just adopt a simple length-prefixing scheme for these last two fields (you could use DER, but that's more complicated than we need, really). You could do something like pick a maximum length (e.g. 65535), reject inputs longer than that, store the length of each field in two bytes (in network order or something else well-defined), and then have the hash be: > > SHA384(derIssuer || derPublicKey || 2-byte length of serialNumber || serialNumber || 2-byte length of firstPartyDomain || firstPartyDomain) Is the 2-byte restriction necessary? In the revision 6, I put the whole int or uint into the hash.

Comment on attachment 8801076 [details] Bug 1264562 - Part 5: Double key OCSP cache with firstPartyDomain (adapted from Tor Browser patch #13670) https://reviewboard.mozilla.org/r/85886/#review85700 The rationale for choosing 2 bytes was that it should be big enough for all inputs this code will actually see and that it is well-defined and type-size-independent (also it's easy to make endian-independent). I was thinking it could be implemented with a 2-byte unsigned char array and some bit shifts/masks. In addition to avoiding implementation-defined behavior, this would be beneficial if/when we persist the OCSP cache to disk (and then potentially sync it across devices). ::: security/certverifier/OCSPCache.cpp:88 (Diff revisions 2 - 3) > } > SECItem certIDSerialNumber = > UnsafeMapInputToSECItem(certID.serialNumber); > + rv = PK11_DigestOp(context.get(), > + BitwiseCast<const unsigned char*>(&certIDSerialNumber.len), > + sizeof certIDSerialNumber.len); nit: sizeof(...)

Comment on attachment 8801077 [details] Bug 1264562 - Part 1: Add firstPartyDomain to socket transport (adapted from Tor Browser patch 13670) https://reviewboard.mozilla.org/r/85202/#review86020 ::: security/certverifier/OCSPCache.h:64 (Diff revision 7) > + // The first party domain is only non-empty when "privacy.firstParty.isolate" > + // is enabled, in order to isolate OCSP cache by first party. > // If it is in the cache, returns by reference the error code of the cached > // status and the time through which the status is considered trustworthy. > bool Get(const mozilla::pkix::CertID& aCertID, > + const char* aFirstPartyDomain, nit: /*out*/ comment? ::: security/certverifier/OCSPCache.h:115 (Diff revision 7) > // See the documentation for CertIDHash in OCSPCache.cpp. > SHA384Buffer mIDHash; > }; > > - bool FindInternal(const mozilla::pkix::CertID& aCertID, /*out*/ size_t& index, > + bool FindInternal(const mozilla::pkix::CertID& aCertID, > + const char* aFirstPartyDomain, again, /*out*/ comment? ::: security/manager/ssl/SSLServerCertVerification.cpp:1338 (Diff revision 7) > sctsFromTLSExtension, time, > infoObject, > infoObject->GetHostNameRaw(), > certList, saveIntermediates, > - flags, &evOidPolicy, > + flags, infoObject-> > + GetFirstPartyDomainRaw(), nit: maybe put infoObject->GetFirstPartyDomainRaw() on a single line?

Comment on attachment 8801077 [details] Bug 1264562 - Part 1: Add firstPartyDomain to socket transport (adapted from Tor Browser patch 13670) https://reviewboard.mozilla.org/r/85888/#review86010 r+ with few comments fixed. ::: netwerk/base/nsISocketTransport.idl:49 (Diff revision 2) > */ > readonly attribute long port; > > /** > + * This is only non-empty when "privacy.firstparty.isolate" is enabled. > + * It is used to create sockets. It must be set before the socket transport "It is used to create sockets" is not a good comment :) ::: netwerk/base/nsSocketTransport2.h:308 (Diff revision 2) > bool mHttpsProxy; > uint32_t mConnectionFlags; > + > + // This is only non-empty when "privacy.firstparty.isolate" is enabled. > + // It is used to create sockets. It must be set before the socket transport > + // is built. Explain in the comment why it must be present in this class. That it's the only way how to carry it down to NSPR layers which are final consumers. ::: netwerk/base/nsSocketTransport2.h:311 (Diff revision 2) > + // This is only non-empty when "privacy.firstparty.isolate" is enabled. > + // It is used to create sockets. It must be set before the socket transport > + // is built. > + nsCString mFirstPartyDomain; > + // To make sure first party domain is set before socket transport is built. > + bool mFirstPartyDomainUsed; you don't need this flag. just check for mFD being null - mFD being non-null means you are after the point setting the first party domain to have any effect, it has alreayd been built and the first party value used. note that you must keep the lock (MutexAutoLock lock(mLock);) when accessing mFD. ::: netwerk/base/nsSocketTransport2.cpp:2406 (Diff revision 2) > +} > + > +NS_IMETHODIMP > +nsSocketTransport::SetFirstPartyDomain(const nsACString &value) > +{ > + MOZ_ASSERT(!mFirstPartyDomainUsed, "setting isolation key after it's used"); assertion is good, but better would be to also throw (return) an error from the method so that consumers in non-debug builds can recognize they do something wrong. also keep in mind my comment on the mFirstPartyDomainUsed member ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp:3085 (Diff revision 2) > } > > socketTransport->SetConnectionFlags(tmpFlags); > > + nsAutoCString firstPartyDomain = > + NS_ConvertUTF16toUTF8(mEnt->mConnInfo->GetOriginAttributes().mFirstPartyDomain); please do this all only when mEnt->mConnInfo->GetOriginAttributes().mFirstPartyDomain is non-empty.

Comment on attachment 8801076 [details] Bug 1264562 - Part 5: Double key OCSP cache with firstPartyDomain (adapted from Tor Browser patch #13670) https://reviewboard.mozilla.org/r/85886/#review86090 Great - r=me. ::: security/certverifier/OCSPCache.cpp:54 (Diff revisions 3 - 4) > + } > + unsigned char array[2]; > + array[0] = length & 255; > + array[1] = (length >> 8) & 255; > + > + return PK11_DigestOp(context.get(), array, 2); nit: let's use MOZ_ARRAY_LENGTH instead of 2

Comment on attachment 8801075 [details] Bug 1264562 - Part 4: Instantiates an NSSCertDBTrustDomain containing the first party domain (adapted from Tor Browser patch #13670) https://reviewboard.mozilla.org/r/85884/#review84626 > I had a thought while looking at this - are we certain the network layer won't join connections where the isolation keys are different? We've made connections aware of origin attributes in bug 1283319, so it shouldn't be a problem.

Pushed by jhao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6ed882bc57d4 Part 1: Add firstPartyDomain to socket transport (adapted from Tor Browser patch 13670) r=mayhemer https://hg.mozilla.org/integration/autoland/rev/bc8904f6dc8d Part 2: Test firstPartyDomain in test_ocsp_caching.js r=keeler https://hg.mozilla.org/integration/autoland/rev/c986e96fe095 Part 3: Store the firstPartyDomain in TransportSecurityInfo (adapted from Tor Browser patch 13670) r=keeler https://hg.mozilla.org/integration/autoland/rev/ae2a34792482 Part 4: Instantiates an NSSCertDBTrustDomain containing the first party domain (adapted from Tor Browser patch #13670) r=keeler https://hg.mozilla.org/integration/autoland/rev/4adb7daf5033 Part 5: Double key OCSP cache with firstPartyDomain (adapted from Tor Browser patch #13670) r=keeler

https://hg.mozilla.org/mozilla-central/rev/6ed882bc57d4 https://hg.mozilla.org/mozilla-central/rev/bc8904f6dc8d https://hg.mozilla.org/mozilla-central/rev/c986e96fe095 https://hg.mozilla.org/mozilla-central/rev/ae2a34792482 https://hg.mozilla.org/mozilla-central/rev/4adb7daf5033

Can we file a followup bug to make this separation work for Origin Attributes in general? So that if some "Origin Attribute A" wants OCSP separation in the future, it is easy to do that?

(In reply to Tanvi Vyas [:tanvi] from comment #78) > Can we file a followup bug to make this separation work for Origin > Attributes in general? So that if some "Origin Attribute A" wants OCSP > separation in the future, it is easy to do that? Jonathan is working on this in bug 1315143.