Bug 1883424: Add scoped key validation to oauth. r=markh

Differential Revision: https://phabricator.services.mozilla.com/D203506

services/fxaccounts/FxAccounts.sys.mjs

@@ -865,7 +865,7 @@ FxAccountsInternal.prototype = {
   _oauth: null,
   get oauth() {
     if (!this._oauth) {
-      this._oauth = new lazy.FxAccountsOAuth(this.fxAccountsClient);
+      this._oauth = new lazy.FxAccountsOAuth(this.fxAccountsClient, this.keys);
     }
     return this._oauth;
   },

services/fxaccounts/FxAccountsKeys.sys.mjs

@@ -131,6 +131,67 @@ export class FxAccountsKeys {
     };
   }
 
+  /**
+   * Validates if the given scoped keys are valid keys
+   *
+   * @param { Object } scopedKeys: The scopedKeys bundle
+   *
+   * @return { Boolean }: true if the scopedKeys bundle is valid, false otherwise
+   */
+  validScopedKeys(scopedKeys) {
+    for (const expectedScope of Object.keys(scopedKeys)) {
+      const key = scopedKeys[expectedScope];
+      if (
+        !key.hasOwnProperty("scope") ||
+        !key.hasOwnProperty("kid") ||
+        !key.hasOwnProperty("kty") ||
+        !key.hasOwnProperty("k")
+      ) {
+        return false;
+      }
+      const { scope, kid, kty, k } = key;
+      if (scope != expectedScope || kty != "oct") {
+        return false;
+      }
+      // We verify the format of the key id is `timestamp-fingerprint`
+      if (!kid.includes("-")) {
+        return false;
+      }
+      const [keyRotationTimestamp, fingerprint] = kid.split("-");
+      // We then verify that the timestamp is a valid timestamp
+      const keyRotationTimestampNum = Number(keyRotationTimestamp);
+      // If the value we got back is falsy it's not a valid timestamp
+      // note that we treat a 0 timestamp as invalid
+      if (!keyRotationTimestampNum) {
+        return false;
+      }
+      // For extra safety, we validate that the timestamp can be converted into a valid
+      // Date object
+      const date = new Date(keyRotationTimestampNum);
+      if (isNaN(date.getTime()) || date.getTime() <= 0) {
+        return false;
+      }
+
+      // Finally, we validate that the fingerprint and the key itself are valid base64 values
+      // Note that we can't verify the fingerprint is correct here because we don't have kb
+      const validB64String = b64String => {
+        let decoded;
+        try {
+          decoded = ChromeUtils.base64URLDecode(b64String, {
+            padding: "reject",
+          });
+        } catch (e) {
+          return false;
+        }
+        return !!decoded;
+      };
+      if (!validB64String(fingerprint) || !validB64String(k)) {
+        return false;
+      }
+    }
+    return true;
+  }
+
   /**
    * Format a JWK kid as hex rather than base64.
    *

services/fxaccounts/FxAccountsOAuth.sys.mjs

@@ -22,6 +22,7 @@ export const ERROR_INVALID_STATE = "INVALID_STATE";
 export const ERROR_SYNC_SCOPE_NOT_GRANTED = "ERROR_SYNC_SCOPE_NOT_GRANTED";
 export const ERROR_NO_KEYS_JWE = "ERROR_NO_KEYS_JWE";
 export const ERROR_OAUTH_FLOW_ABANDONED = "ERROR_OAUTH_FLOW_ABANDONED";
+export const ERROR_INVALID_SCOPED_KEYS = "ERROR_INVALID_SCOPED_KEYS";
 
 /**
  * Handles all logic and state related to initializing, and completing OAuth flows
@@ -32,14 +33,16 @@ export const ERROR_OAUTH_FLOW_ABANDONED = "ERROR_OAUTH_FLOW_ABANDONED";
 export class FxAccountsOAuth {
   #flow;
   #fxaClient;
+  #fxaKeys;
   /**
    * Creates a new FxAccountsOAuth
    *
    * @param { Object } fxaClient: The fxa client used to send http request to the oauth server
    */
