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