wgengine/magicsock: stop depending on UpdateDst in legacy codepaths.

This makes connectivity between ancient and new tailscale nodes slightly
worse in some cases, but only in cases where the ancient version would
likely have failed to get connectivity anyway.

Signed-off-by: David Anderson <danderson@tailscale.com>

cmd/tailscale/depaware.txt

@@ -90,7 +90,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep
         tailscale.com/wgengine/tstun                                 from tailscale.com/wgengine
    W 💣 tailscale.com/wgengine/winnet                                from tailscale.com/wgengine/router
         golang.org/x/crypto/blake2b                                  from golang.org/x/crypto/nacl/box
-        golang.org/x/crypto/blake2s                                  from github.com/tailscale/wireguard-go/device
+        golang.org/x/crypto/blake2s                                  from github.com/tailscale/wireguard-go/device+
         golang.org/x/crypto/chacha20                                 from golang.org/x/crypto/chacha20poly1305
         golang.org/x/crypto/chacha20poly1305                         from crypto/tls+
         golang.org/x/crypto/cryptobyte                               from crypto/ecdsa+

cmd/tailscaled/depaware.txt

@@ -131,7 +131,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de
         tailscale.com/wgengine/tstun                                 from tailscale.com/wgengine+
    W 💣 tailscale.com/wgengine/winnet                                from tailscale.com/wgengine/router
         golang.org/x/crypto/blake2b                                  from golang.org/x/crypto/nacl/box
-        golang.org/x/crypto/blake2s                                  from github.com/tailscale/wireguard-go/device
+        golang.org/x/crypto/blake2s                                  from github.com/tailscale/wireguard-go/device+
         golang.org/x/crypto/chacha20                                 from golang.org/x/crypto/chacha20poly1305
         golang.org/x/crypto/chacha20poly1305                         from crypto/tls+
         golang.org/x/crypto/cryptobyte                               from crypto/ecdsa+

types/key/key.go

@@ -82,6 +82,15 @@ func (k Private) Public() Public {
 	return Public(pub)
 }
 
