[noise] verifying received static remote key

[Trevor Perrin]

On Wed, Nov 1, 2017 at 9:19 PM, David Wong <davidwong.crypto at gmail.com> wrote:
>>> 1) IF the remote static key is known because it was processed during a
>>> pre-message pattern, it needs to be checked against the received one.
>>> If they are different, the handshake needs to be aborted.
>>
>> That's an illegal pattern (section 7.1, pattern validity rule #2 - a
>> key can only be sent once, including pre-messages).
>>
>
> Ah! I read this sentence in the spec several times and didn't really
> grasp its meaning. In this case, it might be good to add in the
> Write/ReadMessage() definition:
>
> WriteMessage() → For "s": Appends EncryptAndHash(s.public_key) to the
> buffer, only if it hasn't been previously sent in a pre-message.
> ReadMessage() → For "s": Ignore this token if the remote static key
> has already been set in a pre-message.

I don't think the spec pseudocode needs to handle invalid patterns.

(A library that handles patterns explicitly might validate them as
part of "defensive programming".  But I'd expect such a library to
abort on discovering an invalid pattern, not ignore things to make the
pattern work.)

If the pattern validity rules are unclear we should improve that -
suggestions welcome.


>> We added a Security consideration in revision 33 to discuss this
>> ("Authentication").
>
> I strongly believe that this is not enough! This is one of the most,
> if not the most, important security foundation of Noise. Without this
> verification the whole security crumbles. Following, I propose the
> addition of a `remoteKeyValid` boolean inside of a handshakeState.
[...]
> If so, this undefined verifyRemoteStaticKey() boolean function could
> be mentioned after the DecryptAndHash step of a ReadMessage():
>
> * Calls DecryptAndHash() on the remaining bytes of the message and
> stores the output into payload_buffer.
> * if a `rs` has been received (not null anymore) and the
> `remoteKeyValid` boolean is false, call
> verifyRemoteStaticKey(payload_buffer, rs)
>
> You would also need to define a `remoteKeyValid` boolean in a
> handshakeState along with s, e, rs and re.
>
> When calling split, it would be also good to mention that it should
> return an error if remoteKeyValid is set to false.
>
> What do you think of this?

I'm not super-eager to make the pseudocode more complicated,
particularly when this is just adding more emphasis ("authentication
is important!") for something the spec already explains.

Also, there are ways of doing authentication that don't fit into a
blocking verifyRemoteStaticKey() function.  For example, maybe the
authentication check requires a network lookup ("is this key good, or
revoked?") so will happen in parallel with the rest of the handshake.

Alternatively, if we want to emphasize this further we could list this
in the "Application responsibilities" as well, or find some other
place to insert a sentence.


Trevor