BIP151: Clarifications on sequence numbers. #426

This clarifies that the ciphertext payload length is meant to update the AEAD as AAD. OpenSSH does this, but in a naive way, as seen here (aadlen is 4 -- poly1305_auth is called on both size and payload).

Although the payload length is AAD, OpenSSH violates both RFCs by not including the AAD length (or the ciphertext length) in the MAC before finalization. This change to the bip includes the size explicitly as AAD.

Update: Nevermind the above change. I confused myself by reading the openssh code too much (AAD not necessary due to bip151's encrypted size).

This change also explicitly mentions sequence numbers are meant to be uint32's which are allowed to overflow. This is in keeping with openssh behavior. I don't think having a 64bit sequence provides any benefit over a 32bit one for bip151's use case: we're guaranteed to have rekeyed by the time the seq number reaches uint32_max, so there's no danger in reusing a previous IV/sequence earlier.

Along with the sequence size, this PR shrinks IV size to 64 bits a la openssh (we weren't using all 96 bits even with 64bit sequences). This allows for a 64 bit chacha counter.

cc @jonasschnelli

Thanks @chjj!

1. AAD

Update: Nevermind the above change. I confused myself by reading the openssh code too much (AAD not necessary due to bip151's encrypted size).

As you updated your PR, I think the way how we build the AEAD is correct.

2. I just checked the ChaCha20Poly1305@openssh.com specs and there it says

<pre> The AEAD is constructed as follows: for each packet, generate a Poly1305 key by taking the first 256 bits of ChaCha20 stream output generated using K_2, an IV consisting of the packet sequence number encoded as an <strong>uint64</strong> under the SSH wire encoding rules and a ChaCha20 block counter of zero. </pre>

And yes, openssh is using a uint32_t as internal sequence number counter (which should be sufficient as you stated).

ACK after squash.

As you updated your PR, I think the way how we build the AEAD is correct.

Yeah, I was confused for a second. I didn't notice openssh had an option for unencrypted payload sizes at first.

I just checked the ChaCha20Poly1305@openssh.com specs and there it says

Hmm, I think they're referring to the IV there (i.e. encoding/casting a uint32 seq number to a uint64 for the IV).

Anyway, glad we got this figured out before more implementations roll out.