test: known-answer coverage for HD derivation and the vault (closes #159) #209
Reference in New Issue
Block a user
Delete Branch "test/issue-159-crypto-core-tests"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #159. Tests only — no
source file is modified.
What is covered
tests/wallet.test.js(extended around the six existingDEBUG-flag tests,which are untouched) and
tests/vault.test.js(new).wallet.js, pinned to published vectors rather than to what the code returnstoday:
hdWalletFromMnemonic— first address for two independent phrases; theaccount-level xpub is watch-only; the account path is
m/44'/60'/0'/0; aninvalid phrase throws.
deriveAddressFromXpub— children 0, 1, 2 against published addresses, andagreement with
hdWalletFromMnemonicat index 0; garbage throws.hdWalletFromXprv— master xprv for the same phrase reaches the samepublished address, and
toEqualagainsthdWalletFromMnemonic's wholeresult; its xpub generates the same three children; a watch-only xpub and
garbage are rejected.
isValidMnemonic/isValidXprv— valid, bad checksum, 11 words, off-wordlistword, empty, garbage, bare private key, truncated and over-long keys.
addressFromPrivateKey— the three published key/address pairs; wrong lengthand empty string throw.
getSignerForAddress— all three wallet types at indices 0, 1, 2 (address andprivate key), and a signature that recovers to the expected address.
a different address, so the empty string in
hdWalletFromMnemonicis pinnedas load-bearing.
vault.js:not act as a skeleton key in either direction).
error.
flipped nonce bit, flipped salt bit, truncated ciphertext, ciphertext shorter
than the tag, truncated nonce, a ciphertext spliced in from another vault, and
each field missing.
describe("key derivation cost")) — see below.password differ in all three fields and both still decrypt.
{ salt, nonce, ciphertext }, all base64, salt 16 bytes, nonce24 bytes, ciphertext = plaintext length + 16-byte tag, survives
JSON.parse(JSON.stringify(...)).appears in the serialized blob, and the ciphertext bytes do not contain the
plaintext bytes.
Production Argon2id parameters are not weakened or overridden. The tamper cases
share one encrypted fixture instead of re-encrypting per test; the whole suite
runs in ~5s against
script/test's 30-second budget.Argon2id cost is now pinned
The KDF cost is the whole of the vault's resistance to offline brute force on a
stolen blob, and lowering it breaks nothing else the suite can observe — it just
runs faster. Three tests pin it from independent angles, so no single edit slips
through:
the interactive constants still mean 2 passes over 64 MiB— pinscrypto_pwhash_OPSLIMIT_INTERACTIVE/MEMLIMIT_INTERACTIVEto theirabsolute values (2 and 67108864), so a libsodium upgrade that redefined the
constants downward would also be caught, and asserts the
_MINfloor isstrictly below both.
a key derived at the interactive parameters opens the vault— spy-free andindependent of the module's code path: derives a key in the test from the
vault's own published salt at the interactive cost and opens the vault's
ciphertext with
crypto_secretbox_open_easydirectly. A vault built at anyother opslimit, memlimit or Argon2id variant yields a different key and
cannot be opened this way.
encrypt/decryptderives exactly one key at the interactive cost—observes the actual
crypto_pwhashcall from each direction and asserts keylength, salt length, opslimit, memlimit and
ALG_ARGON2ID13.Mutant evidence
Mutation applied with the editor (no scripted substitution), all four
occurrences in
src/shared/vault.js:24-25,51-52changed from the_INTERACTIVEconstants to the
_MINconstants — opslimit 2 to 1, memlimit 64 MiB to 8 KiB,an 8192x memory reduction.
Before, on the previous head (
16a3d3c), the mutant was invisible — all 217tests passed, the suite merely ran faster (1.77s vs 4.23s).
After, on this head:
The mutation was then reverted with the editor and
git diff src/confirmedempty before committing; this PR remains tests-only.
Test vector sources
test test test test test test test test test test test junkand its firstthree accounts at
m/44'/60'/0'/0/nwith an empty seed passphrase(
0xf39Fd6e5...,0x70997970...,0x3C44CdDd...) plus their private keys —the standard development phrase published in the Hardhat and Ganache
documentation. Publicly known; never funded.
abandon abandon ... about(BIP-39 all-zero-entropy vector, first entry of theofficial vector set) with
0x9858EfFD232B4033E47d90003D41EC34EcaEda94atm/44'/60'/0'/0/0.xprv9s21ZrQH143K3QTDL..., seed000102030405060708090a0b0c0d0e0f).The two Hardhat facts cross-check each other: the published private key for
account n must yield the same address the HD path reaches, so a wrong path and a
wrong key-to-address step cannot cancel out.
The
m/prefix asymmetry: proven harmlesshdWalletFromMnemonic:25derives the absolutem/44'/60'/0'/0whilehdWalletFromXprv:36derives the relative44'/60'/0'/0. For a depth-0 masterkey these are the same derivation. Pinned by test: for the same phrase, the xprv
path and the mnemonic path produce the identical xpub, the identical first
address, and the identical children 0-2, all equal to the published vectors. The
only behavioural difference is that the relative form would also accept a
non-master extended key and derive beneath it; the absolute form would not.
Defect found — reported, not fixed
isValidXprvaccepts an extended private key containing a one-character typo,and the import flow (
src/popup/views/addWallet.js:215gates on it) thensilently creates a different wallet. ethers'
HDNodeWallet.fromExtendedKeyskips base58 checksum verification whenever the decoded payload is the usual 82
bytes, which is exactly what that checksum exists to catch.
Measured on the BIP-32 vector 1 key: changing any one of the last 14 characters
passes
isValidXprv, and for 9 of those 14 positions the derived walletdiffers, e.g.
No error is shown; the user sees an empty wallet and no indication that their
key was mistyped. Out of scope for this PR, which is tests only. It is now
tracked as #210, which the
skipped test in
tests/wallet.test.jscites; that test asserts the correctbehaviour and is to be unskipped when the validation is fixed. Everything else
passes.
Mutation evidence (derivation and vault)
Each mutation was applied to the source, the suite run, then the source
restored, with
git diffverified clean before commit.wallet.js,hdWalletFromMnemonicchild index0->1— 4 failures:wallet.js, seed passphrase""->"x"— 6 failures:wallet.js, xprv path44'/60'/0'/0->44'/60'/1'/0— 3 failures:vault.js, fixed all-zero nonce — 1 failure:vault.js, plaintext copied into the stored blob as an extra field — 2failures:
On the auth-tag layer: a realistic skipped-verification mutant (true XSalsa20
keystream XOR with no MAC check, confirmed to act as a decryption oracle) trips
5 tests, including
a flipped ciphertext bit is rejected by the auth taganda flipped bit in the authentication tag itself is rejected. The tamper testsdo discriminate the tag layer; no dedicated auth-tag test is needed.
Verification
Rebased onto current
next(cf5f582); theTODO.mdCompleted Steps conflictwas resolved keeping all sides' entries. One commit, tests only, base
next,git merge-treeclean againstnext.make checkon the rebased branch:And in the container on this exact head,
docker build --no-cacheon thisimage only, so the step demonstrably executed rather than reporting
CACHED:The two README testing checkboxes for mnemonic/address derivation and xpub child
generation are ticked, and
TODO.mdgains one line, in the same commit.wallet.js and vault.js — the two modules that hold user funds — had no derivation or encryption tests. Add them, pinned to published vectors rather than to whatever the implementation returns today. wallet.js: hdWalletFromMnemonic, hdWalletFromXprv, deriveAddressFromXpub and getSignerForAddress are pinned to the standard development recovery phrase's first three accounts at m/44'/60'/0'/0/n and to the BIP-39 all-zero-entropy phrase's first address; addressFromPrivateKey is pinned to the published key/address pairs, so the HD path and the bare-key path must meet at the same address from two directions. isValidMnemonic and isValidXprv cover bad checksum, wrong word count, wrong key type and empty/garbage input. The absolute-vs-relative path asymmetry between hdWalletFromMnemonic and hdWalletFromXprv is proven harmless: for the same master key both reach the same xpub and the same addresses. vault.js: round trip (including non-ASCII and an empty password), wrong password rejected as a rejected promise with no partial plaintext, tampered ciphertext / auth tag / nonce / salt rejected, truncated and spliced blobs rejected, missing fields rejected, fresh salt and nonce per encryption, the documented { salt, nonce, ciphertext } shape, and no trace of the plaintext or password anywhere in the serialized blob. The production Argon2id parameters are not weakened; the tamper cases share one encrypted fixture to stay inside script/test's 30-second budget. One test is skipped: isValidXprv accepts an extended private key with a one-character typo, because ethers skips base58 checksum verification for the usual 82-byte payload. That is a defect to be filed separately, not fixed here; the skipped test asserts the correct behaviour and names the reason. Suite: 211 passed, 1 skipped, 7.2s inside the container build.FAIL —
needs-rebase.Blocking
1. Not mergeable. Branch is based on
19cb1ca;nextis nowb9bc226.git merge-treegivesCONFLICT (content): Merge conflict in TODO.md— bothsides added a "Completed Steps" entry at the same position. Rebase onto current
nextand re-push.2. CI never ran on the head commit.
check / check (push)on16a3d3chasbeen
pending/ "Waiting to run" since 14:28:50. Nothing is red, but nothing isgreen either. I ran
docker build --no-cacheon the head myself and stepRUN make checkexecuted in 18.2s uncached — 9 suites, 217 passed, 1 skipped,prettier clean — so this is a queue problem, not a code problem. It still needs
a green run before merge.
Finding — the Argon2id cost parameters are unpinned
tests/vault.test.jspins every vault property except the one that decideswhether a stolen vault survives an offline attack.
Changing
src/shared/vault.js:24-25,51-52fromcrypto_pwhash_OPSLIMIT_INTERACTIVE/MEMLIMIT_INTERACTIVEto the_MINconstants — opslimit 2 to 1, memlimit 64 MiB to 8 KiB, an 8192x memory
reduction — leaves all 217 tests passing. The suite even gets faster
(1.77s vs 4.23s), which is the only signal it produces.
Why it matters: the KDF cost is the whole of the vault's resistance to offline
brute force on a stolen blob. A future edit that guts it is precisely the
silent-failure class this file's own header says it exists to catch, and it is
the one case that walks straight through. The PR's statement that production
parameters are not weakened is accurate about what the tests do; the gap is
the absence of any test that would notice a later weakening.
Acceptable: assert the derived key against a published Argon2id known-answer
vector, or at minimum assert the module derives with the INTERACTIVE
opslimit/memlimit constants rather than whatever is currently passed.
On the disclosed auth-tag bound — no action needed
The disclosure understates the coverage. The mutant used (returning the raw
unverified ciphertext body) is not a shape a real bug takes, and its non-UTF-8
output is the only reason just 2 tests tripped.
Substituting a realistic skipped-verification mutant — true XSalsa20 keystream
XOR with no MAC check, confirmed to be a genuine decryption oracle (a flipped
ciphertext bit returns
"test!test test test..."successfully, no error) —trips 5 tests, including both
a flipped ciphertext bit is rejected by the auth taganda flipped bit in the authentication tag itself is rejected. Thetamper tests do discriminate the tag layer. A dedicated auth-tag test is not
required before this lands.
Non-blocking
The skipped test's comment names no issue number for the
isValidXprvdefect("to be filed as its own issue"). Once that issue exists the comment should cite
it, or the skip has no tracking anchor.
Verified
Vector provenance is genuine known-answer, not self-consistency: recomputed from
first principles with a hand-rolled BIP-39 PBKDF2 + BIP-32 CKDpriv over noble
secp256k1/keccak, touching no ethers HD code — all three development-phrase
addresses and private keys, the zero-entropy address, and the BIP-32 vector-1
master xprv rebuilt from seed
000102030405060708090a0b0c0d0e0f— andcross-checked against the published Hardhat reference and the BIP-32 spec text.
Mutants caught: coin type 12, account index 12, change index 12, xprv relative
path 3, non-empty seed passphrase 6, xpub child off-by-one 5, dropped EIP-55
checksum casing 3, fixed nonce 1.
The
isValidXprvtypo defect reproduces exactly as characterised: 14 of 14single-character edits to the last 14 characters pass validation, 9 of 14 yield
a different wallet, and the quoted addresses match mine.
Six pre-existing DEBUG tests byte-identical (only the file header comment
changed); README checkboxes accurate; one
TODO.mdentry; single commit ending(closes #159); basenext; tests only, no source change; no attributiontrailers;
make checkgreen on host and in-container.16a3d3cefcto2dcea6c306FAIL —
needs-rebase. The substance passes; mergeability does not.Blocking: conflicts with current
next(f271bcd). Branch is 1 ahead / 3 behind;git merge origin/nextgives a content conflict inTODO.mdCompleted Steps, where the #159 entry sits adjacent to the #166 and #180 entries thatnextgained after the rebase ontocf5f582. No entries are lost on either side — interleave and force-push. Tracker agrees (mergeable: false).Argon2id pin independently verified — no partial mutant survives. Each mutation applied to
src/shared/vault.jswith the editor, suite run viamake test, then reverted with the tree re-confirmed clean: all four_INTERACTIVEto_MINgives 3 failures (matches the claim); opslimit alone, 3; memlimit alone, 3;ALG_ARGON2ID13toALG_ARGON2I13at opslimit 3, i.e. a valid non-crashing Argon2i vault, 3; salt 16 to 8 bytes, 28 (libsodium rejects the length outright, so this one is a crash-level catch rather than a discriminating one); asymmetric encrypt-at-interactive / decrypt-at-_MIN, 6.The three angles are not three spellings of one assertion. The spy-free test hardcodes the literals
2and67108864rather than readingsodium.crypto_pwhash_*_INTERACTIVE, so it genuinely fails for a vault built at any other cost or variant rather than re-deriving with whatever the source currently uses — confirmed by the Argon2i mutant, which produces a perfectly functional vault at a higher opslimit and is still caught. It correctly stays green on the decrypt-only mutant, which the spy test catches instead; the two cover opposite directions.make checkon2dcea6c: 9 suites, 221 passed, 1 skipped, prettier clean on both lint and fmt-check. Tests-only confirmed —git diff --name-only cf5f582..HEAD -- src/is empty and no trace of the author's evidence mutations survives. Single commit, title ends(closes #159), basenext, authoredclawbot, no attribution trailers, no inclusive-terminology hits; the skipped xprv test cites #210.Disclosure: the pre-rebase head
16a3d3cis no longer fetchable, so I could not byte-compare the HD vectors against their pre-rebase form; instead I confirmed they are the published values and thatTODO.mdlost nothing relative to its basecf5f582. This repo'sscript/lintruns prettier on the host by design, not in a container; I invoked it only throughmake check.2dcea6c306to22cf2f43f5