diff --git a/client/iface/bind/ice_bind.go b/client/iface/bind/ice_bind.go index cf1a8316c..f55d1fd12 100644 --- a/client/iface/bind/ice_bind.go +++ b/client/iface/bind/ice_bind.go @@ -229,6 +229,10 @@ func (s *ICEBind) createReceiverFn(pc wgConn.BatchReader, conn *net.UDPConn, rxO if err != nil { log.Debugf("failed to handle STUN packet from %s: %v", msg.Addr, err) } + // WireGuard reuses sizes and eps across reads and only skips a slot whose size is + // below the minimum message size. Leaving a consumed slot untouched makes it + // process this buffer again under the previous packet's length and endpoint. + sizes[i] = 0 continue } sizes[i] = msg.N @@ -366,12 +370,12 @@ func putMessages(msgs *[]ipv6.Message, msgsPool *sync.Pool) { // isWireGuardMsg reports whether the packet carries a WireGuard message header: a little-endian // uint32 message type in the range 1..4, which leaves the three bytes following the type byte zero. // -// No STUN message can take that shape. The low byte of a STUN message type holds method and class -// bits and is non-zero for every method (Binding is 0x0001, 0x0101, 0x0111, 0x0011), so the two -// framings are disjoint and this test is exact rather than heuristic. That matters because -// stun.IsMessage only looks at the magic cookie, which in a WireGuard message overlaps the receiver -// index: a session whose index happens to equal the cookie would otherwise have all of its inbound -// data misrouted to the STUN handler until the next rekey. +// No STUN message that ICE exchanges can take that shape. The low byte of a STUN message type holds +// the bottom method bits and a class bit, and it is non-zero for Binding (0x0001, 0x0101, 0x0111, +// 0x0011) and for every other method pion implements, so the two framings do not overlap. That +// matters because stun.IsMessage only looks at the magic cookie, which in a WireGuard message +// overlaps the receiver index: a session whose index happens to equal the cookie would otherwise +// have all of its inbound data misrouted to the STUN handler until the next rekey. func isWireGuardMsg(pkt []byte) bool { if len(pkt) < 4 { return false diff --git a/client/iface/bind/stun_filter_test.go b/client/iface/bind/stun_filter_test.go index 6c313fa88..fc73c6822 100644 --- a/client/iface/bind/stun_filter_test.go +++ b/client/iface/bind/stun_filter_test.go @@ -6,10 +6,13 @@ import ( "encoding/binary" "net" "testing" + "time" "github.com/pion/stun/v3" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/net/ipv4" + wgConn "golang.zx2c4.com/wireguard/conn" ) // magicCookieBytes is the STUN magic cookie as it appears on the wire. In a WireGuard message the @@ -130,6 +133,39 @@ func TestFilterOutStunMessages_IgnoresBytesBeyondPacket(t *testing.T) { assert.False(t, filtered, "a 2 byte packet must not be classified from stale buffer bytes") } +// TestReceiveFn_ClearsSizeOfConsumedPacket covers the accounting WireGuard relies on: sizes is +// reused across reads, so a slot whose packet was consumed as STUN must be reported as empty. +// Otherwise WireGuard reprocesses the same buffer under the previous packet's length, which for a +// WireGuard-shaped packet means it is handled twice. +func TestReceiveFn_ClearsSizeOfConsumedPacket(t *testing.T) { + conn := listenUDP(t, "udp4", "127.0.0.1:0") + defer conn.Close() + + recvFn := receiverCreator{setupICEBind(t)}.CreateReceiverFn( + ipv4.NewPacketConn(conn), conn, false, createMsgPool(), + ) + + msg, err := stun.Build(stun.BindingRequest, stun.TransactionID, stun.Fingerprint) + require.NoError(t, err) + + sender := listenUDP(t, "udp4", "127.0.0.1:0") + defer sender.Close() + _, err = sender.WriteTo(msg.Raw, conn.LocalAddr()) + require.NoError(t, err) + + require.NoError(t, conn.SetReadDeadline(time.Now().Add(3*time.Second))) + + bufs := [][]byte{make([]byte, 1500)} + // A leftover size from an earlier read, which is what makes the missing reset observable. + sizes := []int{148} + eps := make([]wgConn.Endpoint, 1) + + n, err := recvFn(bufs, sizes, eps) + require.NoError(t, err) + require.Equal(t, 1, n) + assert.Zero(t, sizes[0], "consumed STUN packet must not leave a size behind for WireGuard") +} + func TestIsTransportPkg(t *testing.T) { tests := []struct { name string