fix: Correct the SLIP-39 share value and PBKDF2 salt encodings

[Yamaguchi]

Summary

Two encoding defects in Bitcoin::SLIP39 make a share unrecoverable or non-interoperable. Both were found and fixed in tapyrusrb, whose SLIP-39 implementation this one is identical to, and both reproduce on master here.

1. A share value beginning with a 0x00 byte produces a mnemonic this library cannot decode. 4 out of 900 shares (0.44%) generated by SSS.setup_shares(group_threshold: 1, groups: [[2, 3]]) failed to round trip.
2. The PBKDF2 salt encodes the identifier with the minimum number of bytes instead of the two bytes SLIP-39 requires. 256 of the 32767 possible identifiers (0.78%) are affected, and a backup with such an identifier cannot be recovered by any other SLIP-39 implementation.

They live in different files and are reviewed most easily as the two commits of this branch.

Details

Leading zero bytes of the share value

Share#to_words succeeds, but feeding the result back into Share.from_words raises ArgumentError: Invalid mnemonic length. A backup written to paper is silently unrecoverable.

Two places in lib/bitcoin/slip39/share.rb derive the length of the share value from its integer representation, which discards leading zero bytes.

build_word_indices computed the bit length with Integer#bit_length:

value_length = value.to_i(16).bit_length

For a 32-byte value starting with 0x00 this yields 248 instead of 256, so the mnemonic is one word short of what the specification requires.

from_words converted the recovered bits back to hex with Integer#to_even_length_hex, which only pads to a whole byte and cannot restore a leading 00. Even with the writing side fixed, the recovered value would be shorter than the original and SSS.recover_secret would reject it at the "all share values must have the same length" check.

Identifier of the PBKDF2 salt

SLIP-39 defines the PBKDF2 salt as "shamir" || identifier, where the identifier is always encoded as two bytes in big endian order. SSS.get_salt built it with Integer#itb, which uses the minimum number of bytes and therefore emits a single byte whenever the identifier is below 256.

identifier	salt produced	salt required by SLIP-39
5	7368616d697205 (7 bytes)	7368616d69720005 (8 bytes)
256	7368616d69720100 (8 bytes)	7368616d69720100 (8 bytes)

Identifiers come from SecureRandom.random_number(32_767). Encryption and decryption used the same wrong salt, so such a backup could still be recovered by this library, but not by any other SLIP-39 implementation. Recovering it elsewhere yields a different master secret with no error, because the Feistel construction has no integrity check over the passphrase or the salt.

SSS.get_salt now encodes the identifier with [id].pack('n'), which always produces two bytes. Shares that this library has already generated with an identifier below 256 were encrypted with the shorter salt, and the mismatch surfaces as a silently different master secret rather than as an error. SSS.recover_secret therefore takes a legacy_salt keyword argument. When it is true, the salt is built the old way and such a backup can still be recovered. The option is deliberately absent from setup_shares: new shares are always generated according to the specification. The holder of an affected backup can recover it both ways and compare the addresses derived from each result to tell which one is theirs.

None of the vectors in spec/fixtures/slip39/vectors.json has an identifier below 256; the smallest is 282. The defect was therefore invisible to the test suite.

Changes

lib/bitcoin/slip39/share.rb

• build_word_indices derives the bit length from the byte size of the value (value.htb.bytesize * 8).
• The padding length takes an extra % RADIX_BITS so that a value whose bit length is already a multiple of 10 does not gain a spurious all-zero word.
• build_word_indices rejects a value which is not an even number of bytes. from_words derives the padding length from the number of value words modulo 16, which only matches the padding written here for an even number of bytes; an odd one used to be read back as a different value instead of being rejected.
• from_words pads the recovered hex to the exact length implied by the bit count instead of merely rounding up to a whole byte.

lib/bitcoin/slip39/sss.rb

• get_salt takes a legacy flag and encodes the identifier with two bytes unless it is set.
• decrypt takes a legacy_salt argument and passes it to get_salt.
• recover_secret takes a legacy_salt: keyword argument, defaulting to false, and passes it to decrypt.

spec/fixtures/slip39/vectors_short_identifier.json

• Three new test vectors whose identifiers are 157, 98 and 91. They were generated with the reference implementation, python shamir-mnemonic 0.3.0, with the extendable backup flag off, an iteration exponent of 0 and the passphrase TREZOR. Recovering them proves interoperability for the range this library used to get wrong.

spec/bitcoin/slip39_spec.rb

• Round trips share values with one or more leading zero bytes.
• Asserts that a value with a leading zero byte yields the same word count as a value without one.
• Asserts that an odd-length value is rejected.
• Recovers each of the three new vectors.
• Asserts the exact salt bytes for identifiers 5 and 256, in both the specified and the legacy encoding.
• Recovers a 1-of-1 share generated by bitcoinrb 1.14.0 with legacy_salt: true, and asserts that recovering it without the option gives a different result.

All ten new examples fail on master and pass with this change.

Impact

• Bitcoin::SLIP39::Share#to_words, Share.from_words, SSS.recover_secret, SSS.decrypt and SSS.get_salt.
• Values that do not begin with 0x00 are encoded exactly as before, and backups with an identifier of 256 or above use an unchanged salt. All examples in spec/fixtures/slip39/vectors.json still pass.
• Mnemonics previously written out for a share value starting with 0x00 were already undecodable and remain so. Such a backup has to be regenerated.
• Backups with an identifier below 256 that were generated by bitcoinrb 1.14.0 or earlier need legacy_salt: true from now on. Recovering one without the option returns a wrong secret and raises nothing. This has to be stated in the release notes.
• Newly generated shares are interoperable with Trezor and with other SLIP-39 implementations for every identifier.

How to verify

bundle exec rspec spec/bitcoin/slip39_spec.rb

57 examples, 0 failures.

To confirm the first defect on master:

share = Bitcoin::SLIP39::Share.new
share.id = 1234
share.iteration_exp = 0
share.group_index = 0
share.group_threshold = 1
share.group_count = 1
share.member_index = 0
share.member_threshold = 1
share.value = "00" + "11" * 31
share.checksum = share.calculate_checksum
Bitcoin::SLIP39::Share.from_words(share.to_words)
# => ArgumentError: Invalid mnemonic length.

To confirm the second, recover any vector in spec/fixtures/slip39/vectors_short_identifier.json on master. The returned secret differs from the expected one and no exception is raised.