Bug 1280877 - Prevent collections from being overwritten by outdated signed data r=MattN, r=leplatrem draft
authorMark Goodwin <mgoodwin@mozilla.com>
Mon, 18 Jul 2016 19:32:56 +0100
changeset 389222 9d86aa7156f649465c776cc860e044a2d5ba075d
parent 388900 0fbdcd21fad76a00328e67875c6f40dc219235f4
child 525675 7ed34bd06732eb23d151acfc7e9f3a6f0acf562b
push id23319
push usermgoodwin@mozilla.com
push dateMon, 18 Jul 2016 18:42:57 +0000
reviewersMattN, leplatrem
bugs1280877
milestone50.0a1
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