ipn/ipnlocal: validate domain of PopBrowserURL on default control URL (#11394)

If the client uses the default Tailscale control URL, validate that all
PopBrowserURLs are under tailscale.com or *.tailscale.com. This reduces
the risk of a compromised control plane opening phishing pages for
example.

The client trusts control for many other things, but this is one easy
way to reduce that trust a bit.

Fixes #11393

Signed-off-by: Andrew Lytvynov <awly@tailscale.com>

ipn/ipnlocal/local.go

@@ -2503,11 +2503,21 @@ func (b *LocalBackend) validPopBrowserURL(urlStr string) bool {
 	if err != nil {
 		return false
 	}
+	serverURL := b.Prefs().ControlURLOrDefault()
+	if ipn.IsLoginServerSynonym(serverURL) {
+		// When connected to the official Tailscale control plane, only allow
+		// URLs from tailscale.com or its subdomains.
+		if h := u.Hostname(); h != "tailscale.com" && !strings.HasSuffix(u.Hostname(), ".tailscale.com") {
+			return false
+		}
+		// When using a different ControlURL, we cannot be sure what legitimate
+		// PopBrowserURLs they will send. Allow any domain there to avoid
+		// breaking existing user setups.
+	}
 	switch u.Scheme {
 	case "https":
 		return true
 	case "http":
-		serverURL := b.Prefs().ControlURLOrDefault()
 		// If the control server is using plain HTTP (likely a dev server),
 		// then permit http://.
 		return strings.HasPrefix(serverURL, "http://")

ipn/ipnlocal/local_test.go

@@ -2469,3 +2469,41 @@ func TestTailFSManageShares(t *testing.T) {
 		})
 	}
 }
+
+func TestValidPopBrowserURL(t *testing.T) {
+	b := newTestBackend(t)
+	tests := []struct {
+		desc          string
+		controlURL    string
+		popBrowserURL string
+		want          bool
+	}{
+		{"saas_login", "https://login.tailscale.com", "https://login.tailscale.com/a/foo", true},
+		{"saas_controlplane", "https://controlplane.tailscale.com", "https://controlplane.tailscale.com/a/foo", true},
+		{"saas_root", "https://login.tailscale.com", "https://tailscale.com/", true},
+		{"saas_bad_hostname", "https://login.tailscale.com", "https://example.com/a/foo", false},
+		{"localhost", "http://localhost", "http://localhost/a/foo", true},
+		{"custom_control_url_https", "https://example.com", "https://example.com/a/foo", true},
+		{"custom_control_url_https_diff_domain", "https://example.com", "https://other.com/a/foo", true},
+		{"custom_control_url_http", "http://example.com", "http://example.com/a/foo", true},
+		{"custom_control_url_http_diff_domain", "http://example.com", "http://other.com/a/foo", true},
+		{"bad_scheme", "https://example.com", "http://example.com/a/foo", false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			if _, err := b.EditPrefs(&ipn.MaskedPrefs{
+				ControlURLSet: true,
+				Prefs: ipn.Prefs{
+					ControlURL: tt.controlURL,
+				},
+			}); err != nil {
+				t.Fatal(err)
+			}
+
+			got := b.validPopBrowserURL(tt.popBrowserURL)
+			if got != tt.want {
+				t.Errorf("got %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

ipn/ipnlocal/state_test.go

@@ -400,7 +400,14 @@ func TestStateMachine(t *testing.T) {
 	// Attempted non-interactive login with no key; indicate that
 	// the user needs to visit a login URL.
 	t.Logf("\n\nLogin (url response)")
-	notifies.expect(1)
+
+	notifies.expect(2)
+	b.EditPrefs(&ipn.MaskedPrefs{
+		ControlURLSet: true,
+		Prefs: ipn.Prefs{
+			ControlURL: "https://localhost:1/",
+		},
+	})
 	url1 := "https://localhost:1/1"
 	cc.send(nil, url1, false, nil)
 	{
@@ -409,11 +416,11 @@ func TestStateMachine(t *testing.T) {
 		// ...but backend eats that notification, because the user
 		// didn't explicitly request interactive login yet, and
 		// we're already in NeedsLogin state.
-		nn := notifies.drain(1)
+		nn := notifies.drain(2)
 
-		c.Assert(nn[0].Prefs, qt.IsNotNil)
-		c.Assert(nn[0].Prefs.LoggedOut(), qt.IsTrue)
-		c.Assert(nn[0].Prefs.WantRunning(), qt.IsFalse)
+		c.Assert(nn[1].Prefs, qt.IsNotNil)
+		c.Assert(nn[1].Prefs.LoggedOut(), qt.IsTrue)
+		c.Assert(nn[1].Prefs.WantRunning(), qt.IsFalse)
 		c.Assert(ipn.NeedsLogin, qt.Equals, b.State())
 	}
 
@@ -775,12 +782,18 @@ func TestStateMachine(t *testing.T) {
 	// We want to try logging in as a different user, while Stopped.
 	// First, start the login process (without logging out first).
 	t.Logf("\n\nLoginDifferent")
-	notifies.expect(1)
+	notifies.expect(2)
+	b.EditPrefs(&ipn.MaskedPrefs{
+		ControlURLSet: true,
+		Prefs: ipn.Prefs{
+			ControlURL: "https://localhost:1/",
+		},
+	})
 	b.StartLoginInteractive()
 	url3 := "https://localhost:1/3"
 	cc.send(nil, url3, false, nil)
 	{
-		nn := notifies.drain(1)
+		nn := notifies.drain(2)
 		// It might seem like WantRunning should switch to true here,
 		// but that would be risky since we already have a valid
 		// user account. It might try to reconnect to the old account
@@ -789,8 +802,8 @@ func TestStateMachine(t *testing.T) {
 		// Because the login hasn't yet completed, the old login
 		// is still valid, so it's correct that we stay paused.
 		cc.assertCalls("Login")
-		c.Assert(nn[0].BrowseToURL, qt.IsNotNil)
-		c.Assert(*nn[0].BrowseToURL, qt.Equals, url3)
+		c.Assert(nn[1].BrowseToURL, qt.IsNotNil)
+		c.Assert(*nn[1].BrowseToURL, qt.Equals, url3)
 	}
 
 	// Now, let's complete the interactive login, using a different