Bug 1538248 - Verify authenticity of Remote Settings records for Normandy r=mythmon,Gijs
authorMathieu Leplatre <mathieu@mozilla.com>
Mon, 01 Apr 2019 17:48:05 +0000
changeset 467553 7c95672d5215f726e3b7801e290eb26ae181293e
parent 467552 8f80c03171c9ea34ca7c7ee4962647643f000cf0
child 467554 ac417f5eb34d6dd13da2494551cee792689a6e7a
push id35802
push usercbrindusan@mozilla.com
push dateTue, 02 Apr 2019 15:46:39 +0000
treeherdermozilla-central@2eb225d3082f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmythmon, Gijs
bugs1538248
milestone68.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 1538248 - Verify authenticity of Remote Settings records for Normandy r=mythmon,Gijs Verify authenticity of Remote Settings records for Normandy Differential Revision: https://phabricator.services.mozilla.com/D24716
toolkit/components/normandy/lib/NormandyApi.jsm
toolkit/components/normandy/lib/RecipeRunner.jsm
toolkit/components/normandy/test/browser/browser_RecipeRunner.js
--- a/toolkit/components/normandy/lib/NormandyApi.jsm
+++ b/toolkit/components/normandy/lib/NormandyApi.jsm
@@ -50,21 +50,22 @@ var NormandyApi = {
     return this.apiCall("get", endpoint, data);
   },
 
   post(endpoint, data) {
     return this.apiCall("post", endpoint, data);
   },
 
   absolutify(url) {
+    if (url.startsWith("http")) {
+      return url;
+    }
     const apiBase = prefs.getCharPref("api_url");
     const server = new URL(apiBase).origin;
-    if (url.startsWith("http")) {
-      return url;
-    } else if (url.startsWith("/")) {
+    if (url.startsWith("/")) {
       return server + url;
     }
     throw new Error("Can't use relative urls");
   },
 
   async getApiUrl(name) {
     if (!indexPromise) {
       const apiBase = new URL(prefs.getCharPref("api_url"));
@@ -82,60 +83,75 @@ var NormandyApi = {
   },
 
   async fetchSignedObjects(type, filters) {
     const signedObjectsUrl = await this.getApiUrl(`${type}-signed`);
     const objectsResponse = await this.get(signedObjectsUrl, filters);
     const rawText = await objectsResponse.text();
     const objectsWithSigs = JSON.parse(rawText);
 
-    const verifiedObjects = [];
-
-    for (const objectWithSig of objectsWithSigs) {
-      const {signature, x5u} = objectWithSig.signature;
-      const object = objectWithSig[type];
-
-      const serialized = CanonicalJSON.stringify(object);
-      // Check that the rawtext (the object and the signature)
-      // includes the CanonicalJSON version of the object. This isn't
-      // strictly needed, but it is a great benefit for debugging
-      // signature problems.
-      if (!rawText.includes(serialized)) {
-        log.debug(rawText, serialized);
-        throw new NormandyApi.InvalidSignatureError(
-          `Canonical ${type} serialization does not match!`);
-      }
-
-      const certChainResponse = await this.get(this.absolutify(x5u));
-      const certChain = await certChainResponse.text();
-      const builtSignature = `p384ecdsa=${signature}`;
+    return Promise.all(
+      objectsWithSigs.map(async (item) => {
+        // Check that the rawtext (the object and the signature)
+        // includes the CanonicalJSON version of the object. This isn't
+        // strictly needed, but it is a great benefit for debugging
+        // signature problems.
+        const object = item[type];
+        const serialized = CanonicalJSON.stringify(object);
+        if (!rawText.includes(serialized)) {
+          log.debug(rawText, serialized);
+          throw new NormandyApi.InvalidSignatureError(
+            `Canonical ${type} serialization does not match!`);
+        }
+        // Verify content signature using cryptography (will throw if fails).
+        await this.verifyObjectSignature(serialized, item.signature, type);
+        return object;
+      })
+    );
+  },
 
-      const verifier = Cc["@mozilla.org/security/contentsignatureverifier;1"]
-        .createInstance(Ci.nsIContentSignatureVerifier);
+  /**
+   * Verify content signature, by serializing the specified `object` as
+   * canonical JSON, and using the Normandy signer verifier to check that
+   * it matches the signature specified in `signaturePayload`.
+   *
+   * @param {object|String} data The object (or string) to be checked
+   * @param {object} signaturePayload The signature information
+   * @param {String} signaturePayload.x5u The certificate chain URL
+   * @param {String} signaturePayload.signature base64 signature bytes
+   * @param {String} type The object type (eg. `"recipe"`, `"action"`)
+   *
+   * @throws {NormandyApi.InvalidSignatureError} if signature is invalid.
+   */
+  async verifyObjectSignature(data, signaturePayload, type) {
+    const { signature, x5u } = signaturePayload;
+    const certChainResponse = await this.get(this.absolutify(x5u));
+    const certChain = await certChainResponse.text();
+    const builtSignature = `p384ecdsa=${signature}`;
 
-      let valid;
-      try {
-        valid = verifier.verifyContentSignature(
-          serialized,
-          builtSignature,
-          certChain,
-          "normandy.content-signature.mozilla.org"
-        );
-      } catch (err) {
-        throw new NormandyApi.InvalidSignatureError(`${type} signature validation failed: ${err}`);
-      }
+    const serialized = typeof data == "string" ? data : CanonicalJSON.stringify(data);
+
+    const verifier = Cc["@mozilla.org/security/contentsignatureverifier;1"]
+      .createInstance(Ci.nsIContentSignatureVerifier);
 
-      if (!valid) {
-        throw new NormandyApi.InvalidSignatureError(`${type} signature is not valid`);
-      }
-
-      verifiedObjects.push(object);
+    let valid;
+    try {
+      valid = verifier.verifyContentSignature(
+        serialized,
+        builtSignature,
+        certChain,
+        "normandy.content-signature.mozilla.org"
+      );
+    } catch (err) {
+      throw new NormandyApi.InvalidSignatureError(`${type} signature validation failed: ${err}`);
     }
 
-    return verifiedObjects;
+    if (!valid) {
+      throw new NormandyApi.InvalidSignatureError(`${type} signature is not valid`);
+    }
   },
 
   /**
    * Fetch metadata about this client determined by the server.
    * @return {object} Metadata specified by the server
    */
   async classifyClient() {
     const classifyClientUrl = await this.getApiUrl("classify-client");
--- a/toolkit/components/normandy/lib/RecipeRunner.jsm
+++ b/toolkit/components/normandy/lib/RecipeRunner.jsm
@@ -45,17 +45,17 @@ const PREFS_TO_WATCH = [
   RUN_INTERVAL_PREF,
   TELEMETRY_ENABLED_PREF,
   SHIELD_ENABLED_PREF,
   API_URL_PREF,
 ];
 
 XPCOMUtils.defineLazyGetter(this, "gRemoteSettingsClient", () => {
   return RemoteSettings(REMOTE_SETTINGS_COLLECTION, {
-    filterFunc: async recipe => (await RecipeRunner.checkFilter(recipe)) ? recipe : null,
+    filterFunc: async entry => (await RecipeRunner.checkFilter(entry.recipe)) ? entry : null,
   });
 });
 
 /**
  * cacheProxy returns an object Proxy that will memoize properties of the target.
  */
 function cacheProxy(target) {
   const cache = new Map();
@@ -210,17 +210,25 @@ var RecipeRunner = {
       }
     }
 
     // Fetch recipes before execution in case we fail and exit early.
     let recipesToRun;
     try {
       recipesToRun = await this.loadRecipes();
     } catch (e) {
-      // The legacy call to `Normandy.fetchRecipes()` can throw.
+      // Either we failed at fetching the recipes from server (legacy),
+      // or the recipes signature verification failed.
+      let status = Uptake.RUNNER_SERVER_ERROR;
+      if (/NetworkError/.test(e)) {
+        status = Uptake.RUNNER_NETWORK_ERROR;
+      } else if (e instanceof NormandyApi.InvalidSignatureError) {
+        status = Uptake.RUNNER_INVALID_SIGNATURE;
+      }
+      await Uptake.reportRunner(status);
       return;
     }
 
     const actions = new ActionsManager();
     await actions.fetchRemoteActions();
     await actions.preExecution();
 
     // Execute recipes, if we have any.
@@ -237,39 +245,38 @@ var RecipeRunner = {
     await Uptake.reportRunner(Uptake.RUNNER_SUCCESS);
   },
 
   /**
    * Return the list of recipes to run, filtered for the current environment.
    */
   async loadRecipes() {
     // If RemoteSettings is enabled, we read the list of recipes from there.
-    // The JEXL filtering is done via the provided callback.
+    // The JEXL filtering is done via the provided callback (see `gRemoteSettingsClient`).
     if (await FeatureGate.isEnabled("normandy-remote-settings")) {
-      return gRemoteSettingsClient.get();
+      // First, fetch recipes whose JEXL filters match.
+      const entries = await gRemoteSettingsClient.get();
+      // Then, verify the signature of each recipe. It will throw if invalid.
+      return Promise.all(entries.map(async ( { recipe, signature } ) => {
+        await NormandyApi.verifyObjectSignature(recipe, signature, "recipe");
+        return recipe;
+      }));
     }
+
     // Obtain the recipes from the Normandy server (legacy).
     let recipes;
     try {
       recipes = await NormandyApi.fetchRecipes({enabled: true});
       log.debug(
         `Fetched ${recipes.length} recipes from the server: ` +
         recipes.map(r => r.name).join(", ")
       );
     } catch (e) {
       const apiUrl = Services.prefs.getCharPref(API_URL_PREF);
       log.error(`Could not fetch recipes from ${apiUrl}: "${e}"`);
-
-      let status = Uptake.RUNNER_SERVER_ERROR;
-      if (/NetworkError/.test(e)) {
-        status = Uptake.RUNNER_NETWORK_ERROR;
-      } else if (e instanceof NormandyApi.InvalidSignatureError) {
-        status = Uptake.RUNNER_INVALID_SIGNATURE;
-      }
-      await Uptake.reportRunner(status);
       throw e;
     }
     // Evaluate recipe filters
     const recipesToRun = [];
     for (const recipe of recipes) {
       if (await this.checkFilter(recipe)) {
         recipesToRun.push(recipe);
       }
--- a/toolkit/components/normandy/test/browser/browser_RecipeRunner.js
+++ b/toolkit/components/normandy/test/browser/browser_RecipeRunner.js
@@ -200,53 +200,98 @@ decorate_task(
 );
 
 decorate_task(
   withPrefEnv({
     set: [
       ["features.normandy-remote-settings.enabled", true],
     ],
   }),
+  withStub(NormandyApi, "verifyObjectSignature"),
   withStub(ActionsManager.prototype, "runRecipe"),
   withStub(ActionsManager.prototype, "fetchRemoteActions"),
   withStub(ActionsManager.prototype, "finalize"),
   withStub(Uptake, "reportRecipe"),
   async function testReadFromRemoteSettings(
+    verifyObjectSignatureStub,
     runRecipeStub,
     fetchRemoteActionsStub,
     finalizeStub,
     reportRecipeStub,
   ) {
-    const matchRecipe = { id: "match", action: "matchAction", filter_expression: "true", _status: "synced", enabled: true };
-    const noMatchRecipe = { id: "noMatch", action: "noMatchAction", filter_expression: "false", _status: "synced", enabled: true };
-    const missingRecipe = { id: "missing", action: "missingAction", filter_expression: "true", _status: "synced", enabled: true };
+    const matchRecipe =  { name: "match", action: "matchAction", filter_expression: "true" };
+    const noMatchRecipe = { name: "noMatch", action: "noMatchAction", filter_expression: "false" };
+    const missingRecipe = { name: "missing", action: "missingAction", filter_expression: "true" };
 
     const rsCollection = await RecipeRunner._remoteSettingsClientForTesting.openCollection();
-    await rsCollection.create(matchRecipe, { synced: true });
-    await rsCollection.create(noMatchRecipe, { synced: true });
-    await rsCollection.create(missingRecipe, { synced: true });
+    await rsCollection.clear();
+    const fakeSig = { signature: "abc" };
+    await rsCollection.create({ id: "match", recipe: matchRecipe, signature: fakeSig }, { synced: true });
+    await rsCollection.create({ id: "noMatch", recipe: noMatchRecipe, signature: fakeSig }, { synced: true });
+    await rsCollection.create({ id: "missing", recipe: missingRecipe, signature: fakeSig }, { synced: true });
     await rsCollection.db.saveLastModified(42);
     rsCollection.db.close();
 
     await RecipeRunner.run();
 
     Assert.deepEqual(
+      verifyObjectSignatureStub.args,
+      [[matchRecipe, fakeSig, "recipe"], [missingRecipe, fakeSig, "recipe"]],
+      "recipes with matching filters should have their signature verified",
+    );
+    Assert.deepEqual(
       runRecipeStub.args,
       [[matchRecipe], [missingRecipe]],
-      "recipe with matching filters should be executed",
+      "recipes with matching filters should be executed",
     );
     Assert.deepEqual(
       reportRecipeStub.args,
       [[noMatchRecipe, Uptake.RECIPE_DIDNT_MATCH_FILTER]],
       "Filtered-out recipes should be reported",
     );
   }
 );
 
 decorate_task(
+  withPrefEnv({
+    set: [
+      ["features.normandy-remote-settings.enabled", true],
+    ],
+  }),
+  withStub(ActionsManager.prototype, "runRecipe"),
+  withStub(NormandyApi, "get"),
+  withStub(Uptake, "reportRunner"),
+  async function testBadSignatureFromRemoteSettings(
+    runRecipeStub,
+    normandyGetStub,
+    reportRunnerStub,
+  ) {
+    normandyGetStub.resolves({ async text() { return "---CERT x5u----"; } });
+
+    const matchRecipe = { name: "badSig", action: "matchAction", filter_expression: "true" };
+
+    const rsCollection = await RecipeRunner._remoteSettingsClientForTesting.openCollection();
+    await rsCollection.clear();
+    const badSig = { x5u: "http://localhost/x5u", signature: "abc" };
+    await rsCollection.create({ id: "badSig", recipe: matchRecipe, signature: badSig }, { synced: true });
+    await rsCollection.db.saveLastModified(42);
+    rsCollection.db.close();
+
+    await RecipeRunner.run();
+
+    ok(!runRecipeStub.called, "no recipe is executed");
+    Assert.deepEqual(
+      reportRunnerStub.args,
+      [[Uptake.RUNNER_INVALID_SIGNATURE]],
+      "RecipeRunner should report uptake telemetry",
+    );
+  }
+);
+
+decorate_task(
   withMockNormandyApi,
   async function testRunFetchFail(mockApi) {
     const reportRunner = sinon.stub(Uptake, "reportRunner");
 
     const action = {name: "action"};
     mockApi.actions = [action];
     mockApi.fetchRecipes.rejects(new Error("Signature not valid"));