Re: [PATCH net] wireguard: noise: reject responses for replaced initiations

[Jérémy Jean]

On 2026-10-06 09:53, Jason A. Donenfeld wrote:
> On Mon, Oct 05, 2026 at 11:18:33PM +0200, Jason A. Donenfeld wrote:
> > Hi,
> >
> > On Mon, Oct 05, 2026 at 08:35:55PM +0000, Jérémy Jean wrote:
> > > WireGuard can accept an old handshake response after starting a new
> > > handshake. This reinstalls old keys and resets transport counters and
> > > replay state, enabling nonce reuse, replay and packet forgery. This
> > > breaks confidentiality and integrity guarantees.
> > >
> > > Compare ephemeral secrets under the write lock to reject responses for
> > > replaced initiations.
> > >
> > > Fixes: e7096c131e51 ("net: WireGuard secure network tunnel")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Assisted-by: LLM
> > > Signed-off-by: Jérémy Jean <Jeremy.Jean@xxxxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/net/wireguard/noise.c | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/wireguard/noise.c b/drivers/net/wireguard/noise.c
> > > index 9c0a09bf6c95..88cc9acc7dc7 100644
> > > --- a/drivers/net/wireguard/noise.c
> > > +++ b/drivers/net/wireguard/noise.c
> > > @@ -784,10 +784,12 @@ wg_noise_handshake_consume_response(struct message_handshake_response *src,
> > >
> > >  	/* Success! Copy everything to peer */
> > >  	down_write(&handshake->lock);
> > > -	/* It's important to check that the state is still the same, while we
> > > -	 * have an exclusive lock.
> > > +	/* Check that this is still the initiation we authenticated against,
> > > +	 * while we have an exclusive lock.
> > >  	 */
> > > -	if (handshake->state != state) {
> > > +	if (handshake->state != state ||
> > > +	    crypto_memneq(handshake->ephemeral_private, ephemeral_private,
> > > +			  NOISE_PUBLIC_KEY_LEN)) {
> > >  		up_write(&handshake->lock);
> > >  		goto fail;
> > >  	}
> >
> > Could you describe the flow that you think causes a bug? Trying
> > to recreate your mental model. Something like this?
> >
> > == thread 1 ==
> >
> > 	down_read(&handshake->lock);
> > 	state = handshake->state;
> > 	memcpy(hash, handshake->hash, NOISE_HASH_LEN);
> > 	memcpy(chaining_key, handshake->chaining_key, NOISE_HASH_LEN);
> > 	memcpy(ephemeral_private, handshake->ephemeral_private,
> > 	       NOISE_PUBLIC_KEY_LEN);
> > 	memcpy(preshared_key, handshake->preshared_key,
> > 	       NOISE_SYMMETRIC_KEY_LEN);
> > 	up_read(&handshake->lock);
> >
> > 	if (state != HANDSHAKE_CREATED_INITIATION)
> > 		goto fail;
> >
> > 	/* e */
> > 	message_ephemeral(e, src->unencrypted_ephemeral, chaining_key, hash);
> >
> > 	/* ee */
> > 	if (!mix_dh(chaining_key, NULL, ephemeral_private, e))
> > 		goto fail;
> >
> > 	/* se */
> > 	if (!mix_dh(chaining_key, NULL, wg->static_identity.static_private, 
> > e))
> > 		goto fail;
> >
> > 	/* psk */
> > 	mix_psk(chaining_key, hash, key, preshared_key);
> >
> > 	/* {} */
> > 	if (!message_decrypt(NULL, src->encrypted_nothing,
> > 			     sizeof(src->encrypted_nothing), key, hash))
> > 		goto fail;
> >
> > == thread 2 ==
> >
> > 	down_read(&handshake->static_identity->lock);
> > 	down_write(&handshake->lock);
> >
> > 	if (unlikely(!handshake->static_identity->has_identity))
> > 		goto out;
> >
> > 	dst->header.type = cpu_to_le32(MESSAGE_HANDSHAKE_INITIATION);
> >
> > 	handshake_init(handshake->chaining_key, handshake->hash,
> > 		       handshake->remote_static);
> >
> > 	/* e */
> > 	curve25519_generate_secret(handshake->ephemeral_private);
> > 	if (!curve25519_generate_public(dst->unencrypted_ephemeral,
> > 					handshake->ephemeral_private))
> > 		goto out;
> > 	message_ephemeral(dst->unencrypted_ephemeral,
> > 			  dst->unencrypted_ephemeral, handshake->chaining_key,
> > 			  handshake->hash);
> >
> > 	/* es */
> > 	if (!mix_dh(handshake->chaining_key, key, 
> > handshake->ephemeral_private,
> > 		    handshake->remote_static))
> > 		goto out;
> >
> > 	/* s */
> > 	message_encrypt(dst->encrypted_static,
> > 			handshake->static_identity->static_public,
> > 			NOISE_PUBLIC_KEY_LEN, key, handshake->hash);
> >
> > 	/* ss */
> > 	if (!mix_precomputed_dh(handshake->chaining_key, key,
> > 				handshake->precomputed_static_static))
> > 		goto out;
> >
> > 	/* {t} */
> > 	tai64n_now(timestamp);
> > 	message_encrypt(dst->encrypted_timestamp, timestamp,
> > 			NOISE_TIMESTAMP_LEN, key, handshake->hash);
> >
> > 	dst->sender_index = wg_index_hashtable_insert(
> > 		handshake->entry.peer->device->index_hashtable,
> > 		&handshake->entry);
> >
> > 	handshake->state = HANDSHAKE_CREATED_INITIATION;
> > 	ret = true;
> >
> > out:
> > 	up_write(&handshake->lock);
> > 	up_read(&handshake->static_identity->lock);
> >
> > == thread 1 ==
> >
> > 	down_write(&handshake->lock);
> > 	/* It's important to check that the state is still the same, while we
> > 	 * have an exclusive lock.
> > 	 */
> > 	if (handshake->state != state) {
> > 		up_write(&handshake->lock);
> > 		goto fail;
> > 	}
> > 	memcpy(handshake->remote_ephemeral, e, NOISE_PUBLIC_KEY_LEN);
> > 	memcpy(handshake->hash, hash, NOISE_HASH_LEN);
> > 	memcpy(handshake->chaining_key, chaining_key, NOISE_HASH_LEN);
> > 	handshake->remote_index = src->sender_index;
> > 	handshake->state = HANDSHAKE_CONSUMED_RESPONSE;
> > 	up_write(&handshake->lock);
> > 	ret_peer = peer;
> > 	goto out;
> >
> > And now begin_session is called on the older completed handshake 
> > rather
> > than the half-completed newer handshake?
> >
> > Or did you see some other flow?
>
> Okay I think I worked something plausible out:
>
> - two threads begin processing the same response
> - the first succeeds. the second gets halfway, when it blocks on taking
>   a lock that a queued initiation has taken
> - the queued initiation does its thing and resets the state
> - the second thread resumes and completes the old handshake and
>   reinstalls keys
>
> I'll continue analyzing real world feasibility, but in all cases, thank
> you for the patch.
>
> Jason

Hello Jason,

Thanks for your quick reply. Sorry that I could not answer in time.
Your analysis is indeed the scenario that I had in mind, yet, the
second thread does not really have to block, I'd say it can just
be slow?

As for real world feasibility and impacts, I must admit that
I have no idea.

Regards,
Jérémy