Closed Bug 998057 Opened 6 years ago Closed 5 years ago

Certificate pinning needs more tests

Categories

(Core :: Security: PSM, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: mmc, Assigned: mmc)

References

Details

Attachments

(2 files, 3 obsolete files)

No description provided.
No longer blocks: 1002696
Attached patch Add test pinset to the generator (obsolete) — Splinter Review
Assignee: nobody → mmc
Attachment #8415343 - Flags: review?(cviecco)
Comment on attachment 8415343 [details] [diff] [review]
Add test pinset to the generator

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

Since StaticHPKPins.h depends on values output by generate_certs.sh. You should include a comment on generate_certs.sh to let users know that they are required to also run genHPKPStaticPins.js

::: security/manager/boot/src/PreloadedHPKPins.json
@@ +95,5 @@
>      { "name": "addons.mozilla.net", "include_subdomains": true, "pins": "mozilla" },
>      { "name": "cdn.mozilla.net", "include_subdomains": true, "pins": "mozilla_cdn" },
>      { "name": "cdn.mozilla.org", "include_subdomains": true, "pins": "mozilla_cdn" },
>      { "name": "media.mozilla.com", "include_subdomains": true, "pins": "mozilla_cdn" },
> +    { "name": "getpersonas.org", "include_subdomains": true, "pins": "mozilla_cdn" },

getpersonas.org should not be in this list yet. Please remove (this was error introduced by me)

@@ +96,5 @@
>      { "name": "cdn.mozilla.net", "include_subdomains": true, "pins": "mozilla_cdn" },
>      { "name": "cdn.mozilla.org", "include_subdomains": true, "pins": "mozilla_cdn" },
>      { "name": "media.mozilla.com", "include_subdomains": true, "pins": "mozilla_cdn" },
> +    { "name": "getpersonas.org", "include_subdomains": true, "pins": "mozilla_cdn" },
> +    { "name": "pinning.example.com", "include_subdomains": true, "pins": "mozilla_test" }

can you change this so that we have two entries:
subdomainsincluded.pinningtest.example.com

and
nosubdomains.pinningtest.example.com (include_subdomains: false)

::: security/manager/boot/src/genHPKPStaticPins.js
@@ +7,5 @@
>  // 2. [build/obtain firefox binaries]
>  // 3. run `[path to]/run-mozilla.sh [path to]/xpcshell \
>  //                                  [path to]/genHPKPStaticpins.js
> +// Files PreloadedHPKPins.json and default-ee.der must be in the current
> +// working directory.

You did not included default-ee.der in this patch. We should change this to either 
a. have the file locations as parameters 
b. hardcode the paths and require the script to be run from a particular location.
c. copy default-ee.der into the current dir (seems kind of bad)
Attachment #8415343 - Flags: review?(cviecco) → review-
Thanks for the review. I addressed all comments and picked option 3) to copy default-ee.der into the boot/src directory. The better, more permanent fix will come in bug 1002696.
Attachment #8415343 - Attachment is obsolete: true
Attachment #8415365 - Flags: review?(cviecco)
Comment on attachment 8415365 [details] [diff] [review]
Update generator to include test pinsets

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

r+ with comments addressed

::: security/manager/boot/src/PreloadedHPKPins.json
@@ +95,5 @@
>      { "name": "addons.mozilla.net", "include_subdomains": true, "pins": "mozilla" },
>      { "name": "cdn.mozilla.net", "include_subdomains": true, "pins": "mozilla_cdn" },
>      { "name": "cdn.mozilla.org", "include_subdomains": true, "pins": "mozilla_cdn" },
>      { "name": "media.mozilla.com", "include_subdomains": true, "pins": "mozilla_cdn" },
> +    { "name": "included.pinning.example.com", "include_subdomains": true, "pins": "mozilla_test" },

change names to somethin like: with-subdomains.pinning.example.com

::: security/manager/boot/src/genHPKPStaticPins.js
@@ +129,5 @@
> +    let certdb = certdb2.QueryInterface(Ci.nsIX509CertDB);
> +    let testCert = certdb.constructX509(der, der.length);
> +    // We can't include this cert in the previous loop, because it skips
> +    // non-builtin certs and the nickname is not built-in to the cert.
> +    let name = "testCA";

this is not a testCA, rename to something like : EE testing cert
Attachment #8415365 - Flags: review?(cviecco) → review+
Attachment #8415365 - Attachment is obsolete: true
Comment on attachment 8415396 [details] [diff] [review]
Add test pinset to the pin generator (

Thanks, fixed and not checked in because the tree is closed.
Attachment #8415396 - Flags: review+
Attachment #8415396 - Attachment is obsolete: true
Attachment #8415607 - Flags: review?(dkeeler)
Attachment #8415607 - Flags: review?(cviecco)
Comment on attachment 8415607 [details] [diff] [review]
Add tests for certificate pinning (

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

::: security/certverifier/CertVerifier.cpp
@@ +171,5 @@
>      if (CERT_LIST_END(CERT_LIST_NEXT(node), certList)) {
>        bool isBuiltInRoot = false;
>        SECStatus srv = IsCertBuiltInRoot(currentCert, isBuiltInRoot);
> +      if (isBuiltInRoot) {
> +        PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, ("Builtin root"));

I will remove these.
Comment on attachment 8415400 [details] [diff] [review]
Add test pinset to the pin generator (

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

::: security/manager/boot/src/genHPKPStaticPins.js
@@ +122,5 @@
>      certSDKToName[SDK] = name;
>    }
> +  {
> +    // A certificate for *.example.com.
> +    let der = readFileToString("default-ee.der");

Can we take as an argument the path to the folder containing default-ee.der instead of having to copy it in the tree?
Comment on attachment 8415607 [details] [diff] [review]
Add tests for certificate pinning (

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

Looks good with comments addressed.

::: security/manager/boot/src/PreloadedHPKPins.json
@@ +83,3 @@
>        ]
>      },
>      // For pinning tests on pinning.example.com, the issuer must be testCA.

Should be "the certificate must be 'End Entity TestCert'"

@@ +87,5 @@
>        "name": "mozilla_test",
>        "static_spki_hashes": [
>          "End Entity Test Cert"
>        ]
> +    } ],

nit: should the ] be on the next line?