+func (k Private) SharedSecret(pub Public) (ss [32]byte) {
+	apk := (*[32]byte)(&pub)
+	ask := (*[32]byte)(&k)
+	//lint:ignore SA1019 Code copied from wireguard-go, we aim for
+	//minimal changes from it.
+	curve25519.ScalarMult(&ss, ask, apk)
+	return ss
+}
+
 // NewPublicFromHexMem parses a public key in its hex form, given in m.
 // The provided m must be exactly 64 bytes in length.
 func NewPublicFromHexMem(m mem.RO) (Public, error) {

wgengine/magicsock/legacy.go

@@ -5,6 +5,8 @@
 package magicsock
 
 import (
+	"bytes"
+	"crypto/subtle"
 	"encoding/binary"
 	"errors"
 	"fmt"
@@ -16,6 +18,8 @@ import (
 	"github.com/tailscale/wireguard-go/conn"
 	"github.com/tailscale/wireguard-go/device"
 	"github.com/tailscale/wireguard-go/wgcfg"
+	"golang.org/x/crypto/blake2s"
+	"golang.org/x/crypto/chacha20poly1305"
 	"inet.af/netaddr"
 	"tailscale.com/ipn/ipnstate"
 	"tailscale.com/types/key"
@@ -70,17 +74,58 @@ func (c *Conn) createLegacyEndpointLocked(pk key.Public, addrs string) (conn.End
 	return a, nil
 }
 
-func (c *Conn) findLegacyEndpointLocked(ipp netaddr.IPPort, addr *net.UDPAddr) conn.Endpoint {
+func (c *Conn) findLegacyEndpointLocked(ipp netaddr.IPPort, addr *net.UDPAddr, packet []byte) conn.Endpoint {
 	// Pre-disco: look up their addrSet.
 	if as, ok := c.addrsByUDP[ipp]; ok {
+		as.updateDst(addr)
 		return as
 	}
 
-	// Pre-disco: the peer that sent this packet has roamed beyond
+	// We don't know who this peer is. It's possible that it's one of
-	// the knowledge provided by the control server.  If the
+	// our legitimate peers and they've roamed to an address we don't
-	// packet is valid wireguard will call UpdateDst on the
+	// know. If this is a handshake packet, we can try to identify the
-	// original endpoint using this addr.
+	// peer in question.
-	return (*singleEndpoint)(addr)
+	if as := c.peerFromPacketLocked(packet); as != nil {
+		as.updateDst(addr)
+		return as
+	}
+
+	// We have no idea who this is, drop the packet.
+	//
+	// In the past, when this magicsock implementation was the main
+	// one, we tried harder to find a match here: we would pass the
+	// packet into wireguard-go with a "singleEndpoint" implementation
+	// that wrapped the UDPAddr. Then, a patch we added to
+	// wireguard-go would call UpdateDst on that singleEndpoint after
+	// decrypting the packet and identifying the peer (if any),
+	// allowing us to update the relevant addrSet.
+	//
+	// This was a significant out of tree patch to wireguard-go, so we
+	// got rid of it, and instead switched to this logic you're
+	// reading now, which makes a best effort to identify sources for
+	// handshake packets (because they're relatively easy to turn into
+	// a peer public key statelessly), but otherwise drops packets
+	// that come from "roaming" addresses that aren't known to
+	// magicsock.
+	//
+	// The practical consequence of this is that some complex NAT
+	// traversal cases will now fail between a very old Tailscale
+	// client (0.96 and earlier) and a very new Tailscale
+	// client. However, those scenarios were likely also failing on
+	// all-old clients, because the probabilistic NAT opening didn't
+	// work reliably. So, in practice, this simplification means
+	// connectivity looks like this:
+	//
+	//  - old+old client: unchanged
+	//  - old+new client (easy network topology): unchanged
+	//  - old+new client (hard network topology): was bad, now a bit worse
+	//  - new+new client: unchanged
+	//
+	// This degradation is acceptable in that it continue to support
+	// the incremental upgrade of old clients that currently work
+	// well, which is our primary goal for the <100 clients still left
+	// on the oldest pre-DERP versions (as of 2021-01-12).
+	return nil
 }
 
 func (c *Conn) resetAddrSetStatesLocked() {
@@ -90,16 +135,6 @@ func (c *Conn) resetAddrSetStatesLocked() {
 	}
 }
 
-func (c *Conn) sendSingleEndpoint(b []byte, se *singleEndpoint) error {
-	addr := (*net.UDPAddr)(se)
-	if addr.IP.Equal(derpMagicIP) {
-		c.logf("magicsock: [unexpected] DERP BUG: attempting to send packet to DERP address %v", addr)
-		return nil
-	}
-	_, err := c.sendUDPStd(addr, b)
-	return err
-}
-
 func (c *Conn) sendAddrSet(b []byte, as *addrSet) error {
 	var addrBuf [8]netaddr.IPPort
 	dsts, roamAddr := as.appendDests(addrBuf[:0], b)
@@ -129,6 +164,57 @@ func (c *Conn) sendAddrSet(b []byte, as *addrSet) error {
 	return ret
 }
 
+// peerFromPacketLocked extracts returns the addrSet for the peer who sent
+// packet, if derivable.
+func (c *Conn) peerFromPacketLocked(packet []byte) *addrSet {
+	if len(packet) < 4 {
+		return nil
+	}
+	msgType := binary.LittleEndian.Uint32(packet[:4])
+	if msgType != device.MessageInitiationType {
+		// Can't get peer out of a non-handshake packet.
+		return nil
+	}
+
+	var msg device.MessageInitiation
+	reader := bytes.NewReader(packet)
+	err := binary.Read(reader, binary.LittleEndian, &msg)
+	if err != nil {
+		return nil
+	}
+
+	// Process just enough of the handshake to extract the long-term
+	// peer public key. We don't verify the handshake all the way, so
+	// this may be a spoofed packet. The extracted peer MUST NOT be
+	// used for any security critical function. In our case, we use it
+	// as a hint for roaming addresses.
+	var (
+		pub      = c.privateKey.Public()
+		hash     [blake2s.Size]byte
+		chainKey [blake2s.Size]byte
+		peerPK   key.Public
+		boxKey   [chacha20poly1305.KeySize]byte
+	)
+
+	mixHash(&hash, &device.InitialHash, pub[:])
+	mixHash(&hash, &hash, msg.Ephemeral[:])
+	mixKey(&chainKey, &device.InitialChainKey, msg.Ephemeral[:])
+
+	ss := c.privateKey.SharedSecret(key.Public(msg.Ephemeral))
+	if isZero(ss[:]) {
+		return nil
+	}
+
+	device.KDF2(&chainKey, &boxKey, chainKey[:], ss[:])
+	aead, _ := chacha20poly1305.New(boxKey[:])
+	_, err = aead.Open(peerPK[:0], device.ZeroNonce[:], msg.Static[:], hash[:])
+	if err != nil {
+		return nil
+	}
+
+	return c.addrsByKey[peerPK]
+}
+
 func shouldSprayPacket(b []byte) bool {
 	if len(b) < 4 {
 		return false
@@ -325,19 +411,6 @@ func (a *addrSet) dst() netaddr.IPPort {
 	return a.ipPorts[i]
 }
 
-// packUDPAddr packs a UDPAddr in the form wanted by WireGuard.
-func packUDPAddr(ua *net.UDPAddr) []byte {
-	ip := ua.IP.To4()
-	if ip == nil {
-		ip = ua.IP
-	}
-	b := make([]byte, 0, len(ip)+2)
-	b = append(b, ip...)
-	b = append(b, byte(ua.Port))
-	b = append(b, byte(ua.Port>>8))
-	return b
-}
-
 func (a *addrSet) DstToBytes() []byte {
 	return packIPPort(a.dst())
 }
@@ -353,6 +426,12 @@ func (a *addrSet) SrcToString() string { return "" }
 func (a *addrSet) ClearSrc()           {}
 
 func (a *addrSet) UpdateDst(new *net.UDPAddr) error {
+	return nil
+}
+
+// updateDst records receipt of a packet from new. This is used to
+// potentially update the transmit address used for this addrSet.
+func (a *addrSet) updateDst(new *net.UDPAddr) error {
 	if new.IP.Equal(derpMagicIP) {
 		// Never consider DERP addresses as a viable candidate for
 		// either curAddr or roamAddr. It's only ever a last resort
@@ -500,23 +579,22 @@ func (a *addrSet) Addrs() []wgcfg.Endpoint {
 	return eps
 }
 
-// singleEndpoint is a wireguard-go/conn.Endpoint used for "roaming
+func mixKey(dst *[blake2s.Size]byte, c *[blake2s.Size]byte, data []byte) {
-// addressed" in releases of Tailscale that predate discovery
+	device.KDF1(dst, c[:], data)
-// messages. New peers use discoEndpoint.
+}
-type singleEndpoint net.UDPAddr
+
-
+func mixHash(dst *[blake2s.Size]byte, h *[blake2s.Size]byte, data []byte) {
-func (e *singleEndpoint) ClearSrc()           {}
+	hash, _ := blake2s.New256(nil)
-func (e *singleEndpoint) DstIP() net.IP       { return (*net.UDPAddr)(e).IP }
+	hash.Write(h[:])
-func (e *singleEndpoint) SrcIP() net.IP       { return nil }
+	hash.Write(data)
-func (e *singleEndpoint) SrcToString() string { return "" }
+	hash.Sum(dst[:0])
-func (e *singleEndpoint) DstToString() string { return (*net.UDPAddr)(e).String() }
+	hash.Reset()
-func (e *singleEndpoint) DstToBytes() []byte  { return packUDPAddr((*net.UDPAddr)(e)) }
+}
-func (e *singleEndpoint) UpdateDst(dst *net.UDPAddr) error {
+
-	return fmt.Errorf("magicsock.singleEndpoint(%s).UpdateDst(%s): should never be called", (*net.UDPAddr)(e), dst)
+func isZero(val []byte) bool {
-}
+	acc := 1
-func (e *singleEndpoint) Addrs() []wgcfg.Endpoint {
+	for _, b := range val {
-	return []wgcfg.Endpoint{{
+		acc &= subtle.ConstantTimeByteEq(b, 0)
-		Host: e.IP.String(),
+	}
-		Port: uint16(e.Port),
+	return acc == 1
-	}}
 }

wgengine/magicsock/magicsock.go

@@ -1008,8 +1008,6 @@ func (c *Conn) Send(b []byte, ep conn.Endpoint) error {
 		panic(fmt.Sprintf("[unexpected] Endpoint type %T", v))
 	case *discoEndpoint:
 		return v.send(b)
-	case *singleEndpoint:
-		return c.sendSingleEndpoint(b, v)
 	case *addrSet:
 		return c.sendAddrSet(b, v)
 	}
@@ -1418,7 +1416,7 @@ func (c *Conn) runDerpWriter(ctx context.Context, dc *derphttp.Client, ch <-chan
 // Endpoint to find the UDPAddr to return to wireguard anyway, so no
 // benefit unless we can, say, always return the same fake UDPAddr for
 // all packets.
-func (c *Conn) findEndpoint(ipp netaddr.IPPort, addr *net.UDPAddr) conn.Endpoint {
+func (c *Conn) findEndpoint(ipp netaddr.IPPort, addr *net.UDPAddr, packet []byte) conn.Endpoint {
 	c.mu.Lock()
 	defer c.mu.Unlock()
 
@@ -1430,7 +1428,7 @@ func (c *Conn) findEndpoint(ipp netaddr.IPPort, addr *net.UDPAddr) conn.Endpoint
 		}
 	}
 
-	return c.findLegacyEndpointLocked(ipp, addr)
+	return c.findLegacyEndpointLocked(ipp, addr, packet)
 }
 
 type udpReadResult struct {
@@ -1513,7 +1511,10 @@ Top:
 	if from := c.bufferedIPv4From; from != (netaddr.IPPort{}) {
 		c.bufferedIPv4From = netaddr.IPPort{}
 		addr = from.UDPAddr()
-		ep := c.findEndpoint(from, addr)
+		ep := c.findEndpoint(from, addr, c.bufferedIPv4Packet)
+		if ep == nil {
+			goto Top
+		}
 		c.noteRecvActivityFromEndpoint(ep)
 		return copy(b, c.bufferedIPv4Packet), ep, wgRecvAddr(ep, from, addr), nil
 	}
@@ -1600,11 +1601,10 @@ Top:
 		} else {
 			key := wgkey.Key(dm.src)
 			c.logf("magicsock: DERP packet from unknown key: %s", key.ShortString())
-			// TODO(danderson): after we fail to find a DERP endpoint, we
+			ep = c.findEndpoint(ipp, addr, b[:n])
-			// seem to be falling through to passing the packet to
+			if ep == nil {
-			// wireguard with a garbage singleEndpoint. This feels wrong,
+				goto Top
-			// should we goto Top above?
+			}
-			ep = c.findEndpoint(ipp, addr)
 		}
 
 		if !didNoteRecvActivity {
@@ -1617,7 +1617,10 @@ Top:
 			return 0, nil, nil, err
 		}
 		n, addr, ipp = um.n, um.addr, um.ipp
-		ep = c.findEndpoint(ipp, addr)
+		ep = c.findEndpoint(ipp, addr, b[:n])
+		if ep == nil {
+			goto Top
+		}
 		c.noteRecvActivityFromEndpoint(ep)
 		return n, ep, wgRecvAddr(ep, ipp, addr), nil
 
@@ -1658,7 +1661,10 @@ func (c *Conn) ReceiveIPv6(b []byte) (int, conn.Endpoint, *net.UDPAddr, error) {
 			continue
 		}
 
-		ep := c.findEndpoint(ipp, addr)
+		ep := c.findEndpoint(ipp, addr, b[:n])
+		if ep == nil {
+			continue
+		}
 		c.noteRecvActivityFromEndpoint(ep)
 		return n, ep, wgRecvAddr(ep, ipp, addr), nil
 	}

wgengine/magicsock/magicsock_test.go

@@ -1216,7 +1216,7 @@ func testTwoDevicePing(t *testing.T, d *devices) {
 	})
 }
 
-// TestAddrSet tests addrSet appendDests and UpdateDst.
+// TestAddrSet tests addrSet appendDests and updateDst.
 func TestAddrSet(t *testing.T) {
 	tstest.PanicOnLog()
 	rc := tstest.NewResourceCheck()
@@ -1378,7 +1378,7 @@ func TestAddrSet(t *testing.T) {
 				faket = faket.Add(st.advance)
 
 				if st.updateDst != nil {
-					if err := tt.as.UpdateDst(st.updateDst); err != nil {
+					if err := tt.as.updateDst(st.updateDst); err != nil {
 						t.Fatal(err)
 					}
 					continue