Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Apr 5, 2012

Conversation

sipa
Copy link
Member

@sipa sipa commented Apr 5, 2012

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.

@gavinandresen
Copy link
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?

@sipa
Copy link
Member Author

sipa commented Apr 5, 2012

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.

@laanwj
Copy link
Member

laanwj commented Apr 5, 2012

ACK, checking return values is always good

@gmaxwell
Copy link
Contributor

gmaxwell commented Apr 5, 2012

ACK, passes my testing

gmaxwell added a commit that referenced this pull request Apr 5, 2012
Fix #955: Verify status of encrypt/decrypt calls to detect failed padding
@gmaxwell gmaxwell merged commit 4a8d0f3 into bitcoin:master Apr 5, 2012
coblee pushed a commit to litecoin-project/litecoin that referenced this pull request Jul 17, 2012
Fix #955: Verify status of encrypt/decrypt calls to detect failed padding
suprnurd pushed a commit to chaincoin-legacy/chaincoin that referenced this pull request Dec 5, 2017
ptschip pushed a commit to ptschip/bitcoin that referenced this pull request Apr 21, 2018
Fix several tests in script_tests.json
lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Oct 30, 2019
…lid length

9f0868a [Tests] Add tests for CCoins deserialization (random-zebra)
5006d45 CDataStream::ignore Throw exception instead of assert on negative nSize (random-zebra)
b8bc0d5 [Bug] Fix OOM when deserializing UTXO entries with invalid length (random-zebra)
0657a13 [Script] Treat overly long scriptPubKeys as unspendable (random-zebra)
4bfc161 [Script] Introduce constant for maximum CScript length (random-zebra)

Pull request description:

  ref: bitcoin#7933

ACKs for top commit:
  furszy:
    Good, tests passing 👍 , ACK 9f0868a
  Warrows:
    ACK 9f0868a

Tree-SHA512: 9f978d55cc2564ff905642fe624df43f502a297fbc966480164556328091933a9d6eb861bf287f1d07edb1d0e363d0a63ce76df7f01f01b9e73f69ba87a5576f
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants