Closed Bug 1280877 Opened 4 years ago Closed 4 years ago

Blocklists can be replaced by an outdated version.

Categories

(Cloud Services :: Firefox: Common, defect)

defect
Not set
normal

Tracking

(firefox50 fixed)

RESOLVED FIXED
Tracking Status
firefox50 --- fixed

People

(Reporter: mgoodwin, Assigned: mgoodwin)

References

Details

Attachments

(1 file)

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.
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?
Flags: needinfo?(tarek)
See Also: → 1280905
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 ?
Flags: needinfo?(tarek) → needinfo?(mathieu)
> 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
(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?
> 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]}``.
Flags: needinfo?(mathieu)
Depends on: 1282109
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 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/
Attachment #8766299 - Flags: review?(mathieu) → review+
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 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 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
Attachment #8766299 - Flags: review?(MattN+bmo) → review+
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/
(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.
Flags: needinfo?(mathieu)
Yes I confirm, this is the way to go. Sorry about that surprise :/
Flags: needinfo?(mathieu)
https://hg.mozilla.org/integration/mozilla-inbound/rev/92442d706b644c71044249e8a1cb4eb4437f1dbc
Bug 1280877 - Prevent collections from being overwritten by outdated signed data r=MattN, r=leplatrem
Pushed by mgoodwin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92442d706b64
Prevent collections from being overwritten by outdated signed data r=MattN, r=leplatrem
https://hg.mozilla.org/mozilla-central/rev/92442d706b64
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.