1434300 - Distrust Symantec CAs affected by the distrust plan

Status: RESOLVED FIXED

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

Description

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

This bug is to complete the draft plan from M.D.S.P. [1]. 

Collected in here will be:

1) Adapt the tests from Bug 1409259 which verify the console warning to cover a fictitious CA rather than Symantec's CAs, preserving the warning code for future use

2) Move the Symantec distrust code so that it distrusts the certificates rather than printing a warning


[1] https://groups.google.com/d/topic/mozilla.dev.security.policy/FLHRT79e3XE/discussion

Comment 1

[Kai Engert [:KaiE:]]

Do you consider the whitelisting for excluded subCAs part of this bug?

Comment 2

[Kai Engert [:KaiE:]]

(In reply to Kai Engert (:kaie:) from comment #1)
> Do you consider the whitelisting for excluded subCAs part of this bug?

I assume the answer is yes. I see that you had already added whitelists as part of the earlier work that implemented the browser console warnings.

Comment 3

[Kai Engert [:KaiE:]]

I'm looking at the various sources of information for the distrust plan, and I'd like to mention a few observations. (You might already be aware of these details, but maybe it can be helpful.)

(1)

This report:
  https://arkadiyt.com/2018/02/04/quantifying-untrusted-symantec-certificates/
claims that the initial phase of distrust will distrust certificates issued before 2016-06-01, and also certificates issued after 2017-12-01.

However, the console warning in the current implementation 
  https://hg.mozilla.org/integration/autoland/rev/595e27212723#l3.64
appears to check only for certs issued earlier than 2016-06-01,
but doesn't seem to check for certs issued after 2017-12-01.

Google's page
  https://security.googleblog.com/2017/09/chromes-plan-to-distrust-symantec.html
says:
  "December 1, 2017 ... Any certificates issued by Symantec’s old infrastructure 
   after this point will cease working in a future Chrome update."
but it doesn't cleary state when exactly they will no longer be accepted.

Would it make sense to update the Firefox implementation for warn for certificates issued after 2017-12-01, too, and might it be useful to update the warning code early, in a Firefox 58.x or Firefox 59 release?

Should the distrust implemented in this bug for Firefox 60 also distrust certificates that were issused after 2017-12-01?


(2)

The whitelist of intermediates might be incomplete, based on the most recent information.

A recent message on the newsgroup provided a link to this chromium code:
  https://chromium.googlesource.com/chromium/src/+/master/net/data/ssl/symantec

In addition to intermediates from Apple and Google, which you had already added to Firefox, there are 4 intermediates from DigiCert.

Will Firefox whitelist those DigiCert subCAs, too?

Would it make sense to add the DigiCert subCAs to the whitelist in a release earlier than Firefox 60?

If Firefox 58/59 aren't aware of these subCAs, then Firefox 58/59 are probably giving false console warnings for the sites issued by the DigiCert subCAs?

Comment 4

[Kai Engert [:KaiE:]]

One Google subCA has already expired and can probably be removed from the whitelist.

Firefox whitelists 7 Apple subCAs, and the newsgroup posting had mentioned 7 Apple subCAs, too.

However, the page 
  https://chromium.googlesource.com/chromium/src/+/master/net/data/ssl/symantec
only lists 6 Apple subCAs.

It should probably be clarified if the Google page is incomplete, or if Firefox should remove one of the Apple subCAs.

Comment 5

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

Attachment: Bug 1434300 - Update Imminent Distrust status for future Symantec sanctions

This patch does a few things:

1) It adds a permament test mechanism for the "imminent distrust" trust status
in nsNSSCallbacks: a simple xpcshell test to exercise a clause in the imminent
distrust logic in nsNSSCallbacks' IsCertificateDistrustImminent method.

2) This test removes test_symantec_apple_google_unaffected.js as its
functionality is rolled into the new test_imminent_distrust.js.

3) It updates the Symantec imminent distrust warning algorithm to remove the
validity date exception; this warns of the upcoming distrust for those affected
certs in Firefox 63.

This patch does not attempt to edit the browser chrome test that checks the
console; that is a subsequent patch.

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

Comment 6

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

Attachment: Bug 1434300 - Implement the Symantec distrust plan from Bug 1409257

The algorithm from https://hg.mozilla.org/mozilla-central/rev/595e27212723
(Bug 1409259) is adapted in this patch from nsNSSCallbacks into the TrustDomain
decisions.

This patch does not change the algorithm to use SPKI matching, nor add the
additional whitelisted intermediates from DigiCert; that will be done in a
separate commit.

This patch also does not update the pre-existing browser chrome test.

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

Comment 7

[Code Review Bot [:reviewbot]]

Comment on attachment 8950470 [details]
Bug 1434300 - Update Imminent Distrust status for future Symantec sanctions

https://reviewboard.mozilla.org/r/219736/#review225456


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: security/manager/tools/crtshToDNStruct/crtshToDNStruct.py:97
(Diff revision 1)
>      for crtshId in certshIds:
> +        # Try a local file first, then crt.sh
> +        try:
> +            with open(crtshId, "rb") as pemFile:
> +                blocks.append(print_block(pemFile.read(), None))
> +        except:

Error: Do not use bare except' [flake8: E722]

Comment 8

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

Comment on attachment 8950470 [details]
Bug 1434300 - Update Imminent Distrust status for future Symantec sanctions

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

Comment 9

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

Comment on attachment 8950471 [details]
Bug 1434300 - Implement the Symantec distrust plan from Bug 1409257

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

Comment 10

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8950470 [details]
Bug 1434300 - Update Imminent Distrust status for future Symantec sanctions

https://reviewboard.mozilla.org/r/219736/#review225492

::: security/manager/ssl/tests/unit/xpcshell.ini:100
(Diff revision 2)
>  [test_hash_algorithms_wrap.js]
>  # bug 1124289 - run_test_in_child violates the sandbox on android
>  skip-if = toolkit == 'android'
>  [test_hmac.js]
>  [test_intermediate_basic_usage_constraints.js]
> +[test_imminent_distrust.js]

So this doesn't work on Android?

::: security/manager/tools/crtshToDNStruct/crtshToDNStruct.py:94
(Diff revision 2)
>      print("// Script from security/manager/tools/crtshToDNStruct/crtshToDNStruct.py")
>      print("// Invocation: {} {}".format(sys.argv[0], " ".join(certshIds)))
>      print()
>      for crtshId in certshIds:
> +        # Try a local file first, then crt.sh
> +        try:

You've seen the Code Review Bot comment :)

Comment 11

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8950471 [details]
Bug 1434300 - Implement the Symantec distrust plan from Bug 1409257

https://reviewboard.mozilla.org/r/219738/#review225500

