Use unpadded encryption for wallet keys (fixes #933) #950

sipa commented at 1:05 AM on March 19, 2012: member

Wallet keys are 32 bytes, exactly two AES blocks. Using padded encryption makes attacking somewhat easier, as the attacker can check whether the padding is correct after decrypting using an attempted passphrase, rather than needing to do an EC multiplication to check whether the private and public keys match.

sipa commented at 1:10 AM on March 19, 2012: member

Note that this only fixes this issue for newly encrypted wallets. We do not rely on it for security however, so I do not think there is enough reason for reencrypting old keys.

TheBlueMatt commented at 2:48 AM on March 19, 2012: member

ACK for 0.6. Update: after incrementing min-version.

gavinandresen commented at 2:44 PM on March 19, 2012: contributor

NACK for 0.6 : maybe pull early in 0.7 and thoroughly testing backwards/forwards compatibility, although if it creates yet another wallet.dat variation that is not backwards-compatible then I'm not sure the cost is worth the extra security benefit.

(cost is incompatibility with earlier releases and third-party wallet tools)

Use unpadded encryption for wallet keys (fixes #933)

Wallet keys are 32 bytes, exactly two AES blocks. Using padded encryption
makes attacking somewhat easier, as the attacker can check whether the
padding is correct after decrypting using an attempted passphrase, rather
than needing to do an EC multiplication to check whether the private and
public keys match.

c682cdf3ed

Diapolo commented at 3:30 PM on March 19, 2012: none

I'm currently of no help coding wise, but I think an extra security benefit is not only nice to have, but a must. I agree it's a bad idea to include such a wallet-compatibility change at the end of the 0.6 development, but does wallet.dat variation really matter (i.e. is there a standard defined, when this is allowed and when not)?

Is there a safe way to, let's say, force all wallets to use the latest BC client encryption algo / functions, when first used with a new client version?

TheBlueMatt commented at 4:07 PM on March 19, 2012: member

Oops, this needs to increment wallet's min_version. I don't see why there is any objection to making another wallet version that is non-backward-compatible: wallets in 0.6 are already not compatible with any previous versions, so this is just another change that makes them further incompatible. In fact, I would say its better to put this in 0.6 than in 0.7, so that 0.7 wallets arent incompatible (again) with 0.6 wallets. Properly implemented 3rd-party wallet tools should still work fine, or could be made to work with very minimal changes.

sipa commented at 4:19 PM on March 19, 2012: member

(edited) @TheBlueMatt: oh yes, indeed, it needs a min_version update

luke-jr commented at 5:01 PM on April 6, 2012: member

Is this sane now with the -walletupgrade thing?

sipa commented at 9:26 PM on April 6, 2012: member

I'll update this to be compatible with -upgradewallet.

sipa commented at 6:52 PM on June 12, 2012: member

Closing this as it is outdated. Will reopen when redone properly.

sipa closed this on Jun 12, 2012

suprnurd referenced this in commit 123aa04d5b on Dec 5, 2017

ptschip referenced this in commit f12ccf059a on Feb 20, 2018

ptschip referenced this in commit 433857b979 on Feb 25, 2018

ptschip referenced this in commit f7cb6ffbbd on Mar 5, 2018

ptschip referenced this in commit 8f8088bc2e on Mar 7, 2018

lateminer referenced this in commit a03fa6236d on Oct 30, 2019

lateminer referenced this in commit 3d7e16e753 on Oct 30, 2019

sipa referenced this in commit c020cbaa5c on Jul 14, 2021

DrahtBot locked this on Sep 8, 2021