Replace openssl aes encryption/decryption/key creation implementations with our own #5949
Conversation
085dc50 to
43d4308
Compare
|
Is there any reason to be embedding a copy of this code rather than using some library? This isn't even consensus-critical... |
|
@luke-jr Well we're ending up with our own implementation of AES either way, either through #5885 or this. You do have a point that we should not start importing all non-consensus critical dependencies as well. NIH syndrome is an enduring source of irritation to me. But using an external crypto library just for AES is also undesirable. Low-level crypto code is very stable, it is unlikely to require maintenance, so doesn't add to the maintenance burden much. |
| if (!fOk) return false; | ||
| AES256CBCEncrypt enc(chKey, chIV, true); | ||
| nLen = enc.Encrypt(&vchPlaintext[0], vchPlaintext.size(), &vchCiphertext[0]); | ||
| if(nLen < vchPlaintext.size()) |
There was a problem hiding this comment.
'int' and 'size_type' comparison.
Gives a clang warning.
|
Tests are broken on mac osx. Could be mac only because travis did not report any issues. Wondering how this behaves on windows. make check log: https://gist.github.com/jonasschnelli/a8aaf63a219f010dbc1f I think further test does not make sense unless the unit-tests are running through. |
|
@jonasschnelli That should fix your issue. It wasn't actually related to osx. The tests turned up an unintended change in openssl behavior for version 1.0.1j, which was later reverted for 1.0.1k. It should be harmless since it only affects the output of failed decrypts which shouldn't be used anyway, but the tests here go to extra lengths to ensure that our output always matches openssl's. Thanks for testing and reporting, that was a fun one to track down. |
|
@theuni Tests running through now. |
|
|
||
| // Write all but the last block | ||
| while(written + AES_BLOCKSIZE <= size) | ||
| { |
There was a problem hiding this comment.
Coding style nit: braces on the same line
|
lightly tested (at branch point) ACK Meta: Should probably run all new files through the clang formatter, catch all nits ahead of time. |
| written += AES_BLOCKSIZE; | ||
| } | ||
|
|
||
| // When decrypting padding, attempt to run in constant-time |
There was a problem hiding this comment.
Nice tricks for constant-time behavior.
|
Concept ACK. Code review ACK. I didn't test or review the tests. |
|
@jgarzik Reminds me that we wanted to clang-format-all-sources... when will this be done finally? |
|
@jgarzik Thanks for the good suggestion. I formatted the new aes files with clang-format-3.5, ignoring the wonky array init change for now as discussed on IRC. Separate commits to avoid the need to re-ACK the original changes. |
|
Concept ACK. Needs rebase |
|
Rebased. I'm not sure if this is going to end up being useful though, with @jonasschnelli's work towards a new wallet. |
|
@theuni: i really like this. Using this would allow encryption of users wallets without adding openssl (or similar) as dependency. I don't have the experience to judge the risks, but if im right, the AES part is very stable and foreseeable. The critical part (PRNG) is untouched. My plans is to use this for the |
|
Needs rebase. Maybe on top of #6954 ? |
|
Needs rebase, indeed. |
|
Tagged for 0.13 |
|
Time to start merging these. |
The output should always match openssl's, even for failed operations. Even for a decrypt with broken padding, the output is always deterministic (and attemtps to be constant-time).
AES IV's are 16bytes, not 32. This was harmless but confusing. Add WALLET_CRYPTO_IV_SIZE to make its usage explicit.
This makes CCrypter easier to pass aroundf for tests
BytesToKeySHA512AES should be functionally identical to EVP_BytesToKey, but drops the dependency on openssl.
Wallet must come before crypto, otherwise linking fails on some platforms. Includes a tangentially-related general cleanup rather than making the Makefile sloppier.
Verify that results correct (match known values), consistent (encrypt->decrypt matches the original), and compatible with the previous openssl implementation. Also check that failed encrypts/decrypts fail the exact same way as openssl.
|
rebased |
|
Awesome, thanks @theuni |
|
Nice! Going to re-test soon. |
|
@jonasschnelli Some of the cbc test vectors are included here: https://github.com/bitcoin/bitcoin/pull/5949/files#diff-e425713e54c6cfd3f6056f549af0c5e4R368 Those come from http://csrc.nist.gov/publications/nistpubs/800-38a/sp800-38a.pdf Happy to add the others. |
|
utACK |
| @@ -16,6 +17,8 @@ | |||
|
|
|||
| #include <boost/assign/list_of.hpp> | |||
| #include <boost/test/unit_test.hpp> | |||
| #include <openssl/aes.h> | |||
There was a problem hiding this comment.
Do we need these openssl includes here?
|
In https://github.com/sipa/bitcoin/commits/const_aes you can find a rebased version of this PR on top of sipa@27d0d49 which implements AES in constant-time:
|
|
Closing in favor of #7689 |
In the spirit of dropping openssl where possible...
This uses @sipa's AES impelementation from #5885. As I understand it, he is comfortable with pulling this part in before the rest of fortuna.
The bulk of changes here are tests, the actual changes are small. Tests verify that encryption/decryption/keygen work as expected, but also that the results match openssl's exactly (including output buffers in failure cases).
The aes-cbc decryption attempts to be done in constant-time, in case it's re-used in the future in places that may be susceptible to timing attacks. It works with/without padding, so it could be re-used in different contexts.
@sipa I copied your structures for AES_Decrypt/AES_Encrypt for the sake of simplicity, so there's a good bit of duplication here now. I'm happy to re-work with templates, merged classes, separate files, etc.
Besides the unit tests, I've also done some quick sanity checks via rpc to verify that switching back and forth between implementations works as expected. Wallets encrypted with openssl still decrypt fine, and likewise the other way around.