From f7eed25bb91e11514a0b8a7bc965397481bbf1ac Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 3 Feb 2021 18:15:01 -0800 Subject: [PATCH] wgengine/magicsock: filter disco packets and packets when stopped from wireguard Fixes #1167 Fixes tailscale/corp#219 Signed-off-by: Brad Fitzpatrick --- wgengine/magicsock/magicsock.go | 55 ++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 18 deletions(-) diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index d28a07db3..5e3120453 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -304,6 +304,9 @@ type Conn struct { // with IPv4 or IPv6). It's used to suppress log spam and prevent // new connection that'll fail. networkUp syncs.AtomicBool + + // havePrivateKey is whether privateKey is non-zero. + havePrivateKey syncs.AtomicBool } // derpRoute is a route entry for a public key, saying that a certain @@ -1588,6 +1591,9 @@ func (c *Conn) ReceiveIPv4(b []byte) (n int, ep conn.Endpoint, err error) { } // receiveIP is the shared bits of ReceiveIPv4 and ReceiveIPv6. +// +// ok is whether this read should be reported up to wireguard-go (our +// caller). func (c *Conn) receiveIP(b []byte, ua *net.UDPAddr, cache *ippEndpointCache) (ep conn.Endpoint, ok bool) { ipp, ok := netaddr.FromStdAddr(ua.IP, ua.Port, ua.Zone) if !ok { @@ -1600,6 +1606,13 @@ func (c *Conn) receiveIP(b []byte, ua *net.UDPAddr, cache *ippEndpointCache) (ep if c.handleDiscoMessage(b, ipp) { return nil, false } + if !c.havePrivateKey.Get() { + // If we have no private key, we're logged out or + // stopped. Don't try to pass these wireguard packets + // up to wireguard-go; it'll just complain (Issue + // 1167). + return nil, false + } if cache.ipp == ipp && cache.de != nil && cache.gen == cache.de.numStopAndReset() { ep = cache.de } else { @@ -1750,8 +1763,8 @@ func (c *Conn) sendDiscoMessage(dst netaddr.IPPort, dstKey tailcfg.NodeKey, dstD return sent, err } -// handleDiscoMessage reports whether msg was a Tailscale inter-node discovery message -// that was handled. +// handleDiscoMessage handles a discovery message and reports whether +// msg was a Tailscale inter-node discovery message. // // A discovery message has the form: // @@ -1762,11 +1775,18 @@ func (c *Conn) sendDiscoMessage(dst netaddr.IPPort, dstKey tailcfg.NodeKey, dstD // // For messages received over DERP, the addr will be derpMagicIP (with // port being the region) -func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) bool { +func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) (isDiscoMsg bool) { const headerLen = len(disco.Magic) + len(tailcfg.DiscoKey{}) + disco.NonceLen if len(msg) < headerLen || string(msg[:len(disco.Magic)]) != disco.Magic { return false } + + // If the first four parts are the prefix of disco.Magic + // (0x5453f09f) then it's definitely not a valid Wireguard + // packet (which starts with little-endian uint32 1, 2, 3, 4). + // Use naked returns for all following paths. + isDiscoMsg = true + var sender tailcfg.DiscoKey copy(sender[:], msg[len(disco.Magic):]) @@ -1774,20 +1794,21 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) bool { defer c.mu.Unlock() if c.closed { - return true + return } if debugDisco { c.logf("magicsock: disco: got disco-looking frame from %v", sender.ShortString()) } if c.privateKey.IsZero() { // Ignore disco messages when we're stopped. - return false + // Still return true, to not pass it down to wireguard. + return } if c.discoPrivate.IsZero() { if debugDisco { c.logf("magicsock: disco: ignoring disco-looking frame, no local key") } - return false + return } peerNode, ok := c.nodeOfDisco[sender] @@ -1795,9 +1816,7 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) bool { if debugDisco { c.logf("magicsock: disco: ignoring disco-looking frame, don't know node for %v", sender.ShortString()) } - // Returning false keeps passing it down, to WireGuard. - // WireGuard will almost surely reject it, but give it a chance. - return false + return } needsRecvActivityCall := false @@ -1810,7 +1829,7 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) bool { c.logf("magicsock: got disco message from idle peer, starting lazy conf for %v, %v", peerNode.Key.ShortString(), sender.ShortString()) if c.noteRecvActivity == nil { c.logf("magicsock: [unexpected] have node without endpoint, without c.noteRecvActivity hook") - return false + return } needsRecvActivityCall = true } else { @@ -1829,7 +1848,7 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) bool { // Now, recheck invariants that might've changed while we'd // released the lock, which isn't much: if c.closed || c.privateKey.IsZero() { - return true + return } de, ok = c.endpointOfDisco[sender] if !ok { @@ -1838,7 +1857,7 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) bool { return false } c.logf("magicsock: [unexpected] lazy endpoint not created for %v, %v", peerNode.Key.ShortString(), sender.ShortString()) - return false + return } if !endpointFound0 { c.logf("magicsock: lazy endpoint created via disco message for %v, %v", peerNode.Key.ShortString(), sender.ShortString()) @@ -1865,7 +1884,7 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) bool { c.logf("magicsock: disco: failed to open naclbox from %v (wrong rcpt?)", sender) } // TODO(bradfitz): add some counter for this that logs rarely - return false + return } dm, err := disco.Parse(payload) @@ -1879,7 +1898,7 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) bool { // understand. Not even worth logging about, lest it // be too spammy for old clients. // TODO(bradfitz): add some counter for this that logs rarely - return true + return } switch dm := dm.(type) { @@ -1887,14 +1906,14 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) bool { c.handlePingLocked(dm, de, src, sender, peerNode) case *disco.Pong: if de == nil { - return true + return } de.handlePongConnLocked(dm, src) case *disco.CallMeMaybe: if src.IP != derpMagicIPAddr { // CallMeMaybe messages should only come via DERP. c.logf("[unexpected] CallMeMaybe packets should only come via DERP") - return true + return } if de != nil { c.logf("magicsock: disco: %v<-%v (%v, %v) got call-me-maybe, %d endpoints", @@ -1904,8 +1923,7 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) bool { go de.handleCallMeMaybe(dm) } } - - return true + return } func (c *Conn) handlePingLocked(dm *disco.Ping, de *discoEndpoint, src netaddr.IPPort, sender tailcfg.DiscoKey, peerNode *tailcfg.Node) { @@ -2082,6 +2100,7 @@ func (c *Conn) SetPrivateKey(privateKey wgkey.Private) error { return nil } c.privateKey = newKey + c.havePrivateKey.Set(!newKey.IsZero()) if oldKey.IsZero() { c.everHadKey = true