-  constructor(fxaClient) {
+  constructor(fxaClient, fxaKeys) {
     this.#flow = {};
     this.#fxaClient = fxaClient;
+    this.#fxaKeys = fxaKeys;
   }
 
   /**
@@ -131,8 +134,10 @@ export class FxAccountsOAuth {
     // Generate a 43 byte code verifier for PKCE, in accordance with
     // https://datatracker.ietf.org/doc/html/rfc7636#section-7.1 which recommends a
     // 43-octet URL safe string
-    const codeVerifier = new Uint8Array(43);
+    // The byte array is 32 bytes
+    const codeVerifier = new Uint8Array(32);
     crypto.getRandomValues(codeVerifier);
+    // When base64 encoded, it is 43 bytes
     const codeVerifierB64 = ChromeUtils.base64URLEncode(codeVerifier, {
       pad: false,
     });
@@ -147,7 +152,7 @@ export class FxAccountsOAuth {
     // Generate a public, private key pair to be used during the oauth flow
     // to encrypt scoped-keys as they roundtrip through the auth server
     const ECDH_KEY = { name: "ECDH", namedCurve: "P-256" };
-    const key = await crypto.subtle.generateKey(ECDH_KEY, true, ["deriveKey"]);
+    const key = await crypto.subtle.generateKey(ECDH_KEY, false, ["deriveKey"]);
     const publicKey = await crypto.subtle.exportKey("jwk", key.publicKey);
     const privateKey = key.privateKey;
 
@@ -205,6 +210,9 @@ export class FxAccountsOAuth {
       scopedKeys = JSON.parse(
         new TextDecoder().decode(await lazy.jwcrypto.decryptJWE(keys_jwe, key))
       );
+      if (!this.#fxaKeys.validScopedKeys(scopedKeys)) {
+        throw new Error(ERROR_INVALID_SCOPED_KEYS);
+      }
     }
 
     // We make sure no other flow snuck in, and completed before we did

services/fxaccounts/tests/xpcshell/test_keys.js

@@ -99,6 +99,128 @@ add_task(async function test_derive_multiple_keys_at_once() {
   });
 });
 
+add_task(function test_check_valid_scoped_keys() {
+  const keys = new FxAccountsKeys(null);
+  add_task(function test_missing_key_data() {
+    const scopedKeys = {
+      "https://identity.mozilla.com/apps/oldsync": {
+        kty: "oct",
+        kid: "1510726318123-IqQv4onc7VcVE1kTQkyyOw",
+        scope: "https://identity.mozilla.com/apps/oldsync",
+      },
+    };
+    Assert.equal(keys.validScopedKeys(scopedKeys), false);
+  });
+  add_task(function test_unexpected_scope() {
+    const scopedKeys = {
+      "https://identity.mozilla.com/apps/oldsync": {
+        kty: "oct",
+        kid: "1510726318123-IqQv4onc7VcVE1kTQkyyOw",
+        k: "DW_ll5GwX6SJ5GPqJVAuMUP2t6kDqhUulc2cbt26xbTcaKGQl-9l29FHAQ7kUiJETma4s9fIpEHrt909zgFang",
+        scope: "UnexpectedScope",
+      },
+    };
+    Assert.equal(keys.validScopedKeys(scopedKeys), false);
+  });
+  add_task(function test_not_oct_key() {
+    const scopedKeys = {
+      "https://identity.mozilla.com/apps/oldsync": {
+        // Should be "oct"!
+        kty: "EC",
+        kid: "1510726318123-IqQv4onc7VcVE1kTQkyyOw",
+        k: "DW_ll5GwX6SJ5GPqJVAuMUP2t6kDqhUulc2cbt26xbTcaKGQl-9l29FHAQ7kUiJETma4s9fIpEHrt909zgFang",
+        scope: "https://identity.mozilla.com/apps/oldsync",
+      },
+    };
+    Assert.equal(keys.validScopedKeys(scopedKeys), false);
+  });
+  add_task(function test_invalid_kid_not_timestamp() {
+    const scopedKeys = {
+      "https://identity.mozilla.com/apps/oldsync": {
+        kty: "oct",
+        // Does not have the timestamp!
+        kid: "IqQv4onc7VcVE1kTQkyyOw",
+        k: "DW_ll5GwX6SJ5GPqJVAuMUP2t6kDqhUulc2cbt26xbTcaKGQl-9l29FHAQ7kUiJETma4s9fIpEHrt909zgFang",
+        scope: "https://identity.mozilla.com/apps/oldsync",
+      },
+    };
+    Assert.equal(keys.validScopedKeys(scopedKeys), false);
+  });
+  add_task(function test_invalid_kid_not_valid_timestamp() {
+    const scopedKeys = {
+      "https://identity.mozilla.com/apps/oldsync": {
+        kty: "oct",
+        // foo is not a valid timestamp!
+        kid: "foo-IqQv4onc7VcVE1kTQkyyOw",
+        k: "DW_ll5GwX6SJ5GPqJVAuMUP2t6kDqhUulc2cbt26xbTcaKGQl-9l29FHAQ7kUiJETma4s9fIpEHrt909zgFang",
+        scope: "https://identity.mozilla.com/apps/oldsync",
+      },
+    };
+    Assert.equal(keys.validScopedKeys(scopedKeys), false);
+  });
+  add_task(function test_invalid_kid_not_b64_fingerprint() {
+    const scopedKeys = {
+      "https://identity.mozilla.com/apps/oldsync": {
+        kty: "oct",
+        // fingerprint not a valid base64 encoded string.
+        kid: "1510726318123-notvalidb64][",
+        k: "DW_ll5GwX6SJ5GPqJVAuMUP2t6kDqhUulc2cbt26xbTcaKGQl-9l29FHAQ7kUiJETma4s9fIpEHrt909zgFang",
+        scope: "https://identity.mozilla.com/apps/oldsync",
+      },
+    };
+    Assert.equal(keys.validScopedKeys(scopedKeys), false);
+  });
+  add_task(function test_invalid_k_not_base64() {
+    const scopedKeys = {
+      "https://identity.mozilla.com/apps/oldsync": {
+        kty: "oct",
+        kid: "1510726318123-IqQv4onc7VcVE1kTQkyyOw",
+        k: "notavalidb64[]",
+        scope: "https://identity.mozilla.com/apps/oldsync",
+      },
+    };
+    Assert.equal(keys.validScopedKeys(scopedKeys), false);
+  });
+
+  add_task(function test_multiple_scoped_keys_one_invalid() {
+    const scopedKeys = {
+      // Valid
+      "https://identity.mozilla.com/apps/otherscope": {
+        kty: "oct",
+        kid: "1510726318123-IqQv4onc7VcVE1kTQkyyOw",
+        k: "DW_ll5GwX6SJ5GPqJVAuMUP2t6kDqhUulc2cbt26xbTcaKGQl-9l29FHAQ7kUiJETma4s9fIpEHrt909zgFang",
+        scope: "https://identity.mozilla.com/apps/otherscope",
+      },
+      // Invalid
+      "https://identity.mozilla.com/apps/oldsync": {
+        kty: "oct",
+        kid: "1510726318123-IqQv4onc7VcVE1kTQkyyOw",
+        k: "notavalidb64[]",
+        scope: "https://identity.mozilla.com/apps/oldsync",
+      },
+    };
+    Assert.equal(keys.validScopedKeys(scopedKeys), false);
+  });
+
+  add_task(function test_valid_scopedkeys() {
+    const scopedKeys = {
+      "https://identity.mozilla.com/apps/oldsync": {
+        kty: "oct",
+        kid: "1510726318123-IqQv4onc7VcVE1kTQkyyOw",
+        k: "DW_ll5GwX6SJ5GPqJVAuMUP2t6kDqhUulc2cbt26xbTcaKGQl-9l29FHAQ7kUiJETma4s9fIpEHrt909zgFang",
+        scope: "https://identity.mozilla.com/apps/oldsync",
+      },
+      "https://identity.mozilla.com/apps/otherscope": {
+        kty: "oct",
+        kid: "1510726318123-IqQv4onc7VcVE1kTQkyyOw",
+        k: "DW_ll5GwX6SJ5GPqJVAuMUP2t6kDqhUulc2cbt26xbTcaKGQl-9l29FHAQ7kUiJETma4s9fIpEHrt909zgFang",
+        scope: "https://identity.mozilla.com/apps/otherscope",
+      },
+    };
+    Assert.equal(keys.validScopedKeys(scopedKeys), true);
+  });
+});
+
 add_task(async function test_rejects_bad_scoped_key_data() {
   const keys = new FxAccountsKeys(null);
   const uid = "aeaa1725c7a24ff983c6295725d5fc9b";

services/fxaccounts/tests/xpcshell/test_oauth_flow.js

@@ -8,6 +8,7 @@
 const {
   FxAccountsOAuth,
   ERROR_INVALID_SCOPES,
+  ERROR_INVALID_SCOPED_KEYS,
   ERROR_INVALID_STATE,
   ERROR_SYNC_SCOPE_NOT_GRANTED,
   ERROR_NO_KEYS_JWE,
@@ -22,6 +23,7 @@ const { SCOPE_PROFILE, FX_OAUTH_CLIENT_ID } = ChromeUtils.importESModule(
 
 ChromeUtils.defineESModuleGetters(this, {
   jwcrypto: "resource://services-crypto/jwcrypto.sys.mjs",
+  FxAccountsKeys: "resource://gre/modules/FxAccountsKeys.sys.mjs",
 });
 
 initTestLogging("Trace");
@@ -159,17 +161,21 @@ add_task(function test_complete_oauth_flow() {
     const oauthCode = "fake oauth code";
     const sessionToken = "01abcef12";
     const plainTextScopedKeys = {
-      kid: "fake key id",
+      "https://identity.mozilla.com/apps/oldsync": {
-      k: "fake key",
+        kty: "oct",
-      kty: "oct",
+        kid: "1510726318123-IqQv4onc7VcVE1kTQkyyOw",
+        k: "DW_ll5GwX6SJ5GPqJVAuMUP2t6kDqhUulc2cbt26xbTcaKGQl-9l29FHAQ7kUiJETma4s9fIpEHrt909zgFang",
+        scope: "https://identity.mozilla.com/apps/oldsync",
+      },
     };
     const fakeAccessToken = "fake access token";
     const fakeRefreshToken = "fake refresh token";
     // Then, we initialize a fake http client, we'll add our fake oauthToken call
     // once we have started the oauth flow (so we have the public keys!)
     const fxaClient = {};
+    const fxaKeys = new FxAccountsKeys(null);
     // Then, we initialize our oauth object with the given client and begin a new flow
-    const oauth = new FxAccountsOAuth(fxaClient);
+    const oauth = new FxAccountsOAuth(fxaClient, fxaKeys);
     const queryParams = await oauth.beginOAuthFlow(scopes);
     // Now that we have the public keys in `keys_jwk`, we use it to generate a JWE
     // representing our scoped keys
@@ -271,4 +277,72 @@ add_task(function test_complete_oauth_flow() {
     // Finally, we verify that all stored flows were cleared
     Assert.equal(oauth.numOfFlows(), 0);
   });
+  add_task(async function test_complete_oauth_invalid_scoped_keys() {
+    // First, we initialize some fake values we would typically get
+    // from outside our system
+    const scopes = [SCOPE_PROFILE, SCOPE_OLD_SYNC];
+    const oauthCode = "fake oauth code";
+    const sessionToken = "01abcef12";
+    const invalidScopedKeys = {
+      "https://identity.mozilla.com/apps/oldsync": {
+        // ====== This is an invalid key type! Should be "oct", so we will raise an error once we realize
+        kty: "EC",
+        kid: "1510726318123-IqQv4onc7VcVE1kTQkyyOw",
+        k: "DW_ll5GwX6SJ5GPqJVAuMUP2t6kDqhUulc2cbt26xbTcaKGQl-9l29FHAQ7kUiJETma4s9fIpEHrt909zgFang",
+        scope: "https://identity.mozilla.com/apps/oldsync",
+      },
+    };
+    const fakeAccessToken = "fake access token";
+    const fakeRefreshToken = "fake refresh token";
+    // Then, we initialize a fake http client, we'll add our fake oauthToken call
+    // once we have started the oauth flow (so we have the public keys!)
+    const fxaClient = {};
+    const fxaKeys = new FxAccountsKeys(null);
+    // Then, we initialize our oauth object with the given client and begin a new flow
+    const oauth = new FxAccountsOAuth(fxaClient, fxaKeys);
+    const queryParams = await oauth.beginOAuthFlow(scopes);
+    // Now that we have the public keys in `keys_jwk`, we use it to generate a JWE
+    // representing our scoped keys
+    const keysJwk = queryParams.keys_jwk;
+    const decodedKeysJwk = JSON.parse(
+      new TextDecoder().decode(
+        ChromeUtils.base64URLDecode(keysJwk, { padding: "reject" })
+      )
+    );
+    delete decodedKeysJwk.key_ops;
+    const jwe = await jwcrypto.generateJWE(
+      decodedKeysJwk,
+      new TextEncoder().encode(JSON.stringify(invalidScopedKeys))
+    );
+    // We also grab the stored PKCE verifier that the oauth object stored internally
+    // to verify that we correctly send it as a part of our HTTP request
+    const storedVerifier = oauth.getFlow(queryParams.state).verifier;
+
+    // Now we initialize our mock of the HTTP request, it verifies we passed in all the correct
+    // parameters and returns what we'd expect a healthy HTTP Response would look like
+    fxaClient.oauthToken = (sessionTokenHex, code, verifier, clientId) => {
+      Assert.equal(sessionTokenHex, sessionToken);
+      Assert.equal(code, oauthCode);
+      Assert.equal(verifier, storedVerifier);
+      Assert.equal(clientId, queryParams.client_id);
+      const response = {
+        access_token: fakeAccessToken,
+        refresh_token: fakeRefreshToken,
+        scope: scopes.join(" "),
+        keys_jwe: jwe,
+      };
+      return Promise.resolve(response);
+    };
+
+    // Then, we call the completeOAuthFlow function, and get back our access token,
+    // refresh token and scopedKeys
+    try {
+      await oauth.completeOAuthFlow(sessionToken, oauthCode, queryParams.state);
+      Assert.fail(
+        "Should have thrown an error because the scoped keys are not valid"
+      );
+    } catch (err) {
+      Assert.equal(err.message, ERROR_INVALID_SCOPED_KEYS);
+    }
+  });
 });