fix(devicepolicy): repair legacy Windows state permissions

cmd/stepsecurity-dev-machine-guard/main.go

@@ -34,6 +34,7 @@ import (
 	"github.com/step-security/dev-machine-guard/internal/rungate"
 	"github.com/step-security/dev-machine-guard/internal/scan"
 	"github.com/step-security/dev-machine-guard/internal/schtasks"
+	"github.com/step-security/dev-machine-guard/internal/secureuserfile"
 	"github.com/step-security/dev-machine-guard/internal/systemd"
 	"github.com/step-security/dev-machine-guard/internal/tcc"
 	"github.com/step-security/dev-machine-guard/internal/telemetry"
@@ -799,11 +800,15 @@ func runHookStateReconcile(exec executor.Executor, log *progress.Logger, runCfg
 const devicePolicyEnforceTimeout = 30 * time.Second
 
 func resolveDevicePolicyTarget(exec executor.Executor, log *progress.Logger) (executor.Executor, func(), bool) {
-	targetExec, restore, err := devicepolicy.ConfigureCacheTarget(exec)
-	if err != nil {
+	targetExec, restore, err := devicepolicy.ConfigureCacheTarget(exec, log.Warn)
+	if errors.Is(err, secureuserfile.ErrNoTargetUser) {
 		log.Debug("device-policy enforce: no active target user; preserving user-scoped state")
 		return nil, func() {}, false
 	}
+	if err != nil {
+		log.Warn("device-policy enforce: target setup failed: %s", redact.String(err.Error()))
+		return nil, func() {}, false
+	}
 	return targetExec, restore, true
 }
 

cmd/stepsecurity-dev-machine-guard/main_devicepolicy_test.go

@@ -3,6 +3,7 @@ package main
 import (
 	"context"
 	"errors"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -203,6 +204,79 @@ func TestPackageConfigLanes_UseSeparateTimeoutContexts(t *testing.T) {
 	}
 }
 
+type failingTargetExecutor struct {
+	*executor.Mock
+	user *user.User
+	err  error
+}
+
+func (e failingTargetExecutor) LoggedInUser() (*user.User, error) { return e.user, e.err }
+
+func captureDevicePolicyStderr(t *testing.T, fn func()) string {
+	t.Helper()
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	orig := os.Stderr
+	os.Stderr = w
+	done := make(chan string)
+	go func() {
+		b, _ := io.ReadAll(r)
+		done <- string(b)
+	}()
+	fn()
+	_ = w.Close()
+	os.Stderr = orig
+	out := <-done
+	_ = r.Close()
+	return out
+}
+
+func TestResolveDevicePolicyTargetLogsOnlyUnexpectedFailures(t *testing.T) {
+	current, err := user.Current()
+	if err != nil {
+		t.Fatal(err)
+	}
+	// Synthetic token-shaped path segment: the setup error quotes the home path,
+	// and agent.error.log must never carry it unredacted.
+	secret := "ghp_" + strings.Repeat("Z", 24)
+	missingHome := *current
+	missingHome.HomeDir = filepath.Join(t.TempDir(), secret)
+	tests := []struct {
+		name     string
+		user     *user.User
+		err      error
+		wantWarn bool
+	}{
+		{"no active user stays quiet", nil, errors.New("no console session"), false},
+		{"unusable home warns redacted", &missingHome, nil, true},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			mock := executor.NewMock()
+			mock.SetGOOS(model.PlatformWindows)
+			exec := failingTargetExecutor{Mock: mock, user: tc.user, err: tc.err}
+			var ok bool
+			out := captureDevicePolicyStderr(t, func() {
+				_, _, ok = resolveDevicePolicyTarget(exec, progress.NewLogger(progress.LevelInfo))
+			})
+			if ok {
+				t.Fatal("resolveDevicePolicyTarget succeeded, want failure")
+			}
+			if got := strings.Contains(out, "device-policy enforce: target setup failed"); got != tc.wantWarn {
+				t.Fatalf("warned = %v, want %v; output %q", got, tc.wantWarn, out)
+			}
+			if !tc.wantWarn && out != "" {
+				t.Fatalf("output = %q, want none at info level", out)
+			}
+			if strings.Contains(out, secret) {
+				t.Fatalf("output leaked synthetic secret: %q", out)
+			}
+		})
+	}
+}
+
 func TestWSLGuestPolicyRequestsUseGuestIdentity(t *testing.T) {
 	old := config.DeviceID
 	t.Cleanup(func() { config.DeviceID = old })

internal/devicepolicy/cache.go

@@ -10,6 +10,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/step-security/dev-machine-guard/internal/aiagents/redact"
 	"github.com/step-security/dev-machine-guard/internal/executor"
 	"github.com/step-security/dev-machine-guard/internal/model"
 	"github.com/step-security/dev-machine-guard/internal/secureuserfile"
@@ -110,6 +111,12 @@ var cachePathOverride string
 var cacheStateFile *secureuserfile.File
 var cacheLockFile *secureuserfile.File
 
+// cacheWarnf receives state failures that ReadAppliedState swallows, so a cycle
+// that silently reads "owns nothing" still leaves a trace. cacheLastWarn keeps
+// the several per-cycle readers from repeating one line.
+var cacheWarnf func(format string, args ...any)
+var cacheLastWarn string
+
 type targetUserExecutor struct {
 	executor.Executor
 	user *user.User
@@ -122,7 +129,8 @@ func (e targetUserExecutor) LoggedInUser() (*user.User, error) {
 
 // ConfigureCacheTarget pins Windows state to the same active target user used
 // by package writers. Other platforms retain their existing process-user path.
-func ConfigureCacheTarget(exec executor.Executor) (executor.Executor, func(), error) {
+// warnf receives redacted state failures that reads would otherwise swallow.
+func ConfigureCacheTarget(exec executor.Executor, warnf func(string, ...any)) (executor.Executor, func(), error) {
 	if exec == nil || exec.GOOS() != model.PlatformWindows {
 		return exec, func() {}, nil
 	}
@@ -153,14 +161,17 @@ func ConfigureCacheTarget(exec executor.Executor) (executor.Executor, func(), er
 	target := targetUserExecutor{Executor: exec, user: home.User()}
 	cacheMu.Lock()
 	previousPath, previousStateFile, previousLockFile := cachePathOverride, cacheStateFile, cacheLockFile
+	previousWarnf, previousLastWarn := cacheWarnf, cacheLastWarn
 	cachePathOverride = filepath.Join(home.Path(), ".stepsecurity", CacheFilename)
 	cacheStateFile = stateFile
 	cacheLockFile = lockFile
+	cacheWarnf, cacheLastWarn = warnf, ""
 	cacheMu.Unlock()
 	return target, func() {
 		cacheMu.Lock()
 		cachePathOverride = previousPath
 		cacheStateFile, cacheLockFile = previousStateFile, previousLockFile
+		cacheWarnf, cacheLastWarn = previousWarnf, previousLastWarn
 		cacheMu.Unlock()
 		_ = home.Close()
 	}, nil
@@ -258,7 +269,7 @@ func readStateFile() (AppliedStateFile, readStatus, error) {
 		secure, err := cacheStateFile.MetadataSecure(cacheFileMode)
 		if err != nil || !secure {
 			if err == nil {
-				err = fmt.Errorf("devicepolicy: insecure cache metadata: %w", secureuserfile.ErrTargetUnusable)
+				err = fmt.Errorf("%w: %w", errInsecureStateMetadata, secureuserfile.ErrTargetUnusable)
 			}
 			return AppliedStateFile{}, stateUnreadable, err
 		}
@@ -303,6 +314,35 @@ func readStateFile() (AppliedStateFile, readStatus, error) {
 	return f, stateReadable, nil
 }
 
+// prepareStateFile restores the strict permission boundary on a state file left
+// with a broader DACL by an older agent (1.15/1.16 inherited their parent's), so
+// its records stay readable instead of being refused. Permissions only: bytes,
+// owner and every category's records are preserved, and a wrong owner is still
+// refused before anything changes. Absent state is left absent. UNLOCKED: callers
+// hold cacheMu and the state lock, so no peer can rewrite the file mid-repair.
+func prepareStateFile() error {
+	if cacheStateFile == nil {
+		return nil
+	}
+	present, err := cacheStateFile.ParentPresent()
+	if err != nil || !present {
+		return err
+	}
+	if err := cacheStateFile.RepairMetadata(cacheFileMode); err != nil {
+		return fmt.Errorf("devicepolicy: repair state permissions: %w", err)
+	}
+	return nil
+}
+
+// readPreparedStateFile is readStateFile after prepareStateFile, for callers
+// holding both locks. A failed repair is stateUnreadable, so mutators refuse.
+func readPreparedStateFile() (AppliedStateFile, readStatus, error) {
+	if err := prepareStateFile(); err != nil {
+		return AppliedStateFile{}, stateUnreadable, err
+	}
+	return readStateFile()
+}
+
 // ReadAppliedState returns the agent's recorded ownership for one
 // (category, target): (state, true) when a record exists, else (zero, false).
 // An empty target defaults to vscode. It never surfaces an error — a
@@ -313,26 +353,68 @@ func readStateFile() (AppliedStateFile, readStatus, error) {
 // re-applies the policy. Only the MUTATING accessors distinguish unreadable from
 // absent, because only they can destroy what they could not read.
 //
-// It takes no file lock: a lone read modifies nothing, and every write lands by
+// A plain read takes no file lock: it modifies nothing, and every write lands by
 // atomic rename, so a reader sees one complete generation of the file or another —
-// never a half-written one.
+// never a half-written one. Only state an older agent left with broader
+// permissions takes the cross-process lock, to repair it before reading again;
+// absent state never does, so a read creates nothing. A swallowed unreadable
+// state is reported once per target through cacheWarnf, redacted.
 func ReadAppliedState(category, target string) (AppliedTargetState, bool) {
+	s, ok, err := readAppliedStateChecked(category, target)
+	if err != nil {
+		cacheMu.Lock()
+		warnStateRead(err)
+		cacheMu.Unlock()
+	}
+	return s, ok
+}
+
+// readAppliedStateChecked is ReadAppliedState without the warning, returning the
+// error of an unreadable Windows target store (including a failed legacy
+// permission repair) for a caller that must not treat it as owning nothing.
+// Absent, corrupt and future-schema state return a nil error, as does any read
+// on other platforms, whose behavior is unchanged.
+func readAppliedStateChecked(category, target string) (AppliedTargetState, bool, error) {
 	cacheMu.Lock()
 	defer cacheMu.Unlock()
 
 	if target == "" {
 		target = TargetVSCode
 	}
-	f, status, _ := readStateFile()
+	f, status, err := readStateFile()
+	if errors.Is(err, errInsecureStateMetadata) {
+		// A busy lock leaves the first read's stateUnreadable in place.
+		err = withStateLock(func() error {
+			var rerr error
+			f, status, rerr = readPreparedStateFile()
+			return rerr
+		})
+	}
+	if status == stateUnreadable && cacheStateFile != nil {
+		return AppliedTargetState{}, false, err
+	}
 	if status != stateReadable {
-		return AppliedTargetState{}, false
+		return AppliedTargetState{}, false, nil
 	}
 	cat, ok := f.Categories[category]
 	if !ok {
-		return AppliedTargetState{}, false
+		return AppliedTargetState{}, false, nil
 	}
 	s, ok := cat.Targets[target]
-	return s, ok
+	return s, ok, nil
+}
+
+// warnStateRead reports a swallowed read failure. Callers hold cacheMu.
+func warnStateRead(err error) {
+	if cacheWarnf == nil || err == nil {
+		return
+	}
+	msg := redact.String(err.Error())
+	if msg == cacheLastWarn {
+		return
+	}
+	cacheLastWarn = msg
+	cacheWarnf("devicepolicy: read applied state: %s", msg)
 }
 
 // WriteAppliedState records ownership for one (category, target), PRESERVING
@@ -358,7 +440,7 @@ func WriteAppliedState(category, target string, s AppliedTargetState) error {
 		target = TargetVSCode
 	}
 	return withStateLock(func() error {
-		f, status, rerr := readStateFile()
+		f, status, rerr := readPreparedStateFile()
 		switch status {
 		case stateUnreadable:
 			return rerr
@@ -387,7 +469,7 @@ func ProbeAppliedStateWritable() error {
 	defer cacheMu.Unlock()
 
 	return withStateLock(func() error {
-		_, status, err := readStateFile()
+		_, status, err := readPreparedStateFile()
 		switch status {
 		case stateUnreadable:
 			return err
@@ -451,7 +533,7 @@ func ClearAppliedState(category, target string) error {
 		target = TargetVSCode
 	}
 	return withStateLock(func() error {
-		f, status, rerr := readStateFile()
+		f, status, rerr := readPreparedStateFile()
 		switch status {
 		case stateUnreadable:
 			return rerr
@@ -586,6 +668,9 @@ func (e cacheError) Error() string { return string(e) }
 const (
 	errNoHomeDir    = cacheError("devicepolicy: cannot resolve home directory")
 	errFutureSchema = cacheError("devicepolicy: refusing to overwrite a newer-schema state file")
+	// errInsecureStateMetadata: the state file's permissions are broader than the
+	// strict contract, as an older agent left them; the locked paths repair it.
+	errInsecureStateMetadata = cacheError("devicepolicy: insecure cache metadata")
 	// errStateLockBusy: a peer agent process held the state lock for the whole wait
 	// budget. The read-modify-write is abandoned rather than run unlocked — see
 	// withStateLock for why that trade is deliberate.

internal/devicepolicy/cache_test.go

@@ -2,6 +2,7 @@ package devicepolicy
 
 import (
 	"bytes"
+	"context"
 	"encoding/json"
 	"errors"
 	"os"
@@ -31,7 +32,7 @@ func TestConfigureCacheTargetWithoutWindowsUserPreservesState(t *testing.T) {
 	mock := executor.NewMock()
 	mock.SetGOOS(model.PlatformWindows)
 	mock.SetLoggedInUserError(errors.New("session 0"))
-	if target, restore, err := ConfigureCacheTarget(mock); err == nil || restore != nil || target != nil {
+	if target, restore, err := ConfigureCacheTarget(mock, nil); err == nil || restore != nil || target != nil {
 		t.Fatalf("ConfigureCacheTarget returned target=%t, restore=%t, err=%v; want no target error", target != nil, restore != nil, err)
 	}
 	after, err := os.ReadFile(path)
@@ -57,7 +58,7 @@ func TestConfiguredCacheRejectsRedirectedStateParent(t *testing.T) {
 	normalizeSecureTestUser(t, u)
 	mock := executor.NewMock()
 	mock.SetGOOS(model.PlatformWindows)
-	target, restore, err := ConfigureCacheTarget(secureTestExecutor{Executor: mock, user: u})
+	target, restore, err := ConfigureCacheTarget(secureTestExecutor{Executor: mock, user: u}, nil)
 	if err == nil || target != nil || restore != nil {
 		t.Fatalf("ConfigureCacheTarget returned target=%t restore=%t err=%v, want redirected-parent refusal", target != nil, restore != nil, err)
 	}
@@ -844,6 +845,23 @@ func TestClearRefusesUnreadableFile(t *testing.T) {
 	assertStateFileUnchanged(t, path, before)
 }
 
+// TestMarkerClearStillRemovesBlockWithUnreadableState pins the non-Windows side
+// of the Windows-only clear gate: the token-bearing block is still removed, and
+// only the record drop reports the unreadable store.
+func TestMarkerClearStillRemovesBlockWithUnreadableState(t *testing.T) {
+	path, before := seedUnreadableState(t)
+	w := &fakeWriter{value: "block", present: true}
+	r, _ := newNPMRec(t, EffectivePolicy{Category: CategoryPackageConfig, Target: TargetNPM, Clear: true}, w, &npmStore{path: path})
+
+	if err := r.Reconcile(context.Background()); err == nil {
+		t.Fatal("clear with an unreadable state file must report the failed record drop")
+	}
+	if w.clears != 1 || w.present {
+		t.Fatalf("managed block clears=%d present=%v, want removed once", w.clears, w.present)
+	}
+	assertStateFileUnchanged(t, path, before)
+}
+
 func TestUnreadableFileStillReadsAsOwnsNothing(t *testing.T) {
 	// Only the mutating accessors changed. A plain read keeps its no-error contract
 	// and reports owning nothing, which is the safe direction: the reconciler then