Closed Bug 1306470 Opened 4 years ago Closed 3 years ago

Create a services client for augmenting the PKP preload list between releases

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: mgoodwin, Assigned: mgoodwin)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

The services blocklist client provides a mechanism we can use to get public key pin preloads to the browser between releases.

Let's build a client to do this.
Depends on: 1306471
Priority: -- → P1
Whiteboard: [psm-assigned]
Some notes on the incoming patch:
Because of the fact that our binaries ship with a static preload list (see StaticHPKPins.h), we need to maintain a different collection of dynamic preloads for each firefox version.

To avoid sync state issues, we must keep track of which collection we expect to be using (we don't want to do partial sync from some version of one collection to some version of another). We also don't want to store all preloads for all binaries so we need some mechanism to clear entries; this means we need to track which collection we used last time we synced with the server (so that when the collection in use changes, we can clear the data).
                                                                                
Again, in this patch, we're up against naming issues. We're adding a client to blocklist-clients.js which isn't a blocklist. Maybe now isn't the time to address this particular issue - but I thought I'd highlight it.

We can't actually land this until we've had a signing cert created for it and we're ready to serve production and stage collections.
Depends on: 1309257
Comment on attachment 8799788 [details]
Bug 1306470 - Create a services client for augmenting the PKP preload list between releases.

https://reviewboard.mozilla.org/r/84914/#review83926

::: modules/libpref/init/all.js:2202
(Diff revision 1)
>  pref("services.blocklist.addons.collection", "addons");
>  pref("services.blocklist.addons.checked", 0);
>  pref("services.blocklist.plugins.collection", "plugins");
>  pref("services.blocklist.plugins.checked", 0);
> +pref("services.blocklist.pinning.collection", "fx52_pins");
> +pref("services.blocklist.pinning.last_collection", "fx52_pins");

Drive-by comment from a release engineer - looks like these prefs which will need bumping on mozilla-central each merge cycle. We could do this in script which handles the merges, but did you consider constructing the collection name at runtime ?
(In reply to Nick Thomas [:nthomas] from comment #3)
> did you consider constructing the collection
> name at runtime ?

That's a good idea. I'll try something like that.
Comment on attachment 8799788 [details]
Bug 1306470 - Create a services client for augmenting the PKP preload list between releases.

https://reviewboard.mozilla.org/r/84914/#review84476

::: services/common/blocklist-clients.js:271
(Diff revision 1)
>  }
>  
> +function* updatePinningList(records) {
> +  let collectionChanged = false;
> +  let previousCollectionName
> +      = Services.prefs.getCharPref(PREF_BLOCKLIST_LAST_PINNING_COLLECTION);

nitpick: use `const`

::: services/common/blocklist-clients.js:274
(Diff revision 1)
> +  let collectionChanged = false;
> +  let previousCollectionName
> +      = Services.prefs.getCharPref(PREF_BLOCKLIST_LAST_PINNING_COLLECTION);
> +
> +  if (this.collectionName != previousCollectionName) {
> +    collectionChanged = true;

Maybe add a comment that `this` is a blocklist client since this method is used as a `processCallback`

::: services/common/blocklist-clients.js:280
(Diff revision 1)
> +    // if there is an existing collection, clear it (we don't want profiles
> +    // to accumulate entries relating to previous versions)
> +    if (previousCollectionName) {
> +      let db = kintoClient();
> +      let collection = db.collection(previousCollectionName, {});
> +      try {

nitpick: superfluous empty options

::: services/common/blocklist-clients.js:293
(Diff revision 1)
> +      }
> +    }
> +  }
> +
> +  let siteSecurityService = Cc["@mozilla.org/ssservice;1"]
> +                              .getService(Ci.nsISiteSecurityService);

nitpick: use `const`

::: services/common/blocklist-clients.js:303
(Diff revision 1)
> +  // write each KeyPin entry to the preload list
> +  for (let item of records) {
> +    try {
> +      if (item.pinType && item.pinType == "KeyPin" && item.pins &&
> +          item.pins.length) {
> +        siteSecurityService.setKeyPins(item.hostName,

A more idiomatic way would be:
```js
const {pinType, pins=[]} = item;
if (pinType == "KeyPin" && pins.length > 0) {
   ...
}
```

::: services/common/blocklist-clients.js:305
(Diff revision 1)
> +    try {
> +      if (item.pinType && item.pinType == "KeyPin" && item.pins &&
> +          item.pins.length) {
> +        siteSecurityService.setKeyPins(item.hostName,
> +                                      item.includeSubdomains,
> +                                      item.expires,

nitpick: indentation

::: services/common/blocklist-clients.js:317
(Diff revision 1)
> +      // 1254099.
> +    }
> +  }
> +
> +  // if the collection has changed, set pref at
> +  if (collectionChanged) {

*update pref value with new collection name*

::: services/common/tests/unit/test_pin_preloads.js:28
(Diff revision 1)
> +function do_get_kinto_collection(collectionName) {
> +  if (!kintoClient) {
> +    let config = {
> +      // Set the remote to be some server that will cause test failure when
> +      // hit since we should never hit the server directly, only via maybeSync()
> +      remote: "https://firefox.settings.services.mozilla.com/v1/",

The URL below is valid. Is the comment still in sync?

::: services/common/tests/unit/test_pin_preloads.js:49
(Diff revision 1)
> +                             COLLECTION_NAME);
> +
> +  const { PinningPreloadClient } = Cu.import("resource://services-common/blocklist-clients.js");
> +
> +
> +

nitpick: too many blanklines

::: services/common/tests/unit/test_pin_preloads.js:139
(Diff revision 1)
> +  // Check that a sync completes even when there's bad data in the
> +  // collection. This will throw on fail, so just calling maybeSync is an
> +  // acceptible test.
> +  Services.prefs.setCharPref("services.settings.server",
> +                             `http://localhost:${server.identity.primaryPort}/v1`);
> +  yield PinningPreloadClient.maybeSync(5000, Date.now());

Maybe add a note to explain that sample data with last_modified 5000 below is non sense
Comment on attachment 8799788 [details]
Bug 1306470 - Create a services client for augmenting the PKP preload list between releases.

https://reviewboard.mozilla.org/r/84914/#review85312

I'll defer review to leplatrem for this bug as I don't think there's anything that wasn't already said that affects Fx integration.
Attachment #8799788 - Flags: review?(MattN+bmo)
Comment on attachment 8799788 [details]
Bug 1306470 - Create a services client for augmenting the PKP preload list between releases.

https://reviewboard.mozilla.org/r/84914/#review84476

> The URL below is valid. Is the comment still in sync?

Yes; to explain further, any non-local request will cause a test failure. I've modified the comment to make this clearer.
Will this capability allow us to remove pins from the pin list, either from our preloaded one or ones dynamically added by the client in response to HTTP headers? To put it another way, if someone has HPKP-bricked their site, can we use this mechanism to unbrick them?

Gerv
(In reply to Gervase Markham [:gerv] from comment #9)
> if someone has HPKP-bricked their site,
> can we use this mechanism to unbrick them?

Not currently. :tjr actually suggested exactly this to me yesterday. Currently we check preloads after dynamic state from browsing. Maybe we could clobber entries if a preload entry appears that clears state?  Did you have more thoughts on this, Tom?
Flags: needinfo?(tom)
(In reply to Gervase Markham [:gerv] from comment #9)
> Will this capability allow us to remove pins from the pin list, either from
> our preloaded one or ones dynamically added by the client in response to
> HTTP headers?

It is the case that this will clear preloaded pins (whether static preloads or dynamic) - just not dynamic state from browsing.
Yea, I think unbricking websites would be a useful feature and might help some website operators gain confidence in deployment. 

My first question is generally: how are we certain the person who is requesting a preload has the appropriate authority to do so. I assume we basically just check the current header they send aligns with what they're requesting. Do we require a 'preload' directive like for HSTS?  In any event, we would probably want to require them to add a new directive like 'unBrickMePlease="UUID"' which would tell us it's okay to delete any existing, dynamic entry. 

(We need the UUID there to prevent an attacker from spoofing an email at a later point in time. Imagine without the UUID: example.com puts unBrickMePlease in the header, we correspond with them, decide to ship an unbrick on 10/1. They never remove the directive. On 10/20 or 11/5 or whenever, someone spoofs an email and requests a new unbricking. With a UUID present we can be certain each unbrick request is unique. Assuming we record the UUID somewhere (like a commit message) and then check for duplicates!)

Alongside with the deletion we would edit their preload: either update (change the old bad data to the new good stuff), add (if they had nothing preloaded before), delete (if they were preloaded but are now scared off), or do nothing (if they had no preload and don't want to add one.)  This feature will already have the ability to do this part AIUI. 

It's just the other part: we would also want to be able to delete any dynamic stored entry (stored under any key, Private Browsing, 'Normal', or other Origin-Attribute based if that happens).  

It's a little awkward - we're actively reaching into people's profiles and removing a security state the website set for them. So it moves this service from 'Could be used to DOS people by preloading them with invalid pins' to 'Could be used to actively attack them'. So we should be cognizant of that fact and consider how we secure the update mechanism.
Flags: needinfo?(tom)
Comment on attachment 8799788 [details]
Bug 1306470 - Create a services client for augmenting the PKP preload list between releases.

See comment 6
Attachment #8799788 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8799788 [details]
Bug 1306470 - Create a services client for augmenting the PKP preload list between releases.

https://reviewboard.mozilla.org/r/84912/#review89592
Comment on attachment 8799788 [details]
Bug 1306470 - Create a services client for augmenting the PKP preload list between releases.

https://reviewboard.mozilla.org/r/84912/#review89594
Attachment #8799788 - Flags: review?(mathieu) → review+
Comment on attachment 8799788 [details]
Bug 1306470 - Create a services client for augmenting the PKP preload list between releases.

https://reviewboard.mozilla.org/r/84914/#review85450
What is the current status? Is there anything we can do to merge this?
Comment on attachment 8799788 [details]
Bug 1306470 - Create a services client for augmenting the PKP preload list between releases.

https://reviewboard.mozilla.org/r/84914/#review99324

::: services/common/blocklist-clients.js:352
(Diff revision 9)
> +);
> +
> +let processCallback = function() {};
> +if (Services.prefs.getBoolPref(PREF_BLOCKLIST_PINNING_ENABLED)) {
> +  processCallback = updatePinningList;
> +}

nitpick: Maybe it would be more elegant to have this test within the `updatePinningList()` (with an abortive `return;`)?

::: services/common/blocklist-updater.js:87
(Diff revision 9)
> -      }
> -
>        let collection = collectionInfo.collection;
>        let client = gBlocklistClients[collection];
> -      if (client && client.maybeSync) {
> +      if (client &&
> +          client.bucketName == collectionInfo.bucket && client.maybeSync) {

nitpick: I would put the last part of the new condition on a new line

::: services/common/tests/unit/test_blocklist_pinning.js:123
(Diff revision 9)
> +  // Our data now has three new records; all should be in the local collection
> +  connection = yield FirefoxAdapter.openConnection({path: KINTO_STORAGE_PATH});
> +  collection = do_get_kinto_collection(connection, COLLECTION_NAME);
> +  list = yield collection.list();
> +  do_check_eq(list.data.length, 4);
> +  yield connection.close();

Why can't we use the same collection object all along? And end the test with `yield collection.db.close()`?
(In reply to Mathieu Leplatre (:leplatrem) from comment #25)
> What is the current status? Is there anything we can do to merge this?

I'm awaiting QA approval. QA are waiting on the pinning updates being visible via the changes API in stage. My understanding (from speaking with Remy about this yesterday) is that this is currently with Jason.
(In reply to Mathieu Leplatre (:leplatrem) from comment #28)
> Comment on attachment 8799788 [details]
> nitpick: Maybe it would be more elegant to have this test within the
> `updatePinningList()` (with an abortive `return;`)?

Fixed.

> nitpick: I would put the last part of the new condition on a new line

Fixed.

> Why can't we use the same collection object all along? And end the test with
> `yield collection.db.close()`?

We can. Fixed.

(see updated review request)
Pushed by mgoodwin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3e31e8cddf83
Create a services client for augmenting the PKP preload list between releases. r=leplatrem
https://hg.mozilla.org/mozilla-central/rev/3e31e8cddf83
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1325309
You need to log in before you can comment on or make changes to this bug.