Report pinning violations by CA

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rbarnes, Assigned: jcj)

Tracking

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Reporter

Description

5 years ago
If a CA in our program is causing pinning violations, it is likely that they are in violation of some of the terms of the program.  We should augment the Telemetry we already collect on pinning violations so that we log 
(1) whether the root CA is in our program, and 
(2) which CAs have caused pinning violations

The privacy impact of this measurement is minimal.  Measurements submitted from an IP address would indicate which CAs a user has encountered, which provides a degree of information about what sites the user has visited.  But given the number of sites covered by each CA (hundreds to many thousands), the level of information revealed is low.
Strangely, I had the same argument trying to count by pinset, which has similar effects. You're welcome to re-argue :)
Reporter

Comment 2

5 years ago
Reporting by root CA (emphasis on *root* -- the one that's already built in) seems different from reporting by pinset that it's worth re-litigating.  

A report of a violation of a specific pinset says that the reporting user visited a specific host.  A report of a violation by a specific CA only indicates that the user visited any pinned site.  In fact, it's arguably no more granular than the general pin violation reporting we're already doing -- only the CA knows the set of sites for which it's issued certificates, so we, the holders of the measurement data, don't gain any information by the (IP, CA) association.

Comment 3

5 years ago
(In reply to Richard Barnes [:rbarnes] from comment #0)
> If a CA in our program is causing pinning violations, it is likely that they
> are in violation of some of the terms of the program.

Richard, I don't understand how a CA could ever be found to be "_causing_ pinning violations", since AFAIK CAs aren't expected to interact with pins at all.

Are you suggesting that CAs are supposed to be evaluating pins as part of the validation process they perform before issuing a certificate?  (If you are suggesting this, then where is this requirement stated in the Mozilla CA Certificate Policy?)

Thanks.
Flags: needinfo?(rlb)
Reporter

Comment 4

5 years ago
Perhaps "cause" implies to much of a causal link.  A pinning violation reflects a disagreement between two version of reality:

1. The certificate chain
2. The pin

CAs are certainly involved in (1).  The idea of collecting telemetry by CA would be to have some sort of starting point for debugging these violations, as a complement to SSL error reporting (bug 846489).

The major concern is the case where the pin is correct, and the certificate chain is asserting a bogus binding between a key and a domain (i.e., asserting a key not associated with the domain holder).  This case is already forbidden by the BR requirements for verifying authority to use a name (e.g., 11.1.1).  Of course, pins can be incorrect as well.
Flags: needinfo?(rlb)

Comment 5

5 years ago
I agree it's a possible warning something is wrong with the CA ad will be very interesting data.  However, the data won't show conclusively whether the CA mis-issued since the domain operators and the person pinning are not always the same entity.  Inter-department ms-communication is not uncommon, especially among growing businesses, and a domain operator could easily provide domain authorization not realizing a pin was in place.
Reporter

Comment 6

5 years ago
Posted patch bug-1054498.patch (obsolete) — Splinter Review
This patch adds a generic utility for mapping a CERTCeritificate to a histogram bin -- essentially, it gets assigned a number based on where it appears in certdata.txt.  Assuming that that file grows at the tail and not the head/middle, the numbers should stay consistent over time.

It also patches that utility in to PublicKeyPinningService at the same place as other pinning telemetry.
Assignee: nobody → rlb
Status: NEW → ASSIGNED
Attachment #8486020 - Flags: review?(vdjeric)
Attachment #8486020 - Flags: review?(dkeeler)
Comment on attachment 8486020 [details] [diff] [review]
bug-1054498.patch

Review of attachment 8486020 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good start, but I don't think it will work as is.
Particularly:
* How does genRootCAHashes.py deal with adding certificates to the database? (Actually, see the linear search comment.)
* While the generator attempts to filter out certificates that are not actually trusted roots, things like "Explicitly Distrusted Malaysian Digicert Sdn. Bhd." still got added. I recommend re-writing this as an xpcshell script that queries the certificate database and only adds entries for trusted roots.

::: security/manager/boot/src/AccumulateRootCA.h
@@ +2,5 @@
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#ifndef mozilla_psm__AccumulateRootCA_h

nit: I like a blank line after the license boilerplate

@@ +18,5 @@
> +#define ROOT_LIST_BASE  1
> +
> +// Note: New CAs will show up as UNKNOWN_ROOT until
> +// root-hashes.c is updated to include them
> +#include "RootHashes.inc"

nit: update comment or filename to have these match

@@ +20,5 @@
> +// Note: New CAs will show up as UNKNOWN_ROOT until
> +// root-hashes.c is updated to include them
> +#include "RootHashes.inc"
> +
> +static int

nit: int32_t

@@ +21,5 @@
> +// root-hashes.c is updated to include them
> +#include "RootHashes.inc"
> +
> +static int
> +RootCABinNumber(const SECItem* cert)

Anything longer than a few lines really belongs in a .cpp file, so let's go ahead and add one for this. There may be other baseline-requirements/CA-related telemetry utility functions we want to add (for example, gathering BR telemetry might belong in its own file rather than SSLServerCertVerification.cpp).

@@ +28,5 @@
> +  nsresult rv;
> +  nsCOMPtr<nsICryptoHash> hasher = do_CreateInstance("@mozilla.org/security/hash;1", &rv);
> +  if (NS_FAILED(rv)) return UNKNOWN_ROOT;
> +
> +  rv = hasher->Init(nsICryptoHash::SHA1);

This isn't for security, but eventually we'll want to deprecate SHA1 completely, so let's go ahead and use SHA-2 here.

@@ +39,5 @@
> +  rv = hasher->Finish(false, hash);
> +  if (NS_FAILED(rv)) return UNKNOWN_ROOT;
> +
> +  // Compare against list of stored hashes
> +  for (int i = 0; i < ROOT_TABLE_SIZE; ++i) {

Linear search starts losing to binary search around n=100, right? What we could do is assign each root an ID that increases as we add new roots but still store them sorted by hash.

@@ +41,5 @@
> +
> +  // Compare against list of stored hashes
> +  for (int i = 0; i < ROOT_TABLE_SIZE; ++i) {
> +    if (memcmp(ROOT_TABLE[i], hash.get(), hash.Length()) == 0) {
> +      return ROOT_LIST_BASE + i;

Why not just keep with 0-indexing, and have UNKNOWN_ROOT be -1? (see the next comment)

@@ +51,5 @@
> +static void
> +AccumulateRootCA(Telemetry::ID probe, const SECItem* derCert,
> +                 int binsPerRoot = 1, int perRootBin = 0)
> +{
> +  int rawBin = RootCABinNumber(derCert);

If we get UNKNOWN_ROOT, do we really want to accumulate Telemetry for it?

@@ +57,5 @@
> +}
> +
> +static void
> +AccumulateRootCA(Telemetry::ID probe, const CERTCertificate* cert,
> +                 int binsPerRoot=1, int perRootBin=0)

I think we can YAGNI these last two arguments.

::: security/manager/boot/src/PublicKeyPinningService.cpp
@@ +4,5 @@
>  
>  #include "PublicKeyPinningService.h"
>  #include "pkix/nullptr.h"
>  #include "StaticHPKPins.h" // autogenerated by genHPKPStaticpins.js
> +#include "AccumulateRootCA.h"

Technically the entire list of #includes should be sorted/grouped differently, but nbd. (see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#C.2FC.2B.2B_practices "Includes are split...")

@@ +217,5 @@
>        Telemetry::Accumulate(histogram, result ? 1 : 0);
>      }
> +
> +    // We can collect per-CA pinning results in general, because
> +    // they don't reveal private information

We should probably run this by legal/privacy

@@ +218,5 @@
>      }
> +
> +    // We can collect per-CA pinning results in general, because
> +    // they don't reveal private information
> +    CERTCertificate* rootCert = CERT_LIST_TAIL(certList)->cert;

It may be possible for CheckPinsForHostname to be passed an empty certList, so we should check for that before dereferencing.

::: security/manager/boot/src/RootHashes.inc
@@ +9,5 @@
> +
> +/*
> + * Note that the numbers in the table below are offsets from ROOT_TABLE_BASE,
> + * defined in AccumulateRootCA.h.  To find a CA from telemetry, subtract that
> + * value from the telemetry value before looking here.

Isn't this backwards? Otherwise GTE CyberTrust Global Root would be -1...?

@@ +14,5 @@
> + */
> +
> +#define HASH_LEN 20
> +#define ROOT_TABLE_SIZE 183
> +static const uint8_t ROOT_TABLE[184][HASH_LEN] = {

Maybe use ROOT_TABLE_SIZE here? We don't need to null-terminate it if we know the size.

@@ +450,5 @@
> +   0x57, 0xf9, 0x2b, 0xc9, 0xa4, 0xc6, 0x92, 0xe1, 0x42, 0x42},
> +  /* (144) Explicitly Distrusted Malaysian Digicert Sdn. Bhd. (cyb) */
> +  {0x55, 0x50, 0xaf, 0xec, 0xbf, 0xe8, 0xc3, 0xad, 0xc4, 0x0b,
> +   0xe3, 0xad, 0x0c, 0xa7, 0xe4, 0x15, 0x8c, 0x39, 0x59, 0x4f},
> +  /* (145) Explicitly Distrusted Malaysian Digicert Sdn. Bhd. (en) */

This probably shouldn't be here, right?

::: toolkit/components/telemetry/Histograms.json
@@ +6332,5 @@
>      "high": "5000",
>      "n_buckets": 10,
>      "extended_statistics_ok": true
>    },
> +  "CERT_PINNING_CHECKS_BY_CA": {

Why is this one necessary?

@@ +6336,5 @@
> +  "CERT_PINNING_CHECKS_BY_CA": {
> +    "alert_emails": ["pinning@mozilla.org"],
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 500,

Maybe more like 256

@@ +6343,5 @@
> +  "CERT_PINNING_FAILURES_BY_CA": {
> +    "alert_emails": ["pinning@mozilla.org"],
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 500,

same here
Attachment #8486020 - Flags: review?(dkeeler) → review-
Comment on attachment 8486020 [details] [diff] [review]
bug-1054498.patch

Review of attachment 8486020 [details] [diff] [review]:
-----------------------------------------------------------------

I only have a few thoughts on the Telemetry parts of this patch:

- If the ROOT_TABLE array is only used for mapping between CAs and a number in the range (0, 256) for reporting violations etc. to Telemetry, then why not just do an array lookup on the raw SECItemStr->data string instead of bothering with hashing?
- You can't store negative numbers in Telemetry histograms, so if you end up setting UNKNOWN_ROOT to -1, don't also report it to Telemetry :)
- I'd prefer someone from the Privacy team comment on whether it's ok to report CAs used in a session
Attachment #8486020 - Flags: review?(vdjeric)
Assignee

Comment 9

5 years ago
I'm picking up rbarnes' patch. This patchset should resolve all but one of keeler's comments: I haven't converted the Python script to xpcshell, but I will do that next; I just wanted to get a pass on the C++.
Assignee: rlb → jjones
Attachment #8486020 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8505618 - Flags: review?(dkeeler)
Comment on attachment 8505618 [details] [diff] [review]
In-progress continuation of pinning telemetry

Review of attachment 8505618 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, this looks good. I mostly just pointed out style nits. I suggested some changes to how the root hash data is stored, though. I should probably have another look, so r- for now. I didn't review genRootCAHashes.py in expectation that it would be re-implemented as an xpcshell script. Let me know if you have any questions.

::: security/manager/boot/src/AccumulateRootCA.cpp
@@ +9,5 @@
> +#include "prlog.h"
> +
> +using namespace mozilla::psm;
> +
> +

nit: probably only need one blank line here

@@ +15,5 @@
> +PRLogModuleInfo* gPublicKeyPinningTelemetryLog =
> +  PR_NewLogModule("PublicKeyPinningTelemetryService");
> +#endif
> +
> +/**

It's nice to be able to use block comments to comment-out ranges of code, so comments in code should be '//'-style comments

@@ +47,5 @@
> +{
> +  // Compute SHA256 hash of the certificate
> +  nsresult rv;
> +  nsCOMPtr<nsICryptoHash> hasher = do_CreateInstance("@mozilla.org/security/hash;1", &rv);
> +  if (NS_FAILED(rv)) return UNKNOWN_ROOT;

The style for these should be:

if (NS_FAILED(rv)) {
  return UNKNOWN_ROOT;
}

(In general, we don't have one-line ifs and we always include braces around conditional bodies.)

@@ +63,5 @@
> +  // Compare against list of stored hashes
> +  size_t idx;
> +
> +  PR_LOG(gPublicKeyPinningTelemetryLog, PR_LOG_DEBUG,
> +           ("pkpinTelem: First bytes %hu %hu %hu %hu\n",

Don't we want %hhu here? (bytes instead of shorts?) Also, maybe %hhx for hex? Or even %02hhx?
Also, this line should probably be indented 2 less spaces.

@@ +64,5 @@
> +  size_t idx;
> +
> +  PR_LOG(gPublicKeyPinningTelemetryLog, PR_LOG_DEBUG,
> +           ("pkpinTelem: First bytes %hu %hu %hu %hu\n",
> +            hash.get()[0],hash.get()[1],hash.get()[2],hash.get()[3]));

nit: a space each after ','

@@ +66,5 @@
> +  PR_LOG(gPublicKeyPinningTelemetryLog, PR_LOG_DEBUG,
> +           ("pkpinTelem: First bytes %hu %hu %hu %hu\n",
> +            hash.get()[0],hash.get()[1],hash.get()[2],hash.get()[3]));
> +
> +  if (mozilla::BinarySearchIf(ROOT_TABLE, 0, ROOT_TABLE_SIZE, 

nit: watch out for trailing whitespace

@@ +67,5 @@
> +           ("pkpinTelem: First bytes %hu %hu %hu %hu\n",
> +            hash.get()[0],hash.get()[1],hash.get()[2],hash.get()[3]));
> +
> +  if (mozilla::BinarySearchIf(ROOT_TABLE, 0, ROOT_TABLE_SIZE, 
> +                 BinaryHashSearchArrayComparator( (const uint8_t*) hash.get() ), &idx)) {

nit: reinterpret_cast<const uint8_t*>(hash.get()) would be preferred here, I think
Also, no need for spaces after/before (/)

@@ +74,5 @@
> +            idx, ROOT_TABLE_BIN_LOOKUP[idx]));
> +    return (int32_t) ROOT_TABLE_BIN_LOOKUP[idx];
> +  }
> +
> +  // Didn't match. 

nit: trailing whitespace

@@ +87,5 @@
> +mozilla::psm::AccumulateRootCA(mozilla::Telemetry::ID probe, const SECItem* derCert)
> +{
> +  int32_t binId = RootCABinNumber(derCert);
> +
> +  if (UNKNOWN_ROOT != binId) {

nit: in most of PSM, the style for these comparisons is 'if (binID != UNKNOWN_ROOT)'

@@ +96,5 @@
> +/**
> + Convenience method.
> +*/
> +static void
> +mozilla::psm::AccumulateRootCA(mozilla::Telemetry::ID probe, const CERTCertificate* cert)

I'm not sure this method is useful enough to warrant its own definition.

::: security/manager/boot/src/AccumulateRootCA.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_psm__AccumulateRootCA_h

Technically since the #include path would be #include "AccumulateRootCA.h", I think the guard would just be AccumulateRootCA_h.
Also, maybe this file should be called something more like RootCertificateTelemetryUtils.h?

@@ +9,5 @@
> +
> +#include "certt.h"
> +#include "nsComponentManagerUtils.h"
> +#include "nsICryptoHash.h"
> +#include "mozilla/Telemetry.h"

nit: m sorts before n, right?
(And, actually, most of these #includes aren't necessary.)

@@ +14,5 @@
> +
> +namespace mozilla { namespace psm {
> +
> +// Some special values
> +#define UNKNOWN_ROOT    -1

This probably doesn't need to be in the header file.

@@ +18,5 @@
> +#define UNKNOWN_ROOT    -1
> +
> +// Note: New CAs will show up as UNKNOWN_ROOT until
> +// RootHashes.inc is updated to include them
> +#include "RootHashes.inc"

Same here.

@@ +21,5 @@
> +// RootHashes.inc is updated to include them
> +#include "RootHashes.inc"
> +
> +static int32_t
> +RootCABinNumber(const SECItem* cert);

Same.

@@ +24,5 @@
> +static int32_t
> +RootCABinNumber(const SECItem* cert);
> +
> +static void
> +AccumulateRootCA(mozilla::Telemetry::ID probe, const SECItem* derCert);

Maybe something like 'AccumulateTelemetryForRoot'?

::: security/manager/boot/src/PublicKeyPinningService.cpp
@@ +273,5 @@
> +
> +    // We can collect per-CA pinning results in general, because
> +    // they don't reveal private information
> +    CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);
> +    if (! CERT_LIST_END(rootNode, certList)) {

nit: no space after '!'

::: security/manager/boot/src/RootHashes.inc
@@ +9,5 @@
> +
> +
> +#define HASH_LEN 32
> +#define ROOT_TABLE_SIZE 175
> +static const uint8_t ROOT_TABLE[ROOT_TABLE_SIZE][HASH_LEN] = {

Rather than having a #define ROOT_TABLE_SIZE, I think it would be best to use ArrayLength, since we know the length of these arrays at compile-time. That way, we can do things like assert(ArrayLength(ROOT_TABLE) == ArrayLength(ROOT_TABLE_BIN_LOOKUP))
Also, I think it would be best to avoid 2-dimensional arrays of bytes. We can define a utility struct CertHash { const uint8_t hash[HASH_LEN]; } and have ROOT_TABLE be an array of CertHash or something. And, in fact, that would avoid needing two arrays, right? Since we could just include the id in the CertHash struct.
Attachment #8505618 - Flags: review?(dkeeler) → review-
Comment on attachment 8505618 [details] [diff] [review]
In-progress continuation of pinning telemetry

Review of attachment 8505618 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/Histograms.json
@@ +6555,4 @@
>      "n_buckets": 10,
>      "extended_statistics_ok": true
>    },
> +  "CERT_PINNING_CHECKS_BY_CA": {

Oh, I forgot to mention - I'm not sure it's useful and privacy-conserving to have a count of what CAs were involved in successful pins.
Assignee

Comment 12

5 years ago
Posted patch Review updates (obsolete) — Splinter Review
:keeler, thanks for the review! I've resolved all of your nits, restructured the .inc data, took out the telemetry for non-failure pins, converted the Python script to xpcshell, and resolved (I think) the bin number longevity.

I'm particularly concerned about the xpcshell just because I'm brand new to XPCOM. I pretty much looked for examples of others' uses of CertDB (etc), so hopefully I haven't veered too far from course.

Thanks for pointing me in the right direction, keeler!
Attachment #8505618 - Attachment is obsolete: true
Attachment #8505967 - Flags: review?(dkeeler)
Comment on attachment 8505967 [details] [diff] [review]
Review updates

Review of attachment 8505967 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great for a first patch.

::: security/manager/boot/src/RootCertificateTelemetryUtils.cpp
@@ +47,5 @@
> +RootCABinNumber(const SECItem* cert)
> +{
> +  // Compute SHA256 hash of the certificate
> +  nsresult rv;
> +  nsCOMPtr<nsICryptoHash> hasher = do_CreateInstance("@mozilla.org/security/hash;1", &rv);

nsICryptoHash is not threadsafe, new uses are discouraged. I think you're supposed to use PK11_CreateDigestContext(static_cast<SECOidTag>(SEC_OID_foo) and friends from NSS instead. There may be helper functions that I don't know about by now.

::: security/manager/tools/KnownRootHashes.json
@@ +2,5 @@
> +	"roots": [
> +		{
> +			"label": "GTE_CyberTrust_Global_Root",
> +			"binNumber": 1,
> +			"fingerprint": "A5:31:25:18:8D:21:10:AA:96:4B:02:C7:B7:C6:DA:32:03:17:08:94:E5:FB:71:FF:FB:66:67:D5:E6:81:0A:36"

This should probably be base 64 encoded to be consistent with fingerprints in other files.
Assignee

Comment 14

5 years ago
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #13)
> Comment on attachment 8505967 [details] [diff] [review]
> Review updates
> 
> Review of attachment 8505967 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great for a first patch.
> 
> ::: security/manager/boot/src/RootCertificateTelemetryUtils.cpp
> @@ +47,5 @@
> > +RootCABinNumber(const SECItem* cert)
> > +{
> > +  // Compute SHA256 hash of the certificate
> > +  nsresult rv;
> > +  nsCOMPtr<nsICryptoHash> hasher = do_CreateInstance("@mozilla.org/security/hash;1", &rv);
> 
> nsICryptoHash is not threadsafe, new uses are discouraged. I think you're
> supposed to use PK11_CreateDigestContext(static_cast<SECOidTag>(SEC_OID_foo)
> and friends from NSS instead. There may be helper functions that I don't
> know about by now.

I'll take a look.

> ::: security/manager/tools/KnownRootHashes.json
> @@ +2,5 @@
> > +	"roots": [
> > +		{
> > +			"label": "GTE_CyberTrust_Global_Root",
> > +			"binNumber": 1,
> > +			"fingerprint": "A5:31:25:18:8D:21:10:AA:96:4B:02:C7:B7:C6:DA:32:03:17:08:94:E5:FB:71:FF:FB:66:67:D5:E6:81:0A:36"
> 
> This should probably be base 64 encoded to be consistent with fingerprints
> in other files.

Ah, OK. I was also thinking I should go through the trouble of serializing some comments into that file, so I'll do that at the same time.
Reporter

Comment 15

5 years ago
(In reply to J.C. Jones [:jcj] from comment #14)
> (In reply to [:mmc] Monica Chew (please use needinfo) from comment #13)
> > Comment on attachment 8505967 [details] [diff] [review]
> > Review updates
> > 
> > Review of attachment 8505967 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks great for a first patch.
> > 
> > ::: security/manager/boot/src/RootCertificateTelemetryUtils.cpp
> > @@ +47,5 @@
> > > +RootCABinNumber(const SECItem* cert)
> > > +{
> > > +  // Compute SHA256 hash of the certificate
> > > +  nsresult rv;
> > > +  nsCOMPtr<nsICryptoHash> hasher = do_CreateInstance("@mozilla.org/security/hash;1", &rv);
> > 
> > nsICryptoHash is not threadsafe, new uses are discouraged. I think you're
> > supposed to use PK11_CreateDigestContext(static_cast<SECOidTag>(SEC_OID_foo)
> > and friends from NSS instead. There may be helper functions that I don't
> > know about by now.
> 
> I'll take a look.

PK11_HashBuf?

http://dxr.mozilla.org/mozilla-central/source/dom/crypto/WebCryptoTask.cpp#1129

> 
> > ::: security/manager/tools/KnownRootHashes.json
> > @@ +2,5 @@
> > > +	"roots": [
> > > +		{
> > > +			"label": "GTE_CyberTrust_Global_Root",
> > > +			"binNumber": 1,
> > > +			"fingerprint": "A5:31:25:18:8D:21:10:AA:96:4B:02:C7:B7:C6:DA:32:03:17:08:94:E5:FB:71:FF:FB:66:67:D5:E6:81:0A:36"
> > 
> > This should probably be base 64 encoded to be consistent with fingerprints
> > in other files.
> 
> Ah, OK. I was also thinking I should go through the trouble of serializing
> some comments into that file, so I'll do that at the same time.
Assignee

Comment 16

5 years ago
Posted patch bug_1054498 (obsolete) — Splinter Review
Addressing mmc's comments: 

(runtime code)
* Changed the implementation to use DigestBuf.
* Added an assertion for hash comparisons with length mismatches to BinaryHashSearchArrayComparator


(xpcshell script)
* Storing root CA state fingerprints as Base64 values in the JSON file
* Added comments to not manually change the JSON file
* Updated the JSON file to match; the RootHashes.inc did not change
Attachment #8505967 - Attachment is obsolete: true
Attachment #8505967 - Flags: review?(dkeeler)
Attachment #8506424 - Flags: review?(dkeeler)
Reporter

Comment 18

5 years ago
Comment on attachment 8506424 [details] [diff] [review]
bug_1054498

Review of attachment 8506424 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/boot/src/PublicKeyPinningService.cpp
@@ +277,5 @@
> +    if (! CERT_LIST_END(rootNode, certList)) {
> +      // Only log telemetry if the certificate list is non-empty.
> +      if (!result) {
> +        // Log failure conditions specially.
> +        AccumulateTelemetryForRootCA(Telemetry::CERT_PINNING_FAILURES_BY_CA, rootNode->cert);

Telemetry::Accumulate(Telemetry::CERT_PINNING_FAILURES_BY_CA,
                      RootCABinNumber(rootNode->cert));

::: security/manager/boot/src/RootCertificateTelemetryUtils.cpp
@@ +10,5 @@
> +#include "RootHashes.inc"
> +
> +// Note: New CAs will show up as UNKNOWN_ROOT until
> +// RootHashes.inc is updated to include them
> +#define UNKNOWN_ROOT    -1

Make this non-negative so we can measure it.

@@ +12,5 @@
> +// Note: New CAs will show up as UNKNOWN_ROOT until
> +// RootHashes.inc is updated to include them
> +#define UNKNOWN_ROOT    -1
> +
> +using namespace mozilla::psm;

namespace mozilla { namespace psm { 

(And corresponding close at the end

@@ +72,5 @@
> +           idx, ROOT_TABLE[idx].binNumber));
> +    return (int32_t) ROOT_TABLE[idx].binNumber;
> +  }
> +
> +  // Didn't match. 

Nit: Whitespace at end of line

@@ +80,5 @@
> +
> +// Attempt to increment the appropriate bin in the provided Telemetry probe ID. If
> +// the Root CA doesn't have its own bin, we do nothing.
> +static void
> +mozilla::psm::AccumulateTelemetryForRootCA(mozilla::Telemetry::ID probe, const CERTCertificate* cert)

mozilla::psm:: prefix shouldn't be necessary, if you fix the namespace stuff up top.

@@ +84,5 @@
> +mozilla::psm::AccumulateTelemetryForRootCA(mozilla::Telemetry::ID probe, const CERTCertificate* cert)
> +{
> +  int32_t binId = RootCABinNumber(&cert->derCert);
> +
> +  if (UNKNOWN_ROOT != binId) {

We should also be measuring when we get a pin violation with a non-built-in CA.  And given that, this method seems kind of unnecessary -- just expose RootCABinNumber and make the Accumulate call in the calling code.  And then maybe rename to RootCABinNumber.cpp/h.

@@ +85,5 @@
> +{
> +  int32_t binId = RootCABinNumber(&cert->derCert);
> +
> +  if (UNKNOWN_ROOT != binId) {
> +    mozilla::Telemetry::Accumulate(probe, binId);

mozilla:: prefix shouldn't be necessary, if you fix the namespace stuff up top.

::: security/manager/tools/KnownRootHashes.json
@@ +7,5 @@
> +// runs of genRootCAHashes.js; you should never need to manually edit it
> +//***************************************************************************
> +
> +{
> +	"roots": [

Nit: So many spaces!  Can we do a 2-space indent?
Comment on attachment 8506424 [details] [diff] [review]
bug_1054498

Review of attachment 8506424 [details] [diff] [review]:
-----------------------------------------------------------------

Great! I just pointed out a number of style nits, really. r=me with comments addressed.
(Looks like I commented on a lot of things Richard also commented on, but oh well.)

::: security/manager/boot/src/PublicKeyPinningService.cpp
@@ +17,3 @@
>  #include "pkix/pkixtypes.h"
>  #include "prlog.h"
> +#include "RootCertificateTelemetryUtils.h"

maybe comment // autogenerated by genRootCAHashes.js to be consistent?

@@ +271,5 @@
>        Telemetry::Accumulate(histogram, result ? 1 : 0);
>      }
> +
> +    // We can collect per-CA pinning results in general, because
> +    // they don't reveal private information

nit: update comment to be specific that we only collect information for failures

@@ +273,5 @@
> +
> +    // We can collect per-CA pinning results in general, because
> +    // they don't reveal private information
> +    CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);
> +    if (! CERT_LIST_END(rootNode, certList)) {

nit: don't have a space after the "!" here (unless it fails to compile for some reason otherwise?)

@@ +274,5 @@
> +    // We can collect per-CA pinning results in general, because
> +    // they don't reveal private information
> +    CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);
> +    if (! CERT_LIST_END(rootNode, certList)) {
> +      // Only log telemetry if the certificate list is non-empty.

nit: this comment should probably go above the preceding if

@@ +276,5 @@
> +    CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);
> +    if (! CERT_LIST_END(rootNode, certList)) {
> +      // Only log telemetry if the certificate list is non-empty.
> +      if (!result) {
> +        // Log failure conditions specially.

nit: update comment

::: security/manager/boot/src/RootCertificateTelemetryUtils.cpp
@@ +10,5 @@
> +#include "RootHashes.inc"
> +
> +// Note: New CAs will show up as UNKNOWN_ROOT until
> +// RootHashes.inc is updated to include them
> +#define UNKNOWN_ROOT    -1

nit: we probably only need one space after "UNKNOWN_ROOT" and before "-1"

@@ +33,5 @@
> +  {
> +    NS_ASSERTION(len == HASH_LEN, "Hashes should be of the same length.");
> +  }
> +
> +  int operator()(const CertAuthorityHash val) const {

can this be a reference? (i.e. const CertAuthorityHash& val)

@@ +46,5 @@
> +// structure for a matching bin number.
> +static int32_t
> +RootCABinNumber(const SECItem* cert)
> +{
> +  Digest digest;

So technically if code uses Digest, it's supposed to first ensure that NSS is initialized and that it doesn't shutdown while in use. Since this code is only (currently) called from the pinning checking code, which would only be called if NSS were already initialized and not going to shut down, we're probably fine, but in the future this may not be the case. We should at least comment that this could be an issue.

Actually - better plan: file a bug about how Digest should just ensure that NSS is initialized and won't shut down for its lifetime itself.

@@ +72,5 @@
> +           idx, ROOT_TABLE[idx].binNumber));
> +    return (int32_t) ROOT_TABLE[idx].binNumber;
> +  }
> +
> +  // Didn't match. 

nit: trailing whitespace

@@ +80,5 @@
> +
> +// Attempt to increment the appropriate bin in the provided Telemetry probe ID. If
> +// the Root CA doesn't have its own bin, we do nothing.
> +static void
> +mozilla::psm::AccumulateTelemetryForRootCA(mozilla::Telemetry::ID probe, const CERTCertificate* cert)

Looks like this line is >80 chars - we should do our best to remain under that.

@@ +85,5 @@
> +{
> +  int32_t binId = RootCABinNumber(&cert->derCert);
> +
> +  if (UNKNOWN_ROOT != binId) {
> +    mozilla::Telemetry::Accumulate(probe, binId);

As discussed, if UNKNOWN_ROOT is 0, we can accumulate total pin violations for roots not in our program.

::: security/manager/boot/src/RootCertificateTelemetryUtils.h
@@ +7,5 @@
> +#ifndef RootCertificateTelemetryUtils_h
> +#define RootCertificateTelemetryUtils_h
> +
> +#include "mozilla/Telemetry.h"
> +#include "ScopedNSSTypes.h"

nit: this should probably be "certt.h"

@@ +11,5 @@
> +#include "ScopedNSSTypes.h"
> +
> +namespace mozilla { namespace psm {
> +
> +static void

As we discussed, static shouldn't be necessary here.

::: security/manager/boot/src/RootHashes.inc
@@ +15,5 @@
> +
> +static const struct CertAuthorityHash ROOT_TABLE[] = {
> +  {
> +    /* AffirmTrust_Commercial */
> +    { 0x03, 0x76, 0xAB, 0x1D, 0x54, 0xC5, 0xF9, 0x80, 0x3C, 0xE4, 0xB2, 0xE2, 0x01, 0xA0, 0xEE, 0x7E, 

It would be nice if these lines didn't each have a trailing space.

::: security/manager/tools/KnownRootHashes.json
@@ +7,5 @@
> +// runs of genRootCAHashes.js; you should never need to manually edit it
> +//***************************************************************************
> +
> +{
> +	"roots": [

I think two spaces each would be preferred to tabs for this file.

::: security/manager/tools/genRootCAHashes.js
@@ +6,5 @@
> +// 1. [obtain firefox source code]
> +// 2. [build/obtain firefox binaries]
> +// 3. run `[path to]/run-mozilla.sh [path to]/xpcshell \
> +//                                  [path to]/xpcshell genRootCAHashes.js \
> +//                                   && cp RootHashes.inc ../boot/src/RootHashes.inc'

Maybe instead have the output path be an additional argument? (that way, the extra step wouldn't be necessary)

@@ +15,5 @@
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +const Cr = Components.results;
> +
> +const nsIX509Cert = Components.interfaces.nsIX509Cert;

When Ci has been defined, generally using aliases like this isn't necessary. I would just use Ci.nsIX509Cert / Ci.nsIX509CertDB where appropriate.

@@ +18,5 @@
> +
> +const nsIX509Cert = Components.interfaces.nsIX509Cert;
> +const nsX509CertDB = "@mozilla.org/security/x509certdb;1";
> +const nsIX509CertDB = Components.interfaces.nsIX509CertDB;
> +var CertDb = Components.classes[nsX509CertDB].getService(nsIX509CertDB);

nit: let, not var (although in this case maybe const? Or maybe 'let gCertDB' since it's a global.

@@ +53,5 @@
> +const FP_PREAMBLE = "struct CertAuthorityHash {\n" +
> +" const uint8_t hash[HASH_LEN];\n" +
> +" const int32_t binNumber;\n" +
> +"};\n\n" +
> +"static const struct CertAuthorityHash ROOT_TABLE[] = {\n"

nit: this line should have a final ';'

@@ +55,5 @@
> +" const int32_t binNumber;\n" +
> +"};\n\n" +
> +"static const struct CertAuthorityHash ROOT_TABLE[] = {\n"
> +
> +const FP_POSTAMBLE = "};\n"

same

@@ +70,5 @@
> +
> +// Expect an array of bytes and make it C-formatted
> +function hexSlice(bytes, start, end) {
> +  let ret = "";
> +  for (let i=start; i<end; i++) {

nit: spaces around operators: i = start; i < end; (but, strangely, i++ is fine)

@@ +73,5 @@
> +  let ret = "";
> +  for (let i=start; i<end; i++) {
> +    let hex = (0 + bytes.charCodeAt(i).toString(16)).slice(-2).toUpperCase();
> +    ret += "0x" + hex;
> +    if (i<end) ret += ", ";

nit: spaces around operators: "(i < end)"
Also, let's avoid one-line if-statements:

if (i < end) {
  ret += ", ";
}

@@ +77,5 @@
> +    if (i<end) ret += ", ";
> +  }
> +  return ret;
> +}
> + 

nit: trailing whitespace

@@ +82,5 @@
> +function stripComments(buf) {
> +  var lines = buf.split("\n");
> +  let entryRegex = /^\s*\/\//;
> +  let data = "";
> +  for (let i = 0; i < lines.length; ++i) {

nit: either i++ or ++i is fine, but it should be consistent at least per file

@@ +102,5 @@
> +    let buf = NetUtil.readInputStreamToString(stream, stream.available());
> +    return JSON.parse(stripComments(buf));
> +  }
> +  // If there's no input file, bootstrap.
> +  return {roots:[], maxBin:0};

nit: more spaces would probably be good here:

return { roots: [], maxBin: 0 };

@@ +110,5 @@
> +// between bin numbers and the CA-hashes, even as CAs come and go.
> +function writeTrustAnchors(file) {
> +  let fos = FileUtils.openSafeFileOutputStream(file);
> +
> +  let serializedData = JSON.stringify(gTrustAnchors, null, '\t');

nit: I think it would be nice to be two-space-delimited rather than tab-delimited

@@ +125,5 @@
> +    writeString(FILE_HEADER);
> +
> +    // Output the sorted gTrustAnchors
> +    writeString(FP_PREAMBLE);
> +    gTrustAnchors.roots.forEach(function(fp){

nit: add a space before the '{'

@@ +129,5 @@
> +    gTrustAnchors.roots.forEach(function(fp){
> +      let fpBytes = atob(fp.fingerprint);
> +
> +      writeString("  {\n");
> +      writeString("    /* "+fp.label+" */\n");

nit: spaces around operators

@@ +148,5 @@
> +}
> +
> +// Scan our list (linearly) for the given fingerprint string
> +function findTrustAnchorByFingerprint(fingerprint) {
> +  for (var i=0; i<gTrustAnchors.roots.length; i++) {

nit: let, not var, and spaces around operators: 'let i = 0; i < gTrustAnchors.roots.length;'

@@ +172,5 @@
> +  return label;
> +}
> +
> +// Fill in the gTrustAnchors list with trust anchors from the database.
> +function insertTrustAnchorsFromDatabase(){

nit: space before '{'

@@ +192,5 @@
> +      let binaryFingerprint = CommonUtils.hexToBytes(stripColons(cert.sha256Fingerprint));
> +      let encodedFingerprint = btoa(binaryFingerprint);
> +
> +       // Scan to see if this is already in the database.
> +      if (-1 == findTrustAnchorByFingerprint(encodedFingerprint)) {

nit: switch order of operands
Also, maybe use a const ROOT_NOT_ASSIGNED = -1 or something?

@@ +193,5 @@
> +      let encodedFingerprint = btoa(binaryFingerprint);
> +
> +       // Scan to see if this is already in the database.
> +      if (-1 == findTrustAnchorByFingerprint(encodedFingerprint)) {
> + 

nit: trailing whitespace

@@ +196,5 @@
> +      if (-1 == findTrustAnchorByFingerprint(encodedFingerprint)) {
> + 
> +        // Let's get a usable name; some old certs do not have CN= filled out
> +        let label = getLabelForCert(cert);
> + 

nit: trailing whitespace

@@ +199,5 @@
> +        let label = getLabelForCert(cert);
> + 
> +        // Add to list
> +        gTrustAnchors.maxBin += 1;
> +        gTrustAnchors.roots.push({"label":label, "binNumber":gTrustAnchors.maxBin, "fingerprint":encodedFingerprint})

Since we still do support sha1 fingerprints, we should probably specify that this is the sha256 fingerprint, here (i.e. call the field 'sha256Fingerprint').
Also, more spaces:

gTrustAnchors.roots.push({ label: label, binNumber: gTrustAnchors.maxBin, sha256Fingerprint: encodedFingerprint });

Also, also, keep lines <80 chars.

@@ +221,5 @@
> +
> +// Update known hashes before we sort
> +writeTrustAnchors(trustAnchorsFile);
> +
> +// Sort all trust anchors before writing, as AccumulateRootCA.cpp 

nit: trailing whitespace

@@ +237,5 @@
> +     return 0;
> +});
> +
> +// Write the output file.
> +let gRootHashesFileOutputStream = FileUtils.openSafeFileOutputStream(rootHashesFile);

Maybe instead of using globals for this, consider passing around a reference to the stream. No big deal, though.
Attachment #8506424 - Flags: review?(dkeeler) → review+
Assignee

Comment 20

5 years ago
Addressed keeler's comments, carrying r+ forward. 

Details on the updates from the last patchset follow:

> diff --git a/security/manager/boot/src/RootHashes.inc b/security/manager/boot/src/RootHashes.inc

Removed a bunch of whitespace (by changing the generating JS)

> diff --git a/security/manager/tools/KnownRootHashes.json b/security/manager/tools/KnownRootHashes.json

Changed from 4 spaces to 2 spaces of whitespac (also from changes to the JS)

> diff --git a/security/manager/boot/src/PublicKeyPinningService.cpp b/security/manager/boot/src/PublicKeyPinningService.cpp

Documentation update:

> --- a/security/manager/boot/src/PublicKeyPinningService.cpp
> +++ b/security/manager/boot/src/PublicKeyPinningService.cpp
> @@ -266,23 +266,21 @@ CheckPinsForHostname(const CERTCertList 
>        histogram = foundEntry->mTestMode
>          ? Telemetry::CERT_PINNING_MOZ_TEST_RESULTS_BY_HOST
>          : Telemetry::CERT_PINNING_MOZ_RESULTS_BY_HOST;
>        Telemetry::Accumulate(histogram, bucket);
>      } else {
>        Telemetry::Accumulate(histogram, result ? 1 : 0);
>      }
> 
> -    // We can collect per-CA pinning results in general, because
> -    // they don't reveal private information
> +    // We only collect per-CA pinning statistics upon failures.
>      CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);
> -    if (! CERT_LIST_END(rootNode, certList)) {
> -      // Only log telemetry if the certificate list is non-empty.
> +    // Only log telemetry if the certificate list is non-empty.
> +    if (!CERT_LIST_END(rootNode, certList)) {
>        if (!result) {
> -        // Log failure conditions specially.
>          AccumulateTelemetryForRootCA(Telemetry::CERT_PINNING_FAILURES_BY_CA, rootNode->cert);
>        }
>      }
> 
>      PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG,
>             ("pkpin: Pin check %s for %s host '%s' (mode=%s)\n",
>              result ? "passed" : "failed",
>              foundEntry->mIsMoz ? "mozilla" : "non-mozilla",


> diff --git a/security/manager/boot/src/RootCertificateTelemetryUtils.cpp b/security/manager/boot/src/RootCertificateTelemetryUtils.cpp

In the main cppfile I swapped namespace declarations and 
removed the static declarations. Also broke up UNKNOWN_ROOT 
into another status, HASH_FAILURE, with UNKNOWN_ROOT getting 
the 0-bin.
> --- a/security/manager/boot/src/RootCertificateTelemetryUtils.cpp
> +++ b/security/manager/boot/src/RootCertificateTelemetryUtils.cpp
> @@ -2,23 +2,26 @@
>  /* vim: set ts=8 sts=2 et sw=2 tw=80: */
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
>  #include "RootCertificateTelemetryUtils.h"
> 
>  #include "prlog.h"
> -#include "RootHashes.inc"
> +#include "RootHashes.inc" // Note: Generated by genRootCAHashes.js
> +#include "ScopedNSSTypes.h"
> 
>  // Note: New CAs will show up as UNKNOWN_ROOT until
> -// RootHashes.inc is updated to include them
> -#define UNKNOWN_ROOT    -1
> +// RootHashes.inc is updated to include them. 0 is reserved by
> +// genRootCAHashes.js for the unknowns.
> +#define UNKNOWN_ROOT  0
> +#define HASH_FAILURE -1
> 
> -using namespace mozilla::psm;
> +namespace mozilla { namespace psm { 
> 
>  #if defined(PR_LOGGING)
>  PRLogModuleInfo* gPublicKeyPinningTelemetryLog =
>    PR_NewLogModule("PublicKeyPinningTelemetryService");
>  #endif
> 
>  // Used in the BinarySearch method, this does a memcmp between the pointer
>  // provided to its construtor and whatever the binary search is looking for.
> @@ -39,25 +42,25 @@ public:
>    }
> 
>  private:
>    const uint8_t* mTarget;
>  };
> 
>  // Perform a hash of the provided cert, then search in the RootHashes.inc data
>  // structure for a matching bin number.
> -static int32_t
> +int32_t
>  RootCABinNumber(const SECItem* cert)
>  {
>    Digest digest;
> 
>    // Compute SHA256 hash of the certificate
>    nsresult rv = digest.DigestBuf(SEC_OID_SHA256, cert->data, cert->len);
>    if (NS_WARN_IF(NS_FAILED(rv))) {
> -    return UNKNOWN_ROOT;
> +    return HASH_FAILURE;
>    }
> 
>    // Compare against list of stored hashes
>    size_t idx;
> 
>    PR_LOG(gPublicKeyPinningTelemetryLog, PR_LOG_DEBUG,
>             ("pkpinTelem: First bytes %02hx %02hx %02hx %02hx\n",
>              digest.get().data[0], digest.get().data[1], digest.get().data[2], digest.get().data[3]));
> @@ -68,24 +71,28 @@ RootCABinNumber(const SECItem* cert)
>           &idx)) {
> 
>      PR_LOG(gPublicKeyPinningTelemetryLog, PR_LOG_DEBUG,
>            ("pkpinTelem: Telemetry index was %lu, bin is %d\n",
>             idx, ROOT_TABLE[idx].binNumber));
>      return (int32_t) ROOT_TABLE[idx].binNumber;
>    }
> 
> -  // Didn't match. 
> +  // Didn't match.
>    return UNKNOWN_ROOT;
>  }
> 
> 
>  // Attempt to increment the appropriate bin in the provided Telemetry probe ID. If
> -// the Root CA doesn't have its own bin, we do nothing.
> -static void
> -mozilla::psm::AccumulateTelemetryForRootCA(mozilla::Telemetry::ID probe, const CERTCertificate* cert)
> +// there was a hash failure, we do nothing.
> +void
> +AccumulateTelemetryForRootCA(mozilla::Telemetry::ID probe, 
> +  const CERTCertificate* cert)
>  {
>    int32_t binId = RootCABinNumber(&cert->derCert);
> 
> -  if (UNKNOWN_ROOT != binId) {
> -    mozilla::Telemetry::Accumulate(probe, binId);
> +  if (binId != HASH_FAILURE) {
> +    Accumulate(probe, binId);
>    }
>  }
> +
> +} // namespace psm
> +} // namespace mozilla

> diff --git a/security/manager/boot/src/RootCertificateTelemetryUtils.h b/security/manager/boot/src/RootCertificateTelemetryUtils.h

Un-static'd the methods and swapped to the more generic certt.h header.

> --- a/security/manager/boot/src/RootCertificateTelemetryUtils.h
> +++ b/security/manager/boot/src/RootCertificateTelemetryUtils.h
> @@ -3,19 +3,19 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
>  #ifndef RootCertificateTelemetryUtils_h
>  #define RootCertificateTelemetryUtils_h
> 
>  #include "mozilla/Telemetry.h"
> -#include "ScopedNSSTypes.h"
> +#include "certt.h"
> 
>  namespace mozilla { namespace psm {
> 
> -static void
> +void
>  AccumulateTelemetryForRootCA(mozilla::Telemetry::ID probe, const CERTCertificate* cert);
> 
>  } // namespace psm
>  } // namespace mozilla
> 
>  #endif // RootCertificateTelemetryUtils_h

> diff --git a/security/manager/tools/genRootCAHashes.js b/security/manager/tools/genRootCAHashes.js

This is bigger. Details inline:

> --- a/security/manager/tools/genRootCAHashes.js
> +++ b/security/manager/tools/genRootCAHashes.js
> @@ -1,38 +1,36 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

The script now takes the output file as the first argument.

>  // 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 \
> -//                                  [path to]/xpcshell genRootCAHashes.js \
> -//                                   && cp RootHashes.inc ../boot/src/RootHashes.inc'
> +// 3. run `[path to]/run-mozilla.sh [path to]/xpcshell genRootCAHashes.js \
> +//                                  [absolute path to]/RootHashes.inc'
> 
>  // <https://developer.mozilla.org/en/XPConnect/xpcshell/HOWTO>
>  // <https://bugzilla.mozilla.org/show_bug.cgi?id=546628>
>  const Cc = Components.classes;
>  const Ci = Components.interfaces;
>  const Cu = Components.utils;
>  const Cr = Components.results;

I totally didn't understand the Ci constant going on here, but I get it now:

> -const nsIX509Cert = Components.interfaces.nsIX509Cert;
>  const nsX509CertDB = "@mozilla.org/security/x509certdb;1";
> -const nsIX509CertDB = Components.interfaces.nsIX509CertDB;
> -var CertDb = Components.classes[nsX509CertDB].getService(nsIX509CertDB);
> +const CertDb = Components.classes[nsX509CertDB].getService(Ci.nsIX509CertDB);
> 
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/FileUtils.jsm");
>  Cu.import("resource://gre/modules/NetUtil.jsm");
>  Cu.import("resource://services-common/utils.js");
> 
>  const FILENAME_OUTPUT = "RootHashes.inc";
>  const FILENAME_TRUST_ANCHORS = "KnownRootHashes.json";
> +const ROOT_NOT_ASSIGNED = -1;
> 
>  const JSON_HEADER = "// This Source Code Form is subject to the terms of the Mozilla Public\n" +
>  "// License, v. 2.0. If a copy of the MPL was not distributed with this\n" +
>  "// file, You can obtain one at http://mozilla.org/MPL/2.0/. */\n" +
>  "//\n" +
>  "//***************************************************************************\n" +
>  "// This is an automatically generated file. It's used to maintain state for\n" +
>  "// runs of genRootCAHashes.js; you should never need to manually edit it\n" +
> @@ -49,46 +47,48 @@ const FILE_HEADER = "/* This Source Code
>  "/*****************************************************************************/\n" +
>  "\n" +
>  "#define HASH_LEN 32\n";
> 
>  const FP_PREAMBLE = "struct CertAuthorityHash {\n" +
>  " const uint8_t hash[HASH_LEN];\n" +
>  " const int32_t binNumber;\n" +
>  "};\n\n" +
> -"static const struct CertAuthorityHash ROOT_TABLE[] = {\n"
> +"static const struct CertAuthorityHash ROOT_TABLE[] = {\n";
> 
> -const FP_POSTAMBLE = "};\n"
> +const FP_POSTAMBLE = "};\n";

Not using a global outstream anymore:

>  // Helper
> -function writeString(string) {
> -  gRootHashesFileOutputStream.write(string, string.length);
> +function writeString(fos, string) {
> +  fos.write(string, string.length);
>  }
> 
>  // Remove all colons from a string
>  function stripColons(hexString) {
>    return hexString.replace(/:/g, '');
>  }

Fix end-of-line spaces and loop increment consistency:

>  // Expect an array of bytes and make it C-formatted
>  function hexSlice(bytes, start, end) {
>    let ret = "";
> -  for (let i=start; i<end; i++) {
> +  for (let i = start; i < end; i++) {
>      let hex = (0 + bytes.charCodeAt(i).toString(16)).slice(-2).toUpperCase();
>      ret += "0x" + hex;
> -    if (i<end) ret += ", ";
> +    if (i < end - 1) {
> +      ret += ", ";
> +    }
>    }
>    return ret;
>  }
> - 
> +
>  function stripComments(buf) {
> -  var lines = buf.split("\n");
> +  let lines = buf.split("\n");
>    let entryRegex = /^\s*\/\//;
>    let data = "";
> -  for (let i = 0; i < lines.length; ++i) {
> +  for (let i = 0; i < lines.length; i++) {
>      let match = entryRegex.exec(lines[i]);
>      if (!match) {
>        data = data + lines[i];
>      }
>    }
>    return data;
>  }
> 
> @@ -98,68 +98,68 @@ function loadTrustAnchors(file) {
>    if (file.exists()) {
>      let stream = Cc["@mozilla.org/network/file-input-stream;1"]
>                     .createInstance(Ci.nsIFileInputStream);
>      stream.init(file, -1, 0, 0);
>      let buf = NetUtil.readInputStreamToString(stream, stream.available());
>      return JSON.parse(stripComments(buf));
>    }
>    // If there's no input file, bootstrap.
> -  return {roots:[], maxBin:0};
> +  return { roots: [], maxBin: 0 };
>  }

Swap from tabs to two space indentation:

>  // Saves our persistence file so that we don't lose track of the mapping
>  // between bin numbers and the CA-hashes, even as CAs come and go.
>  function writeTrustAnchors(file) {
>    let fos = FileUtils.openSafeFileOutputStream(file);
> 
> -  let serializedData = JSON.stringify(gTrustAnchors, null, '\t');
> +  let serializedData = JSON.stringify(gTrustAnchors, null, '  ');
>    fos.write(JSON_HEADER, JSON_HEADER.length);
>    fos.write(serializedData, serializedData.length);
> 
>    FileUtils.closeSafeFileOutputStream(fos);
>  }
> 

Pass around the outstream:

>  // Write the C++ header file
> -function writeRootHashes() {
> +function writeRootHashes(fos) {
>    try {
> -    writeString(FILE_HEADER);
> +    writeString(fos, FILE_HEADER);
> 
>      // Output the sorted gTrustAnchors
> -    writeString(FP_PREAMBLE);
> -    gTrustAnchors.roots.forEach(function(fp){
> -      let fpBytes = atob(fp.fingerprint);
> +    writeString(fos, FP_PREAMBLE);
> +    gTrustAnchors.roots.forEach(function(fp) {
> +      let fpBytes = atob(fp.sha256Fingerprint);
> 
> -      writeString("  {\n");
> -      writeString("    /* "+fp.label+" */\n");
> -      writeString("    { " + hexSlice(fpBytes, 0,16) + "\n");
> -      writeString("      " + hexSlice(fpBytes, 16,32) + "},\n");
> -      writeString("      " + fp.binNumber + " /* Bin Number */\n");
> +      writeString(fos, "  {\n");
> +      writeString(fos, "    /* "+fp.label+" */\n");
> +      writeString(fos, "    { " + hexSlice(fpBytes, 0, 16) + ",\n");
> +      writeString(fos, "      " + hexSlice(fpBytes, 16, 32) + " },\n");
> +      writeString(fos, "      " + fp.binNumber + " /* Bin Number */\n");
> 
> -      writeString("  },\n");
> +      writeString(fos, "  },\n");
>      });
> -    writeString(FP_POSTAMBLE);
> +    writeString(fos, FP_POSTAMBLE);
> 
> -    writeString("\n");
> +    writeString(fos, "\n");
> 
>    }
>    catch (e) {
>      dump("ERROR: problem writing output: " + e + "\n");
>    }
>  }

Rename the fingerprint to sha256Fingerprint:

>  // Scan our list (linearly) for the given fingerprint string
> -function findTrustAnchorByFingerprint(fingerprint) {
> -  for (var i=0; i<gTrustAnchors.roots.length; i++) {
> -    if (fingerprint == gTrustAnchors.roots[i].fingerprint) {
> +function findTrustAnchorByFingerprint(sha256Fingerprint) {
> +  for (let i = 0; i < gTrustAnchors.roots.length; i++) {
> +    if (sha256Fingerprint == gTrustAnchors.roots[i].sha256Fingerprint) {
>        return i;
>      }
>    }
> -  return -1;
> +  return ROOT_NOT_ASSIGNED;
>  }
> 
>  // Get a clean label for a given certificate; usually the common name.
>  function getLabelForCert(cert) {
>    let label = cert.commonName;
> 
>    if (label.length < 5) {
>      label = cert.subjectName;
> @@ -188,56 +188,67 @@ function insertTrustAnchorsFromDatabase(
> 
>      // If this is a trusted cert
>      if (CertDb.isCertTrusted(cert, CERT_TYPE, TRUST_TYPE)) {
>        // Base64 encode the hex string
>        let binaryFingerprint = CommonUtils.hexToBytes(stripColons(cert.sha256Fingerprint));
>        let encodedFingerprint = btoa(binaryFingerprint);
> 
>         // Scan to see if this is already in the database.
> -      if (-1 == findTrustAnchorByFingerprint(encodedFingerprint)) {
> - 
> +      if (findTrustAnchorByFingerprint(encodedFingerprint) == ROOT_NOT_ASSIGNED) {
> +
>          // Let's get a usable name; some old certs do not have CN= filled out
>          let label = getLabelForCert(cert);
> - 
> +
>          // Add to list
>          gTrustAnchors.maxBin += 1;
> -        gTrustAnchors.roots.push({"label":label, "binNumber":gTrustAnchors.maxBin, "fingerprint":encodedFingerprint})
> +        gTrustAnchors.roots.push(
> +          {
> +            "label": label,
> +            "binNumber": gTrustAnchors.maxBin,
> +            "sha256Fingerprint": encodedFingerprint
> +          });
>        }
>      }
>    }
>  }
> 
>  //
>  //  PRIMARY LOGIC
>  //
> 
> +if (arguments.length < 1) {
> +  throw "Usage: genRootCAHashes.js <absolute path to current RootHashes.inc>";
> +}
> +
>  let trustAnchorsFile = FileUtils.getFile("CurWorkD", [FILENAME_TRUST_ANCHORS]);
> -let rootHashesFile = FileUtils.getFile("CurWorkD", [FILENAME_OUTPUT]);
> +// let rootHashesFile = FileUtils.getFile("CurWorkD", arguments[0]);
> +let rootHashesFile = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
> +rootHashesFile.initWithPath(arguments[0]);
> 
>  // Open the known hashes file; this is to ensure stable bin numbers.
>  let gTrustAnchors = loadTrustAnchors(trustAnchorsFile);
> 
>  // Collect all certificate entries
>  insertTrustAnchorsFromDatabase();
> 
>  // Update known hashes before we sort
>  writeTrustAnchors(trustAnchorsFile);
> 
> -// Sort all trust anchors before writing, as AccumulateRootCA.cpp 
> +// Sort all trust anchors before writing, as AccumulateRootCA.cpp
>  // will perform binary searches
>  gTrustAnchors.roots.sort(function(a, b) {
>    // We need to work from the binary values, not the base64 values.
> -  let aBin = atob(a.fingerprint);
> -  let bBin = atob(b.fingerprint)
> +  let aBin = atob(a.sha256Fingerprint);
> +  let bBin = atob(b.sha256Fingerprint)
> 
>    if (aBin < bBin)
>       return -1;
>    else if (aBin > bBin)
>       return 1;
>     else
>       return 0;
>  });
> 
>  // Write the output file.
> -let gRootHashesFileOutputStream = FileUtils.openSafeFileOutputStream(rootHashesFile);
> -writeRootHashes();
> -FileUtils.closeSafeFileOutputStream(gRootHashesFileOutputStream);
> +let rootHashesFileOutputStream = FileUtils.openSafeFileOutputStream(rootHashesFile);
> +writeRootHashes(rootHashesFileOutputStream);
> +FileUtils.closeSafeFileOutputStream(rootHashesFileOutputStream);
>
Attachment #8506424 - Attachment is obsolete: true
Attachment #8507037 - Flags: review+
Assignee

Comment 21

5 years ago
Previous try came back green; this one is looking good too:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=be21efb20eaa
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/db1ee8cf5a8c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Assignee

Comment 24

5 years ago
Comment on attachment 8507037 [details] [diff] [review]
Update for bug_1054498 patch

Approval Request Comment
[Feature/regressing bug #]: 1054498
[User impact if declined]: Delayed telemetry on impacts of pinning violations
[Describe test coverage new/current, TBPL]: Testing was manual, using known-good (twitter.com, etc.) and known-bad pinning sites (https://pinningtest.appspot.com) and verifying in about:telemetry.
[Risks and why]: Low; only affects pinning violations, and uses known-good telemetry code.
[String/UUID change made/needed]: None
Attachment #8507037 - Flags: approval-mozilla-aurora?
Attachment #8507037 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Depends on: 1105851
You need to log in before you can comment on or make changes to this bug.