Re: [PATCH] xfrm: esp6: fix off-by-one IV counter causing AES-GCM nonce reuse

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

Hello Sabrina,

On 2026-09-28 18:19, Sabrina Dubroca wrote:
> The subject prefix should be "PATCH ipsec" for IPsec bugfixes.

I will try to think about that next time.

> 2026-09-25, 09:51:06 +0000, Jérémy Jean wrote:
>> An off-by-one error in esp6_xmit() advances the IV counter before
>> encrypting each software-GSO segment. For N segments with sequence
>> numbers X through X+N-1, the IV counters are therefore X+1 through 
>> X+N.
>> The following non-GSO packet uses X+N for both its sequence number and
>> IV counter, repeating the last segment's AES-GCM nonce under the same
>> key.
> 
> I find this description very unclear. All I'm managing to understand
> from this is "there's some situation where a packet isn't getting the
> seqno it should". I don't know where the "+1" comes from since for GSO
> the function does +N (xo->seq.low += skb_shinfo(skb)->gso_segs).

I tried to be as explicit as possible, but I apologize if it was not
good enough. My understanding on the full GSO processing isn't as
deep as yours, so here is another try at explaining.

The bug happens after software segmentation in GSO. When a large
amount of data needs to span across several packets, software
segmentation splits it into N smaller skb. After this split, each
smaller skb holding the individual packets has skb_is_gso(skb)
returning false, yet each skb keeps the flag XFRM_GSO_SEGMENT stating
that this skb resulted from a segmentation. Consequently, the skb
goes through the increment below in esp6_xmit():

net/ipv6/esp6_offload.c:
    355		if (xo->flags & XFRM_GSO_SEGMENT) {
    356			esp.esph->seq_no = htonl(seq);
    357
    358			if (!skb_is_gso(skb))
    359				xo->seq.low++;      // <<< increment here
    360			else
    361				xo->seq.low += skb_shinfo(skb)->gso_segs;
    362		}

There are N calls to esp6_xmit() for all the smaller packets, and for
each of them, the current sequence number is first written into the
header, and then the shared counter for the next packet is
incremented. However, the value esp.seqno used to construct the IV is
derived from the counter value _after_ the increment. For example, if
the last packet produced by segmentation has sequence number 100, the
IV is constructed using counter value 101. Then, a subsequent
ordinary packet not going through segmentation is allocated sequence
number 101, yet since it does not have the flag XFRM_GSO_SEGMENT,
there is no increment, and its value is constructed from value 101 as
well. Hence the nonce repetition.

The fix proposes to move the computation of esp.seqno _before_ the
increment.

> Anyway, one process nit and one question on the code:
> 
>> The repeated nonce allows first a passive attacker who knows partial
>> plaintext from one packet to recover corresponding bytes from another
>> one, and second, an active attacker to recover GCM authentication key
>> to forge authentication tags without recovering the AES key.
>> 
>> Fix this by saving the complete current sequence number in esp.seqno
>> before advancing the shared GSO sequence state.
>> 
>> Fixes: 3dca3f38cfb8 ("xfrm: Separate ESP handling from segmentation 
>> for GRO packets.")
> 
> And if there's a crypto leak, this should probably have a "Cc: stable"
> tag.

Noted, thanks.

>> Assisted-by: LLM
>> Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
>> ---
>>  net/ipv6/esp6_offload.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>> 
>> diff --git a/net/ipv6/esp6_offload.c b/net/ipv6/esp6_offload.c
>> index 2289552..05d13cc 100644
>> --- a/net/ipv6/esp6_offload.c
>> +++ b/net/ipv6/esp6_offload.c
>> @@ -346,6 +346,7 @@ static int esp6_xmit(struct xfrm_state *x, struct 
>> sk_buff *skb,  netdev_features
>>  	}
>> 
>>  	seq = xo->seq.low;
>> +	esp.seqno = cpu_to_be64(seq + ((u64)xo->seq.hi << 32));
>> 
>>  	esp.esph = ip_esp_hdr(skb);
>>  	esp.esph->spi = x->id.spi;
>> @@ -364,8 +365,6 @@ static int esp6_xmit(struct xfrm_state *x, struct 
>> sk_buff *skb,  netdev_features
>>  	if (xo->seq.low < seq)
>>  		xo->seq.hi++;
>> 
>> -	esp.seqno = cpu_to_be64(xo->seq.low + ((u64)xo->seq.hi << 32));
> 
> But then esp.seqno can have an inconsistent view of xo->seq.hi
> compared to what esp6_output_tail/esp_output_set_esn will see
> (xo->seq.hi++ just above this)?

This looks like another bug, similar to the boundary case I described 
here for ipv4?
https://lore.kernel.org/all/20260925095128.446450-2-Jeremy.Jean@oss.cyber.gouv.fr/

Regards,
Jérémy