::: security/certverifier/NSSCertDBTrustDomain.cpp:777
(Diff revision 2)
> +{
> +    // PRECONDITION: The rootCert is already verified as being one of the
> +    // affected Symantec roots
> +
> +    // Check the preference to see if this is enabled before proceeding.
> +    // TODO in Bug 1437754

Do you want to land that pref first? Or at the same time?

::: security/manager/ssl/nsNSSCertValidity.h:26
(Diff revision 2)
>    virtual ~nsX509CertValidity() {}
>  
>  private:
>    nsresult FormatTime(const PRTime& aTime,
>                        PRTimeParamFn aParamFn,
> -                      const nsTimeFormatSelector aTimeFormatSelector,
> +                      const mozilla::nsTimeFormatSelector aTimeFormatSelector,

Why?

Comment 12

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

Comment on attachment 8950470 [details]
Bug 1434300 - Update Imminent Distrust status for future Symantec sanctions

https://reviewboard.mozilla.org/r/219736/#review225492

Thanks for the review!

> So this doesn't work on Android?

It's not marked to skip on Android...?

> You've seen the Code Review Bot comment :)

....and it's already resolved with the `except FileNotFoundError`?

Comment 13

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

Comment on attachment 8950471 [details]
Bug 1434300 - Implement the Symantec distrust plan from Bug 1409257

https://reviewboard.mozilla.org/r/219738/#review225500

> Do you want to land that pref first? Or at the same time?

I'll do it after this change. It needs an enum to be plumbed through all the methods of mozilla::pkix, since there's no way to get at preferences otherwise. All that boilerplate is such a PITA... and in an optimal world, I'd assign it to someone junior. But of course, that's me! :)

> Why?

This was necessary to get NSSCertDBTrustDomain.cpp
to compile when including this header. I presume other including files explicitly include the `mozilla` namespace, but mozpkix has its own namespace.

Comment 14

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8950470 [details]
Bug 1434300 - Update Imminent Distrust status for future Symantec sanctions

https://reviewboard.mozilla.org/r/219736/#review225492

> It's not marked to skip on Android...?

my bad... I guess I should sleep more.

> ....and it's already resolved with the `except FileNotFoundError`?

:+1:

Comment 15

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

Comment on attachment 8950470 [details]
Bug 1434300 - Update Imminent Distrust status for future Symantec sanctions

https://reviewboard.mozilla.org/r/219736/#review225710

If you get errors like the test server can't find the new certificate you added when landing this, you might need to touch CLOBBER. Not sure, though. Anyway, lgtm.

::: security/manager/ssl/tests/unit/bad_certs/ee-imminently-distrusted.pem:21
(Diff revision 2)
> +2G8UlObIdzuym5FPDpvuiQIcDIZELUOQO9V+fjeK0CZ4luFnox+cIvnB39pLa5Xd
> +hHUyWMgsf9cW/T1yjjRAS2YX/HUYGjSH9MNhSriiAABers1fyJkn7fdVTav2pQTp
> +5yNdtwrFYkWjw1DG17uj/gtkll3ACw9oztjYTGj/okDI+ViLJqL4QeQb4G4Lpp77
> ++A8JfHGf/yFprXMMExy8FNN8FLIxdN2lX4WwBS5WHQ==
> +-----END CERTIFICATE-----
> +

Is this from the build system generating the file or from directly calling pycert.py? (I ask because I attempted to make it output files with only the one trailing newline, not two...)

::: security/manager/ssl/tests/unit/bad_certs/ee-imminently-distrusted.pem.certspec:4
(Diff revision 2)
> +issuer:Test CA
> +subject:printableString/C=US/CN=Imminently Distrusted End Entity
> +extension:subjectAlternativeName:localhost,imminently-distrusted.example.com
> +extension:authorityInformationAccess:http://localhost:8888/

Technically, not sure we need an OCSP URI here but whatevs.

::: security/manager/tools/crtshToDNStruct/crtshToDNStruct.py:72
(Diff revision 2)
>      print("// {dn}".format(dn=distinguished_name))
>      print("// SHA256 Fingerprint: " + ":".join(fingerprint[:16]))
>      print("//                     " + ":".join(fingerprint[16:]))
> +    if crtshId:
> -    print("// https://crt.sh/?id={crtsh} (crt.sh ID={crtsh})"
> +        print("// https://crt.sh/?id={crtsh} (crt.sh ID={crtsh})"
> -          .format(crtsh=crtshId))
> +              .format(crtsh=crtshId))

Did this just not work before?

Comment 16

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

Comment on attachment 8950471 [details]
Bug 1434300 - Implement the Symantec distrust plan from Bug 1409257

https://reviewboard.mozilla.org/r/219738/#review225722

LGTM with some more return value checks and changing the revoked error to unknown issuer.

::: security/certverifier/NSSCertDBTrustDomain.cpp:812
(Diff revision 2)
> +    // doesn't apply, so exit false before we do any iterating
> +    if (notBefore >= JUNE_1_2016) {
> +      return Success;
> +    }
> +
> +    // This algorithm only applies if either the serverAuth or anyAuth EKUs are

This comment seems out of place, since this function doesn't do any checking wrt eku.

::: security/certverifier/NSSCertDBTrustDomain.cpp:822
(Diff revision 2)
> +
> +    // Look for one of the intermediates to be in the whitelist
> +    bool foundInWhitelist = false;
> +    RefPtr<nsNSSCertList> intCertList = intCerts->GetCertList();
> +
> +    intCertList->ForEachCertificateInChain(

Need a return value check here.

::: security/certverifier/NSSCertDBTrustDomain.cpp:835
(Diff revision 2)
> +        }
> +        return NS_OK;
> +    });
> +
> +    if (!foundInWhitelist) {
> +      return Result::ERROR_REVOKED_CERTIFICATE;

As discussed in the meeting, let's use unknown issuer for now.

::: security/certverifier/NSSCertDBTrustDomain.cpp:930
(Diff revision 2)
>        return Result::ERROR_POLICY_VALIDATION_FAILED;
>      }
>  
>      bool foundRequiredIntermediate = false;
>      RefPtr<nsNSSCertList> intCertList = intCerts->GetCertList();
>      intCertList->ForEachCertificateInChain(

Might be nice to add a return value check here too.

::: security/manager/ssl/tests/unit/test_symantec_apple_google.js:40
(Diff revision 2)
>  add_connection_test("symantec-not-whitelisted-after-cutoff.example.com",
>                      PRErrorCodeSuccess, null, shouldBeImminentlyDistrusted);
>  
> -// Not whitelisted certs before the cutoff are to be distrusted
> +// Not whitelisted certs before the cutoff are to be revoked
>  add_connection_test("symantec-not-whitelisted-before-cutoff.example.com",
> -                    PRErrorCodeSuccess, null, shouldBeImminentlyDistrusted);
> +                    SEC_ERROR_REVOKED_CERTIFICATE, null, null);

Unknown issuer for now.

Comment 17

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

Comment on attachment 8950470 [details]
Bug 1434300 - Update Imminent Distrust status for future Symantec sanctions

https://reviewboard.mozilla.org/r/219736/#review225710

Good to know.

> Is this from the build system generating the file or from directly calling pycert.py? (I ask because I attempted to make it output files with only the one trailing newline, not two...)

The build system generated this one; I forgot about directly using `pucert.py` until later... didn't figure there would be a difference. Want me to regenerate it?

> Did this just not work before?

It worked. It's just trying to make a one-off tool be slightly less ugly.

Comment 18

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

Comment on attachment 8950471 [details]
Bug 1434300 - Implement the Symantec distrust plan from Bug 1409257

https://reviewboard.mozilla.org/r/219738/#review225722

> This comment seems out of place, since this function doesn't do any checking wrt eku.

Oops. Great catch, this needs EKU checks.

Comment 19

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

Attachment: Bug 1434300 - Add a utility to match certificates based on SPKI

This modifies crtshToDNStruct.py to be able to produce SPKI or DN-based lists,
and adds a SPKI-search method to TrustOverrideUtils.h.

This also regenerates the TrustOverride files to use the new script.

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

Comment 20

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

Attachment: Bug 1434300 - Change Symantec Distrust Algorithm's whitelist to SPKI-matching

Because of the DigiCert-controlled sub-CAs and managed-CAs identified as also
needing to be whitelisted [1], and that those CAs are using an increasing number
of certificates all with different Subjects (but identical public keys) [2][3],
we will have to whitelist on SPKI rather than subject DN.

This makes the security/manager/ssl/tests/unit/test_symantec_apple_google.js
integration test much... shorter, as we can no longer test whitelist inclusion.

