ipn/ipnlocal: restrict exit node DoH server based on ACL'ed packet filter

Don't be a DoH DNS server to peers unless the Tailnet admin has permitted
that peer autogroup:internet access.

Updates #1713

Change-Id: Iec69360d8e4d24d5187c26904b6a75c1dabc8979
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

ipn/ipnlocal/local.go

@@ -100,6 +100,8 @@ type LocalBackend struct {
 
 	filterHash deephash.Sum
 
+	filterAtomic atomic.Value // of *filter.Filter
+
 	// The mutex protects the following elements.
 	mu             sync.Mutex
 	httpTestClient *http.Client // for controlclient. nil by default, used by tests.
@@ -160,9 +162,6 @@ func NewLocalBackend(logf logger.Logf, logid string, store ipn.StateStore, e wge
 
 	osshare.SetFileSharingEnabled(false, logf)
 
-	// Default filter blocks everything and logs nothing, until Start() is called.
-	e.SetFilter(filter.NewAllowNone(logf, &netaddr.IPSet{}))
-
 	ctx, cancel := context.WithCancel(context.Background())
 	portpoll, err := portlist.NewPoller()
 	if err != nil {
@@ -182,6 +181,9 @@ func NewLocalBackend(logf logger.Logf, logid string, store ipn.StateStore, e wge
 		portpoll:       portpoll,
 		gotPortPollRes: make(chan struct{}),
 	}
+	// Default filter blocks everything and logs nothing, until Start() is called.
+	b.setFilter(filter.NewAllowNone(logf, &netaddr.IPSet{}))
+
 	b.statusChanged = sync.NewCond(&b.statusLock)
 	b.e.SetStatusCallback(b.setWgengineStatus)
 
@@ -1011,20 +1013,25 @@ func (b *LocalBackend) updateFilter(netMap *netmap.NetworkMap, prefs *ipn.Prefs)
 
 	if !haveNetmap {
 		b.logf("netmap packet filter: (not ready yet)")
-		b.e.SetFilter(filter.NewAllowNone(b.logf, logNets))
+		b.setFilter(filter.NewAllowNone(b.logf, logNets))
 		return
 	}
 
 	oldFilter := b.e.GetFilter()
 	if shieldsUp {
 		b.logf("netmap packet filter: (shields up)")
-		b.e.SetFilter(filter.NewShieldsUpFilter(localNets, logNets, oldFilter, b.logf))
+		b.setFilter(filter.NewShieldsUpFilter(localNets, logNets, oldFilter, b.logf))
 	} else {
 		b.logf("netmap packet filter: %v filters", len(packetFilter))
-		b.e.SetFilter(filter.New(packetFilter, localNets, logNets, oldFilter, b.logf))
+		b.setFilter(filter.New(packetFilter, localNets, logNets, oldFilter, b.logf))
 	}
 }
 
+func (b *LocalBackend) setFilter(f *filter.Filter) {
+	b.filterAtomic.Store(f)
+	b.e.SetFilter(f)
+}
+
 var removeFromDefaultRoute = []netaddr.IPPrefix{
 	// RFC1918 LAN ranges
 	netaddr.MustParseIPPrefix("192.168.0.0/16"),

ipn/ipnlocal/peerapi.go

@@ -41,6 +41,7 @@ import (
 	"tailscale.com/tailcfg"
 	"tailscale.com/util/clientmetric"
 	"tailscale.com/wgengine"
+	"tailscale.com/wgengine/filter"
 )
 
 var initListenConfig func(*net.ListenConfig, netaddr.IP, *interfaces.State, string) error
@@ -759,13 +760,45 @@ func (h *peerAPIHandler) handleServeMetrics(w http.ResponseWriter, r *http.Reque
 }
 
 func (h *peerAPIHandler) replyToDNSQueries() bool {
-	// TODO(bradfitz): maybe lock this down more? what if we're an
-	// exit node but ACLs don't permit autogroup:internet access
-	// from h.peerNode via this node? peerapi bypasses ACL checks,
-	// so we should do additional checks here; but on what? this
-	// node's UDP port 53? our upstream DNS forwarder IP(s)?
-	// For now just offer DNS to any peer if we're an exit node.
-	return h.isSelf || h.ps.b.OfferingExitNode()
+	if h.isSelf {
+		// If the peer is owned by the same user, just allow it
+		// without further checks.
+		return true
+	}
+	b := h.ps.b
+	if !b.OfferingExitNode() {
+		// If we're not an exit node, there's no point to
+		// being a DNS server for somebody.
+		return false
+	}
+	if !h.remoteAddr.IsValid() {
+		// This should never be the case if the peerAPIHandler
+		// was wired up correctly, but just in case.
+		return false
+	}
+	// Otherwise, we're an exit node but the peer is not us, so
+	// we need to check if they're allowed access to the internet.
+	// As peerapi bypasses wgengine/filter checks, we need to check
+	// ourselves. As a proxy for autogroup:internet access, we see
+	// if we would've accepted a packet to 0.0.0.0:53. We treat
+	// the IP 0.0.0.0 as being "the internet".
+	f, ok := b.filterAtomic.Load().(*filter.Filter)
+	if !ok {
+		return false
+	}
+	// Note: we check TCP here because the Filter type already had
+	// a CheckTCP method (for unit tests), but it's pretty
+	// arbitrary. DNS runs over TCP and UDP, so sure... we check
+	// TCP.
+	dstIP := netaddr.IPv4(0, 0, 0, 0)
+	remoteIP := h.remoteAddr.IP()
+	if remoteIP.Is6() {
+		// autogroup:internet for IPv6 is defined to start with 2000::/3,
+		// so use 2000::0 as the probe "the internet" address.
+		dstIP = netaddr.MustParseIP("2000::")
+	}
+	verdict := f.CheckTCP(remoteIP, dstIP, 53)
+	return verdict == filter.Accept
 }
 
 // handleDNSQuery implements a DoH server (RFC 8484) over the peerapi.

ipn/ipnlocal/peerapi_test.go

@@ -19,8 +19,13 @@ import (
 	"strings"
 	"testing"
 
+	"inet.af/netaddr"
+	"tailscale.com/ipn"
 	"tailscale.com/tailcfg"
 	"tailscale.com/tstest"
+	"tailscale.com/types/logger"
+	"tailscale.com/wgengine"
+	"tailscale.com/wgengine/filter"
 )
 
 type peerAPITestEnv struct {
@@ -568,3 +573,55 @@ func TestDeletedMarkers(t *testing.T) {
 	}
 
 }
+
+func TestPeerAPIReplyToDNSQueries(t *testing.T) {
+	var h peerAPIHandler
+
+	h.isSelf = true
+	if !h.replyToDNSQueries() {
+		t.Errorf("for isSelf = false; want true")
+	}
+	h.isSelf = false
+	h.remoteAddr = netaddr.MustParseIPPort("100.150.151.152:12345")
+
+	eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0)
+	h.ps = &peerAPIServer{
+		b: &LocalBackend{
+			e: eng,
+		},
+	}
+	if h.ps.b.OfferingExitNode() {
+		t.Fatal("unexpectedly offering exit node")
+	}
+	h.ps.b.prefs = &ipn.Prefs{
+		AdvertiseRoutes: []netaddr.IPPrefix{
+			netaddr.MustParseIPPrefix("0.0.0.0/0"),
+			netaddr.MustParseIPPrefix("::/0"),
+		},
+	}
+	if !h.ps.b.OfferingExitNode() {
+		t.Fatal("unexpectedly not offering exit node")
+	}
+
+	if h.replyToDNSQueries() {
+		t.Errorf("unexpectedly doing DNS without filter")
+	}
+
+	h.ps.b.setFilter(filter.NewAllowNone(logger.Discard, new(netaddr.IPSet)))
+	if h.replyToDNSQueries() {
+		t.Errorf("unexpectedly doing DNS without filter")
+	}
+
+	f := filter.NewAllowAllForTest(logger.Discard)
+
+	h.ps.b.setFilter(f)
+	if !h.replyToDNSQueries() {
+		t.Errorf("unexpectedly deny; wanted to be a DNS server")
+	}
+
+	// Also test IPv6.
+	h.remoteAddr = netaddr.MustParseIPPort("[fe70::1]:12345")
+	if !h.replyToDNSQueries() {
+		t.Errorf("unexpectedly IPv6 deny; wanted to be a DNS server")
+	}
+}