Bug 1253740 - Define checkSyncKeyRing() which reuploads keys when passwords change, r=markh

MozReview-Commit-ID: B5Ptj4MGAC

--HG--
extra : rebase_source : a3f3de0b46bbcfde8ca46c35db0408f0fda89ca2
extra : intermediate-source : 7955771eedfaa956ae62d94c3d599948d0316e46
extra : source : d282a924f350b4d6ea21e3e1b4c9ea2ad09d3df6

toolkit/components/extensions/ExtensionStorageSync.jsm

@@ -219,6 +219,10 @@ const cryptoCollection = this.cryptoCollection = {
     }
   }),
 
+  isActive() {
+    return this.refCount != 0;
+  },
+
   /**
    * Retrieve the keyring record from the crypto collection.
    *
@@ -251,6 +255,13 @@ const cryptoCollection = this.cryptoCollection = {
     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);
@@ -411,6 +422,7 @@ this.ExtensionStorageSync = {
         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);
@@ -538,10 +550,13 @@ this.ExtensionStorageSync = {
       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();
@@ -556,6 +571,97 @@ this.ExtensionStorageSync = {
     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.

toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js

@@ -17,9 +17,11 @@ const {
 } = 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,
@@ -506,6 +508,63 @@ add_task(function* ensureKeysFor_handles_conflicts() {
   });
 });
 
+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* () {