[EC-598] fix: rpId validation logic

libs/common/src/webauthn/services/domain-utils.spec.ts

@@ -0,0 +1,53 @@
+import { isValidRpId } from "./domain-utils";
+
+// Spec: If options.rp.id is not a registrable domain suffix of and is not equal to effectiveDomain, return a DOMException whose name is "SecurityError", and terminate this algorithm.
+describe("validateRpId", () => {
+  it("should not be valid when rpId is more specific than origin", () => {
+    const rpId = "sub.login.bitwarden.com";
+    const origin = "https://login.bitwarden.com:1337";
+
+    expect(isValidRpId(rpId, origin)).toBe(false);
+  });
+
+  it("should not be valid when effective domains of rpId and origin do not match", () => {
+    const rpId = "passwordless.dev";
+    const origin = "https://login.bitwarden.com:1337";
+
+    expect(isValidRpId(rpId, origin)).toBe(false);
+  });
+
+  it("should not be valid when subdomains are the same but effective domains of rpId and origin do not match", () => {
+    const rpId = "login.passwordless.dev";
+    const origin = "https://login.bitwarden.com:1337";
+
+    expect(isValidRpId(rpId, origin)).toBe(false);
+  });
+
+  it("should be valid when domains of rpId and origin are the same", () => {
+    const rpId = "bitwarden.com";
+    const origin = "https://bitwarden.com";
+
+    expect(isValidRpId(rpId, origin)).toBe(true);
+  });
+
+  it("should be valid when origin is a subdomain of rpId", () => {
+    const rpId = "bitwarden.com";
+    const origin = "https://login.bitwarden.com:1337";
+
+    expect(isValidRpId(rpId, origin)).toBe(true);
+  });
+
+  it("should be valid when domains of rpId and origin are the same and they are both subdomains", () => {
+    const rpId = "login.bitwarden.com";
+    const origin = "https://login.bitwarden.com:1337";
+
+    expect(isValidRpId(rpId, origin)).toBe(true);
+  });
+
+  it("should be valid when origin is a subdomain of rpId and they are both subdomains", () => {
+    const rpId = "login.bitwarden.com";
+    const origin = "https://sub.login.bitwarden.com:1337";
+
+    expect(isValidRpId(rpId, origin)).toBe(true);
+  });
+});

libs/common/src/webauthn/services/domain-utils.ts

@@ -0,0 +1,11 @@
+import { parse } from "tldts";
+
+export function isValidRpId(rpId: string, origin: string) {
+  const parsedOrigin = parse(origin, { allowPrivateDomains: true });
+  const parsedRpId = parse(rpId, { allowPrivateDomains: true });
+
+  return (
+    parsedOrigin.domain === parsedRpId.domain &&
+    parsedOrigin.subdomain.endsWith(parsedRpId.subdomain)
+  );
+}

libs/common/src/webauthn/services/fido2-authenticator.service.ts

@@ -115,7 +115,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
         cipher.fido2Key = await createKeyView(params, keyPair.privateKey);
         const reencrypted = await this.cipherService.encrypt(cipher);
         await this.cipherService.updateWithServer(reencrypted);
-      } catch {
+      } catch (error) {
         throw new Fido2AutenticatorError(Fido2AutenticatorErrorCode.Unknown);
       }
     }

libs/common/src/webauthn/services/fido2-client.service.spec.ts

