From 74aa66cf1f518a4bd93b63fdb6f507d3e92502fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=B8=96=E7=95=8C?= Date: Sat, 11 Jul 2026 16:13:22 +0800 Subject: [PATCH] windivert: Verify cached driver against embedded asset before load --- common/windivert/assets_386.go | 4 - common/windivert/assets_amd64.go | 4 - common/windivert/assets_unsupported.go | 2 +- common/windivert/driver_windows.go | 270 +++++++++++-------- common/windivert/handle_windows.go | 2 +- common/windivert/integration_windows_test.go | 106 ++++++++ common/windivert/windivert.go | 5 +- 7 files changed, 265 insertions(+), 128 deletions(-) diff --git a/common/windivert/assets_386.go b/common/windivert/assets_386.go index 0cbf35ed5..d1dac4cda 100644 --- a/common/windivert/assets_386.go +++ b/common/windivert/assets_386.go @@ -7,8 +7,4 @@ import _ "embed" //go:embed assets/WinDivert32.sys var sysBytes []byte -func assetFiles() []assetFile { - return []assetFile{{"WinDivert32.sys", sysBytes}} -} - func driverSysName() string { return "WinDivert32.sys" } diff --git a/common/windivert/assets_amd64.go b/common/windivert/assets_amd64.go index 2c9fb6c6a..3ff6c1435 100644 --- a/common/windivert/assets_amd64.go +++ b/common/windivert/assets_amd64.go @@ -7,8 +7,4 @@ import _ "embed" //go:embed assets/WinDivert64.sys var sysBytes []byte -func assetFiles() []assetFile { - return []assetFile{{"WinDivert64.sys", sysBytes}} -} - func driverSysName() string { return "WinDivert64.sys" } diff --git a/common/windivert/assets_unsupported.go b/common/windivert/assets_unsupported.go index 04698953f..189de58e2 100644 --- a/common/windivert/assets_unsupported.go +++ b/common/windivert/assets_unsupported.go @@ -2,6 +2,6 @@ package windivert -func assetFiles() []assetFile { return nil } +var sysBytes []byte func driverSysName() string { return "" } diff --git a/common/windivert/driver_windows.go b/common/windivert/driver_windows.go index d6bc59f89..811cb03dd 100644 --- a/common/windivert/driver_windows.go +++ b/common/windivert/driver_windows.go @@ -3,12 +3,14 @@ package windivert import ( + "bytes" "errors" + "io" "os" "path/filepath" "runtime" "strconv" - "sync" + "time" E "github.com/sagernet/sing/common/exceptions" @@ -20,23 +22,12 @@ const ( driverDeviceName = `\\.\WinDivert` ) -var ( - driverOnce sync.Once - driverErr error - // driverDevName is ASCII-safe and must be available before ensureDriver - // so Open can try CreateFile first and only install on FILE_NOT_FOUND. - driverDevName, _ = windows.UTF16PtrFromString(driverDeviceName) -) +// driverDevName is ASCII-safe and must be available before installDriver +// so Open can try CreateFile first and only install on FILE_NOT_FOUND. +var driverDevName, _ = windows.UTF16PtrFromString(driverDeviceName) // Requires SeLoadDriverPrivilege (Administrator). Running the 386 build // under WOW64 on a 64-bit kernel is rejected — use the amd64 build. -func ensureDriver() error { - driverOnce.Do(func() { - driverErr = installDriver() - }) - return driverErr -} - func installDriver() error { if runtime.GOARCH == "386" { var isWow64 bool @@ -46,20 +37,12 @@ func installDriver() error { } } - dir, err := ensureExtracted() - if err != nil { - return err - } - sysPath := filepath.Join(dir, driverSysName()) - sysPathW, err := windows.UTF16PtrFromString(sysPath) - if err != nil { - return E.Cause(err, "windivert: utf16 driver path") - } - - // Serialize driver install across concurrent processes. + // Serialize driver install across concurrent processes. CreateMutex + // hands back a valid handle together with ERROR_ALREADY_EXISTS when + // another install already created the mutex. mutexName, _ := windows.UTF16PtrFromString("WinDivertDriverInstallMutex") mutex, err := windows.CreateMutex(nil, false, mutexName) - if err != nil { + if err != nil && !errors.Is(err, windows.ERROR_ALREADY_EXISTS) { return E.Cause(err, "windivert: create install mutex") } defer windows.CloseHandle(mutex) @@ -69,6 +52,16 @@ func installDriver() error { } defer windows.ReleaseMutex(mutex) + sysPath, sysFile, err := extractVerified() + if err != nil { + return err + } + defer sysFile.Close() + sysPathW, err := windows.UTF16PtrFromString(sysPath) + if err != nil { + return E.Cause(err, "windivert: utf16 driver path") + } + manager, err := windows.OpenSCManager(nil, nil, windows.SC_MANAGER_ALL_ACCESS) if err != nil { return E.Cause(err, "windivert: open SCM") @@ -76,48 +69,34 @@ func installDriver() error { defer windows.CloseServiceHandle(manager) serviceNameW, _ := windows.UTF16PtrFromString(driverServiceName) - service, err := windows.OpenService(manager, serviceNameW, windows.SERVICE_ALL_ACCESS) - if err != nil { - service, err = windows.CreateService( - manager, - serviceNameW, - serviceNameW, - windows.SERVICE_ALL_ACCESS, - windows.SERVICE_KERNEL_DRIVER, - windows.SERVICE_DEMAND_START, - windows.SERVICE_ERROR_NORMAL, - sysPathW, - nil, nil, nil, nil, nil, - ) - if err != nil { - if errors.Is(err, windows.ERROR_SERVICE_EXISTS) { - service, err = windows.OpenService(manager, serviceNameW, windows.SERVICE_ALL_ACCESS) - } - if err != nil { - return wrapDriverInstallError(err) - } + // A stopped service record marked for deletion lingers while any handle + // keeps it alive — including the one OpenService just returned to us. + // StartService on it reports ERROR_SERVICE_DISABLED, and + // ChangeServiceConfig cannot un-doom it (ERROR_SERVICE_MARKED_FOR_DELETE). + // The only way out is to close every handle so SCM drops the record, + // then create it anew. + for attempt := 0; ; attempt++ { + err = tryInstallService(manager, serviceNameW, sysPathW) + if err == nil { + return nil } + retryable := errors.Is(err, windows.ERROR_SERVICE_MARKED_FOR_DELETE) || + errors.Is(err, windows.ERROR_SERVICE_DISABLED) + if !retryable || attempt >= 20 { + return err + } + time.Sleep(50 * time.Millisecond) + } +} + +func tryInstallService(manager windows.Handle, serviceNameW, sysPathW *uint16) error { + service, err := openOrCreateService(manager, serviceNameW, sysPathW) + if err != nil { + return err } defer windows.CloseServiceHandle(service) err = windows.StartService(service, 0, nil) - if err != nil && errors.Is(err, windows.ERROR_SERVICE_DISABLED) { - // A prior process called DeleteService on a still-running kernel - // driver: SCM marks the record for deletion and flips START_TYPE - // to DISABLED until the last handle closes. Re-enable so we can - // start it instead of waiting for a reboot. - err = windows.ChangeServiceConfig( - service, - windows.SERVICE_NO_CHANGE, - windows.SERVICE_DEMAND_START, - windows.SERVICE_NO_CHANGE, - nil, nil, nil, nil, nil, nil, nil, - ) - if err != nil { - return E.Cause(err, "windivert: re-enable disabled service") - } - err = windows.StartService(service, 0, nil) - } if err == nil { // Mark for deletion so the driver unregisters when the last handle // closes or on next reboot. Matches the upstream DLL's behavior: @@ -125,10 +104,50 @@ func installDriver() error { // cleanup responsibility. If another process already started it, // we leave DeleteService to them. _ = windows.DeleteService(service) - } else if !errors.Is(err, windows.ERROR_SERVICE_ALREADY_RUNNING) { - return E.Cause(err, "windivert: start service") + return nil } - return nil + if errors.Is(err, windows.ERROR_SERVICE_ALREADY_RUNNING) { + return nil + } + if errors.Is(err, windows.ERROR_SERVICE_DISABLED) { + // The disabled check precedes the running check: a running service + // marked for deletion reports ERROR_SERVICE_DISABLED instead of + // ERROR_SERVICE_ALREADY_RUNNING. The device is nonetheless up. + var status windows.SERVICE_STATUS + queryErr := windows.QueryServiceStatus(service, &status) + if queryErr == nil && status.CurrentState == windows.SERVICE_RUNNING { + return nil + } + } + return E.Cause(err, "windivert: start service") +} + +func openOrCreateService(manager windows.Handle, serviceNameW, sysPathW *uint16) (windows.Handle, error) { + service, err := windows.OpenService(manager, serviceNameW, windows.SERVICE_ALL_ACCESS) + if err == nil { + return service, nil + } + service, err = windows.CreateService( + manager, + serviceNameW, + serviceNameW, + windows.SERVICE_ALL_ACCESS, + windows.SERVICE_KERNEL_DRIVER, + windows.SERVICE_DEMAND_START, + windows.SERVICE_ERROR_NORMAL, + sysPathW, + nil, nil, nil, nil, nil, + ) + if err == nil { + return service, nil + } + if errors.Is(err, windows.ERROR_SERVICE_EXISTS) { + service, err = windows.OpenService(manager, serviceNameW, windows.SERVICE_ALL_ACCESS) + if err == nil { + return service, nil + } + } + return 0, wrapDriverInstallError(err) } func wrapDriverInstallError(err error) error { @@ -138,75 +157,94 @@ func wrapDriverInstallError(err error) error { return E.Cause(err, "windivert: create service") } -type assetFile struct { - name string - data []byte -} - -var ( - extractOnce sync.Once - extractErr error - extractDir string -) - -// The on-disk copy is protected by Windows Authenticode signature -// enforcement, which rejects any tampered .sys at StartService time. -func ensureExtracted() (string, error) { - extractOnce.Do(func() { - extractDir, extractErr = extractImpl() - }) - return extractDir, extractErr -} - -func extractImpl() (string, error) { - files := assetFiles() - if len(files) == 0 { - return "", E.New("windivert: unsupported architecture ", runtime.GOARCH) +// The cache directory is user-writable, so the .sys found there is +// untrusted: anything (e.g. a validly signed but vulnerable foreign driver) +// could have been planted before we run elevated. The bytes are therefore +// verified against the embedded asset through the returned handle, whose +// share mode denies write, delete, and rename until the caller closes it — +// the kernel maps exactly what was verified. MmLoadSystemImage opens the +// image with read/execute desired access, which the FILE_SHARE_READ grant +// admits, so holding the handle across StartService does not fail the load. +func extractVerified() (string, *os.File, error) { + if len(sysBytes) == 0 { + return "", nil, E.New("windivert: unsupported architecture ", runtime.GOARCH) } base, err := os.UserCacheDir() if err != nil { - return "", E.Cause(err, "windivert: locate user cache dir") + return "", nil, E.Cause(err, "windivert: locate user cache dir") } dir := filepath.Join(base, "sing-box", "windivert", "v"+AssetVersion) err = os.MkdirAll(dir, 0o755) if err != nil { - return "", E.Cause(err, "windivert: mkdir ", dir) + return "", nil, E.Cause(err, "windivert: mkdir ", dir) } + target := filepath.Join(dir, driverSysName()) - for _, asset := range files { - err = ensureAsset(dir, asset) + for attempt := 0; ; attempt++ { + sysFile, err := openDriverFile(target) if err != nil { - return "", err + if !os.IsNotExist(err) { + return "", nil, E.Cause(err, "windivert: open ", target) + } + err = writeDriverFile(target) + if err != nil { + return "", nil, err + } + sysFile, err = openDriverFile(target) + if err != nil { + return "", nil, E.Cause(err, "windivert: open ", target) + } + } + content, err := io.ReadAll(sysFile) + if err != nil { + sysFile.Close() + return "", nil, E.Cause(err, "windivert: read ", target) + } + if bytes.Equal(content, sysBytes) { + return target, sysFile, nil + } + sysFile.Close() + if attempt > 0 { + return "", nil, E.New("windivert: driver file ", target, " is being concurrently modified") + } + err = writeDriverFile(target) + if err != nil { + return "", nil, err } } - return dir, nil } -// Concurrent sing-box processes race on os.Rename (atomic on NTFS); -// whichever wins creates the final file. Writers that lose the race -// silently discard their temp copy. -func ensureAsset(dir string, asset assetFile) error { - target := filepath.Join(dir, asset.name) - _, err := os.Stat(target) - if err == nil { - return nil - } - if !os.IsNotExist(err) { - return E.Cause(err, "windivert: stat ", asset.name) - } - tmp := target + ".tmp-" + strconv.Itoa(os.Getpid()) - err = os.WriteFile(tmp, asset.data, 0o644) +func openDriverFile(path string) (*os.File, error) { + pathW, err := windows.UTF16PtrFromString(path) if err != nil { - return E.Cause(err, "windivert: write ", asset.name) + return nil, err + } + handle, err := windows.CreateFile( + pathW, + windows.GENERIC_READ, + windows.FILE_SHARE_READ, + nil, + windows.OPEN_EXISTING, + windows.FILE_ATTRIBUTE_NORMAL, + 0, + ) + if err != nil { + return nil, err + } + return os.NewFile(uintptr(handle), path), nil +} + +func writeDriverFile(target string) error { + tmp := target + ".tmp-" + strconv.Itoa(os.Getpid()) + err := os.WriteFile(tmp, sysBytes, 0o644) + if err != nil { + return E.Cause(err, "windivert: write ", filepath.Base(target)) } err = os.Rename(tmp, target) if err != nil { os.Remove(tmp) - if _, statErr := os.Stat(target); statErr == nil { - return nil - } - return E.Cause(err, "windivert: rename ", asset.name) + return E.Cause(err, "windivert: rename ", filepath.Base(target)) } return nil } diff --git a/common/windivert/handle_windows.go b/common/windivert/handle_windows.go index a644d6279..800b431c0 100644 --- a/common/windivert/handle_windows.go +++ b/common/windivert/handle_windows.go @@ -63,7 +63,7 @@ func Open(filter *Filter, layer Layer, priority int16, flags Flag) (*Handle, err // Device node missing: kernel driver not loaded. Install + retry. // Matches WinDivertOpen's lazy-install path; avoids racing StartService // against a still-loaded driver whose SCM record is marked for deletion. - err = ensureDriver() + err = installDriver() if err != nil { return nil, err } diff --git a/common/windivert/integration_windows_test.go b/common/windivert/integration_windows_test.go index 1b4ce958c..100ee0d4b 100644 --- a/common/windivert/integration_windows_test.go +++ b/common/windivert/integration_windows_test.go @@ -3,8 +3,12 @@ package windivert import ( + "bytes" "errors" "net/netip" + "os" + "path/filepath" + "strconv" "testing" "time" @@ -67,6 +71,108 @@ func TestIntegrationRecvAbortsOnClose(t *testing.T) { } } +func cachedDriverPath(t *testing.T) string { + t.Helper() + base, err := os.UserCacheDir() + require.NoError(t, err) + return filepath.Join(base, "sing-box", "windivert", "v"+AssetVersion, driverSysName()) +} + +// The driver does not unload when the last handle closes: it stays running +// (and the memory manager keeps its backing image write-locked) until +// explicitly stopped, like `sc stop WinDivert`. The install-time +// DeleteService mark then removes the record once the last SCM handle +// closes. +func stopDriver(t *testing.T) { + t.Helper() + manager, err := windows.OpenSCManager(nil, nil, windows.SC_MANAGER_CONNECT) + require.NoError(t, err) + defer windows.CloseServiceHandle(manager) + serviceNameW, err := windows.UTF16PtrFromString(driverServiceName) + require.NoError(t, err) + service, err := windows.OpenService(manager, serviceNameW, windows.SERVICE_STOP|windows.SERVICE_QUERY_STATUS) + if errors.Is(err, windows.ERROR_SERVICE_DOES_NOT_EXIST) { + return + } + require.NoError(t, err) + defer windows.CloseServiceHandle(service) + var status windows.SERVICE_STATUS + err = windows.ControlService(service, windows.SERVICE_CONTROL_STOP, &status) + if err != nil && + !errors.Is(err, windows.ERROR_SERVICE_NOT_ACTIVE) && + !errors.Is(err, windows.ERROR_SERVICE_CANNOT_ACCEPT_CTRL) { + require.NoError(t, err) + } + require.Eventually(t, func() bool { + queryErr := windows.QueryServiceStatus(service, &status) + return queryErr == nil && status.CurrentState == windows.SERVICE_STOPPED + }, 60*time.Second, 200*time.Millisecond, "driver did not reach SERVICE_STOPPED") +} + +// The image lock on the cached .sys can outlive SERVICE_STOPPED by tens of +// seconds (observed on GitHub-hosted runners), but it only blocks writes +// and deletes — rename is permitted. Move the locked file aside instead of +// waiting for the release. +func plantTamperedDriver(t *testing.T, target string, planted []byte) { + t.Helper() + require.NoError(t, os.MkdirAll(filepath.Dir(target), 0o755)) + err := os.WriteFile(target, planted, 0o644) + if err == nil { + return + } + moved := target + ".locked-" + strconv.FormatInt(time.Now().UnixNano(), 10) + require.NoError(t, os.Rename(target, moved)) + t.Cleanup(func() { os.Remove(moved) }) + require.NoError(t, os.WriteFile(target, planted, 0o644)) +} + +// A foreign .sys planted in the user-writable cache must never reach +// StartService: the install path has to detect the mismatch against the +// embedded asset and repair the file before handing it to SCM. +func TestIntegrationTamperedCacheRepaired(t *testing.T) { + // Open/close once so a working install is the baseline, then stop the + // driver so the cached file is writable for tampering. + h := openHandle(t, nil, FlagSendOnly) + require.NoError(t, h.Close()) + stopDriver(t) + + target := cachedDriverPath(t) + plantTamperedDriver(t, target, []byte("planted payload, not the WinDivert driver")) + + h = openHandle(t, nil, FlagSendOnly) + require.NoError(t, h.Close()) + + content, err := os.ReadFile(target) + require.NoError(t, err) + require.True(t, bytes.Equal(content, sysBytes), "cached driver was not repaired to the embedded asset") +} + +// The verified handle must lock the file against writers and renames until +// install completes; without this, the file could be swapped between +// verification and the kernel mapping it. +func TestIntegrationDriverFileLockedWhileHeld(t *testing.T) { + target := cachedDriverPath(t) + // Stop the driver so the kernel image lock is gone and the failures + // asserted below can only come from the handle extractVerified holds. + stopDriver(t) + plantTamperedDriver(t, target, sysBytes) + + sysPath, sysFile, err := extractVerified() + require.NoError(t, err) + defer sysFile.Close() + + writeErr := os.WriteFile(sysPath, []byte("overwrite attempt"), 0o644) + require.Error(t, writeErr) + require.True(t, errors.Is(writeErr, windows.ERROR_SHARING_VIOLATION), + "expected sharing violation, got %v", writeErr) + + evil := sysPath + ".evil" + require.NoError(t, os.WriteFile(evil, []byte("replacement attempt"), 0o644)) + defer os.Remove(evil) + renameErr := os.Rename(evil, sysPath) + require.Error(t, renameErr) +} + // Two concurrent Open calls must both succeed: the first wins the driver // install race, the second reuses the already-running service. func TestIntegrationConcurrentOpen(t *testing.T) { diff --git a/common/windivert/windivert.go b/common/windivert/windivert.go index 280d11f63..3a3e59278 100644 --- a/common/windivert/windivert.go +++ b/common/windivert/windivert.go @@ -1,8 +1,9 @@ // Package windivert provides a pure-Go binding to the WinDivert kernel // driver on Windows (amd64 and 386). User-mode WinDivert calls are // reimplemented in Go; only the signed kernel driver is embedded as an -// asset, since SCM-installed drivers must live on disk and their -// Authenticode signature forbids modification. +// asset, since SCM-installed drivers must live on disk. The on-disk copy +// is verified byte-for-byte against the embedded asset on every install +// and held open deny-write while the kernel loads it. // // Administrator is required for the first Open in a process so SCM can // load the driver. Upstream: https://github.com/basil00/WinDivert v2.2.2,