Create mechanism/script to partially automate import of cert pinning data

RESOLVED FIXED in mozilla32

Status

()

Core
Security: PSM
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: cviecco, Assigned: mmc)

Tracking

(Blocks: 1 bug)

unspecified
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

178.75 KB, patch
cviecco
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
We need to import the pinning data from Google Chrome so that we can leverage Google's pinning data.
(Reporter)

Updated

5 years ago
Blocks: 744204
The goal of this bug is to create a script that pulls the Chrome list, extracts what we need, and spits it out in a format our pinning back-end can read (probably a .h file).  This should be extensible so that we can pull from other sources as well (such as a Mozilla-maintained list) and merge into one "pre-pinned" data set.
Summary: Create importing of pinning data from Chrome (google) → Create mechanism/script to partially automate import of cert pinning data
I do not think this should block bug 744204 because we can (and should) implement and test bug 744204 without this.
Good call, blocking relationship was backwards.
No longer blocks: 744204
Depends on: 744204
(Reporter)

Updated

5 years ago
Blocks: 776746
(Reporter)

Comment 4

5 years ago
Created attachment 653486 [details]
Importer script, uses data from the mozila deps (bug 776746)

To use you also need the attachment found at https://bugzilla.mozilla.org/show_bug.cgi?id=776746.
Depends on: 1002696
No longer depends on: 1002696
(Reporter)

Comment 5

3 years ago
Created attachment 8416599 [details] [diff] [review]
add-sha1-to-certgenstruct
Attachment #653486 - Attachment is obsolete: true
(Reporter)

Comment 6

3 years ago
Comment on attachment 8416599 [details] [diff] [review]
add-sha1-to-certgenstruct

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

Step1. we need to update the iternals to allow built-in sha1 fingerprints.
Attachment #8416599 - Flags: superreview?(dkeeler)
(Reporter)

Updated

3 years ago
Attachment #8416599 - Flags: superreview?(dkeeler) → feedback?(dkeeler)
Comment on attachment 8416599 [details] [diff] [review]
add-sha1-to-certgenstruct

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

I've thought about this more, and I think if we're going to be storing fingerprints generated by different hashes it's important that we store what type of hash each fingerprint is. This will take up extra space. However, we can get that extra space back by storing the fingerprints as binary data instead of base-64.

For example, instead of:

static const char kGeoTrust_Universal_CAFingerprint[]=
  "lpkiXF3lLlbN0y3y6W0c/qWqPKC7Us2JM8I7XCdEOCA=";

We'll have:

struct KeyFingerprint
{
  SECOidTag hashAlg;
  const unsigned char* hashData;
};

const unsigned char kGeoTrust_Universal_CAFingerprint_data = {
  0x96, 0x99, 0x22, 0x5C, 0x5D, 0xE5, 0x2E, 0x56, 0xCD, 0xD3, 0x2D, 
  0xF2, 0xE9, 0x6D, 0x1C, 0xFE, 0xA5, 0xAA, 0x3C, 0xA0, 0xBB, 0x52,
  0xCD, 0x89, 0x33, 0xC2, 0x3B, 0x5C, 0x27, 0x44, 0x38, 0x20
};

const KeyFingerprint kGeoTrust_Universal_CAFingerprint = {
  SEC_OID_SHA256,
  kGeoTrust_Universal_CAFingerprint_data
};