This patch does not add the DigiCert SPKIs to the list; I will do that in its
own patch.


[1] https://chromium.googlesource.com/chromium/src/+/master/net/data/ssl/symantec/README.md
[2] https://chromium-review.googlesource.com/c/chromium/src/+/916730
[3] https://crt.sh/?spkisha256=ac50b5fb738aed6cb781cc35fbfff7786f77109ada7c08867c04a573fd5cf9ee

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

Comment 21

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

Comment on attachment 8950471 [details]
Bug 1434300 - Implement the Symantec distrust plan from Bug 1409257

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

Comment 22

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8950817 [details]
Bug 1434300 - Add a utility to match certificates based on SPKI

https://reviewboard.mozilla.org/r/220066/#review226050

lgtm, two nits.

::: security/certverifier/tests/gtest/TrustOverrideTest.cpp:112
(Diff revision 1)
> +
> +static mozilla::UniqueCERTCertificate
> +CertFromString(const char* aPem)
> +{
> +  nsCOMPtr<nsIX509Cert> cert;
> +  cert = nsNSSCertificate::ConstructFromDER(const_cast<char*>(aPem), strlen(aPem));

`ConstructFromDER` should probably take a `const char*`.

::: security/manager/tools/crtshToIdentifyingStruct/crtshToIdentifyingStruct.py:52
(Diff revision 1)
>          return "O"
>      if oid == NameOID.ORGANIZATIONAL_UNIT_NAME:
>          return "OU"
>      raise Exception("Unknown OID: {}".format(oid))
>  
> -def print_block(pemData, crtshId):
> +def print_block(pemData, identifierType="DN", crtshId=None):

Do you need the default for `identifierType`? I think you pass one in everyt time.

Comment 23

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8950818 [details]
Bug 1434300 - Change Symantec Distrust Algorithm's whitelist to SPKI-matching

https://reviewboard.mozilla.org/r/220068/#review226052

lgtm, I didn't check all the SPKIs though ;)

Comment 24

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

Comment on attachment 8950817 [details]
Bug 1434300 - Add a utility to match certificates based on SPKI

https://reviewboard.mozilla.org/r/220066/#review226142

Nice - looks good.

::: security/certverifier/TrustOverrideUtils.h:45
(Diff revision 1)
> +  MOZ_ASSERT(aCert);
> +  if (!aCert) {
> +    return false;
> +  }
> +
> +  for (auto &spki: aSpkiList) {

nit: & to the left

::: security/certverifier/tests/gtest/TrustOverrideTest.cpp:72
(Diff revision 1)
> +  0x30, 0x0D, 0x31, 0x0B, 0x30, 0x09, 0x06, 0x03, 0x55, 0x04, 0x03, 0x0C, 0x02,
> +  0x63, 0x61,
> +};
> +
> +static const DataAndLength OverrideCaDNs[]= {
> +  { CAcaDN,

nit: seems like this could be one line, but not a big deal

::: security/certverifier/tests/gtest/TrustOverrideTest.cpp:104
(Diff revision 1)
> +  0x49, 0x23, 0xFA, 0x72, 0x51, 0xC4, 0x31, 0xD5, 0x03, 0xAC, 0xDA, 0x18, 0x0A,
> +  0x35, 0xED, 0x8D, 0x02, 0x03, 0x01, 0x00, 0x01,
> +};
> +
> +static const DataAndLength OverrideCaSPKIs[]= {
> +  { CAcaSPKI,

same idea here

::: security/certverifier/tests/gtest/TrustOverrideTest.cpp:111
(Diff revision 1)
> +};
> +
> +static mozilla::UniqueCERTCertificate
> +CertFromString(const char* aPem)
> +{
> +  nsCOMPtr<nsIX509Cert> cert;

nit: declaration/assignment on the same line? (probably will have to wrap anyway, but still)

Comment 25

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

Comment on attachment 8950817 [details]
Bug 1434300 - Add a utility to match certificates based on SPKI

https://reviewboard.mozilla.org/r/220066/#review226050

> `ConstructFromDER` should probably take a `const char*`.

This seems to be a result of the NSS APIs we're using here (and that there's no such thing as a SECItem pointing to const data). Fixing this for real might be a bit involved (but would be nice in the long run).

Comment 26

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

Comment on attachment 8950471 [details]
Bug 1434300 - Implement the Symantec distrust plan from Bug 1409257

https://reviewboard.mozilla.org/r/219738/#review226158

::: security/certverifier/NSSCertDBTrustDomain.cpp:771
(Diff revision 3)
>    return Success;
>  }
>  
> +// Implement the graduated Symantec distrust algorithm from Bug 1409257
> +static Result
> +CheckForSymantecDistrust(const RefPtr<nsNSSCertList>& certList)

er... so is this a duplicate of IsCertificateDistrustImminent in nsNSSCallbacks.cpp? We should see if we can share code here.

Comment 27

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

Comment on attachment 8950818 [details]
Bug 1434300 - Change Symantec Distrust Algorithm's whitelist to SPKI-matching

https://reviewboard.mozilla.org/r/220068/#review226154

LGTM.

::: commit-message-80351:9
(Diff revision 1)
> +needing to be whitelisted [1], and that those CAs are using an increasing number
> +of certificates all with different Subjects (but identical public keys) [2][3],
> +we will have to whitelist on SPKI rather than subject DN.
> +
> +This makes the security/manager/ssl/tests/unit/test_symantec_apple_google.js
> +integration test much... shorter, as we can no longer test whitelist inclusion.

Can we find an existing chain that should be whitelisted and verify it at a time we know it will be valid?

::: security/certverifier/TrustOverride-AppleGoogleData.inc:206
(Diff revision 1)
> -  { CAAppleISTCA3G1DN,
> -    sizeof(CAAppleISTCA3G1DN) },
> -  { CAAppleISTCA6G1DN,
> -    sizeof(CAAppleISTCA6G1DN) },
> +};
> +
> +static const DataAndLength RootAppleAndGoogleSPKIs[]= {
> +  { CAGoogleInternetAuthorityG2SPKI,
> +    sizeof(CAGoogleInternetAuthorityG2SPKI) },
> +  { CAGoogleInternetAuthorityG2SPKI,

Duplicate entry?

Comment 28

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8950817 [details]
Bug 1434300 - Add a utility to match certificates based on SPKI

https://reviewboard.mozilla.org/r/220066/#review226050

> This seems to be a result of the NSS APIs we're using here (and that there's no such thing as a SECItem pointing to const data). Fixing this for real might be a bit involved (but would be nice in the long run).

Yeah, my point was to use `const` as long as possible. It has to go away before handing it to NSS but could be cast away at that point. Not a big deal for this patch though.

Comment 29

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

Comment on attachment 8950471 [details]
Bug 1434300 - Implement the Symantec distrust plan from Bug 1409257

https://reviewboard.mozilla.org/r/219738/#review226158

> er... so is this a duplicate of IsCertificateDistrustImminent in nsNSSCallbacks.cpp? We should see if we can share code here.

It's not exactly, but you're right, I will parameterize this and put it into the Utils.

Comment 30

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

Comment on attachment 8950817 [details]
Bug 1434300 - Add a utility to match certificates based on SPKI

https://reviewboard.mozilla.org/r/220066/#review226142

> nit: seems like this could be one line, but not a big deal

Fixed in the script and the files. :)

Comment 31

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

Comment on attachment 8950817 [details]
Bug 1434300 - Add a utility to match certificates based on SPKI

https://reviewboard.mozilla.org/r/220066/#review226050

> Yeah, my point was to use `const` as long as possible. It has to go away before handing it to NSS but could be cast away at that point. Not a big deal for this patch though.

Totally fair. Dropping for now.

> Do you need the default for `identifierType`? I think you pass one in everyt time.

I do, but it's not hurting anything, and I'm not a fan of using `**kwargs` with so few options as it, to me, disguises what the function is doing. So I'm going to drop this one for style purposes, though I totally understand where you're coming from.

Comment 32

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

Comment on attachment 8950818 [details]
Bug 1434300 - Change Symantec Distrust Algorithm's whitelist to SPKI-matching

https://reviewboard.mozilla.org/r/220068/#review226154

> Can we find an existing chain that should be whitelisted and verify it at a time we know it will be valid?

Great idea! I'm adding such a test to `test_symantec_apple_google.js`.

> Duplicate entry?

Oops. Thanks :)

