diff --git a/packages/functional-tests/tests/pairing/codeVerifierIsolation.spec.ts b/packages/functional-tests/tests/pairing/codeVerifierIsolation.spec.ts new file mode 100644 index 00000000000..9b0696f72d0 --- /dev/null +++ b/packages/functional-tests/tests/pairing/codeVerifierIsolation.spec.ts @@ -0,0 +1,247 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +/** + * Asserts the property the pairing security review depends on: the PKCE + * code_verifier Firefox generates never reaches web content. + * + * This runs against a real Firefox over Marionette, so it exercises the actual + * FxAccountsOAuth and WebChannel boundary rather than a mock. Two claims: + * + * 1. The return value of beginOAuthFlow(), which the WebChannel forwards to + * the page verbatim, carries the challenge but not the verifier. + * 2. Script running as the page, which is an XSS payload's privilege level, + * cannot obtain the verifier by sending WebChannel messages. + * + * The verifier is read in chrome context and every comparison happens here in + * Node. It is never interpolated into content-context script, which would put + * the secret into the page and invert what the test proves. + */ + +import { createHash } from 'crypto'; +import { test, expect } from '../../lib/fixtures/pairing'; +import { TIMEOUTS } from '../../lib/pairing-constants'; +import { MarionetteClient } from '../../lib/marionette'; + +const SCOPES = ['profile', 'https://identity.mozilla.com/apps/oldsync']; + +// Firefox builds the verifier from 32 random bytes in base64url, so 43 chars. +const VERIFIER_PATTERN = /^[A-Za-z0-9-_]{43}$/; + +// How long the probe collects WebChannel replies before it resolves. +const PROBE_WINDOW_MS = 3000; + +// The commands an injected script could reach for. fxa_status is included +// because it is the one command that does hand real credentials to the page, +// so it is the most plausible place for a verifier to be added by mistake. +const PROBE_COMMANDS = [ + 'fxaccounts:oauth_flow_begin', + 'fxaccounts:oauth_flow_is_active', + 'fxaccounts:fxa_status', + 'fxaccounts:pair_supplicant_metadata', + 'fxaccounts:pair_heartbeat', +]; + +type OAuthFlow = { + /** What the WebChannel forwards to the page verbatim. */ + params: Record; + /** + * Read in chrome rather than derived from `params` here: JSON.stringify drops + * keys whose value is undefined, so the chrome-side list is the stricter one. + */ + paramKeys: string[]; + storedVerifier: string; +}; + +type FlowResult = + | ({ success: true } & OAuthFlow) + | { success: false; error: string }; + +/** + * Start a real OAuth flow in the parent process and return both the params + * Firefox would hand to content and the verifier it kept for itself. + */ +async function beginRealOAuthFlow( + client: MarionetteClient +): Promise { + await client.setContext('chrome'); + const raw = await client.executeAsyncScript( + ` + const [resolve] = arguments; + (async () => { + try { + const { getFxAccountsSingleton } = ChromeUtils.importESModule( + "resource://gre/modules/FxAccounts.sys.mjs" + ); + const fxAccounts = getFxAccountsSingleton(); + const params = await fxAccounts._internal.beginOAuthFlow( + ${JSON.stringify(SCOPES)} + ); + const flow = fxAccounts._internal.oauth.getFlow(params.state); + resolve(JSON.stringify({ + success: true, + params, + paramKeys: Object.keys(params), + storedVerifier: flow.verifier, + })); + } catch (e) { + resolve(JSON.stringify({ success: false, error: e.message })); + } + })(); + `, + { sandbox: 'system', timeoutMs: TIMEOUTS.ASYNC_SCRIPT } + ); + + if (typeof raw !== 'string') { + throw new Error(`Expected a string from beginOAuthFlow, got ${typeof raw}`); + } + const result = JSON.parse(raw) as FlowResult; + if (!result.success) { + throw new Error(`beginOAuthFlow failed: ${result.error}`); + } + return result; +} + +function pkceChallengeFor(verifier: string): string { + return createHash('sha256').update(verifier).digest('base64url'); +} + +/** The probe resolves with a JSON array of WebChannel envelope strings. */ +function parseProbeReplies(raw: unknown): string[] { + if (typeof raw !== 'string') { + throw new Error(`Expected a string from the probe, got ${typeof raw}`); + } + return JSON.parse(raw) as string[]; +} + +/** + * Read the verifier Firefox stored for one specific flow. + * + * Every beginOAuthFlow() call mints a fresh flow with a fresh verifier, so a + * reply can only be checked against the verifier belonging to its own state. + * Comparing against some earlier flow's verifier can never match and would make + * the assertion unfalsifiable. + */ +async function readStoredVerifier( + client: MarionetteClient, + state: string +): Promise { + await client.setContext('chrome'); + const raw = await client.executeScript( + ` + const { getFxAccountsSingleton } = ChromeUtils.importESModule( + "resource://gre/modules/FxAccounts.sys.mjs" + ); + const flow = getFxAccountsSingleton()._internal.oauth.getFlow(arguments[0]); + return flow ? flow.verifier : null; + `, + { sandbox: 'system', args: [state] } + ); + if (typeof raw !== 'string') { + throw new Error(`No stored flow found for state ${state}`); + } + return raw; +} + +test.describe('PKCE code_verifier isolation', () => { + // One test, so one Firefox launch: the fixture is test-scoped, and the two + // boundaries below check the same object. A WebChannel reply carries what + // beginOAuthFlow() returned, so asserting both here costs nothing extra. + test('Firefox keeps the code_verifier out of both the params it hands to content and every WebChannel reply', async ({ + marionetteAuthority, + target, + }) => { + const client = marionetteAuthority.client; + + const { + params, + paramKeys, + storedVerifier: directVerifier, + } = await beginRealOAuthFlow(client); + + // Positive controls. Without these the test would also pass if Firefox + // stopped using PKCE altogether, or handed back an empty verifier. + expect(directVerifier).toMatch(VERIFIER_PATTERN); + expect(params.code_challenge_method).toBe('S256'); + expect(params.code_challenge).toBe(pkceChallengeFor(directVerifier)); + + // Absent by key and by value from what beginOAuthFlow() hands back. + expect(paramKeys).not.toContain('code_verifier'); + expect(paramKeys).not.toContain('verifier'); + expect(JSON.stringify(params)).not.toContain(directVerifier); + + // Load a page on the FxA origin so the WebChannel is in scope, then act + // as injected script would: send each command and collect every reply. + await client.setContext('content'); + await client.navigate(target.contentServerUrl); + + const raw = await client.executeAsyncScript( + ` + const [resolve] = arguments; + const commands = ${JSON.stringify(PROBE_COMMANDS)}; + const scopes = ${JSON.stringify(SCOPES)}; + const seen = []; + function record(event) { + seen.push( + typeof event.detail === 'string' + ? event.detail + : JSON.stringify(event.detail) + ); + } + window.addEventListener('WebChannelMessageToContent', record, true); + commands.forEach(function (command, i) { + window.dispatchEvent( + new CustomEvent('WebChannelMessageToChrome', { + detail: JSON.stringify({ + id: 'account_updates', + message: { + command: command, + data: { scopes: scopes }, + messageId: 'verifier-probe-' + i, + }, + }), + }) + ); + }); + setTimeout(function () { + window.removeEventListener('WebChannelMessageToContent', record, true); + resolve(JSON.stringify(seen)); + }, ${PROBE_WINDOW_MS}); + `, + { timeoutMs: TIMEOUTS.ASYNC_SCRIPT } + ); + + const envelopes = parseProbeReplies(raw); + const replies = envelopes.map((r) => JSON.parse(r).message); + + // Positive control, and the source of the secret to compare against. A + // reply count alone is too weak: the page sends its own fxa_status, so a + // count could be satisfied without oauth_flow_begin ever being reached. + const flowBegin = replies.find( + (m) => + m.command === 'fxaccounts:oauth_flow_begin' && + m.messageId === 'verifier-probe-0' + ); + expect(flowBegin).toBeDefined(); + expect(flowBegin.data.code_challenge_method).toBe('S256'); + + // The verifier for the flow this very reply created. + const storedVerifier = await readStoredVerifier( + client, + flowBegin.data.state + ); + expect(storedVerifier).toMatch(VERIFIER_PATTERN); + expect(flowBegin.data.code_challenge).toBe( + pkceChallengeFor(storedVerifier) + ); + + // The property under test, by key and by value, across every reply the + // page could observe. Scan the whole envelope, not just `message`: a field + // beside `message` is just as visible to page script. + expect(Object.keys(flowBegin.data)).not.toContain('code_verifier'); + for (const envelope of envelopes) { + expect(envelope).not.toContain(storedVerifier); + } + }); +}); diff --git a/packages/fxa-auth-server/lib/routes/oauth/token.spec.ts b/packages/fxa-auth-server/lib/routes/oauth/token.spec.ts index 42ba51d3cd9..530b922a298 100644 --- a/packages/fxa-auth-server/lib/routes/oauth/token.spec.ts +++ b/packages/fxa-auth-server/lib/routes/oauth/token.spec.ts @@ -5,7 +5,7 @@ const crypto = require('crypto'); const Joi = require('joi'); const { Container } = require('typedi'); -const { AppError: AuthError } = require('@fxa/accounts/errors'); +const { AppError: AuthError, OAUTH_ERRNO } = require('@fxa/accounts/errors'); const ScopeSet = require('fxa-shared').oauth.scopes; const { @@ -162,6 +162,16 @@ function joiNotAllowed(err: any, param: string) { expect(err.details[0].message).toBe(`"${param}" is not allowed`); } +// The /oauth/token payload is a Joi.alternatives(), so a rejection always +// surfaces the generic "does not match any of the allowed types" at the top +// level. authorization_code is the first alternative; its own failure reason is +// nested, and that is what these tests need to assert on. +function authorizationCodeAlternativeError(err: any) { + expect(err.isJoi).toBe(true); + expect(err.details[0].type).toBe('alternatives.match'); + return err.details[0].context.details[0]; +} + function resetAndMockDeps() { jest.resetModules(); for (const [key, value] of Object.entries(tokenRoutesDepMocks)) { @@ -776,13 +786,13 @@ describe('token exchange grant_type', () => { }); describe('/oauth/token POST', () => { - describe('exclude_dau input validation', () => { - // tokenRoutes[1] is the POST /oauth/token route (tokenRoutes[0] is /token). - function v(req: any) { - const oauthTokenRoute = tokenRoutes[1]; - return oauthTokenRoute.config.validate.payload.validate(req); - } + // tokenRoutes[1] is the POST /oauth/token route (tokenRoutes[0] is /token). + function v(req: any) { + const oauthTokenRoute = tokenRoutes[1]; + return oauthTokenRoute.config.validate.payload.validate(req); + } + describe('exclude_dau input validation', () => { it('accepts exclude_dau=true for the authorization_code grant', () => { const res = v({ client_id: CLIENT_ID, @@ -815,6 +825,51 @@ describe('/oauth/token POST', () => { }); }); + // Firefox redeems a pairing/sign-in code here, sending + // {grant_type, code, client_id, code_verifier} (FxAccountsClient.oauthToken). + // It holds the verifier in the parent process and never gives it to the web + // page, so the xor on this alternative is what makes a redemption without a + // known verifier fail: a public client cannot substitute a client_secret. + describe('authorization_code grant requires exactly one client credential', () => { + it('accepts a code_verifier alone, the shape Firefox sends', () => { + const res = v({ + client_id: CLIENT_ID, + grant_type: 'authorization_code', + code: CODE, + code_verifier: PKCE_CODE_VERIFIER, + }); + expect(res.error).toBeUndefined(); + expect(res.value.code_verifier).toBe(PKCE_CODE_VERIFIER); + }); + + it('rejects a code redemption carrying neither code_verifier nor client_secret', () => { + const res = v({ + client_id: CLIENT_ID, + grant_type: 'authorization_code', + code: CODE, + }); + const detail = authorizationCodeAlternativeError(res.error); + expect(detail.type).toBe('object.missing'); + expect(detail.context.peers).toEqual(['client_secret', 'code_verifier']); + }); + + it('rejects a code redemption carrying both code_verifier and client_secret', () => { + const res = v({ + client_id: CLIENT_ID, + grant_type: 'authorization_code', + code: CODE, + code_verifier: PKCE_CODE_VERIFIER, + client_secret: CLIENT_SECRET, + }); + const detail = authorizationCodeAlternativeError(res.error); + expect(detail.type).toBe('object.xor'); + expect(detail.context.present).toEqual([ + 'client_secret', + 'code_verifier', + ]); + }); + }); + describe('Glean metrics', () => { it('fires the token created event with exclude_dau false by default', async () => { const request = { @@ -1760,3 +1815,98 @@ describe('exclude_dau carried on the authorization code', () => { }); }); }); + +// The handler is the last thing between a code and a token, so the gate is +// covered by driving it directly. Two of its branches are left out on purpose: +// a stored method other than S256, and a verifier for a challenge-less code. +// Neither can arrive over HTTP, because /authorization pins the method to S256 +// and writes a challenge if and only if the client is public. +describe('PKCE gate on the authorization_code grant', () => { + // Matches the route's pkceHash(): sha256 of the verifier, URL-safe base64, + // unpadded. oauth/util's base64URLEncode is a Buffer.toString('base64url'). + const CODE_CHALLENGE = crypto + .createHash('sha256') + .update(PKCE_CODE_VERIFIER) + .digest('base64url'); + + const CHALLENGED_CODE = { + codeChallenge: CODE_CHALLENGE, + codeChallengeMethod: 'S256', + }; + + async function redeem( + codeOverrides: Record, + payloadOverrides: Record = {} + ) { + resetAndMockDeps(); + // The shared oauth/util stub only carries makeAssertionJWT, but computing a + // pkceHash needs the real base64URLEncode. + jest.doMock('../../oauth/util', () => ({ + ...jest.requireActual('../../oauth/util'), + makeAssertionJWT: async () => ({}), + })); + // The code row carries a real ScopeSet, so match what production's + // generateTokens emits or the route throws while building the response. + jest.doMock('../../oauth/grant', () => ({ + ...tokenRoutesDepMocks['../../oauth/grant'], + generateTokens: (grant: any) => ({ + ...grant, + scope: grant.scope.toString(), + }), + })); + // The gate throws before the code is consumed, so removeCode having been + // called is the observable signal that a redemption cleared it. + const removeCode = jest.fn().mockResolvedValue(null); + const routes = require('./token')({ + ...tokenRoutesArgMocks, + oauthDB: { + ...tokenRoutesArgMocks.oauthDB, + removeCode, + async getCode() { + return { + userId: buf(UID), + clientId: buf(CLIENT_ID), + createdAt: Date.now(), + scope: ScopeSet.fromArray(['profile']), + ...codeOverrides, + }; + }, + }, + }); + await routes[1].handler({ + app: {}, + auth: { credentials: undefined }, + headers: {}, + payload: { + client_id: CLIENT_ID, + grant_type: 'authorization_code', + code: CODE, + ...payloadOverrides, + }, + emitMetricsEvent: () => {}, + }); + return { removeCode }; + } + + it('consumes the code when the verifier matches the stored challenge', async () => { + const { removeCode } = await redeem(CHALLENGED_CODE, { + code_verifier: PKCE_CODE_VERIFIER, + }); + + expect(removeCode).toHaveBeenCalledTimes(1); + }); + + it('rejects a redemption with no verifier when the code carries a challenge', async () => { + await expect(redeem(CHALLENGED_CODE)).rejects.toMatchObject({ + errno: OAUTH_ERRNO.MISSING_PKCE_PARAMETERS, + }); + }); + + it('rejects a verifier that does not hash to the stored challenge', async () => { + await expect( + redeem(CHALLENGED_CODE, { + code_verifier: 'w'.repeat(PKCE_CODE_VERIFIER.length), + }) + ).rejects.toMatchObject({ errno: OAUTH_ERRNO.INCORRECT_CODE_CHALLENGE }); + }); +}); diff --git a/packages/fxa-auth-server/test/remote/oauth_tests.in.spec.ts b/packages/fxa-auth-server/test/remote/oauth_tests.in.spec.ts index af0ad7b0785..e7a2c7678b2 100644 --- a/packages/fxa-auth-server/test/remote/oauth_tests.in.spec.ts +++ b/packages/fxa-auth-server/test/remote/oauth_tests.in.spec.ts @@ -186,6 +186,70 @@ describe.each(testVersions)( expect(devices.length).toBe(1); }); + // Same flow as above, minus the verifier. Firefox holds the verifier in the + // parent process and never hands it to the web content, so this is the shape + // a caller without it is stuck with: a public client cannot fall back to a + // client_secret, so the payload fails the /oauth/token xor and the code is + // never redeemed. + // + // grant_type is sent explicitly, as Firefox does. Omit it and the payload no + // longer matches the authorization_code alternative, so it falls through to + // fxa-credentials (which defaults its own grant_type) and fails later with a + // confusing 401 instead of this 400. + it('refuses to redeem an authorization code without the code_verifier', async () => { + const res = await client.createAuthorizationCode({ + client_id: PUBLIC_CLIENT_ID, + state: 'abc', + code_challenge: MOCK_CODE_CHALLENGE, + code_challenge_method: 'S256', + scope: OAUTH_SCOPE_OLD_SYNC, + access_type: 'offline', + }); + expect(res.code).toBeTruthy(); + + try { + await client.grantOAuthTokens({ + client_id: PUBLIC_CLIENT_ID, + grant_type: 'authorization_code', + code: res.code, + }); + throw new Error('should have thrown'); + } catch (err: any) { + expect(err.errno).toBe(error.ERRNO.INVALID_PARAMETER); + expect(err.code).toBe(400); + expect(err.validation.source).toBe('payload'); + } + }); + + // token.spec covers this branch against a mocked code row. Here the stored + // challenge is the one /authorization wrote, so the S256 comparison runs on + // real data. + it('refuses to redeem an authorization code with an incorrect code_verifier', async () => { + const res = await client.createAuthorizationCode({ + client_id: PUBLIC_CLIENT_ID, + state: 'abc', + code_challenge: MOCK_CODE_CHALLENGE, + code_challenge_method: 'S256', + scope: OAUTH_SCOPE_OLD_SYNC, + access_type: 'offline', + }); + expect(res.code).toBeTruthy(); + + try { + await client.grantOAuthTokens({ + client_id: PUBLIC_CLIENT_ID, + code: res.code, + // Valid per RFC 7636, wrong value, so the payload passes validation + // and the PKCE gate is what rejects it. + code_verifier: 'b'.repeat(43), + }); + throw new Error('should have thrown'); + } catch (err: any) { + expect(err.errno).toBe(error.ERRNO.INVALID_PKCE_CHALLENGE); + expect(err.code).toBe(400); + } + }); + describe('exclude_dau', () => { // The exclude_dau tagging itself is asserted at the unit layer (token.spec // via mockGlean); here we confirm the option is accepted end-to-end and does diff --git a/packages/fxa-settings/src/lib/channels/firefox.test.ts b/packages/fxa-settings/src/lib/channels/firefox.test.ts index 56b01789978..de363646680 100644 --- a/packages/fxa-settings/src/lib/channels/firefox.test.ts +++ b/packages/fxa-settings/src/lib/channels/firefox.test.ts @@ -2,7 +2,12 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -import { firefox, FirefoxCommand } from './firefox'; +import { + firefox, + FirefoxCommand, + buildSyncOAuthSearch, + FxAOAuthFlowBeginResponse, +} from './firefox'; describe('Firefox pairing WebChannel methods', () => { let sendSpy: jest.SpyInstance; @@ -66,3 +71,51 @@ describe('Firefox pairing WebChannel methods', () => { expect(result).toEqual({ ua: 'Mozilla/5.0', city: 'Portland' }); }); }); + +describe('buildSyncOAuthSearch', () => { + const MOCK_CODE_VERIFIER = 'au3dqDz2dOB0_vSikXCUf4S8Gc-37dL-F7sGxtxpR3R'; + + // Mirrors a real fxa_oauth_flow_begin response. Firefox derives the challenge + // from a verifier it keeps in the parent process, so a verifier is never part + // of this payload. + const OAUTH_PARAMS: FxAOAuthFlowBeginResponse = { + action: 'email', + response_type: 'code', + access_type: 'offline', + scope: 'profile https://identity.mozilla.com/apps/oldsync', + client_id: '5882386c6d801776', + state: 'PFYyaGZuNlZ4TGpQdw', + code_challenge: 'BVfwwa_Z33Jhs-GUd62k0d6NIBqXfEjT0dHMseOOtgo', + code_challenge_method: 'S256', + keys_jwk: 'eyJjcnYiOiJQLTI1NiIsImt0eSI6IkVDIn0', + }; + + it('forwards only the allowlisted OAuth params, so no code_verifier reaches /authorization', () => { + // The extra fields matter: with a payload of allowlisted keys only, this + // would still pass if the allowlist were replaced by a spread. The response + // type has no verifier field, but the payload crosses a trust boundary and + // TypeScript is erased, so a compromised Firefox could put one on the wire. + const search = buildSyncOAuthSearch({ + ...OAUTH_PARAMS, + code_verifier: MOCK_CODE_VERIFIER, + sessionToken: 'deadbeef', + unexpected: 'whatever', + } as FxAOAuthFlowBeginResponse); + + expect([...search.keys()].sort()).toEqual([ + 'access_type', + 'action', + 'client_id', + 'code_challenge', + 'code_challenge_method', + 'context', + 'keys_jwk', + 'response_type', + 'scope', + 'service', + 'state', + ]); + expect(search.get('code_challenge')).toBe(OAUTH_PARAMS.code_challenge); + expect(search.toString()).not.toContain(MOCK_CODE_VERIFIER); + }); +});