windivert: Verify cached driver against embedded asset before load

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" }

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" }

common/windivert/assets_unsupported.go

@@ -2,6 +2,6 @@
 
 package windivert
 
-func assetFiles() []assetFile { return nil }
+var sysBytes []byte
 
 func driverSysName() string { return "" }

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
 }

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
 		}

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) {

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,