wgengine/router: look up absolute path to netsh.exe on Windows

This is in response to logs from a customer that show that we're unable
to run netsh due to the following error:

    router: firewall: adding Tailscale-Process rule to allow UDP for "C:\\Program Files\\Tailscale\\tailscaled.exe" ...
    router: firewall: error adding Tailscale-Process rule: exec: "netsh": cannot run executable found relative to current directory:

There's approximately no reason to ever dynamically look up the path of
a system utility like netsh.exe, so instead let's first look for it
in the System32 directory and only if that fails fall back to the
previous behaviour.

Updates #10804

Signed-off-by: Andrew Dunham <andrew@du.nham.ca>
Change-Id: I68cfeb4cab091c79ccff3187d35f50359a690573
This commit is contained in:
Andrew Dunham 2024-01-10 12:37:14 -05:00
parent ca48db0d60
commit 6540d1f018
2 changed files with 61 additions and 1 deletions

View File

@ -12,6 +12,7 @@
"net/netip" "net/netip"
"os" "os"
"os/exec" "os/exec"
"path/filepath"
"slices" "slices"
"strings" "strings"
"sync" "sync"
@ -155,6 +156,13 @@ type firewallTweaker struct {
// stop makes fwProc exit when closed. // stop makes fwProc exit when closed.
fwProcWriter io.WriteCloser fwProcWriter io.WriteCloser
fwProcEncoder *json.Encoder fwProcEncoder *json.Encoder
// The path to the 'netsh.exe' binary, populated during the first call
// to runFirewall.
//
// not protected by mu; netshPath is only mutated inside netshPathOnce
netshPathOnce sync.Once
netshPath string
} }
func (ft *firewallTweaker) clear() { ft.set(nil, nil, nil) } func (ft *firewallTweaker) clear() { ft.set(nil, nil, nil) }
@ -185,10 +193,43 @@ func (ft *firewallTweaker) set(cidrs []string, routes, localRoutes []netip.Prefi
go ft.doAsyncSet() go ft.doAsyncSet()
} }
// getNetshPath returns the path that should be used to execute netsh.
//
// We've seen a report from a customer that we're triggering the "cannot run
// executable found relative to current directory" protection that was added to
// prevent running possibly attacker-controlled binaries. To mitigate this,
// first try looking up the path to netsh.exe in the System32 directory
// explicitly, and then fall back to the prior behaviour of passing "netsh" to
// os/exec.Command.
func (ft *firewallTweaker) getNetshPath() string {
ft.netshPathOnce.Do(func() {
// The default value is the old approach: just run "netsh" and
// let os/exec resolve that into a full path.
ft.netshPath = "netsh"
path, err := windows.KnownFolderPath(windows.FOLDERID_System, 0)
if err != nil {
ft.logf("getNetshPath: error getting FOLDERID_System: %v", err)
return
}
expath := filepath.Join(path, "netsh.exe")
if _, err := os.Stat(expath); err == nil {
ft.netshPath = expath
return
} else if !os.IsNotExist(err) {
ft.logf("getNetshPath: error checking for existence of %q: %v", expath, err)
}
// Keep default
})
return ft.netshPath
}
func (ft *firewallTweaker) runFirewall(args ...string) (time.Duration, error) { func (ft *firewallTweaker) runFirewall(args ...string) (time.Duration, error) {
t0 := time.Now() t0 := time.Now()
args = append([]string{"advfirewall", "firewall"}, args...) args = append([]string{"advfirewall", "firewall"}, args...)
cmd := exec.Command("netsh", args...) cmd := exec.Command(ft.getNetshPath(), args...)
cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true} cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true}
b, err := cmd.CombinedOutput() b, err := cmd.CombinedOutput()
if err != nil { if err != nil {

View File

@ -0,0 +1,19 @@
// Copyright (c) Tailscale Inc & AUTHORS
// SPDX-License-Identifier: BSD-3-Clause
package router
import (
"path/filepath"
"testing"
)
func TestGetNetshPath(t *testing.T) {
ft := &firewallTweaker{
logf: t.Logf,
}
path := ft.getNetshPath()
if !filepath.IsAbs(path) {
t.Errorf("expected absolute path for netsh.exe: %q", path)
}
}