fix: isValidXprv accepts a mistyped extended key and silently imports a different wallet #210

New Issue

clawbot · 2026-08-11T14:32:45+02:00

clawbot commented

2026-08-11 14:32:45 +02:00

isValidXprv accepts an extended private key with a one-character typo. importXprvKey (gated on it at src/popup/views/addWallet.js:215) then creates a different wallet with no error shown.

Root cause: ethers' HDNodeWallet.fromExtendedKey skips base58 checksum verification whenever the decoded payload is the usual 82 bytes — exactly the case the checksum exists to catch.

Measured against the BIP-32 test vector 1 key: changing any one of the last 14 characters passes validation, and 9 of those 14 positions produce a different wallet. Example — correct key gives 0x022b971dFF0C43305e691DEd7a14367AF19D6407; a typo at position -6 gives 0x3F334f0a356d6B46B1d70B590E7437D77100d28D.

Consequence: a user importing an xprv and mistyping one character is shown a successful import of an empty wallet that is not theirs. There is no error and nothing indicates the key was wrong. They will reasonably conclude their funds are gone. The base58 checksum exists precisely to make this impossible, and it is not being enforced.

Found while adding crypto-core coverage in #159, which left a skipped test naming this defect.

Implementation requirements

Verify the base58check checksum in isValidXprv before accepting an extended key — e.g. require encodeBase58Check(bytes.slice(0, 78)) === key — rather than relying on ethers to do it.
Do not rely on fromExtendedKey throwing; it demonstrably does not for this input class.
Check whether isValidXpub and any other extended-key entry point have the same hole, and fix them the same way if so.
The import screen must show a full-sentence error per the README Language & Labeling rules, e.g. "That extended private key is not valid. Please check it and try again."

Definition of done

A one-character typo in any position of a valid xprv is rejected by isValidXprv.
The import screen shows a full-sentence error for a mistyped key and creates no wallet.
A genuinely valid xprv still imports and derives the same addresses as before.
The skipped test rejects an extended key with a one-character typo in tests/wallet.test.js is unskipped and passes.
Coverage includes typos across the full key, not just the tail, and the xpub path if it shares the defect.
TODO.md updated in the same commit.
make check passes.

`isValidXprv` accepts an extended private key with a one-character typo. `importXprvKey` (gated on it at `src/popup/views/addWallet.js:215`) then creates a **different wallet** with no error shown. Root cause: ethers' `HDNodeWallet.fromExtendedKey` skips base58 checksum verification whenever the decoded payload is the usual 82 bytes — exactly the case the checksum exists to catch. Measured against the BIP-32 test vector 1 key: changing any one of the last 14 characters passes validation, and 9 of those 14 positions produce a different wallet. Example — correct key gives `0x022b971dFF0C43305e691DEd7a14367AF19D6407`; a typo at position -6 gives `0x3F334f0a356d6B46B1d70B590E7437D77100d28D`. Consequence: a user importing an xprv and mistyping one character is shown a successful import of an empty wallet that is not theirs. There is no error and nothing indicates the key was wrong. They will reasonably conclude their funds are gone. The base58 checksum exists precisely to make this impossible, and it is not being enforced. Found while adding crypto-core coverage in https://git.eeqj.de/sneak/AutistMask/issues/159, which left a skipped test naming this defect. ## Implementation requirements - Verify the base58check checksum in `isValidXprv` before accepting an extended key — e.g. require `encodeBase58Check(bytes.slice(0, 78)) === key` — rather than relying on ethers to do it. - Do not rely on `fromExtendedKey` throwing; it demonstrably does not for this input class. - Check whether `isValidXpub` and any other extended-key entry point have the same hole, and fix them the same way if so. - The import screen must show a full-sentence error per the README Language & Labeling rules, e.g. "That extended private key is not valid. Please check it and try again." ## Definition of done - [ ] A one-character typo in any position of a valid xprv is rejected by `isValidXprv`. - [ ] The import screen shows a full-sentence error for a mistyped key and creates no wallet. - [ ] A genuinely valid xprv still imports and derives the same addresses as before. - [ ] The skipped test `rejects an extended key with a one-character typo` in `tests/wallet.test.js` is unskipped and passes. - [ ] Coverage includes typos across the full key, not just the tail, and the xpub path if it shares the defect. - [ ] `TODO.md` updated in the same commit. - [ ] `make check` passes.

clawbot added this to the 1.0.0 milestone 2026-08-11 14:32:45 +02:00

clawbot commented

2026-08-11 14:44:45 +02:00

Scope extension — a second xprv import defect in the same code, found by the independent review of #209. Fix both in this unit, since they touch the same functions.

hdWalletFromXprv / getSignerForAddress derive using a RELATIVE path (44'/60'/0'/0). For a depth-0 master xprv that is correct. But importing a NON-master extended key — an account-level xprv at depth 3, which is a normal thing for a user to have — derives 44'/60'/0'/0 BENEATH it rather than rejecting it. The result is a wallet whose addresses correspond to nothing the user holds, created silently, with no error.

Same user-visible failure as the checksum hole already described here: an apparently successful import of a wallet that is not theirs. Nothing currently tests it.

Additional definition of done:

An extended key with depth > 0 is either rejected with a full-sentence error, or handled correctly and deliberately — decide which, and say why in the PR.
If rejected: the import screen explains that an account-level or child key is not supported, rather than failing opaquely.
Tests cover a depth-0 master key (accepted, unchanged behaviour) and a depth-3 account-level key (whatever you decided), demonstrated failing against the current code first.

Scope extension — a second xprv import defect in the same code, found by the independent review of https://git.eeqj.de/sneak/AutistMask/pulls/209. Fix both in this unit, since they touch the same functions. `hdWalletFromXprv` / `getSignerForAddress` derive using a RELATIVE path (`44'/60'/0'/0`). For a depth-0 master xprv that is correct. But importing a NON-master extended key — an account-level xprv at depth 3, which is a normal thing for a user to have — derives `44'/60'/0'/0` BENEATH it rather than rejecting it. The result is a wallet whose addresses correspond to nothing the user holds, created silently, with no error. Same user-visible failure as the checksum hole already described here: an apparently successful import of a wallet that is not theirs. Nothing currently tests it. Additional definition of done: - [ ] An extended key with depth > 0 is either rejected with a full-sentence error, or handled correctly and deliberately — decide which, and say why in the PR. - [ ] If rejected: the import screen explains that an account-level or child key is not supported, rather than failing opaquely. - [ ] Tests cover a depth-0 master key (accepted, unchanged behaviour) and a depth-3 account-level key (whatever you decided), demonstrated failing against the current code first.

clawbot referenced this issue

2026-08-11 14:49:54 +02:00

test: known-answer coverage for HD derivation and the vault (closes #159) #209

clawbot referenced this issue

2026-08-11 15:03:52 +02:00

test: known-answer coverage for HD derivation and the vault (closes #159) #209

clawbot referenced this issue from a commit

2026-08-11 15:14:59 +02:00

fix: enforce the base58 checksum and reject non-master extended keys (closes #210)

clawbot referenced a pull request that will close this issue

2026-08-11 15:15:50 +02:00

fix: enforce the base58 checksum and reject non-master extended keys (closes #210) #232