Audit cryptography

[ryanxcharles]

There was recently a serious error discovered in one of fullnode's dependencies. See this commit for information:

dbeab00

This reminds us that the cryptography in fullnode, both in the depencies and in the library itself, largely consist of recently-written cryptography that has not been properly audited. This issue is to contain the problem of "auditing the cryptography" and keep track of the progress made on this goal.

First, let's just list the dependencies that need to be audited:

• bn.js
• elliptic (particularly the secp256k1 code)
• hash.js
• aes
• bs58
• pbkdf2-compat
• node-crypto
• browserify-crypto

Second, all of the cryptography in fullnode needs to be audited.

"Auditing" is a somewhat vague goal, but I will propose two passes:

Pass 1: Review every line of source code, identify edge cases, confirm that test vectors exist that test the edge cases, or write new test vectors that test the edge cases. Furthermore, identify all standard test vectors and include those.

Pass 2: Re-implement all of the cryptography a second time, identify edge cases in the new implementation, write tests for them, then confirm that the old code passes the new tests (or fix any issues that arise). If there is reason to believe the new cryptography is better, then replace the old cryptography with the new cryptography.

Both passes should be performed on all cryptography dependencies and for all cryptography directly contained in the library.

[ryanxcharles]

Greg Maxwell recently revealed that the development of libsecp256k1 revealed a bug in OpenSSL's big number squaring. They uncovered this bug by running specialized random tests around edge cases that were likely to contain bugs. We should run the same randomized tests on bn.js and elliptic to see if we can catch similar bugs.

• https://www.reddit.com/r/Bitcoin/comments/2rrxq7/on_why_010s_release_notes_say_we_have_reason_to/
• https://np.reddit.com/r/programming/comments/2rrc64/openssl_security_advisory_new_openssl_releases/cnilq2w?context=3
• https://www.openssl.org/news/secadv_20150108.txt

[ryanxcharles]

I have replaced all the string comparisons I could find with constant-time buffer comparisons in 5ffa0af , which should be resistant to timing attacks. I've noticed the bn.cmp method from bn.js is variable-time, so that should be either replaced with a constant-time version, or augmented with another version that is constant-time.