Comment 33

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

Comment on attachment 8950471 [details]
Bug 1434300 - Implement the Symantec distrust plan from Bug 1409257

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

Comment 34

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

Comment on attachment 8950817 [details]
Bug 1434300 - Add a utility to match certificates based on SPKI

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

Comment 35

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

Comment on attachment 8950818 [details]
Bug 1434300 - Change Symantec Distrust Algorithm's whitelist to SPKI-matching

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

Comment 36

[Code Review Bot [:reviewbot]]

Comment on attachment 8950818 [details]
Bug 1434300 - Change Symantec Distrust Algorithm's whitelist to SPKI-matching

https://reviewboard.mozilla.org/r/220068/#review227014


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: security/manager/ssl/tests/unit/test_symantec_apple_google.js:50
(Diff revision 2)
> +
> +  checkCertErrorGenericAtTime(certDB, whitelistedCert, PRErrorCodeSuccess,
> +                              certificateUsageSSLServer, VALIDATION_TIME);
> +
> +  run_next_test();
> +}

Error: Newline required at end of file but not found. [eslint: eol-last]

Comment 37

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8950817 [details]
Bug 1434300 - Add a utility to match certificates based on SPKI

https://reviewboard.mozilla.org/r/220066/#review227128

Comment 38

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

Comment on attachment 8950818 [details]
Bug 1434300 - Change Symantec Distrust Algorithm's whitelist to SPKI-matching

https://reviewboard.mozilla.org/r/220068/#review227014

> Error: Newline required at end of file but not found. [eslint: eol-last]

Fixed.

Comment 39

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

Attachment: Bug 1434300 - Add the DigiCert whitelisted SPKIs

This adds the 4 digicert CAs to our whitelist as specified in Google's details
on the Chromium version of this plan [1].

[1] https://chromium.googlesource.com/chromium/src/+/master/net/data/ssl/symantec/README.md

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

Comment 40

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

Attachment: Bug 1434300 - Disable the b-c imminent distrust test

The browser console test for imminent distrust relies upon certs inserted
into build/pgo/certs/cert8.db and key3.db, and seemed angry when adding new
certs with certutil bumping those to cert9 and key4 foramts. Since time is
getting to be of-the-essence with Bug 1434300, I've filed Bug 1439378 to re-
enable this test post-landing.

https://hg.mozilla.org/mozilla-central/rev/98b1272e170c

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

Comment 41

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

Comment on attachment 8950818 [details]
Bug 1434300 - Change Symantec Distrust Algorithm's whitelist to SPKI-matching

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

Comment 42

[Code Review Bot [:reviewbot]]

Comment on attachment 8952157 [details]
Bug 1434300 - Disable the b-c imminent distrust test

https://reviewboard.mozilla.org/r/221402/#review227214


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/webconsole/test/browser.ini:164
(Diff revision 1)
>  [browser_bug_869003_inspect_cross_domain_object.js]
>  [browser_bug_871156_ctrlw_close_tab.js]
>  [browser_cached_messages.js]
>  [browser_console.js]
>  [browser_console_addonsdk_loader_exception.js]
> -[browser_console_certificate_imminent_distrust.js]
> +#[browser_console_certificate_imminent_distrust.js] # bug 1439378 to re-enable

Error: Use 'disable=<reason>' to disable a test instead of a comment [no-comment-disable: None]

Comment 43

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

Comment on attachment 8952157 [details]
Bug 1434300 - Disable the b-c imminent distrust test

https://reviewboard.mozilla.org/r/221402/#review227214

> Error: Use 'disable=<reason>' to disable a test instead of a comment [no-comment-disable: None]

Ah, cool. I should... probably always run `mach lint` before pushing to review.

Comment 44

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

Comment on attachment 8950470 [details]
Bug 1434300 - Update Imminent Distrust status for future Symantec sanctions

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

Comment 45

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

Comment on attachment 8950471 [details]
Bug 1434300 - Implement the Symantec distrust plan from Bug 1409257

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

Comment 46

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

Comment on attachment 8950817 [details]
Bug 1434300 - Add a utility to match certificates based on SPKI

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

Comment 47

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

Comment on attachment 8950818 [details]
Bug 1434300 - Change Symantec Distrust Algorithm's whitelist to SPKI-matching

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

Comment 48

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

Comment on attachment 8952156 [details]
Bug 1434300 - Add the DigiCert whitelisted SPKIs

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

Comment 49

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

Comment on attachment 8952157 [details]
Bug 1434300 - Disable the b-c imminent distrust test

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

Comment 50

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

Comment on attachment 8950817 [details]
Bug 1434300 - Add a utility to match certificates based on SPKI

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

Comment 51

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

Comment on attachment 8950818 [details]
Bug 1434300 - Change Symantec Distrust Algorithm's whitelist to SPKI-matching

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

Comment 52

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

Comment on attachment 8952156 [details]
Bug 1434300 - Add the DigiCert whitelisted SPKIs

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

Comment 53

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

Comment on attachment 8952157 [details]
Bug 1434300 - Disable the b-c imminent distrust test

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

Comment 54

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

Comment on attachment 8950817 [details]
Bug 1434300 - Add a utility to match certificates based on SPKI

https://reviewboard.mozilla.org/r/220066/#review227612

::: commit-message-e9b06:10
(Diff revisions 1 - 4)
>  
>  This also regenerates the TrustOverride files to use the new script.
>  
>  MozReview-Commit-ID: BhMoJbYXs7Y
> +* * *
> +[mq]: fix_test

Looks like some extra stuff sneaked in here...

::: security/manager/tools/crtshToIdentifyingStruct/crtshToIdentifyingStruct.py:135
(Diff revisions 1 - 4)
>              r.raise_for_status()
>              blocks.append(print_block(r.content, crtshId=certId, identifierType=identifierType))
>  
>      print("static const DataAndLength " + args.listname + "[]= {")
>      for structName in blocks:
> +        if len(structName) < 33:

Ohhh, right - the struct data was auto-generated, not hand-written. Well, looks like you figured out a reasonable way to do this anyway.

Comment 55

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

Comment on attachment 8950818 [details]
Bug 1434300 - Change Symantec Distrust Algorithm's whitelist to SPKI-matching

https://reviewboard.mozilla.org/r/220068/#review227614

::: security/manager/ssl/tests/unit/test_symantec_apple_google.js:41
(Diff revisions 1 - 5)
> +  addCertFromFile(certDB, "test_symantec_apple_google/real-google-g2-intermediate.pem", ",,");
> +  let whitelistedCert = constructCertFromFile("test_symantec_apple_google/real-googlecom.pem");
> +
> +  // Since we don't want to actually try to fetch OCSP for this certificate,
> +  // (as an external fetch is bad in the tests), disable OCSP first.
> +  Services.prefs.setIntPref("security.OCSP.enabled", 0);

You can also pass FLAG_LOCAL_ONLY to the nsIX509CertDB function underlying this, but no big deal.

