diff --git a/SECURITY.md b/SECURITY.md index 19661ef92..142c081c0 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -41,7 +41,8 @@ Out of scope: ## How downloads are checked - **Updates:** an installed copy downloads the new installer from GitHub Releases and checks it against the release's - `SHA256SUMS` file before it runs it. + `SHA256SUMS` file before it runs it. It also checks that Windows trusts the installer's Authenticode signature and + that the publisher who signed the installed Filee signed it; an installer signed by anyone else is discarded. - **Engines:** every engine comes from the official location pinned in [`engines.json`](src/Filee.Engines/Infrastructure/engines.json), with its SHA-256 and size. A download that doesn't match is discarded. diff --git a/src/Filee.App/Services/UpdateService.cs b/src/Filee.App/Services/UpdateService.cs index 12d5d6171..c681f6d3c 100644 --- a/src/Filee.App/Services/UpdateService.cs +++ b/src/Filee.App/Services/UpdateService.cs @@ -1,5 +1,6 @@ // Update check against GitHub Releases. When a newer version is out the app says so (sidebar, notice, tray). In an -// installed copy "Update" downloads the new installer, checks it against the release's SHA256SUMS file and runs it; +// installed copy "Update" downloads the new installer, checks it against the release's SHA256SUMS file and that the +// publisher who signed this copy signed it too (Authenticode), and runs it; // Setup closes Filee, updates it in place (settings are kept) and starts it again. A portable copy, a copy run from // source, or a failed download opens a download page instead. The check works in every build. @@ -11,6 +12,7 @@ using System.Text.Json; using Avalonia.Threading; using CommunityToolkit.Mvvm.ComponentModel; +using Filee.Platform.Windows; using Microsoft.Extensions.Logging; namespace Filee.App.Services; @@ -96,6 +98,15 @@ internal static bool IsInstalledIn(string folder) => /// Whether installs by itself (; tests set it). internal bool InstallsItself { get; init; } = IsInstalled; + /// + /// Who signed a program, if Windows trusts the signature (; the flag also + /// checks revocation). Tests replace it. + /// + internal Func SignerOf { get; init; } = Authenticode.TrustedSigner; + + /// This copy of Filee, whose publisher must have signed the installer too (tests set it). + internal string? OwnProgram { get; init; } = Environment.ProcessPath; + /// The newer version found by the last check (e.g. "1.0.2"), or null when up to date / unknown. [ObservableProperty] [NotifyPropertyChangedFor(nameof(IsUpdateAvailable))] @@ -268,6 +279,8 @@ public async Task UpdateAsync() { var progress = new Progress(percent => DownloadPercent = percent); installer = await DownloadInstallerAsync(Path.Combine(Path.GetTempPath(), "Filee-update"), progress); + // WinVerifyTrust hashes the whole installer and may ask the certificate authority about revocation. + await Task.Run(() => CheckPublisher(installer)); } catch (Exception ex) when (ex is HttpRequestException or TaskCanceledException or IOException or InvalidDataException or UnauthorizedAccessException) @@ -344,6 +357,33 @@ internal async Task DownloadInstallerAsync(string folder, IProgress return target; } + /// + /// Throws (and deletes the installer) unless the publisher that signed this copy + /// of Filee signed as well. The checksum file sits on the same release as the + /// installer, so it can't tell a replaced installer from the real one; the signature can. A copy that is not + /// signed itself (built from source) has no publisher to compare with and keeps to the checksum. + /// + internal void CheckPublisher(string installer) + { + var publisher = OwnProgram is null ? null : SignerOf(OwnProgram, false); + if (publisher is null) + { + _log.LogInformation("This copy of Filee is not signed: the installer is checked by its SHA-256 only"); + return; + } + var signer = SignerOf(installer, true); + if (signer == publisher) + return; + // A signature that holds without the revocation check failed only that check: the certificate authority + // could not be reached (offline, a proxy or a firewall) or the certificate was revoked. + var reason = signer is null && SignerOf(installer, false) == publisher + ? $"is signed by {publisher}, but the revocation check failed (the certificate authority could not be " + + "reached, or the certificate was revoked)" + : $"is signed by {signer ?? "no publisher Windows trusts"}, not by {publisher}"; + TryDelete(installer); + throw new InvalidDataException($"{Path.GetFileName(installer)} {reason}"); + } + private async Task DownloadAsync(ReleaseFile file, string path, IProgress? progress, CancellationToken cancellationToken) { using var stall = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); diff --git a/src/Filee.Platform.Windows/Authenticode.cs b/src/Filee.Platform.Windows/Authenticode.cs new file mode 100644 index 000000000..118253bee --- /dev/null +++ b/src/Filee.Platform.Windows/Authenticode.cs @@ -0,0 +1,135 @@ +// Authenticode signatures of programs: whether Windows trusts a file's signature, and who signed it. +// +// The in-app updater checks the installer it downloaded with this before running it. The SHA-256 from the release's +// checksum file only shows the download arrived intact; whoever could replace the installer on the release could +// replace that file too. The signature shows the installer comes from the publisher that signed this copy of Filee. +// WinVerifyTrust checks the signature and its chain, and the signer is read from the same check, so the certificate +// compared is the one Windows verified. + +using System.Runtime.InteropServices; +using System.Runtime.Versioning; +using System.Security.Cryptography; +using System.Security.Cryptography.X509Certificates; + +namespace Filee.Platform.Windows; + +/// Authenticode checks. Every member is safe to call on any OS. +public static partial class Authenticode +{ + /// + /// The subject of the certificate that signed (e.g. "CN=Open Source Developer, …"), or null + /// when the file is not signed, Windows does not trust its signature, or this is not Windows. + /// + /// + /// Also asks the certificate authorities whether a certificate in the chain was revoked (needs the network). + /// + public static string? TrustedSigner(string path, bool checkRevocation) + { + if (!OperatingSystem.IsWindows()) + return null; + try + { + return Verify(path, checkRevocation); + } + catch (Exception ex) when (ex is DllNotFoundException or EntryPointNotFoundException or CryptographicException) + { + return null; + } + } + + [SupportedOSPlatform("windows")] + private static unsafe string? Verify(string path, bool checkRevocation) + { + var action = GenericVerifyV2; + fixed (char* file = Path.GetFullPath(path)) + { + var fileInfo = new WINTRUST_FILE_INFO { cbStruct = (uint)sizeof(WINTRUST_FILE_INFO), pcwszFilePath = (nint)file }; + var data = new WINTRUST_DATA + { + cbStruct = (uint)sizeof(WINTRUST_DATA), + dwUIChoice = WTD_UI_NONE, + fdwRevocationChecks = checkRevocation ? WTD_REVOKE_WHOLECHAIN : WTD_REVOKE_NONE, + dwUnionChoice = WTD_CHOICE_FILE, + pFile = (nint)(&fileInfo), + dwStateAction = WTD_STATEACTION_VERIFY, + dwProvFlags = checkRevocation ? WTD_REVOCATION_CHECK_CHAIN_EXCLUDE_ROOT : 0, + }; + try + { + if (WinVerifyTrust(InvalidWindow, ref action, ref data) != 0) + return null; + // The signer's own certificate: the first one of the first signer's chain. + var provider = WTHelperProvDataFromStateData(data.hWVTStateData); + var signer = provider == 0 ? 0 : WTHelperGetProvSignerFromChain(provider, 0, false, 0); + var certificate = signer == 0 ? 0 : WTHelperGetProvCertFromChain(signer, 0); + if (certificate == 0) + return null; + // CRYPT_PROVIDER_CERT: a DWORD cbStruct, then (aligned to a pointer) the PCCERT_CONTEXT. + var context = Marshal.ReadIntPtr(certificate, IntPtr.Size); + if (context == 0) + return null; + using var x509 = new X509Certificate2(context); + return x509.Subject; + } + finally + { + data.dwStateAction = WTD_STATEACTION_CLOSE; + WinVerifyTrust(InvalidWindow, ref action, ref data); + } + } + } + + /// WINTRUST_ACTION_GENERIC_VERIFY_V2: the Authenticode policy. + private static readonly Guid GenericVerifyV2 = new("00AAC56B-CD44-11D0-8CC2-00C04FC295EE"); + + /// INVALID_HANDLE_VALUE: no window, so WinVerifyTrust never asks the user. + private const nint InvalidWindow = -1; + + private const uint WTD_UI_NONE = 2; + private const uint WTD_REVOKE_NONE = 0; + private const uint WTD_REVOKE_WHOLECHAIN = 1; + private const uint WTD_CHOICE_FILE = 1; + private const uint WTD_STATEACTION_VERIFY = 1; + private const uint WTD_STATEACTION_CLOSE = 2; + private const uint WTD_REVOCATION_CHECK_CHAIN_EXCLUDE_ROOT = 0x80; + + [StructLayout(LayoutKind.Sequential)] + private struct WINTRUST_FILE_INFO + { + public uint cbStruct; + public nint pcwszFilePath; + public nint hFile; + public nint pgKnownSubject; + } + + [StructLayout(LayoutKind.Sequential)] + private struct WINTRUST_DATA + { + public uint cbStruct; + public nint pPolicyCallbackData; + public nint pSIPClientData; + public uint dwUIChoice; + public uint fdwRevocationChecks; + public uint dwUnionChoice; + public nint pFile; + public uint dwStateAction; + public nint hWVTStateData; + public nint pwszURLReference; + public uint dwProvFlags; + public uint dwUIContext; + public nint pSignatureSettings; + } + + [LibraryImport("wintrust.dll")] + private static partial int WinVerifyTrust(nint hwnd, ref Guid action, ref WINTRUST_DATA data); + + [LibraryImport("wintrust.dll")] + private static partial nint WTHelperProvDataFromStateData(nint stateData); + + [LibraryImport("wintrust.dll")] + private static partial nint WTHelperGetProvSignerFromChain(nint providerData, uint signerIndex, + [MarshalAs(UnmanagedType.Bool)] bool counterSigner, uint counterSignerIndex); + + [LibraryImport("wintrust.dll")] + private static partial nint WTHelperGetProvCertFromChain(nint signer, uint certificateIndex); +} diff --git a/tests/Filee.App.Tests/UpdateServiceTests.cs b/tests/Filee.App.Tests/UpdateServiceTests.cs index 137abb56b..31c0ecd4d 100644 --- a/tests/Filee.App.Tests/UpdateServiceTests.cs +++ b/tests/Filee.App.Tests/UpdateServiceTests.cs @@ -1,10 +1,12 @@ using System.Net; using System.Net.Sockets; +using System.Runtime.InteropServices; using System.Security.Cryptography; using System.Text; using System.Text.Json; using Avalonia.Headless.XUnit; using Filee.App.Services; +using Filee.Platform.Windows; using Microsoft.Extensions.Logging.Abstractions; namespace Filee.App.Tests; @@ -174,6 +176,110 @@ await Assert.ThrowsAsync(() => updates.DownloadInstallerAsync(TempFolder(), null, TestContext.Current.CancellationToken)); } + private const string Publisher = "CN=Open Source Developer, Filee"; + + /// An update service whose own program is signed by and the installer by . + private static UpdateService Signed(string? own, string? installer) => + new(NullLogger.Instance, new FakeNetwork("no network")) + { + OwnProgram = "Filee.exe", + SignerOf = (path, _) => path == "Filee.exe" ? own : installer, + }; + + [Fact] + public void Installer_signed_by_the_same_publisher_is_kept() + { + var path = Path.Combine(TempFolder(), "Filee-9.9.9-win-Setup.exe"); + File.WriteAllText(path, "setup"); + + Signed(Publisher, Publisher).CheckPublisher(path); + + Assert.True(File.Exists(path)); + } + + [Theory] + [InlineData("CN=Someone Else")] + [InlineData(null)] // not signed, or signed with a certificate Windows does not trust + public void Installer_signed_by_anyone_else_is_thrown_away(string? signer) + { + var path = Path.Combine(TempFolder(), "Filee-9.9.9-win-Setup.exe"); + File.WriteAllText(path, "setup"); + + Assert.Throws(() => Signed(Publisher, signer).CheckPublisher(path)); + Assert.False(File.Exists(path)); + } + + [Fact] + public void Failed_revocation_check_is_named_in_the_error() + { + // Offline or behind a proxy, the signature is fine but the certificate authority can't be asked: the + // installer is still thrown away, and the log says why instead of blaming the publisher. + var path = Path.Combine(TempFolder(), "Filee-9.9.9-win-Setup.exe"); + File.WriteAllText(path, "setup"); + var updates = new UpdateService(NullLogger.Instance, new FakeNetwork("no network")) + { + OwnProgram = "Filee.exe", + SignerOf = (file, revocation) => file == "Filee.exe" || !revocation ? Publisher : null, + }; + + var error = Assert.Throws(() => updates.CheckPublisher(path)); + + Assert.Contains("revocation check failed", error.Message); + Assert.False(File.Exists(path)); + } + + [Fact] + public void Installer_is_checked_for_revocation_but_this_copy_is_not() + { + var checks = new List<(string Path, bool Revocation)>(); + var updates = new UpdateService(NullLogger.Instance, new FakeNetwork("no network")) + { + OwnProgram = "Filee.exe", + SignerOf = (path, revocation) => + { + checks.Add((path, revocation)); + return Publisher; + }, + }; + + updates.CheckPublisher("Setup.exe"); + + Assert.Equal([("Filee.exe", false), ("Setup.exe", true)], checks); + } + + [Fact] + public void Unsigned_copy_keeps_to_the_checksum() + { + // Built from source: there is no publisher to compare with, so the installer is not refused for its signature. + var path = Path.Combine(TempFolder(), "Filee-9.9.9-win-Setup.exe"); + File.WriteAllText(path, "setup"); + + Signed(null, null).CheckPublisher(path); + + Assert.True(File.Exists(path)); + } + + [Fact] + public void Unsigned_file_has_no_trusted_signer() + { + var path = Path.Combine(TempFolder(), "unsigned.exe"); + File.WriteAllBytes(path, RandomNumberGenerator.GetBytes(4_096)); + + Assert.Null(Authenticode.TrustedSigner(path, checkRevocation: false)); + } + + [Fact] + public void Signed_program_has_its_publisher_as_signer() + { + Assert.SkipUnless(OperatingSystem.IsWindows(), "Authenticode is Windows only"); + // dotnet.exe carries Microsoft's embedded signature; it sits three folders above the shared runtime + // (dotnet\shared\Microsoft.NETCore.App\). + var dotnet = Path.GetFullPath(Path.Combine(RuntimeEnvironment.GetRuntimeDirectory(), "..", "..", "..", "dotnet.exe")); + Assert.SkipUnless(File.Exists(dotnet), $"no {dotnet}"); + + Assert.Contains("Microsoft", Authenticode.TrustedSigner(dotnet, checkRevocation: false)); + } + [Fact] public void Installer_runs_without_the_wizard_and_without_restarting_windows() {