Skip to content

fix(agent-installer): refuse an untrusted agent data folder - #2168

Merged
Benoît Cortier (CBenoit) merged 9 commits into
masterfrom
claude/agent-installer-data-owner-2f92e5
Oct 28, 2026
Merged

Benoît Cortier (CBenoit) merged 9 commits into
masterfrom
claude/agent-installer-data-owner-2f92e5

Conversation

@CBenoit

@CBenoit Benoît Cortier (CBenoit) commented Oct 27, 2026 •

Copy link
Copy Markdown
Member

The installer writes the agent's data under %ProgramData%\Devolutions\Agent as SYSTEM, but it accepted those folders whatever their owner, applied only the DACL from its SDDL so the folders kept the owner they had, and the uninstall deleted the Agent Identity state with a path-based recursive delete.
An account that owned or could replace one of those folders could therefore keep control of what the installer and the agent write there.

The installer now:

  • refuses an install, upgrade, repair or feature change, before it changes anything, with a localized error naming the folder and the account, when Devolutions or Agent is 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 of Devolutions lets any other account replace it or an entry of it, matching what the agent refuses; a full uninstall is never refused;
  • creates a missing Agent with its final security descriptor, or resets the owner and DACL of an existing one on the handle it checked, and sets both on Agent and Agent\pedm without following any link on the path and without propagating to existing children; a failed reset now fails the run;
  • deletes Agent\identity at uninstall without following any link, holding each directory open without delete sharing while the entries below it are resolved.

Issue: DGW-478
Security: yes

🤖 Generated with Claude Code

The installer must set the owner of the agent data folder, not only
its permissions, and must refuse a data folder or a parent folder
that another account controls, because such a folder may hold entries
that account left there. A full uninstall never refuses, and neither
setting the folder's security nor deleting the Agent Identity state
follows a link.

Issue: DGW-478
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The installer now creates the agent data folder with its final
security descriptor, so it never exists with the DACL it would
inherit from ProgramData, which lets users add entries. When it
resets the security of that folder and of its pedm folder, it also
sets the owner, and it refuses a folder that is a reparse point
rather than applying the security to the link's target. The
security is set on a handle with SetKernelObjectSecurity, which
leaves the entries below the folder unchanged.

A full uninstall deletes the Agent Identity state without following
any link: each directory is opened as itself and held without
delete sharing while the entries below it are resolved, and each
entry is deleted through its own handle. A link is deleted itself,
never its target, and nothing is deleted when the data folder or
its parent is a link.

A new helper reads a folder's owner without following links and
tells whether an owner is trusted: SYSTEM, Administrators,
TrustedInstaller, or a member of Administrators, decided as the
agent decides it. Nothing refuses an untrusted owner yet.

Issue: DGW-478
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An install, upgrade, repair or maintenance run of the Agent installer
now fails, before it changes anything, when %ProgramData%\Devolutions
or %ProgramData%\Devolutions\Agent is a link, has an owner other than
SYSTEM, TrustedInstaller or an administrator, or, for Devolutions,
has a DACL that lets another account delete, rename or replace it or
what it contains. The installer writes the agent's data there as
SYSTEM, so a folder another account controls could redirect or
replace what it writes. A folder whose owner or DACL can't be read is
refused too.

The error names the folder and the account at fault, in the
installer's language, and a silent install logs it. The check runs
before RemoveExistingProducts, and again when the Agent folder is
created, under a handle that keeps Devolutions from being renamed or
replaced until Agent is created or checked inside it.

A full uninstall is never refused, so a machine can always be
uninstalled.

Issue: DGW-478
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The installer's check of %ProgramData%\Devolutions now refuses exactly
the DACLs that the agent refuses for a directory above its data, so
the installer never prepares a folder the agent then won't use. An
entry for OWNER RIGHTS, whose holder is the owner the check already
requires to be trusted, or for an AppContainer package or capability,
which never grants access by itself, no longer causes a refusal. An
entry of a type other than a plain or conditional allow entry or a
deny entry is refused as unreadable, as the agent does.

The pedm folder is now created after the Agent folder, so that it
never creates Agent itself with the DACL Agent would inherit from
ProgramData.

Issue: DGW-478
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The tests of the data folder rules rest on the installer intent that
now states them, rather than on other components' similar checks.

