cloudflare / sslconfig

Cloudflare's Internet facing SSL configuration
BSD 3-Clause "New" or "Revised" License
1.3k stars 132 forks source link

chacha20poly1305 seq_num included into AAD and incorrect MAC for RFC7539 test vectors #60

Closed mmolchan closed 7 years ago

mmolchan commented 7 years ago

Hello,

Test vectors with Additional Authenticated Data seems to be broken, for example this one from RFC7539:

  000  4c 61 64 69 65 73 20 61 6e 64 20 47 65 6e 74 6c  Ladies and Gentl
  016  65 6d 65 6e 20 6f 66 20 74 68 65 20 63 6c 61 73  emen of the clas
  032  73 20 6f 66 20 27 39 39 3a 20 49 66 20 49 20 63  s of '99: If I c
  048  6f 75 6c 64 20 6f 66 66 65 72 20 79 6f 75 20 6f  ould offer you o
  064  6e 6c 79 20 6f 6e 65 20 74 69 70 20 66 6f 72 20  nly one tip for
  080  74 68 65 20 66 75 74 75 72 65 2c 20 73 75 6e 73  the future, suns
  096  63 72 65 65 6e 20 77 6f 75 6c 64 20 62 65 20 69  creen would be i
  112  74 2e                                            t.

   AAD:
   000  50 51 52 53 c0 c1 c2 c3 c4 c5 c6 c7              PQRS........
   Tag:
   1a:e1:0b:59:4f:09:e2:6a:7e:90:2e:cb:d0:60:06:91

With seq_num set to eight zero bytes (to avoid its impact on test vector IV when XORed), patched OpenSSL produces incorrect auth tag:

de 08 a0 26 69 99 53 f2 67 70 82 b9 1f fc cb ed 

I checked the sources and see that ChaPoly EVP API expects AAD in the format of '8 bytes seq_num || actual AAD' when using "EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_TLS1_AAD, aad_len, aad)" to set both seq_num and AAD. aad_len should be 8 + actual_aad_len. XORing padded seq_num with IV works as expected, but later, when AAD and its length is included into Poly1305 MAC calculation, AAD is passed as 'seq_num || AAD', which breaks test vectors. Simple patch that excludes seq_num from AAD tag calculation fixes the test vectors, see last two lines below:

            if (!aead_ctx->draft) {
                /* RFC IV = (0 || iv) ^ seq_num */
                memset(aead_ctx->nonce + 32, 0, 4);
                memcpy(aead_ctx->nonce + 36, aead_ctx->iv, 12);
                *(uint64_t *)(aead_ctx->nonce + 40) ^= *(uint64_t *)(ptr);

            } else {
                /* draft IV = 0 || seq_num */
                memset(aead_ctx->nonce + 32, 0, 8);
                memcpy(aead_ctx->nonce + 40, ptr, 8);
            }

            /* Exclude seq_num from poly1305 AAD auth tag */
            ptr += 8;
            arg -= 8;

The patched version produces correct tag:

1a e1 0b 59 4f 09  e2 6a 7e 90 2e cb d0 60 06 91

So my question is - is it an RFC feature to include seq_num into the auth tag calculation? If so, are there any test vectors for TLS implementation of ChaPoly?

mmolchan commented 7 years ago

Actually I found out a requirement of TLS1.2 to include the following into AAD: " additional_data = seq_num + TLSCompressed.type + TLSCompressed.version + TLSCompressed.length;" So, closing this issue. Since seq_num requirement is so strict in this API, there is no way to verify chacha using unmodified test vectors.