wgengine/magicsock: add filter to ignore disco to old/other ports

Incoming disco packets are now dropped unless they match one of the
current bound ports, or have a zero port*.

The BPF filter passes all packets with a disco header to the raw packet
sockets regardless of destination port (in order to avoid needing to
reconfigure BPF on rebind).

If a BPF enabled node has just rebound, due to restart or rebind, it may
receive and reply to disco ping packets destined for ports other than
those which are presently bound. If the pong is accepted, the pinging
node will now assume that it can send WireGuard traffic to the pinged
port - such traffic will not reach the node as it is not destined for a
bound port.

*The zero port is ignored, if received. This is a speculative defense
and would indicate a problem in the receive path, or the BPF filter.
This condition is allowed to pass as it may enable traffic to flow,
however it will also enable problems with the same symptoms this patch
otherwise fixes.

Fixes #5536

Signed-off-by: James Tucker <james@tailscale.com>
This commit is contained in:
James Tucker 2022-09-02 10:33:46 -07:00 committed by James Tucker
parent be140add75
commit 672c2c8de8
2 changed files with 35 additions and 2 deletions

View File

@ -2991,11 +2991,13 @@ type RebindingUDPConn struct {
mu sync.Mutex // held while changing pconn (and pconnAtomic) mu sync.Mutex // held while changing pconn (and pconnAtomic)
pconn nettype.PacketConn pconn nettype.PacketConn
port uint16
} }
func (c *RebindingUDPConn) setConnLocked(p nettype.PacketConn) { func (c *RebindingUDPConn) setConnLocked(p nettype.PacketConn) {
c.pconn = p c.pconn = p
c.pconnAtomic.Store(p) c.pconnAtomic.Store(p)
c.port = uint16(c.localAddrLocked().Port)
} }
// currentConn returns c's current pconn, acquiring c.mu in the process. // currentConn returns c's current pconn, acquiring c.mu in the process.
@ -3057,6 +3059,12 @@ func (c *RebindingUDPConn) ReadFromNetaddr(b []byte) (n int, ipp netip.AddrPort,
} }
} }
func (c *RebindingUDPConn) Port() uint16 {
c.mu.Lock()
defer c.mu.Unlock()
return c.port
}
func (c *RebindingUDPConn) LocalAddr() *net.UDPAddr { func (c *RebindingUDPConn) LocalAddr() *net.UDPAddr {
c.mu.Lock() c.mu.Lock()
defer c.mu.Unlock() defer c.mu.Unlock()
@ -3081,6 +3089,7 @@ func (c *RebindingUDPConn) closeLocked() error {
if c.pconn == nil { if c.pconn == nil {
return errNilPConn return errNilPConn
} }
c.port = 0
return c.pconn.Close() return c.pconn.Close()
} }

View File

@ -195,11 +195,11 @@ func (c *Conn) listenRawDisco(family string) (io.Closer, error) {
} }
pc.SetReadDeadline(time.Time{}) pc.SetReadDeadline(time.Time{})
go c.receiveDisco(pc) go c.receiveDisco(pc, family == "ip6")
return pc, nil return pc, nil
} }
func (c *Conn) receiveDisco(pc net.PacketConn) { func (c *Conn) receiveDisco(pc net.PacketConn, isIPV6 bool) {
var buf [1500]byte var buf [1500]byte
for { for {
n, src, err := pc.ReadFrom(buf[:]) n, src, err := pc.ReadFrom(buf[:])
@ -213,6 +213,30 @@ func (c *Conn) receiveDisco(pc net.PacketConn) {
// Too small to be a valid UDP datagram, drop. // Too small to be a valid UDP datagram, drop.
continue continue
} }
dstPort := binary.BigEndian.Uint16(buf[2:4])
if dstPort == 0 {
c.logf("[unexpected] disco raw: received packet for port 0")
}
var acceptPort uint16
if isIPV6 {
acceptPort = c.pconn6.Port()
} else {
acceptPort = c.pconn4.Port()
}
if acceptPort == 0 {
// This should only typically happen if the receiving address family
// was recently disabled.
c.logf("[v1] disco raw: dropping packet for port %d as acceptPort=0", dstPort)
continue
}
if dstPort != acceptPort {
c.logf("[v1] disco raw: dropping packet for port %d", dstPort)
continue
}
srcIP, ok := netip.AddrFromSlice(src.(*net.IPAddr).IP) srcIP, ok := netip.AddrFromSlice(src.(*net.IPAddr).IP)
if !ok { if !ok {
c.logf("[unexpected] PacketConn.ReadFrom returned not-an-IP %v in from", src) c.logf("[unexpected] PacketConn.ReadFrom returned not-an-IP %v in from", src)