mirror of
https://github.com/Control-D-Inc/ctrld.git
synced 2026-09-04 13:36:35 +02:00
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
181 lines
6.8 KiB
Go
181 lines
6.8 KiB
Go
//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)
|
|
}
|
|
}
|