From dea9bb2681afbf130da7e32c9a7ae798f02addaa Mon Sep 17 00:00:00 2001 From: Subham Ray Date: Wed, 7 Oct 2026 00:03:57 +0530 Subject: [PATCH] fix(devicepolicy): repair legacy Windows state permissions State files left by older agents inherit their parent's DACL and fail the strict metadata check, so reads report no ownership and writes, probes and clears fail. Repair a correctly owned state file's DACL under the existing cross-process lock before use, preserving its bytes, owner and records. Existing verified objects (state, lock, parent directory) are repaired with READ_CONTROL|WRITE_DAC only, so Modify-only legacy permissions no longer need ownership-change rights. Fresh creation still assigns the owner. On Windows, a marker-based clear now stops before changing configuration when the state store cannot be prepared. Unexpected target-setup and swallowed state-read failures are logged at Warn, redacted. macOS/Linux behavior is unchanged. --- cmd/stepsecurity-dev-machine-guard/main.go | 10 +- .../main_devicepolicy_test.go | 74 ++++ internal/devicepolicy/cache.go | 107 +++++- internal/devicepolicy/cache_test.go | 22 +- internal/devicepolicy/cache_windows_test.go | 342 +++++++++++++++++- internal/devicepolicy/reconcile.go | 11 +- internal/secureuserfile/file.go | 35 +- internal/secureuserfile/file_unix.go | 6 + internal/secureuserfile/file_windows.go | 25 +- internal/secureuserfile/file_windows_test.go | 147 ++++++++ 10 files changed, 750 insertions(+), 29 deletions(-) diff --git a/cmd/stepsecurity-dev-machine-guard/main.go b/cmd/stepsecurity-dev-machine-guard/main.go index 8a5b0f0c..1628e5b2 100644 --- a/cmd/stepsecurity-dev-machine-guard/main.go +++ b/cmd/stepsecurity-dev-machine-guard/main.go @@ -3,6 +3,7 @@ package main import ( "context" "encoding/json" + "errors" "fmt" "io" "os" @@ -33,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" @@ -783,11 +785,15 @@ func runHookStateReconcile(exec executor.Executor, log *progress.Logger) { 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 e7f851c5..848b562d 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" "os" "os/user" "path/filepath" @@ -197,3 +198,76 @@ func TestPackageConfigLanes_UseSeparateTimeoutContexts(t *testing.T) { t.Error("Go context has no deadline") } } + +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) + } + }) + } +} 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) + } +}