From 685b853763b6ed00ab1ef103251e62c739815525 Mon Sep 17 00:00:00 2001
From: Jordan Whited <jordan@tailscale.com>
Date: Thu, 14 Dec 2023 12:54:19 -0800
Subject: [PATCH] wgengine/magicsock: fix handling of derp.PeerGoneMessage
 (#10589)

The switch in Conn.runDerpReader() on the derp.ReceivedMessage type
contained cases other than derp.ReceivedPacket that fell through to
writing to c.derpRecvCh, which should only be reached for
derp.ReceivedPacket. This can result in the last/previous
derp.ReceivedPacket to be re-handled, effectively creating a duplicate
packet. If the last derp.ReceivedPacket happens to be a
disco.CallMeMaybe it may result in a disco ping scan towards the
originating peer on the endpoints contained.

The change in this commit moves the channel write on c.derpRecvCh and
subsequent select awaiting the result into the derp.ReceivedMessage
case, preventing it from being reached from any other case. Explicit
continue statements are also added to non-derp.ReceivedPacket cases
where they were missing, in order to signal intent to the reader.

Fixes #10586

Signed-off-by: Jordan Whited <jordan@tailscale.com>
---
 wgengine/magicsock/derp.go | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/wgengine/magicsock/derp.go b/wgengine/magicsock/derp.go
index 2a9f1cae3..e062c913f 100644
--- a/wgengine/magicsock/derp.go
+++ b/wgengine/magicsock/derp.go
@@ -424,8 +424,8 @@ func (c *Conn) setPeerLastDerpLocked(peer key.NodePublic, regionID, homeID int)
 	}
 }
 
-// derpReadResult is the type sent by runDerpClient to ReceiveIPv4
-// when a DERP packet is available.
+// derpReadResult is the type sent by Conn.runDerpReader to connBind.receiveDERP
+// when a derp.ReceivedPacket is available.
 //
 // Notably, it doesn't include the derp.ReceivedPacket because we
 // don't want to give the receiver access to the aliased []byte.  To
@@ -542,6 +542,17 @@ func (c *Conn) runDerpReader(ctx context.Context, derpFakeAddr netip.AddrPort, d
 					c.addDerpPeerRoute(res.src, regionID, dc)
 				}
 			}
+			select {
+			case <-ctx.Done():
+				return
+			case c.derpRecvCh <- res:
+			}
+			select {
+			case <-ctx.Done():
+				return
+			case <-didCopy:
+				continue
+			}
 		case derp.PingMessage:
 			// Best effort reply to the ping.
 			pingData := [8]byte(m)
@@ -553,6 +564,7 @@ func (c *Conn) runDerpReader(ctx context.Context, derpFakeAddr netip.AddrPort, d
 			continue
 		case derp.HealthMessage:
 			health.SetDERPRegionHealth(regionID, m.Problem)
+			continue
 		case derp.PeerGoneMessage:
 			switch m.Reason {
 			case derp.PeerGoneReasonDisconnected:
@@ -567,23 +579,11 @@ func (c *Conn) runDerpReader(ctx context.Context, derpFakeAddr netip.AddrPort, d
 					regionID, key.NodePublic(m.Peer).ShortString(), m.Reason)
 			}
 			c.removeDerpPeerRoute(key.NodePublic(m.Peer), regionID, dc)
+			continue
 		default:
 			// Ignore.
 			continue
 		}
-
-		select {
-		case <-ctx.Done():
-			return
-		case c.derpRecvCh <- res:
-		}
-
-		select {
-		case <-ctx.Done():
-			return
-		case <-didCopy:
-			continue
-		}
 	}
 }