diff --git a/cmd/tailscale/depaware.txt b/cmd/tailscale/depaware.txt index e8e189677..e2c266c11 100644 --- a/cmd/tailscale/depaware.txt +++ b/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+ diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index 811052f7d..7528faa96 100644 --- a/cmd/tailscaled/depaware.txt +++ b/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+ diff --git a/types/key/key.go b/types/key/key.go index 8b70fffa6..7928c1437 100644 --- a/types/key/key.go +++ b/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) { diff --git a/wgengine/magicsock/legacy.go b/wgengine/magicsock/legacy.go index 083f313db..b6f784438 100644 --- a/wgengine/magicsock/legacy.go +++ b/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 - // the knowledge provided by the control server. If the - // packet is valid wireguard will call UpdateDst on the - // original endpoint using this addr. - return (*singleEndpoint)(addr) + // We don't know who this peer is. It's possible that it's one of + // our legitimate peers and they've roamed to an address we don't + // know. If this is a handshake packet, we can try to identify the + // peer in question. + 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 -// addressed" in releases of Tailscale that predate discovery -// messages. New peers use discoEndpoint. -type singleEndpoint net.UDPAddr - -func (e *singleEndpoint) ClearSrc() {} -func (e *singleEndpoint) DstIP() net.IP { return (*net.UDPAddr)(e).IP } -func (e *singleEndpoint) SrcIP() net.IP { return nil } -func (e *singleEndpoint) SrcToString() string { return "" } -func (e *singleEndpoint) DstToString() string { return (*net.UDPAddr)(e).String() } -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 mixKey(dst *[blake2s.Size]byte, c *[blake2s.Size]byte, data []byte) { + device.KDF1(dst, c[:], data) } -func (e *singleEndpoint) Addrs() []wgcfg.Endpoint { - return []wgcfg.Endpoint{{ - Host: e.IP.String(), - Port: uint16(e.Port), - }} + +func mixHash(dst *[blake2s.Size]byte, h *[blake2s.Size]byte, data []byte) { + hash, _ := blake2s.New256(nil) + hash.Write(h[:]) + hash.Write(data) + hash.Sum(dst[:0]) + hash.Reset() +} + +func isZero(val []byte) bool { + acc := 1 + for _, b := range val { + acc &= subtle.ConstantTimeByteEq(b, 0) + } + return acc == 1 } diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 7b451a0b7..1cacc827d 100644 --- a/wgengine/magicsock/magicsock.go +++ b/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 - // seem to be falling through to passing the packet to - // wireguard with a garbage singleEndpoint. This feels wrong, - // should we goto Top above? - ep = c.findEndpoint(ipp, addr) + ep = c.findEndpoint(ipp, addr, b[:n]) + if ep == nil { + goto Top + } } 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 } diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index 4aac0cdad..72030e962 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/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