[noise] More Implementation Pitfalls: ReadMessage() Error Conditions

[Jason A. Donenfeld]

Hi folks,

WireGuard is a UDP-based protocol, that uses Noise as a component, so
from day one it's been designed with some slightly different
considerations than what has been considered by the three (Rust,
Haskell, Go) implementations that have emerged recently. Namely, there
are no connections, and that means I need to deal with receiving bogus
data on the incoming UDP port.

What I mean specifically is that ReadMessage() needs to either return
success, and have modified the internal state variables per the spec.
Or, importantly, it needs to return error, and not change any internal
state at all. At present, all Noise implementations (except for
WireGuard) will result in a session with an inconsistent state if
ReadMessage() is called on invalid data. It's a trivial denial of
service vector, if used in a UDP-based protocol. Or, if an application
uses one of the existing libraries in a particular way, and
ReadMessage() is repeatedly triggerable after it aborts early, the
internal state could be manipulated in an intentional way.

The solution to this used by WireGuard is simple: I take a mutex on
the state, copy the state to temporary storage on the stack, do all my
various manipulations on that temporary state, and if all the steps
and finally the payload of the handshake are successful, then I copy
the temporary storage back to the original, and release the mutex. And
of course, before returning from the function, I'm very careful to
zero out any private data copied to the stack in a way that the
compiler won't optimize out.

I'd suggest the other implementations also aim for an idempotent
ReadMessage() implementation too. At the least, it prevents people
from shooting themselves in the foot. At the most, it lets these
implementations be used in UDP-based protocols like WireGuard.

Jason