Merge pull request #258 from anywherelan/fix/vpn-malformed-packet-dos

vpn: fix remote crash and CPU-exhaustion from malformed tunnel packets

Author: pymq

application_test.go

@@ -405,6 +405,16 @@ func TestSOCKS5ProxyFallbackToOldProtocol(t *testing.T) {
 	// Remove new protocol handler from peer2 to simulate old peer
 	peer2.app.P2p.Host().RemoveStreamHandler(protocol.Socks5NoAuthMethod)
 
+	// Wait until peer1's peerstore reflects the removal (identify-push). Otherwise
+	// NewStreamMulti optimistically negotiates the stale /socks5-noauth/ against a
+	// peer that no longer handles it, and the dial fails with EOF.
+	ts.Eventually(func() bool {
+		supported, err := peer1.app.P2p.Host().Peerstore().
+			SupportsProtocols(peer2.app.P2p.PeerID(), protocol.Socks5NoAuthMethod)
+		ts.NoError(err)
+		return len(supported) == 0
+	}, 15*time.Second, 100*time.Millisecond)
+
 	// Allow peer1 to use peer2 as exit node
 	peer1Config, err := peer2.api.KnownPeerConfig(peer1.PeerID())
 	ts.NoError(err)

protocol/protocol_test.go

@@ -95,13 +95,22 @@ func TestReadPacketHeader_RejectsBothFlags(t *testing.T) {
 
 func TestReadPacketHeader_RejectsHugeSize(t *testing.T) {
 	var header [8]byte
-	binary.BigEndian.PutUint64(header[:], tunnelMaxLength+1)
+	binary.BigEndian.PutUint64(header[:], uint64(vpn.MaxPacketBodySize)+1)
 
 	_, _, err := ReadPacketHeader(bytes.NewReader(header[:]))
 	require.Error(t, err)
 	require.Contains(t, err.Error(), "exceeds max")
 }
 
+func TestReadPacketHeader_AcceptsMaxBodySize(t *testing.T) {
+	var header [8]byte
+	binary.BigEndian.PutUint64(header[:], uint64(vpn.MaxPacketBodySize))
+
+	size, _, err := ReadPacketHeader(bytes.NewReader(header[:]))
+	require.NoError(t, err)
+	require.Equal(t, uint64(vpn.MaxPacketBodySize), size)
+}
+
 func TestReadPacketHeader_TruncatedStream(t *testing.T) {
 	_, _, err := ReadPacketHeader(bytes.NewReader([]byte{1, 2, 3}))
 	require.Error(t, err)

protocol/tunnel.go

@@ -25,11 +25,6 @@ const (
 	tunnelFlagGatewayForward uint64 = 1 << 63
 	tunnelFlagGatewayReturn  uint64 = 1 << 62
 	tunnelLengthMask         uint64 = ^(tunnelFlagGatewayForward | tunnelFlagGatewayReturn)
-	// tunnelMaxLength bounds the per-packet size we'll accept on read. The
-	// real cap is vpn.maxContentSize (MTU + overhead); this is a generous
-	// upper bound to reject obvious garbage early without crossing package
-	// dependencies.
-	tunnelMaxLength uint64 = 1 << 20
 )
 
 // ReadPacketHeader reads an 8-byte tunnel packet header from the stream and
@@ -53,8 +48,8 @@ func ReadPacketHeader(stream io.Reader) (size uint64, dir vpn.GatewayDir, err er
 		dir = vpn.GatewayDirReturn
 	}
 	size = v & tunnelLengthMask
-	if size > tunnelMaxLength {
-		return 0, 0, fmt.Errorf("invalid tunnel header: size %d exceeds max %d", size, tunnelMaxLength)
+	if size > uint64(vpn.MaxPacketBodySize) {
+		return 0, 0, fmt.Errorf("invalid tunnel header: size %d exceeds max %d", size, vpn.MaxPacketBodySize)
 	}
 	return size, dir, nil
 }

vpn/iface_darwin.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"net"
 	"os/exec"
+	"strings"
 
 	"golang.zx2c4.com/wireguard/tun"
 )
@@ -21,26 +22,33 @@ func newTUN(ifname string, mtu int, localIP net.IP, ipMask net.IPMask) (tun.Devi
 	if err != nil {
 		return nil, fmt.Errorf("create tun: %v", err)
 	}
+	// Close the freshly created device if any later setup step fails, otherwise
+	// the TUN interface leaks.
+	success := false
+	defer func() {
+		if !success {
+			_ = tunDevice.Close()
+		}
+	}()
 	// Interface name must be utun[0-9]*
 	realIfname, err := tunDevice.Name()
 	if err != nil {
 		return nil, fmt.Errorf("get interface name: %v", err)
 	}
 
-	err = exec.Command("ifconfig", realIfname, "inet", ipNet.String(), localIP.String()).Run()
-	if err != nil {
-		return nil, fmt.Errorf("unable to setup interface mask: %v", err)
+	if out, err := exec.Command("ifconfig", realIfname, "inet", ipNet.String(), localIP.String()).CombinedOutput(); err != nil {
+		return nil, fmt.Errorf("unable to setup interface mask: %v: %s", err, strings.TrimSpace(string(out)))
 	}
 
 	ipNetMasked := &net.IPNet{
 		IP:   localIP.Mask(ipMask),
 		Mask: ipMask,
 	}
-	err = exec.Command("route", "-q", "-n", "add", "-inet", ipNetMasked.String(), "-iface", realIfname).Run()
-	if err != nil {
-		return nil, fmt.Errorf("unable to setup interface route: %v", err)
+	if out, err := exec.Command("route", "-q", "-n", "add", "-inet", ipNetMasked.String(), "-iface", realIfname).CombinedOutput(); err != nil {
+		return nil, fmt.Errorf("unable to setup interface route: %v: %s", err, strings.TrimSpace(string(out)))
 	}
 
+	success = true
 	return tunDevice, nil
 }
 

vpn/iface_linux.go

@@ -16,6 +16,14 @@ func newTUN(ifname string, mtu int, localIP net.IP, ipMask net.IPMask) (tun.Devi
 	if err != nil {
 		return nil, fmt.Errorf("create tun: %v", err)
 	}
+	// Close the freshly created device if any later setup step fails, otherwise
+	// the TUN interface leaks.
+	success := false
+	defer func() {
+		if !success {
+			_ = tunDevice.Close()
+		}
+	}()
 
 	link, err := netlink.LinkByName(ifname)
 	if err != nil {
@@ -36,6 +44,7 @@ func newTUN(ifname string, mtu int, localIP net.IP, ipMask net.IPMask) (tun.Devi
 		return nil, fmt.Errorf("unable to UP interface: %v", err)
 	}
 
+	success = true
 	return tunDevice, nil
 }
 

vpn/iface_windows.go

@@ -44,6 +44,14 @@ func newTUN(ifname string, mtu int, localIP net.IP, ipMask net.IPMask) (tun.Devi
 	if err != nil {
 		return nil, fmt.Errorf("do as system: %v", err)
 	}
+	// Close the freshly created adapter if any later setup step fails, otherwise
+	// the Wintun adapter leaks.
+	success := false
+	defer func() {
+		if !success {
+			_ = tunDevice.Close()
+		}
+	}()
 
 	nativeTunDevice := tunDevice.(*tun.NativeTun)
 	luid := winipcfg.LUID(nativeTunDevice.LUID())
@@ -54,7 +62,6 @@ func newTUN(ifname string, mtu int, localIP net.IP, ipMask net.IPMask) (tun.Devi
 	// force NLMTU ourselves via winipcfg, otherwise local apps see MTU=65535,
 	// send oversized packets, and the size check in ReadTUNPackets silently drops them.
 	if err := setInterfaceMTU(logger, luid, winipcfg.AddressFamily(windows.AF_INET), uint32(mtu)); err != nil {
-		tunDevice.Close()
 		return nil, fmt.Errorf("set IPv4 MTU on tun: %v", err)
 	}
 	// TODO: support ipv6. Forwarding still ignores IPv6 packets (see Device.WritePacket),
@@ -74,6 +81,7 @@ func newTUN(ifname string, mtu int, localIP net.IP, ipMask net.IPMask) (tun.Devi
 		return nil, fmt.Errorf("unable to setup interface IP: %v", err)
 	}
 
+	success = true
 	return tunDevice, nil
 }
 

vpn/packet.go

@@ -2,6 +2,7 @@ package vpn
 
 import (
 	"encoding/binary"
+	"fmt"
 	"io"
 	"net"
 
@@ -61,12 +62,17 @@ func (data *Packet) clear() {
 
 func (data *Packet) CopyTo(copyPacket *Packet) {
 	*copyPacket = *data
-	// set Packet reference to a new buffer
+	// Buffer is an array copied by value above, but Packet/Src/Dst are slice
+	// headers still aliasing data.Buffer. Re-derive them as sub-slices of the
+	// copy's own buffer.
 	copyPacket.Packet = copyPacket.Buffer[tunPacketOffset : len(data.Packet)+tunPacketOffset]
+	if data.Src != nil {
+		copyPacket.setAddrs()
+	}
 }
 
 func (data *Packet) ReadFrom(stream io.Reader) (int64, error) {
-	var totalRead = tunPacketOffset
+	totalRead := tunPacketOffset
 	for {
 		n, err := stream.Read(data.Buffer[totalRead:])
 		totalRead += n
@@ -76,6 +82,13 @@ func (data *Packet) ReadFrom(stream io.Reader) (int64, error) {
 		} else if err != nil {
 			return int64(totalRead - tunPacketOffset), err
 		}
+		// The buffer is sized with slack above the MTU (maxContentSize), so a
+		// valid single-packet body never fills it. A full buffer means an
+		// oversized frame; reject it instead of reading into the empty tail
+		// (which would spin on (0, nil) reads).
+		if totalRead == len(data.Buffer) {
+			return int64(totalRead - tunPacketOffset), fmt.Errorf("packet body exceeds max size %d bytes", MaxPacketBodySize)
+		}
 	}
 }
 
@@ -90,19 +103,25 @@ func (data *Packet) Parse() bool {
 		if len(packet) < ipv4.HeaderLen {
 			return false
 		}
+		// Validate the header length against the actual packet so downstream
+		// slicing (RecalculateChecksum) can't run past the packet and panic.
+		// Transport-length is not enforced here (it would drop legit non-first IP
+		// fragments); RecalculateChecksum guards its own transport slicing.
+		ipHeaderLen := int(packet[0]&0x0f) << 2
+		if ipHeaderLen < ipv4.HeaderLen || ipHeaderLen > len(packet) {
+			return false
+		}
+		data.IPProtocol = packet[9]
 
-		data.Src = packet[device.IPv4offsetSrc : device.IPv4offsetSrc+net.IPv4len]
-		data.Dst = packet[device.IPv4offsetDst : device.IPv4offsetDst+net.IPv4len]
 		data.IsIPv6 = false
-		data.IPProtocol = data.Packet[9]
+		data.setAddrs()
 	case ipv6.Version:
 		if len(packet) < ipv6.HeaderLen {
 			return false
 		}
 
-		data.Src = packet[device.IPv6offsetSrc : device.IPv6offsetSrc+net.IPv6len]
-		data.Dst = packet[device.IPv6offsetDst : device.IPv6offsetDst+net.IPv6len]
 		data.IsIPv6 = true
+		data.setAddrs()
 		// TODO: set data.IPProtocol
 	default:
 		return false
@@ -114,24 +133,45 @@ func (data *Packet) Parse() bool {
 func (data *Packet) RecalculateChecksum() {
 	if data.IsIPv6 {
 		// TODO
-	} else {
-		ipHeaderLen := int(data.Packet[0]&0x0f) << 2
-		copy(data.Packet[ipv4offsetChecksum:], []byte{0, 0})
-		ipChecksum := checksumIPv4Header(data.Packet[:ipHeaderLen])
-		binary.BigEndian.PutUint16(data.Packet[ipv4offsetChecksum:], ipChecksum)
-
-		switch protocol := data.Packet[9]; protocol {
-		case IPProtocolTCP:
-			tcpOffsetChecksum := ipHeaderLen + 16
-			copy(data.Packet[tcpOffsetChecksum:], []byte{0, 0})
-			checksum := checksumIPv4TCPUDP(data.Packet[ipHeaderLen:], uint32(protocol), data.Src, data.Dst)
-			binary.BigEndian.PutUint16(data.Packet[tcpOffsetChecksum:], checksum)
-		case IPProtocolUDP:
-			udpOffsetChecksum := ipHeaderLen + 6
-			copy(data.Packet[udpOffsetChecksum:], []byte{0, 0})
-			checksum := checksumIPv4TCPUDP(data.Packet[ipHeaderLen:], uint32(protocol), data.Src, data.Dst)
-			binary.BigEndian.PutUint16(data.Packet[udpOffsetChecksum:], checksum)
+		return
+	}
+	// Guard against malformed lengths so a bad packet can't slice past bounds and
+	// panic, even if RecalculateChecksum is called without a prior valid Parse.
+	ipHeaderLen := int(data.Packet[0]&0x0f) << 2
+	if ipHeaderLen < ipv4.HeaderLen || ipHeaderLen > len(data.Packet) {
+		return
+	}
+	copy(data.Packet[ipv4offsetChecksum:], []byte{0, 0})
+	ipChecksum := checksumIPv4Header(data.Packet[:ipHeaderLen])
+	binary.BigEndian.PutUint16(data.Packet[ipv4offsetChecksum:], ipChecksum)
+
+	switch protocol := data.Packet[9]; protocol {
+	case IPProtocolTCP:
+		if len(data.Packet) < ipHeaderLen+18 {
+			return
 		}
+		tcpOffsetChecksum := ipHeaderLen + 16
+		copy(data.Packet[tcpOffsetChecksum:], []byte{0, 0})
+		checksum := checksumIPv4TCPUDP(data.Packet[ipHeaderLen:], uint32(protocol), data.Src, data.Dst)
+		binary.BigEndian.PutUint16(data.Packet[tcpOffsetChecksum:], checksum)
+	case IPProtocolUDP:
+		if len(data.Packet) < ipHeaderLen+8 {
+			return
+		}
+		udpOffsetChecksum := ipHeaderLen + 6
+		copy(data.Packet[udpOffsetChecksum:], []byte{0, 0})
+		checksum := checksumIPv4TCPUDP(data.Packet[ipHeaderLen:], uint32(protocol), data.Src, data.Dst)
+		binary.BigEndian.PutUint16(data.Packet[udpOffsetChecksum:], checksum)
+	}
+}
+
+func (data *Packet) setAddrs() {
+	if data.IsIPv6 {
+		data.Src = data.Packet[device.IPv6offsetSrc : device.IPv6offsetSrc+net.IPv6len]
+		data.Dst = data.Packet[device.IPv6offsetDst : device.IPv6offsetDst+net.IPv6len]
+	} else {
+		data.Src = data.Packet[device.IPv4offsetSrc : device.IPv4offsetSrc+net.IPv4len]
+		data.Dst = data.Packet[device.IPv4offsetDst : device.IPv4offsetDst+net.IPv4len]
 	}
 }