crypto: more `Span<std::byte>` modernization & follow-ups

sipa commented at 6:34 pm on July 18, 2023: member

This modernizes the ChaCha20 and ChaCha20Aligned interfaces to be Span<std::byte> based, and other improvements.

Modifies all functions and constructors of ChaCha20 and ChaCha20Aligned to be Span<std::byte> based (aligning them with FSChaCha20, AEADChaCha20Poly1305, and FSChaCha20Poly1305)
Remove default constructors, to make sure all call sites provide a key (suggested in #26153 (review))
Wipe key material on rekey for security (suggested in #26153 (review))
Use HexStr on byte vectors in tests (suggested in #27993 (review))
Support std::byte vectors in ConsumeRandomLengthByteVector and ConsumeFixedLengthByteVector, and use it (suggested in #27993 (review))
And a few more.

While related, I don’t see this as a necessary for BIP324.

DrahtBot commented at 6:34 pm on July 18, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theStack, stratospher
Stale ACK	MarcoFalke

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label Utils/log/libs on Jul 18, 2023

in src/random.cpp:668 in 9e1199c7ce outdated

664@@ -664,8 +665,8 @@ FastRandomContext::FastRandomContext(bool fDeterministic) noexcept : requires_se
665     if (!fDeterministic) {
666         return;
667     }
668-    uint256 seed;
669-    rng.SetKey32(seed.begin());
670+    static constexpr std::array<std::byte, rng.KEYLEN> ZERO{};

jonatack commented at 7:10 pm on July 18, 2023:

In commit 9e1199c7ce5

0random.cpp:668:44: error: non-type template argument is not a constant expression
1    static constexpr std::array<std::byte, rng.KEYLEN> ZERO{};
2                                           ^~~~~~~~~~
3random.cpp:668:44: note: implicit use of 'this' pointer is only allowed within the evaluation of a call to a 'constexpr' member function
41 error generated.

0$ clang --version
1Homebrew clang version 16.0.6
2Target: arm64-apple-darwin22.5.0
3Thread model: posix
4InstalledDir: /opt/homebrew/opt/llvm/bin

sipa commented at 7:16 pm on July 18, 2023:

Strange that GCC doesn’t complain about this. Fixed.

in src/bench/chacha20.cpp:16 in 9e1199c7ce outdated

12@@ -13,13 +13,13 @@ static const uint64_t BUFFER_SIZE_LARGE = 1024*1024;
13 
14 static void CHACHA20(benchmark::Bench& bench, size_t buffersize)
15 {
16-    std::vector<uint8_t> key(32,0);
17-    ChaCha20 ctx(key.data());
18-    ctx.Seek64({0, 0}, 0);
19-    std::vector<uint8_t> in(buffersize,0);
20-    std::vector<uint8_t> out(buffersize,0);
21+    std::vector<std::byte> key(32, std::byte{});

maflcko commented at 7:13 pm on July 18, 2023:

nit in 9e1199c7ce5d9a29c6ddf3dc28d8814bb1190a64: Can replace std::byte{} with {}, if it compiles, same below?

sipa commented at 1:17 am on August 14, 2023:

Done.

sipa force-pushed on Jul 18, 2023

in src/random.cpp:599 in 2ff930de1c outdated

595@@ -595,7 +596,7 @@ std::vector<B> FastRandomContext::randbytes(size_t len)
596     if (requires_seed) RandomSeed();
597     std::vector<B> ret(len);
598     if (len > 0) {
599-        rng.Keystream(UCharCast(ret.data()), len);
600+        rng.Keystream(MakeWritableByteSpan(ret));

maflcko commented at 7:18 pm on July 18, 2023:

9e1199c7ce5d9a29c6ddf3dc28d8814bb1190a64: Could replace this whole function with:

0fillrand(MakeWritableByteSpan(ret));

sipa commented at 1:17 am on August 14, 2023:

Done in a separate commit.

in src/test/fuzz/crypto_chacha20.cpp:19 in b1faef7cf4 outdated

19-    if (fuzzed_data_provider.ConsumeBool()) {
20-        const std::vector<unsigned char> key = ConsumeFixedLengthByteVector(fuzzed_data_provider, 32);
21-        chacha20 = ChaCha20{MakeByteSpan(key)};
22-    }
23+    // Just for compatibility with old seeds (it used to test the default constructor which used zero key)
24+    if (!fuzzed_data_provider.ConsumeBool()) return;

maflcko commented at 7:22 pm on July 18, 2023:

Any reason to do this? Fuzz inputs may already invalidate for any code change and maintaining compat for some code changes seems inconsistent, will bloat the fuzz code, and cause maintenance overhead in the future.

Would be good to either remove this, or if you actually want to maintain compat use a ternary operator:

0const auto key{fdp.ConsumeBool()?ConsumeFixedLen(...):std::vec<std::byte>(32)};

sipa commented at 1:18 am on August 14, 2023:

I’ve dropped it.

in src/test/fuzz/crypto_diff_fuzz_chacha20.cpp:275 in b1faef7cf4 outdated

280-    } else {
281-        // The default ChaCha20 constructor is equivalent to using the all-0 key.
282-        ECRYPT_keysetup(&ctx, ZEROKEY, 256, 0);
283-    }
284+    // Just for compatibility with old seeds (it used to test the default constructor which used zero key)
285+    if (!fuzzed_data_provider.ConsumeBool()) return;

maflcko commented at 7:22 pm on July 18, 2023:

same

sipa commented at 1:18 am on August 14, 2023:

I’ve dropped it.

sipa renamed this:
~~crypto: ChaCha20 `Span<std::byte>` modernization & follow-ups~~
crypto: more `Span<std::byte>` modernization & follow-ups
on Jul 18, 2023

sipa force-pushed on Jul 18, 2023

fanquake requested review from theStack on Aug 2, 2023

theStack commented at 0:19 am on August 8, 2023: contributor

Concept ACK

in src/crypto/chacha20.h:36 in 2ff930de1c outdated

32+     */
33+    void Crypt(Span<const std::byte> input, Span<std::byte> output) noexcept;
34+
35 public:
36-    ChaCha20Aligned();
37+    /** Expected key length in constructor and SetKey32. */

maflcko commented at 11:03 am on August 8, 2023:

2ff930de1c7f4683d0a7a72970683e21bf20ba73: SetKey. (no 32)

sipa commented at 1:18 am on August 14, 2023:

Done.

in src/crypto/chacha20.cpp:185 in 2ff930de1c outdated

194+    j10 = this->input[6];
195+    j11 = this->input[7];
196+    j12 = this->input[8];
197+    j13 = this->input[9];
198+    j14 = this->input[10];
199+    j15 = this->input[11];

maflcko commented at 11:05 am on August 8, 2023:

2ff930de1c7f4683d0a7a72970683e21bf20ba73: My preference would be to instead base this on a new commit that just does a input -> m_input (“scripted-diff”) rename

sipa commented at 1:19 am on August 14, 2023:

I have instead used different argument names (in_bytes and out_bytes) to avoid a collision with the member variable altogether.

in src/test/crypto_tests.cpp:170 in 2ff930de1c outdated

165@@ -166,14 +166,14 @@ static void TestChaCha20(const std::string &hex_message, const std::string &hexk
166         lens[1] = InsecureRandRange(hexout.size() / 2U + 1U - lens[0]);
167         lens[2] = hexout.size() / 2U - lens[0] - lens[1];
168 
169-        rng.Seek64(nonce, seek);
170-        outres.assign(hexout.size() / 2U, 0);
171+        rng.Seek(nonce, seek);
172+        outres.assign(hexout.size() / 2U, std::byte{});

maflcko commented at 11:12 am on August 8, 2023:

nit in 2ff930de1c7f4683d0a7a72970683e21bf20ba73: Could use {} over std::byte{}, if it compiles?

sipa commented at 1:19 am on August 14, 2023:

Done.

in src/test/crypto_tests.cpp:838 in 2ff930de1c outdated

705+    c20.Keystream(b2);
706+    c20.Keystream(b3);
707+
708+    BOOST_CHECK(Span{block}.first(5) == Span{b1});
709+    BOOST_CHECK(Span{block}.subspan(5, 7) == Span{b2});
710+    BOOST_CHECK(Span{block}.last(52) == Span{b3});

maflcko commented at 11:15 am on August 8, 2023:

unrelated: In C++20, we’ll probably have to provide overloads for operator==(std::span, std::span), because equality checks are only defined in the std lib for containers that store memory, it seems.

sipa commented at 1:24 am on August 14, 2023:

Indeed, I’m aware.

Just brainstorming at this point, but perhaps we don’t want to actually add an overload for an STL class operator. Perhaps std::ranges::equal can be used instead.

maflcko approved

maflcko commented at 11:30 am on August 8, 2023: member

nice ACK 153a6745fa523a3fd0ccce1304ceb75d4511df20 🔖

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: nice ACK 153a6745fa523a3fd0ccce1304ceb75d4511df20 🔖
3pMD3fmJquqJLFEdbollqeq+L8nBymi3uLOeQKTb65R/+y65NKCnTWqnk7Flu+t57wNz+Avhshe+x1CU260NoDA==

DrahtBot added the label Needs rebase on Aug 10, 2023

sipa force-pushed on Aug 14, 2023

DrahtBot removed the label Needs rebase on Aug 14, 2023

sipa commented at 2:28 am on August 14, 2023: member

Rebased after merge of #28008, and addresses feedback. I’ve dropped the Crypt -> Encrypt / Decrypt change as I felt the duplication that resulted wasn’t worth it anymore.

DrahtBot added the label CI failed on Aug 14, 2023

sipa force-pushed on Aug 14, 2023

DrahtBot removed the label CI failed on Aug 14, 2023

fanquake commented at 11:55 am on August 14, 2023: member

Some changes here are also in #28008, but either or both can go in. @sipa could you update the PR dscription in regards to this, now that #28008 has been merged.

cc @stratospher you might be interested in reviewing here?

in src/crypto/chacha20.h:116 in bc0a7145eb outdated

134+    /** en/deciphers the message <in_bytes> and write the result into <out_bytes>
135+     *
136+     * The size of in_bytes and out_bytes must be equal.
137      */
138-    void Crypt(const unsigned char* input, unsigned char* output, size_t bytes);
139+    void Crypt(Span<const std::byte> in_bytes, Span<std::byte> out_output) noexcept;

stratospher commented at 1:02 pm on August 14, 2023:

bc0a714:

0    void Crypt(Span<const std::byte> in_bytes, Span<std::byte> out_bytes) noexcept;

sipa commented at 7:43 pm on August 17, 2023:

Fixed.

in src/random.cpp:668 in bc0a7145eb outdated

664@@ -664,8 +665,8 @@ FastRandomContext::FastRandomContext(bool fDeterministic) noexcept : requires_se
665     if (!fDeterministic) {
666         return;
667     }
668-    uint256 seed;
669-    rng.SetKey32(seed.begin());
670+    static constexpr std::array<std::byte, ChaCha20::KEYLEN> ZERO{};

stratospher commented at 2:27 pm on August 14, 2023:

bc0a714: maybe include ChaCha20 in the headers?

sipa commented at 7:43 pm on August 17, 2023:

Done.

sipa commented at 5:32 pm on August 14, 2023: member

@sipa could you update the PR dscription in regards to this, now that #28008 has been merged. @fanquake Done.

in src/crypto/chacha20.h:106 in baf93fbd47 outdated

122-    void Seek64(Nonce96 nonce, uint32_t block_counter)
123+    /** Set the 96-bit nonce and 32-bit block counter. See ChaCha20Aligned::Seek. */
124+    void Seek(Nonce96 nonce, uint32_t block_counter) noexcept
125     {
126-        m_aligned.Seek64(nonce, block_counter);
127+        m_aligned.Seek(nonce, block_counter);

stratospher commented at 4:35 am on August 15, 2023:

bc0a714: did #26153 (review) mean clearing m_buffer too when m_bufleft is 0?

sipa commented at 7:44 pm on August 17, 2023:

Good catch, yes. I’ve added a commit that makes ChaCha20::SetKey wipe the buffer.

I don’t think we should in general wipe the buffer whenever m_bufleft=0, but on a rekey operation it makes sense.

stratospher commented at 5:54 am on August 15, 2023: contributor

ACK baf93fb. neat c++!

DrahtBot requested review from maflcko on Aug 15, 2023

maflcko commented at 8:24 am on August 15, 2023: member

ACK baf93fbd47058b182643c5a11fe9a8928276b05f 🙉

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK baf93fbd47058b182643c5a11fe9a8928276b05f  🙉
3bVwXQLzRdyPR0w0spuTcbS3DX/txQkFYl41x1X2FfdLOU1CyGgw5vele1a0LWd0FeAIO4nLjR7ZBleZwl4c0Dw==

DrahtBot removed review request from maflcko on Aug 15, 2023

theStack approved

theStack commented at 0:17 am on August 16, 2023: contributor

Code-review ACK baf93fbd47058b182643c5a11fe9a8928276b05f

Potential follow-up nit: there might be some more instances where using BOOST_CHECK_EQUAL over BOOST_CHECK could make sense, see git grep "BOOST_CHECK(.*==" ./src/test/crypto_tests.cpp.

fanquake commented at 2:33 pm on August 17, 2023: member

@sipa when you get a chance to respond to review comments here, we can decide to either merge as-is, and address in a followup, or touch up here.

maflcko commented at 2:44 pm on August 17, 2023: member

I think they should either be fixed up here or not at all. I don’t think it makes sense to create a follow-up to a follow-up.

sipa commented at 2:45 pm on August 17, 2023: member

I will address them.

crypto: refactor ChaCha20 classes to use Span<std::byte> interface 3da636e08b

random: simplify FastRandomContext::randbytes using fillrand 44c11769a8

crypto: require key on ChaCha20 initialization 7d1cd93234

fuzz: support std::byte in Consume{Fixed,Variable}LengthByteVector bdcbc8594c

tests: miscellaneous hex / std::byte improvements da0ec62e34

crypto: make ChaCha20::SetKey wipe buffer 57cc136282

sipa force-pushed on Aug 17, 2023

sipa commented at 7:52 pm on August 17, 2023: member

Addressed review comments by @stratospher. Note: new last commit.

theStack approved

theStack commented at 11:07 pm on August 17, 2023: contributor

re-ACK 57cc136282c38825e97bbf85728df4bdf1ccc648

(CI failure is unrelated)

DrahtBot requested review from maflcko on Aug 17, 2023

DrahtBot requested review from stratospher on Aug 17, 2023

stratospher commented at 3:50 am on August 18, 2023: contributor

ACK 57cc136.

DrahtBot removed review request from stratospher on Aug 18, 2023

DrahtBot added the label CI failed on Aug 18, 2023

fanquake merged this on Aug 18, 2023

fanquake closed this on Aug 18, 2023

Frank-GER referenced this in commit e6242939ee on Sep 8, 2023

kwvg referenced this in commit 6e06927969 on Feb 20, 2024

kwvg referenced this in commit 6f5afb66e2 on Feb 23, 2024

kwvg referenced this in commit 4abb0bb6f7 on Feb 24, 2024

kwvg referenced this in commit 8db1f7c1cf on Feb 27, 2024

kwvg referenced this in commit 48eff83c6d on Feb 27, 2024

kwvg referenced this in commit 9aee22f206 on Feb 29, 2024

kwvg referenced this in commit da40c7da5f on Mar 5, 2024

kwvg referenced this in commit b60c493265 on Mar 5, 2024

PastaPastaPasta referenced this in commit e84e3d9c85 on Mar 6, 2024

bitcoin locked this on Aug 17, 2024

crypto: more Span<std::byte> modernization & follow-ups #28100

Reviews

Conflicts

crypto: more `Span<std::byte>` modernization & follow-ups #28100