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

guidovranken · 2021-04-26T20:06:27Z

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.

practicalswift · 2021-04-26T20:33:13Z

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

maflcko

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

maflcko · 2021-04-27T06:56:20Z

src/script/sigcache.cpp

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


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 592404f

And our developer notes:

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

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)

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.

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.

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.

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:

Is -D_GLIBCXX_DEBUG_PEDANTIC supported? boostorg/date_time#190

refactor: Avoid UB in util/asmap (advance a dereferenceable iterator outside its valid range) #21802

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.

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.

practicalswift · 2021-04-27T10:50:05Z

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

sed -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 · 2021-05-01T02:18:59Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: Replace &foo[0] with foo.data() #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.

maflcko

ACK, but still needs the tests fixed

src/crypto/chacha20.cpp

laanwj · 2021-05-04T14:51:55Z

Concept ACK

fac30ee refactor: Replace &foo[0] with foo.data() (MarcoFalke) faece47 refactor: Avoid &foo[0] on C-Style arrays (MarcoFalke) face961 refactor: Use only one temporary buffer in CreateObfuscateKey (MarcoFalke) fa05ddd refactor: Use CPubKey vector constructor where possible (MarcoFalke) fabb6df script: Replace address-of idiom with vector data() method (Guido Vranken) Pull request description: The main theme of this refactor is to replace `&foo[0]` with `foo.data()`. The first commit is taken from #21781 with the rationale: * In CSignatureCache::ComputeEntryECDSA, change the way a vector pointer is resolved to prevent invoking undefined behavior if the vector is empty. The other commits aim to remove all `&foo[0]`, where `foo` is any kind of byte representation. The rationale: * Sometimes alternative code without any raw data pointers is easier to read (refer to the respective commit message for details) * If the raw data pointer is needed, `foo.data()` should be preferred, as pointed out in the developer notes. This addresses the instances that have been missed in commit 592404f, and #9804 ACKs for top commit: laanwj: Code review ACK fac30ee practicalswift: cr ACK fac30ee: patch looks correct promag: Code review ACK fac30ee. Tree-SHA512: e7e73146edbc78911a8e8c728b0a1c6b0ed9a88a008e650aa5dbffe72425bd42c76df70199a9cf7e02637448d7593e0eac52fd0f91f59240283e1390ee21bfa5

DrahtBot · 2021-05-05T17:01:21Z

🐙 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".}

jnewbery

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.

src/crypto/chacha20.cpp

maflcko · 2021-05-06T08:08:43Z

Are there steps to reproduce the ChaCha20 issue?

maflcko · 2021-05-21T08:30:33Z

@guidovranken Are you still working on this?

guidovranken · 2021-05-21T23:58:29Z

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?

maflcko · 2021-05-22T04:28:28Z

See #21781 (comment)

maflcko · 2021-05-22T04:30:38Z

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 · 2021-05-25T00:17:50Z

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?

maflcko · 2021-05-25T05:31:38Z

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

guidovranken · 2021-05-25T20:49:41Z

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 · 2021-12-15T11:21:03Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

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 · 2022-03-21T13:06:37Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

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.

maflcko · 2022-03-21T14:10:25Z

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: #21817

guidovranken added 2 commits April 26, 2021 21:47

crypto: Throw exception on passing ChaCha20 key of invalid size

b3f800f

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

65a3368

guidovranken changed the title ~~Fix setkey computeentryecdsa~~ Security enhancements to ChaCha20::SetKey and CSignatureCache::ComputeEntryECDSA Apr 26, 2021

DrahtBot added Consensus Utils/log/libs labels Apr 26, 2021

maflcko reviewed Apr 27, 2021

View reviewed changes

practicalswift mentioned this pull request Apr 29, 2021

refactor: Avoid UB in util/asmap (advance a dereferenceable iterator outside its valid range) #21802

Merged

maflcko mentioned this pull request Apr 30, 2021

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

Merged

maflcko approved these changes May 1, 2021

View reviewed changes

src/crypto/chacha20.cpp Outdated Show resolved Hide resolved

crypto: Replace exception with assert to validate ChaCha20 key size

562cfc1

maflcko removed Consensus Utils/log/libs labels May 5, 2021

DrahtBot added the Needs rebase label May 5, 2021

jnewbery reviewed May 6, 2021

View reviewed changes

src/crypto/chacha20.cpp Show resolved Hide resolved

crypto: Update ChaCha20 method declaration comments

6542bf5

maflcko closed this Mar 21, 2022

maflcko mentioned this pull request Jul 25, 2022

crypto: avoid potential buffer overread in ChaCha20::SetKey #25698

Closed

bitcoin locked and limited conversation to collaborators Mar 21, 2023

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

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

Uh oh!

Conversation

guidovranken commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Apr 26, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 1, 2021

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laanwj commented May 4, 2021

Uh oh!

DrahtBot commented May 5, 2021

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented May 6, 2021

Uh oh!

maflcko commented May 21, 2021

Uh oh!

guidovranken commented May 21, 2021

Uh oh!

maflcko commented May 22, 2021

Uh oh!

maflcko commented May 22, 2021

Uh oh!

guidovranken commented May 25, 2021

Uh oh!

maflcko commented May 25, 2021

Uh oh!

guidovranken commented May 25, 2021

Uh oh!

DrahtBot commented Dec 15, 2021

Uh oh!

DrahtBot commented Mar 21, 2022

Uh oh!

maflcko commented Mar 21, 2022

Uh oh!

Uh oh!

guidovranken commented Apr 26, 2021 •

edited

Loading

practicalswift commented Apr 27, 2021 •

edited

Loading