feat(crypto): upgrade keystore encryption to AES-256-CTR (v2) - #1723
feat(crypto): upgrade keystore encryption to AES-256-CTR (v2)#1723betelthyme wants to merge 1 commit into
Conversation
encryptToKeyStore now derives a 64-byte key (dklen 32 -> 64) and splits it into an independent 32-byte AES-256 key and 32-byte MAC key, writing cipher "aes-256-ctr" and version 2. Previously only the first 16 of 32 derived bytes were used, with aes-128-ctr. decryptFromKeystore derives the AES-key/MAC-key split from the keystore's own cipher and dklen, so existing v1 (aes-128-ctr) keystores still decrypt unchanged. Files newly written are v2 and cannot be read by older xchain-crypto releases (forward-incompatible write; documented in changeset). Tests: v2 known-answer (ciphertext + mac) test, v2 decrypt, and preserved v1 backward-compat decrypt fixtures. All 14 tests pass; build, typecheck, and lint clean. Closes #1720 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR upgrades ChangesKeystore v2 encryption/decryption
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/xchain-crypto/src/crypto.ts (1)
212-226: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winValidate
dklenagainst AES key length to prevent empty MAC key.If a malformed or crafted keystore has
dklen <= aesKeyLength(e.g.,cipher: 'aes-256-ctr'withdklen: 32),macKeywould be an empty buffer. The MAC would then beblake2b(ciphertext)— independent of the password — so any password would pass the MAC check and proceed to decryption, returning garbage instead of anInvalid passworderror. Adding a guard indecryptFromKeystorecloses this edge case.🛡️ Proposed validation in decryptFromKeystore
const aesKeyLength = aesKeyLengthForCipher(keystore.crypto.cipher) + if (kdfparams.dklen < aesKeyLength + 16) { + throw new Error('Invalid keystore: derived key length too short for cipher') + } const encryptionKey = derivedKey.slice(0, aesKeyLength) const macKey = derivedKey.slice(aesKeyLength, kdfparams.dklen)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/xchain-crypto/src/crypto.ts` around lines 212 - 226, In decryptFromKeystore, add a validation after deriving aesKeyLength and before slicing derivedKey to ensure kdfparams.dklen is greater than the AES key length for keystore.crypto.cipher. If dklen is too small (which would make macKey empty), reject the keystore with an Invalid password or malformed keystore error instead of continuing to MAC verification and decryption. Use the existing aesKeyLengthForCipher, derivedKey, and constantTimeEqual flow as the location for the guard.
🧹 Nitpick comments (1)
packages/xchain-crypto/__tests__/crypto.test.ts (1)
87-108: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider adding a v2 wrong-password rejection test.
The wrong-password test at Line 118 only exercises the v1 MAC path. Since v2 uses a 32-byte MAC key (vs 16 bytes in v1), a v2 wrong-password assertion would verify the updated MAC computation rejects incorrect passwords.
🧪 Suggested additional test
+ it('decryptFromKeystore() should reject an incorrect password for v2', async () => { + await expect(decryptFromKeystore(expectedKeystoreV2, 'wrong-password')).rejects.toThrow('Invalid password') + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/xchain-crypto/__tests__/crypto.test.ts` around lines 87 - 108, Add a negative test for encryptToKeyStore()/decryptFromKeystore() that uses the v2 aes-256-ctr fixture and a wrong password, since the existing rejection test only covers the legacy v1 path. Reuse the existing v2 test data and assert that decryptFromKeystore rejects incorrect credentials so the v2 MAC computation (with the 32-byte key path) is exercised and verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/xchain-crypto/src/crypto.ts`:
- Around line 212-226: In decryptFromKeystore, add a validation after deriving
aesKeyLength and before slicing derivedKey to ensure kdfparams.dklen is greater
than the AES key length for keystore.crypto.cipher. If dklen is too small (which
would make macKey empty), reject the keystore with an Invalid password or
malformed keystore error instead of continuing to MAC verification and
decryption. Use the existing aesKeyLengthForCipher, derivedKey, and
constantTimeEqual flow as the location for the guard.
---
Nitpick comments:
In `@packages/xchain-crypto/__tests__/crypto.test.ts`:
- Around line 87-108: Add a negative test for
encryptToKeyStore()/decryptFromKeystore() that uses the v2 aes-256-ctr fixture
and a wrong password, since the existing rejection test only covers the legacy
v1 path. Reuse the existing v2 test data and assert that decryptFromKeystore
rejects incorrect credentials so the v2 MAC computation (with the 32-byte key
path) is exercised and verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 39c6b67b-043a-43a9-b52b-688b3b0c6d1c
📒 Files selected for processing (3)
.changeset/crypto-aes256-keystore.mdpackages/xchain-crypto/__tests__/crypto.test.tspackages/xchain-crypto/src/crypto.ts
Security take: not urgentv1 is not practically “hackable” relative to v2 today. AES-128-CTR has no known practical break for this use case; the real offline risk for both formats is a weak password + stolen keystore file, not AES width. v2 mainly adds long-term/quantum margin and ~2× PBKDF2 cost per guess (
Bottom line: safe to wait if prioritization is tight — this is modernization/hygiene, not a must-ship patch. If/when it ships, call out the integrator note: upgrade all components that share keystore files before creating new ones. |
3 participants
Summary
Fixes #1720.
Upgrades
@xchainjs/xchain-cryptokeystore encryption from AES-128-CTR to AES-256-CTR, introducing keystore format version 2. The previous scheme derived 32 bytes via PBKDF2 but used only the first 16 as the AES key (aes-128-ctr), with bytes 16–32 as the MAC key. We now derive 64 bytes and split them into an independent 32-byte AES-256 key and 32-byte MAC key.What changed (
src/crypto.ts)cipheraes-128-ctr→aes-256-ctr;dklen32→64; newkeystoreVersion = 2.aesKeyLengthForCipher(cipherName)helper — maps cipher → AES key length (16 for aes-128, 32 for aes-256), throws on unsupported ciphers.encryptToKeyStore: splits the derived key intoencryptionKey = slice(0, keyLen)andmacKey = slice(keyLen, dklen), and stampsversion: 2.decryptFromKeystore: derives the same split from the keystore's owncipher/dklenfields, so the byte layout is data-driven rather than hard-coded.Notable design decisions
slice(16,32)as the MAC key would make the MAC key a substring of the encryption key — key reuse across two primitives. Insteaddklenis raised to 64 so the AES key (0–32) and MAC key (32–64) never overlap.cipher/dklen, notversion. Those fields literally determine the byte layout and are already stored in every keystore;versionis the coarse label. This is what keeps decryption of both formats correct.Backward compatibility
aes-128-ctr,dklen32) keystores decrypt unchanged — covered by tests. No migration needed for existing wallets.xchain-cryptoreleases (old code hard-codes a 16-byte key slice and throws on a 32-byte AES-256 key). Existing files are untouched. Integrators sharing keystore files across components on differentxchain-cryptoversions should upgrade them together before creating new keystores. Called out in the changeset.Tests (
__tests__/crypto.test.ts)aes-128-ctrc=600000 and the older c=262144).Export Keystoretest updated to assertaes-256-ctr/dklen 64/version 2.All 14 tests pass; build,
tsc --noEmit, and eslint are clean.Security / compatibility notes
aes-256-ctris supported bycrypto-browserify, so browser/Electron-renderer bundles are unaffected (unlike the recenttimingSafeEqualregression in xchain-crypto 1.0.7: crypto.timingSafeEqual breaks keystore decrypt in browser/bundler environments #1715).Changeset
minorbump for@xchainjs/xchain-crypto.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes