Skip to content
This repository has been archived by the owner on May 13, 2022. It is now read-only.

ensure correct password #191

Merged
merged 1 commit into from Aug 15, 2015

Conversation

AdamISZ
Copy link
Member

@AdamISZ AdamISZ commented Aug 14, 2015

PKCS7 padding has the form: 01 or 0202 or 030303 etc. For a correctly entered password and a correct decryption, there will be a full block of padding at the end of the data: 1010... (16 times, hex). This obviously cannot happen by chance, which is why entering an incorrect password throws a ValueError which the current code then interprets as decryption failure and requests the user to try again.

However, in the case where an entered password is incorrect, it is possible that the decrypted data (before stripping the padding) ends with ..01. This would be interpreted as correctly pkcs7 padded, the 01 would be stripped, and a decrypted seed of length 32+15 = 47 bytes would be returned. No ValueError would be thrown and then a bip 32 master seed would be correctly (but randomly) generated. Note that the bip32 master key generation uses an hmac of the seed data, and so the seed data length is ignored.

So in summary we have a slightly bigger than 1 in 256 chance of a wrong password being accepted here, so this PR insists that the decrypted data has a length of 32 bytes, making this error impossible.

I don't consider this a big deal really, but it's as well to fix it of course. I was 'inspired' by #190 , but there seems to be no chance that this was the cause of the strange behaviour described there.

@AdamISZ
Copy link
Member Author

AdamISZ commented Aug 14, 2015

TLDR: The real situation is that there is a 1 in 8 chance of this happening, not 1 in 256!

The reason is that the slowaes module does not properly verify the padding, incredibly! It checks only the final byte, not the whole set of bytes. E.g.:

If the last byte is 04, it will immediately count it as valid. It will not check if the three preceding bytes are 04 04 04.

I have confirmed this (and confirmed my fix) by making a wallet with password 'abcd', and found that it will also accept 'fg' and 'tt' and so on. Creating, of course, a new random wallet with a different hash.

I have used the same code as I did in tlsnotary to check the pkcs7 padding is properly formatted. It now works correctly. Thus the previous comments no longer apply; this is fairly important.

I now believe this is probably very relevant to #190.

properly check pkcs7 in slowaes
chris-belcher added a commit that referenced this pull request Aug 15, 2015
@chris-belcher chris-belcher merged commit cb0e920 into JoinMarket-Org:master Aug 15, 2015
@AdamISZ AdamISZ deleted the pkcs7sanitycheck branch August 15, 2015 12:26
ghtdak pushed a commit to ghtdak/joinmarket that referenced this pull request Oct 1, 2015
ghtdak pushed a commit to ghtdak/joinmarket that referenced this pull request Oct 4, 2015
ghtdak pushed a commit to ghtdak/joinmarket that referenced this pull request Dec 4, 2015
ensure correct password
[gitreformat yapf-ify (github/ghtdak) on Fri Dec  4 04:51:04 2015]
[from commit: cb0e920]
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.

None yet

2 participants