Bug 1639224 - Verify signature if local timestamp is in the future. r=Gijs, a=dveditz
authorMathieu Leplatre <mathieu@mozilla.com>
Fri, 22 May 2020 00:27:06 +0000
changeset 591560 eccdde6fd9b591291694ff68c73ef877dbdb7e0b
parent 591559 ae3ff02c034925ddd5f1856567db6528e1b9f662
child 591561 5a64be5054868a1220f1f4e5b577738ace3aff36
push id13179
push userryanvm@gmail.com
push dateFri, 22 May 2020 21:56:00 +0000
treeherdermozilla-beta@5a64be505486 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs, dveditz
bugs1639224
milestone77.0
Bug 1639224 - Verify signature if local timestamp is in the future. r=Gijs, a=dveditz Differential Revision: https://phabricator.services.mozilla.com/D76249
services/settings/RemoteSettingsClient.jsm
services/settings/test/unit/test_remote_settings.js
--- a/services/settings/RemoteSettingsClient.jsm
+++ b/services/settings/RemoteSettingsClient.jsm
@@ -281,16 +281,18 @@ class RemoteSettingsClient extends Event
 
   httpClient() {
     const api = new KintoHttpClient(Utils.SERVER_URL);
     return api.bucket(this.bucketName).collection(this.collectionName);
   }
 
   /**
    * Retrieve the collection timestamp for the last synchronization.
+   * This is an opaque and comparable value assigned automatically by
+   * the server.
    *
    * @returns {number}
    *          The timestamp in milliseconds, returns -1 if retrieving
    *          the timestamp from the kinto collection fails.
    */
   async getLastModified() {
     let timestamp = -1;
     try {
@@ -472,21 +474,20 @@ class RemoteSettingsClient extends Event
             localRecords = importedFromDump;
           }
           collectionLastModified = await this.db.getLastModified();
         } catch (e) {
           // Report but go-on.
           Cu.reportError(e);
         }
       }
-
       let syncResult;
       try {
         // Is local timestamp up to date with the server?
-        if (expectedTimestamp <= collectionLastModified) {
+        if (expectedTimestamp == collectionLastModified) {
           console.debug(`${this.identifier} local data is up-to-date`);
           reportStatus = UptakeTelemetry.STATUS.UP_TO_DATE;
 
           // If the data is up-to-date but don't have metadata (records loaded from dump),
           // we fetch them and validate the signature immediately.
           if (this.verifySignature && ObjectUtils.isEmpty(localMetadata)) {
             console.debug(`${this.identifier} pull collection metadata`);
             const metadata = await this.httpClient().getData({
@@ -514,18 +515,19 @@ class RemoteSettingsClient extends Event
           // Otherwise we want to continue with sending the sync event to notify about the created records.
           syncResult = {
             current: importedFromDump,
             created: importedFromDump,
             updated: [],
             deleted: [],
           };
         } else {
-          // Local data is outdated.
-          // Fetch changes from server, and make sure we overwrite local data.
+          // Local data is either outdated or tampered.
+          // In both cases we will fetch changes from server,
+          // and make sure we overwrite local data.
           const startSyncDB = Cu.now() * 1000;
           syncResult = await this._importChanges(
             localRecords,
             collectionLastModified,
             localMetadata,
             expectedTimestamp
           );
           if (gTimingEnabled) {
--- a/services/settings/test/unit/test_remote_settings.js
+++ b/services/settings/test/unit/test_remote_settings.js
@@ -173,23 +173,28 @@ add_task(async function test_throws_when
 });
 add_task(clear_state);
 
 add_task(async function test_sync_event_is_sent_even_if_up_to_date() {
   if (IS_ANDROID) {
     // Skip test: we don't ship remote settings dumps on Android (see package-manifest).
     return;
   }
+  // First, determine what is the dump timestamp. Sync will load it.
+  // Use a timestamp inferior to latest record in dump.
+  await clientWithDump._importJSONDump();
+  const uptodateTimestamp = await clientWithDump.db.getLastModified();
+  await clear_state();
+
+  // Now, simulate that server data wasn't changed since dump was released.
   const startHistogram = getUptakeTelemetrySnapshot(clientWithDump.identifier);
   let received;
   clientWithDump.on("sync", ({ data }) => (received = data));
-  // Use a timestamp inferior to latest record in dump.
-  const timestamp = 1000000000000; // Sun Sep 09 2001
 
-  await clientWithDump.maybeSync(timestamp);
+  await clientWithDump.maybeSync(uptodateTimestamp);
 
   ok(received.current.length > 0, "Dump records are listed as created");
   equal(received.current.length, received.created.length);
 
   const endHistogram = getUptakeTelemetrySnapshot(clientWithDump.identifier);
   const expectedIncrements = { [UptakeTelemetry.STATUS.UP_TO_DATE]: 1 };
   checkUptakeTelemetry(startHistogram, endHistogram, expectedIncrements);
 });
@@ -407,20 +412,21 @@ add_task(
         return true;
       },
     };
     // When dump is loaded, signature is not verified.
     const records = await clientWithDump.get({ verifySignature: true });
     ok(records.length > 0, "dump is loaded");
     ok(!called, "signature is missing but not verified");
 
-    // Synchronize the collection (local data is up-to-date, collection last modified > 42)
+    // Synchronize the collection (local data is up-to-date).
     // Signature verification is disabled (see `clear_state()`), so we don't bother with
     // fetching metadata.
-    await clientWithDump.maybeSync(42);
+    const uptodateTimestamp = await clientWithDump.db.getLastModified();
+    await clientWithDump.maybeSync(uptodateTimestamp);
     let metadata = await clientWithDump.db.getMetadata();
     ok(!metadata, "metadata was not fetched");
 
     // Synchronize again the collection (up-to-date, since collection last modified still > 42)
     clientWithDump.verifySignature = true;
     await clientWithDump.maybeSync(42);
 
     // With signature verification, metadata was fetched.