1151512 - Restrict CNNIC to a whitelist

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Richard Barnes [:rbarnes]]

CNNIC has provided us with a list of currently valid certificates.  Pursuant to our decision on CNNIC after the MCS Holdings mis-issuance, we should update Firefox so that it only accepts certificates from CNNIC that are on this whitelist.

Comment 1

[Richard Barnes [:rbarnes]]

Marking this as sensitive until we have permission to publish the list.

Comment 3

[Richard Barnes [:rbarnes]]

Also: certificates are in DER format, ending in ".cer".

Comment 5

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

Comment on attachment 8589798 [details] [diff]
patch

Review of attachment 8589798 [details] [diff]:
-----------------------------------------------------------------

This code looks very familiar.

I did not review the actual whitelist contents, but I did the Moz::PKIX changes and generator. Everything looks fine here.

Comment 6

[Richard Barnes [:rbarnes]]

Comment on attachment 8589798 [details] [diff]
patch

Review of attachment 8589798 [details] [diff]:
-----------------------------------------------------------------

Overall, I think this is fine.  Couple of minor issues in the comments.  r=me with those addressed.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +25,5 @@
>  #include "ScopedNSSTypes.h"
>  #include "secerr.h"
>  #include "secmod.h"
>  
> +#include "CNNICHashWhitelist.inc"

We should keep an eye on when we can trim / remove this list.  It seems like certificates that are expired can safely be removed from the whitelist; that may be worth doing every so often.  The whitelist can be removed once everything has expired, or once CNNIC has been re-admitted.  It might be worth noting the former date, based on the data available, and adding a comment here about when this #include can be removed.

@@ +727,2 @@
>  Result
>  NSSCertDBTrustDomain::IsChainValid(const DERArray& certArray, Time time)

Just checking here: Is this the right place to add this hook?  I believe the alternative we had discussed was around where GatherSuccessfulValidationTelemetry() is called:

https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/SSLServerCertVerification.cpp#1182

The main drawback of doing it here is that in situations where BuildForward gets called in a fallback configuration, it will run and fail multiple times, instead of succeeding and then hitting this afterward.  For example here:

https://dxr.mozilla.org/mozilla-central/source/security/certverifier/CertVerifier.cpp#126

@@ +736,5 @@
>    if (srv != SECSuccess) {
>      return MapPRErrorCodeToResult(PR_GetError());
>    }
>  
> +  CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);

A comment is probably worthwhile here noting for posterity what we're doing.

@@ +765,5 @@
> +    if (NS_FAILED(nsrv)) {
> +      return Result::FATAL_ERROR_LIBRARY_FAILURE;
> +    }
> +    const uint8_t* certHash(
> +      reinterpret_cast<const uint8_t*>(digest.get().data));

This seems worth breaking the 80-character limit for.

::: security/manager/tools/makeCNNICHashes.js
@@ +5,5 @@
> +// How to run this file:
> +// 1. [obtain firefox source code]
> +// 2. [build/obtain firefox binaries]
> +// 3. run `[path to]/run-mozilla.sh [path to]/xpcshell makeCNNICHashes.js \
> +//                                  [path to]/certlist'

Please note that "certlist" should have a list of paths of certs to be included in the whitelist.

Also, it seems like you're missing "[obtain CNNIC certificates to be whitelisted]"?

@@ +22,5 @@
> +"// file, You can obtain one at http://mozilla.org/MPL/2.0/.\n" +
> +"//\n" +
> +"//***************************************************************************\n" +
> +"// This is an automatically generated file. It shouldn't need to be manually\n" +
> +"// edited.\n" +

Note the script that generated it?

@@ +25,5 @@
> +"// This is an automatically generated file. It shouldn't need to be manually\n" +
> +"// edited.\n" +
> +"//***************************************************************************\n" +
> +"\n" +
> +"#define WHITELIST_HASH_LEN 32\n";

CNNIC_WHITELIST_HASH_LEN ?

@@ +73,5 @@
> +  writeString(fos, HEADER);
> +  writeString(fos, PREAMBLE);
> +  certs.forEach(function(cert) {
> +    writeString(fos, "  {\n");
> +    writeString(fos, "    /* " + getLabelForCert(cert) + " */\n");

As discussed earlier today, let's not include domain names.

@@ +115,5 @@
> +    } catch (e) {}
> +    if (!cert) {
> +      cert = gCertDB.constructX509(certData, certData.length);
> +    }
> +    certs.push(cert);

There's no need to whitelist certificates that have already expired; they'll never be checked against the whitelist anyway.  It looks like nsIX509Cert exposes cert.validity.notAfter, so we should check that here before we process the cert.

Comment 7

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

Attachment: patch v1.1

Thanks for the reviews. Carrying over r+.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c94a39913b47

Comment 8

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

Posted in mozilla.dev.security.policy: https://groups.google.com/d/msg/mozilla.dev.security.policy/czwlDNbwHXM/Ks-MwWSfD6sJ

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c94a39913b47

Comment 10

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

Attachment: patch for aurora

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: users won't be protected against potential further improperly-issued certificates from CNNIC or their subsidiaries/partners.
[Describe test coverage new/current, TreeHerder]: nothing directly tests this, but we do have tests to make sure we didn't break certificate verification in general (also I verified by hand that this works as expected)
[Risks and why]: low - this should only affect a small fraction of sites
[String/UUID change made/needed]: none

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

David, how far will we want to uplift this?  If the whitelist will need to be updated in the future, will it need to change on all branches?

Comment 12

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

Beta is fairly close to release, so I don't think we want to uplift this as far as that right now (although eventually it would be good to have this on ESR). I don't imagine we'll ever need to update the whitelist (except to remove it entirely), but if we do, we probably would want to change it on as many branches as possible. That said, updating the whitelist is easy and low-risk.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8598794 [details] [diff] [review]
patch for aurora

Approved for uplift to aurora.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/34c32b04a1d8