From decd9893e48bf8aba31055088f44527c6d871802 Mon Sep 17 00:00:00 2001 From: Andrew Lytvynov Date: Wed, 13 Mar 2024 17:31:07 -0700 Subject: [PATCH] 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 --- ipn/ipnlocal/local.go | 12 +++++++++++- ipn/ipnlocal/local_test.go | 38 ++++++++++++++++++++++++++++++++++++++ ipn/ipnlocal/state_test.go | 31 ++++++++++++++++++++++--------- 3 files changed, 71 insertions(+), 10 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 818131c16..8a5736b6f 100644 --- a/ipn/ipnlocal/local.go +++ b/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://") diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index d37020ce4..6f840a898 100644 --- a/ipn/ipnlocal/local_test.go +++ b/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) + } + }) + } +} diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index 403233a11..88d449916 100644 --- a/ipn/ipnlocal/state_test.go +++ b/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