bip32: add some of the new bip32 checks to bip32_key_unserialize

Elements Project/libwally-core

2b74c8d7 View source ↗

Public commit record

What the developer wrote

Authored by Ondřej Češka

50/100 · Thin

bip32: add some of the new bip32 checks to bip32_key_unserialize

✓ Specific, descriptive subject✓ Names a concrete action or component! No meaningful explanatory body

The short version

What changed, and why it matters

This commit adds a safety check when loading BIP32 extended keys. Before, the code accepted any 33-byte public key bytes without verifying they form a valid public key. Now it calls a public-key validation function and rejects invalid keys. The change is defensive: it prevents malformed or attacker-chosen public keys from being treated as legitimate BIP32 keys later. The commit also adds test vectors for many other invalid key types, though only the public-key validation is newly enforced in this patch.

Recommended action

Treat as a security hardening improvement and include in release notes. Review whether the remaining commented-out Test Vector 5 checks (depth/fingerprint/index consistency and unknown versions) should also be enforced. No urgent advisory is required unless downstream callers relied on accepting invalid public keys.

Security signals we found

New input-validation call (wally_ec_public_key_verify) added to deserialization path

Rejection of invalid public-key bytes before key material is copied into output struct

Addition of BIP32 Test Vector 5 invalid-key test cases

Defensive hardening of BIP32 key parsing

Technical analysis

Evidence from the diff

In bip32_key_unserialize(), after detecting a public-key version and before copying the public key into the output ext_key, the code now calls wally_ec_public_key_verify(bytes, sizeof(key_out->pub_key)). If validation fails, the function wipes the partially built key and returns failure. This closes a gap where previously any 33-byte value (including invalid curve points or non-standard prefixes) could be loaded as a BIP32 public key. The test file imports BIP32 Test Vector 5 invalid keys and asserts bip32_key_from_base58_alloc returns WALLY_EINVAL for them. The commit message and comments do not frame this as a CVE or credit an external reporter.

Changed components

src/bip32.c:bip32_key_unserialize()src/bip32.c:bip32_key_from_base58_alloc() (caller of unserialize)src/test/test_bip32.py

Inspect captured patch +65 / −0

diff --git a/src/bip32.c b/src/bip32.c
index 55b08ee..b39a3b8 100644
--- a/src/bip32.c
+++ b/src/bip32.c
@@ -577,6 +577,9 @@ int bip32_key_unserialize(const unsigned char *bytes, size_t bytes_len,
             key_out->version == BIP32_VER_TEST_PRIVATE)
             return wipe_key_fail(key_out); /* Public key data in private key */
 
+        if (wally_ec_public_key_verify(bytes, sizeof(key_out->pub_key)) != WALLY_OK)
+            return wipe_key_fail(key_out); /* Invalid public key */
+
         copy_in(key_out->pub_key, bytes, sizeof(key_out->pub_key));
         bip32_key_strip_private_key(key_out);
     }
diff --git a/src/test/test_bip32.py b/src/test/test_bip32.py
index cef95d7..b62c769 100755
--- a/src/test/test_bip32.py
+++ b/src/test/test_bip32.py
@@ -389,6 +389,68 @@ class BIP32Tests(unittest.TestCase):
         self.do_test_vector(vec_1)
         self.do_test_vector(vec_3)
 
