fix(windows): run scheduled task as logged-in user via /ru INTERACTIVE#91
Conversation
Signed-off-by: Swarit Pandey <swarit@stepsecurity.io>
There was a problem hiding this comment.
Pull request overview
Updates the Windows scheduled task creation logic so that, when installed with admin privileges, the task is configured to run as the currently logged-in interactive user rather than as SYSTEM, aligning execution with user-scoped environment/data (HKCU, user profile, etc.).
Changes:
- Change scheduled task
/rufromSYSTEMtoINTERACTIVEfor admin installs. - Update unit test expectation to validate
/ru INTERACTIVE.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/schtasks/schtasks.go | Switch admin task principal to INTERACTIVE and add explanatory inline comments. |
| internal/schtasks/schtasks_test.go | Update tests to expect INTERACTIVE instead of SYSTEM. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // HKCU, %USERPROFILE%, and PATH. /ru SYSTEM would run as | ||
| // NT AUTHORITY\SYSTEM, which can't see any of the user-scoped | ||
| // data the scanner depends on. | ||
| args = append(args, "/ru", "INTERACTIVE") |
There was a problem hiding this comment.
Good catch — addressed in 10f2422. After MkdirAll, when running as admin, we now run icacls to grant BUILTIN\Users (SID 545) Modify rights on the log dir with (OI)(CI) so files and subfolders inherit the permission. Verified on the Windows VM: C:\ProgramData\StepSecurity now shows an explicit BUILTIN\Users:(OI)(CI)(M) ACE alongside the inherited entries. A non-admin interactive user firing the task can now append to agent.log without Access Denied.
| // /ru INTERACTIVE binds the task to the BUILTIN\INTERACTIVE group | ||
| // (SID S-1-5-4) so it fires under the security context of whoever | ||
| // is interactively logged on at trigger time — picking up their |
There was a problem hiding this comment.
Correct — fixed in 10f2422. Updated the comment to refer to NT AUTHORITY\INTERACTIVE (the actual well-known principal for SID S-1-5-4). BUILTIN domain SIDs are S-1-5-32-*, so the original wording was misleading.
…comment The /ru INTERACTIVE scheduled task fires under whatever user is logged on at trigger time, typically a non-admin developer. The default ACLs on C:\ProgramData\StepSecurity (inherited from C:\ProgramData) only grant non-admin users Read & Execute on existing files, so cmd.exe's `>>` redirect to agent.log would fail with Access Denied — and a failed redirect aborts the whole task action, so the periodic scan never runs. Grant BUILTIN\Users (SID 545) Modify rights on the log dir after creating it, propagated to files and subfolders, so any logged-in user can append. Also corrects the /ru INTERACTIVE comment: SID S-1-5-4 is in the NT AUTHORITY domain, not BUILTIN (BUILTIN\* SIDs are S-1-5-32-*).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
internal/schtasks/schtasks.go:54
icacls <dir> /grant ...updates the directory ACL but does not reliably propagate the new inheritable ACE to already-existing files (e.g.,agent.logfrom a previous install). On upgrade, the task can still fail with Access Denied when appending to existing log files. Consider usingicacls <dir> ... /T(or explicitly updating the log files) so upgrades also fix existing ACLs.
_, _, _, icaclsErr := exec.Run(ctx, "icacls", logDir, "/grant", "*S-1-5-32-545:(OI)(CI)M", "/Q")
internal/schtasks/schtasks.go:54
- Using
icacls ... /granton every install/upgrade can accumulate duplicate ACEs for the same SID over time. Prefer/grant:r(replace) to keep the ACL stable across repeated runs.
_, _, _, icaclsErr := exec.Run(ctx, "icacls", logDir, "/grant", "*S-1-5-32-545:(OI)(CI)M", "/Q")
| _, _, _, icaclsErr := exec.Run(ctx, "icacls", logDir, "/grant", "*S-1-5-32-545:(OI)(CI)M", "/Q") | ||
| if icaclsErr != nil { | ||
| log.Warn("could not adjust log dir ACLs (%v) — non-admin users may not be able to write to %s", icaclsErr, logDir) |
| // For admin installs the log dir lives at C:\ProgramData\StepSecurity, which | ||
| // inherits ACLs from C:\ProgramData and only grants non-admin users | ||
| // Read & Execute on the files inside. The /ru INTERACTIVE task fires under | ||
| // whatever user is logged on — typically a non-admin developer — and | ||
| // cmd.exe's `>>` redirect to agent.log would fail with Access Denied, which | ||
| // aborts the whole task action. Grant BUILTIN\Users (SID 545) Modify rights | ||
| // on the log dir, propagated to files and subfolders, so any logged-in | ||
| // user can append to the log files. | ||
| if exec.IsRoot() { | ||
| _, _, _, icaclsErr := exec.Run(ctx, "icacls", logDir, "/grant", "*S-1-5-32-545:(OI)(CI)M", "/Q") | ||
| if icaclsErr != nil { | ||
| log.Warn("could not adjust log dir ACLs (%v) — non-admin users may not be able to write to %s", icaclsErr, logDir) | ||
| } | ||
| } |
What does this PR do?
Type of change
Testing
./stepsecurity-dev-machine-guard --verbose./stepsecurity-dev-machine-guard --json | python3 -m json.toolmake lintmake testRelated Issues