1306471 - Modify the SiteSecurityService to allow dynamic pin preloads

Status: RESOLVED FIXED

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

Description

[Mark Goodwin [:mgoodwin]]

We need to expose some means of adding public key preloads to the site security service.

Comment 1

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 1306471 - Modify the SiteSecurityService to allow dynamic pin preloads

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

Comment 2

[:Cykesiopka]

Comment on attachment 8796328 [details]
Bug 1306471 - Modify the SiteSecurityService to allow dynamic pin preloads

https://reviewboard.mozilla.org/r/82216/#review80966

Forgive the drive by review - I was skimming the patch and noticed some style nits.

::: security/manager/ssl/nsSiteSecurityService.cpp:1149
(Diff revision 1)
> +   // Child processes are not allowed direct access to this.
> +   if (!XRE_IsParentProcess()) {
> +     MOZ_CRASH("Child process: no direct access to nsISiteSecurityService::ClearAll");
> +   }

Nit: Indent these lines by one less space.

::: security/manager/ssl/nsSiteSecurityService.cpp:1201
(Diff revision 1)
> +      value = mPreloadStateStorage->Get(storageKey,
> +                                        mozilla::DataStorage_Persistent);
> +      SiteHPKPState preloadEntry(value);
> +      if (preloadEntry.mState != SecurityPropertySet ||
> +          preloadEntry.IsExpired(aEvalTime) ||
> +          preloadEntry.mSHA256keys.Length() < 1 ) {

Nit: Get rid of the space between the 1 and the ).

