diff --git a/packages/kdbx/src/bytes.ts b/packages/kdbx/src/bytes.ts index ca73b2d..0c34252 100644 --- a/packages/kdbx/src/bytes.ts +++ b/packages/kdbx/src/bytes.ts @@ -186,6 +186,16 @@ export class ByteReader { } #require(count: number): void { + // A negative count (e.g. from a maliciously-crafted Int32 length field in + // a TLV-framed structure — the outer/inner header and VariantDictionary + // parsers all read attacker-controlled lengths this way) would otherwise + // pass the check below, since offset + a negative count is always less + // than offset. readBytes would then walk #offset *backward* instead of + // throwing, which a crafted file can use to pin the cursor at a fixed + // position and loop forever. + if (count < 0) { + throw new RangeError(`byte count must not be negative, got ${count}`); + } if (this.#offset + count > this.#bytes.length) { throw new RangeError( `unexpected end of data: needed ${count} byte(s) at offset ${this.#offset}`, diff --git a/packages/kdbx/src/crypto.ts b/packages/kdbx/src/crypto.ts index c032ae6..daa9161 100644 --- a/packages/kdbx/src/crypto.ts +++ b/packages/kdbx/src/crypto.ts @@ -96,6 +96,19 @@ export async function aesCbcDecrypt( } } +/** + * Sanity ceiling on AES-KDF's `rounds` parameter, which for KDBX 3.1 (and the + * AES-KDF choice in KDBX 4.x KDF parameters) comes straight from the file's + * own, not-yet-authenticated header. `transformHalf` below allocates a + * `16 * rounds`-byte plaintext buffer per half, so an unchecked value near + * Number.MAX_SAFE_INTEGER (the previous ceiling) would attempt a many-exabyte + * allocation just from opening a crafted file, before any credentials are + * checked. This value is far above any real KeePass configuration (this + * project's own default is 60,000) while keeping a worst-case unlock attempt + * bounded to something a browser tab can actually survive. + */ +const KX_MAX_AES_KDF_ROUNDS = 100_000_000n; + /** * AES-KDF transformation (the KDBX 3.1 / legacy key derivation function). * @@ -116,8 +129,10 @@ export async function aesKdfTransform( if (rounds <= 0n) { throw new RangeError('AES-KDF rounds must be positive'); } - if (rounds > BigInt(Number.MAX_SAFE_INTEGER)) { - throw new RangeError('AES-KDF rounds exceed the supported maximum'); + if (rounds > KX_MAX_AES_KDF_ROUNDS) { + throw new RangeError( + `AES-KDF rounds (${rounds}) exceed the maximum this app will run (${KX_MAX_AES_KDF_ROUNDS})`, + ); } const n = Number(rounds); const subtle = kx_getCrypto().subtle; diff --git a/packages/kdbx/src/kdf.ts b/packages/kdbx/src/kdf.ts index 817f084..4e5eab0 100644 --- a/packages/kdbx/src/kdf.ts +++ b/packages/kdbx/src/kdf.ts @@ -16,6 +16,23 @@ import { type VariantDictionary, vdRequireBytes, vdRequireInt } from './variant- const KX_ARGON2_TAG_LENGTH = 32; const KX_BYTES_PER_KIB = 1024n; +/** + * Sanity ceiling on Argon2 cost parameters read from a file's (untrusted, + * pre-authentication) KDF parameters. RFC 9106's own legal ranges — enforced + * separately by the argon2 package itself — allow up to 2^32-1 KiB (~4 TiB) + * of memory and 2^32-1 iterations, since that package has no opinion on what + * a caller finds reasonable. Left unchecked here, a crafted file could force + * an attempted multi-gigabyte-or-larger allocation, or a computation that + * never realistically finishes, just by being opened — with any password + * attempt, since the KDF runs before the file's authenticity is verified. + * These values are generous relative to any real-world KDBX configuration + * (KeePass's own defaults are far below them) while keeping a worst-case + * unlock attempt bounded to something a browser tab can actually survive. + */ +const KX_MAX_ARGON2_MEMORY_KIB = 2 * 1024 * 1024; // 2 GiB +const KX_MAX_ARGON2_ITERATIONS = 64; +const KX_MAX_ARGON2_PARALLELISM = 64; + /** Transform a 32-byte composite key with AES-KDF (KDBX 3.1 and the AES-KDF KDF). */ export async function aesKdf( compositeKey: Uint8Array, @@ -71,6 +88,22 @@ function kx_runArgon2( const memoryBytes = vdRequireInt(params, KdfParam.Argon2Memory); const memory = Number(memoryBytes / KX_BYTES_PER_KIB); + if (parallelism > KX_MAX_ARGON2_PARALLELISM) { + throw new Error( + `Argon2 parallelism (${parallelism}) exceeds the maximum this app will run (${KX_MAX_ARGON2_PARALLELISM})`, + ); + } + if (iterations > KX_MAX_ARGON2_ITERATIONS) { + throw new Error( + `Argon2 iterations (${iterations}) exceeds the maximum this app will run (${KX_MAX_ARGON2_ITERATIONS})`, + ); + } + if (memory > KX_MAX_ARGON2_MEMORY_KIB) { + throw new Error( + `Argon2 memory (${memory} KiB) exceeds the maximum this app will run (${KX_MAX_ARGON2_MEMORY_KIB} KiB)`, + ); + } + const versionParam = params.get(KdfParam.Argon2Version); const version = versionParam === undefined diff --git a/packages/kdbx/tests/aes-kdf.test.ts b/packages/kdbx/tests/aes-kdf.test.ts index 90bda84..7aadfe1 100644 --- a/packages/kdbx/tests/aes-kdf.test.ts +++ b/packages/kdbx/tests/aes-kdf.test.ts @@ -54,3 +54,15 @@ test('aesKdfTransform validates input', async () => { RangeError, ); }); + +test('aesKdfTransform rejects a rounds count above the sanity ceiling without attempting it', async () => { + // rounds comes straight from a file's own (not-yet-authenticated) header; + // this must reject immediately rather than actually attempting anywhere + // near this many AES rounds, or the test would never finish either. + const key = new Uint8Array(32); + const seed = new Uint8Array(32); + await assert.rejects( + () => aesKdfTransform(key, seed, 100_000_001n), + /rounds.*exceed the maximum/, + ); +}); diff --git a/packages/kdbx/tests/bytes.test.ts b/packages/kdbx/tests/bytes.test.ts index 72147c0..3f8bd72 100644 --- a/packages/kdbx/tests/bytes.test.ts +++ b/packages/kdbx/tests/bytes.test.ts @@ -72,6 +72,18 @@ test('ByteReader throws past the end', () => { assert.throws(() => new ByteReader(new Uint8Array(2)).readU32(), RangeError); }); +test('ByteReader rejects a negative byte count instead of seeking backward', () => { + // A crafted TLV length field (e.g. a header field's Int32 size) can be + // negative. Without a check, offset + count still passes the "not past the + // end" bound, and the offset would walk backward instead of erroring — + // letting a malicious file pin the cursor in place and loop forever. + const reader = new ByteReader(new Uint8Array(16)); + reader.readBytes(8); // advance the cursor away from 0 first + assert.throws(() => reader.readBytes(-1), /byte count must not be negative/); + // The failed read must not have moved the cursor. + assert.equal(reader.offset, 8); +}); + test('concatBytes and bytesEqual', () => { assert.deepEqual( concatBytes(new Uint8Array([1]), new Uint8Array([2, 3])), diff --git a/packages/kdbx/tests/kdf.test.ts b/packages/kdbx/tests/kdf.test.ts index 87c6a41..710e66c 100644 --- a/packages/kdbx/tests/kdf.test.ts +++ b/packages/kdbx/tests/kdf.test.ts @@ -68,3 +68,37 @@ test('Argon2 ignores a secret/associated-data parameter stored as the wrong type const key2 = await transformWithKdfParameters(new Uint8Array(32), plain); assert.deepEqual(key1, key2); // the malformed secret param was ignored }); + +// A file's KDF parameters are read before its authenticity is checked (the +// derived key is needed to verify the HMAC in the first place), so an +// untrusted file can request any cost it likes. These three guard against +// that being used to force a multi-gigabyte allocation or a computation that +// never finishes just by being opened, regardless of the password typed. + +test('Argon2 rejects a parallelism above the sanity ceiling', async () => { + const params = argon2Params([[KdfParam.Argon2Parallelism, { type: 'uint32', value: 65 }]]); + await assert.rejects( + () => transformWithKdfParameters(new Uint8Array(32), params), + /parallelism.*exceeds the maximum/, + ); +}); + +test('Argon2 rejects an iteration count above the sanity ceiling', async () => { + const params = argon2Params([[KdfParam.Argon2Iterations, { type: 'uint64', value: 65n }]]); + await assert.rejects( + () => transformWithKdfParameters(new Uint8Array(32), params), + /iterations.*exceeds the maximum/, + ); +}); + +test('Argon2 rejects a memory cost above the sanity ceiling', async () => { + // KDBX stores memory in bytes; one KiB over the 2 GiB ceiling, rejected + // before any allocation is attempted. + const params = argon2Params([ + [KdfParam.Argon2Memory, { type: 'uint64', value: 2n * 1024n * 1024n * 1024n + 1024n }], + ]); + await assert.rejects( + () => transformWithKdfParameters(new Uint8Array(32), params), + /memory.*exceeds the maximum/, + ); +}); diff --git a/pages/0x67/logic.ts b/pages/0x67/logic.ts index 83e89a5..9e1de8e 100644 --- a/pages/0x67/logic.ts +++ b/pages/0x67/logic.ts @@ -255,13 +255,26 @@ function exportFields(entry: XmlElement, group: XmlElement): [string, string][] ]; } +/** A leading `=`, `+`, `-`, `@`, tab, or CR makes Excel, Sheets, and + * LibreOffice Calc treat a CSV field as a formula rather than literal text — + * CWE-1236. Entry data (a Title, URL, or Notes field) is attacker-reachable + * in a way a spreadsheet's own cells normally aren't, so it can't be assumed + * safe. */ +const CSV_FORMULA_TRIGGER = /^[=+\-@\t\r]/; + /** Quote a CSV field only when it needs it (contains a comma, quote, or - * newline), doubling any internal quotes — RFC 4180. */ + * newline), doubling any internal quotes — RFC 4180. RFC 4180 quoting alone + * does not stop a spreadsheet application from evaluating a quoted field's + * content as a formula, so a leading formula-trigger character is neutralized + * first by prefixing a literal apostrophe — the standard mitigation every + * mainstream spreadsheet app already treats as "force text" (the same effect + * as typing `'123` into a cell by hand). */ function csvField(value: string): string { - if (/[",\r\n]/.test(value)) { - return `"${value.replace(/"/g, '""')}"`; + const safe = CSV_FORMULA_TRIGGER.test(value) ? `'${value}` : value; + if (/[",\r\n]/.test(safe)) { + return `"${safe.replace(/"/g, '""')}"`; } - return value; + return safe; } /** Serialize entries (Group, Title, UserName, Password, URL, Notes, Tags) as diff --git a/pages/tests/0x67-logic.test.ts b/pages/tests/0x67-logic.test.ts index 492f259..8f7c334 100644 --- a/pages/tests/0x67-logic.test.ts +++ b/pages/tests/0x67-logic.test.ts @@ -309,6 +309,45 @@ test('toCsv writes just the header row for an empty entry list', () => { assert.equal(toCsv([]), 'Group,Title,UserName,Password,URL,Notes,Tags'); }); +test('toCsv neutralizes a leading formula-trigger character (CSV/Excel formula injection)', () => { + const root = createGroup('Personal'); + const entry = createEntry({ + title: '=1+1', + username: '+15555550100', + password: '-hunter2', + url: '@example.com', + }); + appendChild(root, entry); + + const csv = toCsv(collectAllEntries(root)); + const lines = csv.split('\r\n'); + assert.equal(lines[1], "Personal,'=1+1,'+15555550100,'-hunter2,'@example.com,,"); +}); + +test('toCsv combines formula neutralization with RFC 4180 quoting when a field needs both', () => { + const root = createGroup('Personal'); + const entry = createEntry({ title: '=SUM(A1,"x")' }); + appendChild(root, entry); + + const csv = toCsv(collectAllEntries(root)); + const lines = csv.split('\r\n'); + // The apostrophe is added first; the (now longer) value still contains a + // comma and a quote, so it's also RFC 4180 quoted, same as any other field. + assert.equal(lines[1], `Personal,"'=SUM(A1,""x"")",,,,,`); +}); + +test('toCsv leaves a field with no leading formula-trigger character untouched', () => { + const root = createGroup('Personal'); + // '@' appears, but not leading — only a *leading* trigger character is + // dangerous to a spreadsheet, so this must not be prefixed. + const entry = createEntry({ username: 'plain@example.com' }); + appendChild(root, entry); + + const csv = toCsv(collectAllEntries(root)); + const lines = csv.split('\r\n'); + assert.equal(lines[1], 'Personal,(no title),plain@example.com,,,,'); +}); + test('toXml escapes XML-significant characters and wraps entries in ', () => { const root = createGroup('Root'); const entry = createEntry({ title: 'A & B