Repository navigation
fix(installer): refuse an untrusted data folder - #2172
Merged
Merged
Conversation
Issue: DGW-478 Security: yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…links The Gateway service trusts what it reads from %ProgramData%\Devolutions\Gateway, so the installer must only reset the security of folders that SYSTEM and administrators control. Before it changes anything, an install, upgrade, repair or feature change now fails with a localized error naming the folder and the account when Devolutions or Gateway is a link, is not a directory, can't be read, is owned by an account that is not SYSTEM, TrustedInstaller or an administrator, or when the DACL of Devolutions lets another account replace it or one of its entries. A full uninstall is never refused, so a machine can always be uninstalled. The folders are created and their security set through handles held without delete sharing, so no path is resolved twice. The files below Gateway get the entries it passes on through the handles of their held parents: reparse points are not followed or changed, files with several hard links are skipped and logged, and users.txt is refused when it is a reparse point or a hard link. A new xUnit project covers the checks, the walk and the sequencing, and runs in CI without elevation. Issue: DGW-478 Security: yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Gateway service account had Delete on %ProgramData%\Devolutions\Gateway itself, so it could rename or replace the folder whose security the installer resets, which only SYSTEM and administrators should be able to do. Its entry on the folder now omits Delete and keeps Delete Subfolders and Files, and an inherit-only entry gives it Delete on the files and subfolders below, so its rights on everything the service manages are unchanged. The service never deletes or renames the data folder itself. Issue: DGW-478 Security: yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t lies below Only SYSTEM, administrators and the service account may change what the Gateway stores in %ProgramData%\Devolutions\Gateway, yet the data folder and users.txt also granted LOCAL SERVICE read and write access. That entry is removed; a service account that is itself LOCAL SERVICE keeps its rights through the service account's entries. So that an upgrade also removes the entry from existing data, folders below Gateway now get the same reset as files: explicit entries are kept, protection is removed, and inherited entries are computed again from the held parent, top-down. An entry whose DACL is not marked auto-inherited holds what it inherited as explicit entries, so these are told apart by comparing them with what its folder passed on before the reset, as Windows does when it passes a new DACL on. The reset of an existing Gateway therefore walks the tree in the same call that changes its DACL, on the handle it checked, and the new DACL is stored marked auto-inherited so that entries created later carry inherited entries Windows can tell apart. Issue: DGW-478 Security: yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… its owner The check of %ProgramData%\Devolutions refused an allow entry with a right to replace the folder or its entries unless it named SYSTEM, Administrators or TrustedInstaller, while the owner rule also trusts members of Administrators. An account that may own the folder gains nothing from an entry it could give itself as the owner, and a Devolutions folder that an administrator's application created first, without elevation, carries a full-control entry for that administrator, so such an entry is now accepted. Entries for any other account with a replace right are still refused. Issue: DGW-478 Security: yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Creating or checking %ProgramData%\Devolutions\Gateway already sets its owner and DACL and resets the files and folders below it in the same call, so the separate action that did the same afterwards only walked the tree a second time. It is removed; the users database keeps its own action. Issue: DGW-478 Security: yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…the data folder reset The reset of the data folder's security now leaves the users database directly below it as it is, because another action replaces its DACL and the entries the folder passes on would let users read it until then. A protected directory below the folder keeps its DACL, which an administrator set on purpose, while the entries below it are still reset against it. Every allow entry for LOCAL SERVICE is removed below the folder, in a protected directory too, because only SYSTEM, administrators and the service account may change what the Gateway stores there, and earlier installers gave LOCAL SERVICE write access. The overloads only tests used are removed, and the tests drive the handle the installer holds. The major upgrade is checked to remove the installed product after InstallInitialize, so before the data folder check. Issue: DGW-478 Security: yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…failed trust checks CreateDirectory stores a DACL without marking it auto-inherited even when the descriptor asks for it, so a new data folder's DACL is set again through the new directory's handle. The entries created below it then hold what they inherit as inherited entries, which a later reset tells apart from their explicit ones. When the groups of a folder's owner, or of an account with an entry, can't be resolved, the installer logs why before falling back to the local members of Administrators or refusing the account, so a refused domain administrator can be told apart from a failed lookup. A refusal's text is passed in a record field rather than escaped into the format string, so it is shown as written. Issue: DGW-478 Security: yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Windows Installer replaces a property reference in a record field it inserts, not only in the format string, so a data folder path holding `[NAME]` would show a property value in the refusal. The refusal text is now the format string with each bracket escaped. Creating a folder whose DACL is marked auto-inherited still sets the DACL a second time, since CreateDirectory leaves the mark off even with SE_DACL_AUTO_INHERIT_REQ set. That second open no longer claims a share mode that holds nothing, as the check and the set act on the handle. Issue: DGW-478 Security: yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The refusal shows its text as written only when its record comes from MessageRecord; no test runs Refuse, so the comment there names it. Issue: DGW-478 Security: yes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
Author
ValidationHead
Intent
Decisions
Known limits
UNTESTED
🤖 Generated with Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The installer keeps the Gateway's configuration, keys and databases under
%ProgramData%\Devolutions\Gatewayand writes there as SYSTEM, but it accepted that folder whatever its owner, applied only the DACL from its SDDL so the folder kept the owner it had, and reset the permissions of the files below by path, following any junction in the tree.It also gave LOCAL SERVICE write access to the folder and to
users.txt, and the service account the right to delete the folder itself.The Gateway trusts what it reads from that folder, so an account that owned or could replace it could keep control of the Gateway's configuration.
The installer now:
DevolutionsorGatewayis a link, a file, can't be read, or is owned by anyone but SYSTEM, Administrators, TrustedInstaller or a member of Administrators, or when the DACL ofDevolutionslets another account replace it or an entry of it; a full uninstall is never refused;Gatewaywith its final security descriptor, or resets the owner and DACL of an existing one on the handle it checked, then gives each file and unprotected folder below the entriesGatewaynow passes on, removing those it passed on before, without following any link and skipping files with several hard links; a protected folder keeps its DACL, andusers.txtkeeps its own until its reset;users.txtthrough its handle, refusing a link or a hard-linked file;Issue: DGW-478
Security: yes
🤖 Generated with Claude Code