Comment 56

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

Comment on attachment 8952156 [details]
Bug 1434300 - Add the DigiCert whitelisted SPKIs

https://reviewboard.mozilla.org/r/221400/#review227618

::: commit-message-6e811:6
(Diff revision 3)
> +Bug 1434300 - Add the DigiCert whitelisted SPKIs r?keeler r?wthayer
> +
> +This adds the 4 digicert CAs to our whitelist as specified in Google's details
> +on the Chromium version of this plan [1].
> +
> +[1] https://chromium.googlesource.com/chromium/src/+/master/net/data/ssl/symantec/README.md

Should we link to a particular revision in case this gets removed in the future?

Comment 57

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

Comment on attachment 8950817 [details]
Bug 1434300 - Add a utility to match certificates based on SPKI

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

Comment 58

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

Comment on attachment 8950818 [details]
Bug 1434300 - Change Symantec Distrust Algorithm's whitelist to SPKI-matching

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

Comment 59

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

Comment on attachment 8952156 [details]
Bug 1434300 - Add the DigiCert whitelisted SPKIs

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

Comment 60

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

Comment on attachment 8952157 [details]
Bug 1434300 - Disable the b-c imminent distrust test

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

Comment 61

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

Comment on attachment 8950471 [details]
Bug 1434300 - Implement the Symantec distrust plan from Bug 1409257

https://reviewboard.mozilla.org/r/219738/#review227636

::: security/certverifier/TrustOverrideUtils.h:61
(Diff revision 5)
> +static nsresult
> +CheckForSymantecDistrust(const nsCOMPtr<nsIX509CertList>& intCerts,
> +                         const nsCOMPtr<nsIX509Cert>& eeCert,
> +                         const PRTime& permitAfterDate,
> +                         const DataAndLength (&aWhitelist)[T],
> +                         /* out */ bool& isDistrusted)

