From 376be67f28b09c00a563d9eeb44e2c1aefda7e3a Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Wed, 22 Mar 2023 10:49:05 +0100 Subject: [PATCH] [EC-598] feat: rearrange order of execution --- .../fido2-authenticator.service.spec.ts | 112 ++++++++++++------ .../services/fido2-authenticator.service.ts | 47 ++++---- 2 files changed, 99 insertions(+), 60 deletions(-) diff --git a/libs/common/src/webauthn/services/fido2-authenticator.service.spec.ts b/libs/common/src/webauthn/services/fido2-authenticator.service.spec.ts index 20c9189fbe..725d69bbff 100644 --- a/libs/common/src/webauthn/services/fido2-authenticator.service.spec.ts +++ b/libs/common/src/webauthn/services/fido2-authenticator.service.spec.ts @@ -34,43 +34,6 @@ describe("FidoAuthenticatorService", () => { }); describe("authenticatorMakeCredential", () => { - describe("when vault contains excluded credential", () => { - let excludedCipherView: CipherView; - let params: Fido2AuthenticatorMakeCredentialsParams; - - beforeEach(async () => { - const excludedCipher = createCipher(); - excludedCipherView = await excludedCipher.decrypt(); - params = await createCredentialParams({ - excludeList: [{ id: Fido2Utils.stringToBuffer(excludedCipher.id), type: "public-key" }], - }); - cipherService.get.mockImplementation(async (id) => - id === excludedCipher.id ? excludedCipher : undefined - ); - cipherService.getAllDecrypted.mockResolvedValue([excludedCipherView]); - }); - - /** Spec: wait for user presence */ - it("should request confirmation from user", async () => { - userInterface.confirmDuplicateCredential.mockResolvedValue(true); - - await authenticator.makeCredential(params); - - expect(userInterface.confirmDuplicateCredential).toHaveBeenCalled(); - }); - - /** Spec: then terminate this procedure and return error code */ - it("should throw error if user denies duplication", async () => { - userInterface.confirmDuplicateCredential.mockResolvedValue(false); - - const result = async () => await authenticator.makeCredential(params); - - await expect(result).rejects.toThrowError( - Fido2AutenticatorErrorCode[Fido2AutenticatorErrorCode.CTAP2_ERR_CREDENTIAL_EXCLUDED] - ); - }); - }); - // Spec: If the pubKeyCredParams parameter does not contain a valid COSEAlgorithmIdentifier value that is supported by the authenticator, terminate this procedure and return error code it("should throw error when input does not contain any supported algorithms", async () => { const params = await createCredentialParams({ @@ -127,6 +90,62 @@ describe("FidoAuthenticatorService", () => { }); }); + describe("when vault contains excluded credential", () => { + let excludedCipherView: CipherView; + let params: Fido2AuthenticatorMakeCredentialsParams; + + beforeEach(async () => { + const excludedCipher = createCipher(); + excludedCipherView = await excludedCipher.decrypt(); + params = await createCredentialParams({ + excludeList: [{ id: Fido2Utils.stringToBuffer(excludedCipher.id), type: "public-key" }], + }); + cipherService.get.mockImplementation(async (id) => + id === excludedCipher.id ? excludedCipher : undefined + ); + cipherService.getAllDecrypted.mockResolvedValue([excludedCipherView]); + }); + + /** Spec: wait for user presence */ + it("should request confirmation from user", async () => { + userInterface.confirmDuplicateCredential.mockResolvedValue(true); + + await authenticator.makeCredential(params); + + expect(userInterface.confirmDuplicateCredential).toHaveBeenCalled(); + }); + + /** Spec: then terminate this procedure and return error code */ + it("should throw error if user denies duplication", async () => { + userInterface.confirmDuplicateCredential.mockResolvedValue(false); + + const result = async () => await authenticator.makeCredential(params); + + await expect(result).rejects.toThrowError( + Fido2AutenticatorErrorCode[Fido2AutenticatorErrorCode.CTAP2_ERR_CREDENTIAL_EXCLUDED] + ); + }); + + /** Departure from spec: Check duplication last instead of first */ + it("should not request confirmation from user when input data does not pass checks", async () => { + userInterface.confirmDuplicateCredential.mockResolvedValue(true); + const paramsList: Fido2AuthenticatorMakeCredentialsParams[] = [ + { ...params, options: { rk: "invalid-value" as any } }, + { ...params, options: { uv: "invalid-value" as any } }, + { ...params, pinAuth: { key: "value" } }, + { ...params, pubKeyCredParams: [{ alg: 9001, type: "public-key" }] }, + ]; + + for (const p of paramsList) { + try { + await authenticator.makeCredential(p); + // eslint-disable-next-line no-empty + } catch {} + } + expect(userInterface.confirmDuplicateCredential).not.toHaveBeenCalled(); + }); + }); + describe("when input passes all initial checks", () => { /** Spec: show the items contained within the user and rp parameter structures to the user. */ it("should request confirmation from user", async () => { @@ -153,6 +172,25 @@ describe("FidoAuthenticatorService", () => { ); }); }); + + it("should not request confirmation from user when input data does not pass checks", async () => { + userInterface.confirmDuplicateCredential.mockResolvedValue(true); + const params = await createCredentialParams(); + const paramsList: Fido2AuthenticatorMakeCredentialsParams[] = [ + { ...params, options: { rk: "invalid-value" as any } }, + { ...params, options: { uv: "invalid-value" as any } }, + { ...params, pinAuth: { key: "value" } }, + { ...params, pubKeyCredParams: [{ alg: 9001, type: "public-key" }] }, + ]; + + for (const p of paramsList) { + try { + await authenticator.makeCredential(p); + // eslint-disable-next-line no-empty + } catch {} + } + expect(userInterface.confirmNewCredential).not.toHaveBeenCalled(); + }); }); }); diff --git a/libs/common/src/webauthn/services/fido2-authenticator.service.ts b/libs/common/src/webauthn/services/fido2-authenticator.service.ts index 1abfb062d5..c9ef2a7e70 100644 --- a/libs/common/src/webauthn/services/fido2-authenticator.service.ts +++ b/libs/common/src/webauthn/services/fido2-authenticator.service.ts @@ -20,24 +20,6 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr ) {} async makeCredential(params: Fido2AuthenticatorMakeCredentialsParams): Promise { - const duplicateExists = await this.vaultContainsId( - params.excludeList.map((key) => Fido2Utils.bufferToString(key.id)) - ); - - if (duplicateExists) { - const userConfirmation = await this.userInterface.confirmDuplicateCredential( - [Fido2Utils.bufferToString(params.excludeList[0].id)], - { - credentialName: params.rp.name, - userName: params.user.name, - } - ); - - if (!userConfirmation) { - throw new Fido2AutenticatorError(Fido2AutenticatorErrorCode.CTAP2_ERR_CREDENTIAL_EXCLUDED); - } - } - if (params.pubKeyCredParams.every((p) => p.alg !== Fido2AlgorithmIdentifier.ES256)) { throw new Fido2AutenticatorError(Fido2AutenticatorErrorCode.CTAP2_ERR_UNSUPPORTED_ALGORITHM); } @@ -54,15 +36,34 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr throw new Fido2AutenticatorError(Fido2AutenticatorErrorCode.CTAP2_ERR_PIN_AUTH_INVALID); } - if (!duplicateExists) { - const userVerification = await this.userInterface.confirmNewCredential({ + // In the spec the `excludeList` is checked first. + // We deviate from this because we allow duplicates to be created if the user confirms it, + // and we don't want to ask the user for confirmation if the input params haven't already + // been verified. + const duplicateExists = await this.vaultContainsId( + params.excludeList.map((key) => Fido2Utils.bufferToString(key.id)) + ); + let userVerification = false; + + if (duplicateExists) { + userVerification = await this.userInterface.confirmDuplicateCredential( + [Fido2Utils.bufferToString(params.excludeList[0].id)], + { + credentialName: params.rp.name, + userName: params.user.name, + } + ); + } else { + userVerification = await this.userInterface.confirmNewCredential({ credentialName: params.rp.name, userName: params.user.name, }); + } - if (!userVerification) { - throw new Fido2AutenticatorError(Fido2AutenticatorErrorCode.CTAP2_ERR_OPERATION_DENIED); - } + if (!userVerification && duplicateExists) { + throw new Fido2AutenticatorError(Fido2AutenticatorErrorCode.CTAP2_ERR_CREDENTIAL_EXCLUDED); + } else if (!userVerification && !duplicateExists) { + throw new Fido2AutenticatorError(Fido2AutenticatorErrorCode.CTAP2_ERR_OPERATION_DENIED); } }