Bug 1333810: Encrypt record deletes, r?kmag, rnewman draft
authorEthan Glasser-Camp <eglassercamp@mozilla.com>
Fri, 27 Jan 2017 19:27:10 -0500
changeset 467550 9c76cde475ff5704a8eb4d71e6b1b5ba0a2d18b1
parent 467549 58b6f957d060d32c0867af9e31e9aeb300f4e3e1
child 468307 cbd0712f18411a173aca4f6ab7a8fa8263fef0a6
child 469339 39606f8551698756cbf7c7b54eb43d423c831cf6
push id43215
push usereglassercamp@mozilla.com
push dateSat, 28 Jan 2017 00:31:54 +0000
reviewerskmag, rnewman
bugs1333810
milestone54.0a1
Bug 1333810: Encrypt record deletes, r?kmag, rnewman Camouflage deletes as updates, according to the rules given by the new RemoteTransformer contract. MozReview-Commit-ID: CwVJSs2jOil
services/sync/modules/engines/extension-storage.js
toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js
--- a/services/sync/modules/engines/extension-storage.js
+++ b/services/sync/modules/engines/extension-storage.js
@@ -134,19 +134,26 @@ class EncryptionRemoteTransformer {
         throw new Error("Record ID is missing or invalid");
       }
 
       let IV = Svc.Crypto.generateRandomIV();
       let ciphertext = Svc.Crypto.encrypt(JSON.stringify(record),
                                           keyBundle.encryptionKeyB64, IV);
       let hmac = ciphertextHMAC(keyBundle, id, IV, ciphertext);
       const encryptedResult = {ciphertext, IV, hmac, id};
+
+      // Copy over the _status field, so that we handle concurrency
+      // headers (If-Match, If-None-Match) correctly.
+      // DON'T copy over "deleted" status, because then we'd leak
+      // plaintext deletes.
+      encryptedResult._status = record._status == "deleted" ? "updated" : record._status;
       if (record.hasOwnProperty("last_modified")) {
         encryptedResult.last_modified = record.last_modified;
       }
+
       return encryptedResult;
     });
   }
 
   decode(record) {
     const self = this;
     return Task.spawn(function* () {
       if (!record.ciphertext) {
@@ -178,16 +185,23 @@ class EncryptionRemoteTransformer {
       if (jsonResult.id != record.id) {
         throw new Error("Record id mismatch: " + jsonResult.id + " != " + record.id);
       }
 
       if (record.hasOwnProperty("last_modified")) {
         jsonResult.last_modified = record.last_modified;
       }
 
+      // _status: deleted records were deleted on a client, but
+      // uploaded as an encrypted blob so we don't leak deletions.
+      // If we get such a record, flag it as deleted.
+      if (jsonResult._status == "deleted") {
+        jsonResult.deleted = true;
+      }
+
       return jsonResult;
     });
   }
 
   /**
    * Retrieve keys to use during encryption.
    *
    * Returns a Promise<KeyBundle>.
--- a/toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js
@@ -990,19 +990,20 @@ add_task(function* test_storage_sync_pul
       yield ExtensionStorageSync.syncAll();
       server.clearPosts();
 
       let calls = [];
       yield ExtensionStorageSync.addOnChangedListener(extension, function() {
         calls.push(arguments);
       }, context);
 
-      yield server.addRecord(collectionId, {
+      yield server.encryptAndAddRecord(new CollectionKeyEncryptionRemoteTransformer(extension.id), collectionId, {
         "id": "key-my_2D_key",
-        "deleted": true,
+        "data": 6,
+        "_status": "deleted",
       });
 
       yield ExtensionStorageSync.syncAll();
       const remoteValues = (yield ExtensionStorageSync.get(extension, "my-key", context));
       ok(!remoteValues["my-key"],
          "ExtensionStorageSync.get() shows value was deleted by sync");
 
       equal(server.getPosts().length, 0,
@@ -1056,14 +1057,19 @@ add_task(function* test_storage_sync_pus
       // Doesn't push keys because keys were pushed by a previous test.
       posts = server.getPosts();
       equal(posts.length, 3,
             "deleting a value should trigger another push");
       const post = posts[2];
       assertPostedUpdatedRecord(post, 1000);
       equal(post.path, collectionRecordsPath(collectionId) + "/key-my_2D_key",
             "pushing a deleted value should go to the same path");
-      ok(post.method, "DELETE");
-      ok(!post.body,
-         "deleting a value shouldn't have a body");
+      ok(post.method, "PUT");
+      ok(post.body.data.ciphertext,
+         "deleting a value should have an encrypted body");
+      const decoded = yield new CollectionKeyEncryptionRemoteTransformer(extensionId).decode(post.body.data);
+      equal(decoded._status, "deleted");
+      // Ideally, we'd check that decoded.deleted is not true, because
+      // the encrypted record shouldn't have it, but the decoder will
+      // add it when it sees _status == deleted
     });
   });
 });