Let's have a bit more formal and complete documentation of each of these parameters (e.g. describe how 'permitAfterDate` works)

Comment 62

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

Comment on attachment 8952157 [details]
Bug 1434300 - Disable the b-c imminent distrust test

https://reviewboard.mozilla.org/r/221402/#review227638

Comment 63

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

Comment on attachment 8950471 [details]
Bug 1434300 - Implement the Symantec distrust plan from Bug 1409257

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

Comment 64

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

Comment on attachment 8952157 [details]
Bug 1434300 - Disable the b-c imminent distrust test

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

Comment 65

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

Comment on attachment 8950817 [details]
Bug 1434300 - Add a utility to match certificates based on SPKI

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

Comment 66

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

Comment on attachment 8950818 [details]
Bug 1434300 - Change Symantec Distrust Algorithm's whitelist to SPKI-matching

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

Comment 67

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

Comment on attachment 8952156 [details]
Bug 1434300 - Add the DigiCert whitelisted SPKIs

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

Comment 68

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/603b92a0ebd5
Update Imminent Distrust status for future Symantec sanctions r=fkiefer,keeler
https://hg.mozilla.org/integration/autoland/rev/d8517bfe9eb2
Implement the Symantec distrust plan from Bug 1409257 r=fkiefer,keeler
https://hg.mozilla.org/integration/autoland/rev/ea372af453ef
Disable the b-c imminent distrust test r=keeler
https://hg.mozilla.org/integration/autoland/rev/61b168663a54
Add a utility to match certificates based on SPKI r=fkiefer,keeler
https://hg.mozilla.org/integration/autoland/rev/73a952303cae
Change Symantec Distrust Algorithm's whitelist to SPKI-matching r=fkiefer,keeler
https://hg.mozilla.org/integration/autoland/rev/23485791d3e1
Add the DigiCert whitelisted SPKIs r=keeler

Comment 69

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

Backed out for frequent GTest in AllocReplacement.malloc_check:

https://hg.mozilla.org/integration/autoland/rev/dece9cd799b531d92fb7afaca547bc7d5d73fcf6

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=23485791d3e188f55f1905e0fe95a33e0556cddd&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=163306393&repo=autoland

[task 2018-02-20T22:14:23.444Z] 22:14:23     INFO -  TEST-START | AllocReplacement.malloc_check
[task 2018-02-20T22:14:23.444Z] 22:14:23  WARNING -  TEST-UNEXPECTED-FAIL | AllocReplacement.malloc_check | Value of: (ValidateHookedAllocation([] { return malloc(kAllocAmount); }, free))
[task 2018-02-20T22:14:23.445Z] 22:14:23     INFO -    Actual: false
[task 2018-02-20T22:14:23.445Z] 22:14:23     INFO -  Expected: true @ /builds/worker/workspace/build/src/xpcom/tests/gtest/TestAllocReplacement.cpp:113
[task 2018-02-20T22:14:23.445Z] 22:14:23  WARNING -  TEST-UNEXPECTED-FAIL | AllocReplacement.malloc_check | test completed (time: 0ms)
[task 2018-02-20T22:14:23.446Z] 22:14:23     INFO -  TEST-START | AllocReplacement.calloc_check
[task 2018-02-20T22:14:23.447Z] 22:14:23  WARNING -  TEST-UNEXPECTED-FAIL | AllocReplacement.calloc_check | Value of: (ValidateHookedAllocation([] { return calloc(1, kAllocAmount); }, free))
[task 2018-02-20T22:14:23.447Z] 22:14:23     INFO -    Actual: false
[task 2018-02-20T22:14:23.447Z] 22:14:23     INFO -  Expected: true @ /builds/worker/workspace/build/src/xpcom/tests/gtest/TestAllocReplacement.cpp:120
[task 2018-02-20T22:14:23.447Z] 22:14:23  WARNING -  TEST-UNEXPECTED-FAIL | AllocReplacement.calloc_check | test completed (time: 0ms)
[task 2018-02-20T22:14:23.448Z] 22:14:23     INFO -  TEST-START | AllocReplacement.realloc_check
[task 2018-02-20T22:14:23.448Z] 22:14:23     INFO -  TEST-PASS | AllocReplacement.realloc_check | test completed (time: 0ms)
[task 2018-02-20T22:14:23.449Z] 22:14:23     INFO -  TEST-START | AllocReplacement.posix_memalign_check
[task 2018-02-20T22:14:23.449Z] 22:14:23  WARNING -  TEST-UNEXPECTED-FAIL | AllocReplacement.posix_memalign_check | Value of: (ValidateHookedAllocation([] { void* p = nullptr; int result = posix_memalign(&p, sizeof(void*), kAllocAmount); if (result != 0) { return static_cast<void*>(nullptr); } return p; }, free))
[task 2018-02-20T22:14:23.449Z] 22:14:23     INFO -    Actual: false
[task 2018-02-20T22:14:23.450Z] 22:14:23     INFO -  Expected: true @ /builds/worker/workspace/build/src/xpcom/tests/gtest/TestAllocReplacement.cpp:140
[task 2018-02-20T22:14:23.450Z] 22:14:23  WARNING -  TEST-UNEXPECTED-FAIL | AllocReplacement.posix_memalign_check | test completed (time: 0ms)

Comment 70

[Tim Taubert [:ttaubert] (inactive)]

Looks like this is similar to:

https://bugzilla.mozilla.org/show_bug.cgi?id=1433015#c6

I can reproduce it with a debug build locally, with only the first four patches applied. The new GTest runs before and messes the malloc stats up somehow. To reproduce:

mach gtest "*TrustOverride*:*AllocReplacement*"

Comment 71

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

The TrustOverrideTest is running afoul of Bug 1433015, so I'm going to move it into its own bug depending on 1433015 and we'll reland this.

Comment 72

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

Comment on attachment 8950470 [details]
Bug 1434300 - Update Imminent Distrust status for future Symantec sanctions

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

Comment 73

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

Comment on attachment 8950471 [details]
Bug 1434300 - Implement the Symantec distrust plan from Bug 1409257

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

Comment 74

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

Comment on attachment 8952157 [details]
Bug 1434300 - Disable the b-c imminent distrust test

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

Comment 75

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

Comment on attachment 8950817 [details]
Bug 1434300 - Add a utility to match certificates based on SPKI

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

Comment 76

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

Comment on attachment 8950818 [details]
Bug 1434300 - Change Symantec Distrust Algorithm's whitelist to SPKI-matching

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

Comment 77

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

Comment on attachment 8952156 [details]
Bug 1434300 - Add the DigiCert whitelisted SPKIs

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

Comment 78

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=65e7044c1ed667b08937f5a41c6bfb6fe005749a

Comment 79

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/301e943a2b4d
Update Imminent Distrust status for future Symantec sanctions r=fkiefer,keeler
https://hg.mozilla.org/integration/autoland/rev/23ad5230b111
Implement the Symantec distrust plan from Bug 1409257 r=fkiefer,keeler
https://hg.mozilla.org/integration/autoland/rev/ae7617faf7f7
Disable the b-c imminent distrust test r=keeler
https://hg.mozilla.org/integration/autoland/rev/09e82ace662c
Add a utility to match certificates based on SPKI r=fkiefer,keeler
https://hg.mozilla.org/integration/autoland/rev/1252e0f10727
Change Symantec Distrust Algorithm's whitelist to SPKI-matching r=fkiefer,keeler
https://hg.mozilla.org/integration/autoland/rev/df46a62ed521
Add the DigiCert whitelisted SPKIs r=keeler

Comment 80

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/301e943a2b4d
https://hg.mozilla.org/mozilla-central/rev/23ad5230b111
https://hg.mozilla.org/mozilla-central/rev/ae7617faf7f7
https://hg.mozilla.org/mozilla-central/rev/09e82ace662c
https://hg.mozilla.org/mozilla-central/rev/1252e0f10727
https://hg.mozilla.org/mozilla-central/rev/df46a62ed521

Comment 81

[Christiane Ruetten [INACTIVE]]

Here's a list of SSL hosts broken in current Nightly, generated by running a TLS Canary regression scan against the first 10k SSL hosts in the Umbrella top list. The leading number is the hosts's Umbrella rank. That's 292/10000 hosts of Symantec fallout, so almost 3% at this point:

2,api-global.netflix.com
4,ichnaea.netflix.com
30,secure.netflix.com
55,customerevents.netflix.com
63,icloud.com
252,configuration.apple.com
295,spotxchange.com
296,search.spotxchange.com
312,dt.adsafeprotected.com
322,odr.mookie1.com
360,pixel.adsafeprotected.com
442,static.adsafeprotected.com
451,appsflyer.com
483,360yield.com
507,gspe35-ssl.ls.apple.com
536,fw.adsafeprotected.com
548,netmng.com
602,t.appsflyer.com
608,gspe1-ssl.ls.apple.com
632,t.mookie1.com
682,cl2.apple.com
692,tlg.mookie1.com
701,servedby.flashtalking.com
716,uiboot.netflix.com
807,stats.appsflyer.com
933,cl4.apple.com
1016,cl5.apple.com
1054,jivox.com
1262,events.appsflyer.com
1283,e.visualdna.com
1335,synch.optimatic.com
1581,mobileadtrading.com
1663,track-east.mobileadtrading.com
1762,sync.jivox.com
1839,pandora.com
1859,gfe.nvidia.com
1916,cl3.apple.com
2033,pl.yumenetworks.com
2094,pxl.jivox.com
2167,csm2waycm-atl.netmng.com
2286,gmtdmp.mookie1.com
2331,pubmatic2waycm-atl.netmng.com
2421,s.btstatic.com
2539,yume.com
2582,pool.admedo.com
2594,www.pandora.com
2620,delivery.optimatic.com
2851,vast.optimatic.com
2954,rb.adnxs.com
2980,rudy.adsnative.com
3031,lijit2waycm.netmng.com
3036,rtb.optimatic.com
3063,events.gfe.nvidia.com
3064,mediaserver-cont-sv5-1-v4v6.pandora.com
3065,tracking.optimatic.com
3082,mediaserver-cont-sv5-2-v4v6.pandora.com
3094,mediaserver-cont-ch1-1-v4v6.pandora.com
3100,mediaserver-cont-dc6-2-v4v6.pandora.com
3115,mediaserver-cont-dc6-1-v4v6.pandora.com
3119,mediaserver-cont-ch1-2-v4v6.pandora.com
3152,fdz.flashtalking.com
3225,mediaserver-cont-sv5-3-v4v6.pandora.com
3340,gspe21-ssl.ls.apple.com
3382,spotx.tv
3393,rc2waycm-atl.netmng.com
3408,opx2waycm-atl.netmng.com
3409,js.spotx.tv
3497,fwapi.adsafeprotected.com
3519,fm.flashtalking.com
3563,pool.adizio.com
3607,audio-sv5-t1-1-v4v6.pandora.com
3630,audio-sv5-t1-2-v4v6.pandora.com
3661,audio-dc6-t1-2-v4v6.pandora.com
3665,audio-ch1-t1-2-v4v6.pandora.com
3678,audio-ch1-t1-1-v4v6.pandora.com
3709,audio-dc6-t1-1-v4v6.pandora.com
3780,ntracking.optimatic.com
3831,adserver.pandora.com
3964,as.jivox.com
3985,mobile.adsafeprotected.com
4043,engine.4dsply.com
4050,api.adsnative.com
4088,myway.com
4170,tuner.pandora.com
4173,dc-storm.com
4364,search.namequery.com
4624,verizonwireless.com
4643,bluekai2waycm.netmng.com
4691,spixel.adsafeprotected.com
4827,app.appsflyer.com
4843,mobile-static.adsafeprotected.com
4856,argo.svcmot.com
4894,static.adsnative.com
4912,playercdn.jivox.com
5046,pixel.bilinmedia.net
5065,syncva.vzmessages.com
5102,pm.adsafeprotected.com
5212,cdn.adsafeprotected.com
5369,cdn.avazutracking.net
5518,buy.itunes.apple.com
5532,media0.giphy.com
5596,media1.giphy.com
5597,media3.giphy.com
5661,cdn.engine.4dsply.com
5678,media2.giphy.com
5945,mm.melia.com
5947,dts.ushareit.com
5982,p7-buy.itunes.apple.com
6069,zdbb.net
6091,j2waycm.netmng.com
6146,p17-buy.itunes.apple.com
6167,p3-buy.itunes.apple.com
6210,p19-buy.itunes.apple.com
6230,p12-buy.itunes.apple.com
6242,mbis.vzmessages.com
6245,evs.jivox.com
6248,eu-gmtdmp.gd1.mookie1.com
6279,p24-buy.itunes.apple.com
6317,audio-sv5-t2-1-v4v6.pandora.com
6323,p20-buy.itunes.apple.com
6411,p9-buy.itunes.apple.com
6436,audio-sv5-t2-2-v4v6.pandora.com
6456,p14-buy.itunes.apple.com
6466,glympse.com
6485,api.glympse.com
6548,p11-buy.itunes.apple.com
6549,eyeota2waycm.netmng.com
6553,sumologic.com
6568,p23-buy.itunes.apple.com
6580,p18-buy.itunes.apple.com
6583,nyt2.dc-storm.com
6587,p40-buy.itunes.apple.com
6592,p32-buy.itunes.apple.com
6601,p45-buy.itunes.apple.com
6620,track.appsflyer.com
6647,p22-buy.itunes.apple.com
6672,impression.appsflyer.com
6687,p8-buy.itunes.apple.com
6727,p26-buy.itunes.apple.com
6730,j2waycm-us-wdc.netmng.com
6734,p42-buy.itunes.apple.com
6801,p16-buy.itunes.apple.com
6839,p41-buy.itunes.apple.com
6859,sandbox.itunes.apple.com
6873,p10-buy.itunes.apple.com
6881,mediaforge.com
6884,p27-buy.itunes.apple.com
6957,p21-buy.itunes.apple.com
6976,audio-dc6-t2-1-v4v6.pandora.com
6997,p35-buy.itunes.apple.com
7000,p25-buy.itunes.apple.com
7003,audio-dc6-t2-2-v4v6.pandora.com
7009,p2-buy.itunes.apple.com
7027,bevo-us-east-1.adsnative.com
7056,p6-buy.itunes.apple.com
7119,p50-buy.itunes.apple.com
7140,p46-buy.itunes.apple.com
7177,api-v.vzmessages.com
7190,p49-buy.itunes.apple.com
7192,intljs.rmtag.com
7196,p15-buy.itunes.apple.com
7198,embed.ly
7217,p44-buy.itunes.apple.com
7242,p48-buy.itunes.apple.com
7253,wondershare.com
7336,p36-buy.itunes.apple.com
7345,p33-buy.itunes.apple.com
7401,audio-ch1-t2-2-v4v6.pandora.com
7411,audio-ch1-t2-1-v4v6.pandora.com
7413,cdn.jivox.com
7414,p47-buy.itunes.apple.com
7484,p43-buy.itunes.apple.com
7485,geistm.com
7574,spectrum.com
7621,us-gmtdmp.mookie1.com
7656,clashofclans.com
7672,p29-buy.itunes.apple.com
7681,espn.com.br
7701,p38-buy.itunes.apple.com
7725,p55-buy.itunes.apple.com
7772,p30-buy.itunes.apple.com
7829,cm.ushareit.com
7860,p28-buy.itunes.apple.com
7895,endpoint2.collection.sumologic.com
7907,p53-buy.itunes.apple.com
7916,p52-buy.itunes.apple.com
7944,p31-buy.itunes.apple.com
7994,p71-buy.itunes.apple.com
8051,imsns.cequintvzwidml.com
8220,app.box.com
8338,cl1.apple.com
8366,w.visualdna.com
8486,p37-buy.itunes.apple.com
8604,p54-buy.itunes.apple.com
8682,ev.visualdna.com
8704,p70-buy.itunes.apple.com
8775,p56-buy.itunes.apple.com
8789,messaging-notifications.api.nytimes.com
8941,one.pandora.com
9008,alooma.com
9030,p51-buy.itunes.apple.com
9306,p34-buy.itunes.apple.com
9336,p58-buy.itunes.apple.com
9369,assetscdn.jivox.com
9524,p59-buy.itunes.apple.com
9566,mail.globo.com
9619,cf.ushareit.com
9637,peakgames.net
9651,p73-buy.itunes.apple.com
9665,p57-buy.itunes.apple.com
9680,query.hicloud.com
9684,elb.flashtalking.com
9728,p60-buy.itunes.apple.com
9903,lemonpi.io
9908,pixel.ad
9992,tremor2waycm-atl.netmng.com
10032,p72-buy.itunes.apple.com
10037,logicnow.us
10054,d.lemonpi.io
10110,insight.ucweb.com
10156,presentationtracking.netflix.com
10163,track-west.mobileadtrading.com
10177,ox-d.justpremium.com
10222,liveramp2waycm-atl.netmng.com
10224,centro.pixel.ad
10320,3gimg.qq.com
10342,api-cache.adsnative.com
10364,fsr.lenovomm.com
10414,res.wx.qq.com
10415,tagcommander.com
10503,api.peakgames.net
10572,inputs.alooma.com
10608,register.appsflyer.com
10774,mediaiqdigital.com
11023,gb-gmtdmp.mookie1.com
11083,nypi.dc-storm.com
11089,p39-buy.itunes.apple.com
11390,internet.sony.tv
11464,api.embed.ly
11793,rec.api.nytimes.com
11798,tags.mediaforge.com
11800,stats.mediaforge.com
11813,samsungresources.visionobjects.com
11820,adups.com
11828,vivo.com.cn
11834,mg-bid.optimatic.com
11913,dc3-vault.myvzw.com
11927,metro.co.uk
12144,s.zkcdn.net
12206,j2waycm-us-sjc.netmng.com
12298,m.qpic.cn
12339,ios.bugly.qq.com
12480,connect.qq.com
12586,cdn.adsnative.com
12974,bl.ecbsn.com
12981,cdn.embed.ly
13064,nan.netmng.com
13357,tracking.freckleinc.com
13516,goal.com
13687,myharmony.com
13699,polyvore.com
13865,gcm.netmng.com
13915,tbapi.search.ask.com
14005,bevo-eu-west-1.adsnative.com
14027,inbox.clashofclans.com
14047,geoip.newsdev.nytimes.com
14077,fdzcf.flashtalking.com
14122,la4-c2-chi.salesforceliveagent.com
14127,reading-list.api.nytimes.com
14161,cdn.alooma.com
14364,mx-gmtdmp.mookie1.com
14547,a-us-sjc.netmng.com
14551,avery-us-west-2-svc.logicnow.us
14571,markets.on.nytimes.com
14617,d.la4-c2-chi.salesforceliveagent.com
14638,syncor.vzmessages.com
14683,youdao.com
14749,it-gmtdmp.mookie1.com
14853,content.api.nytimes.com
14905,na.gmtdmp.com
14908,cdn01.boxcdn.net
14958,dispatcher.360in.com
14981,svcs.myharmony.com
15082,www.polyvore.com
15134,pgdt.gtimg.cn
15265,hertz.com
15340,hyprmx.com
15610,de-gmtdmp.mookie1.com
15665,exp.360in.com
15691,ucus.ucweb.com
15696,api.appsflyer.com
15837,i.gtimg.cn
15839,m.spotx.tv
15967,www.metro.co.uk
16027,abs.vzmessages.com
16185,luxup.ru
16464,mediabrix.com

Comment 82

[Christiane Ruetten [INACTIVE]]

The fallout list broken down to affected domains:

360in.com
360yield.com
4dsply.com
adizio.com
admedo.com
adnxs.com
adsafeprotected.com
adsnative.com
adups.com
alooma.com
apple.com
appsflyer.com
ask.com
avazutracking.net
bilinmedia.net
box.com
boxcdn.net
btstatic.com
cequintvzwidml.com
clashofclans.com
dc-storm.com
embed.ly
espn.com.br
flashtalking.com
freckleinc.com
geistm.com
giphy.com
globo.com
glympse.com
gmtdmp.com
goal.com
gtimg.cn
hertz.com
hicloud.com
hyprmx.com
icloud.com
jivox.com
justpremium.com
lemonpi.io
lenovomm.com
logicnow.us
luxup.ru
mediabrix.com
mediaforge.com
mediaiqdigital.com
melia.com
metro.co.uk
mobileadtrading.com
mookie1.com
myharmony.com
myvzw.com
myway.com
namequery.com
netflix.com
netmng.com
nvidia.com
nytimes.com
optimatic.com
pandora.com
peakgames.net
pixel.ad
polyvore.com
qpic.cn
qq.com
rmtag.com
salesforceliveagent.com
sony.tv
spectrum.com
spotx.tv
spotxchange.com
sumologic.com
svcmot.com
tagcommander.com
ucweb.com
ushareit.com
verizonwireless.com
visionobjects.com
visualdna.com
vivo.com.cn
vzmessages.com
wondershare.com
youdao.com
yume.com
yumenetworks.com
zdbb.net
zkcdn.net

Comment 83

[Selena Deckelmann :selenamarie :selena]

(In reply to Christiane Ruetten [:cr] from comment #82)
> The fallout list broken down to affected domains:

Thanks for the list, :cr. Could you transform the list into an attachment to this bug? Easier for others to consume/download.

Comment 84

[Christiane Ruetten [INACTIVE]]

Attachment: List of affected hosts among the top 10k SSL Umbrella list

Attached list of affected hosts among the top 10k SSL Umbrella list.

Comment 85

[Christiane Ruetten [INACTIVE]]

Attachment: List of affected domains among the top 10k SSL Umbrella list

Attached list of affected domains among the top 10k SSL Umbrella list.

Comment 86

[Christiane Ruetten [INACTIVE]]

We'll be running a regression scan against the full 1M Umbrella list tonight. The list will be waaay longer and slightly noisier.

Comment 87

[Matt Wobensmith [:mwobensmith][:matt:]]

(In reply to Christiane Ruetten [:cr] from comment #86)
> We'll be running a regression scan against the full 1M Umbrella list
> tonight. The list will be waaay longer and slightly noisier.

I'm doing that now. You just beat me to it. ;)

Comment 88

[Matt Wobensmith [:mwobensmith][:matt:]]

CR and I have been running scans simultaneously. My 10k host scan is here:

https://tlscanary.mozilla.org/runs/2018-02-23-11-56-37

She will likely finish her complete run before I do, so we can wait for her results. Spoiler alert: web is broken. ;)

Comment 89

[Christiane Ruetten [INACTIVE]]

Attachment: JSON log of full regression run of full Umbrella 1M list

Comment 90

[Christiane Ruetten [INACTIVE]]

Attachment: Host list of full Umbrella 1M regression run

Comment 91

[Christiane Ruetten [INACTIVE]]

The full regression against 496833 hosts run yielded 8434 SEC_ERROR_UNKNOWN_ISSUER regressions. The affected roots and their frequencies are (among a few seemingly self-signed roots in the error set that require further investigation):

4846,GeoTrust Global CA
1794,VeriSign Class 3 Public Primary Certification Authority - G5
881,thawte Primary Root CA
443,GeoTrust Primary Certification Authority - G3
207,thawte Primary Root CA - G3
101,GeoTrust Primary Certification Authority
83,VeriSign Universal Root Certification Authority
11,VeriSign Class 3 Public Primary Certification Authority - G3

Comment 92

[Kathleen Wilson]

I'm getting SEC_ERROR_UNKNOWN_ISSUER for evite.com on nightly.
GeoTrust Global CA -> GeoTrust SSL CA - G3 -> *.evite.com

Comment 93

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

Some other domains with GeoTrust certs are also affected. Personally I hit mojeid.cz (informing them now).

Comment 94

[Christiane Ruetten [INACTIVE]]

Before reporting affected roots, or individual domains or hosts, please check whether they are already covered by comment 91 or comment 90.

Comment 95

[Michal Stanke (Mozilla.cz) [:mstanke][:MikkCZ]]

(In reply to Christiane Ruetten [:cr] from comment #94)
> Before reporting affected roots, or individual domains or hosts, please
> check whether they are already covered by comment 91 or comment 90.

In case of mojeid.cz it's GeoTrust Extended Validation SHA256 SSL CA. According to the SSL Labs test results here (https://www.ssllabs.com/ssltest/analyze.html?d=mojeid.cz&s=217.31.205.60&hideResults=on&latest) it actually is a certificate that will become invalid.

Comment 96

[Christiane Ruetten [INACTIVE]]

"www.mojeid.cz" is issued by "GeoTrust Extended Validation SHA256 SSL CA" which is issued by "GeoTrust Primary Certification Authority - G3" which is already on our list. No surprise there.

When a root cert is distrusted, naturally all its intermediates are distrusted as well, and that list can easily be extracted from the run log in comment 89, so no need to report all affected intermediates individually.

Or am I missing your point?

Comment 97

[Julien Vehent]

In addition to :cr's TLS Canary run, I'm scanning the top1m for symantec distrust in the TLS Observatory. It's slower, but has more details.
Current status is posted here, which I'll update as the scan progresses: https://gist.github.com/jvehent/0fbfb71bae06e48163f276592417079a

Comment 98

[Daniel Holbert [:dholbert]]

Note: it seems this bug's effects are now preffed off on trunk, via bug 1437754.  If you want to get the strictness back, you can set about:config pref security.pki.distrust_ca_policy to 1.

Comment 99

[Julien Cristau [:jcristau]]

The strictness will come back in bug 1442075

Comment 100

[Christiane Ruetten [INACTIVE]]

Here's a visualization of the Internet's healing process: http://ec2-34-218-48-154.us-west-2.compute.amazonaws.com:8000/ . We'll probably be running regular TLS Canary scans there for the next few weeks, but after that the link will go away.

Comment 101

[Kohei Yoshino]

Somehow Amazon (at least .com, .ca and .co.jp) uses VeriSign Class 3 Public Primary Certification Authority - G5 instead of their own root but it's not included in the 1M list or even 10K list. Are we probably missing more?

Comment 102

[Wayne Thayer]

The changes in Firefox 60 only affect Symantec certificates issued after 1-June 2016. The certificates I see on amazon.com and amazon.ca were issued in 2017 and will need to be replaced before Firefox 63 is released in October.

Comment 103

[Julien Vehent]

Attachment: symantec_distrusted_domains_sort.txt

TLS Observatory stats from this week, mostly confirming TLS Canary's data and projecting to Fx 63:

Sample size: 542,112 sites from Cisco Umbrella top1m (the rest doesn't support TLS)

Distrusted in Firefox 60: 9,421
               but 2,930 of those certs expire before 60 is released
               so only 6,491 need action (1.19% of TLS sites, 0.64% of all sites)

Distrusted in Firefox 63: 88,537
               but 45,768 of those certs expire before 63 is released
               so only 42,770 need action (7.88% of TLS sites, 4.27% of all sites)

Domains impacted, once subdomains are removed: 17,508

Comment 104

[Christiane Ruetten [INACTIVE]]

Unfortunately last week someone shut down the EC2 instance responsible for scanning and plotting, and it now has a new IP address. You need to use http://ec2-54-244-28-222.us-west-2.compute.amazonaws.com:8000/ from now on.

Comment 105

[Christiane Ruetten [INACTIVE]]

The instance was shut down again. New address is http://ec2-52-88-196-97.us-west-2.compute.amazonaws.com:8000/ .

Comment 106

[Christiane Ruetten [INACTIVE]]

The scanning/plotting instance was shut down again. New address is http://ec2-52-36-248-35.us-west-2.compute.amazonaws.com:8000/ .

Comment 107

[Dimitry Andric]

Interestingly https://ftp.isc.org/ is also broken due to this, whereas https://www.isc.org/ isn't.  Maybe they should be notified... :)

Comment 108

[Christiane Ruetten [INACTIVE]]

The scanning/plotting instance was shut down again. If you still like to follow along the development, its new address is http://ec2-35-167-193-121.us-west-2.compute.amazonaws.com:8000/ .

Comment 109

[Julien Vehent]

I moved the TLS Observatory stats to a spreadsheet where the weekly scans will be progressively added. The chart there also shows the downward trend. https://docs.google.com/spreadsheets/d/1bHc6902vPspec-uzdlHFi9AupCnZcvqYLHnz89Hyh24/edit#gid=0