From f6f6a5611d60b60e072551dd8ff81822333b4a28 Mon Sep 17 00:00:00 2001 From: Vincent Date: Tue, 10 Mar 2026 13:31:28 +0000 Subject: [PATCH 1/4] harden security --- cmd/src/login_oauth.go | 32 ++++++++++++++++++++++++++------ cmd/src/login_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/cmd/src/login_oauth.go b/cmd/src/login_oauth.go index 95cf01a5bd..9d3d078112 100644 --- a/cmd/src/login_oauth.go +++ b/cmd/src/login_oauth.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "net/url" "os/exec" "runtime" "time" @@ -103,20 +104,39 @@ func runOAuthDeviceFlow(ctx context.Context, endpoint string, out io.Writer, cli return token, nil } -func openInBrowser(url string) error { - if url == "" { +// validateBrowserURL checks that rawURL is a valid HTTP(S) URL to prevent +// command injection via malicious URLs returned by an OAuth server. +func validateBrowserURL(rawURL string) error { + u, err := url.Parse(rawURL) + if err != nil { + return fmt.Errorf("invalid URL: %w", err) + } + if u.Scheme != "http" && u.Scheme != "https" { + return fmt.Errorf("unsupported URL scheme %q: only http and https are allowed", u.Scheme) + } + if u.Host == "" { + return fmt.Errorf("URL has no host") + } + return nil +} + +func openInBrowser(rawURL string) error { + if rawURL == "" { return nil } + if err := validateBrowserURL(rawURL); err != nil { + return err + } + var cmd *exec.Cmd switch runtime.GOOS { case "darwin": - cmd = exec.Command("open", url) + cmd = exec.Command("open", rawURL) case "windows": - // "start" is a cmd.exe built-in; the empty string is the window title. - cmd = exec.Command("cmd", "/c", "start", "", url) + cmd = exec.Command("rundll32", "url.dll,FileProtocolHandler", rawURL) default: - cmd = exec.Command("xdg-open", url) + cmd = exec.Command("xdg-open", rawURL) } return cmd.Run() } diff --git a/cmd/src/login_test.go b/cmd/src/login_test.go index e909ff7310..4fdda96f7b 100644 --- a/cmd/src/login_test.go +++ b/cmd/src/login_test.go @@ -204,6 +204,32 @@ func TestSelectLoginFlow(t *testing.T) { }) } +func TestValidateBrowserURL(t *testing.T) { + tests := []struct { + name string + url string + wantErr bool + }{ + {name: "valid https", url: "https://example.com/device?code=ABC", wantErr: false}, + {name: "valid http", url: "http://localhost:3080/auth", wantErr: false}, + {name: "command injection ampersand", url: "https://example.com & calc.exe", wantErr: true}, + {name: "command injection pipe", url: "https://x | powershell -enc ZABp", wantErr: true}, + {name: "file scheme", url: "file:///etc/passwd", wantErr: true}, + {name: "javascript scheme", url: "javascript:alert(1)", wantErr: true}, + {name: "empty scheme", url: "://no-scheme", wantErr: true}, + {name: "no host", url: "https://", wantErr: true}, + {name: "relative path", url: "/just/a/path", wantErr: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateBrowserURL(tt.url) + if (err != nil) != tt.wantErr { + t.Errorf("validateBrowserURL(%q) error = %v, wantErr %v", tt.url, err, tt.wantErr) + } + }) + } +} + func restoreStoredOAuthLoader(t *testing.T, loader func(context.Context, string) (*oauth.Token, error)) { t.Helper() From 987ba5c3eb7530d33cfc3819553e7f2c7c7e044f Mon Sep 17 00:00:00 2001 From: Vincent Date: Tue, 10 Mar 2026 13:32:05 +0000 Subject: [PATCH 2/4] use OpenURL --- cmd/src/login_oauth.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/src/login_oauth.go b/cmd/src/login_oauth.go index 9d3d078112..7d5df0c26a 100644 --- a/cmd/src/login_oauth.go +++ b/cmd/src/login_oauth.go @@ -134,7 +134,7 @@ func openInBrowser(rawURL string) error { case "darwin": cmd = exec.Command("open", rawURL) case "windows": - cmd = exec.Command("rundll32", "url.dll,FileProtocolHandler", rawURL) + cmd = exec.Command("rundll32", "url.dll,OpenURL", rawURL) default: cmd = exec.Command("xdg-open", rawURL) } From 11fb29d497969626cf15d00f3b4e9da58556b7c3 Mon Sep 17 00:00:00 2001 From: Vincent Date: Tue, 10 Mar 2026 14:32:50 +0000 Subject: [PATCH 3/4] use correct lib --- cmd/src/login_oauth.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/cmd/src/login_oauth.go b/cmd/src/login_oauth.go index 7d5df0c26a..e0b88ce453 100644 --- a/cmd/src/login_oauth.go +++ b/cmd/src/login_oauth.go @@ -9,6 +9,7 @@ import ( "runtime" "time" + "github.com/sourcegraph/sourcegraph/lib/errors" "github.com/sourcegraph/src-cli/internal/api" "github.com/sourcegraph/src-cli/internal/cmderrors" "github.com/sourcegraph/src-cli/internal/oauth" @@ -109,13 +110,13 @@ func runOAuthDeviceFlow(ctx context.Context, endpoint string, out io.Writer, cli func validateBrowserURL(rawURL string) error { u, err := url.Parse(rawURL) if err != nil { - return fmt.Errorf("invalid URL: %w", err) + return errors.Wrapf(err, "invalid URL: %s", rawURL) } if u.Scheme != "http" && u.Scheme != "https" { - return fmt.Errorf("unsupported URL scheme %q: only http and https are allowed", u.Scheme) + return errors.Newf("unsupported URL scheme %q: only http and https are allowed", u.Scheme) } if u.Host == "" { - return fmt.Errorf("URL has no host") + return errors.New("URL has no host") } return nil } @@ -139,4 +140,4 @@ func openInBrowser(rawURL string) error { cmd = exec.Command("xdg-open", rawURL) } return cmd.Run() -} +} \ No newline at end of file From 2eb08308ab030f0caa78850deebf180fe6f88639 Mon Sep 17 00:00:00 2001 From: Vincent Date: Tue, 10 Mar 2026 16:24:12 +0000 Subject: [PATCH 4/4] test --- cmd/src/login_oauth.go | 2 +- cmd/src/login_test.go | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/cmd/src/login_oauth.go b/cmd/src/login_oauth.go index e0b88ce453..5074083da8 100644 --- a/cmd/src/login_oauth.go +++ b/cmd/src/login_oauth.go @@ -140,4 +140,4 @@ func openInBrowser(rawURL string) error { cmd = exec.Command("xdg-open", rawURL) } return cmd.Run() -} \ No newline at end of file +} diff --git a/cmd/src/login_test.go b/cmd/src/login_test.go index 4fdda96f7b..6577c19f9e 100644 --- a/cmd/src/login_test.go +++ b/cmd/src/login_test.go @@ -230,6 +230,46 @@ func TestValidateBrowserURL(t *testing.T) { } } +// TestValidateBrowserURL_WindowsRundll32Escape tests that validateBrowserURL blocks +// payloads that could abuse the Windows "rundll32 url.dll,OpenURL" browser opener +// (LOLBAS T1218.011). If any of these cases pass validation, an attacker-controlled +// URL could execute arbitrary files via rundll32. +// Reference: https://lolbas-project.github.io/lolbas/Libraries/Url/ +func TestValidateBrowserURL_WindowsRundll32Escape(t *testing.T) { + tests := []struct { + name string + url string + }{ + // url.dll OpenURL can launch .hta payloads via mshta.exe + {name: "hta via file protocol", url: "file:///C:/Temp/payload.hta"}, + // url.dll OpenURL can launch executables from .url shortcut files + {name: "url shortcut file", url: "file:///C:/Temp/launcher.url"}, + // url.dll OpenURL / FileProtocolHandler can run executables directly + {name: "exe via file protocol", url: "file:///C:/Windows/System32/calc.exe"}, + // Obfuscated file protocol handler variant + {name: "obfuscated file handler", url: "file:///C:/Temp/payload.exe"}, + // UNC path via file scheme to remote payload + {name: "unc path file scheme", url: "file://attacker.com/share/payload.exe"}, + // data: URI could be passed through to a handler + {name: "data uri", url: "data:text/html,"}, + // vbscript scheme + {name: "vbscript scheme", url: "vbscript:Execute(\"MsgBox(1)\")"}, + // about scheme + {name: "about scheme", url: "about:blank"}, + // ms-msdt protocol handler (Follina-style) + {name: "ms-msdt handler", url: "ms-msdt:/id PCWDiagnostic /skip force /param"}, + // search-ms protocol handler + {name: "search-ms handler", url: "search-ms:query=calc&crumb=location:\\\\attacker.com\\share"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := validateBrowserURL(tt.url); err == nil { + t.Errorf("validateBrowserURL(%q) = nil; want error (payload must be blocked to prevent rundll32 url.dll,OpenURL abuse)", tt.url) + } + }) + } +} + func restoreStoredOAuthLoader(t *testing.T, loader func(context.Context, string) (*oauth.Token, error)) { t.Helper()