1280877 - Blocklists can be replaced by an outdated version.

Status: RESOLVED FIXED

(Cloud Services :: Firefox: Common, defect)

Description

[Mark Goodwin [:mgoodwin]]

The design of the blocklist client allows for collection state to be restored in the event of some problem with the integrity of the blocklist. This works by checking the signature of the blocklist on sync and, if the signature is not valid, requesting the complete blocklist. If the complete blocklist that has been fetched has a good signature, the old blocklist replaced with the new blocklist on the client.

There is a problem with this approach; if an attacker can generate invalid signatures (easy), obtain a previous collection state and signature (easy) and serve these to a client (with https, hard - CDN attack or similar - otherwise, easy) then they can downgrade the blocklist version on the client.

We should fix this. A good fix would be to check that the data in blocklists (or updates) is more recent than the last modified time of the local collection.

Comment 1

[Mark Goodwin [:mgoodwin]]

We could infer the last_modified from the data served during a sync (or fetch of the whole blocklist) - but this won't work for removals.

Is there some way we could include some last_modified data outside of the records in the collection state? Might another approach be to rev the last_modified on some record still in the collection?

Comment 2

[Tarek Ziadé (:tarek)]

I propose we add the collection last_modified key inside the payload at the top level.

e.g.

{'last_modified': xxx, 'data': <original data>}

Mathieu what do you think ? I guess this implies changes on both sides. Maybe we should add a "version" field as well so the next changes can be managed gracefully by clients ?

Comment 3

[Tarek Ziadé (:tarek)]

see https://github.com/Kinto/kinto/issues/687

Comment 4

[Mathieu Leplatre [:leplatrem]]

> I propose we add the collection last_modified key inside the payload at the top level.

I am not a big fan of duplicating the response header values in the response body.

As I asked on Github, why don't we just include the current ETag when computing the signature?

On the server, it only requires a minor change in the kinto-signer plugin, and in the client, we have it at hand (both in sync hook and http client) before checking the signature.

Would it make sense?

- https://github.com/Kinto/kinto-http.js#listing-records
- https://kintojs.readthedocs.io/en/latest/extending/#hooks

Comment 5

[Mark Goodwin [:mgoodwin]]

(In reply to Mathieu Leplatre (:leplatrem) from comment #4)
> On the server, it only requires a minor change in the kinto-signer plugin,
> and in the client, we have it at hand (both in sync hook and http client)
> before checking the signature.
> 
> Would it make sense?

I'm OK with this approach if we do it quickly. The only disadvantage with this approach is that these signature will be incompatible with clients that expect the old-style signature (not an issue right now - but will become one when we've enabled the feature).

We need to coordinate the change in the client and in the signer. If we're all agreed on this approach, I'll get a patch prepared and reviewed in preparation.

How do you want to want to include the etag? String append or insert into the JS object before stringification?

Comment 6

[Mathieu Leplatre [:leplatrem]]

> I'm OK with this approach if we do it quickly.

No problem. I can prepare a patch for kinto-signer so that you can try it locally. 

> String append or insert into the JS object before stringification?

I would go for insert into the JS object, like: ``{last_modified: "<ETag string value>", data: [...records]}``.

Comment 7

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 1280877 - Prevent collections from being overwritten by outdated signed data

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

Comment 8

[Mathieu Leplatre [:leplatrem]]

https://reviewboard.mozilla.org/r/61278/#review58142

::: services/common/blocklist-clients.js:129
(Diff revision 1)
>                           .createInstance(Ci.nsIContentSignatureVerifier);
>  
>        let records;
> +
> +      let lastModified = payload.lastModified ? payload.lastModified :
> +                                                payload.last_modified;

Instead of this ternary, I would suggest to take advantage of the `ignoreLocal` parameter instead. The lastModified attribute is available when called as an incoming hook (ie. when ignoreLocal is false).

Alternatively, you could build a payload object with a lastModified attribute when calling validateCollectionSignature manually, instead of passing the raw result of fetchRemoteCollection()

