Bug 1280877 - Prevent collections from being overwritten by outdated signed data r=MattN, r=leplatrem
authorMark Goodwin <mgoodwin@mozilla.com>
Mon, 18 Jul 2016 19:32:56 +0100
changeset 305703 92442d706b644c71044249e8a1cb4eb4437f1dbc
parent 305702 80b9e593a3aeddb2fade68ad88619b9b31d599d2
child 305704 8df802dec5fbf1e04401e87a15acd2a04bff7680
push id30467
push usercbook@mozilla.com
push dateWed, 20 Jul 2016 09:21:53 +0000
treeherdermozilla-central@e904e18d7dfc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN, leplatrem
bugs1280877
milestone50.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1280877 - Prevent collections from being overwritten by outdated signed data r=MattN, r=leplatrem MozReview-Commit-ID: JI7DEVEW7P6
services/common/blocklist-clients.js
services/common/tests/unit/test_blocklist_signatures.js
--- a/services/common/blocklist-clients.js
+++ b/services/common/blocklist-clients.js
@@ -118,24 +118,32 @@ class BlocklistClient {
     return Task.spawn((function* () {
       // this is a content-signature field from an autograph response.
       const {x5u, signature} = yield fetchCollectionMetadata(collection);
       const certChain = yield fetch(x5u).then((res) => res.text());
 
       const verifier = Cc["@mozilla.org/security/contentsignatureverifier;1"]
                          .createInstance(Ci.nsIContentSignatureVerifier);
 
-      let records;
-      if (!ignoreLocal) {
+      let toSerialize;
+      if (ignoreLocal) {
+        toSerialize = {
+          last_modified: `${payload.last_modified}`,
+          data: payload.data
+        };
+      } else {
         const localRecords = (yield collection.list()).data;
-        records = mergeChanges(localRecords, payload.changes);
-      } else {
-        records = payload.data;
+        const records = mergeChanges(localRecords, payload.changes);
+        toSerialize = {
+          last_modified: `${payload.lastModified}`,
+          data: records
+        };
       }
-      const serialized = CanonicalJSON.stringify(records);
+
+      const serialized = CanonicalJSON.stringify(toSerialize);
 
       if (verifier.verifyContentSignature(serialized, "p384ecdsa=" + signature,
                                           certChain,
                                           this.signerName)) {
         // In case the hash is valid, apply the changes locally.
         return payload;
       }
       throw new Error(INVALID_SIGNATURE);
@@ -186,20 +194,23 @@ class BlocklistClient {
         } catch (e) {
           if (e.message == INVALID_SIGNATURE) {
             // if sync fails with a signature error, it's likely that our
             // local data has been modified in some way.
             // We will attempt to fix this by retrieving the whole
             // remote collection.
             let payload = yield fetchRemoteCollection(collection);
             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
-            yield collection.clear();
-            yield collection.loadDump(payload.data);
+            if (payload.last_modified >= collection.lastModified) {
+              yield collection.clear();
+              yield collection.loadDump(payload.data);
+            }
           } else {
             throw e;
           }
         }
         // Read local collection of records.
         let list = yield collection.list();
 
         yield this.processCallback(list.data);
--- a/services/common/tests/unit/test_blocklist_signatures.js
+++ b/services/common/tests/unit/test_blocklist_signatures.js
@@ -48,16 +48,46 @@ function setRoot() {
 function getCertChain() {
   const chain = [];
   for (let file of CHAIN_FILES) {
     chain.push(getFileData(do_get_file(CERT_DIR + file)));
   }
   return chain.join("\n");
 }
 
+function* checkRecordCount(count) {
+  // open the collection manually
+  const base = Services.prefs.getCharPref(PREF_SETTINGS_SERVER);
+  const bucket = Services.prefs.getCharPref(PREF_BLOCKLIST_BUCKET);
+  const collectionName =
+      Services.prefs.getCharPref(PREF_BLOCKLIST_ONECRL_COLLECTION);
+
+  const Kinto = loadKinto();
+
+  const FirefoxAdapter = Kinto.adapters.FirefoxAdapter;
+
+  const config = {
+    remote: base,
+    bucket: bucket,
+    adapter: FirefoxAdapter,
+  };
+
+  const db = new Kinto(config);
+  const collection = db.collection(collectionName);
+
+  yield collection.db.open();
+
+  // Check we have the expected number of records
+  let records = yield collection.list();
+  do_check_eq(count, records.data.length);
+
+  // Close the collection so the test can exit cleanly
+  yield collection.db.close();
+}
+
 // Check to ensure maybeSync is called with correct values when a changes
 // document contains information on when a collection was last modified
 add_task(function* test_check_signatures(){
   const port = server.identity.primaryPort;
 
   // a response to give the client when the cert chain is expected
   function makeMetaResponseBody(lastModified, signature) {
     return {
@@ -239,17 +269,17 @@ add_task(function* test_check_signatures
       "Content-Type: application/json; charset=UTF-8",
       "ETag: \"1000\""
     ],
     status: {status: 200, statusText: "OK"},
     responseBody: JSON.stringify({"data": []})
   };
 
   const RESPONSE_BODY_META_EMPTY_SIG = makeMetaResponseBody(1000,
-    "lJj7PfrLvvLcDBPBWQrV10rY5s1OlUAITx9UT-K_wzxmgEgS7vy8LzJQh5-rdpXHfZW5lKM5itpYwyscV9LkJSuVaozITP81_5zg8Pw6OifmqHcvBE81AtRv0r_eBVd0");
+    "vxuAg5rDCB-1pul4a91vqSBQRXJG_j7WOYUTswxRSMltdYmbhLRH8R8brQ9YKuNDF56F-w6pn4HWxb076qgKPwgcEBtUeZAO_RtaHXRkRUUgVzAr86yQL4-aJTbv3D6u");
 
   // The collection metadata containing the signature for the empty
   // collection.
   const RESPONSE_META_EMPTY_SIG =
     makeMetaResponse(1000, RESPONSE_BODY_META_EMPTY_SIG,
                      "RESPONSE_META_EMPTY_SIG");
 
   // Here, we map request method and path to the available responses
@@ -279,17 +309,17 @@ add_task(function* test_check_signatures
         "Content-Type: application/json; charset=UTF-8",
         "ETag: \"3000\""
     ],
     status: {status: 200, statusText: "OK"},
     responseBody: JSON.stringify({"data": [RECORD2, RECORD1]})
   };
 
   const RESPONSE_BODY_META_TWO_ITEMS_SIG = makeMetaResponseBody(3000,
-    "f4pA2tYM5jQgWY6YUmhUwQiBLj6QO5sHLD_5MqLePz95qv-7cNCuQoZnPQwxoptDtW8hcWH3kLb0quR7SB-r82gkpR9POVofsnWJRA-ETb0BcIz6VvI3pDT49ZLlNg3p");
+    "dwhJeypadNIyzGj3QdI0KMRTPnHhFPF_j73mNrsPAHKMW46S2Ftf4BzsPMvPMB8h0TjDus13wo_R4l432DHe7tYyMIWXY0PBeMcoe5BREhFIxMxTsh9eGVXBD1e3UwRy");
 
   // A signature response for the collection containg RECORD1 and RECORD2
   const RESPONSE_META_TWO_ITEMS_SIG =
     makeMetaResponse(3000, RESPONSE_BODY_META_TWO_ITEMS_SIG,
                      "RESPONSE_META_TWO_ITEMS_SIG");
 
   const twoItemsResponses = {
     "GET:/v1/buckets/blocklists/collections/certificates/records?_sort=-last_modified&_since=1000":
@@ -310,17 +340,17 @@ add_task(function* test_check_signatures
       "Content-Type: application/json; charset=UTF-8",
       "ETag: \"4000\""
     ],
     status: {status: 200, statusText: "OK"},
     responseBody: JSON.stringify({"data": [RECORD3, RECORD1_DELETION]})
   };
 
   const RESPONSE_BODY_META_THREE_ITEMS_SIG = makeMetaResponseBody(4000,
-    "wxVc0AvHZZ0fyZR8tZVtZRBrsVNYIBxOjaKZXgnjyJqfwnyENSZkJLQlm3mho-J_QAxDTp7QPXXVSA-r1SrE3rlqV4BkqE9NTGREKvl5BJzaDEOtxH7VF5WMw49k8q0O");
+    "MIEmNghKnkz12UodAAIc3q_Y4a3IJJ7GhHF4JYNYmm8avAGyPM9fYU7NzVo94pzjotG7vmtiYuHyIX2rTHTbT587w0LdRWxipgFd_PC1mHiwUyjFYNqBBG-kifYk7kEw");
 
   // signature response for the collection containing RECORD2 and RECORD3
   const RESPONSE_META_THREE_ITEMS_SIG =
     makeMetaResponse(4000, RESPONSE_BODY_META_THREE_ITEMS_SIG,
                      "RESPONSE_META_THREE_ITEMS_SIG");
 
   const oneAddedOneRemovedResponses = {
     "GET:/v1/buckets/blocklists/collections/certificates/records?_sort=-last_modified&_since=3000":
@@ -380,17 +410,17 @@ add_task(function* test_check_signatures
   const RESPONSE_BODY_META_BAD_SIG = makeMetaResponseBody(4000,
       "aW52YWxpZCBzaWduYXR1cmUK");
 
   const RESPONSE_META_BAD_SIG =
       makeMetaResponse(4000, RESPONSE_BODY_META_BAD_SIG, "RESPONSE_META_BAD_SIG");
 
   const badSigGoodSigResponses = {
     // In this test, we deliberately serve a bad signature initially. The
-    // subsequent sitnature returned is a valid one for the three item
+    // subsequent signature returned is a valid one for the three item
     // collection.
     "GET:/v1/buckets/blocklists/collections/certificates?":
       [RESPONSE_META_BAD_SIG, RESPONSE_META_THREE_ITEMS_SIG],
     // The first collection state is the three item collection (since
     // there's a sync with no updates) - but, since the signature is wrong,
     // another request will be made...
     "GET:/v1/buckets/blocklists/collections/certificates/records?_sort=-last_modified&_since=4000":
       [RESPONSE_EMPTY_NO_UPDATE],
@@ -402,16 +432,40 @@ add_task(function* test_check_signatures
     // checked against the valid signature - so the sync should succeed.
     "GET:/v1/buckets/blocklists/collections/certificates/records?_sort=id":
       [RESPONSE_COMPLETE_INITIAL_SORTED_BY_ID]
   };
 
   registerHandlers(badSigGoodSigResponses);
   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.
+    "GET:/v1/buckets/blocklists/collections/certificates?":
+      [RESPONSE_META_BAD_SIG, RESPONSE_META_EMPTY_SIG],
+    // The first collection state is the current state (since there's no update
+    // - but, since the signature is wrong, another request will be made)
+    "GET:/v1/buckets/blocklists/collections/certificates/records?_sort=-last_modified&_since=4000":
+      [RESPONSE_EMPTY_NO_UPDATE],
+    // The next request is for the full collection sorted by id. This will be
+    // checked against the valid signature and last_modified times will be
+    // compared. Sync should fail, even though the signature is good,
+    // because the local collection is newer.
+    "GET:/v1/buckets/blocklists/collections/certificates/records?_sort=id":
+      [RESPONSE_EMPTY_INITIAL],
+  };
+
+  // ensure our collection hasn't been replaced with an older, empty one
+  yield checkRecordCount(2);
+
+  registerHandlers(badSigGoodOldResponses);
+  yield OneCRLBlocklistClient.maybeSync(5000, startTime);
+
   const allBadSigResponses = {
     // In this test, we deliberately serve only a bad signature.
     "GET:/v1/buckets/blocklists/collections/certificates?":
       [RESPONSE_META_BAD_SIG],
     // The first collection state is the three item collection (since
     // there's a sync with no updates) - but, since the signature is wrong,
     // another request will be made...
     "GET:/v1/buckets/blocklists/collections/certificates/records?_sort=-last_modified&_since=4000":
@@ -422,43 +476,17 @@ add_task(function* test_check_signatures
       [RESPONSE_COMPLETE_INITIAL_SORTED_BY_ID]
   };
 
   registerHandlers(allBadSigResponses);
   try {
     yield OneCRLBlocklistClient.maybeSync(6000, startTime);
     do_throw("Sync should fail (the signature is intentionally bad)");
   } catch (e) {
-    // open the collection manually
-    const base = Services.prefs.getCharPref(PREF_SETTINGS_SERVER);
-    const bucket = Services.prefs.getCharPref(PREF_BLOCKLIST_BUCKET);
-    const collectionName =
-      Services.prefs.getCharPref(PREF_BLOCKLIST_ONECRL_COLLECTION);
-
-    const Kinto = loadKinto();
-
-    const FirefoxAdapter = Kinto.adapters.FirefoxAdapter;
-
-    const config = {
-      remote: base,
-      bucket: bucket,
-      adapter: FirefoxAdapter,
-    };
-
-    const db = new Kinto(config);
-    const collection = db.collection(collectionName);
-
-    yield collection.db.open();
-
-    // Check we have the expected number of records
-    let records = yield collection.list();
-    do_check_eq(2, records.data.length);
-
-    // Close the collection so the test can exit cleanly
-    yield collection.db.close()
+    yield checkRecordCount(2);
   }
 });
 
 function run_test() {
   // ensure signatures are enforced
   Services.prefs.setBoolPref(PREF_BLOCKLIST_ENFORCE_SIGNING, true);
 
   // get a signature verifier to ensure nsNSSComponent is initialized