mirror of
https://github.com/okieselbach/AutopilotMonitor.git
synced 2026-10-24 20:53:06 +00:00
fix(security): self-update verifies and extracts the ZIP through one exclusive FileShare.None handle — SHA-256 is computed on the same stream the ZipArchive reads, so the file can no longer be swapped between verification and extraction (TOCTOU); handle is released before cleanup, open failure aborts with integrity_mismatch marker
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -1,4 +1,4 @@
|
|||||||
using System;
|
using System;
|
||||||
using System.Diagnostics;
|
using System.Diagnostics;
|
||||||
using System.IO;
|
using System.IO;
|
||||||
using System.IO.Compression;
|
using System.IO.Compression;
|
||||||
@@ -268,8 +268,10 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Runtime
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Step 3: Download ZIP (timeout = downloadTimeoutMs)
|
// Step 3: Download ZIP (timeout = downloadTimeoutMs)
|
||||||
// ZIP lives under %ProgramData% (SYSTEM/Admin ACL) instead of C:\Windows\Temp so
|
// ZIP lives under %ProgramData% instead of C:\Windows\Temp so a non-admin local
|
||||||
// a non-admin local user cannot plant a junction at the ZIP filename.
|
// user cannot plant a junction at the ZIP filename. The directory inherits the
|
||||||
|
// ProgramData ACL (no explicit hardening — the updater runs during OOBE where only
|
||||||
|
// SYSTEM exists); the verify→extract TOCTOU is closed by the exclusive handle below.
|
||||||
var zipDir = Environment.ExpandEnvironmentVariables(Constants.AgentUpdateDownloadDir);
|
var zipDir = Environment.ExpandEnvironmentVariables(Constants.AgentUpdateDownloadDir);
|
||||||
Directory.CreateDirectory(zipDir);
|
Directory.CreateDirectory(zipDir);
|
||||||
var zipPath = Path.Combine(zipDir, "AutopilotMonitor-Agent-Update.zip");
|
var zipPath = Path.Combine(zipDir, "AutopilotMonitor-Agent-Update.zip");
|
||||||
@@ -304,10 +306,51 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Runtime
|
|||||||
expectedSha256 = null;
|
expectedSha256 = null;
|
||||||
}
|
}
|
||||||
|
|
||||||
var verifySw = Stopwatch.StartNew();
|
// Steps 3b + 4 share ONE exclusive handle (FileShare.None): the bytes that are
|
||||||
var integrityOk = VerifyZipIntegrity(zipPath, expectedSha256, log);
|
// hashed are the bytes that get extracted. With separate opens a local user who
|
||||||
verifySw.Stop();
|
// pre-owned the Updates directory could swap the ZIP after the hash passed and
|
||||||
verifyMs = verifySw.ElapsedMilliseconds;
|
// before extraction (TOCTOU). The handle is disposed BEFORE CleanupStaging so the
|
||||||
|
// ZIP delete does not fail with a sharing violation.
|
||||||
|
FileStream zipStream;
|
||||||
|
try
|
||||||
|
{
|
||||||
|
zipStream = new FileStream(zipPath, FileMode.Open, FileAccess.Read, FileShare.None);
|
||||||
|
}
|
||||||
|
catch (Exception ex)
|
||||||
|
{
|
||||||
|
log($"Self-update: cannot open downloaded ZIP exclusively — {ex.Message}. Aborting update.");
|
||||||
|
CleanupStaging(null, zipPath);
|
||||||
|
if (writeMarkers)
|
||||||
|
WriteSkipMarker("integrity_mismatch", currentVersion, latestVersion, "downloaded ZIP could not be opened exclusively", log);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
bool integrityOk;
|
||||||
|
bool extractOk;
|
||||||
|
var stagingDir = Environment.ExpandEnvironmentVariables(Constants.AgentUpdateStagingDir);
|
||||||
|
using (zipStream)
|
||||||
|
{
|
||||||
|
var verifySw = Stopwatch.StartNew();
|
||||||
|
integrityOk = VerifyZipIntegrity(zipStream, expectedSha256, log);
|
||||||
|
verifySw.Stop();
|
||||||
|
verifyMs = verifySw.ElapsedMilliseconds;
|
||||||
|
|
||||||
|
if (integrityOk)
|
||||||
|
{
|
||||||
|
// PR5: integrity verified — silent before, opaque whether the verify step actually ran.
|
||||||
|
log($"Self-update: ZIP integrity verified ({verifyMs}ms)");
|
||||||
|
|
||||||
|
// Step 4: Extract to staging directory (same handle as the hash above)
|
||||||
|
var extractSw = Stopwatch.StartNew();
|
||||||
|
extractOk = ExtractToStaging(zipStream, stagingDir, log);
|
||||||
|
extractSw.Stop();
|
||||||
|
extractMs = extractSw.ElapsedMilliseconds;
|
||||||
|
}
|
||||||
|
else
|
||||||
|
{
|
||||||
|
extractOk = false;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
if (!integrityOk)
|
if (!integrityOk)
|
||||||
{
|
{
|
||||||
@@ -316,15 +359,6 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Runtime
|
|||||||
WriteSkipMarker("integrity_mismatch", currentVersion, latestVersion, "SHA-256 mismatch on downloaded ZIP", log);
|
WriteSkipMarker("integrity_mismatch", currentVersion, latestVersion, "SHA-256 mismatch on downloaded ZIP", log);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
// PR5: integrity verified — silent before, opaque whether the verify step actually ran.
|
|
||||||
log($"Self-update: ZIP integrity verified ({verifyMs}ms)");
|
|
||||||
|
|
||||||
// Step 4: Extract to staging directory
|
|
||||||
var stagingDir = Environment.ExpandEnvironmentVariables(Constants.AgentUpdateStagingDir);
|
|
||||||
var extractSw = Stopwatch.StartNew();
|
|
||||||
var extractOk = ExtractToStaging(zipPath, stagingDir, log);
|
|
||||||
extractSw.Stop();
|
|
||||||
extractMs = extractSw.ElapsedMilliseconds;
|
|
||||||
if (!extractOk)
|
if (!extractOk)
|
||||||
{
|
{
|
||||||
if (writeMarkers)
|
if (writeMarkers)
|
||||||
@@ -444,8 +478,10 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Runtime
|
|||||||
/// <summary>
|
/// <summary>
|
||||||
/// Verifies the SHA-256 hash of the downloaded ZIP against the expected hash.
|
/// Verifies the SHA-256 hash of the downloaded ZIP against the expected hash.
|
||||||
/// Returns true if the hash matches or if no expected hash is available (backward compat).
|
/// Returns true if the hash matches or if no expected hash is available (backward compat).
|
||||||
|
/// Operates on the caller's exclusive handle (not the path) so the verified bytes are the
|
||||||
|
/// ones later extracted; the stream is rewound to position 0 on return.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
private static bool VerifyZipIntegrity(string zipPath, string expectedSha256, Action<string> log)
|
private static bool VerifyZipIntegrity(FileStream zipStream, string expectedSha256, Action<string> log)
|
||||||
{
|
{
|
||||||
if (string.IsNullOrWhiteSpace(expectedSha256))
|
if (string.IsNullOrWhiteSpace(expectedSha256))
|
||||||
{
|
{
|
||||||
@@ -456,9 +492,10 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Runtime
|
|||||||
try
|
try
|
||||||
{
|
{
|
||||||
using (var sha256 = SHA256.Create())
|
using (var sha256 = SHA256.Create())
|
||||||
using (var stream = File.OpenRead(zipPath))
|
|
||||||
{
|
{
|
||||||
var hashBytes = sha256.ComputeHash(stream);
|
zipStream.Position = 0;
|
||||||
|
var hashBytes = sha256.ComputeHash(zipStream);
|
||||||
|
zipStream.Position = 0;
|
||||||
var actualHash = BitConverter.ToString(hashBytes).Replace("-", "").ToLowerInvariant();
|
var actualHash = BitConverter.ToString(hashBytes).Replace("-", "").ToLowerInvariant();
|
||||||
|
|
||||||
if (string.Equals(actualHash, expectedSha256.ToLowerInvariant(), StringComparison.Ordinal))
|
if (string.Equals(actualHash, expectedSha256.ToLowerInvariant(), StringComparison.Ordinal))
|
||||||
@@ -576,7 +613,7 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Runtime
|
|||||||
/// the staging dir (e.g. ..\..\System32\evil.dll). We iterate entries manually and
|
/// the staging dir (e.g. ..\..\System32\evil.dll). We iterate entries manually and
|
||||||
/// reject any whose resolved absolute path falls outside the staging root.
|
/// reject any whose resolved absolute path falls outside the staging root.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
private static bool ExtractToStaging(string zipPath, string stagingDir, Action<string> log)
|
private static bool ExtractToStaging(FileStream zipStream, string stagingDir, Action<string> log)
|
||||||
{
|
{
|
||||||
try
|
try
|
||||||
{
|
{
|
||||||
@@ -592,7 +629,8 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Runtime
|
|||||||
stagingRoot += Path.DirectorySeparatorChar;
|
stagingRoot += Path.DirectorySeparatorChar;
|
||||||
|
|
||||||
int fileCount = 0;
|
int fileCount = 0;
|
||||||
using (var archive = ZipFile.OpenRead(zipPath))
|
zipStream.Position = 0;
|
||||||
|
using (var archive = new ZipArchive(zipStream, ZipArchiveMode.Read, leaveOpen: true))
|
||||||
{
|
{
|
||||||
foreach (var entry in archive.Entries)
|
foreach (var entry in archive.Entries)
|
||||||
{
|
{
|
||||||
|
|||||||
Reference in New Issue
Block a user