Security enhancements to ChaCha20::SetKey and CSignatureCache::ComputeEntryECDSA

guidovranken commented at 8:06 pm on April 26, 2021: contributor

Add a check to ChaCha20::SetKey which asserts that the specified key size is valid. This is an inexpensive check that guards against (accidental) misuse, which could result in out-of-bound reads.
In CSignatureCache::ComputeEntryECDSA, change the way a vector pointer is resolved to prevent invoking undefined behavior if the vector is empty.

crypto: Throw exception on passing ChaCha20 key of invalid size b3f800f56b

script: Replace address-of idiom with vector data() method 65a3368a23

guidovranken renamed this:
~~Fix setkey computeentryecdsa~~
Security enhancements to ChaCha20::SetKey and CSignatureCache::ComputeEntryECDSA
on Apr 26, 2021

practicalswift commented at 8:33 pm on April 26, 2021: contributor

Concept ACK: preconditions should be checked where possible

I take it you have started fuzzing Bitcoin Core again @guidovranken? That’s very good news for Bitcoin :)

Don’t forget to any fuzzing trophies to https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Fuzz-Trophies :)

DrahtBot added the label Consensus on Apr 26, 2021

DrahtBot added the label Utils/log/libs on Apr 26, 2021

in src/script/sigcache.cpp:56 in 65a3368a23 outdated

52@@ -53,7 +53,7 @@ class CSignatureCache
53     ComputeEntryECDSA(uint256& entry, const uint256 &hash, const std::vector<unsigned char>& vchSig, const CPubKey& pubkey) const
54     {
55         CSHA256 hasher = m_salted_hasher_ecdsa;
56-        hasher.Write(hash.begin(), 32).Write(&pubkey[0], pubkey.size()).Write(&vchSig[0], vchSig.size()).Finalize(entry.begin());
57+        hasher.Write(hash.begin(), 32).Write(&pubkey[0], pubkey.size()).Write(vchSig.data(), vchSig.size()).Finalize(entry.begin());

MarcoFalke commented at 6:56 am on April 27, 2021:

Why is this changed here, but the other 55 instances are left as is? Including the one in this line and another one in this file?

See also commit 592404f03

And our developer notes:

0- Watch out for out-of-bounds vector access. `&vch[vch.size()]` is illegal,
1  including `&vch[0]` for an empty vector. Use `vch.data()` and `vch.data() +
2  vch.size()` instead.

MarcoFalke commented at 7:17 am on April 27, 2021:

Also, it could make sense to open two pull requests for the changes here, since they seem unrelated? (One of them is also failing the fuzz task, btw)

guidovranken commented at 8:35 am on April 27, 2021:

Why is this changed here, but the other 55 instances are left as is? Including the one in this line and another one in this file?

I didn’t change the two instances of &pubkey[0] in this file because the CPubkey subscript operator returns a reference to a regular array instead of a vector, so no undefined behavior rules are violated.

I didn’t change any other instances of &vch[0] because this instance specifically was the only one which came up with fuzzing (so far) with -D_GLIBCXX_DEBUG.

Also, it could make sense to open two pull requests for the changes here, since they seem unrelated? @laanwj suggested I create two commits in 1 PR.

(One of them is also failing the fuzz task, btw)

It fails because it instantiates ChaCha20 with a key that is 17..31 bytes large whereas my change requires that it is either 16 or 32 bytes.

I can either relax the condition in ChaCha20 to allow keys 16..32 bytes large, or change the fuzzer to pick one of (16, 32), let me know which you prefer.

practicalswift commented at 11:01 am on April 27, 2021:

I didn’t change any other instances of &vch[0] because this instance specifically was the only one which came up with fuzzing (so far) with -D_GLIBCXX_DEBUG.

-D_GLIBCXX_DEBUG is great! :) @guidovranken, you don’t happen to have any notes saved on any tweaks you had to make to get a full -D_GLIBCXX_DEBUG compile including dependencies for Bitcoin Core? IIRC our Boost dependencies were a bit cumbersome to get compiled with -D_GLIBCXX_DEBUG at least historically.

