Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
Replace OpenSSL AES with ctaes-based version #7689
changed the title from
Replace OpenSSL AES with our own constant-time version (edit of #5949)
Replace OpenSSL AES with our own constant-time version
Mar 14, 2016
referenced this pull request
Mar 15, 2016
I don't think we should be using low-level crypto primitives code developed by us that has ~zero chance of being reviewed or used by anyone other than us. I don't care how good we think we are, thats just not a good practice.
Maybe stick this in libsecp256k1 instead?
@petertodd the "go put it in another library" response has a verifiable history of killing useful progress here (see also continued use of the problematic and fairly scary openssl RNG), you wouldn't provide the same complaint for random "found on the internet" code that was demonstratively broken. Seems misplaced. We don't have any performance concerns for AES but in a generic library there would be performance concerns and a different construction might be called for.
We could also not go as far making it a separate library, but do abstract out the inner AES logic as a separate C file and publish that under a different repository, together with tests. @petertodd I agree in theory that it has little chance of being reviewed elsewhere this way, but what about reviewers here? We have several reimplementations of other crypto primitives, in which bugs could have been introduced. Did anyone check by comparing their code line by line with an alternate implementation? If not, whether it includes original design or not is not very relevant.
I agree with @petertodd that ideally the code should be published separately from bitcoin as well.
This doesn't need to be a generic library. We'd like this code to be self-contained (and have a special requirement here) so using OpenSSL et al is not an option, and maintaining a new generic library is a lot of work and responsibility too.
But I can understand that some people would find a specific implementation of AES just for bitcoin core as risky. (And even though it's not used in consensus, nor anything network-facing, a bug in the wallet encryption would be a big deal.)
Yes, people have done so.
Please no, that's scope creep for libsecp256k1. You're not trying to turn secp256k1 into a generic crypto library are you?
I'd already suggested sipa split out and convert to C before he posted it because this have have independent interest as is probably the smallest constant time implementation of AES I've seen, or at least the smallest that doesn't have embarrassingly bad performance-- so no objection there.
I have had this code running on 104 cores for several days, running a test that feeds random input through encode and decode with random keys and compares it to AES-NI.
The current maximum long term rate for the wallet application of this code in the current network is roughly 7 decrypts per second of roughly 48 bytes. At the current speed of my test harness it means that I have tested the equivalent of the network's maximum rate for thirty one thousand years without finding a fault.
Mutation testing showed very very high error correlation, meaning that any error manually introduced in the software made every execution (or nearly every execution) wrong. (So far I have not found any candidate error that didn't have this effect though automated searching, though I wouldn't be totally shocked if there were one-- it's still the case that this codebase has very high error correlation).
Pieter has a new version which is a straightforward port to plain C89 with some minor cleanup and some size reductions I contributed (the code size is still larger than the smallest AES implementations I can find (which aren't constant time), but not enormously so). I'll soon update my testing harness to that code and continue. I've also now reviewed the C89 version pretty extensively.
The constant time AES core code is now factored out to a new repository; for now, it's available at http://github.com/sipa/ctaes/
The pull request here has been updated to use a subtree of that project, with C++ wrappers around it.
The build code is very simple: ctaes does not have any configuration or own build system, so Bitcoin Core just builds it as part of its own process. I have not included ctaes's tests here for simplicity; the relevant AES C++ wrapper code does have its own tests though.
I didn't see any tests that explicit test for failure with invalid padding, except perhaps in the test that makes sure it behaves the same as OpenSSL. Perhaps if the CBC mode is ported to C in a later PR that could be addressed.
utACK. Good work sipa and cfields.