1409259 - Implement a console warning for Symantec CAs affected by the distrust plan

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P1)

Description

[J.C. Jones [:jcj] (he/they)]

Add logic to identify the Symantec CAs that are to be affected by the plan for the graduated distrust of Symantec roots to PSM, and then use that logic to emit a warning to the developer console.

This warning will be best-effort; architectural limitations will keep us from evaluating this code for resumed sessions. As there won't be resumed sessions once the distrust is in place, I'm not intending to depend on a re-architecture of session resumption for this bug. (Though it sure would be nice...)

Comment 1

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 1409259 - Refactor "TrustOverrides" header for existing trust overrides

Since we'll need the same structs and mechanisms to work with the Symantec roots,
this patch makes the matching function generic and moves it into a new header,
"TrustOverrides.h".

This also moves the GlobalSignData out into "TrustOverride-GlobalSignData.inc"
and the WoSign/StartCom to "TrustOverride-StartComAndWoSignData.inc".

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

Comment 2

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 1409259 - Add Symantec root and Apple/Google intermediate lists

This is only the list of affected Symantec roots. It does not include any
carved out intermediates at this time, so it should be considered a work-in-
progress, just as an example of what's coming for feedback purposes.

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

Comment 3

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 1409259 - Add a console warning for soon-to-be-distrusted roots

This patch adds a new diagnostic status flag to nsIWebProgressListener,
STATE_CERT_DISTRUST_IMMINENT, which indicates that the certificate chain is
going to change validity due to an upcoming distrust event. The first of
these events is this bug, affecting various roots from Symantec.

The STATE_CERT_DISTRUST_IMMINENT flag is set by nsNSSCallbacks and passed,
via nsSecureBrowserUIImpl, to browser.js where it is used to alert the console.

Adding this sort of diagnostic printing to be accessible to browser.js is a
long-desired goal, as future functionality can start doing more decision-making
there. We may, for example, also want to degrade the lock icon, which will be
straightforward with this flag.

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

Comment 4

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8919951 [details]
Bug 1409259 - Add Symantec root and Apple/Google intermediate lists

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

Comment 5

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8920009 [details]
Bug 1409259 - Add a console warning for soon-to-be-distrusted roots

Mark, can you take a brief look at this and see if it's generally going the direction of your patch, and doing sort of what you suggested I do?

Thanks!

Comment 6

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

Comment on attachment 8919950 [details]
Bug 1409259 - Refactor "TrustOverrides" header for existing trust overrides

https://reviewboard.mozilla.org/r/190896/#review196500

Cool - lgtm with the template change.

::: security/certverifier/NSSCertDBTrustDomain.cpp:37
(Diff revision 1)
>  #include "pkix/pkix.h"
>  #include "pkix/pkixnss.h"
>  #include "prerror.h"
>  #include "secerr.h"
>  
> -#include "StartComAndWoSignData.inc"
> +#include "TrustOverrides.h"

Hmmm - maybe TrustOverrideUtils.h?

::: security/certverifier/TrustOverrides.h:19
(Diff revision 1)
> +  uint32_t len;
> +};
> +
> +static bool
> +CertDNIsInList(const CERTCertificate* aCert, const DataAndLength* aDnList,
> +               const size_t aListSz)

