1315143 - Make OCSP use Origin Attribute framework

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P3)

Description

[Jonathan Hao (inactive) [:jhao]]

In bug 1264562 and bug 1312794, we isolated OCSP cache and requests by first party domain only because we didn't want to isolate them by containers.  However, we should still make them use origin attribute framework in case we need to isolated them by other attributes in the future.

Comment 1

[Jonathan Hao (inactive) [:jhao]]

Attachment: Bug 1315143 - Make OCSP use Origin Attribute framework (PSM).

Review commit: https://reviewboard.mozilla.org/r/91662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91662/

Comment 2

[Jonathan Hao (inactive) [:jhao]]

Attachment: Bug 1315143 - Make OCSP use Origin Attribute framework (Necko).

Review commit: https://reviewboard.mozilla.org/r/91664/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91664/

Comment 3

[Jonathan Hao (inactive) [:jhao]]

Hi Honza and David, could you help review these patches?  I replace the first party domain with origin attributes, but still use only first party domain to make hashes.

I also added a small test in OCSPCacheTest.cpp to ensure that OCSP cache is not isolated by containers.

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 8808956 [details]
Bug 1315143 - Make OCSP use Origin Attribute framework (Necko).

https://reviewboard.mozilla.org/r/91664/#review91634

::: netwerk/base/nsSocketTransport2.cpp:2410
(Diff revision 1)
> +nsSocketTransport::SetScriptableOriginAttributes(JSContext* aCx,
> +    JS::Handle<JS::Value> aOriginAttributes)
> +{
> +    MutexAutoLock lock(mLock);
> +    NS_ENSURE_FALSE(mFD.IsInitialized(), NS_ERROR_FAILURE);
> +    

ws

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8808955 [details]
Bug 1315143 - Make OCSP use Origin Attribute framework (PSM).

https://reviewboard.mozilla.org/r/91662/#review91782

This looks good in general, although there are a few minor nits to address. I'd also like Cykesiopka to have a quick look (particularly for #include issues and namespace style concerns).

::: security/certverifier/NSSCertDBTrustDomain.h:187
(Diff revision 1)
>    CertVerifier::PinningMode mPinningMode;
>    const unsigned int mMinRSABits;
>    ValidityCheckingMode mValidityCheckingMode;
>    CertVerifier::SHA1Mode mSHA1Mode;
>    NetscapeStepUpPolicy mNetscapeStepUpPolicy;
> -  const char* mFirstPartyDomain;
> +  NeckoOriginAttributes mOriginAttributes;

Should this maybe just be a non-owning const reference?

::: security/certverifier/OCSPCache.cpp:219
(Diff revision 1)
>  {
>    MutexAutoLock lock(mMutex);
>  
>    size_t index;
> -  if (!FindInternal(aCertID, aFirstPartyDomain, index, lock)) {
> +  if (!FindInternal(aCertID, aOriginAttributes, index, lock)) {
>      LogWithCertID("OCSPCache::Get(%p,\"%s\") not in cache", aCertID,

I don't think "%s" works here any longer, since OriginAttributes.mFirstPartyDomain is a wide string, right? We should either convert to utf-8 or maybe "%S" will work?

::: security/manager/ssl/TransportSecurityInfo.cpp:102
(Diff revision 1)
>    *aPort = mPort;
>    return NS_OK;
>  }
>  
>  nsresult
> -TransportSecurityInfo::SetFirstPartyDomain(const nsACString& aFirstPartyDomain)
> +TransportSecurityInfo::SetOriginAttributes(const NeckoOriginAttributes& aOriginAttributes)

style nit: this line is >80 characters. I guess just putting the whole 'const NeckoOriginAttributes& aOriginAttributes' on the next line indented two spaces would be best.

::: security/manager/ssl/nsNSSCallbacks.cpp:114
(Diff revision 1)
>      priorityChannel->AdjustPriority(nsISupportsPriority::PRIORITY_HIGHEST);
>  
>    chan->SetLoadFlags(nsIRequest::LOAD_ANONYMOUS |
>                       nsIChannel::LOAD_BYPASS_SERVICE_WORKER);
>  
> -  if (!mRequestSession->mFirstPartyDomain.IsEmpty()) {
> +  // Only record first party domain on OCSP channels.

I was a bit confused by this comment initially. It might help to restructure and expand it: "For OCSP requests, only the first party domain aspect of origin attributes is used. This means that OCSP requests are shared across different containers."

::: security/manager/ssl/tests/unit/head_psm.js:327
(Diff revision 1)
>   *   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.
> - * @param {String} aFirstPartyDomain
> + * @param {String} aOriginAttributes

nit: this isn't a string any more, right? Also let's update the documentation to say this is optional.

::: security/manager/ssl/tests/unit/head_psm.js:328
(Diff revision 1)
>   *   after the TLS handshake succeeds.
>   * @param {Function} aAfterStreamOpen
>   *   A callback function that is called with the nsISocketTransport once the
>   *   output stream is ready.
> - * @param {String} aFirstPartyDomain
> + * @param {String} aOriginAttributes
>   *   The first party domain which will be used to double-key the OCSP cache.

nit: should probably update this line to refer to the origin attribute's first party domain

Comment 6

[:Cykesiopka]

Comment on attachment 8808955 [details]
Bug 1315143 - Make OCSP use Origin Attribute framework (PSM).

https://reviewboard.mozilla.org/r/91662/#review92582

This looks good, but FYI you're going to have to rebase and fix some resultant build errors.

::: security/certverifier/NSSCertDBTrustDomain.h:83
(Diff revision 1)
>                         CertVerifier::PinningMode pinningMode,
>                         unsigned int minRSABits,
>                         ValidityCheckingMode validityCheckingMode,
>                         CertVerifier::SHA1Mode sha1Mode,
>                         NetscapeStepUpPolicy netscapeStepUpPolicy,
> -                       const char* firstPartyDomain,
> +                       const NeckoOriginAttributes& originAttributes,

This needs an explicit `#include "mozilla/BasePrincipal.h"` or something.

::: security/certverifier/OCSPCache.h:63
(Diff revision 1)
> -  // The first party domain is only non-empty when "privacy.firstParty.isolate"
> -  // is enabled, in order to isolate OCSP cache by first party.
> +  // We only use the origin attributes' first party domain, which is only
> +  // non-empty when "privacy.firstParty.isolate" is enabled, in order to isolate
> +  // OCSP cache by first party.

Nit: I would reword this and mention why we even need to pass in origin attributes first.
Maybe something like this:
```
The passed in origin attributes are used to isolate the OCSP cache.
We currently only use the first party domain portion of the attributes, and it is non-empty only when "privacy.firstParty.isolate" is enabled.
```
I would also move this comment after "If it is in the cache [...]".

::: security/certverifier/OCSPCache.h:74
(Diff revision 1)
> -  // The first party domain is only non-empty when "privacy.firstParty.isolate"
> -  // is enabled, in order to isolate OCSP cache by first party.
> +  // We only use the origin attributes' first party domain, which is only
> +  // non-empty when "privacy.firstParty.isolate" is enabled, in order to isolate
> +  // OCSP cache by first party.

Same "reword" comment as above.

::: security/certverifier/OCSPCache.cpp:63
(Diff revision 1)
>  
>  // 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.
>  // Let serialNumberLen be the number of bytes of serialNumber.
> -// The first party domain is only non-empty when "privacy.firstParty.isolate"
> +// Let firstPartyDomain be the first party domain of aOriginAttributes.

Nit: The actual parameter is named `originAttributes`.
It would also be nice to fix the out of date lines above that reference `aFoo` parameters while we're here, but up to you.

::: security/certverifier/OCSPCache.cpp:116
(Diff revision 1)
> -    rv = DigestLength(context, firstPartyDomainLen);
> +  nsCString firstPartyDomain =
> +    NS_ConvertUTF16toUTF8(originAttributes.mFirstPartyDomain);

Nit: NS_ConvertUTF16toUTF8 is a class, so the following should work:
```cpp
NS_ConvertUTF16toUTF8 firstPartyDomain(originAttributes.mFirstPartyDomain);
```
(This is apparently more efficient.)

::: security/certverifier/OCSPRequestor.h:17
(Diff revision 1)
>  
>  namespace mozilla { namespace psm {
>  
>  // The memory returned via |encodedResponse| is owned by the given arena.
>  Result DoOCSPRequest(const UniquePLArenaPool& arena, const char* url,
> -                     const char* firstPartyDomain,
> +                     const NeckoOriginAttributes& originAttributes,

This should have a forward declaration or something.

::: security/manager/ssl/nsSSLSocketProvider.cpp:11
(Diff revision 1)
>  
>  #include "nsSSLSocketProvider.h"
>  #include "nsNSSIOLayer.h"
>  #include "nsError.h"
>  
> +using mozilla::NeckoOriginAttributes;

Needs an explicit include or something.

::: security/manager/ssl/nsTLSSocketProvider.cpp:11
(Diff revision 1)
>  
>  #include "nsTLSSocketProvider.h"
>  #include "nsNSSIOLayer.h"
>  #include "nsError.h"
>  
> +using mozilla::NeckoOriginAttributes;

Needs an explicit include or something.

Comment 7

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8808955 [details]
Bug 1315143 - Make OCSP use Origin Attribute framework (PSM).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91662/diff/1-2/

Comment 8

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8808956 [details]
Bug 1315143 - Make OCSP use Origin Attribute framework (Necko).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91664/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/690c0cdddff7
Make OCSP use Origin Attribute framework (Necko). r=Cykesiopka,keeler
https://hg.mozilla.org/integration/autoland/rev/81a11a25d25d
Make OCSP use Origin Attribute framework (PSM). r=mayhemer

Comment 10

[Jonathan Hao (inactive) [:jhao]]

Thanks, Honza, David, and Cykesiopka.

Comment 11

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=6577878&repo=autoland

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eaefbcd7fd7f
Backed out changeset 81a11a25d25d 
https://hg.mozilla.org/integration/autoland/rev/0cb0af1888e3
Backed out changeset 690c0cdddff7 for bustage

Comment 13

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8808955 [details]
Bug 1315143 - Make OCSP use Origin Attribute framework (PSM).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91662/diff/2-3/

Comment 14

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8808956 [details]
Bug 1315143 - Make OCSP use Origin Attribute framework (Necko).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91664/diff/2-3/

Comment 15

[Jonathan Hao (inactive) [:jhao]]

Hi, David and Cykesiopka.

I fixed the bustage that caused the backout, but then I got this.

https://treeherder.mozilla.org/logviewer.html#?job_id=31095906&repo=try#L5831

Do you have any idea what could cause this error?

Thanks.

Comment 16

[:Cykesiopka]

(In reply to Jonathan Hao [:jhao] (Away until 11/21) from comment #15)
> Hi, David and Cykesiopka.
> 
> I fixed the bustage that caused the backout, but then I got this.
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=31095906&repo=try#L5831
> 
> Do you have any idea what could cause this error?
> 
> Thanks.

Looks like you got bitten by different warnings being enabled in different directories.
certverifier/ has quite a few warnings enabled.

Here's the relevant section of the log:
> 23:34:52     INFO -  Unified_cpp_certverifier0.cpp
> 23:34:52     INFO -  c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\obj-firefox\dist\include\js/Value.h(448): error C2220: warning treated as error - no 'object' file generated
> 23:34:52     INFO -  Warning: C4365 in c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\obj-firefox\dist\include\js\Value.h: 'return': conversion from 'const int32_t' to 'JS::Value::PayloadType', signed/unsigned mismatch
> 23:34:52     INFO -  c:\builds\moz2_slave\try-w32-0000000000000000000000\build\src\obj-firefox\dist\include\js/Value.h(448): warning C4365: 'return': conversion from 'const int32_t' to 'JS::Value::PayloadType', signed/unsigned mismatch

The code in question: https://hg.mozilla.org/mozilla-central/file/28e2a6dde76ab6ad4464a3662df1bd57af04398a/js/public/Value.h#l448
> PayloadType toNunboxPayload() const {
>     return data.s.payload.i32;
> }

This is getting triggered because of the new BasePrincipal.h includes.
BasePrincipal.h includes nsJSPrincipals.h, which includes jsapi.h, which includes Value.h.

I guess the proper fix here would be to address all these mismatches in Value.h.
But, it would probably be easier to just surround the include with pragmas to disable the warnings being triggered (I think this is possible, but I'm not 100% sure), and file a bug to have the Value.h mismatches fixed.

Comment 17

[Dana Keeler (she/her) (use needinfo) [:keeler]]

That sounds like a good approach.

Comment 18

[Jonathan Hao (inactive) [:jhao]]

Thanks, Cykesiopka and David.  I disabled the one in Value.h, but I got two more in RootingAPI.h and Vector.h.  And I can't use pragma warning push/pop because there's an unclosed pragma warning push in Vector.h ...  So I think I have to use pragma default to re-enable those warning.

https://treeherder.mozilla.org/logviewer.html#?job_id=31528230&repo=try#L4534

Comment 19

[Jonathan Hao (inactive) [:jhao]]

Attachment: Bug 1315143 - Disable warnings caused by including BasePrincipal.h

Review commit: https://reviewboard.mozilla.org/r/94342/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94342/

Comment 20

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8808955 [details]
Bug 1315143 - Make OCSP use Origin Attribute framework (PSM).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91662/diff/3-4/

Comment 21

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8808956 [details]
Bug 1315143 - Make OCSP use Origin Attribute framework (Necko).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91664/diff/3-4/

Comment 22

[Jonathan Hao (inactive) [:jhao]]

These patches can now build on windows.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a75876c5654d9c3b0315fb6cc10a64781ca8e6d

Comment 23

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8812680 [details]
Bug 1315143 - Disable warnings caused by including BasePrincipal.h

https://reviewboard.mozilla.org/r/94342/#review94700

It's unfortunate but I think this is a fine workaround unless Cykesiopka has a better idea.

::: security/certverifier/CertVerifier.h:19
(Diff revision 1)
>  #include "mozilla/Telemetry.h"
>  #include "mozilla/UniquePtr.h"
>  #include "pkix/pkixtypes.h"
>  
> +#if defined(_MSC_VER)
> +#pragma warning( disable : 4324 4365 5032 )

style nit: from what I can see, it's more common to not have spaces before/after the parentheses and colon in these situations

Comment 24

[:Cykesiopka]

Comment on attachment 8812680 [details]
Bug 1315143 - Disable warnings caused by including BasePrincipal.h

https://reviewboard.mozilla.org/r/94342/#review94968

The general idea is fine; I don't have better suggestions at the moment.

::: security/certverifier/CertVerifier.h:18
(Diff revision 1)
> -#include "mozilla/BasePrincipal.h"
>  #include "mozilla/Telemetry.h"
>  #include "mozilla/UniquePtr.h"
>  #include "pkix/pkixtypes.h"
>  
> +#if defined(_MSC_VER)

We should probably have a comment here noting what files are forcing us to disable the warnings.

::: security/certverifier/CertVerifier.h:23
(Diff revision 1)
> +#if defined(_MSC_VER)
> +#pragma warning( disable : 4324 4365 5032 )
> +#endif /* defined(_MSC_VER) */
> +#include "mozilla/BasePrincipal.h"
> +#if defined(_MSC_VER)
> +#pragma warning( default : 4324 4365 5032 )

https://msdn.microsoft.com/en-us/library/2c8f766e.aspx says "default" means:
"[...] This also turns on a specified warning that is off by default [...]".

So unless I'm misunderstanding, using "default" in a header file doesn't seem like a great idea.

We should probably use the push, disable, pop pattern that's much more common in m-c instead.

Comment 25

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8812680 [details]
Bug 1315143 - Disable warnings caused by including BasePrincipal.h

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94342/diff/1-2/

Comment 26

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8812680 [details]
Bug 1315143 - Disable warnings caused by including BasePrincipal.h

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94342/diff/2-3/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80a39e170b41
Make OCSP use Origin Attribute framework (PSM). r=Cykesiopka,keeler
https://hg.mozilla.org/integration/autoland/rev/498b09513061
Make OCSP use Origin Attribute framework (Necko). r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/d7dd10403797
Disable warnings caused by including BasePrincipal.h r=Cykesiopka,keeler

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/80a39e170b41
https://hg.mozilla.org/mozilla-central/rev/498b09513061
https://hg.mozilla.org/mozilla-central/rev/d7dd10403797

Comment 29

[Ethan Tseng [:ethan]]

Jonathan, can you request to uplift this bug to Aurora 52?

Comment 30

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8808955 [details]
Bug 1315143 - Make OCSP use Origin Attribute framework (PSM).

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: If declined, Firefox 52 will not have full first party isolation support.  We need to get these patches in ESR 52, so that Tor Browser can take them.
[Describe test coverage new/current, TreeHerder]: covered by test_ocsp_caching.js and OCSPCacheTest.cpp
[Risks and why]: minimum because first party isolation is pref'ed off.
[String/UUID change made/needed]: none

Comment 31

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8808956 [details]
Bug 1315143 - Make OCSP use Origin Attribute framework (Necko).

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: If declined, Firefox 52 will not have full first party isolation support.  We need to get these patches in ESR 52, so that Tor Browser can take them.
[Describe test coverage new/current, TreeHerder]: covered by test_ocsp_caching.js and OCSPCacheTest.cpp
[Risks and why]: minimum because first party isolation is pref'ed off.
[String/UUID change made/needed]: none

Comment 32

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8812680 [details]
Bug 1315143 - Disable warnings caused by including BasePrincipal.h

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: If declined, Firefox 52 will not have full first party isolation support.  We need to get these patches in ESR 52, so that Tor Browser can take them.
[Describe test coverage new/current, TreeHerder]: covered by test_ocsp_caching.js and OCSPCacheTest.cpp
[Risks and why]: minimum because first party isolation is pref'ed off.
[String/UUID change made/needed]: none

Comment 33

[Jonathan Hao (inactive) [:jhao]]

Also, these patches blocks bug 1316283, which is very critical for containers and first party isolation.

Comment 34

[Julien Cristau [:jcristau]]

Comment on attachment 8808955 [details]
Bug 1315143 - Make OCSP use Origin Attribute framework (PSM).

use origin attribute framework, aurora52+

Comment 35

[Julien Cristau [:jcristau]]

Comment on attachment 8808956 [details]
Bug 1315143 - Make OCSP use Origin Attribute framework (Necko).

use origin attribute framework, aurora52+

Comment 36

[Julien Cristau [:jcristau]]

Comment on attachment 8812680 [details]
Bug 1315143 - Disable warnings caused by including BasePrincipal.h

use origin attribute framework, aurora52+

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c916ea845d36
https://hg.mozilla.org/releases/mozilla-aurora/rev/b045ef0bb77c
https://hg.mozilla.org/releases/mozilla-aurora/rev/4d95d6b47637