diff --git a/plugins/endpoints/api/siteagent.py b/plugins/endpoints/api/siteagent.py index 6c6b4527..897573f3 100644 --- a/plugins/endpoints/api/siteagent.py +++ b/plugins/endpoints/api/siteagent.py @@ -410,6 +410,57 @@ function Set-HklmValueBothViews {{ }} }} +# Lock SOFTWARE\\GE\\ShopDB to SYSTEM and Administrators, in both views. +# +# WHY. The shared fleet key is written here and stays until every consumer is +# on a per-PC credential, and the DACL this key inherits by default grants +# BUILTIN\\Users read - so any logged-on user on any bay could read the fleet +# key, which is the exact exposure the per-PC credential migration exists to +# close. Everything that legitimately reads this key runs as SYSTEM (the agent +# service, the GE-Enforce reporters) or as an admin (the adoption tool, +# Set-ShopDBServerPins.ps1). The tray reads no registry at all - it talks to the +# service over the named pipe - so nothing loses access here. +# +# It also protects ServerPins from a standard user, which is what makes the +# registry safe to treat as authoritative for pinning. +function Protect-HklmShopdbKey {{ + param([Parameter(Mandatory)] [string] $SubKey) + $rights = [System.Security.AccessControl.RegistryRights] + foreach ($view in @([Microsoft.Win32.RegistryView]::Registry64, + [Microsoft.Win32.RegistryView]::Registry32)) {{ + $base = [Microsoft.Win32.RegistryKey]::OpenBaseKey( + [Microsoft.Win32.RegistryHive]::LocalMachine, $view) + try {{ + $key = $base.OpenSubKey( + $SubKey, + [Microsoft.Win32.RegistryKeyPermissionCheck]::ReadWriteSubTree, + $rights::ChangePermissions -bor $rights::ReadPermissions) + if (-not $key) {{ continue }} + try {{ + $acl = $key.GetAccessControl( + [System.Security.AccessControl.AccessControlSections]::Access) + # Break inheritance and DROP the inherited entries rather than + # copying them in: copying is what silently preserves the + # BUILTIN\\Users read this function exists to remove. + $acl.SetAccessRuleProtection($true, $false) + foreach ($rule in @($acl.Access)) {{ $null = $acl.RemoveAccessRule($rule) }} + foreach ($sid in @("S-1-5-18", "S-1-5-32-544")) {{ + $who = New-Object System.Security.Principal.SecurityIdentifier($sid) + $acl.AddAccessRule((New-Object System.Security.AccessControl.RegistryAccessRule( + $who, + $rights::FullControl, + [System.Security.AccessControl.InheritanceFlags]::ContainerInherit, + [System.Security.AccessControl.PropagationFlags]::None, + [System.Security.AccessControl.AccessControlType]::Allow))) + }} + $key.SetAccessControl($acl) + }} + finally {{ $key.Dispose() }} + }} + finally {{ $base.Dispose() }} + }} +}} + # REG_MULTI_SZ in both views, compare-before-write so a reinstall with the same # pins leaves the value (and its last-write time) alone. function Set-HklmMultiStringBothViews {{ @@ -492,6 +543,17 @@ else {{ Write-Warning "no server pin in this package; ServerPins left as is." }} +# Harden the key AFTER the writes: the DACL is replaced, not merged, so writing +# first and locking second keeps this correct whether or not the key pre-existed. +try {{ + Protect-HklmShopdbKey -SubKey "SOFTWARE\\GE\\ShopDB" + Write-Host " HKLM\\SOFTWARE\\GE\\ShopDB restricted to SYSTEM and Administrators." +}} +catch {{ + Write-Warning "could not restrict HKLM\\SOFTWARE\\GE\\ShopDB: $($_.Exception.Message)" + Write-Warning "the shared key on this PC stays readable by any logged-on user until that is fixed." +}} + # 1a. Trust the bundled server certificate, if one shipped. The agent's outbound # TLS to a self-signed or internal-CA server fails closed until the machine # trusts the cert. Importing into LocalMachine\\Root covers the SYSTEM service. diff --git a/tests/test_plugins/test_endpoints_siteagent_build.py b/tests/test_plugins/test_endpoints_siteagent_build.py index dc7ec00d..b5c0f236 100644 --- a/tests/test_plugins/test_endpoints_siteagent_build.py +++ b/tests/test_plugins/test_endpoints_siteagent_build.py @@ -289,6 +289,62 @@ def test_leaf_certificate_is_imported_into_root(client, db, auth_headers): assert json.loads(files['appsettings.json'])['serverpins'] == [leafpin] +def test_installer_restricts_the_registry_key_to_system_and_admins( + client, db, auth_headers, servercert): + """The shared key stays in HKLM until every consumer is on a per-PC + credential, so what matters meanwhile is who can read it. The key inherits a + DACL that grants BUILTIN\\Users read, which is the exposure the per-PC + migration exists to close - the installer must not leave it that way.""" + files = _files(_build(client, auth_headers, collectorkey=SECRET, + includesharedkey='true')) + script = files['install.ps1'].decode('ascii') + + assert 'function Protect-HklmShopdbKey' in script + assert 'Protect-HklmShopdbKey -SubKey "SOFTWARE\\GE\\ShopDB"' in script + # SYSTEM and Administrators by SID, so a localised Windows still matches. + assert '"S-1-5-18", "S-1-5-32-544"' in script + # Inheritance broken WITHOUT copying the inherited entries: copying them is + # what silently preserves the Users read. + assert 'SetAccessRuleProtection($true, $false)' in script + assert 'RemoveAccessRule($rule)' in script + + +def test_the_key_is_written_before_it_is_locked(client, db, auth_headers, servercert): + """SetAccessControl replaces the DACL rather than merging, so a lock that ran + first would be undone by nothing - but a write that ran after a lock the + installer no longer holds rights for would fail. Order is the guarantee.""" + script = _files(_build(client, auth_headers, collectorkey=SECRET, + includesharedkey='true'))['install.ps1'].decode('ascii') + + write = script.index('-Name "CollectorKey" -Value $CollectorKey') + lock = script.index('Protect-HklmShopdbKey -SubKey') + assert write < lock + + +def test_the_registry_lock_failing_does_not_fail_the_install( + client, db, auth_headers, servercert): + """A bay that cannot take the ACL still needs its agent. It must say plainly + that the key is readable there, rather than dying or staying quiet.""" + script = _files(_build(client, auth_headers, collectorkey=SECRET, + includesharedkey='true'))['install.ps1'].decode('ascii') + + lock = script.index('Protect-HklmShopdbKey -SubKey') + tail = script[lock:lock + 600] + assert 'catch' in tail + assert 'readable by any logged-on user' in tail + + +def test_the_keyless_package_still_locks_the_key(client, db, auth_headers, servercert): + """ServerPins live in the same key and the registry is authoritative for + pinning, so a standard user must not be able to rewrite them either.""" + files = _files(_build(client, auth_headers, collectorkey=SECRET, + includesharedkey=False)) + script = files['install.ps1'].decode('ascii') + + assert b'CollectorKey' not in files['install.ps1'] + assert 'Protect-HklmShopdbKey -SubKey "SOFTWARE\\GE\\ShopDB"' in script + + def test_install_script_helper_without_pins_or_key(): script = siteagent._install_script('https://shopdb.test', '', 900, serverpins=[], includesharedkey=False)