fix/security: prevent command injection on windows (#1272)

Author: evict

cmd/src/login_oauth.go

@@ -4,10 +4,12 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"net/url"
 	"os/exec"
 	"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"
@@ -103,20 +105,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 errors.Wrapf(err, "invalid URL: %s", rawURL)
+	}
+	if u.Scheme != "http" && u.Scheme != "https" {
+		return errors.Newf("unsupported URL scheme %q: only http and https are allowed", u.Scheme)
+	}
+	if u.Host == "" {
+		return errors.New("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,OpenURL", rawURL)
 	default:
-		cmd = exec.Command("xdg-open", url)
+		cmd = exec.Command("xdg-open", rawURL)
 	}
 	return cmd.Run()
 }

cmd/src/login_test.go

@@ -204,6 +204,72 @@ 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)
+			}
+		})
+	}
+}
+
+// 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,<script>alert(1)</script>"},
+		// 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()