diff --git a/.changeset/crypto-keystore-mac-iv.md b/.changeset/crypto-keystore-mac-iv.md new file mode 100644 index 000000000..7a31057fc --- /dev/null +++ b/.changeset/crypto-keystore-mac-iv.md @@ -0,0 +1,17 @@ +--- +'@xchainjs/xchain-crypto': minor +--- + +Include the CTR IV in the keystore MAC (format version 2) + +New keystores written by `encryptToKeyStore` use `version: 2` and compute +`blake2b(macKey || iv || ciphertext)` so an attacker cannot flip bits in the +decrypted plaintext by tampering with `cipherparams.iv` without invalidating +the MAC (#1721). + +**Reading:** v1 keystores (`version: 1`, MAC over `macKey || ciphertext` only) +still decrypt unchanged. + +**Writing:** newly created files are v2 and cannot be verified by older +`@xchainjs/xchain-crypto` releases that only check the v1 MAC layout. Existing +files are unaffected. diff --git a/packages/xchain-crypto/__tests__/crypto.test.ts b/packages/xchain-crypto/__tests__/crypto.test.ts index 8a3c8c83b..30301ae79 100755 --- a/packages/xchain-crypto/__tests__/crypto.test.ts +++ b/packages/xchain-crypto/__tests__/crypto.test.ts @@ -18,7 +18,8 @@ describe('Keystore regression test for encrypt/decrypt with internal migration', const phrase = 'patient use either flash couple jump castle true broccoli cancel brand mechanic' const password = '1234' - const expectedKeystore = { + // v1 fixture: MAC = blake2b(macKey || ciphertext) — IV not covered (#1721 legacy) + const expectedKeystoreV1 = { crypto: { cipher: 'aes-128-ctr', ciphertext: @@ -38,6 +39,18 @@ describe('Keystore regression test for encrypt/decrypt with internal migration', meta: 'xchain-keystore', } + // Same salt/iv/ciphertext as v1, but MAC includes IV (v2). Ciphertext is unchanged because + // AES-CTR still uses the same key material layout; only the MAC binding changes. + const expectedKeystoreV2 = { + crypto: { + ...expectedKeystoreV1.crypto, + mac: '801caafeca2863d0486c00464fb954ff512e8146e331710173f2c2994068f89f', + }, + id: '9ad9ea91-22ad-46a7-9613-4f9d190e32ab', + version: 2, + meta: 'xchain-keystore', + } + // Keystore produced with the legacy c=262144 iteration count. The iteration count is // stored in the keystore itself, so older keystores must still decrypt unchanged. const legacyKeystore = { @@ -60,22 +73,28 @@ describe('Keystore regression test for encrypt/decrypt with internal migration', meta: 'xchain-keystore', } - it('encryptToKeyStore() should produce expected ciphertext and mac', async () => { + it('encryptToKeyStore() should produce expected v2 ciphertext and mac (IV bound into MAC)', async () => { jest .spyOn(crypto, 'randomBytes') - .mockImplementationOnce(() => Buffer.from(expectedKeystore.crypto.kdfparams.salt, 'hex')) // salt - .mockImplementationOnce(() => Buffer.from(expectedKeystore.crypto.cipherparams.iv, 'hex')) // iv + .mockImplementationOnce(() => Buffer.from(expectedKeystoreV2.crypto.kdfparams.salt, 'hex')) // salt + .mockImplementationOnce(() => Buffer.from(expectedKeystoreV2.crypto.cipherparams.iv, 'hex')) // iv const keystore = await encryptToKeyStore(phrase, password) - expect(keystore.crypto.ciphertext).toBe(expectedKeystore.crypto.ciphertext) - expect(keystore.crypto.mac).toBe(expectedKeystore.crypto.mac) - expect(keystore.crypto.kdfparams).toEqual(expectedKeystore.crypto.kdfparams) - expect(keystore.crypto.cipherparams).toEqual(expectedKeystore.crypto.cipherparams) + expect(keystore.version).toBe(2) + expect(keystore.crypto.ciphertext).toBe(expectedKeystoreV2.crypto.ciphertext) + expect(keystore.crypto.mac).toBe(expectedKeystoreV2.crypto.mac) + expect(keystore.crypto.kdfparams).toEqual(expectedKeystoreV2.crypto.kdfparams) + expect(keystore.crypto.cipherparams).toEqual(expectedKeystoreV2.crypto.cipherparams) }) - it('decryptFromKeystore() should return original phrase', async () => { - const result = await decryptFromKeystore(expectedKeystore, password) + it('decryptFromKeystore() should return original phrase for v2 keystores', async () => { + const result = await decryptFromKeystore(expectedKeystoreV2, password) + expect(result).toBe(phrase) + }) + + it('decryptFromKeystore() should still decrypt v1 (MAC without IV) keystores', async () => { + const result = await decryptFromKeystore(expectedKeystoreV1, password) expect(result).toBe(phrase) }) @@ -85,7 +104,37 @@ describe('Keystore regression test for encrypt/decrypt with internal migration', }) it('decryptFromKeystore() should reject an incorrect password', async () => { - await expect(decryptFromKeystore(expectedKeystore, 'wrong-password')).rejects.toThrow('Invalid password') + await expect(decryptFromKeystore(expectedKeystoreV2, 'wrong-password')).rejects.toThrow('Invalid password') + }) + + it('decryptFromKeystore() should reject a v2 keystore whose IV was tampered', async () => { + const tampered = { + ...expectedKeystoreV2, + crypto: { + ...expectedKeystoreV2.crypto, + cipherparams: { + // flip last nibble of the IV — changes CTR keystream without touching ciphertext/mac blob as stored + iv: 'dffdb8bbe92e9a00e173eaa20f1a3785', + }, + }, + } + await expect(decryptFromKeystore(tampered, password)).rejects.toThrow('Invalid password') + }) + + it('decryptFromKeystore() would not reject the same IV tamper on v1 (documents legacy gap)', async () => { + // CTR: same ciphertext + wrong IV decrypts to garbage, but v1 MAC does not bind IV so + // verification still passes — this is the #1721 issue. We assert that behavior remains + // for legacy files only (not a desired property for new keystores). + const tamperedV1 = { + ...expectedKeystoreV1, + crypto: { + ...expectedKeystoreV1.crypto, + cipherparams: { iv: 'dffdb8bbe92e9a00e173eaa20f1a3785' }, + }, + } + // MAC check passes; decrypted phrase is not the original + const garbled = await decryptFromKeystore(tamperedV1, password) + expect(garbled).not.toBe(phrase) }) // Browsers/bundlers polyfill `crypto` with crypto-browserify, which lacks @@ -103,12 +152,12 @@ describe('Keystore regression test for encrypt/decrypt with internal migration', }) it('decryptFromKeystore() should return original phrase via fallback comparison', async () => { - const result = await decryptFromKeystore(expectedKeystore, password) + const result = await decryptFromKeystore(expectedKeystoreV2, password) expect(result).toBe(phrase) }) it('decryptFromKeystore() should still reject an incorrect password via fallback comparison', async () => { - await expect(decryptFromKeystore(expectedKeystore, 'wrong-password')).rejects.toThrow('Invalid password') + await expect(decryptFromKeystore(expectedKeystoreV2, 'wrong-password')).rejects.toThrow('Invalid password') }) }) }) @@ -140,7 +189,7 @@ describe('Export Keystore', () => { expect(keystore.crypto.kdf).toEqual('pbkdf2') expect(keystore.crypto.kdfparams.prf).toEqual('hmac-sha256') expect(keystore.crypto.kdfparams.c).toEqual(600000) - expect(keystore.version).toEqual(1) + expect(keystore.version).toEqual(2) expect(keystore.meta).toEqual('xchain-keystore') }) }) diff --git a/packages/xchain-crypto/src/crypto.ts b/packages/xchain-crypto/src/crypto.ts index 6107fb7e1..8a4251948 100755 --- a/packages/xchain-crypto/src/crypto.ts +++ b/packages/xchain-crypto/src/crypto.ts @@ -15,6 +15,10 @@ const dklen = 32 // Derived key length const c = 600000 // Iteration count (OWASP-recommended minimum for PBKDF2-HMAC-SHA256) const hashFunction = 'sha256' // Hash function const meta = 'xchain-keystore' // Metadata +/** Keystores written by this package. v2+ MAC covers the IV (see #1721). */ +const keystoreVersion = 2 +/** First version whose MAC includes `cipherparams.iv`. */ +const macIncludesIvFromVersion = 2 /** * The Keystore interface. @@ -69,6 +73,27 @@ const constantTimeEqual = (a: Buffer, b: Buffer): boolean => { return diff === 0 } +/** + * Whether this keystore version's MAC authenticates the CTR IV. + * + * v1: blake2b(macKey || ciphertext) — IV not covered (CTR malleability; #1721). + * v2+: blake2b(macKey || iv || ciphertext) + */ +const keystoreMacIncludesIv = (version: number): boolean => version >= macIncludesIvFromVersion + +/** + * Compute the keystore integrity MAC. + * + * @param macKey Second half of the PBKDF2-derived key (bytes 16–32 for current layout). + * @param iv CTR IV bytes. + * @param ciphertext Encrypted payload. + * @param includeIv When true, bind the IV into the MAC (v2+). + */ +const computeKeystoreMac = (macKey: Buffer, iv: Buffer, ciphertext: Buffer, includeIv: boolean): Buffer => { + const parts = includeIv ? [macKey, iv, ciphertext] : [macKey, ciphertext] + return Buffer.from(blake2b(Buffer.concat(parts), { dkLen: 32 })) +} + /** * Generates a new mnemonic phrase. * @param {number} size The size of the phrase in words. Default is 12. @@ -136,12 +161,12 @@ export const encryptToKeyStore = async (phrase: string, password: string): Promi } const derivedKey = await pbkdf2Async(Buffer.from(password), salt, kdfParams.c, kdfParams.dklen, hashFunction) - const cipherIV = crypto.createCipheriv(cipher, derivedKey.slice(0, 16), iv) + const encryptionKey = derivedKey.slice(0, 16) + const macKey = derivedKey.slice(16, 32) + const cipherIV = crypto.createCipheriv(cipher, encryptionKey, iv) const cipherText = Buffer.concat([cipherIV.update(Buffer.from(phrase, 'utf8')), cipherIV.final()]) - const mac_bytes: Uint8Array = blake2b(Buffer.concat([derivedKey.slice(16, 32), Buffer.from(cipherText)]), { - dkLen: 32, - }) - const mac: string = Buffer.from(mac_bytes).toString('hex') + // v2+: include IV so CTR keystream offset cannot be tampered without failing the MAC (#1721) + const mac = computeKeystoreMac(macKey, iv, Buffer.from(cipherText), true).toString('hex') const cryptoStruct = { cipher: cipher, @@ -155,7 +180,7 @@ export const encryptToKeyStore = async (phrase: string, password: string): Promi const keystore = { crypto: cryptoStruct, id: ID, - version: 1, + version: keystoreVersion, meta: meta, } @@ -180,18 +205,16 @@ export const decryptFromKeystore = async (keystore: Keystore, password: string): ) const ciphertext = Buffer.from(keystore.crypto.ciphertext, 'hex') - const mac_bytes: Uint8Array = blake2b(Buffer.concat([derivedKey.slice(16, 32), ciphertext]), { dkLen: 32 }) - const computedMac = Buffer.from(mac_bytes) + const iv = Buffer.from(keystore.crypto.cipherparams.iv, 'hex') + const macKey = derivedKey.slice(16, 32) + const includeIv = keystoreMacIncludesIv(keystore.version) + const computedMac = computeKeystoreMac(macKey, iv, ciphertext, includeIv) const expectedMac = Buffer.from(keystore.crypto.mac, 'hex') // Constant-time comparison to avoid leaking MAC bytes via timing if (computedMac.length !== expectedMac.length || !constantTimeEqual(computedMac, expectedMac)) throw new Error('Invalid password') - const decipher = crypto.createDecipheriv( - keystore.crypto.cipher, - derivedKey.slice(0, 16), - Buffer.from(keystore.crypto.cipherparams.iv, 'hex'), - ) + const decipher = crypto.createDecipheriv(keystore.crypto.cipher, derivedKey.slice(0, 16), iv) const phrase = Buffer.concat([decipher.update(ciphertext), decipher.final()]) return phrase.toString('utf8')