fix(agent): verify DACL and owner of package manager binaries before elevated execution - #1889
Conversation
Let maintainers know that an action is required on their side
|
|
Implementation notes:
|
There was a problem hiding this comment.
Pull request overview
Adds security validation for package-manager executables before elevated Windows execution.
Changes:
- Generalizes file owner/DACL validation.
- Applies validation to direct and script-embedded executables.
- Adds elevated/non-elevated ACL tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
policy_security.rs |
Generalizes security verification. |
broker/mod.rs |
Exposes the security module internally. |
windows/process.rs |
Validates resolved executables. |
windows/mod.rs |
Propagates elevation state and validates WinGet/Chocolatey paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
devolutions-agent/src/broker/policy_security.rs:198
- No negative test exercises the ancestor-directory rejection path: the writable-executable tests fail on the executable's own owner/DACL, while the System32 test only covers acceptance. Removing this call would therefore leave the suite green and reopen the directory-swap vulnerability. Add a test with an otherwise trusted executable beneath an untrusted/Everyone-tamperable ancestor and assert that verification fails because of that ancestor.
verify_ancestor_directories(&final_path, &subject)?;
|
|
||
| /// Access rights on an ancestor directory that allow swapping a path component underneath | ||
| /// a verified executable (renaming or deleting entries, or rewriting the directory's own | ||
| /// security descriptor). Rights that only allow *adding* new entries are deliberately not |
There was a problem hiding this comment.
[P1] Reject untrusted directory create rights for elevated executables
FILE_WRITE_DATA (FILE_ADD_FILE on directories) and FILE_APPEND_DATA (FILE_ADD_SUBDIRECTORY) are deliberately omitted, so an untrusted user can create a DLL or other application-loaded resource beside the verified executable. Holding the executable handle prevents replacement of that file, but not loader side-loading when the elevated process starts. Treat directory create rights as tampering rights and cover this with a regression test.
There was a problem hiding this comment.
Fixed in 09ed939. The directory hosting the executable is now checked against a stricter PARENT_DIRECTORY_TAMPER_MASK that additionally rejects FILE_ADD_FILE, FILE_ADD_SUBDIRECTORY and GENERIC_WRITE, closing the DLL-planting/side-loading vector. Higher ancestors keep the rename/delete mask: stock drive roots grant Authenticated Users create rights (e.g. add-subdirectory on C:\), and create rights above the hosting directory cannot redirect an existing path component. Regression tests added for both the rejected create rights on the hosting directory and the tolerated create rights on higher ancestors.
09ed939 to
89836d1
Compare
Extract the owner/DACL verification logic from the policy file check into reusable helpers so the same admin-only-writability validation can be applied to other security-sensitive files: - verify_admin_only_writable(file, subject) with subject-aware errors - verify_admin_only_writable_path(path, subject), fail-closed on open - verify_elevated_executable_security(path, requires_elevation), a no-op unless the executable is about to run elevated Issue: DGW-434 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…elevated execution Before running a package-manager executable with an elevated (SYSTEM) token, verify that the binary is owned by SYSTEM or built-in Administrators and that its DACL does not grant write access to non-admin users. This prevents privilege escalation where a standard user plants or overwrites a package-manager binary (e.g. via a user-writable PATH entry) and gets it executed with elevated rights. The check is enforced at every elevated executable resolution point: - resolve_executable / create_process for directly spawned binaries (dotnet.exe, trusted PowerShell hosts) - resolve_winget_executable and resolve_trusted_chocolatey_executable, whose binaries are embedded into generated batch scripts and never reach create_process as the resolved executable Non-elevated (per-user) executions are unaffected, keeping user-scope installs (pip venvs, ~/.cargo, ~/.bun, etc.) working as before. Issue: DGW-434 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ustedInstaller Address review findings on the elevated executable verification: - Close the TOCTOU window between verification and image load: the executable is now opened without write or delete sharing and the returned VerifiedExecutable guard keeps that handle alive until the spawned process (or the batch script embedding the path) has finished, so the verified object cannot be written, deleted, or renamed in between. Execution uses the final path resolved from the verified handle, defeating reparse-point retargeting of the supplied name, and every ancestor directory is checked so untrusted principals cannot rename or delete path components either (inherit-only ACEs such as the CREATOR OWNER template are correctly ignored). - Trust NT SERVICE\TrustedInstaller for executables: Windows-protected binaries (cmd.exe, taskkill.exe, Windows PowerShell, WinGet under WindowsApps) are owned by and writable by TrustedInstaller, so the previous SYSTEM/Administrators-only rule rejected every elevated plan. The policy file check deliberately keeps the stricter set. A new test verifies a real protected System32 executable end to end. Issue: DGW-434 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ted binaries A principal able to add entries beside a verified executable can plant a DLL or another application-loaded resource that the elevated process side-loads at start. The directory hosting the executable is now checked against a stricter mask that also rejects FILE_ADD_FILE, FILE_ADD_SUBDIRECTORY and GENERIC_WRITE. Higher ancestors keep the rename/delete mask, since stock drive roots grant create rights to unprivileged users and those cannot redirect an existing path component. Issue: DGW-434 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Master's capability probing calls the winget/chocolatey resolvers, which now take a requires_elevation flag. Probing is not an elevated execution, so no ACL verification is performed there. Also restore SeChangeNotify after the SharedPrivileges refcount test: the last guard drop disabled this default-enabled privilege process-wide, making concurrent tests relying on path normalization fail intermittently. Issue: DGW-434 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
89836d1 to
84ac720
Compare
4e10a49
into
master
Before running a package-manager executable with an elevated (SYSTEM) token, the Agent package broker now verifies that the binary is owned by SYSTEM or the built-in Administrators group and that its DACL does not grant write access to non-admin users. Without this check, a standard user could plant or overwrite a package-manager binary (for example via a user-writable PATH entry) and have it executed with elevated rights, resulting in local privilege escalation.
The verification is fail-closed and enforced at every elevated executable resolution point: directly spawned binaries (dotnet.exe, trusted PowerShell hosts) as well as winget and Chocolatey binaries that are embedded into generated batch scripts. Non-elevated per-user executions are unaffected, so user-scope installs (pip virtualenvs, ~/.cargo, ~/.bun, and so on) keep working as before.
Issue: DGW-434