From dce5c0f184ab64df4715cff1c16b8352b6afa617 Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Tue, 25 Jun 2024 11:06:04 +0200 Subject: [PATCH] [PM-6413] Add http loophole for localhost (#9236) * [PM-6413] feat: add http loophole for localhost Fixes #6882 * feat: add sanity check * feat: change fido2 filters to allow scripts on localhost * [PM-6413] fix: injection tests --- .../fido2/background/fido2.background.spec.ts | 2 +- .../fido2/background/fido2.background.ts | 2 +- .../fido2/content/fido2-content-script.ts | 4 +- .../fido2/content/fido2-page-script.ts | 4 +- ...do2-page-script.webauthn-supported.spec.ts | 9 +-- .../fido2/fido2-client.service.spec.ts | 58 +++++++++++++++++++ .../services/fido2/fido2-client.service.ts | 10 +++- 7 files changed, 79 insertions(+), 10 deletions(-) diff --git a/apps/browser/src/autofill/fido2/background/fido2.background.spec.ts b/apps/browser/src/autofill/fido2/background/fido2.background.spec.ts index f0c5fc5695..43d07bdea1 100644 --- a/apps/browser/src/autofill/fido2/background/fido2.background.spec.ts +++ b/apps/browser/src/autofill/fido2/background/fido2.background.spec.ts @@ -32,7 +32,7 @@ const contentScriptDetails = { ...sharedScriptInjectionDetails, }; const sharedRegistrationOptions = { - matches: ["https://*/*"], + matches: ["https://*/*", "http://localhost/*"], excludeMatches: ["https://*/*.xml*"], allFrames: true, ...sharedExecuteScriptOptions, diff --git a/apps/browser/src/autofill/fido2/background/fido2.background.ts b/apps/browser/src/autofill/fido2/background/fido2.background.ts index a58d32da16..0524a2529f 100644 --- a/apps/browser/src/autofill/fido2/background/fido2.background.ts +++ b/apps/browser/src/autofill/fido2/background/fido2.background.ts @@ -33,7 +33,7 @@ export class Fido2Background implements Fido2BackgroundInterface { runAt: "document_start", }; private readonly sharedRegistrationOptions: SharedFido2ScriptRegistrationOptions = { - matches: ["https://*/*"], + matches: ["https://*/*", "http://localhost/*"], excludeMatches: ["https://*/*.xml*"], allFrames: true, ...this.sharedInjectionDetails, diff --git a/apps/browser/src/autofill/fido2/content/fido2-content-script.ts b/apps/browser/src/autofill/fido2/content/fido2-content-script.ts index ad9f526f6c..171aa7cd83 100644 --- a/apps/browser/src/autofill/fido2/content/fido2-content-script.ts +++ b/apps/browser/src/autofill/fido2/content/fido2-content-script.ts @@ -17,7 +17,9 @@ import { MessageWithMetadata, Messenger } from "./messaging/messenger"; (function (globalContext) { const shouldExecuteContentScript = globalContext.document.contentType === "text/html" && - globalContext.document.location.protocol === "https:"; + (globalContext.document.location.protocol === "https:" || + (globalContext.document.location.protocol === "http:" && + globalContext.document.location.hostname === "localhost")); if (!shouldExecuteContentScript) { return; diff --git a/apps/browser/src/autofill/fido2/content/fido2-page-script.ts b/apps/browser/src/autofill/fido2/content/fido2-page-script.ts index cf14471359..5f91e6c081 100644 --- a/apps/browser/src/autofill/fido2/content/fido2-page-script.ts +++ b/apps/browser/src/autofill/fido2/content/fido2-page-script.ts @@ -8,7 +8,9 @@ import { Messenger } from "./messaging/messenger"; (function (globalContext) { const shouldExecuteContentScript = globalContext.document.contentType === "text/html" && - globalContext.document.location.protocol === "https:"; + (globalContext.document.location.protocol === "https:" || + (globalContext.document.location.protocol === "http:" && + globalContext.document.location.hostname === "localhost")); if (!shouldExecuteContentScript) { return; diff --git a/apps/browser/src/autofill/fido2/content/fido2-page-script.webauthn-supported.spec.ts b/apps/browser/src/autofill/fido2/content/fido2-page-script.webauthn-supported.spec.ts index 46ef4d60d6..292d0e0118 100644 --- a/apps/browser/src/autofill/fido2/content/fido2-page-script.webauthn-supported.spec.ts +++ b/apps/browser/src/autofill/fido2/content/fido2-page-script.webauthn-supported.spec.ts @@ -16,8 +16,9 @@ const mockGlobalThisDocument = { contentType: "text/html", location: { ...originalGlobalThis.document.location, - href: "https://localhost", - origin: "https://localhost", + href: "https://bitwarden.com", + origin: "https://bitwarden.com", + hostname: "bitwarden.com", protocol: "https:", }, }; @@ -166,8 +167,8 @@ describe("Fido2 page script with native WebAuthn support", () => { ...mockGlobalThisDocument, location: { ...mockGlobalThisDocument.location, - href: "http://localhost", - origin: "http://localhost", + href: "http://bitwarden.com", + origin: "http://bitwarden.com", protocol: "http:", }, })); diff --git a/libs/common/src/platform/services/fido2/fido2-client.service.spec.ts b/libs/common/src/platform/services/fido2/fido2-client.service.spec.ts index 13e1d8f282..597f2d8f32 100644 --- a/libs/common/src/platform/services/fido2/fido2-client.service.spec.ts +++ b/libs/common/src/platform/services/fido2/fido2-client.service.spec.ts @@ -19,6 +19,7 @@ import { } from "../../abstractions/fido2/fido2-client.service.abstraction"; import { Utils } from "../../misc/utils"; +import * as DomainUtils from "./domain-utils"; import { Fido2AuthenticatorService } from "./fido2-authenticator.service"; import { Fido2ClientService } from "./fido2-client.service"; import { Fido2Utils } from "./fido2-utils"; @@ -36,6 +37,7 @@ describe("FidoAuthenticatorService", () => { let domainSettingsService: MockProxy; let client!: Fido2ClientService; let tab!: chrome.tabs.Tab; + let isValidRpId!: jest.SpyInstance; beforeEach(async () => { authenticator = mock(); @@ -44,6 +46,8 @@ describe("FidoAuthenticatorService", () => { vaultSettingsService = mock(); domainSettingsService = mock(); + isValidRpId = jest.spyOn(DomainUtils, "isValidRpId"); + client = new Fido2ClientService( authenticator, configService, @@ -58,6 +62,10 @@ describe("FidoAuthenticatorService", () => { tab = { id: 123, windowId: 456 } as chrome.tabs.Tab; }); + afterEach(() => { + isValidRpId.mockRestore(); + }); + describe("createCredential", () => { describe("input parameters validation", () => { // Spec: If sameOriginWithAncestors is false, return a "NotAllowedError" DOMException. @@ -113,6 +121,7 @@ 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. + // This is actually checked by `isValidRpId` function, but we'll test it here as well it("should throw error if rp.id is not valid for this origin", async () => { const params = createParams({ origin: "https://passwordless.dev", @@ -126,6 +135,20 @@ describe("FidoAuthenticatorService", () => { await rejects.toBeInstanceOf(DOMException); }); + // Sanity check to make sure that we use `isValidRpId` to validate the rp.id + it("should throw if isValidRpId returns false", async () => { + const params = createParams(); + authenticator.makeCredential.mockResolvedValue(createAuthenticatorMakeResult()); + // `params` actually has a valid rp.id, but we're mocking the function to return false + isValidRpId.mockReturnValue(false); + + const result = async () => await client.createCredential(params, tab); + + const rejects = expect(result).rejects; + await rejects.toMatchObject({ name: "SecurityError" }); + await rejects.toBeInstanceOf(DOMException); + }); + it("should fallback if origin hostname is found in neverDomains", async () => { const params = createParams({ origin: "https://bitwarden.com", @@ -151,6 +174,16 @@ describe("FidoAuthenticatorService", () => { await rejects.toBeInstanceOf(DOMException); }); + it("should not throw error if localhost is http", async () => { + const params = createParams({ + origin: "http://localhost", + rp: { id: undefined, name: "localhost" }, + }); + authenticator.makeCredential.mockResolvedValue(createAuthenticatorMakeResult()); + + await client.createCredential(params, tab); + }); + // Spec: If credTypesAndPubKeyAlgs is empty, return a DOMException whose name is "NotSupportedError", and terminate this algorithm. it("should throw error if no support key algorithms were found", async () => { const params = createParams({ @@ -360,6 +393,7 @@ 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. + // This is actually checked by `isValidRpId` function, but we'll test it here as well it("should throw error if rp.id is not valid for this origin", async () => { const params = createParams({ origin: "https://passwordless.dev", @@ -373,6 +407,20 @@ describe("FidoAuthenticatorService", () => { await rejects.toBeInstanceOf(DOMException); }); + // Sanity check to make sure that we use `isValidRpId` to validate the rp.id + it("should throw if isValidRpId returns false", async () => { + const params = createParams(); + authenticator.getAssertion.mockResolvedValue(createAuthenticatorAssertResult()); + // `params` actually has a valid rp.id, but we're mocking the function to return false + isValidRpId.mockReturnValue(false); + + const result = async () => await client.assertCredential(params, tab); + + const rejects = expect(result).rejects; + await rejects.toMatchObject({ name: "SecurityError" }); + await rejects.toBeInstanceOf(DOMException); + }); + it("should fallback if origin hostname is found in neverDomains", async () => { const params = createParams({ origin: "https://bitwarden.com", @@ -506,6 +554,16 @@ describe("FidoAuthenticatorService", () => { expect.anything(), ); }); + + it("should not throw error if localhost is http", async () => { + const params = createParams({ + origin: "http://localhost", + }); + params.rpId = undefined; + authenticator.getAssertion.mockResolvedValue(createAuthenticatorAssertResult()); + + await client.assertCredential(params, tab); + }); }); describe("assert discoverable credential", () => { diff --git a/libs/common/src/platform/services/fido2/fido2-client.service.ts b/libs/common/src/platform/services/fido2/fido2-client.service.ts index 5b6a13bc3f..d22b91fda0 100644 --- a/libs/common/src/platform/services/fido2/fido2-client.service.ts +++ b/libs/common/src/platform/services/fido2/fido2-client.service.ts @@ -103,7 +103,10 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction { } params.rp.id = params.rp.id ?? parsedOrigin.hostname; - if (parsedOrigin.hostname == undefined || !params.origin.startsWith("https://")) { + if ( + parsedOrigin.hostname == undefined || + (!params.origin.startsWith("https://") && parsedOrigin.hostname !== "localhost") + ) { this.logService?.warning(`[Fido2Client] Invalid https origin: ${params.origin}`); throw new DOMException("'origin' is not a valid https origin", "SecurityError"); } @@ -238,7 +241,10 @@ export class Fido2ClientService implements Fido2ClientServiceAbstraction { params.rpId = params.rpId ?? parsedOrigin.hostname; - if (parsedOrigin.hostname == undefined || !params.origin.startsWith("https://")) { + if ( + parsedOrigin.hostname == undefined || + (!params.origin.startsWith("https://") && parsedOrigin.hostname !== "localhost") + ) { this.logService?.warning(`[Fido2Client] Invalid https origin: ${params.origin}`); throw new DOMException("'origin' is not a valid https origin", "SecurityError"); }