Issue: DGW-478
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A directory handle with only READ_CONTROL or FILE_READ_ATTRIBUTES takes
no part in sharing checks, so denying delete sharing on it kept nothing
from renaming the directory. Every handle that holds a directory now
also requests FILE_TRAVERSE, through one helper used by the data folder
check and by the Agent Identity deletion.

Setting the security of a data folder now opens each folder above it
without following links and holds it while the next one is opened, so a
link at Devolutions can't redirect the change to another folder.

An existing Agent folder gets its owner and DACL reset on the handle it
was checked on, while Devolutions is held, and a failure fails the run.
The reset actions now fail the run too and no longer run at a full
uninstall. A file named Devolutions or Agent is refused with its own
localized error instead of failing later.

Issue: DGW-478
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The folder can't be opened at all, so the test checks that an open that
fails for a reason other than not-found is refused as unreadable.

Issue: DGW-478
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@CBenoit

Copy link
Copy Markdown
Member Author

Validation

Head b244bff73.

  • Lines changed: production code +1159 −26 (Actions/*.cs), tests +845 −64, localization resources and intent +104 −0.
  • dotnet test package/AgentWindowsManaged.Tests: 160 passed, 0 failed (non-elevated run; the owner assertions only discriminate when elevated).
  • ci/check-oracle-markers.ps1 -Base a66a019d0: passes.
  • Update-InstallerLocalization.ps1 -Product agent run twice: second run leaves the tree unchanged.
  • MSI built locally with placeholder binaries; InstallExecuteSequence: InstallInitialize 1500, CA.checkProgramDataDirectory 1503, RemoveExistingProducts 1504, CreateFolders 3700, CA.createProgramDataDirectory 3702, CA.createProgramDataPedmDirectories 3704, then the two permission resets, InstallFiles 4000.
  • Held directory handles request FILE_TRAVERSE so that their share mode applies; tests assert a held directory can't be renamed.
  • Two independent review rounds on the whole delta; the second found nothing blocking.
  • The DACL rule for Devolutions follows the agent's ancestor rule in refactor(agent): keep private files with private-fs #2154 (head 9b8b60380f6c); a later change there needs the installer to follow.

Intent

The Data folder section of package/AgentWindowsManaged/INTENT.md and the uninstall sentence under Agent Identity were approved verbatim by the maintainer before the code; the tests cite them.

Known limits

  • A Devolutions folder created by a non-elevated process carries a full-control entry for that user, so the installer refuses it, as the agent does; the fix is to give the folder to SYSTEM or Administrators and remove the entry.
  • The reset of an existing Agent doesn't propagate to the entries it already holds.
  • A Devolutions the installer created stays after a refusal.
  • The Gateway installer (package/WindowsManaged) isn't changed here.

UNTESTED

  • SecureDirectory.DeleteTree: each directory handle stays open until the entries below it are done.
  • DataFolders.Create: the Devolutions handle stays open until Agent is created, or checked and reset.
  • DataFolders.Create: setting the owner of an existing Agent to SYSTEM succeeds only for a caller allowed to set another owner (the installer's SYSTEM process).
  • DataFolders.Check/Create: an existing Agent that is a link or has an untrusted owner is refused (needs a passing Devolutions, whose owner a non-elevated test can't set).
  • SecureDirectory.IsTrustedOwner: a member of Administrators is trusted through Authz, or the local member list when Authz fails.
  • CustomActions.Refuse: a refusal is sent as InstallMessage.Error, which a silent install logs and an interactive one shows.
  • CustomActions.SetProgramDataPedmDirectoryPermissions: a missing pedm folder doesn't fail the run.

🤖 Generated with Claude Code

An elevated runner's account renders as an SDDL alias, and DirectorySecurity reorders the entries it reads, so both tests now compare descriptors read and rendered the same way.

Issue: DGW-478

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A member of Administrators may already own the shared folder, so an entry that lets one rename or replace it gives no account more control than the owner rule accepts; it is common on a folder an administrator's own process created.

Issue: DGW-478
Security: yes

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@CBenoit
Benoît Cortier (CBenoit) merged commit 1b90503 into master Oct 28, 2026
52 checks passed
@CBenoit
Benoît Cortier (CBenoit) deleted the claude/agent-installer-data-owner-2f92e5 branch October 28, 2026 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant