Add chacha20/poly1305 and chacha20poly1305_AEAD from openssh #14050

jonasschnelli · 2018-08-24T15:11:35Z

This adds ChaCha20 and Poly1305 and the openSSH form of the AEAD.

The implementations of ChaCha20 are from DJB himself and directly taken (unchanged as much as possible) from the openSSH source code: https://github.com/openssh/openssh-portable/blob/master/chacha.c

Poly1305 is written by Andrew Moon is also directly taken (unchanged as much as possible) from the openSSH source code: https://github.com/openssh/openssh-portable/blob/master/poly1305.c

The code for the ChaCha20/Poly1305@openssh AEAD is taken from https://github.com/openssh/openssh-portable/blob/master/cipher-chachapoly.c with minimal changes.

More details on the AEAD construction:
https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.chacha20poly1305
https://gist.github.com/jonasschnelli/c530ea8421b8d0e80c51486325587c52

Test vectors are taken from the RFC draft:
https://tools.ietf.org/html/draft-agl-tls-chacha20poly1305-04#section-7

Taking existing implementations should reduce risks of screwing up at the implementation level.

OUT OF SCOPE:

AVX,SSE acceleration (possible, see libsodium a.s.o.)
To Bitcoin Core adjusted C++ implementations
Combining with the existing ChaCha20 RNG
(can all be done later, after verification of the correctness)

This is a subset of #14032 and a pre-requirement for BIP151.

DrahtBot · 2018-08-24T15:49:32Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15649 (Add ChaCha20Poly1305@Bitcoin AEAD by jonasschnelli)
#15519 (Add Poly1305 implementation by jonasschnelli)
#15512 (Add ChaCha20 encryption option (XOR) by jonasschnelli)
#14047 (Add HKDF_HMAC256_L32 and method to negate a private key by jonasschnelli)
#14032 (Add p2p layer encryption with ECDH/ChaCha20Poly1305 by jonasschnelli)

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.

DesWurstes · 2018-08-25T07:09:02Z

Why not reuse the existing ChaCha20?

jonasschnelli · 2018-08-27T11:59:23Z

@DesWurstes: see PR description (out of scope).

jonasschnelli · 2018-08-27T19:37:51Z

Added benchmark for the AEAD (and for direct comparison also added the dbl-SHA (HASH) benchmark).
Benchmarking 1MB data as well as 256bytes.

My system reports (!we compare SSE4 SHA256 vs non-optimized chacha20):

# Benchmark, evals, iterations, total, min, max, median
CHACHA20POLY1305AEAD_BIG, 5, 340, 3.68279, 0.00215035, 0.00219169, 0.00216025
CHACHA20POLY1305AEAD_SMALL, 5, 250000, 1.08673, 8.51516e-07, 8.93585e-07, 8.61119e-07
HASH256_BIG, 5, 340, 3.81384, 0.00222589, 0.00226436, 0.00224086
HASH256_SMALL, 5, 250000, 1.1305, 8.96669e-07, 9.15482e-07, 9.03866e-07

ken2812221 · 2018-08-29T17:17:56Z

src/crypto/chachapoly_aead.cpp

+       might try to eliminate "superfluous" memsets. If there's an easy way to
+       detect memset_s, it would be better to use that. */
+#if defined(_MSC_VER)
+    SecureZeroMemory(ptr, len);


Seems this should include windows.h

practicalswift · 2018-09-01T21:05:50Z

src/crypto/chachapoly_aead.h

+                           uint8_t *dest, const uint8_t *src, uint32_t len,
+                           uint32_t aadlen, int is_encrypt);
+
+// extracts the LE 24bit (3byte) length from the AAD and puts it into a uint32_t (host endiannes)


Typo found by codespell: endiannes

Sjors

Re out of scope "Combining with the existing ChaCha20 RNG": perhaps you can add some tests to show they're identical?

Sjors · 2018-09-02T10:33:08Z

src/test/crypto_tests.cpp

+    */
+
+    static const struct chacha20_testvector chacha20_testvectors[] = {
+        {{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,


This would be much easier to verify against the RFC document if you parse a hex string.

Sjors · 2018-09-02T10:44:38Z

src/crypto/poly1305.cpp

+    (p)[3] = (uint8_t)((v) >> 24);                                             \
+  } while (0)
+
+void poly1305_auth(unsigned char out[POLY1305_TAGLEN], const unsigned char *m,


Why did you remove the poly1305_init, poly1305_update, poly1305_finish separation?

Which separation? I took Andrew Moons implementation found at https://github.com/openssh/openssh-portable/blob/90e51d672711c19a36573be1785caf35019ae7a8/poly1305.c

Ah, I see, in that case the source code URL at the top of the file is confusing; probably should add a link to Moons' implementation instead / in addition.

Sjors · 2018-09-02T10:48:24Z

src/crypto/chacha.cpp

+  x->input[3] = U8TO32_LITTLE(constants + 12);
+}
+
+void chacha_ivsetup(chacha_ctx *x, const u8 *iv, const u8 *counter) {


I can confirm this function in 09ec49ba31867187cecd631593d37bcc5f3ca72f is identical to the original at de624c626ea081929df000b09932dbc804eda51d, modulo some whitespace.

jonasschnelli · 2018-09-02T10:59:33Z

@Sjors The existing ChaCha20 implementation doesn't do the XORing and can therefore only be used for the RNG.

sipa · 2018-09-02T11:34:21Z

Instead of those U8TOU32_LE macros you can use our own function (ReadLE32, WriteLE32).

…

jonasschnelli · 2018-09-02T11:44:09Z

@sipa: agree. Though as written in the PR description. I'd like to keep the code (extracted from openSSH) as identical as possible. I think (or hope) we can do refactoring after this has been merged.

practicalswift · 2018-09-12T09:49:35Z

src/crypto/chachapoly_aead.cpp

+  chacha_ivsetup(&ctx->header_ctx, (uint8_t *)&seqnr64, NULL);
+  chacha_encrypt_bytes(&ctx->header_ctx, ciphertext, buf, 3);
+  *len24_out = 0;
+  *len24_out = buf[0] | (buf[1] << 8) | (buf[2] << 16);


This seems like a mistake? *len24_out is assigned twice? *len24_out = 0; should be removed? :-)

I'm not sure if applying 3 time a uint_8t (24bits) the way its done here will guarantee to set the other 8 bits to 0. This is why I initialise with 0, then set bit 0 to 23. Could be that I'm wrong though.

We wouldn't initialise before assignment if the RHS was an integer, right? :-)

buf[0] | (buf[1] << 8) | (buf[2] << 16) evaluates to an integer.

But don't trust me – I'm just a fellow human.

Let's verify using our favourite C++ interpreter cling:

$ cling ****************** CLING ****************** * Type C++ code and press enter to run it * * Type .q to exit * ******************************************* [cling]$ typeid(1).name() (const char *) "i" [cling]$ #include <cstdint> [cling]$ uint8_t buf[3] = {1, 2, 3} (uint8_t [3]) { '0x01', '0x02', '0x03' } [cling]$ typeid(buf[0] | (buf[1] << 8) | (buf[2] << 16)).name() (const char *) "i" [cling]$ buf[0] | (buf[1] << 8) | (buf[2] << 16) (int) 197121

:-)

src/crypto/poly1305.cpp

practicalswift · 2018-09-12T15:08:09Z

src/test/crypto_tests.cpp

+    assert(out_len == 255);
+    chacha20poly1305_crypt(&aead_ctx, seqnr, plaintext_buf_new, ciphertext_buf,
+        252, 3, 0);
+    assert(memcmp(plaintext_buf, plaintext_buf_new, 252) == 0);


Shouldn't this be assert(memcmp(plaintext_buf, plaintext_buf_new, 255) == 0); or assert(memcmp(plaintext_buf, plaintext_buf_new, sizeof(plaintext_buf)) == 0);?

src/crypto/poly1305.cpp

practicalswift · 2018-09-30T15:53:04Z

src/crypto/chachapoly_aead.cpp

+                           uint32_t aadlen, int is_encrypt) {
+  const uint8_t one[8] = {1, 0, 0, 0, 0, 0, 0, 0}; /* NB little-endian */
+  uint8_t expected_tag[POLY1305_TAGLEN], poly_key[POLY1305_KEYLEN];
+  int r = -1;


This is a dead store :-)

Initialize to zero instead and remove the r = 0 below?

src/crypto/chachapoly_aead.cpp

jonasschnelli · 2018-10-01T13:18:19Z

Fixed points reported by @practicalswift

practicalswift · 2018-12-07T08:37:11Z

I'm afraid this PR doesn't compile when rebased on master

jonasschnelli · 2019-03-01T16:14:20Z

Partially superseded by #15512 (the ChaCha20 part)

jonasschnelli · 2019-03-24T19:43:49Z

Closing...
It's now replaced with #15512, #15519 and #15649.

jonasschnelli mentioned this pull request Aug 24, 2018

Add p2p layer encryption with ECDH/ChaCha20Poly1305 #14032

Closed

1 task

DrahtBot mentioned this pull request Aug 24, 2018

Add HKDF_HMAC256_L32 and method to negate a private key #14047

Merged

laanwj added the P2P label Aug 25, 2018

jonasschnelli force-pushed the 2018/08/bip151_chachapoly1305 branch 5 times, most recently from fcfa804 to 90702f4 Compare August 28, 2018 19:14

jonasschnelli added a commit to jonasschnelli/bitcoin that referenced this pull request Aug 29, 2018

--- NEW CRYPTO (bitcoin#14047, bitcoin#14049, bitcoin#14050) ---

955ac1d

ken2812221 reviewed Aug 29, 2018

View reviewed changes

jonasschnelli force-pushed the 2018/08/bip151_chachapoly1305 branch 4 times, most recently from f42f6d5 to ac477bc Compare August 31, 2018 19:49

practicalswift reviewed Sep 1, 2018

View reviewed changes

jonasschnelli force-pushed the 2018/08/bip151_chachapoly1305 branch from ac477bc to d415a02 Compare September 2, 2018 08:02

Sjors reviewed Sep 2, 2018

View reviewed changes

practicalswift reviewed Sep 12, 2018

View reviewed changes

src/crypto/poly1305.cpp Show resolved Hide resolved

practicalswift reviewed Sep 12, 2018

View reviewed changes

practicalswift reviewed Sep 30, 2018

View reviewed changes

src/crypto/poly1305.cpp Show resolved Hide resolved

practicalswift reviewed Sep 30, 2018

View reviewed changes

src/crypto/chachapoly_aead.cpp Show resolved Hide resolved

practicalswift reviewed Sep 30, 2018

View reviewed changes

src/crypto/chachapoly_aead.cpp Show resolved Hide resolved

practicalswift reviewed Sep 30, 2018

View reviewed changes

src/crypto/chachapoly_aead.cpp Show resolved Hide resolved

jonasschnelli added 3 commits October 1, 2018 14:58

Add chacha20/poly1305 and chacha20poly1305_AEAD from openssh

124a14f

QA: add ChaCha20/Poly1305 test vectors from the RFC draft

ae68baa

Add benchmark for ChaCha20Poly1305-openssh AEAD and dbl-SHA256

e65a418

jonasschnelli force-pushed the 2018/08/bip151_chachapoly1305 branch from d415a02 to e65a418 Compare October 1, 2018 13:17

jonasschnelli closed this Mar 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add chacha20/poly1305 and chacha20poly1305_AEAD from openssh #14050

Add chacha20/poly1305 and chacha20poly1305_AEAD from openssh #14050

jonasschnelli commented Aug 24, 2018

DrahtBot commented Aug 24, 2018 •

edited

Loading

DesWurstes commented Aug 25, 2018

jonasschnelli commented Aug 27, 2018

jonasschnelli commented Aug 27, 2018

ken2812221 Aug 29, 2018

practicalswift Sep 1, 2018

jonasschnelli Sep 2, 2018

Sjors left a comment

Sjors Sep 2, 2018

Sjors Sep 2, 2018

jonasschnelli Sep 2, 2018

Sjors Sep 2, 2018 •

edited

Loading

Sjors Sep 2, 2018 •

edited

Loading

jonasschnelli commented Sep 2, 2018

sipa commented Sep 2, 2018 via email

jonasschnelli commented Sep 2, 2018

practicalswift Sep 12, 2018

jonasschnelli Oct 1, 2018

practicalswift Oct 1, 2018 •

edited

Loading

practicalswift Sep 12, 2018 •

edited

Loading

practicalswift Sep 30, 2018 •

edited

Loading

jonasschnelli commented Oct 1, 2018

practicalswift commented Dec 7, 2018

jonasschnelli commented Mar 1, 2019

jonasschnelli commented Mar 24, 2019

Add chacha20/poly1305 and chacha20poly1305_AEAD from openssh #14050

Add chacha20/poly1305 and chacha20poly1305_AEAD from openssh #14050

Conversation

jonasschnelli commented Aug 24, 2018

DrahtBot commented Aug 24, 2018 • edited Loading

Conflicts

DesWurstes commented Aug 25, 2018

jonasschnelli commented Aug 27, 2018

jonasschnelli commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

Sjors Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

jonasschnelli commented Sep 2, 2018

sipa commented Sep 2, 2018 via email

jonasschnelli commented Sep 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

practicalswift Oct 1, 2018 • edited Loading

Choose a reason for hiding this comment

practicalswift Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

practicalswift Sep 30, 2018 • edited Loading

Choose a reason for hiding this comment

jonasschnelli commented Oct 1, 2018

practicalswift commented Dec 7, 2018

jonasschnelli commented Mar 1, 2019

jonasschnelli commented Mar 24, 2019

DrahtBot commented Aug 24, 2018 •

edited

Loading

Sjors Sep 2, 2018 •

edited

Loading

Sjors Sep 2, 2018 •

edited

Loading

practicalswift Oct 1, 2018 •

edited

Loading

practicalswift Sep 12, 2018 •

edited

Loading

practicalswift Sep 30, 2018 •

edited

Loading