+    def test_vector_5_invalid_keys(self):
+        """BIP32 Test Vector 5: invalid extended keys must be rejected"""
+        invalid_keys = [
+            # pubkey version / prvkey mismatch
+            'xpub661MyMwAqRbcEYS8w7XLSVeEsBXy79zSzH1J8vCdxAZningWLdN3'
+            'zgtU6LBpB85b3D2yc8sfvZU521AAwdZafEz7mnzBBsz4wKY5fTtTQBm',
+            # prvkey version / pubkey mismatch
+            'xprv9s21ZrQH143K24Mfq5zL5MhWK9hUhhGbd45hLXo2Pq2oqzMMo63o'
+            'StZzFGTQQD3dC4H2D5GBj7vWvSQaaBv5cxi9gafk7NF3pnBju6dwKvH',
+            # invalid pubkey prefix 04
+            'xpub661MyMwAqRbcEYS8w7XLSVeEsBXy79zSzH1J8vCdxAZningWLdN3'
+            'zgtU6Txnt3siSujt9RCVYsx4qHZGc62TG4McvMGcAUjeuwZdduYEvFn',
+            # invalid prvkey prefix 04
+            'xprv9s21ZrQH143K24Mfq5zL5MhWK9hUhhGbd45hLXo2Pq2oqzMMo63o'
+            'StZzFGpWnsj83BHtEy5Zt8CcDr1UiRXuWCmTQLxEK9vbz5gPstX92JQ',
+            # invalid pubkey prefix 01
+            'xpub661MyMwAqRbcEYS8w7XLSVeEsBXy79zSzH1J8vCdxAZningWLdN3'
+            'zgtU6N8ZMMXctdiCjxTNq964yKkwrkBJJwpzZS4HS2fxvyYUA4q2Xe4',
+            # invalid prvkey prefix 01
+            'xprv9s21ZrQH143K24Mfq5zL5MhWK9hUhhGbd45hLXo2Pq2oqzMMo63o'
+            'StZzFAzHGBP2UuGCqWLTAPLcMtD9y5gkZ6Eq3Rjuahrv17fEQ3Qen6J',
+            # Note we don't currently fail keys where the fingerprint
+            # and index are inconsistent, as some implementations do
+            # not set them correctly (but nonetheless work for their
+            # intended purpose(s)).
+            # zero depth with non-zero parent fingerprint (xprv)
+            #'xprv9s2SPatNQ9Vc6GTbVMFPFo7jsaZySyzk7L8n2uqKXJen3KUmvQNT'
+            #'uLh3fhZMBoG3G4ZW1N2kZuHEPY53qmbZzCHshoQnNf4GvELZfqTUrcv',
+            # zero depth with non-zero parent fingerprint (xpub)
+            #'xpub661no6RGEX3uJkY4bNnPcw4URcQTrSibUZ4NqJEw5eBkv7ovTwgi'
+            #'T91XX27VbEXGENhYRCf7hyEbWrR3FewATdCEebj6znwMfQkhRYHRLpJ',
+            # zero depth with non-zero index (xprv)
+            #'xprv9s21ZrQH4r4TsiLvyLXqM9P7k1K3EYhA1kkD6xuquB5i39AU8KF4'
+            #'2acDyL3qsDbU9NmZn6MsGSUYZEsuoePmjzsB3eFKSUEh3Gu1N3cqVUN',
+            # zero depth with non-zero index (xpub)
+            #'xpub661MyMwAuDcm6CRQ5N4qiHKrJ39Xe1R1NyfouMKTTWcguwVcfrZJ'
+            #'aNvhpebzGerh7gucBvzEQWRugZDuDXjNDRmXzSZe4c7mnTK97pTvGS8',
+            # unknown extended key version (1)
+            'DMwo58pR1QLEFihHiXPVykYB6fJmsTeHvyTp7hRThAtCX8CvYzgPcn8X'
+            'nmdfHGMQzT7ayAmfo4z3gY5KfbrZWZ6St24UVf2Qgo6oujFktLHdHY4',
+            # unknown extended key version (2)
+            'DMwo58pR1QLEFihHiXPVykYB6fJmsTeHvyTp7hRThAtCX8CvYzgPcn8X'
+            'nmdfHPmHJiEDXkTiJTVV9rHEBUem2mwVbbNfvT2MTcAqj3nesx8uBf9',
+            # private key 0 not in 1..n-1
+            'xprv9s21ZrQH143K24Mfq5zL5MhWK9hUhhGbd45hLXo2Pq2oqzMMo63o'
+            'StZzF93Y5wvzdUayhgkkFoicQZcP3y52uPPxFnfoLZB21Teqt1VvEHx',
+            # private key n not in 1..n-1
+            'xprv9s21ZrQH143K24Mfq5zL5MhWK9hUhhGbd45hLXo2Pq2oqzMMo63o'
+            'StZzFAzHGBP2UuGCqWLTAPLcMtD5SDKr24z3aiUvKr9bJpdrcLg1y3G',
+            # invalid pubkey 020000000000000000000000000000000000000000000000000000000000000007
+            'xpub661MyMwAqRbcEYS8w7XLSVeEsBXy79zSzH1J8vCdxAZningWLdN3'
+            'zgtU6Q5JXayek4PRsn35jii4veMimro1xefsM58PgBMrvdYre8QyULY',
+            # invalid checksum
+            'xprv9s21ZrQH143K3QTDL4LXw2F7HEK3wJUD2nW2nRk4stbPy6cq3jPP'
+            'qjiChkVvvNKmPGJxWUtg6LnF5kejMRNNU3TGtRBeJgk33yuGBxrMPHL',
+        ]
+
+        key_out = POINTER(ext_key)()
+        for base58_key in invalid_keys:
+            ret = bip32_key_from_base58_alloc(utf8(base58_key), byref(key_out))
+            self.assertEqual(ret, WALLY_EINVAL)
+
     def do_test_vector(self, vec):
 
         # BIP32 Test vector 1

Risk score

Why this scored 59/100

Our methodology →

Potential impact 18/30

Exploitability 12/25

Stealth signal 8/15

Affected reach 10/15

Confidence 7/10

Evidence quality 4/5

Human-validated context

Community notes

Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.

No validated notes yet.

The AI analysis stands alone for now. Submit a note if you can add evidence or important context.

What the developer wrote

What changed, and why it matters

Security signals we found

Evidence from the diff

Changed components

Why this scored 59/100

Community notes

Related watches

crypto: add ec_public_key_compress to produce a compressed pubkey

sign: disallow overlong der encodings and zero r or s values

build: add fuzzing infrastructure for security testing