bitcoinjs / bip38

BIP38 is a standard process to encrypt Bitcoin and crypto currency private keys that is less susceptible to brute force attacks thus protecting the user.
http://cryptocoinjs.com/modules/currency/bip38/
MIT License
206 stars 100 forks source link

No support for SegWit addresses #76

Closed ValleZ closed 2 years ago

ValleZ commented 2 years ago

6PYRFsPKrpyk2qdz4Sm4QSrWzMKpNy6LCoCZJ8z5gtmubk3kG9P6Vft9dz password 12345

It fails because it's a SegWit address. Decrypted key is KxQdS9UeCDv8stcfADiMMttVXgNvFAnwhrGhseDMjUHXW2Unxaoz. Address ​bc1qkjntlwk9m99xj6j2tdpgzefu6uku0k88k94q6a

From https://github.com/BlueWallet/BlueWallet/issues/4156

junderw commented 2 years ago

The spec for bip38 does not support segwit addresses, nor does it support wrapped segwit addresses, nor will it support p2tr addresses.

Primarily because the standard has been abandoned for over 7 years and no one is willing to update it. (And to be honest, no one should really be using it anymore... it should only be used for recovering old wallets generated with this code 7 years ago)

https://github.com/bitcoin/bips/blob/f0cb046b116e3c05814eeb6dc48180b96fa16613/bip-0038.mediawiki

ValleZ commented 2 years ago

I don't see any public key or address format in BIP38 specification https://github.com/bitcoin/bips/blob/master/bip-0038.mediawiki

It means if address in in bech32 format, clients should not try to generate legacy address format instead, calculate its has and declare that password is incorrect.

junderw commented 2 years ago

In the bip:

the bit with value 0x20 when set indicates the key should be converted to a bitcoin address using the compressed public key format.

"bitcoin address using the compressed public key format" refers to base58 addresses that start with a 1 which contain the hash160 of the compressed DER key.

Segwit did not exist when BIP38 was created.

junderw commented 2 years ago

Also, can you tell me which wallet generated the encrypted key for you?

ValleZ commented 2 years ago

This is from https://github.com/ValleZ/Paper-Wallet Also https://kimbatt.github.io/ can generate BIP38 keys for segwit addresses (that implementation uses legacy address for hash verification) Mycelium wallet for android also supports this format.

junderw commented 2 years ago

The point of BIP38 is to encrypt and decrypt a private key. The only place where an address comes into play is password verification. Since the checksum of the address is compared to check that the password is correct.

If you use the wrong password, the outcome of decryption will produce a different incorrect private key. Which produces a different address. Which produces a different checksum.

So in the context of bip38, "wrong address" is the same meaning of "wrong password"

This is why the spec CAN NOT support multiple addresses, and why kimbatt uses legacy address for hash verification.


That said, I was able to decrypt a key generated from kimbatt's page using this library.

You should fix your android app to only use "legacy" addresses for hash verification moving forward... but since your app was generating invalid encrypted keys, you will need to generate all types of addresses and pass verification if any of the hashes match.

junderw commented 2 years ago

What you do with the private key is irrelevant to the BIP. If you want to use the private key to do anything, that doesn't matter.

"the spec CAN NOT support multiple addresses" is referring to "for hash verification"

This is why this library fails on your keys with "incorrect password"

ValleZ commented 2 years ago

It makes sense, thank you!

The update to the BIP still leaves an option to encrypt a private key what was used to generate SegWit address, but restricts hash verification. In this sense the example key is invalid but, say, 6PnNjUF6ZHc1qNGqqBM5NedfWZrgv6kCdBr9C1WszSN6Ht2ePdbsxd656k with password 12345 would still can map to bc1qm5lqjzjrzcwh0zkzsvwhf0luf2qd7tf5fvz4kk

I personally think that BIP should still not specify address type to have a way to recognize which address type was used.

ValleZ commented 2 years ago

It would map 6PnNjUF6ZHc1qNGqqBM5NedfWZrgv6kCdBr9C1WszSN6Ht2ePdbsxd656k to 1MApegei4hk5uGC1iB53gdKcTwpYrXe6eX and 6PYRFsPKrpyk2qdz4Sm4QSrWzMKpNy6LCoCZJ8z5gtmubk3kG9P6Vft9dz to bc1qkjntlwk9m99xj6j2tdpgzefu6uku0k88k94q6a