Reject out-of-range secp256k1 keys in wif and xprv apps - #71
Open
akarve wants to merge 1 commit into
Open
Conversation
The wif (2') and xprv (32') applications took a 32-byte slice of the BIP-85 entropy as a secp256k1 secret key without checking that it is a valid scalar. A value of 0 or >= the curve order n is not a usable key, and different downstream libraries would either throw, silently reduce mod n, or emit a degenerate key, so the derivation was underspecified for that (~1 in 2**127) case. Add validate_secp256k1_key() to apps/shared and call it from both apps so the derivation fails loudly and deterministically, mirroring the existing BIP-32 validate_private_child_params check. Callers should retry with the next child index. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
wif(2') andxprv(32') applications take a 32-byte slice of the BIP-85 entropy and use it directly as a secp256k1 secret key, without checking that it is a valid scalar. A secret of0or>= n(the curve order) is not a usable key. For such a value, different downstream libraries would either throw, silently reduce modn, or emit a degenerate key, so the derivation was underspecified for that case.This is astronomically rare (~1 in 2**127, the same bound BIP-32 gives), so it will realistically never occur in practice. But it should fail loudly and deterministically rather than depend on the consumer, and I was surprised we didn't already do this given the analogous BIP-32 check already in the codebase.
Changes
validate_secp256k1_key()toapps/shared.py. It raisesValueErrorif the 32-byte value is0or>= SECP256k1.order, mirroring the wording of the existingvalidate_private_child_paramsinbip32.py. The caller should retry with the next child index.WifApp.apply(entropy[:32], the secret exponent) andXprvApp.apply(entropy[32:], the private key).TestSecp256k1Validationwith boundary cases (1,n-1accepted;0,n,n+1,2**256-1rejected) and app-level cases that a crafted entropy makeswif/xprvraise.The existing test vectors are unaffected (their keys are all in range), and the full suite plus
make checkpass.Note
This intentionally does not touch the Nostr application in #70, which is not on
mainyet and produces a secp256k1 key the same way. If this lands, #70 should reusevalidate_secp256k1_keyfor the same reason. It would be good to settle the policy once and keep the BIP-85 text (bitcoin/bips#2126) consistent with it.Test plan
poetry run pytest -m ""(full suite passes)make check(black, isort, flake8, version-sync all clean)Made with Cursor