search

AlphaWallet / alpha-wallet-android

An advanced Ethereum mobile wallet
https://www.alphawallet.com
MIT License
596 stars 530 forks source link

Should show an error, instead of silently importing a invalid private key and then fail when signing #2689

Open hboon opened 2 years ago

hboon commented 2 years ago

Private keys for the secp256k1 curve must be smaller than the order n, which is fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141 and must not be 0…0 so these keys are invalid:

fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141
fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364142
0000000000000000000000000000000000000000000000000000000000000000

While this is valid:

fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364140

Should show an error, instead of silently importing the invalid private key and then fail when signing

Context: https://canary.discord.com/channels/548357107267928064/991499612617719848/991562068975161426

(on iOS, it crashes, which isn't good either https://github.com/AlphaWallet/alpha-wallet-ios/issues/4921)

Test cases used for iOS: https://github.com/AlphaWallet/alpha-wallet-ios/pull/4926/files#diff-ba15c35c2c0ace5c3cb860323254268977e248980976a3480334bd4078d751d2

JamesSmartCell commented 2 years ago

Interesting bug, how did you find it? Was it reported?

hboon commented 2 years ago

Interesting bug, how did you find it? Was it reported?

Sort of, refer to the Discord link in 1st comment.

hboon commented 2 years ago

I forgot to say, if you can call secp256k1_ec_seckey_verify(), it might replace manual validation. For iOS, we are just going to do it manually.

Also updated 1st comment to say that 0000000000000000000000000000000000000000000000000000000000000000 is invalid too.

hboon commented 2 years ago

@JamesSmartCell I guess a defensive check by quietly signing a hardcoded message with the key and EC-recover to compare the address would help too? We do this on iOS for seed phrases, but not for private key imports.

hboon commented 1 year ago

@JamesSmartCell would you help close this if it's done or not an issue anymore?