CVE-2016-7838: Untrusted Search Path (CWE-426)
value: Untrusted search path vulnerability in WinSparkle versions prior to 0.5.3 allows remote attackers to execute arbitrary code via a specially crafted executable file in an unspecified directory.
CWE description: The product searches for critical resources using an externally-supplied search path that can point to resources that are not under the product's direct control.
CVSS v3 base score: 7.8
Published: 2017-06-09T16:29Z
Repository: https://github.com/vslavik/winsparkle
Language: C++
Files changed: src/updatedownloader.cpp
Commit date: 2016-12-12 18:32:14 +0100
Commit message: 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).

Diff: diff --git a/src/updatedownloader.cpp b/src/updatedownloader.cpp
index 72b3664..b689330 100644
--- a/src/updatedownloader.cpp
+++ b/src/updatedownloader.cpp
@@ -43,27 +43,34 @@ 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
     // if we put it in $TMP, some DLLs could be there and interfere with the
     // installer.
     //
     // 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;
         RPC_STATUS status = UuidToString(&uuid, &uuidStr);
         dir += reinterpret_cast<wchar_t*>(uuidStr);
@@ -190,10 +197,25 @@ void UpdateDownloader::CleanLeftovers()
 
     std::wstring tmpdir;
     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};
     fos.wFunc = FO_DELETE;
     fos.pFrom = tmpdir.c_str();

Vulnerable code:     wchar_t tmpdir[MAX_PATH+1];
    if ( GetTempPath(MAX_PATH+1, tmpdir) == 0 )
        throw Win32Exception("Cannot create temporary directory");
        dir += L"Update-";

Fixed code: 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;
}

    const std::wstring tmpdir = GetUniqueTempDirectoryPrefix();
    // 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;
    }