Closed Bug 1553831 Opened 5 years ago Closed 5 years ago

Fetch missing metadata on sync even if local data is up-to-date

Categories

(Firefox :: Remote Settings Client, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- fixed

People

(Reporter: standard8, Assigned: leplatrem)

References

Details

(Keywords: sec-want, Whiteboard: [adv-main69-])

Attachments

(2 files)

STR

  1. Have a local Firefox profile that's up to date, and the server settings data is the same as the dump stored in the client.
  2. Run the code:
const settings = RemoteSettings(`mykey`);
records = await settings.get({verifySignature: true});

Actual Result

=> Error: Invalid content signature (mykey)

Expected Result

=> No errors, records returned correctly.

Since the local dump is the same as the server settings, we haven't stored the metadata from the server. Therefore the signature cannot be validated and this throws.

We still want to be able to avoid the server fetch round trip - and just use the local dump - so that we can avoid issues like server not available or first profile startup causing long round trips.

If I read this code correctly...
https://searchfox.org/mozilla-central/rev/952521e6164ddffa3f34bc8cfa5a81afc5b859c4/services/settings/RemoteSettingsClient.jsm#221
...the first call .get() should not verify signature if a dump is loaded.

The following .get() will, indeed, since the local DB will have records but no sync was triggered (and no metadata was pulled).

We still want to be able to avoid the server fetch round trip - and just use the local dump - so that we can avoid issues like server not available or first profile startup causing long round trips.

On the second call to .get() I find it difficult to avoid fetching metadata if they're missing. Would that work for you?

Presumably RemoteSettings will normally poll sometime soon after first startup for a new profile?

If so, then if it is also storing the metadata on that poll, that is probably reasonable. At the moment, we never seem to store the metadata if the local dump is the same as the server.

Though, will the signature expire if the server isn't polled for a couple of days (e.g. Firefox isn't run over the weekend)?

Though, will the signature expire if the server isn't polled for a couple of days (e.g. Firefox isn't run over the weekend)?

I assume a signature never expires as long as the root hash is not changed. And I assume that we'll host all the certificates chains we've ever used for a long time. :ulfr you confirm?

Presumably RemoteSettings will normally poll sometime soon after first startup for a new profile?

Yes, but there is no timing guarantee. Depending on your code, if you call .get() a second time before it syncs, you'll have this InvalidaSignature (I'll add unit tests).

If so, then if it is also storing the metadata on that poll, that is probably reasonable. At the moment, we never seem to store the metadata if the local dump is the same as the server.

Exactly, the records dumps don't contain the collection signature (should they?).

I can handle the situation in .get() and pull the metadata at the moment (implicit network call):

diff --git a/services/settings/RemoteSettingsClient.jsm b/services/settings/RemoteSettingsClient.jsm
--- a/services/settings/RemoteSettingsClient.jsm
+++ b/services/settings/RemoteSettingsClient.jsm
@@ -229,17 +229,22 @@ class RemoteSettingsClient extends Event
     // Read from the local DB.
     const kintoCollection = await this.openCollection();
     const { data } = await kintoCollection.list({ filters, order });
 
     // Verify signature of local data.
     if (verifySignature) {
       const localRecords = data.map(r => kintoCollection.cleanLocalFields(r));
       const timestamp = await kintoCollection.db.getLastModified();
-      const metadata = await kintoCollection.metadata();
+      let metadata = await kintoCollection.metadata();
+      if (!metadata) {
+        // No sync occured yet, may have records from dump but no metadata.
+        console.debug(`Required metadata for ${this.identifier}, fetching from server.`);
+        metadata = await kintoCollection.pullMetadata(client);
+      }
       await this._validateCollectionSignature([],
                                               timestamp,
                                               metadata,
                                               { localRecords });
     }
 
     // Filter the records based on `this.filterFunc` results.
     return this._filterEntries(data);

But you may prefer handle the case yourself explicitly, since that's probably what you'll have to do if content does not match signature anyway.

while(retry) {
  try {
    // Read local, if empty and dump present then load else sync.
    return await ignoreListSettings.get({verifySignature: true});
  } catch (e) {
    const kintoCollection = await ignoreListSettings.openCollection();
    await kintoCollection.clear();
  }
}

Let me know! You may want to compare performance (re-loading dump vs. fetch metadata + sig verification). For small dumps it should be faster to reload IDK

Flags: needinfo?(jvehent)

(In reply to Mathieu Leplatre [:leplatrem] from comment #4)

Though, will the signature expire if the server isn't polled for a couple of days (e.g. Firefox isn't run over the weekend)?