If we make this function a template and pass a reference to an array of a certain (i.e. a template parameter) size, we can make some simplifications (and it'll be safer).

::: security/certverifier/TrustOverrides.h:21
(Diff revision 1)
> +
> +static bool
> +CertDNIsInList(const CERTCertificate* aCert, const DataAndLength* aDnList,
> +               const size_t aListSz)
> +{
> +  MOZ_ASSERT(aCert && aDnList);

Let's add the non-debug code to handle getting a null pointer as well.

::: security/certverifier/TrustOverrides.h:23
(Diff revision 1)
> +CertDNIsInList(const CERTCertificate* aCert, const DataAndLength* aDnList,
> +               const size_t aListSz)
> +{
> +  MOZ_ASSERT(aCert && aDnList);
> +
> +  for (size_t i = 0; i < aListSz; i++) {

I think this can be `for (auto& dn : aDnList) {` with the above change.

Comment 7

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8919950 [details]
Bug 1409259 - Refactor "TrustOverrides" header for existing trust overrides

https://reviewboard.mozilla.org/r/190896/#review196500

Thanks! Good ideas.

Comment 8

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 1409259 - Add the Apple and Google sub-CA data.

These sub-CAs are to be explicitly whitelisted in the distrust logic being
applied to Symantec root CAs.

Originally compiled from here:
https://groups.google.com/d/msg/mozilla.dev.security.policy/FLHRT79e3XE/riCrpXsfAgAJ

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

Comment 9

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8919950 [details]
Bug 1409259 - Refactor "TrustOverrides" header for existing trust overrides

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

Comment 10

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8919951 [details]
Bug 1409259 - Add Symantec root and Apple/Google intermediate lists

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

Comment 11

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8920009 [details]
Bug 1409259 - Add a console warning for soon-to-be-distrusted roots

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

Comment 12

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8919951 [details]
Bug 1409259 - Add Symantec root and Apple/Google intermediate lists

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

Comment 13

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8920423 [details]
Bug 1409259 - Add the Apple and Google sub-CA data.

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

Comment 14

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8920009 [details]
Bug 1409259 - Add a console warning for soon-to-be-distrusted roots

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

Comment 15

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 1409259 - Implement the Symantec distrust algorithm WIP

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

Comment 16

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8920009 [details]
Bug 1409259 - Add a console warning for soon-to-be-distrusted roots

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

Comment 17

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

Comment on attachment 8919951 [details]
Bug 1409259 - Add Symantec root and Apple/Google intermediate lists

https://reviewboard.mozilla.org/r/190898/#review196850

Seems reasonable but I think we should include the generation script.

::: security/certverifier/TrustOverride-SymantecData.inc:1
(Diff revision 4)
> +// Script downloaded from https://gist.github.com/ed5d869cd8da3951e393f4a2d8a3391f

Maybe just include the script in security/manager/tools?

::: security/certverifier/TrustOverride-SymantecData.inc:4
(Diff revision 4)
> +// Script downloaded from https://gist.github.com/ed5d869cd8da3951e393f4a2d8a3391f
> +// Invocation: crtshToDNStruct.py 17 3381895 847444 4350 4174851 4175126 12729019 8983600 12726040 8983601 30 3382830 254193 8984570 68409 26682 2771491 93 1039083
> +
> +// /C=US/O=GeoTrust Inc./CN=GeoTrust Global CA

If I'm reading the script correctly, this is only part of the subject DN (and not necessarily in the same order as in the certificate). Maybe it would be best to print a representation of the DN as-is?

Comment 18

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

Comment on attachment 8920423 [details]
Bug 1409259 - Add the Apple and Google sub-CA data.

https://reviewboard.mozilla.org/r/191392/#review196904

Same idea as the previous commit.

Comment 19

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

Comment on attachment 8920009 [details]
Bug 1409259 - Add a console warning for soon-to-be-distrusted roots

https://reviewboard.mozilla.org/r/190980/#review196852

Looks good, modulo some minor issues (well, and tests).

::: browser/base/content/browser.js:7507
(Diff revision 4)
> +    if (this._isCertDistrustImminent) {
> +      let consoleMsg = Cc["@mozilla.org/scripterror;1"].createInstance(Ci.nsIScriptError);
> +      let windowId = gBrowser.selectedBrowser.innerWindowID;
> +      // Use uri.prePath instead of initWithSourceURI() so that these can be
> +      // de-duplicated on the scheme+host+port combination.
> +      consoleMsg.initWithWindowID("The security certificate in use for this web site " +

This needs to be a localized message.
Also I guess we need a test for this :/

::: browser/base/content/browser.js:7508
(Diff revision 4)
> +      let consoleMsg = Cc["@mozilla.org/scripterror;1"].createInstance(Ci.nsIScriptError);
> +      let windowId = gBrowser.selectedBrowser.innerWindowID;
> +      // Use uri.prePath instead of initWithSourceURI() so that these can be
> +      // de-duplicated on the scheme+host+port combination.
> +      consoleMsg.initWithWindowID("The security certificate in use for this web site " +
> +                      "will be invalid  in a future release of Firefox. For more " +

nit: extra space after 'invalid'

::: security/manager/ssl/nsNSSCallbacks.cpp:1443
(Diff revision 4)
> +  // TODO: Update to the real source of the certificate, once Bug 1406856 lands
> +  nsCOMPtr<nsIX509CertList> succeededCertChain = new nsNSSCertList();
> +  bool distrustImminent;
> +  if (NS_SUCCEEDED(IsCertificateDistrustImminent(succeededCertChain,
> +                                                 &distrustImminent))) {
> +    if (distrustImminent) {

I guess technically this is a "lonely if" (i.e. this could be just one compound statement, but then it might look bad due to wrapping long lines, so... ¯\_(ツ)_/¯ )

Comment 20

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

Comment on attachment 8920582 [details]
Bug 1409259 - Implement the Symantec distrust algorithm WIP

https://reviewboard.mozilla.org/r/191596/#review196854

This seems like a reasonable approach. Have we verified that the affected certificate hierarchies will really only involve chains of length 3? (Like, are there no undisclosed constrained subCAs involved?)
Also, you know, tests.

::: security/manager/ssl/nsNSSCallbacks.cpp:1267
(Diff revision 1)
>      sslStatus->SetCertificateTransparencyInfo(certificateTransparencyInfo);
>    }
>  }
>  
>  static nsresult
> +SplitTripleCertificateChain(nsIX509CertList* aCertList,

Let's document this - what it expects, what it returns, etc.

::: security/manager/ssl/nsNSSCallbacks.cpp:1269
(Diff revision 1)
>  }
>  
>  static nsresult
> +SplitTripleCertificateChain(nsIX509CertList* aCertList,
> +                            /* out */ nsCOMPtr<nsIX509Cert>& aRoot,
> +                            /* out */ nsCOMPtr<nsIX509Cert>& aIntermediate,

Depending, we may want an nsIX509CertList here (and plural "intermediates").

::: security/manager/ssl/nsNSSCallbacks.cpp:1278
(Diff revision 1)
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsCOMPtr<nsISimpleEnumerator> chainElt;
> +  nsresult rv = aCertList->GetEnumerator(getter_AddRefs(chainElt));
> +  NS_ENSURE_SUCCESS(rv, rv);

I think we should avoid NS_ENSURE_SUCCESS and similar macros for new code.

::: security/manager/ssl/nsNSSCallbacks.cpp:1308
(Diff revision 1)
> +  if (!hasMoreCerts) {
> +    return NS_ERROR_FAILURE; // Insufficient chain length
> +  }
> +
> +  {
> +    nsCOMPtr<nsISupports> certSupports;

Factor this code pattern out?

::: security/manager/ssl/nsNSSCallbacks.cpp:1340
(Diff revision 1)
> +
> +  if (!hasMoreCerts) {
> +    return NS_OK; // standard length 3 chain
> +  }
> +
> +  return NS_ERROR_FAILURE; // Unexpectedly long chain, not relevant

Are we sure there are no chains of length other than 3 that we'll want to apply this to?

::: security/manager/ssl/nsNSSCallbacks.cpp:1352
(Diff revision 1)
>    MOZ_ASSERT(aResult);
>    if (!aCertList || !aResult) {
>      return NS_ERROR_INVALID_ARG;
>    }
>  
> -  // TODO: Implement algorithm below (and add tests)
> +  nsCOMPtr<nsIX509Cert> rootCert, intCert, eeCert;

nit: one declaration per line

::: security/manager/ssl/nsNSSCallbacks.cpp:1371
(Diff revision 1)
> +    return NS_OK;
> +  }
>  
>    // If the end entity's notBefore date is not before 2016-06-01, exit false
> +  nsCOMPtr<nsIX509CertValidity> validity;
> +  eeCert->GetValidity(getter_AddRefs(validity));

This will probably never fail, but we should still be checking these return values.

::: security/manager/ssl/nsNSSCallbacks.cpp:1374
(Diff revision 1)
>    // If the end entity's notBefore date is not before 2016-06-01, exit false
> +  nsCOMPtr<nsIX509CertValidity> validity;
> +  eeCert->GetValidity(getter_AddRefs(validity));
>  
> -  // If the intermediate is one of the whitelisted intermediates, exit false
> +  PRTime notBefore;
> +  validity->GetNotBefore(&notBefore);

Same here.

::: security/manager/ssl/nsNSSCallbacks.cpp:1380
(Diff revision 1)
>  
> -  // Otherwise, this certificate will imminently be distrusted, exit true
> +  // PRTime is microseconds since the epoch, whereas JS time is milliseconds.
> +  // (new Date("2016-06-01T00:00:00Z")).getTime() * 1000
> +  static const PRTime JUNE_1_2016 = 1464739200000000;
> +
> +  if (notBefore > JUNE_1_2016) {

`*aResult = notBefore <= JUNE_1_2016;`?

Comment 21

[J.C. Jones [:jcj] (he/they)]

Gerv:

I've authored a temporary string that says:

> The security certificate in use for this web site will be invalid  in a future release of Firefox. For more information, visit https://FILL_ME_IN/

I need to get that into the localization queue, but regarding the FILL_ME_IN website: Can you take on writing something up for this circumstance? Since I figure we might want to use this code in the future, I'm generically referring to the flags here as indicating a certificate that will be "imminently distrusted," so I am kind of imagining a wiki or SUMO page for "imminent PKI distrust actions" of which there could be a heading here for the upcoming work with the Symantec roots.

Comment 22

[Gervase Markham [:gerv]]

I would say:

The security certificate in use on this web site will no longer be trusted in a future release of Firefox. For more information, visit https://FILL_ME_IN/

Here's a start for the FILL_ME_IN:

https://wiki.mozilla.org/CA/Upcoming_Distrust_Actions

Gerv

Comment 23

[J.C. Jones [:jcj] (he/they)]

Thanks, Gerv!

Comment 24

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8919951 [details]
Bug 1409259 - Add Symantec root and Apple/Google intermediate lists

https://reviewboard.mozilla.org/r/190898/#review196850

> If I'm reading the script correctly, this is only part of the subject DN (and not necessarily in the same order as in the certificate). Maybe it would be best to print a representation of the DN as-is?

Good idea.

Comment 25

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8920009 [details]
Bug 1409259 - Add a console warning for soon-to-be-distrusted roots

https://reviewboard.mozilla.org/r/190980/#review196852

> This needs to be a localized message.
> Also I guess we need a test for this :/

I don't think we can test this until the next patch, but I'm writing one using DN matching against the completed distrust algorithm.

Comment 26

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8919950 [details]
Bug 1409259 - Refactor "TrustOverrides" header for existing trust overrides

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

Comment 27

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8919951 [details]
Bug 1409259 - Add Symantec root and Apple/Google intermediate lists

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

Comment 28

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8920009 [details]
Bug 1409259 - Add a console warning for soon-to-be-distrusted roots

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

Comment 29

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

Comment on attachment 8919951 [details]
Bug 1409259 - Add Symantec root and Apple/Google intermediate lists

https://reviewboard.mozilla.org/r/190898/#review199176

Cool - lgtm with the python nits addressed. The naming the RootDNs thing isn't as important.

::: security/manager/tools/crtshToDNStruct/crtshToDNStruct.py:1
(Diff revision 5)
> +#!/usr/bin/env python

Looks like `./mach lint` notes some style issues with this file.

::: security/manager/tools/crtshToDNStruct/crtshToDNStruct.py:84
(Diff revision 5)
> +    r.raise_for_status()
> +
> +    cert = x509.load_pem_x509_certificate(r.content, default_backend())
> +    blocks.append(print_block(cert))
> +
> +  print("static const DataAndLength RootDNs[]= {")

Maybe add an argument so RootDNs can be what it'll end up being? (e.g. RootSymantecDNs for the Symantec certs)

Comment 30

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8919951 [details]
Bug 1409259 - Add Symantec root and Apple/Google intermediate lists

https://reviewboard.mozilla.org/r/190898/#review199176

Thanks!

> Looks like `./mach lint` notes some style issues with this file.

Aha!

> Maybe add an argument so RootDNs can be what it'll end up being? (e.g. RootSymantecDNs for the Symantec certs)

It also needs deduplication logic, but how often are we going to run this thing? I figure I can make it nicer next time.

Comment 31

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 1409259 - Add browser console test for the distrust console message

This patch finishes out the earlier patches in the series by implementing
the symantec distrust algorithm in nsNSSCallbacks.cpp and wiring it to the
GetSucceededCertChain call added in Bug 1406856.

Tests are in the next patch.

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

Comment 32

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8919950 [details]
Bug 1409259 - Refactor "TrustOverrides" header for existing trust overrides

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

Comment 33

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8919951 [details]
Bug 1409259 - Add Symantec root and Apple/Google intermediate lists

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190898/diff/5-6/

Comment 34

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8920009 [details]
Bug 1409259 - Add a console warning for soon-to-be-distrusted roots

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190980/diff/5-6/

Comment 35

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

Comment on attachment 8923613 [details]
Bug 1409259 - Add browser console test for the distrust console message

https://reviewboard.mozilla.org/r/194742/#review199794

Great - well decomposed.

::: security/manager/ssl/nsNSSCallbacks.cpp:1269
(Diff revision 1)
>    }
>  }
>  
>  static nsresult
>  IsCertificateDistrustImminent(nsIX509CertList* aCertList,
>                                /* out */ bool* aResult) {

I guess this goes in the previous commit, but I think we should use a reference here.

::: security/manager/ssl/nsNSSCallbacks.cpp:1327
(Diff revision 1)
> +
> +  // PRTime is microseconds since the epoch, whereas JS time is milliseconds.
> +  // (new Date("2016-06-01T00:00:00Z")).getTime() * 1000
> +  static const PRTime JUNE_1_2016 = 1464739200000000;
> +
> +  // If the end entity's notBefore date is not before 2016-06-01, exit false

Maybe flip this comment around? (i.e. if the ee's notBefore is before the cutoff, return true) I found it a bit difficult to reconcile the description of what was happening with the logical representation.

::: security/manager/ssl/nsNSSCallbacks.cpp:1484
(Diff revision 1)
>    } else {
>      DetermineEVAndCTStatusAndSetNewCert(status, fd, infoObject);
>    }
>  
> -  // TODO: Update to the real source of the certificate, once Bug 1406856 lands
> -  nsCOMPtr<nsIX509CertList> succeededCertChain = new nsNSSCertList();
> +  nsCOMPtr<nsIX509CertList> succeededCertChain;
> +  // This always returns NS_OK

Although it can return a null list... maybe make a note of this?

Comment 36

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8923613 [details]
Bug 1409259 - Add browser console test for the distrust console message

https://reviewboard.mozilla.org/r/194742/#review199794

Thanks!

> I guess this goes in the previous commit, but I think we should use a reference here.

This must have been a rebase issue, because I'm sure we already did this somewhere. Sorry.

> Maybe flip this comment around? (i.e. if the ee's notBefore is before the cutoff, return true) I found it a bit difficult to reconcile the description of what was happening with the logical representation.

Will do, and this also should happen first, as it's the least expensive check.

Comment 37

[J.C. Jones [:jcj] (he/they)]

Attachment: Bug 1409259 - Add xpcshell tests for the Symantec distrust

This commit adds two new xpcshell tests, both of them testing whether the
security state in TransportSecurityInfo includes the new
STATE_CERT_DISTRUST_IMMINENT flag under the correct circumstances.

The first test, test_symantec_apple_google.js, tests the four combinations of
certs that chain to an affected Symantec root: with/without a whitelisted
intermediate, and before/after the notBefore cutoff date.

The second test, test_symantec_apple_google_unaffected.js, tests an unrelated
ca->intermediate->ee chain that does not chain to an affected root, and ensures
the flag is not set.

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

Comment 38

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8919950 [details]
Bug 1409259 - Refactor "TrustOverrides" header for existing trust overrides

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

Comment 39

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8919951 [details]
Bug 1409259 - Add Symantec root and Apple/Google intermediate lists

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190898/diff/6-7/

Comment 40

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8920009 [details]
Bug 1409259 - Add a console warning for soon-to-be-distrusted roots

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190980/diff/6-7/

Comment 41

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8923613 [details]
Bug 1409259 - Add browser console test for the distrust console message

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

Comment 42

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8920009 [details]
Bug 1409259 - Add a console warning for soon-to-be-distrusted roots

https://reviewboard.mozilla.org/r/190980/#review200394

LGTM.

::: security/manager/ssl/nsNSSCallbacks.cpp:1441
(Diff revision 7)
> +  bool distrustImminent;
> +  if (NS_SUCCEEDED(IsCertificateDistrustImminent(succeededCertChain,
> +                                                 &distrustImminent))) {
> +    if (distrustImminent) {

As David already noted, maybe change this to:

```
rv = IsCertificateDistrustImminent(succeededCertChain, &distrustImminent);
if (NS_SUCCEEDED(rv) && distrustImminent) {
  // ...
}
```

?

Comment 43

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8920009 [details]
Bug 1409259 - Add a console warning for soon-to-be-distrusted roots

https://reviewboard.mozilla.org/r/190980/#review200394

Thanks, Tim!

> As David already noted, maybe change this to:
> 
> ```
> rv = IsCertificateDistrustImminent(succeededCertChain, &distrustImminent);
> if (NS_SUCCEEDED(rv) && distrustImminent) {
>   // ...
> }
> ```
> 
> ?

Okay, okay :D

Comment 44

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

Comment on attachment 8924032 [details]
Bug 1409259 - Add xpcshell tests for the Symantec distrust

https://reviewboard.mozilla.org/r/195244/#review200562

LGTM - just a few comments. In particular, we probably don't actually need the test_symantec_apple_google_unaffected directory at all.

::: security/manager/ssl/tests/unit/test_symantec_apple_google.js:11
(Diff revision 1)
> +// Tests handling of certificates issued by Symantec. If such
> +// certificates have a notBefore before 1 June 2016, and are not
> +// issued by an Apple or Google intermediate, they should emit a
> +// warning to the console.
> +
> +const STATE_CERT_DISTRUST_IMMINENT = 0x00008000;

We should probably use Ci.nsIWebProgressListener.STATE_CERT_DISTRUST_IMMINENT

::: security/manager/ssl/tests/unit/test_symantec_apple_google.js:32
(Diff revision 1)
> +
> +add_tls_server_setup("SymantecSanctionsServer", "test_symantec_apple_google");
> +
> +// Whitelisted certs aren't to be distrusted
> +add_connection_test("symantec-whitelist-after-cutoff.example.com",
> +                    PRErrorCodeSuccess, noOp, shouldNotBeImminentlyDistrusted);

I think we should be safe passing null instead of noOp.

::: security/manager/ssl/tests/unit/test_symantec_apple_google/ee-from-whitelist-after-cutoff.pem.certspec:3
(Diff revision 1)
> +issuer:printableString/C=US/O=Google Inc/CN=Google Internet Authority G2
> +subject:ee-from-whitelist-after-cutoff
> +validity:20160601-20500101

Keep in mind if we ever implement validity period limits we would have to update/regenerate these.

::: security/manager/ssl/tests/unit/test_symantec_apple_google_unaffected.js:11
(Diff revision 1)
> +// Tests handling of certificates issued by Symantec. If such
> +// certificates have a notBefore before 1 June 2016, and are not
> +// issued by an Apple or Google intermediate, they should emit a
> +// warning to the console.
> +
> +const STATE_CERT_DISTRUST_IMMINENT = 0x00008000;

Same as the other test.

::: security/manager/ssl/tests/unit/test_symantec_apple_google_unaffected.js:15
(Diff revision 1)
> +
> +const STATE_CERT_DISTRUST_IMMINENT = 0x00008000;
> +
> +function shouldNotBeImminentlyDistrusted(aTransportSecurityInfo) {
> +  let isDistrust = aTransportSecurityInfo.securityState & STATE_CERT_DISTRUST_IMMINENT;
> +  Assert.ok(!isDistrust, "This host should not be imminently distrusted");

I feel like we can piggyback this test onto pretty much any other successful connection test and avoid adding these certificates files/this extra directory. Alternatively, if it's worth it to have a separate test for this case (to logically separate test cases and not have everything spaghettify), maybe just re-use a preexisting hostname/certificate from the BadCertServer or OCSPStaplingServer.

Comment 45

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8924032 [details]
Bug 1409259 - Add xpcshell tests for the Symantec distrust

https://reviewboard.mozilla.org/r/195244/#review200562

Thanks, good observations!

Comment 46

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8919950 [details]
Bug 1409259 - Refactor "TrustOverrides" header for existing trust overrides

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190896/diff/5-6/

Comment 47

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8919951 [details]
Bug 1409259 - Add Symantec root and Apple/Google intermediate lists

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190898/diff/7-8/

Comment 48

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8920009 [details]
Bug 1409259 - Add a console warning for soon-to-be-distrusted roots

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190980/diff/7-8/

Comment 49

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8924032 [details]
Bug 1409259 - Add xpcshell tests for the Symantec distrust

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

Comment 50

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8923613 [details]
Bug 1409259 - Add browser console test for the distrust console message

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

Comment 51

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8923613 [details]
Bug 1409259 - Add browser console test for the distrust console message

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

Comment 52

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8923613 [details]
Bug 1409259 - Add browser console test for the distrust console message

David - I don't know what I did to this (increasingly large) patchset to make Reviewboard think you've already reviewed this patch but this (the browser console test) is new tonight.

Comment 53

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8923613 [details]
Bug 1409259 - Add browser console test for the distrust console message

https://reviewboard.mozilla.org/r/194742/#review201148

::: devtools/client/webconsole/test/browser_console_certificate_imminent_distrust.js:40
(Diff revision 4)
> +// Finally, copy in that key as a .ca file:
> +// (NOTE: files ended in .ca are added as trusted roots by the mochitest harness)
> +//   cp ../../../security/manager/ssl/tests/unit/test_symantec_apple_google/test-ca.pem symantecRoot.ca

Do we need to add `symantecRoot.p12` then? Seems like that's just transient.

::: devtools/client/webconsole/test/browser_console_certificate_imminent_distrust.js:77
(Diff revision 4)
> +    let opened = waitForBrowserConsole();
> +
> +    let hud = HUDService.getBrowserConsole();
> +    ok(!hud, "browser console is not open");
> +    info("wait for the browser console to open with ctrl-shift-j");
> +    EventUtils.synthesizeKey("j", { accelKey: true, shiftKey: true }, window);

Should we maybe use `await HUDService.toggleBrowserConsole()` here? I remember EventUtils being a little fickle with regard to focus, leading to intermittent issues sometimes. We might not even need to wait for it to open then?

Comment 54

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8923613 [details]
Bug 1409259 - Add browser console test for the distrust console message

https://reviewboard.mozilla.org/r/194742/#review201190

LGTM otherwise.

Comment 55

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8923613 [details]
Bug 1409259 - Add browser console test for the distrust console message

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

Comment 56

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8923613 [details]
Bug 1409259 - Add browser console test for the distrust console message

https://reviewboard.mozilla.org/r/194742/#review201148

Thanks!

Comment 57

[J.C. Jones [:jcj] (he/they)]

Try runs are good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=163e941eab23ae2780f859145e290d842ea04440
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e08dceb6dd8dea622e18314676204dd9e070afe7

So let's do this thing.

Note to those coming after: This will cause some amount of performance hit for sites using Symantec roots, which we'll see in telemetry. It should be small, and is unfortunately unavoidable for this policy, though if it's bad we could try re-doing this as a hash map implementation.

Thanks to David and Tim for all the reviews!

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79d1ac7232f3
Refactor "TrustOverrides" header for existing trust overrides r=keeler
https://hg.mozilla.org/integration/autoland/rev/d3acb68f73c4
Add Symantec root and Apple/Google intermediate lists r=keeler
https://hg.mozilla.org/integration/autoland/rev/595e27212723
Add a console warning for soon-to-be-distrusted roots r=keeler,ttaubert
https://hg.mozilla.org/integration/autoland/rev/862c080c2913
Add xpcshell tests for the Symantec distrust r=keeler
https://hg.mozilla.org/integration/autoland/rev/98b1272e170c
Add browser console test for the distrust console message r=keeler,ttaubert

Comment 59

[J.C. Jones [:jcj] (he/they)]

(In reply to J.C. Jones [:jcj] from comment #57)
> Note to those coming after: This will cause some amount of performance hit
> for sites using Symantec roots, which we'll see in telemetry. It should be
> small, and is unfortunately unavoidable for this policy 

Matt: You'll see this as a performance hit in the Canary for affected Symantec sites.

Comment 60

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/79d1ac7232f3
https://hg.mozilla.org/mozilla-central/rev/d3acb68f73c4
https://hg.mozilla.org/mozilla-central/rev/595e27212723
https://hg.mozilla.org/mozilla-central/rev/862c080c2913
https://hg.mozilla.org/mozilla-central/rev/98b1272e170c

Comment 61

[Francesco Lodolo [:flod]]

(In reply to J.C. Jones [:jcj] from comment #21)
> Gerv:
> 
> I've authored a temporary string that says:
> 
> > The security certificate in use for this web site will be invalid  in a future release of Firefox. For more information, visit https://FILL_ME_IN/
> 
> I need to get that into the localization queue, but regarding the FILL_ME_IN
> website: Can you take on writing something up for this circumstance? Since I
> figure we might want to use this code in the future, I'm generically
> referring to the flags here as indicating a certificate that will be
> "imminently distrusted," so I am kind of imagining a wiki or SUMO page for
> "imminent PKI distrust actions" of which there could be a heading here for
> the upcoming work with the Symantec roots.

Should it be a concern that this string will always say Firefox, since it’s hard coded instead of using a placeholder for the branding? Normally it would be, but I’m not sure in this context.

I guess it's OK to say "Firefox" in Nightly and Beta, not so sure in a rebranded build?

Comment 62

[J.C. Jones [:jcj] (he/they)]

(In reply to Francesco Lodolo [:flod] from comment #61)
> Should it be a concern that this string will always say Firefox, since it’s
> hard coded instead of using a placeholder for the branding? Normally it
> would be, but I’m not sure in this context.

This is my first time doing anything with strings that users see, so you'd know better than I. That said, this is specifically a Mozilla CA Certificate Program action, so it might be a little fuzzy?

I think we could definitely do a follow on to either remove the branding or add in a branding variable. We need to do it quick though before this merges to Beta. :)

Just as a try, what about just deleting "of Firefox":

> certImminentDistrust.message = The security certificate in use on this web site will no longer be trusted in a future release. For more information, visit https://wiki.mozilla.org/CA/Upcoming_Distrust_Actions

?

Comment 63

[Francesco Lodolo [:flod]]

(In reply to J.C. Jones [:jcj] from comment #62)
> I think we could definitely do a follow on to either remove the branding or
> add in a branding variable. We need to do it quick though before this merges
> to Beta. :)

Not only that. I haven't exposed that string to localization tools yet, if I do the procedure to change it is
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

If we can fix it quickly, i.e. in the next 2 or 3 days, you can fix just the text without changing the string ID.

> Just as a try, what about just deleting "of Firefox":
> 
> > certImminentDistrust.message = The security certificate in use on this web site will no longer be trusted in a future release. For more information, visit https://wiki.mozilla.org/CA/Upcoming_Distrust_Actions

That would work for me, but not sure whose call it is to approve this change.

Comment 64

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx58 release notes.

Comment 65

[Kohei Yoshino]

Somehow I missed this bug but posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/symantec-issued-certificates-will-soon-be-distrusted/