diff --git a/client/iface/bind/ice_bind.go b/client/iface/bind/ice_bind.go index f55d1fd12..2d35b9c6f 100644 --- a/client/iface/bind/ice_bind.go +++ b/client/iface/bind/ice_bind.go @@ -27,8 +27,9 @@ const ( wgMsgTypeHandshakeInitiation uint32 = 1 // wgMsgTypeTransport is the highest WireGuard message type. wgMsgTypeTransport uint32 = 4 - // wgKeepaliveSize is the size of a WireGuard transport message with an empty payload. - wgKeepaliveSize = 32 + // wgMinMsgSize is the smallest WireGuard message: transport data with an empty + // payload, which is what a keepalive is. + wgMinMsgSize = 32 ) type receiverCreator struct { @@ -229,9 +230,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. + // 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 } @@ -367,17 +369,19 @@ func putMessages(msgs *[]ipv6.Message, msgsPool *sync.Pool) { msgsPool.Put(msgs) } -// 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. +// 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 +// after the type byte zero, in a packet long enough to hold any WireGuard message. // -// 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. +// A well formed STUN message cannot take that shape. Its length field sits in the two +// bytes the type must leave zero, and for a message of at least wgMinMsgSize bytes that +// field holds at least 12, so the two framings do not overlap. The test has to be this +// tight 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 { + if len(pkt) < wgMinMsgSize { return false } @@ -385,13 +389,14 @@ func isWireGuardMsg(pkt []byte) bool { return msgType >= wgMsgTypeHandshakeInitiation && msgType <= wgMsgTypeTransport } -// isTransportPkg reports whether the packet is WireGuard transport data carrying a payload, which -// is what counts as peer activity. Keepalives hold no payload and are exactly wgKeepaliveSize. +// isTransportPkg reports whether the packet is WireGuard transport data carrying a +// payload, which is what counts as peer activity. A keepalive holds no payload and is +// exactly wgMinMsgSize bytes. func isTransportPkg(buffers [][]byte, n int) bool { if n < 4 || n > len(buffers[0]) { return false } msgType := binary.LittleEndian.Uint32(buffers[0][:4]) - return msgType == wgMsgTypeTransport && n > wgKeepaliveSize + return msgType == wgMsgTypeTransport && n > wgMinMsgSize } diff --git a/client/iface/bind/stun_filter_test.go b/client/iface/bind/stun_filter_test.go index fc73c6822..0e118e0fd 100644 --- a/client/iface/bind/stun_filter_test.go +++ b/client/iface/bind/stun_filter_test.go @@ -15,15 +15,15 @@ import ( wgConn "golang.zx2c4.com/wireguard/conn" ) -// magicCookieBytes is the STUN magic cookie as it appears on the wire. In a WireGuard message the -// same offset holds the receiver (or sender) index, which is a random uint32, so a session can draw -// exactly this value. +// magicCookieBytes is the STUN magic cookie as it appears on the wire. In a +// WireGuard message the same offset holds the receiver (or sender) index, which is +// a random uint32, so a session can draw exactly this value. var magicCookieBytes = []byte{0x21, 0x12, 0xA4, 0x42} const testBufSize = 1500 -// wgMsg builds a WireGuard message of the given type and size, with the index field at bytes 4:8 -// set to index. +// wgMsg builds a WireGuard message of the given type and size, with the index field +// at bytes 4:8 set to index. func wgMsg(msgType uint32, size int, index []byte) []byte { pkt := make([]byte, size) binary.LittleEndian.PutUint32(pkt[:4], msgType) @@ -31,8 +31,8 @@ func wgMsg(msgType uint32, size int, index []byte) []byte { return pkt } -// intoBuffer copies pkt into a full-size receive buffer, the way the kernel read does, so tests see -// the same buffer/length split as the hot path. +// intoBuffer copies pkt into a full-size receive buffer, the way the kernel read +// does, so tests see the same buffer/length split as the hot path. func intoBuffer(pkt []byte) [][]byte { buf := make([]byte, testBufSize) copy(buf, pkt) @@ -46,7 +46,7 @@ func TestFilterOutStunMessages_PassesWireGuardWithCookieShapedIndex(t *testing.T size int }{ {"transport data", wgMsgTypeTransport, 128}, - {"keepalive", wgMsgTypeTransport, wgKeepaliveSize}, + {"keepalive", wgMsgTypeTransport, wgMinMsgSize}, {"handshake initiation", wgMsgTypeHandshakeInitiation, 148}, {"handshake response", 2, 92}, {"cookie reply", 3, 64}, @@ -81,8 +81,9 @@ func TestFilterOutStunMessages_FiltersRealSTUNMessage(t *testing.T) { assert.Empty(t, buffers[0], "consumed buffer must be emptied so WireGuard does not see it") } -// TestIsWireGuardMsg_DisjointFromSTUN locks the invariant the filter relies on: the second byte of a -// STUN message type is never zero, so no STUN message can be mistaken for a WireGuard header. +// TestIsWireGuardMsg_DisjointFromSTUN locks the invariant the filter relies on: a +// well formed STUN message long enough to be a WireGuard message always has a +// non-zero length field, so it cannot be mistaken for a WireGuard header. func TestIsWireGuardMsg_DisjointFromSTUN(t *testing.T) { types := []stun.MessageType{ stun.BindingRequest, @@ -92,9 +93,13 @@ func TestIsWireGuardMsg_DisjointFromSTUN(t *testing.T) { } for _, msgType := range types { - msg, err := stun.Build(msgType, stun.TransactionID) + // Long enough that the length guard is not what makes this pass. + msg, err := stun.Build(msgType, stun.TransactionID, + stun.NewUsername("remoteUfrag:localUfrag"), stun.Fingerprint) require.NoError(t, err) - assert.False(t, isWireGuardMsg(msg.Raw), "%s must not look like a WireGuard message", msgType) + require.GreaterOrEqual(t, len(msg.Raw), wgMinMsgSize, "precondition: %s", msgType) + assert.False(t, isWireGuardMsg(msg.Raw), + "%s must not look like a WireGuard message", msgType) } } @@ -115,13 +120,13 @@ func TestIsWireGuardMsg(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - assert.Equal(t, tc.want, isWireGuardMsg(tc.pkt)) + assert.Equal(t, tc.want, isWireGuardMsg(tc.pkt), "wrong classification for %s", tc.name) }) } } -// TestFilterOutStunMessages_IgnoresBytesBeyondPacket guards against classifying on buffer contents -// left over from an earlier, longer packet. +// TestFilterOutStunMessages_IgnoresBytesBeyondPacket guards against classifying on +// buffer contents left over from an earlier, longer packet. func TestFilterOutStunMessages_IgnoresBytesBeyondPacket(t *testing.T) { buf := make([]byte, testBufSize) copy(buf[4:8], magicCookieBytes) @@ -133,10 +138,11 @@ 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. +// 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() @@ -156,7 +162,8 @@ func TestReceiveFn_ClearsSizeOfConsumedPacket(t *testing.T) { 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. + // A leftover size from an earlier read, which is what makes the missing reset + // observable. sizes := []int{148} eps := make([]wgConn.Endpoint, 1) @@ -174,14 +181,35 @@ func TestIsTransportPkg(t *testing.T) { want bool }{ {"transport data with payload", wgMsg(wgMsgTypeTransport, 128, nil), 128, true}, - {"keepalive", wgMsg(wgMsgTypeTransport, wgKeepaliveSize, nil), wgKeepaliveSize, false}, + {"keepalive", wgMsg(wgMsgTypeTransport, wgMinMsgSize, nil), wgMinMsgSize, false}, {"handshake initiation", wgMsg(wgMsgTypeHandshakeInitiation, 148, nil), 148, false}, {"stale type bytes beyond packet", wgMsg(wgMsgTypeTransport, 128, nil), 2, false}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - assert.Equal(t, tc.want, isTransportPkg(intoBuffer(tc.pkt), tc.n)) + assert.Equal(t, tc.want, isTransportPkg(intoBuffer(tc.pkt), tc.n), + "wrong activity classification for %s", tc.name) }) } } + +// TestFilterOutStunMessages_ConsumesSTUNWithWireGuardShapedType covers the one STUN +// encoding whose leading bytes collide with a WireGuard message type: method 0x080 as a +// request encodes to 0x0200, so the type byte reads as a handshake response and the byte +// after it is zero. Only the length check keeps such a message out of WireGuard's hands. +// pion implements no method in that range, so this is a synthetic worst case rather than +// traffic ICE produces. +func TestFilterOutStunMessages_ConsumesSTUNWithWireGuardShapedType(t *testing.T) { + msg, err := stun.Build(stun.NewType(stun.Method(0x080), stun.ClassRequest), stun.TransactionID) + require.NoError(t, err) + require.Equal(t, []byte{0x02, 0x00, 0x00, 0x00}, msg.Raw[:4], + "precondition: the leading bytes read as a WireGuard message type") + + buffers := intoBuffer(msg.Raw) + bind := &ICEBind{} + + filtered, err := bind.filterOutStunMessages(buffers, len(msg.Raw), &net.UDPAddr{}) + assert.NoError(t, err) + assert.True(t, filtered, "STUN message must be consumed despite its WireGuard-shaped type") +}