fix(crypto): include CTR IV in keystore MAC (v2)

.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.

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')
   })
 })

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')