-
Notifications
You must be signed in to change notification settings - Fork 717
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
Replace OpenSSL AES with ctaes-based version #1486
Conversation
Been testing encryption/decryption for passphrases, keys, bip38, before/after this PR. All good so far.
just rebase this PR on latest master (it's a simple rebase without conflicts) as the block index serialization is changed. |
git-subtree-dir: src/crypto/ctaes git-subtree-split: cd3c3ac31fac41cc253bf5780b55ecd8d7368545
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. Coming from upstream 1c391a5
Coming from upstream 9049cde
BytesToKeySHA512AES should be functionally identical to EVP_BytesToKey, but drops the dependency on openssl. Coming from upstream 976f9ec
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.
This makes CCrypter easier to pass aroundf for tests coming from fb96831
0eabf5c
to
8adbaab
Compare
Just rebased the PR to make the review process easier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good backport. ACK 8adbaab
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far, so good.
Minor note about un-necessary header includes in the crypto unit tests. They are from the original upstream PR, but just aren't needed and were later removed upstream (bitcoin#15919)
src/test/crypto_tests.cpp
Outdated
@@ -19,6 +20,8 @@ | |||
|
|||
#include <boost/assign/list_of.hpp> | |||
#include <boost/test/unit_test.hpp> | |||
#include <openssl/aes.h> | |||
#include <openssl/evp.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to add openssl header includes here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
… used Coming from btc@a34081b7c398847c37a587029c7ad7f3a3396c8e
Updated as per @Fuzzbawls suggestion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK a49d5d4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK cleanup commit a49d5d4
f8fd095 Add missing lock in crypter GetKeys. (furszy) 366bc8b Get rid of LockObject and UnlockObject unused methods. (furszy) 121e5c0 [Wallet] Change CCrypter to use vectors with secure allocator (furszy) Pull request description: Built on top of #1486. Commits starting in `d058064` . This PR back port, partially, [upstream@8753](bitcoin#8753) . -- The last three commits left to back port in a future PR -- Containing the following changes: 1) Changing CCrypter to use vectors with secure allocator instead of have to lock stack memory pages to prevent the memory from being swapped to disk. 2) Removing the unused LockObject and UnlockObject methods. 3) Adding a missing lock in the `CCryptoKeyStore::GetKeys` method. ACKs for top commit: Fuzzbawls: utACK f8fd095 random-zebra: utACK f8fd095 and merging... Tree-SHA512: c1b760d37da93623ade0fb2565e8f060a4b7f5a109633cbe885732a16ed32614ba1f07d351fadcd57a19edf1343bdee1a81ee27898b4f124b6ddb15cc226d0d2
Coming from upstream 7689