Bug 1253740 - Define checkSyncKeyRing() which reuploads keys when passwords change, r=markh
☠☠ backed out by 796ad3765645 ☠ ☠
authorEthan Glasser-Camp <eglassercamp@mozilla.com>
Mon, 03 Oct 2016 19:19:13 -0400
changeset 320164 486a200fd6f086bdb2f32fa044f87a09144d0be2
parent 320163 97a6ee1fddfced804aa7e516d3fc5ff4feb30a00
child 320165 5e234fe1099c7fa65103b25eb0ed4c710f872f09
push id20751
push userphilringnalda@gmail.com
push dateSun, 30 Oct 2016 18:06:35 +0000
treeherderfx-team@e3279760cd97 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh
bugs1253740
milestone52.0a1
Bug 1253740 - Define checkSyncKeyRing() which reuploads keys when passwords change, r=markh MozReview-Commit-ID: B5Ptj4MGAC
toolkit/components/extensions/ExtensionStorageSync.jsm
toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js
--- a/toolkit/components/extensions/ExtensionStorageSync.jsm
+++ b/toolkit/components/extensions/ExtensionStorageSync.jsm
@@ -214,16 +214,20 @@ const cryptoCollection = this.cryptoColl
     if (this.refCount == 0) {
       const oldPromise = this._kintoCollectionPromise;
       this._kintoCollectionPromise = null;
       const collection = yield oldPromise;
       yield collection.db.close();
     }
   }),
 
+  isActive() {
+    return this.refCount != 0;
+  },
+
   /**
    * Retrieve the keyring record from the crypto collection.
    *
    * You can use this if you want to check metadata on the keyring
    * record rather than use the keyring itself.
    *
    * @returns {Promise<Object>}
    */
@@ -246,16 +250,23 @@ const cryptoCollection = this.cryptoColl
     } else {
       // We never actually use the default key, so it's OK if we
       // generate one multiple times.
       collectionKeys.generateDefaultKey();
     }
     return collectionKeys;
   }),
 
