mirror of
https://github.com/netbirdio/netbird.git
synced 2026-08-04 19:55:09 -04:00
Clear the size of a packet consumed by the STUN handler
WireGuard reuses the sizes and endpoints slices across reads and skips a slot only when its size is below the minimum message size. A packet consumed as STUN left the previous read's size in place, so WireGuard processed the same buffer again under a stale length and endpoint.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user