@@ +95,5 @@
>      { "name": "addons.mozilla.net", "include_subdomains": true, "pins": "mozilla" },
>      { "name": "cdn.mozilla.net", "include_subdomains": true, "pins": "mozilla_cdn" },
>      { "name": "cdn.mozilla.org", "include_subdomains": true, "pins": "mozilla_cdn" },
>      { "name": "media.mozilla.com", "include_subdomains": true, "pins": "mozilla_cdn" },
> +    { "name": "include-subdomains.pinning.example.com", "include_subdomains": true, "pins": "mozilla_test" },

I'm wondering if we should include "mozilla" somewhere in this name so if there's a name collision we only have ourselves to blame, instead of stomping on some third party's party

::: security/manager/ssl/tests/unit/test_cert_overrides.js
@@ +229,5 @@
>                                      expectedResultBefore, expectedResultAfter) {
>    let certToDistrust = constructCertFromFile(certFileName);
>  
>    add_test(function () {
> +    // Add an entry to the override service that says to distrust the cert

It's actually the NSS certificate db that gets the entry that says to distrust the cert.

::: security/manager/ssl/tests/unit/test_pinning.js
@@ +1,1 @@
> +// For all cases, the acceptable pinset includes only certificates pinned to

nit: modeline and license boilerplate

@@ +20,5 @@
> +"use strict";
> +
> +do_get_profile(); // must be called before getting nsIX509CertDB
> +const certdb = Cc["@mozilla.org/security/x509certdb;1"]
> +                  .getService(Ci.nsIX509CertDB);

nit: move one space left

@@ +25,5 @@
> +
> +function test_strict() {
> +  // In strict mode, we always evaluate pinning data, regardless of whether the
> +  // issuer is a built-in trust anchor.
> +  add_test(function () {

nit: no space after "function" (although in some places we do do this...)

::: security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp
@@ +28,1 @@
>  const BadCertHost sBadCertHosts[] =

So... this isn't strictly about bad certs anymore - maybe file a follow-up bug to re-name this something more inclusive/descriptive?

::: security/manager/ssl/tests/unit/tlsserver/generate_certs.sh
@@ +139,5 @@
>  
>  make_CA testCA 'CN=Test CA' test-ca.der
>  make_CA otherCA 'CN=Other test CA' other-test-ca.der
> +
> +# For pinning tests, we always use this certificate in the pinset.

I'm not sure this comment is helpful, since we use this certificate elsewhere as well.

::: security/manager/ssl/tests/unit/xpcshell.ini
@@ +65,5 @@
>  [test_cert_eku.js]
>  # Bug 989485 : this test this fails on slow devices
>  skip-if = os == "android" || (buildapp == "b2g" && processor == "arm")
>  requesttimeoutfactor = 4
> +[test_pinning.js]

We'll need to skip on b2g and android, I think, because these tests don't quite work there yet.
Attachment #8415607 - Flags: review+
Comment on attachment 8415607 [details] [diff] [review]
Add tests for certificate pinning (

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

r+ (plus keeler b2g android comment)

::: security/manager/boot/src/PreloadedHPKPins.json
@@ +87,5 @@
>        "name": "mozilla_test",
>        "static_spki_hashes": [
>          "End Entity Test Cert"
>        ]
> +    } ],

Since you are modifing this, I would prefer the '],' on the next line
Attachment #8415607 - Flags: review?(cviecco) → review+
(In reply to David Keeler (:keeler) from comment #11)
> Comment on attachment 8415400 [details] [diff] [review]
> Add test pinset to the pin generator (
> 
> Review of attachment 8415400 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: security/manager/boot/src/genHPKPStaticPins.js
> @@ +122,5 @@
> >      certSDKToName[SDK] = name;
> >    }
> > +  {
> > +    // A certificate for *.example.com.
> > +    let der = readFileToString("default-ee.der");
> 
> Can we take as an argument the path to the folder containing default-ee.der
> instead of having to copy it in the tree?

This will be fixed in bug 1002696. Fixed other comments except for the domain name change, per our discussion.

Try looked good, so checking in:
https://tbpl.mozilla.org/?tree=Try&rev=88ffc63004a9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9c8fbf297d51
And, backed out because 744204 did not actually land yet. Land, Camilo!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/658330efb152
(In reply to Camilo Viecco (:cviecco) from comment #16)
> as this not landing?
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/affd460bc3d7

Ah, my bad -- I checked bug 744204 and didn't see anything landed, and also didn't scroll past the first page of tbpl/inbound.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f39b83a39cf2
https://hg.mozilla.org/mozilla-central/rev/762f8c20a109
https://hg.mozilla.org/mozilla-central/rev/f39b83a39cf2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.