The current ChaCha20 implementation does not support message encryption (it can only output the keystream which is sufficient for the RNG).
This PR adds the actual XORing of the plaintext with the keystream in order to return the desired ciphertext.
Required for v2 message transport protocol.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
101@@ -100,6 +102,10 @@ void ChaCha20::Output(unsigned char* c, size_t bytes)
102
103 for (;;) {
104 if (bytes < 64) {
105+ if (m != nullptr) {
106+ for (i = 0;i < bytes;++i) tmp[i] = m[i];
memcpy(tmp, m, bytes) might be simpler and easy to read than this for loop.
151@@ -146,6 +152,25 @@ void ChaCha20::Output(unsigned char* c, size_t bytes)
152 x14 += j14;
153 x15 += j15;
154
155+ if (m != nullptr) {
156+ x0 = XOR(x0, ReadLE32(m + 0));
x0 ^= ReadLE32(m + 0) more readable and less error-prone than x0 = XOR(x0, ReadLE32(m + 0))?
19@@ -20,7 +20,9 @@ class ChaCha20
20 void SetKey(const unsigned char* key, size_t keylen);
21 void SetIV(uint64_t iv);
22 void Seek(uint64_t pos);
23- void Output(unsigned char* output, size_t bytes);
24+
25+ // if <input> is provided, the ciphertext will be written to <output> otherwise the current keystream
197 rng.Seek(seek);
198 std::vector<unsigned char> out = ParseHex(hexout);
199 std::vector<unsigned char> outres;
200 outres.resize(out.size());
201- rng.Output(outres.data(), outres.size());
202+
assert(hex_message.empty() || m.size() == out.size())
199 std::vector<unsigned char> outres;
200 outres.resize(out.size());
201- rng.Output(outres.data(), outres.size());
202+
203+ // perform the ChaCha20 round(s), if message is provided it will output the encrypted ciphertext otherwise the keystream
204+ rng.Output(hex_message.empty() ? nullptr : m.data(), outres.data(), outres.size());
rng.Output twice if hex_message is nonempty, once with input and once without, and confirm that xoring the two outputs yields the input again.
70@@ -71,7 +71,7 @@ void ChaCha20::Seek(uint64_t pos)
71 input[13] = pos >> 32;
72 }
73
74-void ChaCha20::Output(unsigned char* c, size_t bytes)
75+void ChaCha20::Output(const unsigned char* m, unsigned char* c, size_t bytes)
I think it would be better to duplicate the logic here and turn the stream cipher version into a separate function.
As is, it introduces an unnecessary branch in the output version.
What’s the reasoning here? Performance?
How does the version in the refrence implementation compare?
0void ECRYPT_keystream_bytes(ECRYPT_ctx *x,u8 *stream,u32 bytes)
1{
2 u32 i;
3 for (i = 0;i < bytes;++i) stream[i] = 0;
4 ECRYPT_encrypt_bytes(x,stream,stream,bytes);
5}
ie setting Output(c, bytes) to call Crypt(c, c, bytes)?
256+ x12 += j12;
257+ x13 += j13;
258+ x14 += j14;
259+ x15 += j15;
260+
261+ if (m != nullptr) {
if.
19@@ -20,7 +20,9 @@ class ChaCha20
20 void SetKey(const unsigned char* key, size_t keylen);
21 void SetIV(uint64_t iv);
22 void Seek(uint64_t pos);
23- void Output(unsigned char* output, size_t bytes);
24+
19@@ -20,7 +20,9 @@ class ChaCha20
20 void SetKey(const unsigned char* key, size_t keylen);
21 void SetIV(uint64_t iv);
22 void Seek(uint64_t pos);
23- void Output(unsigned char* output, size_t bytes);
24+
25+ void Output(unsigned char* c, size_t bytes);
26+ void Crypt(const unsigned char* input, unsigned char* output, size_t bytes);
Now that this class can encrypt plaintext, the class comment should be updated from “A PRNG class for ChaCha20.”
Really, I’d like to see more commenting for this class in general (ie note that SetIV sets the nonce, Seek sets the counter, that the counter is 64 bytes and the nonce is 64 bytes as in the original ChaCha20 spec instead of 32 byte counter/96 byte nonce as in the IETF spec).
(there are a couple of good resources on the different counter/nonce sizes here: https://libsodium.gitbook.io/doc/advanced/stream_ciphers/chacha20 and here: http://loup-vaillant.fr/tutorials/chacha20-design)
The IETF has a slightly modified recommendation AFAIK. We use Bernstein-Vanilly 64bit nonce and 64bit block counter.
19@@ -20,7 +20,9 @@ class ChaCha20
20 void SetKey(const unsigned char* key, size_t keylen);
21 void SetIV(uint64_t iv);
22 void Seek(uint64_t pos);
23- void Output(unsigned char* output, size_t bytes);
24+
25+ void Output(unsigned char* c, size_t bytes);
Keystream now that the class is not just used for PRNG
Keystream
206+ rng.Crypt(m.data(), outres.data(), outres.size());
207+ } else {
208+ rng.Output(outres.data(), outres.size());
209+ }
210 BOOST_CHECK(out == outres);
211+ if (!hex_message.empty()) {
re: #15512 (review)
Not sure exactly what you want to get rid of, but if you’re talking about the out == outres check below, I suggested it and think it’d be better to keep, because it means if the test passes you can be reassured that Crypt() and Output() results are consistent without manually verifying 4c616 ^ 6e2e3 == 224f5… in the test cases.
Implementation and tests look good and match the reference implementation here: https://cr.yp.to/streamciphers/timings/estreambench/submissions/salsa20/chacha8/merged/chacha.c and test vector here: https://tools.ietf.org/html/rfc7539#section-2.4.2
I have a few minor comments inline, and would be curious about the answer to: #15512 (review)
20+ ctx.SetIV(0);
21+ ctx.Seek(0);
22+ std::vector<uint8_t> in(buffersize,0);
23+ std::vector<uint8_t> out(buffersize,0);
24+ while (state.KeepRunning())
25+ ctx.Crypt(in.data(), out.data(), in.size());
HASH() below.
23+ std::vector<uint8_t> out(buffersize,0);
24+ while (state.KeepRunning())
25+ ctx.Crypt(in.data(), out.data(), in.size());
26+}
27+
28+static void HASH(benchmark::State& state, size_t buffersize)
CHash256 benchmarks to chacha20.cpp?
Looks great. Thanks for adding the comments to chacha20.h!
Just a couple of nits in the bench file. I’m still curious about the code duplication between Keystream() and Crypt().
Looks good. utACK 2dfe2751713c814aea53b5a7563eb74ad1baea00.
In general, it’s better to have less code duplication, so I’d like to hear from @sipa his reasoning for #15512 (review)
ok, my aesthetic preference is for less duplication, either by having a single Keystream/Crypt function, or by calling Keystream() from Crypt() with a zero’ed message. As sipa points out though, this code is unlikely to be modified much in future, so it’s not a big deal either way.
utACK 2dfe2751713c814aea53b5a7563eb74ad1baea00
utACK 2dfe2751713c814aea53b5a7563eb74ad1baea00. Changes since last review are just renaming the Crypt method, adding comments, and simplifying the benchmark.
I just thought that it’d be preferable not to burden the RNG code with branches that are only used for encryption.
I think the duplication is fine, but it’s unclear if the concern with branches is about readability or performance. If it’s performance, you could give the previous Output() function an additional template<bool use_input> template argument, and have Crypt() and Keystream() both call it inlined.
295+ WriteLE32(c + 56, x14);
296+ WriteLE32(c + 60, x15);
297+
298+ if (bytes <= 64) {
299+ if (bytes < 64) {
300+ for (i = 0;i < bytes;++i) ctarget[i] = c[i];
bytes < 64 line 215 get’s executed which makes ctarget and c equal pointers, therefore it’s copying a value to itself.
tmp (because we do 64bytes always, see line 216, c points to tmp if <64 bytes). So in the case of <64 bytes, c points to the temporary 64byte buffer and ctarget points to the function provided c argument.
17@@ -17,10 +18,17 @@ class ChaCha20
18 public:
19 ChaCha20();
20 ChaCha20(const unsigned char* key, size_t keylen);
21- void SetKey(const unsigned char* key, size_t keylen);
22- void SetIV(uint64_t iv);
23- void Seek(uint64_t pos);
24- void Output(unsigned char* output, size_t bytes);
25+ void SetKey(const unsigned char* key, size_t keylen); //!< set key with flexible keylength; 256bit recommended */