Modify the SiteSecurityService to allow dynamic pin preloads

RESOLVED FIXED in Firefox 52

Status

()

Core
Security: PSM
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mgoodwin, Assigned: mgoodwin)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [psm-assigned])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
We need to expose some means of adding public key preloads to the site security service.
Comment hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
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.

Updated

2 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [psm-assigned]

Comment 3

2 years ago
mozreview-review
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)
`
Attachment #8796328 - Flags: review?(dkeeler)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 years ago
mozreview-review-reply
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()
(Assignee)

Comment 6

2 years ago
mozreview-review
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

2 years ago
mozreview-review
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
Attachment #8796328 - Flags: review?(dkeeler) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

2 years ago
Pushed by mgoodwin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a9c47d6024ba
Modify the SiteSecurityService to allow dynamic pin preloads r=keeler

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a9c47d6024ba
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.