Require a minimum length before classifying a packet as WireGuard

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
 }

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")
+}