+  updateKBHash: Task.async(function* (kbHash) {
+    const coll = yield this._kintoCollectionPromise;
+    yield coll.update({id: STORAGE_SYNC_CRYPTO_KEYRING_RECORD_ID,
+                       kbHash: kbHash},
+                      {patch: true});
+  }),
+
   upsert: Task.async(function* (record) {
     const collection = yield this._kintoCollectionPromise;
     yield collection.upsert(record);
   }),
 
   sync: Task.async(function* () {
     if (!this._kintoCollectionPromise) {
       throw new Error("tried to sync without any live uses of the Kinto collection!");
@@ -406,16 +417,17 @@ this.ExtensionStorageSync = {
       const extIds = Array.from(extensions, extension => extension.id);
       log.debug(`Syncing extension settings for ${JSON.stringify(extIds)}\n`);
       if (extIds.length == 0) {
         // No extensions to sync. Crypto probably isn't even
         // initialized. Get out.
         return;
       }
       yield this.ensureKeysFor(extIds);
+      yield this.checkSyncKeyRing();
       const promises = Array.from(collectionPromises.entries(), ([extension, collPromise]) => {
         return collPromise.then(coll => {
           return this.sync(extension, coll);
         });
       });
       yield Promise.all(promises);
     } finally {
       yield cryptoCollection.decrementUses();
@@ -533,35 +545,129 @@ this.ExtensionStorageSync = {
    * @returns {Promise<CollectionKeyManager>}
    */
   ensureKeysFor: Task.async(function* (extIds) {
     const collectionKeys = yield cryptoCollection.getKeyRing();
     if (collectionKeys.hasKeysFor(extIds)) {
       return collectionKeys;
     }
 
+    const kbHash = yield this.getKBHash();
     const newKeys = yield collectionKeys.ensureKeysFor(extIds);
     const newRecord = {
       id: STORAGE_SYNC_CRYPTO_KEYRING_RECORD_ID,
       keys: newKeys.asWBO().cleartext,
+      // Add a field for the current kB hash.
+      kbHash: kbHash,
     };
     yield cryptoCollection.upsert(newRecord);
     const result = yield cryptoCollection.sync();
     if (result.resolved.length != 0) {
       // We had a conflict which was automatically resolved. We now
       // have a new keyring which might have keys for the
       // collections. Recurse.
       return yield this.ensureKeysFor(extIds);
     }
 
     // No conflicts. We're good.
     return newKeys;
   }),
 
   /**
+   * Get the current user's hashed kB.
+   *
+   * @returns sha256 of the user's kB as a hex string
+   */
+  getKBHash: Task.async(function* () {
+    const signedInUser = yield this._fxaService.getSignedInUser();
+    if (!signedInUser) {
+      throw new Error("User isn't signed in!");
+    }
+
+    if (!signedInUser.kB) {
+      throw new Error("User doesn't have kB??");
+    }
+
+    let kBbytes = CommonUtils.hexToBytes(signedInUser.kB);
+    let hasher = Cc["@mozilla.org/security/hash;1"]
+                    .createInstance(Ci.nsICryptoHash);
+    hasher.init(hasher.SHA256);
+    return CommonUtils.bytesAsHex(CryptoUtils.digestBytes(signedInUser.uid + kBbytes, hasher));
+  }),
+
+  /**
+   * Update the kB in the crypto record.
+   */
+  updateKeyRingKB: Task.async(function* () {
+    const signedInUser = yield this._fxaService.getSignedInUser();
+    if (!signedInUser) {
+      // Although this function is meant to be called on login,
+      // it's not unreasonable to check any time, even if we aren't
+      // logged in.
+      //
+      // If we aren't logged in, we don't have any information about
+      // the user's kB, so we can't be sure that the user changed
+      // their kB, so just return.
+      return;
+    }
+
+    const thisKBHash = yield this.getKBHash();
+    yield cryptoCollection.updateKBHash(thisKBHash);
+  }),
+
+  /**
+   * Make sure the keyring is up to date and synced.
+   *
+   * This is called on log-in events to maintain the keyring in the
+   * correct state on the server. It's also called on syncs to make
+   * sure that we don't sync anything to any collection unless the key
+   * for that collection is on the server.
+   */
+  checkSyncKeyRing: Task.async(function* () {
+    if (!cryptoCollection.isActive()) {
+      // We got called while no extensions use storage.sync. We don't
+      // have any access to the crypto record, so just let this
+      // notification slip through our fingers. If we do get
+      // extensions later, we'll pick this up on a subsequent sync.
+      log.info("Tried to check keyring, but no extensions are loaded. Ignoring.");
+      return;
+    }
+
+    yield this.updateKeyRingKB();
+
+    const cryptoKeyRecord = yield cryptoCollection.getKeyRingRecord();
+    if (cryptoKeyRecord && cryptoKeyRecord._status !== "synced") {
+      // We haven't successfully synced the keyring since the last
+      // change. This could be because kB changed and we touched the
+      // keyring, or it could be because we failed to sync after
+      // adding a key. Either way, take this opportunity to sync the
+      // keyring.
+      //
+      // We use server_wins here because whatever is on the server is
+      // at least consistent with itself -- the crypto in the keyring
+      // matches the crypto on the collection records. This is because
+      // we generate and upload keys just before syncing data.
+      //
+      // We can also get into the unhappy situation where we need to
+      // resolve conflicts with a record uploaded by a client with a
+      // different password. In this case, we will fail (throw an
+      // exception) because we can't decrypt the record. This behavior
+      // is correct because we have absolutely no chance of resolving
+      // conflicts intelligently -- in particular, we can't prove that
+      // uploading our key won't erase keys that have already been
+      // used on remote data. If this happens, hopefully the user will
+      // relogin so that all devices have a consistent kB; this will
+      // ensure that the most recent version on the server is
+      // encrypted with the same kB that other devices have, and they
+      // will sync the keyring successfully on subsequent syncs.
+      yield cryptoCollection.sync();
+    }
+  }),
+
+  /**
    * Get the collection for an extension, consulting a cache to
    * save time.
    *
    * @param {Extension} extension
    *                    The extension for which we are seeking
    *                    a collection.
    * @param {Context} context
    *                  The context of the extension, so that we can
--- a/toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js
@@ -12,19 +12,21 @@ const {
   CollectionKeyEncryptionRemoteTransformer,
   cryptoCollection,
   idToKey,
   extensionIdToCollectionId,
   keyToId,
 } = Cu.import("resource://gre/modules/ExtensionStorageSync.jsm");
 Cu.import("resource://services-sync/engines/extension-storage.js");
 Cu.import("resource://services-sync/keys.js");
+Cu.import("resource://services-sync/util.js");
 
 /* globals BulkKeyBundle, CommonUtils, EncryptionRemoteTransformer */
 /* globals KeyRingEncryptionRemoteTransformer */
+/* globals Utils */
 
 function handleCannedResponse(cannedResponse, request, response) {
   response.setStatusLine(null, cannedResponse.status.status,
                          cannedResponse.status.statusText);
   // send the headers
   for (let headerLine of cannedResponse.sampleHeaders) {
     let headerElements = headerLine.split(":");
     response.setHeader(headerElements[0], headerElements[1].trimLeft());
@@ -501,16 +503,73 @@ add_task(function* ensureKeysFor_handles
       ok(body.keys.collections[extensionId],
          `decrypted failed post should have a key for ${extensionId}`);
       notEqual(body.keys.collections[extensionId], RANDOM_KEY.keyPairB64,
                `decrypted failed post should have a randomly-generated key for ${extensionId}`);
     });
   });
 });
 
+add_task(function* checkSyncKeyRing_reuploads_keys() {
+  // Verify that when keys are present, they are reuploaded with the
+  // new kB when we call touchKeys().
+  const extensionId = uuid();
+  let extensionKey;
+  yield* withContextAndServer(function* (context, server) {
+    yield* withSignedInUser(loggedInUser, function* () {
+      server.installCollection("storage-sync-crypto");
+      server.etag = 765;
+
+      // Prompt ExtensionStorageSync to initialize crypto
+      yield ExtensionStorageSync.get({id: extensionId}, "random-key", context);
+      yield cryptoCollection._clear();
+
+      // Do an `ensureKeysFor` to generate some keys.
+      let collectionKeys = yield ExtensionStorageSync.ensureKeysFor([extensionId]);
+      ok(collectionKeys.hasKeysFor([extensionId]),
+         `ensureKeysFor should return a keyring that has a key for ${extensionId}`);
+      extensionKey = collectionKeys.keyForCollection(extensionId).keyPairB64;
+      equal(server.getPosts().length, 1,
+            "generating a key that doesn't exist on the server should post it");
+    });
+
+    // The user changes their password. This is their new kB, with
+    // the last f changed to an e.
+    const NOVEL_KB = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdee";
+    const newUser = Object.assign({}, loggedInUser, {kB: NOVEL_KB});
+    let postedKeys;
+    yield* withSignedInUser(newUser, function* () {
+      yield ExtensionStorageSync.checkSyncKeyRing();
+
+      let posts = server.getPosts();
+      equal(posts.length, 2,
+            "when kB changes, checkSyncKeyRing should post the keyring reencrypted with the new kB");
+      postedKeys = posts[1];
+      assertPostedUpdatedRecord(postedKeys, 765);
+
+      let body = yield assertPostedEncryptedKeys(postedKeys);
+      deepEqual(body.keys.collections[extensionId], extensionKey,
+                `the posted keyring should have the same key for ${extensionId} as the old one`);
+    });
+
+    // Verify that with the old kB, we can't decrypt the record.
+    yield* withSignedInUser(loggedInUser, function* () {
+      let error;
+      try {
+        yield new KeyRingEncryptionRemoteTransformer().decode(postedKeys.body.data);
+      } catch (e) {
+        error = e;
+      }
+      ok(error, "decrypting the keyring with the old kB should fail");
+      ok(Utils.isHMACMismatch(error),
+         "decrypting the keyring with the old kB should throw an HMAC mismatch");
+    });
+  });
+});
+
 add_task(function* test_storage_sync_pulls_changes() {
   yield* withContextAndServer(function* (context, server) {
     yield* withSignedInUser(loggedInUser, function* () {
       let transformer = new CollectionKeyEncryptionRemoteTransformer(extensionId);
       server.installCollection(collectionId);
       server.installCollection("storage-sync-crypto");
 
       let calls = [];