diff --git a/cmd/stepsecurity-dev-machine-guard/main.go b/cmd/stepsecurity-dev-machine-guard/main.go index 0fc03b19..65497448 100644 --- a/cmd/stepsecurity-dev-machine-guard/main.go +++ b/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 } diff --git a/cmd/stepsecurity-dev-machine-guard/main_devicepolicy_test.go b/cmd/stepsecurity-dev-machine-guard/main_devicepolicy_test.go index b9aecd3e..1f04e53c 100644 --- a/cmd/stepsecurity-dev-machine-guard/main_devicepolicy_test.go +++ b/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 }) diff --git a/internal/devicepolicy/cache.go b/internal/devicepolicy/cache.go index c574ddad..170648ba 100644 --- a/internal/devicepolicy/cache.go +++ b/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. diff --git a/internal/devicepolicy/cache_test.go b/internal/devicepolicy/cache_test.go index 80157ce3..97291719 100644 --- a/internal/devicepolicy/cache_test.go +++ b/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 diff --git a/internal/devicepolicy/cache_windows_test.go b/internal/devicepolicy/cache_windows_test.go index dc219a50..64ec5215 100644 --- a/internal/devicepolicy/cache_windows_test.go +++ b/internal/devicepolicy/cache_windows_test.go @@ -3,13 +3,19 @@ package devicepolicy import ( + "context" + "encoding/json" + "errors" + "fmt" "os" "os/user" "path/filepath" + "strings" "testing" "github.com/step-security/dev-machine-guard/internal/executor" "github.com/step-security/dev-machine-guard/internal/secureuserfile" + "golang.org/x/sys/windows" ) func TestAppliedStateWindowsUsesTargetUserSecurity(t *testing.T) { @@ -20,7 +26,7 @@ func TestAppliedStateWindowsUsesTargetUserSecurity(t *testing.T) { } u.HomeDir = homeDir normalizeSecureTestUser(t, u) - target, restore, err := ConfigureCacheTarget(secureTestExecutor{Executor: executor.NewReal(), user: u}) + target, restore, err := ConfigureCacheTarget(secureTestExecutor{Executor: executor.NewReal(), user: u}, nil) if err != nil { t.Fatal(err) } @@ -60,3 +66,337 @@ func TestAppliedStateWindowsUsesTargetUserSecurity(t *testing.T) { } } } + +// The legacy fixtures are what the retained 1.15 release driver +// (dmg-windows-state-v115-repro, state-repro seed) leaves behind: an IDE sibling +// record plus the npm record and block, with a synthetic token. 1.15 records the +// rendered block itself under written_settings.npmrc, has none of the newer +// file_created/resolved_path fields, and writes the state without the strict +// DACL, so it inherits its parent's ACL. +const ( + legacyNPMPolicy = `{"ecosystem":"npm","registry_url":"https://registry.example.com/javascript","auth":{"scheme":"stepsecurity_device_token","api_key":"testkey"}}` + legacyNPMHash = "sha256:1111111111111111111111111111111111111111111111111111111111111111" + legacyIDEHash = "sibling-unchanged" + legacyNPMRC = "# unrelated user setting\nfund=false\n" + + "# BEGIN StepSecurity Secure Registry -- managed by dmg\n" + + "registry=https://registry.example.com/javascript\n" + + "//registry.example.com/javascript/:_authToken=testkey::dev:test-device\n" + + "# END StepSecurity Secure Registry\n" +) + +const legacyStateFixture = `{ + "schema_version": 1, + "categories": { + "ide_extension": { + "targets": { + "vscode": {"applied_hash": "sibling-unchanged", "written_settings": {"test": "preserve"}, "fetched_at": "2026-09-01T00:00:00Z"} + } + }, + "package_config": { + "targets": { + "npm": {"applied_hash": "sha256:1111111111111111111111111111111111111111111111111111111111111111", "written_settings": {"npmrc": "registry=https://registry.example.com/javascript\n//registry.example.com/javascript/:_authToken=testkey::dev:test-device"}, "fetched_at": "2026-09-01T00:00:00Z"} + } + } + } +} +` + +// configureLegacyWindowsState pins the cache to a temporary home seeded with +// the legacy fixture and an inherited-ACL lock, and returns the state path, a +// sink of the warnings ReadAppliedState emits, and the target executor. +// targetSID overrides the target user's SID when non-empty. +func configureLegacyWindowsState(t *testing.T, seed bool, targetSID string) (string, *[]string, executor.Executor) { + t.Helper() + homeDir := t.TempDir() + u, err := user.Current() + if err != nil { + t.Fatal(err) + } + u.HomeDir = homeDir + normalizeSecureTestUser(t, u) + ownerUID := u.Uid + if targetSID != "" { + u.Uid = targetSID + } + path := filepath.Join(homeDir, ".stepsecurity", CacheFilename) + if seed { + if err := os.Mkdir(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + // The 1.15 lab state granted the owner only inherited Modify, so a + // standard user cannot assign ownership on the directory, lock or state. + setInheritedModifyOnly(t, filepath.Dir(path), ownerUID) + for name, data := range map[string]string{path: legacyStateFixture, path + stateLockSuffix: ""} { + if err := os.WriteFile(name, []byte(data), 0o600); err != nil { + t.Fatal(err) + } + } + } + var warnings []string + warnf := func(format string, args ...any) { warnings = append(warnings, fmt.Sprintf(format, args...)) } + target, restore, err := ConfigureCacheTarget(secureTestExecutor{Executor: executor.NewReal(), user: u}, warnf) + if err != nil { + t.Fatal(err) + } + t.Cleanup(restore) + // A foreign target cannot inspect the fixture's metadata at all; only the + // correctly owned fixture must start out readable but insecure. + if seed && targetSID == "" { + if secure, err := cacheStateFile.MetadataSecure(cacheFileMode); err != nil || secure { + t.Fatalf("legacy fixture metadata = %v, %v, want insecure", secure, err) + } + } + return path, &warnings, target +} + +func requireStateBytes(t *testing.T, path, want string) { + t.Helper() + got, err := os.ReadFile(path) + if err != nil || string(got) != want { + t.Fatalf("state bytes = %q, %v, want unchanged legacy fixture", got, err) + } +} + +func requireRepairedQuietly(t *testing.T, warnings []string) { + t.Helper() + if secure, err := cacheStateFile.MetadataSecure(cacheFileMode); err != nil || !secure { + t.Fatalf("repaired state metadata = %v, %v, want secure", secure, err) + } + if len(warnings) != 0 { + t.Fatalf("warnings = %q, want none", warnings) + } +} + +func requireRecord(t *testing.T, category, target, wantHash string) { + t.Helper() + got, ok := ReadAppliedState(category, target) + if !ok || got.AppliedHash != wantHash { + t.Fatalf("ReadAppliedState(%s, %s) = %+v, %v, want hash %s", category, target, got, ok, wantHash) + } +} + +func TestAppliedStateWindowsRepairsLegacyStateFromEveryEntryPoint(t *testing.T) { + tests := []struct { + name string + first func(t *testing.T, path string) + }{ + {"read", func(t *testing.T, path string) { + requireRecord(t, CategoryPackageConfig, TargetNPM, legacyNPMHash) + requireStateBytes(t, path, legacyStateFixture) + }}, + {"write", func(t *testing.T, _ string) { + if err := WriteAppliedState(CategoryPackageConfig, TargetNPM, AppliedTargetState{AppliedHash: "sha256:npm-next"}); err != nil { + t.Fatalf("WriteAppliedState: %v", err) + } + requireRecord(t, CategoryPackageConfig, TargetNPM, "sha256:npm-next") + }}, + {"probe", func(t *testing.T, path string) { + if err := ProbeAppliedStateWritable(); err != nil { + t.Fatalf("ProbeAppliedStateWritable: %v", err) + } + requireStateBytes(t, path, legacyStateFixture) + }}, + {"clear", func(t *testing.T, _ string) { + if err := ClearAppliedState(CategoryPackageConfig, TargetNPM); err != nil { + t.Fatalf("ClearAppliedState: %v", err) + } + if got, ok := ReadAppliedState(CategoryPackageConfig, TargetNPM); ok { + t.Fatalf("cleared npm record still present: %+v", got) + } + }}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + path, warnings, _ := configureLegacyWindowsState(t, true, "") + tc.first(t, path) + requireRecord(t, CategoryIDEExtension, TargetVSCode, legacyIDEHash) + requireRepairedQuietly(t, *warnings) + }) + } +} + +func TestAppliedStateWindowsLegacyStateWaitsForPeerLock(t *testing.T) { + path, warnings, exec := configureLegacyWindowsState(t, true, "") + npmPath := filepath.Join(filepath.Dir(filepath.Dir(path)), ".npmrc") + if err := os.WriteFile(npmPath, []byte(legacyNPMRC), 0o600); err != nil { + t.Fatal(err) + } + holdStateLockUntilCleanup(t) + + for range 2 { + if got, ok := ReadAppliedState(CategoryPackageConfig, TargetNPM); ok { + t.Fatalf("read without the lock returned %+v, want not owned", got) + } + } + if len(*warnings) != 1 || !strings.Contains((*warnings)[0], string(errStateLockBusy)) { + t.Fatalf("warnings = %q, want one lock-busy warning", *warnings) + } + if err := WriteAppliedState(CategoryPackageConfig, TargetNPM, AppliedTargetState{AppliedHash: "sha256:x"}); !errors.Is(err, errStateLockBusy) { + t.Fatalf("WriteAppliedState error = %v, want errStateLockBusy", err) + } + if secure, err := cacheStateFile.MetadataSecure(cacheFileMode); err != nil || secure { + t.Fatalf("state metadata = %v, %v, want unrepaired while a peer holds the lock", secure, err) + } + // A clear that cannot repair and read its record must not remove the block. + if err := runLegacyNPMLane(exec, &fakeReporter{}, true); !errors.Is(err, errStateLockBusy) { + t.Fatalf("npm clear error = %v, want errStateLockBusy", err) + } + if got, err := os.ReadFile(npmPath); err != nil || string(got) != legacyNPMRC { + t.Fatalf("npmrc after refused clear = %q, %v, want unchanged", got, err) + } + requireStateBytes(t, path, legacyStateFixture) +} + +func TestAppliedStateWindowsWrongOwnerIsReportedNotRepaired(t *testing.T) { + systemSID, err := windows.CreateWellKnownSid(windows.WinLocalSystemSid) + if err != nil { + t.Fatal(err) + } + path, warnings, _ := configureLegacyWindowsState(t, true, systemSID.String()) + if ownerSID(t, path).Equals(systemSID) { + t.Skip("fixture is already owned by SYSTEM") + } + before := ownerSID(t, path) + + if _, ok := ReadAppliedState(CategoryPackageConfig, TargetNPM); ok { + t.Fatal("wrong-owner state read as owned") + } + if len(*warnings) != 1 || !strings.Contains((*warnings)[0], "devicepolicy: read applied state:") { + t.Fatalf("warnings = %q, want one read failure", *warnings) + } + if err := ClearAppliedState(CategoryPackageConfig, TargetNPM); !errors.Is(err, secureuserfile.ErrTargetUnusable) { + t.Fatalf("ClearAppliedState error = %v, want ErrTargetUnusable", err) + } + requireStateBytes(t, path, legacyStateFixture) + if after := ownerSID(t, path); !after.Equals(before) { + t.Fatalf("owner changed from %s to %s", before, after) + } +} + +func TestAppliedStateWindowsReadWithoutStateCreatesNothing(t *testing.T) { + path, warnings, _ := configureLegacyWindowsState(t, false, "") + if _, ok := ReadAppliedState(CategoryPackageConfig, TargetNPM); ok { + t.Fatal("absent state read as owned") + } + if _, err := os.Lstat(filepath.Dir(path)); !os.IsNotExist(err) { + t.Fatalf("state directory created by a read: %v", err) + } + if len(*warnings) != 0 { + t.Fatalf("warnings = %q, want none", *warnings) + } +} + +func setInheritedModifyOnly(t *testing.T, dir, ownerUID string) { + t.Helper() + sd, err := windows.SecurityDescriptorFromString("D:P(A;OICI;0x1301bf;;;" + ownerUID + ")(A;OICI;FA;;;SY)") + if err != nil { + t.Fatal(err) + } + acl, _, err := sd.DACL() + if err != nil { + t.Fatal(err) + } + if err := windows.SetNamedSecurityInfo(dir, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION, nil, nil, acl, nil); err != nil { + t.Fatal(err) + } +} + +// TestAppliedStateWindowsLegacyNPMLaneRepairsAndKeepsSibling drives the npm lane +// over the 1.15 fixture: enforcement and removal each repair the state first, +// remove only the owned block and record, and keep the IDE sibling. Clear as the +// first operation after the upgrade must still restore the user's file. +func TestAppliedStateWindowsLegacyNPMLaneRepairsAndKeepsSibling(t *testing.T) { + tests := []struct { + name string + steps []bool // true = clear + }{ + {"apply repeat clear", []bool{false, false, true, true}}, + {"clear first", []bool{true, true}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + path, warnings, exec := configureLegacyWindowsState(t, true, "") + npmPath := filepath.Join(filepath.Dir(filepath.Dir(path)), ".npmrc") + if err := os.WriteFile(npmPath, []byte(legacyNPMRC), 0o600); err != nil { + t.Fatal(err) + } + for i, clear := range tc.steps { + rep := &fakeReporter{} + if err := runLegacyNPMLane(exec, rep, clear); err != nil { + t.Fatalf("step %d (clear=%v): %v", i, clear, err) + } + npm, err := os.ReadFile(npmPath) + if err != nil || !strings.Contains(string(npm), "fund=false") { + t.Fatalf("step %d: unrelated npm setting lost: %v", i, err) + } + managed := strings.Contains(string(npm), "registry=https://registry.example.com/javascript") + _, owned := ReadAppliedState(CategoryPackageConfig, TargetNPM) + if clear { + if managed || owned { + t.Fatalf("step %d: after clear block=%v record=%v, want both gone", i, managed, owned) + } + } else { + if !managed || !owned || len(rep.reports) == 0 { + t.Fatalf("step %d: after apply block=%v record=%v reports=%d", i, managed, owned, len(rep.reports)) + } + if got := rep.reports[len(rep.reports)-1].State; got != StateCompliant && got != StateDriftDetected { + t.Fatalf("step %d: report state = %q, want compliant or drift_detected", i, got) + } + } + requireRecord(t, CategoryIDEExtension, TargetVSCode, legacyIDEHash) + requireRepairedQuietly(t, *warnings) + } + }) + } +} + +// runLegacyNPMLane is one npm reconcile cycle, wired as main wires the lane. +func runLegacyNPMLane(exec executor.Executor, rep *fakeReporter, clear bool) error { + ep := EffectivePolicy{Category: CategoryPackageConfig, Target: TargetNPM, Hash: legacyNPMHash, Enforcement: "dmg"} + if clear { + ep.Clear = true + } else { + ep.Policy = json.RawMessage(legacyNPMPolicy) + } + var w *NPMRCWriter + r := &Reconciler{ + Fetcher: &fakeFetcher{ep: ep}, Reporter: rep, CustomerID: "test-org", DeviceID: "test-device", + Platform: "windows", Category: CategoryPackageConfig, Target: TargetNPM, + Render: func(raw json.RawMessage) (string, error) { return RenderNPMRCBlock(raw, "test-device") }, + OwnsByMarker: true, + OwnershipKey: NPMOwnedKey, + OwnershipStateValue: NPMOwnershipValue, + } + r.InitWriter = func() error { + var err error + if w, err = NewNPMRCWriter(exec); err != nil { + return err + } + r.Writer = w + r.Converged = w.Converged + r.CompleteState = w.CompleteState + r.PrepareClear = w.PrepareClear + r.ProbeExpected = w.ProbeExpected + r.RestoreSnapshot = w.RestoreSnapshot + r.ProbeContent = w.ProbeContentNPM + return nil + } + err := r.Reconcile(context.Background()) + if w != nil { + _ = w.Close() + } + return err +} + +func ownerSID(t *testing.T, path string) *windows.SID { + t.Helper() + descriptor, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.OWNER_SECURITY_INFORMATION) + if err != nil { + t.Fatal(err) + } + owner, _, err := descriptor.Owner() + if err != nil || owner == nil { + t.Fatalf("owner(%q): %v", path, err) + } + return owner +} diff --git a/internal/devicepolicy/reconcile.go b/internal/devicepolicy/reconcile.go index 23f05190..19cb0b46 100644 --- a/internal/devicepolicy/reconcile.go +++ b/internal/devicepolicy/reconcile.go @@ -675,11 +675,14 @@ func (r *Reconciler) dropClearedState(cat, tgt string, hadPrev bool) error { // anything else — so a value-equality gate is both unnecessary and unsafe here: // lost or corrupt state, a drifted block, or an empty marker shell would // otherwise strand a token-bearing block on disk forever after unassignment. -// Clear is called unconditionally (a no-op when there is no block) and the state -// record is dropped UNCONDITIONALLY afterward — a store read that failed or lied -// (no record found) must not leave an orphan behind; Drop is idempotent. +// Missing or corrupt state still clears the block and then drops the record +// (Drop is idempotent). Only an unreadable Windows state store, such as a failed +// legacy permission repair, refuses before touching the block. func (r *Reconciler) handleClearByMarker(cat, tgt string) error { - prev, hadPrev := r.readState(cat) + prev, hadPrev, err := readAppliedStateChecked(cat, r.stateTarget()) + if err != nil { + return fmt.Errorf("devicepolicy: clear: read state: %w", err) + } if r.PrepareClear != nil { if err := r.PrepareClear(prev, hadPrev); err != nil { return fmt.Errorf("devicepolicy: prepare clear %s: %w", r.Writer.Location(), err) diff --git a/internal/secureuserfile/file.go b/internal/secureuserfile/file.go index 33b35d41..f43555f2 100644 --- a/internal/secureuserfile/file.go +++ b/internal/secureuserfile/file.go @@ -286,7 +286,7 @@ func (h *Home) EnsureParent(relativePath string) error { if !created && h.goos == model.PlatformWindows && i == len(components)-1 { secure, err := h.metadata.secure(handle, h, mode) if err == nil && !secure { - err = h.applyMetadata(h, handle, mode, true) + err = repairSecureMetadata(h, handle, mode, true) if err == nil { secure, err = h.metadata.secure(handle, h, mode) if err == nil && !secure { @@ -639,6 +639,29 @@ func (f *File) MetadataSecure(want os.FileMode) (bool, error) { return f.home.metadata.secure(file, f.home, want) } +// RepairMetadata restores the platform permission boundary on an existing leaf +// already owned by the target user, changing permissions only: bytes and owner +// are untouched, and an absent or already-secure leaf is left as is. +func (f *File) RepairMetadata(want os.FileMode) error { + file, err := f.openMetadata() + if err != nil || file == nil { + return err + } + defer file.Close() + secure, err := f.home.metadata.secure(file, f.home, want) + if err != nil || secure { + return err + } + if err := repairSecureMetadata(f.home, file, want, false); err != nil { + return err + } + secure, err = f.home.metadata.secure(file, f.home, want) + if err == nil && !secure { + err = fmt.Errorf("secure user file: repaired metadata remains insecure: %w", ErrTargetUnusable) + } + return err +} + func (f *File) openMetadata() (*os.File, error) { rt, err := f.resolveLeaf() if err != nil { @@ -849,12 +872,12 @@ func (f *File) OpenLock() (*os.File, error) { } return nil, cause } - if !created { - if err := f.home.VerifyOwner(file, rt.base); err != nil { - return fail(err) - } + if created { + err = f.applyMetadata(file, FileMode, false) + } else if err = f.home.VerifyOwner(file, rt.base); err == nil { + err = repairSecureMetadata(f.home, file, FileMode, false) } - if err := f.applyMetadata(file, FileMode, false); err != nil { + if err != nil { return fail(err) } if err := f.home.VerifyOwner(file, rt.base); err != nil { diff --git a/internal/secureuserfile/file_unix.go b/internal/secureuserfile/file_unix.go index 0c42bc0c..bce2cd01 100644 --- a/internal/secureuserfile/file_unix.go +++ b/internal/secureuserfile/file_unix.go @@ -38,6 +38,12 @@ func applySecureMetadata(h *Home, f *os.File, mode os.FileMode, _ bool) error { return nil } +// repairSecureMetadata is applySecureMetadata on POSIX: fchown to the already +// verified owner is a no-op, so no extra privilege is requested. +func repairSecureMetadata(h *Home, f *os.File, mode os.FileMode, directory bool) error { + return applySecureMetadata(h, f, mode, directory) +} + func checkSecurePlatformOwner(_ *Home, _ *os.File) error { return nil } func newOwnerReader() ownerReader { return unixOwnerReader{} } diff --git a/internal/secureuserfile/file_windows.go b/internal/secureuserfile/file_windows.go index ffc27c01..35dda2d8 100644 --- a/internal/secureuserfile/file_windows.go +++ b/internal/secureuserfile/file_windows.go @@ -40,6 +40,17 @@ func (windowsOwnerReader) ownerUIDGID(*os.File) (uint32, uint32, bool, error) { } func applySecureMetadata(h *Home, f *os.File, _ os.FileMode, directory bool) error { + return setSecureDACL(h, f, directory, true) +} + +// repairSecureMetadata rewrites only the DACL of an object whose owner is +// already verified. The owner's implicit READ_CONTROL|WRITE_DAC suffices, so a +// legacy object granting the user only Modify (no WRITE_OWNER) stays repairable. +func repairSecureMetadata(h *Home, f *os.File, _ os.FileMode, directory bool) error { + return setSecureDACL(h, f, directory, false) +} + +func setSecureDACL(h *Home, f *os.File, directory, assignOwner bool) error { targetSID, err := windows.StringToSid(h.targetUser.Uid) if err != nil { return fmt.Errorf("secure user file: target SID: %w", err) @@ -60,7 +71,15 @@ func applySecureMetadata(h *Home, f *os.File, _ os.FileMode, directory bool) err if err != nil { return fmt.Errorf("secure user file: build ACL: %w", err) } - handle, err := reopenSecurityHandle(f, windows.READ_CONTROL|windows.WRITE_DAC|windows.WRITE_OWNER) + access := uint32(windows.READ_CONTROL | windows.WRITE_DAC) + info := windows.SECURITY_INFORMATION(windows.DACL_SECURITY_INFORMATION | windows.PROTECTED_DACL_SECURITY_INFORMATION) + var owner *windows.SID + if assignOwner { + access |= windows.WRITE_OWNER + info |= windows.OWNER_SECURITY_INFORMATION + owner = targetSID + } + handle, err := reopenSecurityHandle(f, access) if err != nil { return fmt.Errorf("secure user file: reopen for metadata: %w", err) } @@ -68,8 +87,8 @@ func applySecureMetadata(h *Home, f *os.File, _ os.FileMode, directory bool) err if err := windows.SetSecurityInfo( handle, windows.SE_FILE_OBJECT, - windows.OWNER_SECURITY_INFORMATION|windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION, - targetSID, + info, + owner, nil, acl, nil, diff --git a/internal/secureuserfile/file_windows_test.go b/internal/secureuserfile/file_windows_test.go index 6f6df9d2..ed83af12 100644 --- a/internal/secureuserfile/file_windows_test.go +++ b/internal/secureuserfile/file_windows_test.go @@ -3,6 +3,7 @@ package secureuserfile import ( + "bytes" "errors" "os" "os/user" @@ -273,9 +274,155 @@ func TestSecureUserFile_PreexistingWrongOwnerRejected(t *testing.T) { t.Fatal(err) } defer h.Close() + before := windowsSecurityDescriptor(t, path) file := openSecureTestFile(t, h, "config") if _, _, _, err := file.Read(); !errors.Is(err, ErrTargetUnusable) { t.Fatalf("Read error = %v, want ErrTargetUnusable", err) } + if err := file.RepairMetadata(FileMode); !errors.Is(err, ErrTargetUnusable) { + t.Fatalf("RepairMetadata error = %v, want ErrTargetUnusable", err) + } + if after := windowsSecurityDescriptor(t, path); after != before { + t.Fatalf("wrong-owner descriptor changed: %s -> %s", before, after) + } assertWindowsOwner(t, path, originalOwner) } + +// setModifyOnlyACL gives path the shape a legacy object can have: the target +// user holds Modify (0x1301bf: no WRITE_DAC or WRITE_OWNER), not Full Control, +// so only the owner's implicit READ_CONTROL|WRITE_DAC can change its permissions. +func setModifyOnlyACL(t *testing.T, h *Home, path string, directory bool) { + t.Helper() + flags := "" + if directory { + flags = "OICI" + } + sd, err := windows.SecurityDescriptorFromString("D:P(A;" + flags + ";0x1301bf;;;" + h.targetUser.Uid + ")(A;" + flags + ";FA;;;SY)") + if err != nil { + t.Fatal(err) + } + acl, _, err := sd.DACL() + if err != nil { + t.Fatal(err) + } + if err := windows.SetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION, nil, nil, acl, nil); err != nil { + t.Fatal(err) + } +} + +func windowsSecurityDescriptor(t *testing.T, path string) string { + t.Helper() + descriptor, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.OWNER_SECURITY_INFORMATION|windows.DACL_SECURITY_INFORMATION) + if err != nil { + t.Fatalf("GetNamedSecurityInfo(%q): %v", path, err) + } + return descriptor.String() +} + +func requireOwnerAssignmentDenied(t *testing.T, h *Home, path string, directory bool) { + t.Helper() + f, err := os.Open(path) + if err != nil { + t.Fatal(err) + } + defer f.Close() + if err := applySecureMetadata(h, f, FileMode, directory); !errors.Is(err, windows.ERROR_ACCESS_DENIED) { + t.Fatalf("owner-assigning metadata on Modify-only %q = %v, want access denied", path, err) + } +} + +func requireSecurePath(t *testing.T, h *Home, path string, mode os.FileMode) { + t.Helper() + f, err := os.Open(path) + if err != nil { + t.Fatal(err) + } + defer f.Close() + if secure, err := h.MetadataSecure(f, mode); err != nil || !secure { + t.Fatalf("metadata %q = %v, %v, want secure", path, secure, err) + } +} + +func TestSecureUserFile_RepairWithoutOwnershipRights(t *testing.T) { + home := t.TempDir() + h := newSecureTestHome(t, home) + probe := filepath.Join(t.TempDir(), "probe") + if err := os.WriteFile(probe, nil, FileMode); err != nil { + t.Fatal(err) + } + setModifyOnlyACL(t, h, probe, false) + if f, err := os.Open(probe); err != nil { + t.Fatal(err) + } else { + err = applySecureMetadata(h, f, FileMode, false) + _ = f.Close() + if err == nil { + t.Skip("this token can assign ownership without WRITE_OWNER; run as a standard user") + } + } + targetSID, err := windows.StringToSid(h.targetUser.Uid) + if err != nil { + t.Fatal(err) + } + dir := filepath.Join(home, ".stepsecurity") + if err := os.Mkdir(dir, ParentMode); err != nil { + t.Fatal(err) + } + statePath := filepath.Join(dir, "state.json") + lockPath := statePath + ".lock" + original := []byte(`{"schema_version":1}` + "\n") + for _, path := range []string{statePath, lockPath} { + if err := os.WriteFile(path, original, FileMode); err != nil { + t.Fatal(err) + } + setModifyOnlyACL(t, h, path, false) + requireOwnerAssignmentDenied(t, h, path, false) + } + setModifyOnlyACL(t, h, dir, true) + requireOwnerAssignmentDenied(t, h, dir, true) + + if err := h.EnsureParent(filepath.Join(".stepsecurity", "state.json")); err != nil { + t.Fatalf("EnsureParent on Modify-only parent: %v", err) + } + requireSecurePath(t, h, dir, ParentMode) + + state := openSecureTestFile(t, h, filepath.Join(".stepsecurity", "state.json")) + if err := state.RepairMetadata(FileMode); err != nil { + t.Fatalf("RepairMetadata on Modify-only file: %v", err) + } + requireSecurePath(t, h, statePath, FileMode) + + lockFile := openSecureTestFile(t, h, filepath.Join(".stepsecurity", "state.json.lock")) + lock, err := lockFile.OpenLock() + if err != nil { + t.Fatalf("OpenLock on Modify-only lock: %v", err) + } + _ = lock.Close() + requireSecurePath(t, h, lockPath, FileMode) + + for _, path := range []string{dir, statePath, lockPath} { + assertWindowsOwner(t, path, targetSID) + } + for _, path := range []string{statePath, lockPath} { + got, err := os.ReadFile(path) + if err != nil || !bytes.Equal(got, original) { + t.Fatalf("%q bytes = %q, %v, want unchanged", path, got, err) + } + } + + before := windowsSecurityDescriptor(t, statePath) + if err := state.RepairMetadata(FileMode); err != nil { + t.Fatalf("RepairMetadata on secure file: %v", err) + } + if after := windowsSecurityDescriptor(t, statePath); after != before { + t.Fatalf("secure file descriptor changed: %s -> %s", before, after) + } + + absent := openSecureTestFile(t, h, filepath.Join(".stepsecurity", "absent.json")) + if err := absent.RepairMetadata(FileMode); err != nil { + t.Fatalf("RepairMetadata on absent file: %v", err) + } + if _, err := os.Lstat(filepath.Join(dir, "absent.json")); !os.IsNotExist(err) { + t.Fatalf("absent file created: %v", err) + } +}