[noise] New revision: "noh2" branch, revision 3

[Jason A. Donenfeld]

Hi Trevor,

This is looking much better. I'm compelled by the noh2 branch now. I
especially like moving transport messages back to simple CipherSuite
operations, while the HandshakeState remains this fancy state machine which
does its thing and then gets out of the way. This is more or less a natural
way of organizing an implementation of it too. The HandshakeState object
has-a CipherState object. My previous Noise version had things divided
exactly like this. With the recent changes, the only difference is that I
make `h` a part of the HandshakeState, and not part of the CipherState,
since it's not useful for transport messages anymore, and that way I can
unambiguously zero out sizeof(struct HandshakeState) at the right point in
time. That change might make the spec unnecessarily verbose though, so I'm
not sure it's worthwhile, as it's not "substantive" either.

A few other remarks and nits:

- "All Noise messages are 65535 bytes in length or less."
Why? This limitation seems rather pointless. The RFC's ChaPoly gives us a
32bit internal block counter, so we can safely make messages that are
rather huge. It seems like that is something best left up to the upper
layers to decide, especially since the framing is there too.

- "KDF(kdf_key, input): Takes a kdf_key of 256 bits and some input data and
returns a new value for the cipher key `k`. "
It's not really useful here to name the return value. Could be shortened to
"..and returns a new value for a cipher key."

- "HMAC-SHA2-256 is an example KDF." "SHA2-256 is an example hash function."
Are the examples still relevant, since the cipher suite section below
specifies them too? The latter is missing ` ` by the way.

- "Decrypt(ciphertext): Calls DECRYPT(k, n, h, ciphertext) to get a
plaintext, then increments n and returns the plaintext. If an
authentication failure occurs all variables are set to zeros and the error
is signalled to the caller."
signalled -> signaled
Which variables are set to zero? The local k, n, h, and ciphertext that are
on DECRYPT's stack? The k, n, and h that belongs to the present CipherState
object? If the latter, thats a bad DoS. If the former, I don't know if it
makes sense to include implementation details like that; hopefully
implementors are already zeroing their stacks appropriately.

- "on the next data_len + 16"
16 is hard coded here. That probably makes sense, as earlier you also
explicitly specify having a 128-bit tag in no matter what ciphersuite. But
in case you did want to abstract things with an AUTHLEN, here's a 16.

- "For "dhxy" calls cipherstate.MixKey(DH(x, ry)) and sets has_key to True."
Can you italicize the x and y inside the ` ` too?

- "ReadHandshakeMessage(buffer, descriptor): Takes a byte buffer containing
a message, and a descriptor, and returns a payload. If a decryption error
occurs all variables are set to zeros and the error is signalled to the
caller."
Same issue with spelling typo and zeroing ambiguity.

- "EndHandshake(): Returns two new CipherState objects by calling
cipherstate.Split()."
...and zeros everything out.

- "Big-endian is preferred because..."
Grumble grumble.


By the way, after you merge this to master, is there a reason for keeping
around noh and noise2 branches? Or are these just left over from old times?

Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://moderncrypto.org/mail-archive/noise/attachments/20150901/67cc1ea0/attachment.html>