From c7ed3ec27d84b160e525f2b0f7e443d8404d0dac Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Sat, 25 Jul 2026 12:48:27 +0200 Subject: [PATCH] more safe SecureBootCertReader and section plan layout update --- docs/agent/secureboot-ca2023-detection.md | 15 +++ docs/log.md | 4 + .../UefiSecureBootCertReaderTests.cs | 34 ++++++ .../Interop/UefiSecureBootCertReader.cs | 115 +++++++++++++----- .../settings/tenant/sections/SectionPlan.tsx | 42 ++++--- 5 files changed, 166 insertions(+), 44 deletions(-) diff --git a/docs/agent/secureboot-ca2023-detection.md b/docs/agent/secureboot-ca2023-detection.md index e63fe7c9..e256b123 100644 --- a/docs/agent/secureboot-ca2023-detection.md +++ b/docs/agent/secureboot-ca2023-detection.md @@ -37,6 +37,21 @@ Fail-soft contract like `OobeStateReader`: never throws; failures map to status (`not_uefi` = legacy BIOS via `ERROR_INVALID_FUNCTION`, `variable_not_found`, `privilege_denied`, `error_`). +This is the repo's only privilege-adjustment code, so it also carries the ownership rules +any future one should copy: + +* The process token lives in a `SafeHandle` assigned by the P/Invoke marshaler, so the CLR + owns it from the moment `OpenProcessToken` returns — the critical finalizer releases it + even if `Dispose` never runs (async exception before the instance reaches its `using`). +* The restore obligation is recorded immediately after the successful `AdjustTokenPrivileges`, + and only when the privilege was not *already* enabled; nothing that can throw sits between + the adjustment and that assignment. `Dispose` is idempotent, swallows a failed restore, and + releases the handle in a `finally`. +* The privilege is adjusted on the **process** token, so enable → read → restore is a + process-wide side effect and runs under a static gate. Without it a concurrent reader would + see "already enabled", correctly skip its own restore, and then lose the privilege mid-read + when the first reader disposes — a spurious `privilege_denied` on a machine that holds it. + Fields land on the existing `secureboot_status` event (once per agent start, restart-deduped by the StartupEventGate): `uefiFirmwareReadStatus`, per-cert booleans (`uefiDbHasWindowsUefiCa2023`, `uefiDbHasWindowsProductionPca2011`, diff --git a/docs/log.md b/docs/log.md index 1d9686cc..74243d31 100644 --- a/docs/log.md +++ b/docs/log.md @@ -1,5 +1,9 @@ # Log +## 2026-07-25 + +* **Update**: `agent/secureboot-ca2023-detection.md` — review follow-up on 90012d9 hardened the privilege scope, the repo's only privilege-adjustment code and therefore the precedent for any future one. The process token moved from a raw `IntPtr` to a `SafeHandle` assigned by the P/Invoke marshaler, so the CLR owns it from the moment `OpenProcessToken` returns and the critical finalizer releases it even when `Dispose` never runs — previously an async exception between the successful open and the caller's `using` leaked the handle *with the privilege enabled and no owner left to restore it*. `Dispose` became idempotent and now releases the handle in a `finally`, so a throwing restore cannot strand it; the restore obligation is recorded immediately after the successful `AdjustTokenPrivileges` with nothing throwable in between. The whole enable → read → restore sequence runs under a static gate because the privilege is adjusted on the *process* token: a concurrent reader would see "already enabled", correctly skip its own restore, and then lose the privilege mid-read when the first reader disposed, reporting `privilege_denied` on a machine that holds it. + ## 2026-07-24 * **Creation**: Added `backend/server-actions-on-demand-diagnostics.md` — the ServerAction channel (queue on `Sessions.PendingActionsJson`, deliver piggy-backed on the next ingest response, at-least-once with `Type|RuleId|QueuedAt` dedup and sequential dispatch) and the on-demand log collection built on it: the portal's always-visible **Collect Logs** button, the new `EndpointPolicy.TenantAdminOrOperator` tier with the mandatory in-function per-type re-gate (Operators queue only `request_diagnostics`), the member-readable `diagnosticsUploadConfigured` feature flag, the admin quick-config path that must queue `rotate_config` before `request_diagnostics` (works against the deployed fleet — a one-time override would not, the mode gate is agent-side), sequence-based progress matching immune to agent clock skew, and the offboarding cascade's hosted-prefix sweep closing the latest-wins orphan-blob gap. diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Monitoring/UefiSecureBootCertReaderTests.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Monitoring/UefiSecureBootCertReaderTests.cs index 4d0ad34b..128ac102 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Monitoring/UefiSecureBootCertReaderTests.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Monitoring/UefiSecureBootCertReaderTests.cs @@ -161,6 +161,40 @@ namespace AutopilotMonitor.Agent.V2.Core.Tests.Monitoring Assert.False(string.IsNullOrEmpty(snapshot.KekStatus)); } + /// + /// SeSystemEnvironmentPrivilege is adjusted on the PROCESS token, so the enable → + /// read → restore sequence is a process-wide side effect. Unserialized, a second + /// reader observes "already enabled", skips its own restore, and then loses the + /// privilege mid-read when the first reader disposes — surfacing as a privilege_denied + /// that disagrees with the other readers. The machine's actual privilege state does + /// not matter here: whatever it is, every reader must report the SAME status. + /// + [Fact] + public void Read_is_serialized_across_concurrent_callers() + { + const int readers = 8; + var snapshots = new UefiSecureBootCertSnapshot[readers]; + var failures = new System.Exception[readers]; + + var threads = new System.Threading.Thread[readers]; + for (var i = 0; i < readers; i++) + { + var index = i; + threads[i] = new System.Threading.Thread(() => + { + try { snapshots[index] = UefiSecureBootCertReader.Read(); } + catch (System.Exception ex) { failures[index] = ex; } + }); + threads[i].Start(); + } + foreach (var thread in threads) thread.Join(); + + Assert.All(failures, Assert.Null); + Assert.All(snapshots, s => Assert.False(string.IsNullOrEmpty(s.DbStatus))); + Assert.All(snapshots, s => Assert.Equal(snapshots[0].DbStatus, s.DbStatus)); + Assert.All(snapshots, s => Assert.Equal(snapshots[0].KekStatus, s.KekStatus)); + } + private static byte[] Concat(params byte[][] parts) { var total = 0; diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Interop/UefiSecureBootCertReader.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Interop/UefiSecureBootCertReader.cs index 01684ac6..83c9f5ed 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Interop/UefiSecureBootCertReader.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Interop/UefiSecureBootCertReader.cs @@ -1,5 +1,6 @@ using System; using System.Runtime.InteropServices; +using Microsoft.Win32.SafeHandles; namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Interop { @@ -56,6 +57,13 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Interop private const int InitialBufferSize = 64 * 1024; private const int RetryBufferSize = 256 * 1024; + // SeSystemEnvironmentPrivilege is adjusted on the PROCESS token, so enable → read → + // restore is a process-wide side effect and must run as one atomic sequence. Without + // this gate a second reader would observe "already enabled", correctly skip its own + // restore, and then have the privilege disabled underneath it when the first reader + // finishes — a spurious privilege_denied on a machine that actually holds it. + private static readonly object FirmwareReadGate = new object(); + /// /// Reads db and KEK and evaluates the certificate presence flags. Never throws; /// per-variable failures surface as / @@ -67,6 +75,10 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Interop var snapshot = new UefiSecureBootCertSnapshot(); try { + // The critical section covers the whole privileged window, not just the + // adjustment — see FirmwareReadGate. Only synchronous kernel calls and byte + // scans run inside it, so it cannot block on anything else the agent does. + lock (FirmwareReadGate) using (var privilege = SystemEnvironmentPrivilege.TryEnable()) { if (!privilege.Enabled) @@ -168,23 +180,42 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Interop /// of the instance and restores the previous privilege state on dispose. SYSTEM holds /// the privilege (default-disabled); without elevation stays /// false and the caller reports . + /// + /// Crash-safety. This is the repo's only privilege-adjustment code, so the + /// ownership rules are spelled out: the token is held in a + /// assigned by the P/Invoke marshaler, so the CLR owns it from the moment + /// OpenProcessToken returns — it is released by the critical finalizer even if + /// this instance never reaches the caller's using (async exception between the + /// call and the assignment). is idempotent, never throws, and + /// releases the handle in a finally so a failed restore cannot leak it. The + /// restore obligation is recorded immediately after the successful adjustment, so no + /// throw in between can strand the process with the privilege left enabled. + /// /// private sealed class SystemEnvironmentPrivilege : IDisposable { public bool Enabled { get; private set; } - private IntPtr _token = IntPtr.Zero; + private SafeTokenHandle _token; private NativeMethods.TOKEN_PRIVILEGES _previousState; private bool _restoreNeeded; + private bool _disposed; public static SystemEnvironmentPrivilege TryEnable() { var instance = new SystemEnvironmentPrivilege(); try { + SafeTokenHandle token; if (!NativeMethods.OpenProcessToken(NativeMethods.GetCurrentProcess(), - NativeMethods.TOKEN_ADJUST_PRIVILEGES | NativeMethods.TOKEN_QUERY, out instance._token)) + NativeMethods.TOKEN_ADJUST_PRIVILEGES | NativeMethods.TOKEN_QUERY, out token)) + { + // Failed open still yields a (closed/invalid) SafeHandle instance. + if (token != null) token.Dispose(); return instance; + } + + instance._token = token; NativeMethods.LUID luid; if (!NativeMethods.LookupPrivilegeValueW(null, "SeSystemEnvironmentPrivilege", out luid)) @@ -197,52 +228,80 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Interop Attributes = NativeMethods.SE_PRIVILEGE_ENABLED }; + NativeMethods.TOKEN_PRIVILEGES previousState; uint returnLength; - if (!NativeMethods.AdjustTokenPrivileges(instance._token, false, ref newState, + if (!NativeMethods.AdjustTokenPrivileges(token, false, ref newState, (uint)Marshal.SizeOf(typeof(NativeMethods.TOKEN_PRIVILEGES)), - out instance._previousState, out returnLength)) + out previousState, out returnLength)) return instance; // AdjustTokenPrivileges returns TRUE even when the privilege is not held — - // the real verdict is ERROR_NOT_ALL_ASSIGNED in the last error. + // the real verdict is ERROR_NOT_ALL_ASSIGNED in the last error. Nothing was + // adjusted in that case, so there is nothing to restore. if (Marshal.GetLastWin32Error() == ErrorNotAllAssigned) return instance; - // Restore only when the privilege was not already enabled before. - instance._restoreNeeded = instance._previousState.PrivilegeCount > 0 - && (instance._previousState.Attributes & NativeMethods.SE_PRIVILEGE_ENABLED) == 0; + // The token is now modified: record the restore obligation before anything + // else can throw. Restore only when the privilege was not already enabled. + instance._previousState = previousState; + instance._restoreNeeded = previousState.PrivilegeCount > 0 + && (previousState.Attributes & NativeMethods.SE_PRIVILEGE_ENABLED) == 0; instance.Enabled = true; } catch { - // Fail-soft: Enabled stays false. + // Fail-soft: Enabled stays false. Whatever was acquired before the failure + // is already tracked on the instance and released by Dispose. } return instance; } public void Dispose() { - if (_token != IntPtr.Zero) + if (_disposed) return; + _disposed = true; + Enabled = false; + + var token = _token; + _token = null; + if (token == null) return; + + try { - try + if (_restoreNeeded) { - if (_restoreNeeded) - { - NativeMethods.TOKEN_PRIVILEGES ignored; - uint returnLength; - NativeMethods.AdjustTokenPrivileges(_token, false, ref _previousState, - (uint)Marshal.SizeOf(typeof(NativeMethods.TOKEN_PRIVILEGES)), - out ignored, out returnLength); - } + _restoreNeeded = false; + NativeMethods.TOKEN_PRIVILEGES ignored; + uint returnLength; + NativeMethods.AdjustTokenPrivileges(token, false, ref _previousState, + (uint)Marshal.SizeOf(typeof(NativeMethods.TOKEN_PRIVILEGES)), + out ignored, out returnLength); } - catch - { - // Best effort — the process token privilege state is not load-bearing - // for anything else the agent does. - } - NativeMethods.CloseHandle(_token); - _token = IntPtr.Zero; } + catch + { + // Best effort — the process token privilege state is not load-bearing for + // anything else the agent does, and Dispose must never throw. + } + finally + { + token.Dispose(); + } + } + } + + /// + /// Owns a process-token handle. ReleaseHandle runs on the finalizer thread when + /// is skipped, so it must not throw — + /// a single CloseHandle P/Invoke satisfies that. + /// + private sealed class SafeTokenHandle : SafeHandleZeroOrMinusOneIsInvalid + { + public SafeTokenHandle() : base(true) { } + + protected override bool ReleaseHandle() + { + return NativeMethods.CloseHandle(handle); } } @@ -278,7 +337,7 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Interop [DllImport("advapi32.dll", SetLastError = true)] [return: MarshalAs(UnmanagedType.Bool)] - public static extern bool OpenProcessToken(IntPtr processHandle, uint desiredAccess, out IntPtr tokenHandle); + public static extern bool OpenProcessToken(IntPtr processHandle, uint desiredAccess, out SafeTokenHandle tokenHandle); [DllImport("advapi32.dll", SetLastError = true, CharSet = CharSet.Unicode)] [return: MarshalAs(UnmanagedType.Bool)] @@ -286,7 +345,7 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Interop [DllImport("advapi32.dll", SetLastError = true)] [return: MarshalAs(UnmanagedType.Bool)] - public static extern bool AdjustTokenPrivileges(IntPtr tokenHandle, bool disableAllPrivileges, + public static extern bool AdjustTokenPrivileges(SafeTokenHandle tokenHandle, bool disableAllPrivileges, ref TOKEN_PRIVILEGES newState, uint bufferLength, out TOKEN_PRIVILEGES previousState, out uint returnLength); [DllImport("kernel32.dll", SetLastError = true)] diff --git a/src/Web/autopilot-monitor-web/app/settings/tenant/sections/SectionPlan.tsx b/src/Web/autopilot-monitor-web/app/settings/tenant/sections/SectionPlan.tsx index e5a0a728..aaa8671a 100644 --- a/src/Web/autopilot-monitor-web/app/settings/tenant/sections/SectionPlan.tsx +++ b/src/Web/autopilot-monitor-web/app/settings/tenant/sections/SectionPlan.tsx @@ -40,7 +40,7 @@ function FeatureList({ features, checkClass, muted = false }: { features: string return (