(which looks longer, but actually should take up less space when compiled)
Attachment #8416599 - Flags: feedback?(dkeeler) → feedback-
Taking as Camilo requested.
Assignee: nobody → mmc
Duplicate of this bug: 1004275
Created attachment 8419818 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (
Comment on attachment 8419818 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (

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

Major changes to static pin generation:
- Download and import all of Chrome's static certs and pinsets
- Add test_mode so that we import Chrome's pinsets as test (this can be moved to another bug if you like -- I included it because otherwise we can't import Chrome's pinsets)
- Add struct StaticFullPinset { sha1 pinset, sha256 pinset }
- Refactor EvalPinWithPinset to understand StaticFullPinset

This needs comment cleanup before review. I need some help with the unittest changes -- test_pinning.js is throwing SSL_ERROR_BAD_CERT_DOMAIN which I vaguely remember is caused by generate_certs.sh not knowing the correct alt names, but as you can see I re-ran the generator and it is still failing.

We probably don't need ChromeNames.json (a map from Chrome's pinset names to their sha256 hashes that I generated with go), but for some reason constructx509FromBase64 is failing on all but one of the PEMs...

Also tested by going to https://twitter.com and observing through logging that the pinset for twitter.com was indeed in test mode.
Attachment #8416599 - Attachment is obsolete: true
Comment on attachment 8419818 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (

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

Please review new struct StaticFullPinset and interface changes to EvalPinWithPinset.
Attachment #8419818 - Flags: feedback?(dkeeler)
Attachment #8419818 - Flags: feedback?(cviecco)
Blocks: 1004350
Created attachment 8419912 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (r=cviecco,keeler)

Bizarre - the unittest failures go away if generate_cert uses test-mode.pinning.example.com instead of *.test-mode.pinning.example.com. I wonder if I am running up against size limits.
Attachment #8419818 - Attachment is obsolete: true
Attachment #8419818 - Flags: feedback?(dkeeler)
Attachment #8419818 - Flags: feedback?(cviecco)
Attachment #8419912 - Flags: review?(dkeeler)
Attachment #8419912 - Flags: review?(cviecco)
Comment on attachment 8419912 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (r=cviecco,keeler)

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

::: security/manager/boot/src/StaticHPKPins.h
@@ +191,5 @@
> +  &kPinset_mozilla_sha1,
> +  &kPinset_mozilla_sha256
> +};
> +
> +static const char* kPinset_mozilla_test_sha1_Data[] = {

Windows doesn't like this, so I will switch these out to nullptr.

https://tbpl.mozilla.org/?tree=Try&rev=fbd5784c6ed6
(Reporter)

Comment 15

3 years ago
Comment on attachment 8419912 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (r=cviecco,keeler)

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

Good start, There are several structural issues that need to be addressed before I go into full review mode.

::: security/manager/boot/src/PublicKeyPinningService.cpp
@@ +111,5 @@
>             ("pkpin: certArray common_name: '%s'\n",
>              CERT_GetCommonName(&(currentCert->issuer))));
> +    // SHA256 is more trustworthy, so try that first.
> +    if (EvalPinWithPinset(currentCert, SEC_OID_SHA256, pinset)) {
> +      return true;

No, we shuld first try  sha256 for all certs and then we try all sha1 fp, if you must then make a new evalChain againsPinset (where the pinset and the fp SECOID are given).

::: security/manager/ssl/tests/unit/tlsserver/pkcs11.txt
@@ +1,3 @@
>  library=
>  name=NSS Internal PKCS #11 Module
> +parameters=configdir='sql:./security/manager/ssl/tests/unit/tlsserver' certPrefix='' keyPrefix='' secmod='secmod.db' flags= updatedir='' updateCertPrefix='' updateKeyPrefix='' updateid='' updateTokenDescription='' 

Why is this change in this patch? If we thin that should not be here then we should file another patch.

::: security/manager/tools/PreloadedHPKPins.json
@@ +32,5 @@
>        // In that same bug we also notice that our cdn sites use Verisign and
>        // Baltimore
>        "name": "mozilla",
> +      "sha1_hashes": [],
> +      "sha256_hashes": [

This division sha256 vs sha1 should not exist here. This file is describes what each pinset will contain by naming the certificates names as they exist in the mozilla root store.

::: security/manager/tools/genHPKPStaticPins.js
@@ +9,5 @@
>  //                                  [path to]/genHPKPStaticpins.js \
>  //                                  [absolute path to]/PreloadedHPKPins.json \
>  //                                  [absolute path to]/default-ee.der \
> +//                                  [absolute path to]/StaticHPKPins.h \
> +//                                  [absolute path to]/ChromeNames.json

Where is this file coming from? This dependency is not needed.

@@ +59,5 @@
> +const PINSETDEF = "/* Pinsets are each an ordered list by the actual value of the fingerprint */\n" +
> +  "typedef struct {\n" +
> +  "  const size_t size;\n" +
> +  "  const char* const* data;\n" +
> +  "} StaticPinset;\n\n" +

StaticPinarray?

@@ +63,5 @@
> +  "} StaticPinset;\n\n" +
> +  "typedef struct {\n" +
> +  "  const StaticPinset* sha1;\n" +
> +  "  const StaticPinset* sha256;\n" +
> +  "} StaticFullPinset;\n\n";

StaticPinset?

@@ +179,5 @@
> +// and stick the hash into certSKDToName
> +// - If the entry has a PEM format, parse the PEM, find the Mozilla pin name
> +// and stick the hash in certSKDToName
> +// We MUST be able to find a corresponding cert nickname for the Chrome names,
> +// otherwise we skip all pinsets referring to that Chrome name.

Why? lets make a new entry. prefixed with GOOGLE_SPECIFIC_PIN (or something similar). And include them. What is your worry?
Attachment #8419912 - Flags: review?(dkeeler) → review-
Comment on attachment 8419912 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (r=cviecco,keeler)

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

Thanks for the review, Camilo. I will work on the changes.
Attachment #8419912 - Flags: review?(cviecco)
Comment on attachment 8419912 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (r=cviecco,keeler)

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

I have some comments too.

::: security/manager/boot/src/PublicKeyPinningService.cpp
@@ +51,5 @@
>  }
>  
> +static bool
> +EvalPinWithPinset(CERTCertificate* cert,
> +                  SECOidTag hashType,

This can probably go on the previous line, depending on the length.

@@ +76,5 @@
> +           ("pkpin: Unsupported hash type: %d\n", hashType));
> +    return false;
> +  }
> +
> +  // Compare, linear search for now...

I'm not sure this comment is helpful. It's pretty clear this is linear search.

@@ +77,5 @@
> +    return false;
> +  }
> +
> +  // Compare, linear search for now...
> +  for (size_t j = 0; j < pinset->size; j++){

nit: maybe "index" instead of "j"?

@@ +111,5 @@
>             ("pkpin: certArray common_name: '%s'\n",
>              CERT_GetCommonName(&(currentCert->issuer))));
> +    // SHA256 is more trustworthy, so try that first.
> +    if (EvalPinWithPinset(currentCert, SEC_OID_SHA256, pinset)) {
> +      return true;

I don't think it's important to make sure we evaluate sha256 for the entire chain and then again with sha1. If there's a vulnerability with sha1, we'll stop using it entirely. Until then, if we trust it enough to use it at all, I think code clarity is more important that strictly preferring sha256 to sha1.

::: security/manager/boot/src/StaticHPKPins.h
@@ +281,5 @@
>  
>  static const TransportSecurityPreload kPublicKeyPinningPreloadList[] = {
> +  { "accounts.google.com", true, true, &kPinset_google },
> +  { "addons.mozilla.net", true, false, &kPinset_mozilla },
> +  { "addons.mozilla.org", true, false, &kPinset_mozilla },

Do we not want to keep these in test mode for now?

::: security/manager/ssl/tests/unit/test_pinning.js
@@ +92,3 @@
>  };
>  
>  function check_pinning_telemetry() {

Don't forget to check the test-mode telemetry when that's added.

::: security/manager/ssl/tests/unit/tlsserver/generate_certs.sh
@@ +141,5 @@
>  
>  make_CA testCA 'CN=Test CA' test-ca.der
>  make_CA otherCA 'CN=Other test CA' other-test-ca.der
>  
> +make_EE localhostAndExampleCom 'CN=Test End-entity' testCA "localhost,*.example.com,*.pinning.example.com,*.include-subdomains.pinning.example.com,*.exclude-subdomains.pinning.example.com,test-mode.pinning.example.com"

This certificate shouldn't need the test-mode.pinning.example.com alt name.

@@ +146,2 @@
>  # Make an EE cert issued by otherCA
> +make_EE otherIssuerEE 'CN=Wrong CA Pin Test End-Entity' otherCA "*.include-subdomains.pinning.example.com,*.exclude-subdomains.pinning.example.com,test-mode.pinning.example.com"

I think the issue was that an alt name of *.test-mode.example.com doesn't match test-mode.example.com.

::: security/manager/ssl/tests/unit/tlsserver/pkcs11.txt
@@ +1,3 @@
>  library=
>  name=NSS Internal PKCS #11 Module
> +parameters=configdir='sql:./security/manager/ssl/tests/unit/tlsserver' certPrefix='' keyPrefix='' secmod='secmod.db' flags= updatedir='' updateCertPrefix='' updateKeyPrefix='' updateid='' updateTokenDescription='' 

Changes to this file happen as a result of running generate_certs.sh. We should treat it like a binary file (like cert9.db and friends).

::: security/manager/tools/ChromeNames.json
@@ +1,2 @@
> +{
> +  "VeriSignClass3": "sRJBQqWhpaKIGcc1NA7/jJ4vgWj+47oYfyU7waOS1+I=",

Did you write this by hand? We should probably automate this file as well (that can be a follow-up bug).

::: security/manager/tools/genHPKPStaticPins.js
@@ +9,5 @@
>  //                                  [path to]/genHPKPStaticpins.js \
>  //                                  [absolute path to]/PreloadedHPKPins.json \
>  //                                  [absolute path to]/default-ee.der \
> +//                                  [absolute path to]/StaticHPKPins.h \
> +//                                  [absolute path to]/ChromeNames.json

I don't think this documentation matches the actual use of the arguments.

@@ +17,3 @@
>          "<absolute path to default-ee.der> " +
> +        "<absolute path to StaticHPKPins.h>" +
> +        "<absolute path to ChromeNames.json>";

Same with the documentation (e.g. default-ee.der seems to be the third argument).

@@ +48,5 @@
>  "#include <stdint.h>" +
>  "\n";
> +
> +const DOMAINHEADER = "/* Domainlist */\n" +
> +  "typedef struct {\n" +

While we're doing this, let's not use typedef. I think we can get away with:
struct TransportSecurityPreload {
  const char* mHost;
  // etc.
};

@@ +49,5 @@
>  "\n";
> +
> +const DOMAINHEADER = "/* Domainlist */\n" +
> +  "typedef struct {\n" +
> +  "  const char *mHost;\n" +

nit: const char*

@@ +120,5 @@
>  }
>  
> +function download(filename) {
> +  var req = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +            .createInstance(Ci.nsIXMLHttpRequest);

nit: indent this line two spaces

@@ +126,5 @@
> +  try {
> +    req.send();
> +  }
> +  catch (e) {
> +    throw "ERROR: problem downloading '" + SOURCE + "': " + e;

s/SOURCE/filename/

@@ +130,5 @@
> +    throw "ERROR: problem downloading '" + SOURCE + "': " + e;
> +  }
> +
> +  if (req.status != 200) {
> +    throw "ERROR: problem downloading '" + SOURCE + "': status " + req.status;

s/SOURCE/filename/

@@ +143,5 @@
> +  try {
> +    data = JSON.parse(result);
> +  }
> +  catch (e) {
> +    throw "ERROR: could not parse data from '" + SOURCE + "': " + e;

s/SOURCE/filename/

@@ +150,5 @@
> +}
> +
> +// Returns a nickname from the given pem, if it exists.
> +function getNicknameFromPem(pem) {
> +  let certdb = certdb2.QueryInterface(Ci.nsIX509CertDB);

Couple thoughts about this:
1. Let's keep with the convention of indicating global variables by prefixing with "g" (so "gCertDB")
2. Let's have this where we declare gCertDB:
const gCertDB = Cc["@mozilla.org/security/x509certdb;1"]
                  .getService(Ci.nsIX509CertDB2);
gCertDB.QueryInterface(Ci.nsIX509CertDB);

If I understand correctly, this should make all of CertDB and CertDB2's methods available.

@@ +166,5 @@
> +}
> +
> +// Downloads the static certs file and tries to map Google Chrome nicknames to
> +// Mozilla nicknames, as well as storing any hashes for pins for which we don't
> +// have root PEMs.  Each entry consists of a line containing the name of the

Super-pedantic nit: one space after "."

@@ +179,5 @@
> +// and stick the hash into certSKDToName
> +// - If the entry has a PEM format, parse the PEM, find the Mozilla pin name
> +// and stick the hash in certSKDToName
> +// We MUST be able to find a corresponding cert nickname for the Chrome names,
> +// otherwise we skip all pinsets referring to that Chrome name.

Well, this could result in an incomplete pinset, right? That's bad because we could falsely report a pinning violation.
Comment on attachment 8419912 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (r=cviecco,keeler)

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

::: security/manager/boot/src/PublicKeyPinningService.cpp
@@ +111,5 @@
>             ("pkpin: certArray common_name: '%s'\n",
>              CERT_GetCommonName(&(currentCert->issuer))));
> +    // SHA256 is more trustworthy, so try that first.
> +    if (EvalPinWithPinset(currentCert, SEC_OID_SHA256, pinset)) {
> +      return true;

This was easy to fix, so I went ahead and did it.

::: security/manager/boot/src/StaticHPKPins.h
@@ +281,5 @@
>  
>  static const TransportSecurityPreload kPublicKeyPinningPreloadList[] = {
> +  { "accounts.google.com", true, true, &kPinset_google },
> +  { "addons.mozilla.net", true, false, &kPinset_mozilla },
> +  { "addons.mozilla.org", true, false, &kPinset_mozilla },

I went ahead and made it so that if the preloads.json doesn't specify, then test_mode is true. Also I went ahead and specified test_mode for all the mozilla domains in the json.

::: security/manager/ssl/tests/unit/test_pinning.js
@@ +92,3 @@
>  };
>  
>  function check_pinning_telemetry() {

The telemetry requires special handling, so I'd like to do that as a separate bug.

::: security/manager/tools/ChromeNames.json
@@ +1,2 @@
> +{
> +  "VeriSignClass3": "sRJBQqWhpaKIGcc1NA7/jJ4vgWj+47oYfyU7waOS1+I=",

I did, but using go libraries since our libraries were barfing :P I agree we shouldn't need this file. If I can't fix it I'll file another bug for the constructX509FromBase64 errors.

::: security/manager/tools/genHPKPStaticPins.js
@@ +179,5 @@
> +// and stick the hash into certSKDToName
> +// - If the entry has a PEM format, parse the PEM, find the Mozilla pin name
> +// and stick the hash in certSKDToName
> +// We MUST be able to find a corresponding cert nickname for the Chrome names,
> +// otherwise we skip all pinsets referring to that Chrome name.

It won't result in an incomplete pinset if we take the hashes from ChromeNames.json. We import a pinset iff we have hashes for all the names.
Created attachment 8420263 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (
Attachment #8419912 - Attachment is obsolete: true
Comment on attachment 8420263 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (

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

I am still struggling with generate_cert issues, but please see if this satisfies your major comments. Fixed all nits.

I decided that StaticPinArray was not a good name, so used StaticFingerprints instead since its members are all called kFingerprint something.
Attachment #8420263 - Flags: review?(dkeeler)
Attachment #8420263 - Flags: review?(cviecco)
Comment on attachment 8420263 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (

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

Missed some of David's nits.
Attachment #8420263 - Flags: review?(dkeeler)
Attachment #8420263 - Flags: review?(cviecco)
(Reporter)

Comment 22

3 years ago
Comment on attachment 8420263 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (

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

Getting there, we still need to get rid of that ChromeNames.json.

::: security/manager/boot/src/PublicKeyPinningService.cpp
@@ +24,5 @@
>    PR_NewLogModule("PublicKeyPinningService");
>  #endif
>  
>  /**
>   Computes in the location specified by base64Out the SHA256 digest

nit, fix comment

@@ +28,5 @@
>   Computes in the location specified by base64Out the SHA256 digest
>   of the DER Encoded subject Public Key Info for the given cert
>  */
>  static SECStatus
> +GetBase64HashSPKI(const CERTCertificate* cert, SECOidTag hashType,

nit constfy the hashtype

@@ +55,3 @@
>   */
>  static bool
> +EvalCertWithHashType(const CERTCertificate* cert, SECOidTag hashType,

nit: constfigy the hashtype

@@ +56,5 @@
>  static bool
> +EvalCertWithHashType(const CERTCertificate* cert, SECOidTag hashType,
> +                     const StaticPinset* pinset)
> +{
> +  const StaticFingerprints* fps = nullptr;

please use a meaningful name for variables. (frames per second? films packed statically?)

@@ +61,5 @@
> +  if (hashType == SEC_OID_SHA256) {
> +    fps = pinset->sha256;
> +  } else if (hashType == SEC_OID_SHA1) {
> +    fps = pinset->sha1;
> +  } else if (!fps) {

move the (!fps) outside the else (so that we can have null fingerprits for type). And move this selection to EvalChainWithHashType so that this comparison is done just once. (the signature of this call would change to be 'cert, StaticFingerPrints *')

@@ +77,5 @@
> +  }
> +
> +  PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG,
> +         ("pkpin: base_64(hash(key)='%s'\n", base64Out.get()));
> +

Add comment mentioning linear search

@@ +93,5 @@
> + * Returns true if a given chain matches any hashType fingerprints from the
> + * given pinset, false otherwise.
> + */
> +static bool
> +EvalChainWithHashType(const CERTCertList* certList, SECOidTag hashType,

nit const hashType

@@ +189,5 @@
>    if (foundEntry && foundEntry->pinset) {
> +    bool result = EvalChainWithPinset(certList, foundEntry->pinset);
> +    if (!foundEntry->mTestMode) {
> +      Telemetry::Accumulate(Telemetry::CERT_PINNING_EVALUATION_RESULTS,
> +                            result);

result is bool, the input here is an int. Please keep as it was, you are probably getting a compilation warning here.

::: security/manager/boot/src/StaticHPKPins.h
@@ +298,3 @@
>  
> +static const StaticPinset kPinset_mozilla = {
> +  &kPinset_mozilla_sha1,

change this and all instances of empty lists to nullpointers

::: security/manager/tools/genHPKPStaticPins.js
@@ +416,5 @@
> +              pinset.sha1_hashes, "sha1");
> +  writePinset(certNameToSKD, certSKDToName, pinset.name,
> +              pinset.sha256_hashes, "sha256");
> +  let prefix = "kPinset_" + pinset.name;
> +  writeTo("static const StaticPinset " + prefix + " = {\n" +

if the pinarrays are of len=0 put nullptr as the value of the pinset for that hash type.

@@ +422,3 @@
>  }
>  
> +function writePinset(certNameToSKD, certSKDToName, name, hashes, type) {

rename function (writes a statc fingerprint) . And we should not be cluttering the output with empty lists (no not write if len=0).
Attachment #8420263 - Flags: review-
Created attachment 8420343 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (
Attachment #8420263 - Attachment is obsolete: true
Created attachment 8420346 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (
Attachment #8420343 - Attachment is obsolete: true
Comment on attachment 8420346 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (

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

Please take another look. I'm working on a unittest to demonstrate the base64 parsing issue and will file that as a separate bug.
Attachment #8420346 - Flags: review?(dkeeler)
Attachment #8420346 - Flags: review?(cviecco)
Created attachment 8420365 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (
Attachment #8420346 - Attachment is obsolete: true
Attachment #8420346 - Flags: review?(dkeeler)
Attachment #8420346 - Flags: review?(cviecco)
Comment on attachment 8420365 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (

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

I found my bug. I was not resetting the PEM string correctly when the cert ended.
Attachment #8420365 - Flags: review?(dkeeler)
Attachment #8420365 - Flags: review?(cviecco)
Created attachment 8420385 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (
Attachment #8420365 - Attachment is obsolete: true
Attachment #8420365 - Flags: review?(dkeeler)
Attachment #8420365 - Flags: review?(cviecco)
Comment on attachment 8420385 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (

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

I missed Camilo's comment 22. As discussed on IRC, changed EvalCertWithHashType to take StaticFingerprints* instead of StaticPinset*, but we either need the secoid or the base64 string to do the comparison, so I left it with the secoid.

I also didn't implement Camilo's request for a comment about linear search, since David already asked for it to be removed.

About constifying secoid input parameters: I don't think this makes any sense, because primitives are passed by copy in C, and there is no way for them to mutate. David, please arbitrate.

Changed to nullptr for empty lists.

Thanks,
Monica
Attachment #8420385 - Flags: review?(dkeeler)
Attachment #8420385 - Flags: review?(cviecco)
Comment on attachment 8420385 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (

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

I think this looks good. I just have a few style nits.
SECOidTag parameters don't need to be const. They aren't const anywhere else in the code.

::: security/manager/boot/src/PublicKeyPinningService.cpp
@@ +73,5 @@
> +
> +  PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG,
> +         ("pkpin: base_64(hash(key)='%s'\n", base64Out.get()));
> +
> +  for (size_t i = 0; i < fingerprints->size; i++){

nit: ") {" at the end of the line

@@ +189,4 @@
>  
>    if (foundEntry && foundEntry->pinset) {
> +    bool result = EvalChainWithPinset(certList, foundEntry->pinset);
> +    if (!foundEntry->mTestMode) {

Going with the philosophy of checking and handling the exceptional case first, I think it would be safer to have this test be "if (foundEntry->mTestMode)". That way, the normal code flow of "let's enforce this pin" is more clear. Let's chat on irc if this isn't clear (I feel like I'm not making myself clear, here...)

::: security/manager/tools/genHPKPStaticPins.js
@@ +25,5 @@
>  let { Services } = Cu.import("resource://gre/modules/Services.jsm", {});
>  
> +let gCertDB = Cc["@mozilla.org/security/x509certdb;1"]
> +                 .getService(Ci.nsIX509CertDB2);
> +gCertDB = gCertDB.QueryInterface(Ci.nsIX509CertDB);

Just plain "gCertDB.QueryInterface(Ci.nsIX509CertDB);" doesn't work?

@@ +75,5 @@
> +let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
> +file.initWithPath(arguments[2]);
> +let gFileOutputStream = FileUtils.openSafeFileOutputStream(file);
> +
> +function writeTo(string) {

nit: This really isn't a big deal, but originally, the semantics of "writeTo" was intended to be "write (this string) to (that file)". Without the "that file" parameter, the name doesn't make as much sense. Maybe "writeString"?

@@ +149,5 @@
> +  }
> +  return data;
> +}
> +
> +// Returns a nickname from the given pem, if it exists.

I think this comment doesn't match the function here.

@@ +208,5 @@
> +            hash = line.substring(SHA1_PREFIX.length);
> +          } else if (line.startsWith(SHA256_PREFIX)) {
> +            hash = line.substring(SHA256_PREFIX);
> +          }
> +          chromeNameToHash[chromeName] = line;

Maybe add a comment that we're storing the whole line here because it includes the hash type prefix? (that's what's going on, right?)

@@ +227,5 @@
> +          if (hash in certSKDToName) {
> +            mozName = certSKDToName[hash];
> +          } else {
> +            // Not one of our built-in certs. Prefix the name with
> +            // GOOGLE_PIN.

Maybe say "GOOGLE_PIN_" to be more clear (it's confusing when the identifier indicating the value "GOOGLE_PIN_" is called GOOGLE_PIN_PREFIX).

@@ +247,5 @@
> +  return [ chromeNameToHash, chromeNameToMozName ];
> +}
> +
> +// We can only import pinsets from chrome if for every name in the pinset:
> +// - We have a hash from the certs.file, or

What is the "certs.file"?

@@ +333,5 @@
>      if (!isCertBuiltIn(cert)) {
>        continue;
>      }
>      let name = cert.nickname.substr(BUILT_IN_NICK_PREFIX.length);
> +    let SKD  = cert.sha256SubjectPublicKeyInfoDigest;

nit: there are two spaces after "SKD"

@@ +386,5 @@
> +  let sha1Name = "nullptr";
> +  let sha256Name = "nullptr";
> +  if (pinset.sha1_hashes && pinset.sha1_hashes.length > 0) {
> +    writeFingerprints(certNameToSKD, certSKDToName, pinset.name,
> +                pinset.sha1_hashes, "sha1");

nit: indentation

@@ +392,4 @@
>    }
> +  if (pinset.sha256_hashes && pinset.sha256_hashes.length > 0) {
> +    writeFingerprints(certNameToSKD, certSKDToName, pinset.name,
> +                pinset.sha256_hashes, "sha256");

nit: indentation

@@ +406,5 @@
> +  for (let certName of hashes) {
> +    SKDList.push(certNameToSKD[certName]);
> +  }
> +  for (let skd of SKDList.sort()) {
> +    writeTo("  " + nameToAlias(certSKDToName[skd])  + ",\n");

nit: there's two spaces before the last "+"

@@ +414,5 @@
> +    writeTo("  0\n");
> +  }
> +  writeTo("};\n");
> +  writeTo("static const StaticFingerprints " + varPrefix + " = { " +
> +          hashes.length + ", " + varPrefix + "_Data};\n\n");

nit: add a space after _Data

@@ +418,5 @@
> +          hashes.length + ", " + varPrefix + "_Data};\n\n");
> +}
> +
> +function writeEntry(entry) {
> +  let printVal = "  { \""+ entry.name +"\",\ ";

nit: spaces around + operators, please

@@ +489,5 @@
> +
> +  // Write actual fingerprints.
> +  Object.keys(usedFingerprints).sort().forEach(function(certName) {
> +    if (certName) {
> +      writeTo("/* "+ certName + " */\n");

nit: spaces around +

@@ +510,5 @@
> +
> +  // Write the domainlist entries.
> +  writeTo(DOMAINHEADER);
> +  writeDomainList(chromeImportedEntries);
> +  writeTo("\n\n");

nit: not sure we need the extra blank line here

@@ +519,5 @@
> +let [ chromeNameToHash, chromeNameToMozName ] = downloadAndParseChromeCerts(
> +  CHROME_CERT_SOURCE, certSKDToName);
> +let [ chromeImportedPinsets, chromeImportedEntries ] =
> +  downloadAndParseChromePins(CHROME_JSON_SOURCE, chromeNameToHash,
> +    chromeNameToMozName, certNameToSKD, certSKDToName);

nit: we could probably indent the rest of these arguments to line up with the ( on the previous line
Attachment #8420385 - Flags: review?(dkeeler) → review+
Comment on attachment 8420385 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (

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

It looks like b2g does not define nullptr, so I'm going to have to use 0.

https://tbpl.mozilla.org/?tree=Try&rev=479388bd6f95

::: security/manager/tools/genHPKPStaticPins.js
@@ +519,5 @@
> +let [ chromeNameToHash, chromeNameToMozName ] = downloadAndParseChromeCerts(
> +  CHROME_CERT_SOURCE, certSKDToName);
> +let [ chromeImportedPinsets, chromeImportedEntries ] =
> +  downloadAndParseChromePins(CHROME_JSON_SOURCE, chromeNameToHash,
> +    chromeNameToMozName, certNameToSKD, certSKDToName);

81 chars :) I think I got everything else. I won't upload another attachment so Camilo's comments appear in the same diff.
(Reporter)

Comment 32

3 years ago
Comment on attachment 8420385 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (

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

Almost there. We should discuss the test-issue no on this bug, so that you dont have to have another round of reviews.

::: security/manager/boot/src/PublicKeyPinningService.cpp
@@ +107,5 @@
> +    const StaticFingerprints* fingerprints = nullptr;
> +    if (hashType == SEC_OID_SHA256) {
> +      fingerprints = pinset->sha256;
> +    } else if (hashType == SEC_OID_SHA1) {
> +      fingerprints = pinset->sha1;

What I seem to have been unable to express PROPIATELY is that this block (from const StaticFingerprints* fingerprints = nullptr; including the elseif branch) should be at the top of this function. The check for if (!fingerprints) also there so we can return early.

::: security/manager/boot/src/StaticHPKPins.h
@@ +464,5 @@
> +
> +/* Sort hostnames for binary search. */
> +static const TransportSecurityPreload kPublicKeyPinningPreloadList[] = {
> +  { "accounts.google.com", true, true, &kPinset_google },
> +  { "addons.mozilla.net", true, true, &kPinset_mozilla },

Why are mozilla sites on the test-set? (No previous discussion, since we agreed so set the pref to off my impression is that that would be left to non-test)

::: security/manager/tools/PreloadedHPKPins.json
@@ +34,2 @@
>        "name": "mozilla",
> +      "sha256_hashes": [

This is a bad name for this file (did I missed your comment on why this was OK?)

::: security/manager/tools/genHPKPStaticPins.js
@@ +118,5 @@
>    }
>    return false;
>  }
>  
> +function download(filename) {

we should have some sense of retries as this would be run automatically and we want to be somewhat safe against network transient problems.
Attachment #8420385 - Flags: review?(cviecco) → review-
Created attachment 8421109 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (
Attachment #8420385 - Attachment is obsolete: true
Comment on attachment 8421109 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (

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

Thanks for the review.
(In reply to Camilo Viecco (:cviecco) from comment #32)
> What I seem to have been unable to express PROPIATELY is that this block
> (from const StaticFingerprints* fingerprints = nullptr; including the elseif
> branch) should be at the top of this function. The check for if
> (!fingerprints) also there so we can return early.

Fixed (finally, I think).
 
> ::: security/manager/boot/src/StaticHPKPins.h
> @@ +464,5 @@
> > +
> > +/* Sort hostnames for binary search. */
> > +static const TransportSecurityPreload kPublicKeyPinningPreloadList[] = {
> > +  { "accounts.google.com", true, true, &kPinset_google },
> > +  { "addons.mozilla.net", true, true, &kPinset_mozilla },
> 
> Why are mozilla sites on the test-set? (No previous discussion, since we
> agreed so set the pref to off my impression is that that would be left to
> non-test)

As discussed over vidyo, let's try to get errors through test-mode telemetry rather than user reports.

> ::: security/manager/tools/PreloadedHPKPins.json
> @@ +34,2 @@
> >        "name": "mozilla",
> > +      "sha256_hashes": [
> 
> This is a bad name for this file (did I missed your comment on why this was
> OK?)

Leaving as discussed (also it makes processing pinset objects much easier if they are consistent between Mozilla and Chrome, which includes both hash types).

> ::: security/manager/tools/genHPKPStaticPins.js
> @@ +118,5 @@
> >    }
> >    return false;
> >  }
> >  
> > +function download(filename) {
> 
> we should have some sense of retries as this would be run automatically and
> we want to be somewhat safe against network transient problems.

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1009076.

Thanks for the tip about pkix/nullptr.h, trying that now: https://tbpl.mozilla.org/?tree=Try&rev=4264616924a1

If it doesn't work I will revert back to 0.
Attachment #8421109 - Flags: review?(cviecco)
(Reporter)

Comment 35

3 years ago
Comment on attachment 8421109 [details] [diff] [review]
Implement sha1 support, import Chrome's pinsets wholesale, add test mode (

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

r+ with nit addressed.

::: security/manager/boot/src/PublicKeyPinningService.cpp
@@ +96,2 @@
>    CERTCertificate* currentCert;
> +  CERTCertListNode* node;

nit move this to just before the for loop.
Attachment #8421109 - Flags: review?(cviecco) → review+
Try looked good, so pushed.

https://tbpl.mozilla.org/?tree=Try&rev=4264616924a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/91959a8f21ae
https://hg.mozilla.org/mozilla-central/rev/91959a8f21ae
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(Reporter)

Updated

3 years ago
Blocks: 1009635
You need to log in before you can comment on or make changes to this bug.