IPsec replay sequence number overflow behavior? (RFC4303 section 3.3.3)

[Paul Moore]

Hello all,

As part of the IPv6 "gap analysis" that the Linux Foundation is currently
doing I've been looking at the IPsec auditing requirements as defined in
RFC4303 and I came across some odd behavior regarding SA sequence number
overflows ...

RFC4303 states the following:

3.3.3. Sequence Number Generation

The sender's counter is initialized to 0 when an SA is established.
The sender increments the sequence number (or ESN) counter for this
SA and inserts the low-order 32 bits of the value into the Sequence
Number field. Thus, the first packet sent using a given SA will
contain a sequence number of 1.

If anti-replay is enabled (the default), the sender checks to ensure
that the counter has not cycled before inserting the new value in the
Sequence Number field. In other words, the sender MUST NOT send a
packet on an SA if doing so would cause the sequence number to cycle.
An attempt to transmit a packet that would result in sequence number
overflow is an auditable event. The audit log entry for this event
SHOULD include the SPI value, current date/time, Source Address,
Destination Address, and (in IPv6) the cleartext Flow ID.

The related code in net/xfrm/xfrm_output.c:xfrm_output() looks like this:

if (x->type->flags & XFRM_TYPE_REPLAY_PROT) {
XFRM_SKB_CB(skb)->seq = ++x->replay.oseq;
if (xfrm_aevent_is_on())
xfrm_replay_notify(x, XFRM_REPLAY_UPDATE);
}

Which doesn't appear to take into account sequence number overflow at all.
Granted, it does send notifications to userspace but it doesn't do anything
to prevent the packet from being sent if the sequence number wraps. I'm
still a few years behind in my IPsec specifications so I could be missing
something here (extended sequence numbers spring to mind and the kernel's
curious mixing of 32bit and 64bit types for SA sequence number counters) but
at first glance this appears to be a bug ... yes/no?

If it is a bug, I think the basic fix should be pretty simple, changing the
above xfrm_output() code to the following:

if (x->type->flags & XFRM_TYPE_REPLAY_PROT) {
XFRM_SKB_CB(skb)->seq = ++x->replay.oseq;
+ if (x->replay.oseq == 0)
+ goto error;
if (xfrm_aevent_is_on())
xfrm_replay_notify(x, XFRM_REPLAY_UPDATE);
}

[Herbert Xu]

Post by Paul Moore
If it is a bug, I think the basic fix should be pretty simple, changing the
if (x->type->flags & XFRM_TYPE_REPLAY_PROT) {
XFRM_SKB_CB(skb)->seq = ++x->replay.oseq;
+ if (x->replay.oseq == 0)
+ goto error;

Yes we need this check.

However please add an unlikely around it since it's a 1-in-4
billion event :)

Cheers,

[Paul Moore]

Post by Herbert Xu

Post by Paul Moore
If it is a bug, I think the basic fix should be pretty simple, changing
if (x->type->flags & XFRM_TYPE_REPLAY_PROT) {
XFRM_SKB_CB(skb)->seq = ++x->replay.oseq;
+ if (x->replay.oseq == 0)
+ goto error;

Yes we need this check.
However please add an unlikely around it since it's a 1-in-4
billion event :)

:)

Thanks for clearing that up, I'll send a patch this week; complete with an
unlikely (similar to the RFC quality IPsec audit patch I sent on Friday) and
a decrement to the sequence counter in case of rollover.

[Herbert Xu]

Post by Paul Moore
Thanks for clearing that up, I'll send a patch this week; complete with an
unlikely (similar to the RFC quality IPsec audit patch I sent on Friday) and
a decrement to the sequence counter in case of rollover.

Actually I think we should just use the SA expire mechanism
to do this. The reason is that the overflow we want to detect
only applies to 32-bit sequence numbers. In future we will be
making our sequence numbers 64-bit.

When we do that we can no longer just check against wrapping
to zero since we need to know whether ESNs are in use or not.

The easiest fix is to just force the hard_packet_limit to 2^32.
upon SA creation.

Cheers,

[Patrick McHardy]

Post by Herbert Xu

Post by Paul Moore
Thanks for clearing that up, I'll send a patch this week; complete with an
unlikely (similar to the RFC quality IPsec audit patch I sent on Friday) and
a decrement to the sequence counter in case of rollover.

Actually I think we should just use the SA expire mechanism
to do this. The reason is that the overflow we want to detect
only applies to 32-bit sequence numbers. In future we will be
making our sequence numbers 64-bit.
When we do that we can no longer just check against wrapping
to zero since we need to know whether ESNs are in use or not.
The easiest fix is to just force the hard_packet_limit to 2^32.
upon SA creation.

Won't this break with manually installed SAs (without a keying
daemon)?


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Herbert Xu]

Post by Patrick McHardy
Won't this break with manually installed SAs (without a keying
daemon)?

Well what's being suggested here will already break that anyway :)

Alternatively we can take the interpretation that it's the KM's
responsibility to set the appropriate hard life time if ESNs are
not in use.

Either way is fine with me.

Cheers,

[Paul Moore]

Post by Herbert Xu

Post by Patrick McHardy
Won't this break with manually installed SAs (without a keying
daemon)?

Well what's being suggested here will already break that anyway :)
Alternatively we can take the interpretation that it's the KM's
responsibility to set the appropriate hard life time if ESNs are
not in use.
Either way is fine with me.
Cheers,

Sorry for the delay, I got distracted ...

Rereading the thread it's unclear to me which solution was deemed "correct".
I'm not a big fan of fiddling/forcing SA lifetimes unless we have no other
option; if someone is foolish enough to use manual keying with replay
protection and no mechanism to catch rollover then they most likely have
larger problems. It's the whole "we'll provide you with the gun, but you
have to shoot yourself" argument as applied to SA lifetimes.

However, you guys have to deal with this code more often than I do so I'll
deffer to your better judgment.

[Joy Latten]

Post by Paul Moore
Rereading the thread it's unclear to me which solution was deemed "correct".
I'm not a big fan of fiddling/forcing SA lifetimes unless we have no other
option; if someone is foolish enough to use manual keying with replay
protection and no mechanism to catch rollover then they most likely have
larger problems. It's the whole "we'll provide you with the gun, but you
have to shoot yourself" argument as applied to SA lifetimes.

Also, the ipsec rfc require auotmated SA management when
using anti-replay service and that the option be disabled
when SAs are manually setup.

It may not stop anyone, but we can always point to rfc. :-)

Joy

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html