Fix #955: Verify status of encrypt/decrypt calls to detect failed padding #1039

1. 
   sipa commented at 12:10 AM on April 5, 2012: member

   Issue #955 was discovered to be caused by users entering an incorrect password that accidentally (with a chance of 1/255) resulted in valid AES-CBC padding, which causes a decrypted secret of 45-47 bytes, causing a failed CKey::SetSecret call. In the normal case, a bad password causes an incorrect padding which is detected by the decrypt function. As the return code is not checked, the previously (incorrectly) decrypted 32 bytes are used anyway, leading to a succesful CKey::SetSecret call.

   So in summary: we didn't know we were padding by default, and hence forgot to check for the case of a failed padding, which apparently resulted in something of the expected size.

2. Verify status of encrypt/decrypt calls to detect failed padding e5c027b49b
3. 
   gavinandresen commented at 1:14 AM on April 5, 2012: contributor

   ACK. What would be a good test? Encrypt a wallet with a long random password and then run a brute-force guesser via RPC-- expect an eventual crash before this fix, expect it to run forever after?

4. 
   sipa commented at 1:46 AM on April 5, 2012: member

   Via RPC it does not crash, you just get a different error.

   $ (for A in $(seq 1 5000); do ./bitcoind walletpassphrase test$A 1; done) 2>&1 | tee res.txt error: {"code":-14,"message":"Error: The wallet passphrase entered was incorrect."} error: {"code":-14,"message":"Error: The wallet passphrase entered was incorrect."} error: {"code":-14,"message":"Error: The wallet passphrase entered was incorrect."} error: {"code":-1,"message":"CKey::SetSecret() : secret must be 32 bytes"} error: {"code":-14,"message":"Error: The wallet passphrase entered was incorrect."}

   The -1 error appearing in 254/(255*255) ~= 1/256 cases (in particular: when the last padding byte(s) of the master key does not decrypt to 0x01 or 0x0202 or 0x030303, ..., and the last padding byte of the first checked wallet key do start with such a sequence.

   With this patch, no -1 error should be produced anymore.

5. 
   laanwj commented at 5:50 AM on April 5, 2012: member

   ACK, checking return values is always good

6. 
   gmaxwell commented at 1:17 PM on April 5, 2012: contributor

   ACK, passes my testing

7. gmaxwell referenced this in commit 4a8d0f3b10 on Apr 5, 2012
8. gmaxwell merged this on Apr 5, 2012
9. gmaxwell closed this on Apr 5, 2012

   ---

10. coblee referenced this in commit c6d44f8ef9 on Jul 17, 2012
11. suprnurd referenced this in commit 2f93a0d208 on Dec 5, 2017
12. ptschip referenced this in commit b0dc8d2189 on Apr 21, 2018
13. lateminer referenced this in commit 41d19c106f on Oct 30, 2019
14. DrahtBot locked this on Sep 8, 2021

Milestone
0.6.1