Replace openssl aes encryption/decryption/key creation implementations with our own

theuni commented at 0:14 am on March 27, 2015: member

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.

theuni force-pushed on Mar 27, 2015

luke-jr commented at 1:13 am on March 27, 2015: member

Is there any reason to be embedding a copy of this code rather than using some library? This isn’t even consensus-critical…

laanwj commented at 7:14 am on March 27, 2015: member

@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.

laanwj added the label Improvement on Mar 27, 2015

in src/wallet/crypter.cpp: in 43d43081be outdated

 98-    EVP_CIPHER_CTX_cleanup(&ctx);
 99-
100-    if (!fOk) return false;
101+    AES256CBCEncrypt enc(chKey, chIV, true);
102+    nLen = enc.Encrypt(&vchPlaintext[0], vchPlaintext.size(), &vchCiphertext[0]);
103+    if(nLen < vchPlaintext.size())

jonasschnelli commented at 6:32 pm on April 1, 2015:

‘int’ and ‘size_type’ comparison. Gives a clang warning.

jonasschnelli commented at 6:52 pm on April 1, 2015: contributor

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 Compile log: https://gist.github.com/jonasschnelli/8f947da1148b79950ead

I think further test does not make sense unless the unit-tests are running through.

theuni force-pushed on Apr 2, 2015

theuni commented at 3:58 pm on April 2, 2015: member

@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.

jonasschnelli commented at 8:03 pm on April 2, 2015: contributor

@theuni Tests running through now. Slightly testes ACK (create encrypted wallets, exchanged between master-built-bitcoind and this PR bitcoind, dumped wallet, compared keys [tested wallet.dat exchanging in both directions]).

in src/crypto/aes.cpp: in cb801f9df0 outdated

1126+
1127+    memcpy(mixed, iv, AES_BLOCKSIZE);
1128+
1129+    // Write all but the last block
1130+    while(written + AES_BLOCKSIZE <= size)
1131+    {

sipa commented at 1:35 am on April 3, 2015:

Coding style nit: braces on the same line

jgarzik commented at 2:01 am on April 3, 2015: contributor

lightly tested (at branch point) ACK

Meta: Should probably run all new files through the clang formatter, catch all nits ahead of time.

in src/crypto/aes.cpp: in cb801f9df0 outdated

1171+            *out++ ^= prev[i];
1172+        prev = data + written;
1173+        written += AES_BLOCKSIZE;
1174+    }
1175+
1176+    // When decrypting padding, attempt to run in constant-time

sipa commented at 2:39 am on April 3, 2015:

Nice tricks for constant-time behavior.

sipa commented at 2:39 am on April 3, 2015: member

Concept ACK. Code review ACK. I didn’t test or review the tests.

Diapolo commented at 2:41 pm on April 3, 2015: none

@jgarzik Reminds me that we wanted to clang-format-all-sources… when will this be done finally?

theuni commented at 0:35 am on April 4, 2015: member

@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.

jtimon commented at 1:31 pm on June 21, 2015: contributor

Concept ACK. Needs rebase

theuni force-pushed on Jun 25, 2015

theuni commented at 9:17 pm on June 25, 2015: member

Rebased. I’m not sure if this is going to end up being useful though, with @jonasschnelli’s work towards a new wallet.

jonasschnelli commented at 6:52 am on June 26, 2015: contributor

@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 corewallet/crypter.cpp stuff. If someone decides to not compile the “traditional” wallet, he could get rid of openssl (unless he uses QT bip70 support).

jtimon commented at 4:23 pm on November 10, 2015: contributor

Needs rebase. Maybe on top of #6954 ?

sipa commented at 1:18 pm on November 28, 2015: member

Needs rebase, indeed.

laanwj added this to the milestone 0.13.0 on Dec 3, 2015

laanwj commented at 11:50 am on December 3, 2015: member

Tagged for 0.13

laanwj commented at 12:55 pm on February 1, 2016: member

Time to start merging these. Needs rebase, though.

Add AES implementation 86707bfe09

crypto: add AES 128/256 CBC classes

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).

ce04d7c80f

crypto: add aes cbc tests 0d3d2df0eb

crypter: fix the stored initialization vector size

AES IV's are 16bytes, not 32. This was harmless but confusing.

Add WALLET_CRYPTO_IV_SIZE to make its usage explicit.

37bdde8d8d

crypter: constify encrypt/decrypt

This makes CCrypter easier to pass aroundf for tests

7bf56ea81a

crypter: hook up the new aes cbc classes 56bbf5e904

crypter: add a BytesToKey clone to replace the use of openssl

BytesToKeySHA512AES should be functionally identical to EVP_BytesToKey, but
drops the dependency on openssl.

1d2fe8e52e

crypter: shuffle Makefile so that crypto can be used by the wallet

Wallet must come before crypto, otherwise linking fails on some platforms.

Includes a tangentially-related general cleanup rather than making the Makefile
sloppier.

be7f4d9bba

crypter: add tests for crypter

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.

952dfb42b1

theuni force-pushed on Mar 1, 2016

theuni commented at 6:08 pm on March 1, 2016: member

rebased

laanwj commented at 2:00 pm on March 2, 2016: member

Awesome, thanks @theuni

jonasschnelli commented at 3:37 pm on March 2, 2016: contributor

Nice! Going to re-test soon. What about adding the NIST test vectors to the test code? I have create a C test script recently with most NIST AES test vectors. https://github.com/libbtc/libbtc/blob/master/test/aes_tests.c?

theuni commented at 5:59 pm on March 2, 2016: member

@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.

laanwj added the label Priority High on Mar 3, 2016

sipa commented at 4:52 am on March 5, 2016: member

utACK

in src/test/crypto_tests.cpp: in 952dfb42b1

16@@ -16,6 +17,8 @@
17 
18 #include <boost/assign/list_of.hpp>
19 #include <boost/test/unit_test.hpp>
20+#include <openssl/aes.h>

paveljanik commented at 8:28 am on March 5, 2016:

Do we need these openssl includes here?

sipa commented at 7:30 pm on March 13, 2016: member

In https://github.com/sipa/bitcoin/commits/const_aes you can find a rebased version of this PR on top of https://github.com/sipa/bitcoin/commit/27d0d49e317bd366bdbf5f922fc11d933af06fef which implements AES in constant-time:

It uses the bit-slicing approach from http://www.iacr.org/archive/ches2009/57470001/57470001.pdf
Not super-optimized, and 15-25 times slower than the older variable-time table-based approach from #5885, but can still do ~2 MiB/s for short messages and ~10 MiB/s for long ones.
The same algorithm could be used to perform 2 or 4 (on 32-bit resp. 64-bit systems) AES operations in parallel at the same speed. This isn’t implemented because it’s not usable for (single) CBC mode operations.

laanwj commented at 8:21 am on March 15, 2016: member

Closing in favor of #7689

laanwj closed this on Mar 15, 2016

DrahtBot locked this on Sep 8, 2021

Replace openssl aes encryption/decryption/key creation implementations with our own #5949