::: services/common/blocklist-clients.js:138
(Diff revision 1)
>        } else {
>          records = payload.data;
>        }
> -      const serialized = CanonicalJSON.stringify(records);
> +      let toSerialize = {
> +        last_modified: lastModified + "",
> +        data: records

nitpick: could also be written `${lastModified}`

::: services/common/blocklist-clients.js:205
(Diff revision 1)
>              yield this.validateCollectionSignature(payload, collection, true);
> -            // if the signature is good (we haven't thrown), replace the
> +            // if the signature is good (we haven't thrown), and the remote
> +            // last_modified is newer than the local last_modified, replace the
>              // local data
> +            let lastModified = payload.last_modified;
> +            if (payload.last_modified >= collection.lastModified) {

Looks like `lastModified` variable is not used here.

::: services/common/tests/unit/test_blocklist_signatures.js:443
(Diff revision 1)
>    yield OneCRLBlocklistClient.maybeSync(5000, startTime);
>  
> +  const badSigGoodOldResponses = {
> +    // In this test, we deliberately serve a bad signature initially. The
> +    // subsequent sitnature returned is a valid one for the three item
> +    // collection.

nit typo: signature

::: services/common/tests/unit/test_blocklist_signatures.js:447
(Diff revision 1)
> +    // subsequent sitnature returned is a valid one for the three item
> +    // collection.
> +    "GET:/v1/buckets/blocklists/collections/certificates?":
> +      [RESPONSE_META_BAD_SIG, RESPONSE_META_EMPTY_SIG],
> +    // The first collection state is the the current state (since there's no
> +    // update - but, since the signature is wrong, another request will be made)

nit type: the the

Comment 9

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8766299 [details]
Bug 1280877 - Prevent collections from being overwritten by outdated signed data

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

Comment 10

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8766299 [details]
Bug 1280877 - Prevent collections from being overwritten by outdated signed data

https://reviewboard.mozilla.org/r/61278/#review58148

::: services/common/blocklist-clients.js:139
(Diff revisions 1 - 2)
>          records = payload.data;
>        }
>        let toSerialize = {
> -        last_modified: lastModified + "",
> +        last_modified: `${lastModified}`,
>          data: records
>        };

nit: this block can still be simplified IMO. Something like:

```
  let toSerialize;
  if (ignoreLocal) {
    toSerialize = payload;
  } else {
    const localRecords = (yield collection.list()).data;
    const records = mergeChanges(localRecords, payload.changes);
    toSerialize = {
      last_modified: `${payload.lastModified}`,
      data: records
    };
  }
```

Comment 11

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8766299 [details]
Bug 1280877 - Prevent collections from being overwritten by outdated signed data

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

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8766299 [details]
Bug 1280877 - Prevent collections from being overwritten by outdated signed data

https://reviewboard.mozilla.org/r/61278/#review59168

Sorry for the delay due to PTO

Comment 13

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=85497e544dfa

Comment 14

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=25aeb74f7d0b

Comment 15

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8766299 [details]
Bug 1280877 - Prevent collections from being overwritten by outdated signed data

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

Comment 16

[Mark Goodwin [:mgoodwin]]

(In reply to Mathieu Leplatre (:leplatrem) from comment #10)
> Comment on attachment 8766299 [details]
> nit: this block can still be simplified IMO. Something like:
> 
> ```
>   let toSerialize;
>   if (ignoreLocal) {
>     toSerialize = payload;
>   } else {
>     const localRecords = (yield collection.list()).data;
>     const records = mergeChanges(localRecords, payload.changes);
>     toSerialize = {
>       last_modified: `${payload.lastModified}`,
>       data: records
>     };
>   }
> ```

So it turns out, this won't work with the new http and client lib - since 'next' is tagged onto the payload. I've modified this to work around this issue (based on guidance from Niko) - please confirm you are OK with this change.

Comment 17

[Mathieu Leplatre [:leplatrem]]

Yes I confirm, this is the way to go. Sorry about that surprise :/

Comment 18

[Mark Goodwin [:mgoodwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92442d706b644c71044249e8a1cb4eb4437f1dbc
Bug 1280877 - Prevent collections from being overwritten by outdated signed data r=MattN, r=leplatrem

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92442d706b64
Prevent collections from being overwritten by outdated signed data r=MattN, r=leplatrem

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/92442d706b64