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

["Jérémy Jean" <jeremy.jean@oss.cyber.gouv.fr>]

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@vger.kernel.org
>> > Assisted-by: LLM
>> > Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
>> > ---
>> >  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