Fix: pass an explicit byte width where a fixed-length value is required

[RexStarBSV]

CONTRIBUTING.md asks that a change be discussed in an issue first. I have written this up as if it were that issue, so everything needed to judge it is here; if you would rather start with an issue and leave the code for later, say so and I will open one and close this.

Summary

integer_to_bytes(data) called without bytes_num takes its width from the value:

bytes_num = bytes_num or ((data.bit_length() if data > 0 else 1) + 7) // 8

That is the documented behaviour and it is right for the flags, magics and prefixes that make up most of its callers. But twelve call sites in bip38/bip38.py use it to build values that must be exactly 16 or 32 bytes. When such a value happens to begin with 0x00 it comes back 15 or 31 bytes, and the caller's length assumption breaks. Each affected value has a 1 in 256 chance of starting with 0x00.

The symptom depends on where the short value lands:

Symptom	Where
ValueError: plaintext block must be 16 bytes	encrypt(), create_new_encrypted_wif() — the short block reaches AES
Secp256k1Error: Invalid private key bytes	decrypt() — the recovered key is 31 bytes
PassphraseError: Incorrect passphrase	decrypt(), confirm_code() — the short value is silently mis-sliced, and nothing notices until the address comparison at the end

The third row is the one I would draw attention to: the passphrase is correct, and the library says it is not.

The non-EC decrypt() case is worth separate mention because it is not transient. The trigger there is the private key's own first byte being 0x00, which is a property of the key, so an affected key encrypts without complaint and then cannot be decrypted again — not with a retry, not with any passphrase.

Reproduction

Public API only, fixed inputs, correct passphrase in every case, library defaults. All five inputs are vectors this PR adds to tests/data/values.json.

from bip38 import BIP38
from bip38.cryptocurrencies import Bitcoin

bip38 = BIP38(cryptocurrency=Bitcoin, network="mainnet")
failures = 0


def case(name, expected, call):
    global failures
    try:
        got = call()
    except Exception as ex:
        failures += 1
        print(f"FAIL  {name}\n        expected: {expected}\n"
              f"        got:      {type(ex).__name__}: {ex}")
        return
    if got != expected:
        failures += 1
        print(f"FAIL  {name}\n        expected: {expected}\n        got:      {got}")
    else:
        print(f"ok    {name}")


# 1. encrypt(), 0x0142. private_key[0:16] ^ derived_half_1[0:16] begins with
#    0x00, so a 15-byte block reaches AES.
case("encrypt() 0x0142", "6PYMkqeQ5kS3w3z3Mq1BmckT1kGQrFdEaAQXEAzKriqck6EmTmyujxh8Zx",
     lambda: bip38.encrypt(
         wif="L1LnVsrt7h15oRcoqRrAtHbxqgYMXGpi7ELKCvxs7P231WGo4VhJ",
         passphrase="TestingOneTwoThree"))

# 2. decrypt(), 0x0142. The private key itself begins with 0x00. 1.4.1 produces
#    this encrypted WIF and then cannot read it back - with any passphrase,
#    ever, because the trigger is a property of the key.
case("decrypt() 0x0142", "KwDmZ7ZitK4CSmS8LctpMAbJpGU4qbNm4GeVwmfxfQxQ3SDh1ZU8",
     lambda: bip38.decrypt(
         encrypted_wif="6PYPSxToG1Mz2a2NCFjqCQ9Fp5YmUYrwLLFY3B1p1oTC1fNFMj4v7hg8ZT",
         passphrase="TestingOneTwoThree"))

# 3. create_new_encrypted_wif(), 0x0143. seed_b[:16] ^ derived_half_1 begins
#    with 0x00.
case("create_new_encrypted_wif() 0x0143",
     "6PnZAmv15AX5PV9Wnw8p3iAt1qMK3a36QjZxwqtDPayfEtcY6zNgB1w1G5",
     lambda: bip38.create_new_encrypted_wif(
         intermediate_passphrase="passphraserDFxboKK9cTkBQMb73vdzgsXB5L6cCMFCzTVoMTpMWYD8SJXv3jcKyHbRWBcza",
         wif_type="wif-compressed",
         seed="a38986efae3c7f8b13e8c797e7debb253f9ee6ce71b954ad")["encrypted_wif"])

# 4. confirm_code(), 0x0143. A half of point_b begins with 0x00, the rebuilt
#    public key is 32 bytes instead of 33, and the correct passphrase is
#    reported as incorrect. 1.4.1 produced this confirmation code itself.
case("confirm_code() 0x0143", "1FX6wgfFVT5GrgQVZt7a18CNmxNaKjo8dU",
     lambda: bip38.confirm_code(
         passphrase="MOLON LABE",
         confirmation_code="cfrm38VUJkJrqyjo6FaV5mSZfjbhLGrPZna93Nrbq51FLU2rMjnrDte8rVEuqn3j5KiZKy4s8c4"))