::: security/manager/ssl/tests/unit/test_pinning_dynamic.js:91
(Diff revision 1)
>                               "x.b.pinning2.example.com", 0),
>       "x.b.pinning2.example.com should have HPKP status");
>  }
>  
>  function checkStateRead(aSubject, aTopic, aData) {
> -  equal(aData, SSS_STATE_FILE_NAME,
> +  if (aData !== SSS_STATE_FILE_NAME) {

Nit: Our style guide says "== is preferred to ===", so this should probably be ```!=```.

::: security/manager/ssl/tests/unit/test_pinning_dynamic.js:198
(Diff revision 1)
> +  checkFail(certFromFile('a.preload.example.com-badca'), "a.preload.example.com");
> +  checkOK(certFromFile('a.preload.example.com-pinningroot'), "a.preload.example.com");

Nit: Use double quotes (I plan on eventually turning on an ESLint rule that enforces this under most situations).
Same below.

Comment 3

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

Comment on attachment 8796328 [details]
Bug 1306471 - Modify the SiteSecurityService to allow dynamic pin preloads

https://reviewboard.mozilla.org/r/82216/#review80868

In general this looks good - this is all working how we want. I do want to double-check the tests after they get updated, so I'll just clear the review for now.

::: security/manager/ssl/nsISiteSecurityService.idl:146
(Diff revision 1)
>       */
>      boolean isSecureURI(in uint32_t aType, in nsIURI aURI, in uint32_t aFlags,
>                          [optional] out boolean aCached);
>  
>      /**
> -     * Removes all security state by resetting to factory-original settings.
> +     * Removes all private security state by resetting to factory-original

This was intended to remove information from any headers encountered in normal or private browsing. Maybe "non-preloaded" would be a better description?

::: security/manager/ssl/nsISiteSecurityService.idl:188
(Diff revision 1)
>       * @param aSha256Pins array of hashed key fingerprints (SHA-256, base64)
>       */
>       boolean setKeyPins(in string aHost, in boolean aIncludeSubdomains,
> -                        in unsigned long aMaxAge, in unsigned long aPinCount,
> -                        [array, size_is(aPinCount)] in string aSha256Pins);
> +                        in int64_t aExpires, in unsigned long aPinCount,
> +                        [array, size_is(aPinCount)] in string aSha256Pins,
> +                        [optional] in boolean aIsPreload);

nit: add documentation for aIsPreload

::: security/manager/ssl/nsSiteSecurityService.h:146
(Diff revision 1)
>                              bool* aIncludeSubdomains, uint32_t* aFailureResult);
>    nsresult ProcessPKPHeader(nsIURI* aSourceURI, const char* aHeader,
>                              nsISSLStatus* aSSLStatus, uint32_t flags,
>                              uint64_t* aMaxAge, bool* aIncludeSubdomains,
>                              uint32_t* aFailureResult);
> -  nsresult SetHPKPState(const char* aHost, SiteHPKPState& entry, uint32_t flags);
> +  nsresult SetHPKPState(const char* aHost, SiteHPKPState& entry, uint32_t flags, bool aIsPreload);

nit: long line

::: security/manager/ssl/nsSiteSecurityService.cpp:258
(Diff revision 1)
>    bool storageWillPersist = false;
>    nsresult rv = mSiteStateStorage->Init(storageWillPersist);
>    if (NS_WARN_IF(NS_FAILED(rv))) {
>      return rv;
>    }
> +  rv = mPreloadStateStorage->Init(storageWillPersist);

Maybe we should have two different booleans here for each call to DataStorage::Init (in theory, we could perhaps get two different answers, although in practice I imagine we won't). Or maybe we should check the boolean after each call? (Although then in most cases we would warn twice, which seems a little sloppy.)

::: security/manager/ssl/nsSiteSecurityService.cpp:1151
(Diff revision 1)
>  NS_IMETHODIMP
> +nsSiteSecurityService::ClearPreloads()
> +{
> +   // Child processes are not allowed direct access to this.
> +   if (!XRE_IsParentProcess()) {
> +     MOZ_CRASH("Child process: no direct access to nsISiteSecurityService::ClearAll");

nit: update error text (s/ClearAll/ClearPreloads/)

::: security/manager/ssl/nsSiteSecurityService.cpp:1199
(Diff revision 1)
>          privateEntry.mSHA256keys.Length() < 1 ) {
> +      // not in private storage, try dynamic preload
> +      value = mPreloadStateStorage->Get(storageKey,
> +                                        mozilla::DataStorage_Persistent);
> +      SiteHPKPState preloadEntry(value);
> +      if (preloadEntry.mState != SecurityPropertySet ||

There's a bit of repetition here - let's add a small helper function.

::: security/manager/ssl/tests/unit/test_pinning_dynamic.js:91
(Diff revision 1)
>                               "x.b.pinning2.example.com", 0),
>       "x.b.pinning2.example.com should have HPKP status");
>  }
>  
>  function checkStateRead(aSubject, aTopic, aData) {
> -  equal(aData, SSS_STATE_FILE_NAME,
> +  if (aData !== SSS_STATE_FILE_NAME) {

We need this test to only run after both the site security service state and the security preload files have been read, right? So let's do something more like:

* If aData is something completely unexpected, note a test failure.
* Keep track of which notifications this function has received, and only proceed with the test when it's seen both of them (i.e. return otherwise, as it's already doing).

::: security/manager/ssl/tests/unit/test_pinning_dynamic.js:126
(Diff revision 1)
>    checkDefaultSiteHPKPStatus();
>  
>  
>    // add includeSubdomains to a.pinning2.example.com
> -  gSSService.setKeyPins("a.pinning2.example.com", true, 1000, 2,
> +  gSSService.setKeyPins("a.pinning2.example.com", true,
> +                        new Date().getTime() + 1000, 2,

Originally these lived for 1000 seconds, so we probably want to add 1000000 milliseconds to the value from getTime for all of these.

::: security/manager/ssl/tests/unit/test_pinning_dynamic.js:194
(Diff revision 1)
>       "Not built-in nonexistent.example.com should not have HPKP status");
>    ok(gSSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HPKP,
>                               "include-subdomains.pinning.example.com", 0),
>       "Built-in include-subdomains.pinning.example.com should have HPKP status");
>  
>    gSSService.setKeyPins("a.pinning2.example.com", false, 0, 1,

We should add a comment here that this is what checkExpiredState is checking expired (this confused me a bit before I realized what was going on). In fact, we should probably update this 0 to `new Date().getTime()`.

::: security/manager/ssl/tests/unit/test_pinning_dynamic.js:225
(Diff revision 1)
>    checkFail(certFromFile('b.pinning2.example.com-badca'), "b.pinning2.example.com");
>    checkOK(certFromFile('b.pinning2.example.com-pinningroot'), "b.pinning2.example.com");
>    checkFail(certFromFile('x.b.pinning2.example.com-badca'), "x.b.pinning2.example.com");
>    checkOK(certFromFile('x.b.pinning2.example.com-pinningroot'), "x.b.pinning2.example.com");
>  
> +  // Check that the preloaded pins still work after private data is cleared

If I'm understanding correctly, these testcases don't have to do with expiration, so we can just include them in the previous test block.

::: security/manager/ssl/tests/unit/test_pinning_dynamic/a.pinning2.example.com-pinningroot.pem:1
(Diff revision 1)
>  -----BEGIN CERTIFICATE-----

This file seems to have unrelated changes (I imagine it's the newline-at-end-of-file, which many of these files don't have for some reason). Let's just leave this out for now and address all of the affected files at once in another bug.

::: security/manager/ssl/tests/unit/test_pinning_dynamic/a.preload.example.com-badca.pem:1
(Diff revision 1)
> +-----BEGIN CERTIFICATE-----

For all of these, we need the commented-out changes to test_pinning_dynamic/moz.build

::: security/manager/ssl/tests/unit/test_sss_eviction.js:13
(Diff revision 1)
>  
>  var gSSService = null;
>  var gProfileDir = null;
>  
>  function do_state_written(aSubject, aTopic, aData) {
> -  equal(aData, SSS_STATE_FILE_NAME);
> +  if (aData !== SSS_STATE_FILE_NAME) {

For all of these tests that aren't testing anything specific to the preload file, I think we should rework this slightly to make it more clear that we're essentially ignoring that notification. I would do something more like:

`
if (aData == PRELOAD_STATE_FILE_NAME) {
  return;
}

equal(aData, SSS_STATE_FILE_NAME);
... (continue as before)
`

Comment 4

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8796328 [details]
Bug 1306471 - Modify the SiteSecurityService to allow dynamic pin preloads

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

Comment 5

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8796328 [details]
Bug 1306471 - Modify the SiteSecurityService to allow dynamic pin preloads

https://reviewboard.mozilla.org/r/82216/#review80868

> If I'm understanding correctly, these testcases don't have to do with expiration, so we can just include them in the previous test block.

These do actually need to run after the other tests have finished because otherwise clearing the data (which we need to test) interferes with the other things. I've moved this stuff to checkPreloadClear()

Comment 6

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8796328 [details]
Bug 1306471 - Modify the SiteSecurityService to allow dynamic pin preloads

https://reviewboard.mozilla.org/r/82216/#review81974

::: security/manager/ssl/tests/unit/test_sss_savestate.js:105
(Diff revision 2)
>    gProfileDir = do_get_profile();
>    let SSService = Cc["@mozilla.org/ssservice;1"]
>                      .getService(Ci.nsISiteSecurityService);
>    // Put an HPKP entry
> -  SSService.setKeyPins("dynamic-pin.example.com", true, 1000, 1,
> -                       [NON_ISSUED_KEY_HASH]);
> +  SSService.setKeyPins("dynamic-pin.example.com", true,
> +                       new Date().getTime() + 1000, 1, [NON_ISSUED_KEY_HASH]);

This should be 1000000

Comment 7

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

Comment on attachment 8796328 [details]
Bug 1306471 - Modify the SiteSecurityService to allow dynamic pin preloads

https://reviewboard.mozilla.org/r/82216/#review82318

Cool - looks good to me with comments addressed.

::: security/manager/ssl/nsISiteSecurityService.idl:184
(Diff revisions 1 - 2)
>       * @param aHost the hostname (punycode) that pins will apply to
>       * @param aIncludeSubdomains whether these pins also apply to subdomains
>       * @param aExpires the time this pin should expire (millis since epoch)
>       * @param aPinCount number of keys being pinnned
>       * @param aSha256Pins array of hashed key fingerprints (SHA-256, base64)
> +     * @param aIsPreload are these key pins for a preload entry?

nit: let's add that the default is false

::: security/manager/ssl/nsSiteSecurityService.cpp:1158
(Diff revisions 1 - 2)
> -   }
> +  }
>  
>    return mPreloadStateStorage->Clear();
>  }
>  
> +bool entryStateOK(SiteHPKPState& state, mozilla::pkix::Time& aEvalTime) {

I think the function name here is logically flipped from what it actually does (so we should either change the name or negate the expression and then negate the result when calling it).

::: security/manager/ssl/tests/unit/test_pinning_dynamic.js:93
(Diff revisions 1 - 2)
>                               "x.b.pinning2.example.com", 0),
>       "x.b.pinning2.example.com should have HPKP status");
>  }
>  
>  function checkStateRead(aSubject, aTopic, aData) {
> -  if (aData !== SSS_STATE_FILE_NAME) {
> +  dump("file is "+aData);

nit: we probably don't need the debugging output any more

::: security/manager/ssl/tests/unit/test_sss_eviction.js:48
(Diff revision 2)
>    ok(foundLegitSite);
>    do_test_finished();
>  }
>  
>  function do_state_read(aSubject, aTopic, aData) {
> -  equal(aData, SSS_STATE_FILE_NAME);
> +  if (aData !== SSS_STATE_FILE_NAME) {

Let's do the same thing here and just add
`
  if (aData == PRELOAD_STATE_FILE_NAME) {
    return;
  }
`

::: security/manager/ssl/tests/unit/test_sss_readstate_huge.js:23
(Diff revision 2)
> +    return;
> +  }
> +
>    equal(aData, SSS_STATE_FILE_NAME);
>  
> +

nit: probably don't need this extra blank line

Comment 8

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8796328 [details]
Bug 1306471 - Modify the SiteSecurityService to allow dynamic pin preloads

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

Comment 9

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9379945911f8

Comment 10

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8796328 [details]
Bug 1306471 - Modify the SiteSecurityService to allow dynamic pin preloads

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9c47d6024ba
Modify the SiteSecurityService to allow dynamic pin preloads r=keeler

Comment 12

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

Eslint complained with delay about the |return| after the |throw|, so I removed the |return|:
https://hg.mozilla.org/integration/autoland/rev/a9c47d6024ba#l5.79

https://hg.mozilla.org/integration/autoland/rev/3745b8ba92b1faf5da9762831b72de987b962b68

Comment 13

[Phil Ringnalda (:philor)]

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