I assume a signature never expires as long as the root hash is not changed. And I assume that we'll host all the certificates chains we've ever used for a long time. :ulfr you confirm?

Signatures are valid until the certificates associated with them expire, and operationally we make sure that those certificates will be valid for at least 30 days from the time of signing (primarily to account for users with bad clocks).

If a user does not open Firefox for more than 30 days, you can expect signatures to be invalid.

Flags: needinfo?(jvehent)

If a user does not open Firefox for more than 30 days, you can expect signatures to be invalid.

Oh, thanks Julien! Good to know :)

:Standard8, I guess it's acceptable to fall in the case of invalid/flush/reload for this situation, no?

Priority: -- → P2

So I think on the Search side, we can catch the invalid signature, clear the database and then get it again (which will probably get it from the dump, but that's ok).

That's probably the worst case. Generally, we'll only need to query once per session - any updates will be via the notifications.

In theory, most profiles will then be able to Sync later on, and get the metadata, and so next time they start, we'll still be good.

Hence, the only bit that I think this bug now needs to address, is saving the metadata from the Sync even if the records haven't changed from what's in the local dump.

Assignee: nobody → mathieu
Summary: When RemoteSettings(...).get() is used with verifySignature, it throws a signature error if loaded from the local dump → Fetch missing metadata on sync even if local data is up-to-date

Landed:
https://hg.mozilla.org/integration/autoland/rev/4f62dfaee27f3d0620a44d092a85ee652bf0a0ca
https://hg.mozilla.org/integration/autoland/rev/d03a5195626223590399f35603fdd0161cf18fca

Backed out for XPCshell failure in services/settings/test/unit/test_remote_settings.js:
https://hg.mozilla.org/integration/autoland/rev/f5a8d8e97356dcbaa021a4d82f06239af1edd538

Push failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&group_state=expanded&selectedJob=249340237&revision=d03a5195626223590399f35603fdd0161cf18fca
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=249361807&repo=autoland&lineNumber=1811
[task 2019-05-31T11:40:21.969Z] 11:40:21 INFO - TEST-PASS | services/settings/test/unit/test_remote_settings.js | test_get_does_not_verify_signature_if_load_dump - [test_get_does_not_verify_signature_if_load_dump : 233] signature is missing but not verified - true == true
[task 2019-05-31T11:40:21.969Z] 11:40:21 INFO - services/settings/test/unit/test_remote_settings.js | console.debug: services.settings:
[task 2019-05-31T11:40:21.969Z] 11:40:21 INFO - services/settings/test/unit/test_remote_settings.js | Verify signature of local data
[task 2019-05-31T11:40:21.969Z] 11:40:21 INFO - services/settings/test/unit/test_remote_settings.js | [10482, Main Thread] WARNING: GetLinkType is not supported without a bridge connection: file /builds/worker/workspace/build/src/netwerk/system/android/nsAndroidNetworkLinkService.cpp, line 47
[task 2019-05-31T11:40:21.970Z] 11:40:21 INFO - [Exception... "Unexpected error" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: resource://testing-common/httpd.js :: finish :: line 3455" data: no]
[task 2019-05-31T11:40:21.970Z] 11:40:21 WARNING - TEST-UNEXPECTED-FAIL | services/settings/test/unit/test_remote_settings.js | test_get_does_not_verify_signature_if_load_dump - [test_get_does_not_verify_signature_if_load_dump : 242] signer was not called - false == true
[task 2019-05-31T11:40:21.970Z] 11:40:21 INFO - test_remote_settings.js:test_get_does_not_verify_signature_if_load_dump:242
[task 2019-05-31T11:40:21.970Z] 11:40:21 INFO - /sdcard/tests/xpc/head.js:_do_main:227
[task 2019-05-31T11:40:21.970Z] 11:40:21 INFO - /sdcard/tests/xpc/head.js:_execute_test:529
[task 2019-05-31T11:40:21.970Z] 11:40:21 INFO - -e:null:1
[task 2019-05-31T11:40:21.970Z] 11:40:21 INFO - exiting test

Flags: needinfo?(mathieu)
Flags: needinfo?(mathieu)

Is this something we want to backport to Beta for the next ESR?

Flags: needinfo?(mathieu)

Is this something we want to backport to Beta for the next ESR?

No thanks.

It's a new feature to support Bug 1556431, I believe it was accidentally marked as «secure»

Flags: needinfo?(mathieu)
Group: core-security-release
Whiteboard: [adv-main69-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: