smartalock / wireguard-lwip

WireGuard Implementation for lwIP
Other
192 stars 29 forks source link

wireguard_check_replay() always rejects first data packet of session #5

Closed bwindrim closed 1 year ago

bwindrim commented 1 year ago

In wireguard.c, line 326, wireguard_check_replay() test for the seq argument being non-zero. This is almost the first thing that the function does, and if seq is zero then the function always returns false (i.e. rejecting the packet). Since seq is the sequential packet count, beginning at zero, this means that the first data packet of a session will always be dropped by WireGuard and will require retransmission. Furthermore, since sessions are re-created every 2-3 minutes - resetting the sequence count to zero - single packets may continue to be dropped at regular intervals.

This (and other) behaviour of wireguard_check_replay() appears to come directly from RFC2401:

if (seq == 0) return 0;             /* first == 0 or wrapped */

However the replay-checking logic in the Linux WireGuard function counter_validate(), which implements a different windowing algorithm based on RFC6479, appears to contain no such special treatment of a sequence value of zero.

Since the dropped packets will tend to be re-transmitted, either by the TCP layer, or by the application, no actual failures are likely to be observed, but the behaviour (and that of the RFC2401 example code) is puzzling.

smartalock commented 1 year ago

Thanks for reporting the issue.

I think the issue is that the replay detection algorithms in RFC2401 and RFC6479 assume that the first packet will have sequence number 1

If you look at the counter_validate() function in the Linux implementation (https://git.zx2c4.com/wireguard-linux/tree/drivers/net/wireguard/receive.c#n295) you can see that the counter is incremented before the check starts. I have added a similar fix in 0c44df308984abfb9c9df628002cdf8154c55506 which should hopefully fix this