BIP324: Cipher suite #25361

pull dhruv wants to merge 5 commits into bitcoin:master from dhruv:bip324-cipher-suite changing 19 files +1012 −597
  1. dhruv commented at 3:40 pm on June 13, 2022: contributor

    This PR supersedes #20962 and introduces a two-layered cipher suite used in the latest draft of BIP324.

    • Inner layer uses RFC8439 which comes with a formal security analysis so any novelty introduced by our cipher suite still offers a baseline confidence in confidentiality and authenticity. The RFC8439 instance is re-keyed every 256 messages for forward secrecy.
    • Outer layer uses a forward secure version of ChaCha20, FSChaCha20 which re-keys itself every 256 messages for forward secrecy. It is used to encrypt the message length resulting in a pseudorandom byte stream.

    The dependency tree for BIP324 PRs is here.

  2. fanquake added the label P2P on Jun 13, 2022
  3. DrahtBot added the label Needs rebase on Jun 14, 2022
  4. dhruv force-pushed on Jun 25, 2022
  5. dhruv commented at 3:51 am on June 25, 2022: contributor
    Rebased
  6. dhruv force-pushed on Jun 25, 2022
  7. DrahtBot removed the label Needs rebase on Jun 25, 2022
  8. DrahtBot commented at 4:16 pm on June 25, 2022: 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. A summary of reviews will appear here.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #27479 (BIP324: ElligatorSwift integrations by sipa)
    • #26684 (bench: add readblock benchmark by andrewtoth)

    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.

  9. in src/test/crypto_tests.cpp:250 in 340c1ca331 outdated 
    173+        c20.SetRFC8439Nonce(nonce);
174+        c20.SeekRFC8439(seek);
175+        std::vector<unsigned char> keystream;
176+        keystream.resize(CHACHA20_ROUND_OUTPUT);
177+        c20.Keystream(keystream.data(), CHACHA20_ROUND_OUTPUT);
178+        BOOST_CHECK_EQUAL(HexStr(keystream).substr(0, hex_expected_keystream.size()), hex_expected_keystream);
    stratospher commented at 8:13 am on June 28, 2022:
    is there any particular reason why the entire CHACHA20_ROUND_OUTPUT(64 bytes of keystream) is not being compared? for example, the test vectors in L571-L576 compare only first 32 bytes of keystream.
    dhruv commented at 2:44 am on July 7, 2022:
    No reason other than I’m using the test vectors as-is from the hyperlink in the comment before those vectors.
  10. dhruv force-pushed on Jun 30, 2022
  11. dhruv commented at 4:15 am on June 30, 2022: contributor
    Found necessary fixes from sanitizers, fuzz tests and unit tests in downstream branches. Pushed those changes. Ready for further review.
  12. dhruv force-pushed on Jul 6, 2022
  13. dhruv commented at 7:44 pm on July 6, 2022: contributor
    Updated to correctly rekey using CSHA256 instead of CHash256. Thanks for the catch, @stratospher !
  14. dhruv force-pushed on Sep 11, 2022
  15. dhruv commented at 8:17 pm on September 11, 2022: contributor

    Updated to:

    • Expose the RFC8439 AAD via the BIP324CipherSuite interface.
    • Remove multiple plaintext interface from RFC8439 – it ended up not being super helpful to get to the optimization I was trying
    • No longer expose the mac tag from RFC8439 - the tag is a detail internal to the AEAD and the ciphertext + mac is now treated as a blob

    Ready for further review.

  16. in src/crypto/chacha20.h:53 in 9d9ffc4a59 outdated 
    44@@ -39,4 +45,48 @@ class ChaCha20
