From bb454857348245a7397f9e4fbb3a902f4ac25913 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Va=CC=81clav=20Slavi=CC=81k?= <vaclav@slavik.io>
Date: Mon, 12 Dec 2016 18:32:14 +0100
Subject: [PATCH] Verify UpdateTempDir isn't maliciously modified

Don't just delete the stored updates temporary directory on launch, but
validate the registry key to ensure it is in an expected location and
follows WinSparkle's naming convention. This is to prevent malicious
users from modifying this registry key and forcing the host app to
delete arbitrary directories (a user being able to delete the key would
be able to delete the directory themselves, so it's not a serious issue,
but it nevertheless is bad behavior and shouldn't be possible).
---
 src/updatedownloader.cpp | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/src/updatedownloader.cpp b/src/updatedownloader.cpp
index 72b36642..b6893305 100644
--- a/src/updatedownloader.cpp
+++ b/src/updatedownloader.cpp
@@ -45,6 +45,17 @@ namespace winsparkle
 namespace
 {
 
+std::wstring GetUniqueTempDirectoryPrefix()
+{
+    wchar_t tmpdir[MAX_PATH + 1];
+    if (GetTempPath(MAX_PATH + 1, tmpdir) == 0)
+        throw Win32Exception("Cannot create temporary directory");
+
+    std::wstring dir(tmpdir);
+    dir += L"Update-";
+    return dir;
+}
+
 std::wstring CreateUniqueTempDirectory()
 {
     // We need to put downloaded updates into a directory of their own, because
@@ -53,15 +64,11 @@ std::wstring CreateUniqueTempDirectory()
     //
     // This code creates a new randomized directory name and tries to create it;
     // this process is repeated if the directory already exists.
-    wchar_t tmpdir[MAX_PATH+1];
-    if ( GetTempPath(MAX_PATH+1, tmpdir) == 0 )
-        throw Win32Exception("Cannot create temporary directory");
+    const std::wstring tmpdir = GetUniqueTempDirectoryPrefix();
 
     for ( ;; )
     {
         std::wstring dir(tmpdir);
-        dir += L"Update-";
-
         UUID uuid;
         UuidCreate(&uuid);
         RPC_WSTR uuidStr;
@@ -192,6 +199,21 @@ void UpdateDownloader::CleanLeftovers()
     if ( !Settings::ReadConfigValue("UpdateTempDir", tmpdir) )
         return;
 
+    // Check that the directory actually is a valid update temp dir, to prevent
+    // malicious users from forcing us into deleting arbitrary directories:
+    try
+    {
+        if (tmpdir.find(GetUniqueTempDirectoryPrefix()) != 0)
+        {
+            Settings::DeleteConfigValue("UpdateTempDir");
+            return;
+        }
+    }
+    catch (Win32Exception&) // cannot determine temp directory
+    {
+        return;
+    }
+
     tmpdir.append(1, '\0'); // double NULL-terminate for SHFileOperation
 
     SHFILEOPSTRUCT fos = {0};