From 62e1e165c469d5153ff127eed695bb304d652fb1 Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Fri, 3 Nov 2023 18:27:32 +0100 Subject: [PATCH] [PM-4531] Do not override webauthn on excluded domains (#6790) * [PM-4531] feat: bypass fido2 if origin present in neverDomains * [PM-4531] feat: bypass fido2 during asserts as well * [PM-4531] fix: crashes when using `localhost` * [PM-4531] fix: add missing check * [PM-4531] fix: broken TLD logic * [PM-4531] feat: only allow localhost --- .../browser/src/background/main.background.ts | 1 + .../vault/services/fido2/domain-utils.spec.ts | 30 +++++++++++++++++ .../src/vault/services/fido2/domain-utils.ts | 8 +++-- .../fido2/fido2-client.service.spec.ts | 32 +++++++++++++++++-- .../services/fido2/fido2-client.service.ts | 20 ++++++++---- 5 files changed, 80 insertions(+), 11 deletions(-) diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index d9b65362de..8e4b9fe83b 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -598,6 +598,7 @@ export default class MainBackground { this.fido2AuthenticatorService, this.configService, this.authService, + this.stateService, this.logService ); diff --git a/libs/common/src/vault/services/fido2/domain-utils.spec.ts b/libs/common/src/vault/services/fido2/domain-utils.spec.ts index 7c9c27869a..4b99c06cde 100644 --- a/libs/common/src/vault/services/fido2/domain-utils.spec.ts +++ b/libs/common/src/vault/services/fido2/domain-utils.spec.ts @@ -23,6 +23,36 @@ describe("validateRpId", () => { expect(isValidRpId(rpId, origin)).toBe(false); }); + it("should not be valid when rpId and origin are both different TLD", () => { + const rpId = "bitwarden"; + const origin = "localhost"; + + expect(isValidRpId(rpId, origin)).toBe(false); + }); + + // Only allow localhost for rpId, need to properly investigate the implications of + // adding support for ip-addresses and other TLDs + it("should not be valid when rpId and origin are both the same TLD", () => { + const rpId = "bitwarden"; + const origin = "bitwarden"; + + expect(isValidRpId(rpId, origin)).toBe(false); + }); + + it("should not be valid when rpId and origin are ip-addresses", () => { + const rpId = "127.0.0.1"; + const origin = "127.0.0.1"; + + expect(isValidRpId(rpId, origin)).toBe(false); + }); + + it("should be valid when domains of rpId and origin are localhost", () => { + const rpId = "localhost"; + const origin = "https://localhost:8080"; + + expect(isValidRpId(rpId, origin)).toBe(true); + }); + it("should be valid when domains of rpId and origin are the same", () => { const rpId = "bitwarden.com"; const origin = "https://bitwarden.com"; diff --git a/libs/common/src/vault/services/fido2/domain-utils.ts b/libs/common/src/vault/services/fido2/domain-utils.ts index 20b6e41700..6f11ddb1a7 100644 --- a/libs/common/src/vault/services/fido2/domain-utils.ts +++ b/libs/common/src/vault/services/fido2/domain-utils.ts @@ -5,7 +5,11 @@ export function isValidRpId(rpId: string, origin: string) { const parsedRpId = parse(rpId, { allowPrivateDomains: true }); return ( - parsedOrigin.domain === parsedRpId.domain && - parsedOrigin.subdomain.endsWith(parsedRpId.subdomain) + (parsedOrigin.domain == null && + parsedOrigin.hostname == parsedRpId.hostname && + parsedOrigin.hostname == "localhost") || + (parsedOrigin.domain != null && + parsedOrigin.domain == parsedRpId.domain && + parsedOrigin.subdomain.endsWith(parsedRpId.subdomain)) ); } diff --git a/libs/common/src/vault/services/fido2/fido2-client.service.spec.ts b/libs/common/src/vault/services/fido2/fido2-client.service.spec.ts index 3bab5da102..46aa40c049 100644 --- a/libs/common/src/vault/services/fido2/fido2-client.service.spec.ts +++ b/libs/common/src/vault/services/fido2/fido2-client.service.spec.ts @@ -3,6 +3,7 @@ import { mock, MockProxy } from "jest-mock-extended"; import { AuthService } from "../../../auth/abstractions/auth.service"; import { AuthenticationStatus } from "../../../auth/enums/authentication-status"; import { ConfigServiceAbstraction } from "../../../platform/abstractions/config/config.service.abstraction"; +import { StateService } from "../../../platform/abstractions/state.service"; import { Utils } from "../../../platform/misc/utils"; import { Fido2AuthenticatorError, @@ -27,6 +28,7 @@ describe("FidoAuthenticatorService", () => { let authenticator!: MockProxy; let configService!: MockProxy; let authService!: MockProxy; + let stateService!: MockProxy; let client!: Fido2ClientService; let tab!: chrome.tabs.Tab; @@ -34,8 +36,9 @@ describe("FidoAuthenticatorService", () => { authenticator = mock(); configService = mock(); authService = mock(); + stateService = mock(); - client = new Fido2ClientService(authenticator, configService, authService); + client = new Fido2ClientService(authenticator, configService, authService, stateService); configService.getFeatureFlag.mockResolvedValue(true); tab = { id: 123, windowId: 456 } as chrome.tabs.Tab; }); @@ -97,7 +100,7 @@ describe("FidoAuthenticatorService", () => { it("should throw error if rp.id is not valid for this origin", async () => { const params = createParams({ origin: "https://passwordless.dev", - rp: { id: "bitwarden.com", name: "Bitwraden" }, + rp: { id: "bitwarden.com", name: "Bitwarden" }, }); const result = async () => await client.createCredential(params, tab); @@ -107,10 +110,22 @@ describe("FidoAuthenticatorService", () => { await rejects.toBeInstanceOf(DOMException); }); + it("should fallback if origin hostname is found in neverDomains", async () => { + const params = createParams({ + origin: "https://bitwarden.com", + rp: { id: "bitwarden.com", name: "Bitwarden" }, + }); + stateService.getNeverDomains.mockResolvedValue({ "bitwarden.com": null }); + + const result = async () => await client.createCredential(params, tab); + + await expect(result).rejects.toThrow(FallbackRequestedError); + }); + 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" }, + rp: { id: "bitwarden.com", name: "Bitwarden" }, }); const result = async () => await client.createCredential(params, tab); @@ -295,6 +310,17 @@ describe("FidoAuthenticatorService", () => { await rejects.toBeInstanceOf(DOMException); }); + it("should fallback if origin hostname is found in neverDomains", async () => { + const params = createParams({ + origin: "https://bitwarden.com", + }); + stateService.getNeverDomains.mockResolvedValue({ "bitwarden.com": null }); + + const result = async () => await client.assertCredential(params, tab); + + await expect(result).rejects.toThrow(FallbackRequestedError); + }); + it("should throw error if origin is not an http domain", async () => { const params = createParams({ origin: "http://passwordless.dev", diff --git a/libs/common/src/vault/services/fido2/fido2-client.service.ts b/libs/common/src/vault/services/fido2/fido2-client.service.ts index 0d113d5d45..76f51c1f71 100644 --- a/libs/common/src/vault/services/fido2/fido2-client.service.ts +++ b/libs/common/src/vault/services/fido2/fido2-client.service.ts @@ -5,6 +5,7 @@ import { AuthenticationStatus } from "../../../auth/enums/authentication-status" import { FeatureFlag } from "../../../enums/feature-flag.enum"; import { ConfigServiceAbstraction } from "../../../platform/abstractions/config/config.service.abstraction"; import { LogService } from "../../../platform/abstractions/log.service"; +import { StateService } from "../../../platform/abstractions/state.service"; import { Utils } from "../../../platform/misc/utils"; import { Fido2AuthenticatorError, @@ -40,6 +41,7 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction { private authenticator: Fido2AuthenticatorService, private configService: ConfigServiceAbstraction, private authService: AuthService, + private stateService: StateService, private logService?: LogService ) {} @@ -84,6 +86,12 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction { const parsedOrigin = parse(params.origin, { allowPrivateDomains: true }); params.rp.id = params.rp.id ?? parsedOrigin.hostname; + const neverDomains = await this.stateService.getNeverDomains(); + if (neverDomains != null && parsedOrigin.hostname in neverDomains) { + this.logService?.warning(`[Fido2Client] Excluded domain`); + throw new FallbackRequestedError(); + } + if (parsedOrigin.hostname == undefined || !params.origin.startsWith("https://")) { this.logService?.warning(`[Fido2Client] Invalid https origin: ${params.origin}`); throw new DOMException("'origin' is not a valid https origin", "SecurityError"); @@ -211,15 +219,15 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction { throw new FallbackRequestedError(); } - const { domain: effectiveDomain } = parse(params.origin, { allowPrivateDomains: true }); - if (effectiveDomain == undefined) { - this.logService?.warning(`[Fido2Client] Invalid origin: ${params.origin}`); - throw new DOMException("'origin' is not a valid domain", "SecurityError"); - } - const parsedOrigin = parse(params.origin, { allowPrivateDomains: true }); params.rpId = params.rpId ?? parsedOrigin.hostname; + const neverDomains = await this.stateService.getNeverDomains(); + if (neverDomains != null && parsedOrigin.hostname in neverDomains) { + this.logService?.warning(`[Fido2Client] Excluded domain`); + throw new FallbackRequestedError(); + } + if (parsedOrigin.hostname == undefined || !params.origin.startsWith("https://")) { this.logService?.warning(`[Fido2Client] Invalid https origin: ${params.origin}`); throw new DOMException("'origin' is not a valid https origin", "SecurityError");