45     void Crypt(const unsigned char* input, unsigned char* output, size_t bytes);
46 };
47 
48+class FSChaCha20
49+{
50+private:
51+    ChaCha20 c20;
52+    size_t rekey_interval;
53+    uint32_t messages_with_key;
    stratospher commented at 8:08 am on September 18, 2022:

    9d9ffc4: naming it as a counter would be more intuitive.

    0    uint32_t message_counter;
    dhruv commented at 4:32 am on September 29, 2022:
    Fixed.
  17. in src/crypto/bip324_suite.cpp:62 in 361e971893 outdated 
    57+bool BIP324CipherSuite::Crypt(const Span<const std::byte> input, Span<std::byte> output,
58+                              BIP324HeaderFlags& flags, bool encrypt)
59+{
60+    // check buffer boundaries
61+    if (
62+        // if we encrypt, make sure the destination has the space for the length field, header, ciphertext and MAC
    stratospher commented at 8:11 am on September 18, 2022:

    361e9718:

    0        // if we encrypt, make sure the destination has the space for the length field, header, payload ciphertext and MAC
    dhruv commented at 4:32 am on September 29, 2022:
    Updated.
  18. in src/crypto/bip324_suite.cpp:76 in 361e971893 outdated 
    71+        // output will be encrypted length + encrypted (header and payload) + mac tag
72+        uint32_t ciphertext_len = BIP324_HEADER_LEN + input.size();
73+        WriteLE32(reinterpret_cast<unsigned char*>(&ciphertext_len), ciphertext_len);
74+
75+        std::vector<std::byte> input_vec;
76+        input_vec.resize(BIP324_HEADER_LEN + input.size());
    stratospher commented at 8:13 am on September 18, 2022:
    361e971: input_vec can be defined directly with the required size.
    dhruv commented at 4:32 am on September 29, 2022:
    Fixed.
  19. in src/crypto/bip324_suite.h:48 in 701a967fde outdated 
    43+
44+public:
45+    BIP324CipherSuite(const BIP324Key& K_L, const BIP324Key& K_P, const BIP324Key& rekey_salt)
46+        : fsc20{K_L, rekey_salt, REKEY_INTERVAL},
47+          rekey_ctr{0},
48+          msg_ctr{0},
    aureleoules commented at 11:49 am on September 21, 2022:
    use class initializer instead
    dhruv commented at 4:32 am on September 29, 2022:
    Fixed.
  20. in src/crypto/bip324_suite.h:21 in 701a967fde outdated 
    16+static constexpr size_t BIP324_HEADER_LEN = 1;       // bytes
17+static constexpr size_t BIP324_LENGTH_FIELD_LEN = 3; // bytes
18+static constexpr size_t REKEY_INTERVAL = 256;        // messages
19+static constexpr size_t NONCE_LENGTH = 12;           // bytes
20+
21+enum BIP324HeaderFlags : uint8_t {
    aureleoules commented at 12:44 pm on September 21, 2022:
    maybe use the scoped enum class instead?
    dhruv commented at 4:31 am on September 29, 2022:
    It’s useful to have the implicit type conversion here for fuzz testing as the header flags are a byte that a peer could stuff with arbitrary bits.
  21. in src/crypto/chacha20.h:74 in 701a967fde outdated 
    69+               const std::array<std::byte, FSCHACHA20_KEYLEN>& rekey_salt,
70+               size_t rekey_interval)
71+        : c20{reinterpret_cast<const unsigned char*>(key.data()), FSCHACHA20_KEYLEN},
72+          rekey_interval{rekey_interval},
73+          messages_with_key{0},
74+          rekey_counter{0},
    aureleoules commented at 12:52 pm on September 21, 2022:
    use class initializer
    dhruv commented at 4:31 am on September 29, 2022:
    Fixed.
  22. in src/bench/rfc8439.cpp:8 in 361e971893 outdated 
    4@@ -5,6 +5,7 @@
5 #include <assert.h>
6 #include <bench/bench.h>
7 #include <crypto/rfc8439.h>
8+#include <span.h>
    stratospher commented at 6:48 pm on September 21, 2022:
    361e971: changes in src/bench/rfc8439.cpp, src/crypto/rfc8439.h, src/test/fuzz/crypto_rfc8439.cpp can be moved to the previous previous commit. (a2a038a)
    dhruv commented at 4:31 am on September 29, 2022:
    Done where applicable.
  23. DrahtBot added the label Needs rebase on Sep 25, 2022
  24. dhruv force-pushed on Sep 29, 2022
  25. dhruv commented at 4:33 am on September 29, 2022: contributor

    Latest push:

  26. DrahtBot removed the label Needs rebase on Sep 29, 2022
  27. dhruv force-pushed on Oct 1, 2022
  28. dhruv commented at 4:40 am on October 1, 2022: contributor

    Latest push:

    • Changes in working group:
      • Nomenclature changes
      • Encrypted packet length is now just length of contents rather than len(header + contents)
      • Rekey salt is 23 bytes instead of 32 bytes to allow just one SHA256 compression to achieve new_key = SHA256(rekey_salt || old_key)

    Improvements:

    • Cache mid-state when rekeying

    Bug fixes:

    • Found that previous code was using the little endian contents_len to perform encryption. Fixed.
  29. DrahtBot added the label Needs rebase on Oct 19, 2022
  30. dhruv force-pushed on Oct 20, 2022
  31. dhruv commented at 10:14 pm on October 20, 2022: contributor
    Rebased. Bringing in upstream changes, ready for further review.
  32. dhruv force-pushed on Oct 21, 2022
  33. dhruv commented at 5:40 am on October 21, 2022: contributor
    Rebased.
  34. DrahtBot removed the label Needs rebase on Oct 21, 2022
  35. DrahtBot added the label Needs rebase on Nov 19, 2022
  36. DrahtBot removed the label Needs rebase on Nov 21, 2022
  37. dhruv force-pushed on Nov 23, 2022
  38. dhruv commented at 5:10 am on November 23, 2022: contributor
    Bringing in upstream rebase (#26153)
  39. DrahtBot added the label Needs rebase on Nov 30, 2022
  40. dhruv force-pushed on Dec 15, 2022
  41. dhruv commented at 4:50 pm on December 15, 2022: contributor

    Latest push:

    • Rebased upstream changes
    • New rekey ratchet mechanism (forward security) based on ChaCha20 instead of SHA256. BIP changes are WIP and coming soon.
  42. DrahtBot removed the label Needs rebase on Dec 15, 2022
  43. DrahtBot added the label Needs rebase on Dec 25, 2022
  44. dhruv force-pushed on Jan 12, 2023
  45. dhruv commented at 7:07 pm on January 12, 2023: contributor
    Rebased
  46. DrahtBot removed the label Needs rebase on Jan 12, 2023
  47. dhruv force-pushed on Jan 23, 2023
  48. dhruv commented at 10:12 pm on January 23, 2023: contributor
    Rebased changes from #26153
  49. dhruv force-pushed on Feb 2, 2023
  50. dhruv commented at 6:25 am on February 2, 2023: contributor
    Rebased. Brought in changes from #26153.
  51. DrahtBot added the label Needs rebase on Feb 15, 2023
  52. RFC8439 nonce and counter for ChaCha20 8e04f058a4
  53. RFC8439 implementation and tests 7a9d2fb8cf
  54. Adding forward secure FSChaCha20 acd664e805
  55. dhruv force-pushed on Feb 21, 2023
  56. dhruv commented at 3:31 am on February 21, 2023: contributor
    Rebased.
  57. DrahtBot removed the label Needs rebase on Feb 21, 2023
  58. in src/crypto/bip324_suite.cpp:28 in 225023d9e3 outdated 
    23+    for (; n > 0; n--)
24+        ret |= *p1++ ^ *p2++;
25+    return (ret != 0);
26+}
27+
28+#endif // TIMINGSAFE_BCMP
    theStack commented at 0:31 am on March 19, 2023:

    In commit 225023d9e3ea5a89037ef8a4f4404a0fdd3f1cf7: This function is not used anywhere in this file and hence can be removed (or rather moved to rfc8439.cpp, see other review comment).

    dhruv commented at 3:56 am on March 21, 2023:
    I think I forgot to remove this previously used code. Thanks for pointing it out.
  59. in src/crypto/rfc8439.cpp:25 in 7a9d2fb8cf outdated 
    20+    for (; n > 0; n--)
21+        ret |= *p1++ ^ *p2++;
22+    return (ret != 0);
23+}
24+
25+#endif // RFC8439_TIMINGSAFE_BCMP
    theStack commented at 0:52 am on March 19, 2023:

    In commit 7a9d2fb8cf11eef8b6bd87ad1066df374bf3b620: Is there any reason to prefix the define and the function with “rfc8439”? Note that the point here is to let the build system detect if the function timingsafe_bcmp is available on the system (if yes, the define HAVE_TIMINGSAFE_BCMP is set) and only use the custom implementation if we need to. With this current construct, we would always use the custom one, since RFC8439_TIMINGSAFE_BCMP would obviously never be set. This should probably just be like on master (or in bip324_suite.cpp, see other review comment):

     0#ifndef HAVE_TIMINGSAFE_BCMP
 1#define HAVE_TIMINGSAFE_BCMP
 2
 3int timingsafe_bcmp(const unsigned char* b1, const unsigned char* b2, size_t n)
 4{
 5    const unsigned char *p1 = b1, *p2 = b2;
 6    int ret = 0;
 7
 8    for (; n > 0; n--)
 9        ret |= *p1++ ^ *p2++;
10    return (ret != 0);
11}
12
13#endif // HAVE_TIMINGSAFE_BCMP
    dhruv commented at 3:57 am on March 21, 2023:
    Done. Although I did leave it as-is in the second commit and addressed it in the fourth commit because I was getting a linker error for multiple definitions trying it on the second commit. The net change isn’t different however.
    fanquake commented at 7:34 pm on March 21, 2023:

    The net change isn’t different however.

    No, the net change is you’ve pointlessly broken things like git bisect. Please try and avoid doing that.

    dhruv commented at 8:13 pm on March 21, 2023:
    @fanquake not sure I understand. At which commit is the build broken? I specifically did it in a way that avoided that but may be misunderstanding something.
    fanquake commented at 8:24 pm on March 21, 2023:
    @dhruv ah, my misread was that you’d left the linker errors between commits 2 & 4. Although, I think introducing this, just to change in later commits is still not ideal. Will re-mark this as resolved, and post new review below.
  60. BIP324 Cipher Suite 187761fc8a
  61. Allow for RFC8439 AD in cipher suite interface 8405856ed9
  62. dhruv force-pushed on Mar 21, 2023
  63. dhruv commented at 3:58 am on March 21, 2023: contributor
    Addressed review by @theStack.
  64. in src/crypto/rfc8439.cpp:33 in 7a9d2fb8cf outdated 
    28+{
29+    return (len % 16 == 0) ? len : (len / 16 + 1) * 16;
30+}
31+
32+void ComputeRFC8439Tag(const std::array<std::byte, POLY1305_KEYLEN>& polykey,
33+                       Span<const std::byte> aad, Span<const std::byte> ciphertext,
    theStack commented at 7:03 pm on March 21, 2023:

    nit: const-correctness

    0                       const Span<const std::byte> aad, const Span<const std::byte> ciphertext,
  65. in src/crypto/rfc8439.cpp:60 in 7a9d2fb8cf outdated 
    55+    std::array<std::byte, POLY1305_KEYLEN> polykey;
56+    c20.Keystream(reinterpret_cast<unsigned char*>(polykey.data()), POLY1305_KEYLEN);
57+    return polykey;
58+}
59+
60+void RFC8439Crypt(ChaCha20& c20, const Span<const std::byte> in_bytes, Span<std::byte> out_bytes)
    theStack commented at 7:04 pm on March 21, 2023:

    nit: seems like this function is never needed in another module, also not in follow-up PRs (checked at #24545)

    0static void RFC8439Crypt(ChaCha20& c20, const Span<const std::byte> in_bytes, Span<std::byte> out_bytes)
  66. theStack commented at 7:04 pm on March 21, 2023: contributor

    Addressed review by @theStack.

    Thanks! Another (non-blocking) suggestion: considering that the keys for RFC8439 have a fixed size, would it be worth it to introduce a RFC8439Key type that uses std::array<std::byte, RFC8439_KEYLEN>? I feel that’s the slightly cleaner interface with enforcing the length already at compile-time, and we could remove the asserts in the RFC8439Encrypt/RFC8439Decrypt functions. The only annoying part is that the test/benchmark/fuzz code gets a bit longer, as one needs to convert from vector to array, and for the zero_key, one can’t use the comfortable constructor for initializing the object with a repeated count of items.

  67. in src/crypto/rfc8439.cpp:15 in 7a9d2fb8cf outdated 
    10+#include <cstring>
11+
12+#ifndef RFC8439_TIMINGSAFE_BCMP
13+#define RFC8439_TIMINGSAFE_BCMP
14+
15+int rfc8439_timingsafe_bcmp(const unsigned char* b1, const unsigned char* b2, size_t n)
    fanquake commented at 10:36 am on March 22, 2023:
    In 7a9d2fb8cf11eef8b6bd87ad1066df374bf3b620: It is fairly awkward to introduce a second copy of timingsafe_bcmp (one already in chacha_poly_aead), with a different name, and broken #ifdef logic, just to then rename it when you delete the original timingsafe_bcmp in a later commit. Although I guess re-using/extracting timingsafe_bcmp early from the to-be-deleted file is also a bit awkward.
  68. in src/crypto/rfc8439.h:19 in 7a9d2fb8cf outdated 
    14+
15+constexpr static size_t RFC8439_KEYLEN = 32;
16+
17+void RFC8439Encrypt(const Span<const std::byte> aad, const Span<const std::byte> key, const std::array<std::byte, 12>& nonce, const Span<const std::byte> plaintext, Span<std::byte> output);
18+
19+// returns false if authentication fails
    fanquake commented at 10:37 am on March 22, 2023:
    In 7a9d2fb8cf11eef8b6bd87ad1066df374bf3b620: Make these Doxygen comments?
  69. in src/crypto/rfc8439.h:15 in 8405856ed9 
    10+
11+#include <array>
12+#include <cstddef>
13+#include <vector>
14+
15+constexpr static size_t RFC8439_KEYLEN = 32;
    fanquake commented at 10:39 am on March 22, 2023:

    In 7a9d2fb8cf11eef8b6bd87ad1066df374bf3b620: These could be

    0constexpr static size_t RFC8439_KEYLEN{32};
  70. in src/bench/rfc8439.cpp:5 in 8405856ed9 
    0@@ -0,0 +1,76 @@
1+// Copyright (c) 2022 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#include <assert.h>
    fanquake commented at 10:40 am on March 22, 2023:
    In 7a9d2fb8cf11eef8b6bd87ad1066df374bf3b620: nit use c++ headers, i.e <cassert>
  71. in src/bench/bip324_suite.cpp:4 in 8405856ed9 
    0@@ -0,0 +1,117 @@
1+// Copyright (c) 2019-2020 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
    fanquake commented at 10:42 am on March 22, 2023:
    nit: additional newline
  72. in src/bench/bip324_suite.cpp:9 in 8405856ed9 
    0@@ -0,0 +1,117 @@
1+// Copyright (c) 2019-2020 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
6+#include <assert.h>
7+#include <bench/bench.h>
8+#include <crypto/bip324_suite.h>
9+#include <crypto/rfc8439.h> // for the RFC8439_EXPANSION constant
    fanquake commented at 10:43 am on March 22, 2023:
    Please don’t add for xyz comments. They are unmaintainable, and if someone wants to know why an include is used, they can look at IWYU etc. Note that you could also add newly-added files to ci/test/06-script_b.sh to have the includes checked.
  73. in src/crypto/bip324_suite.cpp:54 in 8405856ed9 
    49+        uint32_t contents_len = input.size();
50+        WriteLE32(reinterpret_cast<unsigned char*>(&contents_len), contents_len);
51+
52+        std::vector<std::byte> header_and_contents(BIP324_HEADER_LEN + input.size());
53+
54+        memcpy(header_and_contents.data(), &flags, BIP324_HEADER_LEN);
    fanquake commented at 10:47 am on March 22, 2023: 
    0        std::memcpy(header_and_contents.data(), &flags, BIP324_HEADER_LEN);

    Same for memset etc.

  74. in src/crypto/chacha20.cpp:145 in 8e04f058a4 outdated 
    141@@ -127,7 +142,7 @@ inline void ChaCha20Aligned::Keystream64(unsigned char* c, size_t blocks)
142         x15 += j15;
143 
144         ++j12;
145-        if (!j12) ++j13;
146+        if (!j12 && !is_rfc8439) ++j13;
    sipa commented at 2:40 pm on March 24, 2023:
    I don’t think there is a need for these is_rfc8439 values, because overflowing the block counter in RFC8439 mode is simply not allowed. If it happens, both incrementing j13 or not incrementing it are wrong. If you want to do anything, it should be an assertion failure.
  75. in src/bench/rfc8439.cpp:18 in 7a9d2fb8cf outdated 
    13+static constexpr uint64_t AAD_SIZE = 32;
14+static constexpr uint64_t PLAINTEXT_SIZE_TINY = 64;
15+static constexpr uint64_t PLAINTEXT_SIZE_SMALL = 256;
16+static constexpr uint64_t PLAINTEXT_SIZE_LARGE = 1024 * 1024;
17+
18+static std::vector<std::byte> zero_key(32, std::byte{0x00});
    sipa commented at 2:41 pm on March 24, 2023:
    I think this variable can be constant (and if it isn’t, it probably shouldn’t be a global). Also, globals should be upper case.
  76. in src/bench/rfc8439.cpp:20 in 7a9d2fb8cf outdated 
    15+static constexpr uint64_t PLAINTEXT_SIZE_SMALL = 256;
16+static constexpr uint64_t PLAINTEXT_SIZE_LARGE = 1024 * 1024;
17+
18+static std::vector<std::byte> zero_key(32, std::byte{0x00});
19+static std::vector<std::byte> aad(AAD_SIZE, std::byte{0x00});
20+std::array<std::byte, 12> nonce = {std::byte{0x00}, std::byte{0x01}, std::byte{0x02}, std::byte{0x03},
    sipa commented at 2:42 pm on March 24, 2023:
    static const here as well?
  77. fanquake commented at 11:11 am on May 6, 2023: member
    Closing for now. This will be picked up again later. BIP324 review attention should be directed towards #27479 and https://github.com/bitcoin-core/secp256k1/pull/1129.
  78. fanquake closed this on May 6, 2023
  79. achow101 referenced this in commit b4794740f8 on Jul 12, 2023
  80. sidhujag referenced this in commit 4adf185bcf on Jul 12, 2023
  81. achow101 referenced this in commit 306157ae92 on Jul 17, 2023
  82. sidhujag referenced this in commit 177a9921d0 on Jul 19, 2023
  83. fanquake referenced this in commit b2ec0326fd on Aug 10, 2023
  84. bitcoin locked this on May 5, 2024


dhruv DrahtBot stratospher aureleoules theStack fanquake sipa

Labels
P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-28 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me