From 1ba0e92c4360b98df8e67ea593c841fcef98ed84 Mon Sep 17 00:00:00 2001 From: bitgobot Date: Mon, 5 Oct 2026 19:08:08 +0000 Subject: [PATCH] fix(sdk-api): route browser v1 decrypt straight to SJCL In browser bundles webpack maps `crypto` to `crypto-browserify`, whose AES-CCM (browserify-aes) does not work. Every v1 decrypt therefore paid for a full PBKDF2 run that was guaranteed to fail, and then logged a fallback warning on every successful decrypt. Route browser and worker runtimes straight to the frozen SJCL decoder, gated by the same parseV1Envelope iter-cap check the native path uses so DoS protection is preserved. Node keeps native-first with the SJCL fallback. Also drop the adjacent dead test seams: the Node-injected decryptV1.browser.ts suite (it injected crypto-browserify but never exercised real browser dispatch), the decryptV1WithCrypto injection parameter that existed only for it, and the now-unused crypto-browserify devDependency. Real browser dispatch is now proven by a Cypress component spec on BitGoAPI.decrypt covering the absent-`v` legacy envelope and the iter-cap boundary. docs/sjcl-replacement.md states the engine boundary precisely. Why: customers decrypting legacy v1 key material in browser bundles pay a wasted PBKDF2 and see a misleading console warning per call, and the SJCL deprecation effort needs an exact, documented boundary between the native and frozen-SJCL code paths. Ticket: WCN-2576 Session-Id: b906da45-6cf8-444a-9623-1e04551347ae Task-Id: f81a1b1b-b7ee-46e9-a522-7708ed6ccc64 Requested-By: Pranav Jain --- docs/sjcl-replacement.md | 33 ++++++ modules/sdk-api/package.json | 3 - modules/sdk-api/src/decryptV1.ts | 44 ++----- modules/sdk-api/src/encrypt.ts | 60 +++++----- .../sdk-api/test/unit/decryptV1.browser.ts | 108 ----------------- .../src/components/BitGoAPI/decrypt.spec.tsx | 109 ++++++++++++++++++ 6 files changed, 180 insertions(+), 177 deletions(-) create mode 100644 docs/sjcl-replacement.md delete mode 100644 modules/sdk-api/test/unit/decryptV1.browser.ts create mode 100644 modules/web-demo/src/components/BitGoAPI/decrypt.spec.tsx diff --git a/docs/sjcl-replacement.md b/docs/sjcl-replacement.md new file mode 100644 index 0000000000..0ab3b7c6a1 --- /dev/null +++ b/docs/sjcl-replacement.md @@ -0,0 +1,33 @@ +# SJCL Replacement — v1 Decrypt Boundary + +Part of the SJCL deprecation effort ([WCN-43](https://linear.app/bitgo/issue/WCN-43/big-23-003-wp1-unmaintained-cryptographic-library), [WCN-1067](https://linear.app/bitgo/issue/WCN-1067/big-26-004-deprecated-cryptographic-library-utilized-sjcl)). This document states precisely where each crypto engine runs for local encrypt/decrypt, and why the browser v1 path intentionally still uses SJCL. + +## Current boundary + +| Envelope | Node runtime | Browser / worker runtime | +| ------------------------------------- | ----------------------------------------------------------------------------- | ---------------------------------------------------------- | +| v1 (SJCL PBKDF2-SHA256 + AES-256-CCM) | Native `node:crypto` AES-CCM (`decryptV1`), with SJCL as a rollout safety net | Frozen `@bitgo/sjcl` decoder, straight — no native attempt | +| v2 (Argon2id + AES-256-GCM) | Argon2id + WebCrypto AES-GCM (`encryptV2`/`decryptV2`) | Argon2id + WebCrypto AES-GCM — identical code | + +- **Node v1**: `decryptV1WithFallback` tries `decryptV1` (native `node:crypto` `aes-*-ccm`) first. Any native failure falls through to `sjcl.decrypt` so callers are never blocked during rollout; only an iter-cap violation is rethrown. A `console.warn` fires only when the two engines disagree (native fails, SJCL succeeds). +- **Browser/worker v1**: `isBrowserRuntime()` (true when `window` or `self` exists) routes `decryptV1WithFallback` straight to `sjcl.decrypt`. Before handing over, it runs the same `parseV1Envelope` envelope check the native path uses, rethrowing only iter-cap violations to preserve DoS protection; any other shape rejection (e.g. a legacy envelope without a `v` field) is a form SJCL still accepts and is left to SJCL to decide. The native attempt is skipped entirely: webpack substitutes `crypto-browserify` (backed by `browserify-aes`) for `node:crypto` in browser bundles, and its AES-CCM does not work, so the attempt would only pay for a doomed PBKDF2 run before falling back. +- **v2 everywhere**: v2 is Argon2id + WebCrypto AES-GCM in both Node and browser bundles; it never touches SJCL. + +The same boundary is enforced by tests: Node behavior in `modules/sdk-api/test/unit/decryptV1.ts`, and real-browser dispatch (including the absent-`v` and iter-cap boundary cases) by the Cypress component spec `modules/web-demo/src/components/BitGoAPI/decrypt.spec.tsx`. + +## Why there is no WebCrypto CCM implementation for browser v1 + +An alternative was considered: replace the SJCL decoder with a hand-written WebCrypto composition. It was rejected: + +- WebCrypto has **no AES-CCM primitive**. A replacement would have to be hand-assembled from AES-CTR/CBC plus manual CCM formatting — i.e. brand-new, unreviewed cryptographic code. +- The threat model is local self-decrypt of the user's own envelope; there is no real security benefit to offset the risk of new crypto. +- It would not remove the `@bitgo/sjcl` dependency anyway: `encryptV1` and the `eddsaMPCv2` fallback still use SJCL (tracked as follow-up work). + +Instead, `@bitgo/sjcl` is kept as a frozen, decrypt-only shim for browser v1, scoped to the SJCL envelope format and iteration-capped via `V1_MAX_ITER` (`100_000`, 10x the 10,000 iterations BitGo-produced envelopes use). + +## Remaining SJCL surface (follow-ups) + +- `encryptV1` — v1 encryption is legacy; still calls `sjcl.encrypt`. +- `eddsaMPCv2` — retains an SJCL fallback path. + +These are intentionally out of scope of the browser v1 decrypt routing work. diff --git a/modules/sdk-api/package.json b/modules/sdk-api/package.json index de4448d724..d845c2d200 100644 --- a/modules/sdk-api/package.json +++ b/modules/sdk-api/package.json @@ -58,9 +58,6 @@ "secrets.js-grempe": "^1.1.0", "superagent": "^9.0.1" }, - "devDependencies": { - "crypto-browserify": "^3.12.0" - }, "overrides": { "degenerator": "5.0.0" }, diff --git a/modules/sdk-api/src/decryptV1.ts b/modules/sdk-api/src/decryptV1.ts index 5ea26d3ff6..24c3dc86b0 100644 --- a/modules/sdk-api/src/decryptV1.ts +++ b/modules/sdk-api/src/decryptV1.ts @@ -3,18 +3,6 @@ import { createDecipheriv, pbkdf2 } from 'crypto'; import * as t from 'io-ts'; import { promisify } from 'util'; -/** - * Minimal shape the decrypt path needs from a crypto module. Both `node:crypto` - * and `crypto-browserify` satisfy this. Passing this in from tests lets the - * browser-shim test suite exercise the real decrypt code instead of a copy. - */ -export interface CryptoModule { - pbkdf2: typeof pbkdf2; - createDecipheriv: typeof createDecipheriv; -} - -const defaultCrypto: CryptoModule = { pbkdf2, createDecipheriv }; - /** * Upper bound on PBKDF2 iterations accepted from a v1 envelope. BitGo-produced * v1 envelopes use 10,000; this cap is 10x that. Envelope validation enforces @@ -70,16 +58,15 @@ function ccmNonceLength(plaintextLen: number): number { } /** - * Decrypt a parsed v1 envelope given a crypto module. - * - * v1 = PBKDF2-SHA256(password, salt, iter, keyLen) then AES-CCM(key, nonce, ct||tag). - * Byte-for-byte compatible with `sjcl.decrypt` output for the same envelope. + * Decrypt a v1 (SJCL PBKDF2-SHA256 + AES-CCM) envelope with Node's native + * `node:crypto`. * - * Exported so tests can inject `crypto-browserify` and exercise the exact - * runtime path the webpack browser bundle produces, without duplicating the - * decrypt logic. + * Node runtimes only: the `crypto` webpack substitutes in browser bundles + * (`crypto-browserify`) has no working AES-CCM, so `decryptV1WithFallback` + * routes browser runtimes straight to the frozen SJCL decoder instead of + * ever calling this. */ -export async function decryptV1WithCrypto(password: string, ciphertext: string, crypto: CryptoModule): Promise { +export async function decryptV1(password: string, ciphertext: string): Promise { const env = parseV1Envelope(ciphertext); const salt = Buffer.from(env.salt, 'base64'); const ivFull = Buffer.from(env.iv, 'base64'); @@ -94,9 +81,9 @@ export async function decryptV1WithCrypto(password: string, ciphertext: string, const iv = ivFull.subarray(0, nonceLen); const keyBytes = env.ks / 8; - const key: Buffer = await promisify(crypto.pbkdf2)(password, salt, env.iter, keyBytes, 'sha256'); + const key: Buffer = await promisify(pbkdf2)(password, salt, env.iter, keyBytes, 'sha256'); - const decipher = crypto.createDecipheriv(`aes-${env.ks}-ccm`, key, iv, { authTagLength: tagBytes }); + const decipher = createDecipheriv(`aes-${env.ks}-ccm`, key, iv, { authTagLength: tagBytes }); decipher.setAuthTag(authTag); const aad = env.adata ? Buffer.from(env.adata, 'utf8') : Buffer.alloc(0); decipher.setAAD(aad, { plaintextLength: cipher.length }); @@ -104,16 +91,3 @@ export async function decryptV1WithCrypto(password: string, ciphertext: string, const pt = Buffer.concat([decipher.update(cipher), decipher.final()]); return pt.toString('utf8'); } - -/** - * Decrypt a v1 (SJCL PBKDF2-SHA256 + AES-CCM) envelope. - * - * Runs the same `node:crypto` code on server and browser. The BitGoJS webpack - * config already maps `crypto` -> `crypto-browserify`, whose `aes-256-ccm` and - * `pbkdf2` implementations are byte-compatible with Node's native ones and - * with SJCL's envelope format. Parity is guarded by tests in - * `test/unit/decryptV1.browser.ts`. - */ -export async function decryptV1(password: string, ciphertext: string): Promise { - return decryptV1WithCrypto(password, ciphertext, defaultCrypto); -} diff --git a/modules/sdk-api/src/encrypt.ts b/modules/sdk-api/src/encrypt.ts index 556adf3fd4..f635711b8f 100644 --- a/modules/sdk-api/src/encrypt.ts +++ b/modules/sdk-api/src/encrypt.ts @@ -1,34 +1,18 @@ import * as sjcl from '@bitgo/sjcl'; import { randomBytes } from 'crypto'; -import { decryptV1, V1_MAX_ITER } from './decryptV1'; +import { decryptV1, parseV1Envelope } from './decryptV1'; import { decryptV2, encryptV2 } from './encryptV2'; /** - * True when running in a bundled browser build. `browserify-aes` -- the AES - * polyfill webpack substitutes for `node:crypto` in browser bundles -- has no - * AES-CCM mode at all, so native v1 decrypt fails on literally every call - * there. There's no point paying for the doomed native attempt (a full - * PBKDF2 run) before falling back; go straight to SJCL. + * True in a browser main thread (`window`) or worker (`self`) runtime. The + * `crypto` webpack substitutes for `node:crypto` there (`crypto-browserify`, + * backed by `browserify-aes`) has no working AES-CCM, so a native v1 attempt + * would only pay for a doomed PBKDF2 run before falling back. Browser + * runtimes route v1 decrypt straight to SJCL instead. */ -const isBrowserRuntime = typeof window !== 'undefined'; - -/** - * SJCL has no upper bound on `iter`, so a hostile v1 envelope could burn CPU - * running an inflated PBKDF2. Native decrypt's codec normally catches this - * before any KDF work runs; the browser path skips native entirely, so it - * needs this narrow check to preserve the same DoS protection. - */ -function assertIterWithinCap(ciphertext: string): void { - let iter: unknown; - try { - iter = JSON.parse(ciphertext)?.iter; - } catch { - return; // malformed JSON -- let sjcl.decrypt raise its own error - } - if (typeof iter === 'number' && iter > V1_MAX_ITER) { - throw new Error(`v1 decrypt: iter ${iter} exceeds cap of ${V1_MAX_ITER}`); - } +function isBrowserRuntime(): boolean { + return typeof window !== 'undefined' || typeof self !== 'undefined'; } /** @@ -106,11 +90,16 @@ function isIterCapViolation(err: unknown): boolean { /** * v1 decrypt with an SJCL safety net. * - * Design intent during rollout: zero false negatives. Any native failure - * (envelope shape our stricter codec rejects, framing bug, unsupported - * algorithm, auth-tag mismatch, etc.) falls through to `sjcl.decrypt` so the - * caller is never blocked. The only exception is an iter-cap violation, - * which is rethrown to preserve DoS protection. + * Node: native `node:crypto` first, SJCL fallback. Design intent during + * rollout: zero false negatives. Any native failure (envelope shape our + * stricter codec rejects, framing bug, unsupported algorithm, auth-tag + * mismatch, etc.) falls through to `sjcl.decrypt` so the caller is never + * blocked. The only exception is an iter-cap violation, which is rethrown + * to preserve DoS protection. + * + * Browser/worker: skip the native attempt entirely (its polyfilled AES-CCM + * is broken) and go straight to `sjcl.decrypt`, after running the same + * `parseV1Envelope` iter-cap check the native path would run. * * The console.warn only fires when native fails AND SJCL succeeds -- i.e. * when the two engines disagree, which is the only signal worth @@ -126,10 +115,19 @@ export async function decryptV1WithFallback( password: string, ciphertext: string, native: (pw: string, ct: string) => Promise = decryptV1, - isBrowser: boolean = isBrowserRuntime + isBrowser: boolean = isBrowserRuntime() ): Promise { if (isBrowser) { - assertIterWithinCap(ciphertext); + try { + parseV1Envelope(ciphertext); + } catch (parseErr) { + // Only honor the iter-cap violation: SJCL has no upper bound on `iter`, + // so falling through would let a hostile envelope burn CPU running an + // inflated PBKDF2. Any other shape rejection (e.g. a legacy envelope + // without a `v` field) is a form SJCL still accepts, so let + // sjcl.decrypt make the final call. + if (isIterCapViolation(parseErr)) throw parseErr; + } return sjcl.decrypt(password, ciphertext); } try { diff --git a/modules/sdk-api/test/unit/decryptV1.browser.ts b/modules/sdk-api/test/unit/decryptV1.browser.ts deleted file mode 100644 index cd52944cd9..0000000000 --- a/modules/sdk-api/test/unit/decryptV1.browser.ts +++ /dev/null @@ -1,108 +0,0 @@ -import * as sjcl from '@bitgo/sjcl'; -import assert from 'assert'; - -import { decryptV1WithCrypto, V1_MAX_ITER } from '../../src'; -import { KEYCARD_BOX_A, KEYCARD_BOX_B, KEYCARD_PASSWORD, KEYCARD_PLAINTEXT_PREFIX } from './fixtures/keycard'; -// eslint-disable-next-line @typescript-eslint/no-var-requires -const browserCrypto = require('crypto-browserify'); - -/** - * sjcl.encrypt's typings require salt/iv, but the runtime picks them from - * sjcl.random when omitted. Feed real random words so the call type-checks - * without an `as` cast. - */ -function sjclEncrypt(password: string, plaintext: string, params: sjcl.SjclCipherParams): string { - const salt = sjcl.random.randomWords(2); - const iv = sjcl.random.randomWords(4); - return sjcl.encrypt(password, plaintext, { ...params, salt, iv }); -} - -/** - * Exercises the real `decryptV1WithCrypto` code path with `crypto-browserify` - * injected as the crypto module. This is exactly what webpack bundles for the - * browser (its `crypto` shim), so a green test here proves the browser build - * stays byte-compatible with SJCL-produced envelopes and the Node path. - */ -function decryptV1Browser(password: string, ciphertext: string): Promise { - return decryptV1WithCrypto(password, ciphertext, browserCrypto); -} - -describe('decryptV1 browser path (crypto-browserify shim)', () => { - const password = 'myPassword'; - const plaintext = 'Hello, Browser!'; - - it('produces the same plaintext as sjcl.decrypt', async () => { - const ciphertext = sjclEncrypt(password, plaintext, { iter: 10000, ks: 256, ts: 64, mode: 'ccm' }); - assert.strictEqual(await decryptV1Browser(password, ciphertext), sjcl.decrypt(password, ciphertext)); - }); - - it('handles adata (AAD)', async () => { - const ciphertext = sjclEncrypt(password, plaintext, { - iter: 10000, - ks: 256, - ts: 64, - mode: 'ccm', - adata: 'ctx-A', - }); - assert.strictEqual(await decryptV1Browser(password, ciphertext), plaintext); - }); - - it('handles UTF-8 password + plaintext', async () => { - const utf8Password = 'pässwörd中文🔐'; - const utf8Plaintext = 'passphrase: 秘密キー ☃🔑'; - const ciphertext = sjclEncrypt(utf8Password, utf8Plaintext, { iter: 10000, ks: 256, ts: 64, mode: 'ccm' }); - assert.strictEqual(await decryptV1Browser(utf8Password, ciphertext), utf8Plaintext); - }); - - it('handles large plaintext (>64 KiB, forces L=3 nonce framing)', async () => { - const large = 'x'.repeat(70_000); - const ciphertext = sjclEncrypt(password, large, { iter: 1000, ks: 256, ts: 64, mode: 'ccm' }); - assert.strictEqual(await decryptV1Browser(password, ciphertext), large); - }); - - it('handles aes-128 envelopes', async () => { - const ciphertext = sjclEncrypt(password, plaintext, { iter: 10000, ks: 128, ts: 64, mode: 'ccm' }); - assert.strictEqual(await decryptV1Browser(password, ciphertext), plaintext); - }); - - it('handles 128-bit tag envelopes', async () => { - const ciphertext = sjclEncrypt(password, plaintext, { iter: 10000, ks: 256, ts: 128, mode: 'ccm' }); - assert.strictEqual(await decryptV1Browser(password, ciphertext), plaintext); - }); - - it('rejects wrong password', async () => { - const ciphertext = sjclEncrypt(password, plaintext, { iter: 10000, ks: 256, ts: 64, mode: 'ccm' }); - await assert.rejects(() => decryptV1Browser('wrongPassword', ciphertext)); - }); - - it('rejects envelope with iter above cap before running PBKDF2', async () => { - const ciphertext = sjclEncrypt(password, plaintext, { iter: 10000, ks: 256, ts: 64, mode: 'ccm' }); - const envelope = JSON.parse(ciphertext); - envelope.iter = V1_MAX_ITER + 1; - const start = Date.now(); - await assert.rejects(() => decryptV1Browser(password, JSON.stringify(envelope)), /iter/); - assert.ok(Date.now() - start < 100, 'must reject before any KDF work'); - }); - - it('parity across 50 randomised inputs', async () => { - const { randomBytes } = await import('crypto'); - for (let i = 0; i < 50; i++) { - const pw = randomBytes(16).toString('hex'); - const pt = randomBytes(1 + Math.floor(Math.random() * 500)).toString('base64'); - const ciphertext = sjclEncrypt(pw, pt, { iter: 1000, ks: 256, ts: 64, mode: 'ccm' }); - assert.strictEqual(await decryptV1Browser(pw, ciphertext), pt, `iteration ${i}`); - } - }); - - it('Box A + Box B: shim decrypt matches SJCL byte-for-byte', async () => { - for (const [label, ct] of [ - ['A', KEYCARD_BOX_A], - ['B', KEYCARD_BOX_B], - ] as const) { - const sjclResult = sjcl.decrypt(KEYCARD_PASSWORD, ct); - const shimResult = await decryptV1Browser(KEYCARD_PASSWORD, ct); - assert.strictEqual(shimResult, sjclResult, `Box ${label} mismatch`); - assert.ok(shimResult.startsWith(KEYCARD_PLAINTEXT_PREFIX)); - } - }); -}); diff --git a/modules/web-demo/src/components/BitGoAPI/decrypt.spec.tsx b/modules/web-demo/src/components/BitGoAPI/decrypt.spec.tsx new file mode 100644 index 0000000000..dd211a3305 --- /dev/null +++ b/modules/web-demo/src/components/BitGoAPI/decrypt.spec.tsx @@ -0,0 +1,109 @@ +/// +import { BitGoAPI, V1_MAX_ITER } from '@bitgo/sdk-api'; + +/** + * WCN-2576: in a real browser bundle, v1 decrypt must route straight to the + * frozen SJCL decoder without ever attempting the native `node:crypto` path. + * The `crypto` webpack substitutes for `node:crypto` there (`crypto-browserify`, + * backed by `browserify-aes`) has no working AES-CCM, so any native attempt + * would burn a full PBKDF2 run, fail, and then log the fallback warning on the + * SJCL success. A correct decrypt with no `[bitgo-sdk] v1 native decrypt + * failed` console.warn therefore proves the browser branch was taken. + */ +describe('BitGoAPI.decrypt v1 browser routing', () => { + const password = 'myPassword'; + const plaintext = 'Hello, Browser!'; + + let sdk: BitGoAPI; + let v1Ciphertext: string; + + before(async () => { + // clientConstants={} prevents the constructor's background fetchConstants() + // network call; decrypt is fully local, so no request is ever needed. + sdk = new BitGoAPI({ env: 'test', clientConstants: {} }); + v1Ciphertext = await sdk.encrypt({ + password, + input: plaintext, + encryptionVersion: 1, + }); + }); + + it('decrypts a v1 envelope via the SJCL decoder without attempting the broken native path', () => { + const warn = cy.stub(console, 'warn'); + // Sanity: this spec must run in a real browser runtime for it to prove + // browser dispatch (window implies self, so isBrowserRuntime() is true). + expect(typeof window).to.not.equal('undefined'); + + cy.wrap(sdk.decrypt({ password, input: v1Ciphertext })).should( + 'equal', + plaintext, + ); + cy.wrap(warn).should('not.be.called'); + }); + + it('decrypts a legacy envelope without a `v` field (a shape only the SJCL decoder accepts)', () => { + const warn = cy.stub(console, 'warn'); + const envelope = JSON.parse(v1Ciphertext); + expect(envelope.v).to.equal(1); + delete envelope.v; + + cy.wrap(sdk.decrypt({ password, input: JSON.stringify(envelope) })).should( + 'equal', + plaintext, + ); + cy.wrap(warn).should('not.be.called'); + }); + + it('rejects an iter-cap violation before any KDF work runs', () => { + const warn = cy.stub(console, 'warn'); + const envelope = JSON.parse(v1Ciphertext); + envelope.iter = V1_MAX_ITER + 1; + const input = JSON.stringify(envelope); + + // Resolve with the rejection and the elapsed time instead of failing the + // command chain, so both can be asserted directly. + type Measured = { error: unknown; elapsedMs: number }; + const start = performance.now(); + const measured: Promise = sdk.decrypt({ password, input }).then( + (): Measured => ({ + error: undefined, + elapsedMs: performance.now() - start, + }), + (error: unknown): Measured => ({ + error, + elapsedMs: performance.now() - start, + }), + ); + + cy.wrap(measured).then((result) => { + const { error, elapsedMs } = result as Measured; + expect(error, 'iter above the cap must be rejected').to.be.an('error'); + expect((error as Error).message).to.match(/iter/); + // A PBKDF2 run at iter = V1_MAX_ITER + 1 takes on the order of seconds, + // while the envelope check is single-digit milliseconds. + expect(elapsedMs).to.be.lessThan(1000); + expect(warn).to.not.be.called; + }); + }); + + it('surfaces the SJCL auth failure as `incorrect password`', () => { + const warn = cy.stub(console, 'warn'); + // Resolve with the rejection instead of failing the command chain, so the + // mapped message can be asserted directly (Cypress does not bundle + // chai-as-promised, so `be.rejectedWith` is not available). + type Outcome = { error: unknown }; + const outcome: Promise = sdk + .decrypt({ password: 'wrongPassword', input: v1Ciphertext }) + .then( + (): Outcome => ({ error: undefined }), + (error: unknown): Outcome => ({ error }), + ); + + cy.wrap(outcome).then((result) => { + const { error } = result as Outcome; + expect(error, 'wrong password must be rejected').to.be.an('error'); + expect((error as Error).message).to.equal('incorrect password'); + expect(warn).to.not.be.called; + }); + }); +});