From 891d5d98216e1c96c8d413f6d34f4989f0e47f35 Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Thu, 30 Jul 2026 15:49:09 +0700 Subject: [PATCH] cmd/cli: drop stale intercept enforcement at startup, before API bootstrap A host carrying orphaned WFP filters from a ctrld build that predates session-scoped ownership cannot recover on its own. In Firewall Mode those filters block all non-allowlisted outbound traffic machine-wide, which denies the replacement ctrld's own API bootstrap. API-managed startup then sits in the resolver-config retry loop forever and never reaches startWFPFilters, where the only stale-sublayer cleanup lived. Cleanup needs startup, startup needs the network, the network needs cleanup - the host stays locked out until a reboot. Add cleanupStaleDNSInterceptState() and call it early in run(), before the network-up wait and before the API preflight. On Windows it deletes ctrld's WFP sublayer, which takes its child filters with it, so a previous process's enforcement is gone before this process makes its first connection. Objects owned by a live session cannot be deleted, so a running ctrld is unaffected and "nothing to clean up" stays at debug level; an actual removal logs a warning, since it means a previous ctrld left machine-wide enforcement installed. macOS and the other platforms get no-op implementations: pf enforcement does not outlive the process, and startDNSIntercept already flushes the anchor and removes a stale anchor file before loading rules. The cleanup inside startWFPFilters stays as a second line of defense. The cleanup session is deliberately NOT dynamic. FwpmSubLayerDeleteByKey0 is documented to fail with FWP_E_DYNAMIC_SESSION_IN_PROGRESS when called from a dynamic session for an object that was not added in one, and the only orphans that can exist are exactly those: a ctrld predating session-scoped ownership added its sublayer statically. Anything a newer ctrld leaves behind is removed by the OS when its session ends. Opening this session dynamically would have made the cleanup a no-op in the one case it exists for, while still logging "no stale WFP state". A concurrently running ctrld is protected by documented ownership rather than by the child-filter question below: a session-scoped ctrld's sublayer belongs to a different dynamic session, so the delete fails with FWP_E_WRONG_SESSION. That matters because this call is made unconditionally at startup, in every intercept mode, so an interactive "ctrld run" alongside a healthy service reaches it. A ctrld predating session scoping has no such protection: it holds a non-dynamic sublayer, which is exactly what this targets and is indistinguishable from an orphan. Two guards cover that instead. Elevation, because opening a WFP engine and deleting ctrld's sublayer must not be reachable from an unprivileged local process - FwpmEngineOpen0 is expected to fail without elevation, but that is a property of the API rather than something this code checked, and it was the only barrier. And interactive invocation, since a service start is not interactive: the deadlock case still gets cleaned, while a hand-run "ctrld run" beside a live service does not strip its enforcement. That second guard requires positive evidence of absence - ctrldServiceLiveness answers unknown for an unreachable SCM or a service mid-stop, and unknown skips the cleanup exactly as running does, because "could not be observed" is not "not there". Both are asserted against the deletion itself, not only against the predicate: a caller-level test substitutes the WFP delete and the guard inputs, so a future change that stops consulting the guard, or consults it and deletes anyway, fails rather than staying green. Delete failures are no longer collapsed into "nothing to clean up". Only FWP_E_SUBLAYER_NOT_FOUND means that; any other code means state exists under our GUID that we could not remove, which is the lockout condition itself, so it is logged as a warning naming the code. Whether deleting the sublayer is sufficient is left explicitly UNRESOLVED rather than asserted. It is sufficient only if the delete also removes the filters inside it. FwpmSubLayerDeleteByKey0's Remarks say nothing about child filters either way, while object management states that an object cannot be deleted until everything referencing it has been - and FWP_E_IN_USE exists for that. Whether a filter's subLayerKey counts as such a reference is not documented, and this code cannot be exercised off-Windows, so the earlier claim that the delete "takes its child filters with it" is removed from the comment here and from docs/wfp-dns-intercept.md, which carried it from before this branch. The behaviour is safe under both readings: the delete is attempted, and FWP_E_IN_USE is reported rather than counted as success, so a support log distinguishes "cleared it" from "could not clear it". If a live Windows check shows FWP_E_IN_USE against orphaned filters, the cleanup must enumerate and delete those filters first. That is deliberately not written blind: FWPM_FILTER_ENUM_TEMPLATE0 has no sublayer field, so selecting ctrld's own filters means reading subLayerKey at a computed offset in FWPM_FILTER0, and getting that offset wrong would delete other software's filters - a worse failure than not cleaning up. Refs: https://learn.microsoft.com/en-us/windows/win32/api/fwpmu/nf-fwpmu-fwpmsublayerdeletebykey0 Refs: https://learn.microsoft.com/en-us/windows/win32/fwp/object-management --- cmd/cli/cli.go | 8 + cmd/cli/dns_intercept_cleanup_windows_test.go | 180 +++++++++++++++ cmd/cli/dns_intercept_darwin.go | 6 + cmd/cli/dns_intercept_others.go | 4 + cmd/cli/dns_intercept_windows.go | 213 +++++++++++++++++- cmd/cli/service_windows.go | 61 +++++ docs/wfp-dns-intercept.md | 17 +- 7 files changed, 483 insertions(+), 6 deletions(-) create mode 100644 cmd/cli/dns_intercept_cleanup_windows_test.go diff --git a/cmd/cli/cli.go b/cmd/cli/cli.go index abc2558..c1127f5 100644 --- a/cmd/cli/cli.go +++ b/cmd/cli/cli.go @@ -298,6 +298,14 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { p.Info().Msgf("Starting ctrld %s", curVersion()) p.Info().Msgf("OS: %s", osVersion()) + // Drop enforcement left behind by a previous ctrld process before doing anything + // that needs the network. A previous run that died without cleaning up can leave + // machine-wide block filters installed (Firewall Mode blocks all non-allowlisted + // outbound traffic), which would deny this process's own API bootstrap below and + // leave it retrying forever - never reaching the cleanup that lives inside + // intercept startup. No-op when no stale state exists. + cleanupStaleDNSInterceptState() + // Wait for network up. if !ctrldnet.Up() { notifyExitToLogServer() diff --git a/cmd/cli/dns_intercept_cleanup_windows_test.go b/cmd/cli/dns_intercept_cleanup_windows_test.go new file mode 100644 index 0000000..bdb7fbd --- /dev/null +++ b/cmd/cli/dns_intercept_cleanup_windows_test.go @@ -0,0 +1,180 @@ +//go:build windows + +package cli + +import ( + "errors" + "testing" +) + +// TestStaleCleanupRequiresPositiveEvidence pins the guard that protects a live +// pre-session-scoped ctrld service. +// +// ctrldServiceLiveness cannot answer for an unreachable SCM, a caller without rights, or a +// service mid-stop. Folding those into "stopped" would let a hand-run "ctrld run" delete +// the sublayer of a live old-build service - the exact enforcement strip the interactive +// guard exists to prevent - because that build's non-dynamic sublayer looks like an +// orphan. Only a positive stopped answer may unlock the cleanup. +func TestStaleCleanupRequiresPositiveEvidence(t *testing.T) { + tests := []struct { + name string + interactive bool + liveness serviceLiveness + want bool + }{ + // A service start is the deadlock case: nothing of ctrld's is live yet, and this + // is the only path that can break a host locked out by orphaned filters. + {"service start with a running service", false, serviceLivenessRunning, true}, + {"service start with an unknown state", false, serviceLivenessUnknown, true}, + {"service start with a stopped service", false, serviceLivenessStopped, true}, + + // Interactive: only positive evidence of absence unlocks it. + {"ctrld run alongside a live service", true, serviceLivenessRunning, false}, + {"ctrld run when the SCM cannot be queried", true, serviceLivenessUnknown, false}, + {"ctrld run with the service stopped or absent", true, serviceLivenessStopped, true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := staleCleanupAllowed(tc.interactive, tc.liveness); got != tc.want { + t.Errorf("staleCleanupAllowed(%v, %v) = %v, want %v", tc.interactive, tc.liveness, got, tc.want) + } + }) + } + + // The zero value must be the safe one: a serviceLiveness that was never assigned - a + // future code path that forgets to set it - must not read as "nothing is live". + if staleCleanupAllowed(true, serviceLiveness(0)) { + t.Error("the zero serviceLiveness must not unlock the cleanup: an unset value is not evidence of absence") + } +} + +// stubStaleCleanup installs fakes for the cleanup's guard inputs and for the deletion +// itself, and returns a pointer to the delete-attempt count. +// +// The delete is a WFP syscall: running it for real would remove live filters from the +// machine running the tests, so the assertion has to be made against a substituted delete. +func stubStaleCleanup(t *testing.T, elevated bool, elevErr error, interactive bool, liveness serviceLiveness) *int { + t.Helper() + oldElev, oldInter, oldLive, oldDel := staleCleanupElevatedFn, staleCleanupInteractiveFn, staleCleanupLivenessFn, deleteStaleWFPSublayerFn + t.Cleanup(func() { + staleCleanupElevatedFn = oldElev + staleCleanupInteractiveFn = oldInter + staleCleanupLivenessFn = oldLive + deleteStaleWFPSublayerFn = oldDel + }) + + deletes := 0 + staleCleanupElevatedFn = func() (bool, error) { return elevated, elevErr } + staleCleanupInteractiveFn = func() bool { return interactive } + staleCleanupLivenessFn = func() serviceLiveness { return liveness } + deleteStaleWFPSublayerFn = func() { deletes++ } + return &deletes +} + +// TestCleanupStaleStateConsultsTheGuardBeforeDeleting is the caller-level half of the +// guard's coverage. +// +// staleCleanupAllowed being correct proves nothing on its own: cleanupStaleDNSInterceptState +// could stop calling it, or call it and delete anyway, and a predicate-only test would stay +// green while a hand-run "ctrld run" stripped a live service's enforcement. This asserts on +// the deletion itself - whether the WFP delete is attempted at all - which is the behaviour +// that matters. +func TestCleanupStaleStateConsultsTheGuardBeforeDeleting(t *testing.T) { + tests := []struct { + name string + elevated bool + elevErr error + interactive bool + liveness serviceLiveness + wantDeletes int + }{ + { + // The deadlock case this function exists for: a service start, where nothing + // of ctrld's is live and the host may be carrying orphaned block-all filters. + name: "service start attempts the delete", + elevated: true, + wantDeletes: 1, + }, + { + // A service start does not consult the SCM at all, so a running service + // reported here must not change the outcome. + name: "service start is not gated on service state", + elevated: true, + liveness: serviceLivenessRunning, + wantDeletes: 1, + }, + { + name: "interactive run with a stopped service attempts the delete", + elevated: true, + interactive: true, + liveness: serviceLivenessStopped, + wantDeletes: 1, + }, + { + // Deleting here would strip a live pre-session-scoped service's enforcement. + name: "interactive run beside a live service does not delete", + elevated: true, + interactive: true, + liveness: serviceLivenessRunning, + }, + { + // The concern this test was added for: "could not tell" is not absence. + name: "interactive run with an unreadable SCM does not delete", + elevated: true, + interactive: true, + liveness: serviceLivenessUnknown, + }, + { + // Elevation is the only barrier between an unprivileged local process and a + // path that opens a WFP engine and deletes ctrld's sublayer. + name: "an unelevated caller does not delete", + elevated: false, + }, + { + name: "an elevation check that fails does not delete", + elevErr: errors.New("cannot determine privilege"), + }, + { + // Elevation reported true alongside an error is not a yes. + name: "an inconclusive elevation check does not delete", + elevated: true, + elevErr: errors.New("cannot determine privilege"), + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + deletes := stubStaleCleanup(t, tc.elevated, tc.elevErr, tc.interactive, tc.liveness) + + cleanupStaleDNSInterceptState() + + if *deletes != tc.wantDeletes { + t.Errorf("WFP delete attempts = %d, want %d", *deletes, tc.wantDeletes) + } + }) + } +} + +// TestCleanupStaleStateDoesNotQueryTheSCMOnAServiceStart pins the ordering the deadlock +// recovery depends on. +// +// The cleanup runs before the network-up wait and before API preflight, on the path a +// locked-out host has to take. Consulting the SCM there would make the one case this +// exists for depend on a query that can block or fail - and a failure answers Unknown, +// which refuses the cleanup. A service start must not ask. +func TestCleanupStaleStateDoesNotQueryTheSCMOnAServiceStart(t *testing.T) { + deletes := stubStaleCleanup(t, true, nil, false, serviceLivenessStopped) + queried := false + staleCleanupLivenessFn = func() serviceLiveness { + queried = true + return serviceLivenessRunning + } + + cleanupStaleDNSInterceptState() + + if queried { + t.Error("a service start queried the SCM: an unreadable SCM would then refuse the cleanup the locked-out host needs") + } + if *deletes != 1 { + t.Errorf("WFP delete attempts = %d, want 1", *deletes) + } +} diff --git a/cmd/cli/dns_intercept_darwin.go b/cmd/cli/dns_intercept_darwin.go index 9087744..96634c9 100644 --- a/cmd/cli/dns_intercept_darwin.go +++ b/cmd/cli/dns_intercept_darwin.go @@ -1954,6 +1954,12 @@ func buildDNSQueryPacket(domain string) []byte { return append(header, question...) } +// cleanupStaleDNSInterceptState is a startup hook for enforcement that can outlive +// the process. macOS needs no work here: startDNSIntercept flushes the anchor and +// removes a stale anchor file before loading rules, and the pf anchor alone does not +// block traffic until ctrld loads rules into it. +func cleanupStaleDNSInterceptState() {} + // pfInterceptMonitor runs asynchronously after interface changes are detected. // It probes pf interception with exponential backoff and forces a full pf reload // if the probe fails. Only one instance runs at a time (singleton via atomic.Bool). diff --git a/cmd/cli/dns_intercept_others.go b/cmd/cli/dns_intercept_others.go index 6c5e088..f425bf3 100644 --- a/cmd/cli/dns_intercept_others.go +++ b/cmd/cli/dns_intercept_others.go @@ -49,6 +49,10 @@ func (p *prog) pfInterceptMonitor() {} // reconcileForwardedSources is a no-op on unsupported platforms (macOS-only). func (p *prog) reconcileForwardedSources() {} +// cleanupStaleDNSInterceptState is a no-op on unsupported platforms — there is no +// intercept state that can outlive the process here. +func cleanupStaleDNSInterceptState() {} + // osHealthcheckSuppressed always returns false on non-Windows platforms — // WFP loopback protect (the trigger for suppression) is Windows-only. func (p *prog) osHealthcheckSuppressed() bool { return false } diff --git a/cmd/cli/dns_intercept_windows.go b/cmd/cli/dns_intercept_windows.go index 6c9e40e..d6db4f9 100644 --- a/cmd/cli/dns_intercept_windows.go +++ b/cmd/cli/dns_intercept_windows.go @@ -15,6 +15,7 @@ import ( "time" "unsafe" + "github.com/kardianos/service" "golang.org/x/sys/windows" "golang.org/x/sys/windows/registry" @@ -130,6 +131,28 @@ const ( // mode to override third-party WFP blocks (e.g., OpenVPN's block-outside-dns). fwpmFilterFlagClearActionRight uint32 = 0x00000008 + // WFP error codes from winerror.h, needed to tell "there was nothing to clean + // up" apart from "there was, and we could not remove it" - which is the lockout + // condition itself and must not be logged as success. + // See: https://learn.microsoft.com/en-us/windows/win32/fwp/wfp-error-codes + // + // FWP_E_SUBLAYER_NOT_FOUND: the sub-layer does not exist. + fwpESubLayerNotFound uintptr = 0x80320007 + // FWP_E_IN_USE: the object is referenced by other objects, so it cannot be + // deleted. Whether a sublayer's own filters count as such references is not + // documented - see the UNRESOLVED note on cleanupStaleDNSInterceptState. It is + // handled because if it does occur, deleting the sublayer alone did not clear the + // orphaned enforcement. + fwpEInUse uintptr = 0x8032000A + // FWP_E_DYNAMIC_SESSION_IN_PROGRESS: the call is not allowed from within a + // dynamic session. Returned when deleting an object that was NOT added in a + // dynamic session - i.e. exactly the stale objects this cleanup targets. + fwpEDynamicSessionInProgress uintptr = 0x8032000B + // FWP_E_WRONG_SESSION: the call was made from the wrong session. Returned when + // deleting an object added by a *different* dynamic session, i.e. one owned by + // another live ctrld. + fwpEWrongSession uintptr = 0x8032000C + // fwpmSessionFlagDynamic is FWPM_SESSION_FLAG_DYNAMIC from fwpmtypes.h. // // Every WFP object added through a dynamic session is owned by that session and @@ -1120,6 +1143,183 @@ func (p *prog) removeOrphanedCtrldNRPTRule(reason string) { ops.signal() } +// cleanupStaleDNSInterceptState removes ctrld-owned WFP objects left behind by a +// previous process. It runs at startup before anything that needs network access. +// +// Filters installed by a ctrld that predates session-scoped ownership (or by any +// process whose session was not dynamic) survive that process's death. In Firewall +// Mode those include machine-wide block-all filters, so the whole host - browsers, +// other users, and a replacement ctrld's own API bootstrap - stays denied outbound +// traffic until a reboot. A replacement that cannot reach the API never finishes +// preflight, so it never reaches the cleanup inside startWFPFilters and the host +// stays locked out. Deleting the sublayer here is the attempt to break that deadlock +// before the first API call. +// +// RELEASE GATE: whether deleting the sublayer is sufficient must be answered on a live +// host that carries real pre-fix orphaned filters - run this binary there and record the +// HRESULT. If it is FWP_E_IN_USE, the delete did not remove the child filters, the block-all +// stays active, and this needs the enumerate-and-delete fallback described below before the +// self-heal can be claimed to work. +// +// UNRESOLVED until then: whether deleting the sublayer is sufficient. It is sufficient only if +// the delete also removes the filters inside it. FwpmSubLayerDeleteByKey0's Remarks +// say nothing about child filters either way, while Object Management states the +// general rule that "an object cannot be deleted until all objects that reference it +// have first been deleted" - and FWP_E_IN_USE exists for exactly that. Whether a +// filter's subLayerKey counts as such a reference is not documented, and this is +// Windows-only syscall code that cannot be exercised off-Windows, so it is not +// asserted here in either direction. What the code does is safe under both readings: +// it attempts the delete and, on FWP_E_IN_USE, says so instead of reporting success, +// so a support log distinguishes "cleared it" from "could not clear it". If a live +// Windows check shows FWP_E_IN_USE against orphaned filters, this needs to enumerate +// and delete them first - which is deliberately not written blind, because +// FWPM_FILTER_ENUM_TEMPLATE0 has no sublayer field, so selecting ctrld's own filters +// means reading subLayerKey at a computed offset in FWPM_FILTER0, and getting that +// offset wrong would delete other software's filters. +// +// Deleting objects owned by a live session is expected to fail; those are cleaned up +// by the OS when that session closes. Every failure is therefore debug-level: no +// stale state is the normal case. +// +// Two guards decide whether this may run at all. Elevation, because opening a WFP engine +// and deleting ctrld's sublayer must not be reachable from an unprivileged local process: +// FwpmEngineOpen0 is expected to fail without elevation, but that is a documented +// property of the API rather than something this code checks, and it is the only barrier. +// And interactive invocation, because a live ctrld built before session-scoped ownership +// holds a *non-dynamic* sublayer - exactly what this targets - so the HRESULTs cannot +// tell that service apart from an orphan. A service start is not interactive, so the +// deadlock case still gets cleaned; a hand-run "ctrld run" alongside a live service does +// not strip its enforcement. That second guard requires positive evidence that nothing is +// live: an SCM that cannot be queried is not an absent service, so an unknown answer skips +// the cleanup exactly as a running one does. +func cleanupStaleDNSInterceptState() { + if elevated, err := staleCleanupElevatedFn(); err != nil || !elevated { + mainLog.Load().Debug().Err(err).Msg("DNS intercept: skipping stale WFP state cleanup - not elevated") + return + } + interactive := staleCleanupInteractiveFn() + liveness := serviceLivenessStopped + if interactive { + liveness = staleCleanupLivenessFn() + } + if !staleCleanupAllowed(interactive, liveness) { + reason := "the ctrld service is running" + if liveness == serviceLivenessUnknown { + reason = "the ctrld service's state could not be determined" + } + mainLog.Load().Info().Msgf("DNS intercept: skipping stale WFP state cleanup - %s and a pre-session-scoped build's sublayer is indistinguishable from an orphan", reason) + return + } + deleteStaleWFPSublayerFn() +} + +// Seams for the guard decision and the deletion it protects. The delete is a WFP syscall +// that cannot be run in a test even on Windows - it would remove real filters - so the +// only way to assert that the guard is consulted *before* anything is deleted is to +// substitute both ends. +var ( + staleCleanupElevatedFn = hasElevatedPrivilege + staleCleanupInteractiveFn = service.Interactive + staleCleanupLivenessFn = ctrldServiceLiveness + deleteStaleWFPSublayerFn = deleteStaleWFPSublayer +) + +// deleteStaleWFPSublayer opens a WFP engine and deletes ctrld's sublayer. Callers must +// have established that no live ctrld owns it; see cleanupStaleDNSInterceptState. +func deleteStaleWFPSublayer() { + var engineHandle uintptr + session := fwpmSession0{} + sessionName, _ := windows.UTF16PtrFromString("ctrld Stale State Cleanup") + session.displayData.name = sessionName + // Deliberately NOT a dynamic session. FwpmSubLayerDeleteByKey0 is documented to + // fail with FWP_E_DYNAMIC_SESSION_IN_PROGRESS when called from a dynamic session + // for an object that was not added in one - and the only orphans that can exist + // are exactly those: a ctrld built before session-scoped ownership added its + // sublayer statically. Anything a *newer* ctrld leaves behind is removed by the OS + // when its session ends, so there is nothing for this to clean there. Opening this + // session dynamically would therefore make the whole cleanup a no-op in the one + // case it exists for. + // + // What protects a live ctrld is documented ownership, not the child-filter question + // above: a session-scoped ctrld's sublayer belongs to a different dynamic session, so + // the delete fails with FWP_E_WRONG_SESSION. A ctrld that predates session scoping has + // no such protection here - it holds a non-dynamic sublayer, indistinguishable from an + // orphan - which is why the caller refuses to run this from an interactive invocation + // unless the service is provably not live. FWP_E_IN_USE may refuse the delete as well, + // but whether it does is the open question, so nothing relies on it. + // + // This session adds no objects, so it needs no automatic teardown of its own. + + const rpcCAuthnDefault = 0xFFFFFFFF + r1, _, _ := procFwpmEngineOpen0.Call( + 0, + uintptr(rpcCAuthnDefault), + 0, + uintptr(unsafe.Pointer(&session)), + uintptr(unsafe.Pointer(&engineHandle)), + ) + if r1 != 0 { + mainLog.Load().Debug().Msgf("DNS intercept: could not open WFP engine for stale state cleanup (HRESULT 0x%x)", r1) + return + } + defer procFwpmEngineClose0.Call(engineHandle) + + r1, _, _ = procFwpmSubLayerDeleteByKey0.Call( + engineHandle, + uintptr(unsafe.Pointer(&ctrldSubLayerGUID)), + ) + switch r1 { + case 0: + // Warn, not info: this means a previous ctrld left machine-wide enforcement + // installed, which is worth seeing in a support log. + mainLog.Load().Warn().Msg("DNS intercept: removed WFP filters left by a previous ctrld process (including any Firewall Mode block-all) before startup") + case fwpESubLayerNotFound: + // The normal case: nothing was left behind. + mainLog.Load().Debug().Msg("DNS intercept: no stale WFP state from a previous process") + default: + // Something is installed under our GUID and we could not remove it. Never + // report this as "nothing to clean up": if it is orphaned enforcement, this is + // the lockout condition, and the operator needs the code to act on. + // FWP_E_WRONG_SESSION means another live ctrld owns it, which is benign. + // FWP_E_IN_USE is the answer the release gate above is waiting for: the delete was + // refused while something still references the sublayer, which would mean the + // enumerate-and-delete fallback is required before this self-heal works at all. + mainLog.Load().Warn().Msgf("DNS intercept: found WFP state under ctrld's sublayer but could not remove it (%s); if ctrld is not already running, enforcement from a previous process may still be active", + wfpDeleteErrString(r1)) + } +} + +// staleCleanupAllowed reports whether the stale-state cleanup may run. +// +// A service start (not interactive) always may: that is the deadlock this exists to break, +// and nothing else of ctrld's is live at that point in startup. +// +// A hand-run "ctrld run" may only proceed on positive evidence that nothing is live. A +// pre-session-scoped build's sublayer is indistinguishable from an orphan, so cleaning +// while that service runs would strip its enforcement. Unknown is therefore treated like +// running, not like stopped - an SCM that cannot be queried is not an absent service. +func staleCleanupAllowed(interactive bool, liveness serviceLiveness) bool { + if !interactive { + return true + } + return liveness == serviceLivenessStopped +} + +// wfpDeleteErrString names the delete failures that carry a specific meaning for stale +// state, so a support log says which case was hit rather than only a raw HRESULT. +func wfpDeleteErrString(r1 uintptr) string { + switch r1 { + case fwpEInUse: + return "FWP_E_IN_USE: refused while the sublayer is still referenced (see the release gate at cleanupStaleDNSInterceptState)" + case fwpEDynamicSessionInProgress: + return "FWP_E_DYNAMIC_SESSION_IN_PROGRESS: cannot delete a non-dynamic object from a dynamic session" + case fwpEWrongSession: + return "FWP_E_WRONG_SESSION: owned by another live session" + default: + return fmt.Sprintf("HRESULT 0x%x", r1) + } +} + // startWFPFilters opens the WFP engine and adds all block/permit filters. // Called only in hard intercept mode. func (p *prog) startWFPFilters(state *wfpState) error { @@ -1150,9 +1350,16 @@ func (p *prog) startWFPFilters(state *wfpState) error { } mainLog.Load().Info().Msgf("DNS intercept: WFP engine opened (handle: 0x%x, session-scoped)", engineHandle) - // Clean up any sublayer left by an older ctrld that used a non-dynamic session - // (or by a build predating session-scoped ownership). Deleting the sublayer - // removes all its child filters. + // Clean up a sublayer left over from an earlier session. + // + // Note this runs on the dynamic session opened above, and + // FwpmSubLayerDeleteByKey0 is documented to fail with + // FWP_E_DYNAMIC_SESSION_IN_PROGRESS from a dynamic session when the object was not + // added in one. So this cannot clear state left by a build that predates + // session-scoped ownership - cleanupStaleDNSInterceptState(), which opens a + // non-dynamic session at startup, is what handles that. What remains for this call + // is a leftover from a previous dynamic session whose teardown had not completed + // when we opened ours. r1, _, _ = procFwpmSubLayerDeleteByKey0.Call( engineHandle, uintptr(unsafe.Pointer(&ctrldSubLayerGUID)), diff --git a/cmd/cli/service_windows.go b/cmd/cli/service_windows.go index 0c6d521..a5a2ac2 100644 --- a/cmd/cli/service_windows.go +++ b/cmd/cli/service_windows.go @@ -1,6 +1,7 @@ package cli import ( + "errors" "os" "runtime" "syscall" @@ -8,6 +9,7 @@ import ( "unsafe" "golang.org/x/sys/windows" + "golang.org/x/sys/windows/svc" "golang.org/x/sys/windows/svc/mgr" ) @@ -33,6 +35,65 @@ func hasElevatedPrivilege() (bool, error) { return token.IsMember(sid) } +// serviceLiveness is what could be established about the installed ctrld service. The +// three states are distinct because a caller that must not disturb a live service has to +// treat "could not tell" like "live", not like "stopped". +type serviceLiveness int + +const ( + // serviceLivenessUnknown means the question could not be answered: the SCM was + // unreachable, the caller lacked rights, or the query failed. + serviceLivenessUnknown serviceLiveness = iota + // serviceLivenessRunning means the service is running, starting, or paused - in every + // case a process that owns state. + serviceLivenessRunning + // serviceLivenessStopped means the service is installed and stopped, or not installed + // at all. Nothing of ctrld's is live. + serviceLivenessStopped +) + +// ctrldServiceLiveness reports what can be established about the installed ctrld service. +// +// Only serviceLivenessStopped is positive evidence that nothing is live. Every failure +// answers serviceLivenessUnknown rather than folding into "stopped": the SCM being +// unreachable says nothing about whether a service is running, and a caller that acts on +// that as absence would strip a live service's state. +// +// "Not installed" is deliberately stopped, not unknown: that is the answer, and it is +// exactly the host that needs stale state cleaned - an uninstall that left filters behind +// has no service left to protect. +func ctrldServiceLiveness() serviceLiveness { + m, err := mgr.Connect() + if err != nil { + return serviceLivenessUnknown + } + defer m.Disconnect() + + s, err := m.OpenService(ctrldServiceName) + if err != nil { + if errors.Is(err, windows.ERROR_SERVICE_DOES_NOT_EXIST) { + return serviceLivenessStopped + } + return serviceLivenessUnknown + } + defer s.Close() + + status, err := s.Query() + if err != nil { + return serviceLivenessUnknown + } + switch status.State { + case svc.Running, svc.StartPending, svc.ContinuePending, svc.PausePending, svc.Paused: + return serviceLivenessRunning + case svc.Stopped: + return serviceLivenessStopped + default: + // StopPending, and any state a later Windows adds: a process may still be + // holding its state, so this is no answer. + return serviceLivenessUnknown + } +} + // ConfigureWindowsServiceFailureActions checks if the given service // has the correct failure actions configured, and updates them if not. func ConfigureWindowsServiceFailureActions(serviceName string) error { diff --git a/docs/wfp-dns-intercept.md b/docs/wfp-dns-intercept.md index 60b5428..ada8225 100644 --- a/docs/wfp-dns-intercept.md +++ b/docs/wfp-dns-intercept.md @@ -247,9 +247,20 @@ by the VPN's own WFP rules. ``` **Crash Recovery:** -On startup, `FwpmSubLayerDeleteByKey0` removes any stale sublayer from a previous -unclean shutdown, including all its child filters (deterministic GUID ensures we -only clean up our own). +On startup, `cleanupStaleDNSInterceptState()` calls `FwpmSubLayerDeleteByKey0` to +remove any stale sublayer from a previous unclean shutdown (the deterministic GUID +ensures we only ever target our own). It opens a **non-dynamic** session, because a +dynamic one cannot delete objects that were not added in a dynamic session — which is +what a build predating session-scoped ownership leaves behind. + +Whether deleting the sublayer also removes the filters inside it is **not documented** +either way, and is unresolved (see the note on `cleanupStaleDNSInterceptState`). If it +does not, the call returns `FWP_E_IN_USE` and the orphaned filters need deleting first; +that case is logged as a warning rather than reported as a successful cleanup, so it is +visible in a support log instead of silently leaving enforcement active. + +Since every current build uses a dynamic session, the OS removes ctrld's filters when +the process dies, so this recovery path only matters for state left by an older build. ## NRPT Probe and Auto-Heal