Skip to content

Commit

Permalink
Verify UpdateTempDir isn't maliciously modified
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
vslavik committed Dec 12, 2016
1 parent 786d180 commit bb45485
Showing 1 changed file with 27 additions and 5 deletions.
32 changes: 27 additions & 5 deletions src/updatedownloader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -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};
Expand Down

0 comments on commit bb45485

Please sign in to comment.