more safe SecureBootCertReader and section plan layout update

This commit is contained in:
Oliver Kieselbach
2026-07-25 12:48:27 +02:00
parent f311e8746b
commit c7ed3ec27d
5 changed files with 166 additions and 44 deletions
+15
View File
@@ -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_<code>`).
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`,
+4
View File
@@ -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.
@@ -161,6 +161,40 @@ namespace AutopilotMonitor.Agent.V2.Core.Tests.Monitoring
Assert.False(string.IsNullOrEmpty(snapshot.KekStatus));
}
/// <summary>
/// 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.
/// </summary>
[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;
@@ -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();
/// <summary>
/// Reads db and KEK and evaluates the certificate presence flags. Never throws;
/// per-variable failures surface as <see cref="UefiSecureBootCertSnapshot.DbStatus"/> /
@@ -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 <see cref="Enabled"/> stays
/// false and the caller reports <see cref="StatusPrivilegeDenied"/>.
/// <para>
/// <b>Crash-safety.</b> This is the repo's only privilege-adjustment code, so the
/// ownership rules are spelled out: the token is held in a <see cref="SafeHandle"/>
/// assigned by the P/Invoke marshaler, so the CLR owns it from the moment
/// <c>OpenProcessToken</c> returns — it is released by the critical finalizer even if
/// this instance never reaches the caller's <c>using</c> (async exception between the
/// call and the assignment). <see cref="Dispose"/> is idempotent, never throws, and
/// releases the handle in a <c>finally</c> 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.
/// </para>
/// </summary>
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();
}
}
}
/// <summary>
/// Owns a process-token handle. <c>ReleaseHandle</c> runs on the finalizer thread when
/// <see cref="SystemEnvironmentPrivilege.Dispose"/> is skipped, so it must not throw —
/// a single <c>CloseHandle</c> P/Invoke satisfies that.
/// </summary>
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)]
@@ -40,7 +40,7 @@ function FeatureList({ features, checkClass, muted = false }: { features: string
return (
<ul className="space-y-2.5">
{features.map((f) => (
<li key={f} className={`flex items-start gap-2 text-sm ${muted ? "text-gray-500" : "text-gray-700"}`}>
<li key={f} className={`flex items-start gap-2 text-sm ${muted ? "text-gray-600" : "text-gray-700"}`}>
<CheckIcon className={`w-4 h-4 mt-0.5 shrink-0 ${checkClass}`} />
<span>{f}</span>
</li>
@@ -55,7 +55,7 @@ function PlusList({ features }: { features: string[] }) {
<ul className="space-y-2.5">
{features.map((f) => (
<li key={f} className="flex items-start gap-2 text-sm font-medium text-gray-900">
<PlusIcon className="w-4 h-4 mt-0.5 shrink-0 text-purple-600" />
<PlusIcon className="w-4 h-4 mt-0.5 shrink-0 text-purple-600 dark:text-purple-400" />
<span>{f}</span>
</li>
))}
@@ -66,6 +66,12 @@ function PlusList({ features }: { features: string[] }) {
/**
* Plan section: two side-by-side plan cards (Community and Enterprise). The tenant's current plan
* is highlighted so the edition is obvious at a glance; the Enterprise card teases what it adds.
*
* Dark mode: the tinted card surfaces use slash-opacity utilities (bg-purple-50/40), which the
* global `.dark .bg-*` override map in globals.css does NOT match — they are separate class tokens.
* Without the explicit `dark:` counterparts below the cards render as a washed-out light overlay on
* the dark page. Secondary copy uses text-gray-600 rather than -400/-500: both of those collapse to
* the same dim #64748b in dark mode, which fails contrast on a tinted card.
*/
export function SectionPlan() {
const { editionInfo, startTrial, startingTrial, user } = useTenantConfig();
@@ -120,16 +126,18 @@ export function SectionPlan() {
{/* Community card */}
<div
className={`rounded-xl border p-6 flex flex-col ${
!isEnterprise ? "border-gray-800 ring-1 ring-gray-800 bg-gray-50/60" : "border-gray-200"
!isEnterprise
? "border-gray-800 ring-1 ring-gray-800 bg-gray-50/60 dark:border-slate-500 dark:ring-slate-500 dark:bg-slate-900/40"
: "border-gray-200"
}`}
>
<div className="flex items-start justify-between gap-2">
<div>
<h3 className="text-lg font-semibold text-gray-900">Community</h3>
<p className="text-sm text-gray-500 mt-0.5">The full product, free</p>
<p className="text-sm text-gray-600 mt-0.5">The full product, free</p>
</div>
{!isEnterprise && (
<span className="inline-flex items-center px-2.5 py-1 rounded-full text-xs font-semibold bg-gray-900 text-white">
<span className="inline-flex items-center px-2.5 py-1 rounded-full text-xs font-semibold bg-gray-900 text-white dark:bg-slate-200 dark:text-slate-900">
Current plan
</span>
)}
@@ -137,7 +145,7 @@ export function SectionPlan() {
<div className="mt-4 mb-5">
<span className="text-2xl font-bold text-gray-900">Free</span>
<span className="text-sm text-gray-500"> — and stays free</span>
<span className="text-sm text-gray-600"> — and stays free</span>
</div>
<FeatureList features={communityFeatures} checkClass="text-emerald-500" />
@@ -146,13 +154,15 @@ export function SectionPlan() {
{/* Enterprise card */}
<div
className={`rounded-xl border p-6 flex flex-col ${
isEnterprise ? "border-purple-500 ring-1 ring-purple-500 bg-purple-50/40" : "border-purple-200 bg-purple-50/20"
isEnterprise
? "border-purple-500 ring-1 ring-purple-500 bg-purple-50/40 dark:bg-purple-950/40"
: "border-purple-200 bg-purple-50/20 dark:bg-purple-950/20"
}`}
>
<div className="flex items-start justify-between gap-2">
<div>
<h3 className="text-lg font-semibold text-purple-900">Enterprise</h3>
<p className="text-sm text-gray-500 mt-0.5">Higher limits, support & MSP</p>
<p className="text-sm text-gray-600 mt-0.5">Higher limits, support & MSP</p>
</div>
{isEnterprise ? (
<span className="inline-flex items-center px-2.5 py-1 rounded-full text-xs font-semibold bg-purple-600 text-white">
@@ -169,18 +179,18 @@ export function SectionPlan() {
<span className="text-2xl font-bold text-purple-900">
{isEnterprise ? "Active" : "Pricing TBA"}
</span>
{!isEnterprise && <span className="text-sm text-gray-500"> — to be announced</span>}
{!isEnterprise && <span className="text-sm text-gray-600"> — to be announced</span>}
</div>
<p className="text-xs font-medium uppercase tracking-wide text-gray-400 mb-2.5">
<p className="text-xs font-medium uppercase tracking-wide text-gray-600 mb-2.5">
Everything in Community
</p>
<FeatureList features={sharedFeatures} checkClass="text-gray-400" muted />
<FeatureList features={sharedFeatures} checkClass="text-gray-500" muted />
<div className="flex items-center gap-3 my-4" aria-hidden="true">
<span className="h-px flex-1 bg-purple-200" />
<span className="text-xs font-semibold uppercase tracking-wide text-purple-600">Plus</span>
<span className="h-px flex-1 bg-purple-200" />
<span className="h-px flex-1 bg-purple-200 dark:bg-purple-800" />
<span className="text-xs font-semibold uppercase tracking-wide text-purple-600 dark:text-purple-300">Plus</span>
<span className="h-px flex-1 bg-purple-200 dark:bg-purple-800" />
</div>
<PlusList features={enterpriseExtras} />
@@ -188,7 +198,7 @@ export function SectionPlan() {
{!isEnterprise && (
<div className="mt-auto pt-5">
{trialConsumed ? (
<p className="text-sm text-gray-500">
<p className="text-sm text-gray-600">
This tenant has already used its Enterprise trial. To move to Enterprise,{" "}
<a
href="https://github.com/okieselbach/Autopilot-Monitor/issues"
@@ -246,7 +256,7 @@ export function SectionPlan() {
</div>
</div>
<p className="text-xs text-gray-400 mt-5">
<p className="text-xs text-gray-600 mt-5">
{isEnterprise && editionInfo.isTrial
? "When the trial ends, the tenant returns to Community automatically."
: "Scope, pricing and timeline for Enterprise will be announced. Community stays free."}