If anyone wants to add a CI job which runs the tests (unit + functional + fuzz) under -D_GLIBCXX_DEBUG I’d be more than happy to review such a PR.

I can either relax the condition in ChaCha20 to allow keys 16..32 bytes large, or change the fuzzer to pick one of (16, 32), […]

Asserting the precondition (like you’ve already done) and modifying the fuzzer to fulfil said precondition is probably the best option.

fanquake commented at 11:14 am on April 27, 2021:

tweaks you had to make to get a full -D_GLIBCXX_DEBUG compile including dependencies for Bitcoin Core? IIRC our Boost dependencies were a bit cumbersome to get compiled with -D_GLIBCXX_DEBUG at least historically.

Note that depends builds with DEBUG=1 have always used -D_GLIBCXX_DEBUG && D_GLIBCXX_DEBUG_PEDANTIC. If building depends with DEBUG=1 don’t work for some reason, please open an issue.

MarcoFalke commented at 9:35 am on April 29, 2021:

Thanks @guidovranken and @fanquake for suggesting both -D_GLIBCXX_DEBUG and -D_GLIBCXX_DEBUG_PEDANTIC

Looks like there are more issues than this one. I’ve reported them here:

MarcoFalke commented at 10:00 am on April 29, 2021:

Is there a downside splitting this into two pulls? I’d still prefer that to make it easier to review, because both commits can be reviewed independently. Also, it makes a cleaner history and discussion because two unrelated threads aren’t intertwined.

MarcoFalke commented at 6:26 pm on April 30, 2021:

I went ahead and split the raw data pointer commit into a separate pull (#21817). I hope this is ok. The commit hash needed to be changed, but the author information is fully preserved.

MarcoFalke commented at 6:56 am on April 27, 2021: member

I wonder if this is better done with a scripted-diff

practicalswift commented at 10:50 am on April 27, 2021: contributor

I wonder if this is better done with a scripted-diff

If anyone wants to tackle &vch[0]-to-vch.data() using a scripted-diff then this could be used as a starting point (untested beyond make check: careful manual review of the changes will be required obviously):

0sed -i 's%&\([a-zA-Z0-9_]\+\)\[0\]%\1.data()%g' $(git grep -lE '&([a-zA-Z0-9_]+)\[0\]' -- build_msvc/testconsensus/testconsensus.cpp src/bench/block_assemble.cpp src/random.cpp src/script/sigcache.cpp src/test/compress_tests.cpp src/test/crypto_tests.cpp src/test/fuzz/util.cpp src/test/script_standard_tests.cpp src/test/script_tests.cpp src/wallet/crypter.cpp src/wallet/wallet.cpp)

DrahtBot commented at 2:18 am on May 1, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21817 (refactor: Replace &foo[0] with foo.data() by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/crypto/chacha20.cpp:28 in b3f800f56b outdated

23@@ -23,6 +24,10 @@ static const unsigned char tau[] = "expand 16-byte k";
24 
25 void ChaCha20::SetKey(const unsigned char* k, size_t keylen)
26 {
27+    if (keylen != 16 && keylen != 32) {
28+        throw std::invalid_argument("ChaCha20 key size must be 16 or 32");

MarcoFalke commented at 6:52 am on May 1, 2021:

Wouldn’t it be better to make this an assert? This would allow to compile the crypto code with noexcept

laanwj commented at 2:51 pm on May 4, 2021:

I too think an assert would be more appropriate here. It’s unclear how an exception will be handled. Also the caller is responsible that a valid key is passed. When this doesn’t happen it is a coding bug, it makes more sense to immediately terminate the process.

guidovranken commented at 11:50 pm on May 21, 2021:

Replaced with assert.

MarcoFalke approved

MarcoFalke commented at 6:52 am on May 1, 2021: member

ACK, but still needs the tests fixed

laanwj commented at 2:51 pm on May 4, 2021: member

Concept ACK

crypto: Replace exception with assert to validate ChaCha20 key size 562cfc18c8

MarcoFalke referenced this in commit 32f1f021bf on May 5, 2021

MarcoFalke removed the label Consensus on May 5, 2021

MarcoFalke removed the label Utils/log/libs on May 5, 2021

DrahtBot added the label Needs rebase on May 5, 2021

DrahtBot commented at 5:01 pm on May 5, 2021: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

in src/crypto/chacha20.cpp:25 in 562cfc18c8 outdated

23@@ -23,6 +24,8 @@ static const unsigned char tau[] = "expand 16-byte k";
24 
25 void ChaCha20::SetKey(const unsigned char* k, size_t keylen)

jnewbery commented at 8:04 am on May 6, 2021:

This function’s declaration is commented:

0    void SetKey(const unsigned char* key, size_t keylen); //!< set key with flexible keylength; 256bit recommended */

That should be updated to say that keylen is in bytes, and that only 16 and 32 are valid.

jnewbery commented at 8:07 am on May 6, 2021:

The ctor which takes a keylen should also be commented to document assumptions.

guidovranken commented at 11:54 pm on May 21, 2021:

Done.

jnewbery commented at 8:05 am on May 6, 2021: member

PR description should be updated: Add a check to ChaCha20::SetKey which ~~validates the size of the specified key, and throws an exception if it is invalid~~ asserts that the specified key size is valid.

MarcoFalke commented at 8:08 am on May 6, 2021: member

Are there steps to reproduce the ChaCha20 issue?

MarcoFalke commented at 8:30 am on May 21, 2021: member

@guidovranken Are you still working on this?

crypto: Update ChaCha20 method declaration comments 6542bf5de4

guidovranken commented at 11:58 pm on May 21, 2021: contributor

PR description should be updated: Add a check to ChaCha20::SetKey which ~~validates the size of the specified key, and throws an exception if it is invalid~~ asserts that the specified key size is valid.

PR desc updated.

Are there steps to reproduce the ChaCha20 issue?

Should I add a test?

@guidovranken Are you still working on this?

I’ve addressed the outstanding issues. Anything else?

MarcoFalke commented at 4:28 am on May 22, 2021: member

See #21781 (comment)

MarcoFalke commented at 4:30 am on May 22, 2021: member

Should I add a test?

For example. Any other way to first observe the bug and then to observe the fix is also welcome.

guidovranken commented at 0:17 am on May 25, 2021: contributor

Should I add a test?

For example. Any other way to first observe the bug and then to observe the fix is also welcome.

How can I test if an assertion failure occurs in the tests?

MarcoFalke commented at 5:31 am on May 25, 2021: member

I was wondering how you’ve found the bug on master. Code review or some other way?

guidovranken commented at 8:49 pm on May 25, 2021: contributor

I was wondering how you’ve found the bug on master. Code review or some other way?

I found the bug with Cryptofuzz, which is now running on OSS-Fuzz as part of the bitcoin-core project.

In Cryptofuzz, I work around the bug by not passing an invalid key size: https://github.com/guidovranken/cryptofuzz/blob/61f27a0afa28942a8bdb8de49775cb734acf0121/modules/bitcoin/module.cpp#L252-L254

I can remove that check once this PR is merged, and add no unit tests to bitcoin. OK?

DrahtBot commented at 11:21 am on December 15, 2021: member

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

DrahtBot commented at 1:06 pm on March 21, 2022: member

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

MarcoFalke commented at 2:10 pm on March 21, 2022: member

Closing for now due to inactivity. I think a new pull can be opened if anyone picks this up. Most of the discussion is about the already merged part: https://github.com/bitcoin/bitcoin/pull/21817

MarcoFalke closed this on Mar 21, 2022

DrahtBot locked this on Mar 21, 2023

Security enhancements to ChaCha20::SetKey and CSignatureCache::ComputeEntryECDSA #21781

Conflicts