# 5. decrypt(), 0x0143. seed_b begins with 0x00, so the seed is rebuilt one
#    byte short and the address never matches. 1.4.1 produced this WIF itself.
case("decrypt() 0x0143", "00b726e7be80ddb389866b2769ae533d1a5c0f9b88cb1982",
     lambda: bip38.decrypt(
         encrypted_wif="6PnRgbHq8rHEE7dR4ZhYxB8D5RvWD3UketbS3Dg4jFDPY7a2e7vjf6PsTe",
         passphrase="MOLON LABE", detail=True)["seed"])

print(f"\n{failures} of 5 cases failed")
raise SystemExit(1 if failures else 0)

On 1.4.1 this prints 5 of 5 cases failed. With this change it prints 0 of 5 cases failed.

In cases 2, 4 and 5 the ciphertext is what 1.4.1 itself emits — I generated each with the unpatched library and compared byte for byte. Those are cases where the library writes output it cannot read back.

How often

Each fixed-width value has a 1 in 256 chance of beginning with 0x00, so for a call that builds k of them the failure rate is 1 - (255/256)^k. Counting k per entry point gives:

Call	k	Predicted	Measured, 10,000 uniform samples
encrypt() 0x0142	2	1 in 128	1 in 99
decrypt() 0x0142	1	1 in 256	1 in 309
create_new_encrypted_wif() 0x0143	4	1 in 64	1 in 67
confirm_code() 0x0143	2	1 in 128	1 in 137
decrypt() 0x0143	3	1 in 86	1 in 89

The measured column is a single 10,000-sample run per path, so the sampling error is visible — a second 30,000-sample run of encrypt() gave 1 in 152, and the two runs pooled give 1 in 134 against the predicted 1 in 128. The model is the reliable number; the samples are there to show it holds. Sampling used deliberately weak scrypt (N=16, r=8, p=1) to make the sample sizes affordable; the trigger is the leading byte of a derived value, which is uniform regardless of the scrypt cost parameters.

The twelve call sites

Line numbers as of v1.4.1. Each needs a width the value cannot supply:

Line	Function	Needs	Value
232, 235	encrypt	16	the two AES blocks of the private key
339	create_new_encrypted_wif	16	seed_b[:16] ^ derived_half_1
342	create_new_encrypted_wif	16	(encrypted_half_1[8:] + seed_b[16:]) ^ derived_half_2
354, 357	create_new_encrypted_wif	16	the two halves of point_b
456, 459	confirm_code	16	the two halves of point_b
578	decrypt	32	the recovered private key, non-EC
636	decrypt	16	encrypted_half_1[8:] + seed_b[16:]
644	decrypt	16	seed_b[:16]
653	decrypt	32	pass_factor * factor_b mod the curve order

There are 53 calls to integer_to_bytes in the repository. Twelve are changed here. Of the remaining 41, four already pass an explicit width and one is in a test; the rest take their value from a module constant (MAGIC_*, the 0x0142/0x0143 prefixes, CONFIRMATION_CODE_PREFIX, the flag bytes, COMPRESSED_PRIVATE_KEY_PREFIX) or from a per-coin prefix — wif_prefix in wif.py, address_prefix in p2pkh_address.py.

The per-coin prefixes are worth a note, because the obvious worry there is a prefix of zero — and there is one. Enumerating all 155 cryptocurrencies: the 67 distinct wif_prefix values are each a single byte, and of the 65 distinct address_prefix values, seven are 0x00, Bitcoin mainnet among them. Those sites are nonetheless correct, because integer_to_bytes computes its width as (data.bit_length() if data > 0 else 1), and that else 1 returns exactly one byte for zero. So the prefix paths are unaffected and I have left them alone rather than widen this PR.

Why the existing tests pass

This is the part I would most like to flag, because it is why a fix without new vectors could regress silently.

All nine test vectors published in BIP-0038 pass on the unpatched code. I ran them: 9 of 9 green against 1.4.1. Not one of them has a private key whose first byte is 0x00, so not one can reach this. The same is true of the repository's own data — the twelve decrypt vectors in tests/data/values.json carry twelve private keys, and the smallest leading byte among them is 0x09.

So the specification does not supply a vector that can catch this, and the suite as it stands cannot either. New vectors are not optional here; they are the only thing standing between this fix and a silent regression later.

What changed

bip38/bip38.py — twelve lines, each gaining an explicit width argument, e.g.

         private_key: bytes = integer_to_bytes(
-            bytes_to_integer(decrypted_half_1 + decrypted_half_2) ^ bytes_to_integer(derived_half_1)
+            bytes_to_integer(decrypted_half_1 + decrypted_half_2) ^ bytes_to_integer(derived_half_1), 32
         )