@@ -28,7 +28,7 @@ describe("FidoAuthenticatorService", () => {
   });
 
   describe("createCredential", () => {
-    describe("invalid input parameters", () => {
+    describe("input parameters validation", () => {
       // Spec: If sameOriginWithAncestors is false, return a "NotAllowedError" DOMException.
       it("should throw error if sameOriginWithAncestors is false", async () => {
         const params = createParams({ sameOriginWithAncestors: false });
@@ -81,10 +81,23 @@ describe("FidoAuthenticatorService", () => {
       });
 
       // Spec: If options.rp.id is not a registrable domain suffix of and is not equal to effectiveDomain, return a DOMException whose name is "SecurityError", and terminate this algorithm.
-      it("should throw error if rp.id does not match origin effective domain", async () => {
+      it("should throw error if rp.id is not valid for this origin", async () => {
         const params = createParams({
-          origin: "passwordless.dev",
+          origin: "https://passwordless.dev",
-          rp: { id: "bitwarden.com", name: "Bitwarden" },
+          rp: { id: "bitwarden.com", name: "Bitwraden" },
+        });
+
+        const result = async () => await client.createCredential(params);
+
+        const rejects = expect(result).rejects;
+        await rejects.toMatchObject({ name: "SecurityError" });
+        await rejects.toBeInstanceOf(DOMException);
+      });
+
+      it("should throw error if origin is not an https domain", async () => {
+        const params = createParams({
+          origin: "http://passwordless.dev",
+          rp: { id: "bitwarden.com", name: "Bitwraden" },
         });
 
         const result = async () => await client.createCredential(params);
@@ -179,7 +192,7 @@ describe("FidoAuthenticatorService", () => {
 
     function createParams(params: Partial<CreateCredentialParams> = {}): CreateCredentialParams {
       return {
-        origin: params.origin ?? "bitwarden.com",
+        origin: params.origin ?? "https://bitwarden.com",
         sameOriginWithAncestors: params.sameOriginWithAncestors ?? true,
         attestation: params.attestation,
         authenticatorSelection: params.authenticatorSelection,
@@ -234,9 +247,22 @@ describe("FidoAuthenticatorService", () => {
       });
 
       // Spec: If options.rp.id is not a registrable domain suffix of and is not equal to effectiveDomain, return a DOMException whose name is "SecurityError", and terminate this algorithm.
-      it("should throw error if rp.id does not match origin effective domain", async () => {
+      it("should throw error if rp.id is not valid for this origin", async () => {
         const params = createParams({
-          origin: "passwordless.dev",
+          origin: "https://passwordless.dev",
+          rpId: "bitwarden.com",
+        });
+
+        const result = async () => await client.assertCredential(params);
+
+        const rejects = expect(result).rejects;
+        await rejects.toMatchObject({ name: "SecurityError" });
+        await rejects.toBeInstanceOf(DOMException);
+      });
+
+      it("should throw error if origin is not an http domain", async () => {
+        const params = createParams({
+          origin: "http://passwordless.dev",
           rpId: "bitwarden.com",
         });
 
@@ -345,7 +371,7 @@ describe("FidoAuthenticatorService", () => {
       return {
         allowedCredentialIds: params.allowedCredentialIds ?? [],
         challenge: params.challenge ?? Fido2Utils.bufferToString(randomBytes(16)),
-        origin: params.origin ?? RpId,
+        origin: params.origin ?? "https://bitwarden.com",
         rpId: params.rpId ?? RpId,
         timeout: params.timeout,
         userVerification: params.userVerification,

libs/common/src/webauthn/services/fido2-client.service.ts

@@ -20,6 +20,8 @@ import {
 } from "../abstractions/fido2-client.service.abstraction";
 import { Fido2Utils } from "../abstractions/fido2-utils";
 
+import { isValidRpId } from "./domain-utils";
+
 export class Fido2ClientService implements Fido2ClientServiceAbstraction {
   constructor(private authenticator: Fido2AuthenticatorService) {}
 
@@ -36,14 +38,15 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction {
       throw new TypeError("Invalid 'user.id' length");
     }
 
-    const { domain: effectiveDomain } = parse(params.origin, { allowPrivateDomains: true });
+    const parsedOrigin = parse(params.origin, { allowPrivateDomains: true });
-    if (effectiveDomain == undefined) {
+    const rpId = params.rp.id ?? parsedOrigin.domain;
-      throw new DOMException("'origin' is not a valid domain", "SecurityError");
+
+    if (parsedOrigin.domain == undefined || !params.origin.startsWith("https://")) {
+      throw new DOMException("'origin' is not a valid https origin", "SecurityError");
     }
 
-    const rpId = params.rp.id ?? effectiveDomain;
+    if (!isValidRpId(rpId, params.origin)) {
-    if (effectiveDomain !== rpId) {
+      throw new DOMException("'rp.id' cannot be used with the current origin", "SecurityError");
-      throw new DOMException("'rp.id' does not match origin effective domain", "SecurityError");
     }
 
     let credTypesAndPubKeyAlgs: PublicKeyCredentialParam[];
@@ -72,19 +75,15 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction {
     const clientDataJSON = JSON.stringify(collectedClientData);
     const clientDataJSONBytes = Utils.fromByteStringToArray(clientDataJSON);
     const clientDataHash = await crypto.subtle.digest({ name: "SHA-256" }, clientDataJSONBytes);
-
     if (abortController.signal.aborted) {
       throw new DOMException(undefined, "AbortError");
     }
-
     const timeout = setAbortTimeout(
       abortController,
       params.authenticatorSelection?.userVerification,
       params.timeout
     );
-
     const excludeCredentialDescriptorList: PublicKeyCredentialDescriptor[] = [];
-
     if (params.excludeCredentials !== undefined) {
       for (const credential of params.excludeCredentials) {
         try {
@@ -97,7 +96,6 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction {
         } catch {}
       }
     }
-
     const makeCredentialParams: Fido2AuthenticatorMakeCredentialsParams = {
       requireResidentKey:
         params.authenticatorSelection?.residentKey === "required" ||
@@ -117,7 +115,6 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction {
         displayName: params.user.displayName,
       },
     };
-
     let makeCredentialResult;
     try {
       makeCredentialResult = await this.authenticator.makeCredential(
@@ -133,12 +130,10 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction {
       }
       throw new DOMException(undefined, "NotAllowedError");
     }
-
     if (abortController.signal.aborted) {
       throw new DOMException(undefined, "AbortError");
     }
     clearTimeout(timeout);
-
     return {
       credentialId: Fido2Utils.bufferToString(makeCredentialResult.credentialId),
       attestationObject: Fido2Utils.bufferToString(makeCredentialResult.attestationObject),
@@ -158,9 +153,15 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction {
       throw new DOMException("'origin' is not a valid domain", "SecurityError");
     }
 
-    const rpId = params.rpId ?? effectiveDomain;
+    const parsedOrigin = parse(params.origin, { allowPrivateDomains: true });
-    if (effectiveDomain !== rpId) {
+    const rpId = params.rpId ?? parsedOrigin.domain;
-      throw new DOMException("'rp.id' does not match origin effective domain", "SecurityError");
+
+    if (parsedOrigin.domain == undefined || !params.origin.startsWith("https://")) {
+      throw new DOMException("'origin' is not a valid https origin", "SecurityError");
+    }
+
+    if (!isValidRpId(rpId, params.origin)) {
+      throw new DOMException("'rp.id' cannot be used with the current origin", "SecurityError");
     }
 
     const collectedClientData = {