fix(update): check the installer's Authenticode signer before running it

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.

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) =>
     /// <summary>Whether <see cref="UpdateAsync"/> installs by itself (<see cref="IsInstalled"/>; tests set it).</summary>
     internal bool InstallsItself { get; init; } = IsInstalled;
 
+    /// <summary>
+    /// Who signed a program, if Windows trusts the signature (<see cref="Authenticode.TrustedSigner"/>; the flag also
+    /// checks revocation). Tests replace it.
+    /// </summary>
+    internal Func<string, bool, string?> SignerOf { get; init; } = Authenticode.TrustedSigner;
+
+    /// <summary>This copy of Filee, whose publisher must have signed the installer too (tests set it).</summary>
+    internal string? OwnProgram { get; init; } = Environment.ProcessPath;
+
     /// <summary>The newer version found by the last check (e.g. "1.0.2"), or null when up to date / unknown.</summary>
     [ObservableProperty]
     [NotifyPropertyChangedFor(nameof(IsUpdateAvailable))]
@@ -268,6 +279,8 @@ public async Task UpdateAsync()
         {
             var progress = new Progress<int>(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<string> DownloadInstallerAsync(string folder, IProgress<int>
         return target;
     }
 
+    /// <summary>
+    /// Throws <see cref="InvalidDataException"/> (and deletes the installer) unless the publisher that signed this copy
+    /// of Filee signed <paramref name="installer"/> 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.
+    /// </summary>
+    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<int>? progress, CancellationToken cancellationToken)
     {
         using var stall = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);

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;
+
+/// <summary>Authenticode checks. Every member is safe to call on any OS.</summary>
+public static partial class Authenticode
+{
+    /// <summary>
+    /// The subject of the certificate that signed <paramref name="path"/> (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.
+    /// </summary>
+    /// <param name="checkRevocation">
+    /// Also asks the certificate authorities whether a certificate in the chain was revoked (needs the network).
+    /// </param>
+    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);
+            }
+        }
+    }
+
+    /// <summary>WINTRUST_ACTION_GENERIC_VERIFY_V2: the Authenticode policy.</summary>
+    private static readonly Guid GenericVerifyV2 = new("00AAC56B-CD44-11D0-8CC2-00C04FC295EE");
+
+    /// <summary>INVALID_HANDLE_VALUE: no window, so WinVerifyTrust never asks the user.</summary>
+    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);
+}

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<HttpRequestException>(() =>
             updates.DownloadInstallerAsync(TempFolder(), null, TestContext.Current.CancellationToken));
     }
 
+    private const string Publisher = "CN=Open Source Developer, Filee";
+
+    /// <summary>An update service whose own program is signed by <paramref name="own"/> and the installer by <paramref name="installer"/>.</summary>
+    private static UpdateService Signed(string? own, string? installer) =>
+        new(NullLogger<UpdateService>.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<InvalidDataException>(() => 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<UpdateService>.Instance, new FakeNetwork("no network"))
+        {
+            OwnProgram = "Filee.exe",
+            SignerOf = (file, revocation) => file == "Filee.exe" || !revocation ? Publisher : null,
+        };
+
+        var error = Assert.Throws<InvalidDataException>(() => 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<UpdateService>.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\<version>).
+        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()
     {