No signature changes, no new names, no new dependency, nothing added to utils.py. This matches what point.py already does at its three calls, and bip38.py at line 147.

I did consider changing the default in integer_to_bytes instead, since one line would be smaller than twelve. It does not work: the required width is not a function of the value — 0x0142 must be 2 bytes, a magic 8, a flag 1, an AES block 16, a private key 32 — and no rule over the integer alone separates those. Making the default a fixed 32 fails 7 of the 14 existing tests. The helper is behaving as documented. What is missing is at the twelve call sites, which need a specific width and never ask for one.

tests/data/values.json — twelve vectors, one per fixed call site, appended to the four existing lists and separated by a blank line in the same way the decrypt list already groups its entries. Two encrypt, four create_new_encrypted_wif, two confirm_code, four decrypt. They reuse the intermediate passphrase and passphrases already in the file, so the only thing that varies is the key or seed. No test code changed — the existing loops pick them up.

Dropped onto unpatched 1.4.1, these vectors fail 4 of the 5 BIP38 tests (test_bip38_encrypt, test_create_new_encrypted_wif, test_confirm_code, test_bip38_decrypt); test_intermediate_code passes, correctly, since intermediate_code() has no affected call site.

Backward compatibility

Nothing that worked before changes. I captured 31,715 operations from unpatched 1.4.1 and replayed the identical inputs against the patched build:

operations compared                          31715
succeeded before and after, byte-identical   31408
succeeded before, output CHANGED                 0
failed before, succeeds now                    307
succeeded before, fails now                      0

Every encrypted WIF, confirmation code, intermediate code, address and recovered seed is unchanged. No previously readable ciphertext becomes unreadable; the patch only affects inputs on which 1.4.1 raised.

One behaviour on the failure path does change, and the table above cannot show it because it only counts successes. Given an incorrect passphrase, 1.4.1 raises Secp256k1Error rather than PassphraseError in roughly 1 case in 208 — whenever the garbage it reconstructs happens to begin with a zero byte. After this change that path raises PassphraseError consistently, which is what the docstring already promises. I mention it in case anyone is catching the narrower exception. Overflow is not a new risk either: every fixed site XORs operands already at the stated width, and line 653 is reduced modulo the curve order, which is below 2^256, so to_bytes cannot overflow.

That the recovered keys are the right ones is provable without trusting me

A BIP38 key commits to its own plaintext: bytes 3..7 of the payload hold the first four bytes of
SHA256d over the address of the key inside it. It is the same commitment decrypt() already
checks before returning. Both leading-zero vectors this PR adds satisfy it, so the keys 1.4.1
cannot recover are demonstrably the keys those ciphertexts were made from:

6PYPSxToG1Mz2a2NCFjqCQ9Fp5YmUYrwLLFY3B1p1oTC1fNFMj4v7hg8ZT   passphrase "TestingOneTwoThree"
  bytes 3..7 of the key                             5a703444
  SHA256d("18Hiym9apXwnCowwBNDTXj1Rkfju88XZXZ")[:4]  5a703444

6PnN6NobDqyJte5dEPACBBWVNphybs3TAn6NnCfvF5gxJMmAcCnyH13igK   passphrase "MOLON LABE"
  bytes 3..7 of the key                             1550b4f6
  SHA256d("1PsobFXA8Cp2p7kfsyLtHkdxoLgDyjf4Cz")[:4]  1550b4f6

Both recovered secrets begin 0x00 — 0007a6de… and 0096a5ad… — which is exactly the case
1.4.1 cannot return. Anyone can check the match with a base58 decoder and two SHA-256 calls; it
does not depend on this patch, on my tooling, or on my word.

Verification

• coverage run -m pytest — 14 passed.
• The 9 BIP-0038 vectors — 9 of 9, before and after.
• 49,599 random operations across the five entry points — 466 failures before the change, 0 after.
• Coverage is unchanged at 94%. The new vectors run the same lines with different data, so they raise no percentage; their value is in the data, not the line count.

One cost worth naming: the added vectors take the suite from 37 to 53 scrypt evaluations at the spec's default parameters, which is the dominant term in the run time. Measured on an idle machine, pytest goes from 9.9s to 14.1s. If you would rather have fewer, the four create_new_encrypted_wif vectors are the cheapest to keep (they use the intermediate code and so skip the expensive KDF entirely) and the two confirm_code vectors are the most expensive at three evaluations each. I am happy to trim to whatever set you prefer.

Notes

• Targets master, based on 4d3c721.
• I have not touched CHANGELOG.md or bip38/info.py, since both look release-scoped and yours to write. If it helps, an entry under Fix Bugs: might read: Pass an explicit byte length where a fixed-width value is required, so values beginning with 0x00 are no longer built one byte short.
• Happy to split the fix and the vectors into separate PRs, rebase, or adjust anything to taste.