Index: chrome/installer/mini_installer/mini_installer.cc |
diff --git a/chrome/installer/mini_installer/mini_installer.cc b/chrome/installer/mini_installer/mini_installer.cc |
index 3ecd6e5fa90c156117f7d6d00e846d551a1a1003..bb4fc8d83b33fdf4bbf75b083790b258ff60e1e7 100644 |
--- a/chrome/installer/mini_installer/mini_installer.cc |
+++ b/chrome/installer/mini_installer/mini_installer.cc |
@@ -22,6 +22,15 @@ |
#pragma comment(linker, "/MERGE:.rdata=.text") |
#include <windows.h> |
+ |
+// #define needed to link in RtlGenRandom(), a.k.a. SystemFunction036. See the |
+// "Community Additions" comment on MSDN here: |
+// http://msdn.microsoft.com/en-us/library/windows/desktop/aa387694.aspx |
+#define SystemFunction036 NTAPI SystemFunction036 |
+#include <NTSecAPI.h> |
+#undef SystemFunction036 |
+ |
+#include <sddl.h> |
#include <shellapi.h> |
#include <stdlib.h> |
@@ -539,6 +548,68 @@ void DeleteExtractedFiles(const wchar_t* base_path, |
::RemoveDirectory(base_path); |
} |
+// Returns true if they supplied path supports ACLs. |
+bool IsAclSupportedForPath(const wchar_t* path) { |
+ PathString volume; |
+ DWORD flags = 0; |
+ return ::GetVolumePathName(path, volume.get(), volume.capacity()) && |
+ ::GetVolumeInformation(volume.get(), NULL, 0, NULL, NULL, &flags, |
+ NULL, 0) || |
+ (flags & FILE_PERSISTENT_ACLS); |
+} |
+ |
+// Retrieves the SID of the current user. If successful, the sid parameter must |
+// be freed with ::LocalFree |
+bool GetCurrentUserSid(wchar_t** sid) { |
+ HANDLE token; |
+ if (!::OpenProcessToken(::GetCurrentProcess(), TOKEN_QUERY, &token)) |
+ return false; |
+ |
+ DWORD size = 0; |
+ TOKEN_USER* user = NULL; |
+ bool result = false; |
+ ::GetTokenInformation(token, TokenUser, &user, 0, &size); |
forshaw
2015/12/05 10:26:31
Technically speaking there's a distinction between
jschuh
2015/12/05 18:37:38
This is why I said from the beginning that I *hate
jschuh
2015/12/05 18:49:14
Just to clarify -- since I've mucked this up a few
grt (UTC plus 2)
2015/12/07 14:56:22
I'm far from an expert at the NT security model. B
jschuh
2015/12/11 04:07:24
Yup, we should be good on all cases now. The redun
|
+ if (size && GetLastError() == ERROR_INSUFFICIENT_BUFFER) { |
+ if (user = reinterpret_cast<TOKEN_USER*>(::LocalAlloc(LPTR, size))) { |
+ if (GetTokenInformation(token, TokenUser, user, size, &size)) |
+ result = ::ConvertSidToStringSid(user->User.Sid, sid); |
+ ::LocalFree(user); |
+ } |
+ } |
+ ::CloseHandle(token); |
+ return result; |
+} |
+ |
+// Sets an ACL allowing only the current user, admin, and system. |
+bool SetSecurityDescriptor(PSECURITY_DESCRIPTOR* sd, const wchar_t* path) { |
+ *sd = NULL; |
+ if (!IsAclSupportedForPath(path)) |
+ return true; |
+ |
+ wchar_t* sid = NULL; |
+ if (!GetCurrentUserSid(&sid)) |
+ return false; |
+ |
+ // The largest SID is under 200 characters, so 300 should give enough slack. |
+ StackString<300> sddl; |
+ bool result = sddl.append( |
+ L"D:PAI(A;ID;FA;;;BA)" // Admin: Full control. |
+ L"(A;OICIIOID;GA;;;BA)" |
+ L"(A;ID;FA;;;SY)" // System: Full control. |
+ L"(A;OICIIOID;GA;;;SY)" |
+ L"(A;OICIIOID;GA;;;CO)" // Owner: Full control. |
+ L"(A;ID;FA;;;") && |
+ sddl.append(sid) && sddl.append(L")"); |
+ |
+ if (result) { |
+ result = ::ConvertStringSecurityDescriptorToSecurityDescriptor( |
+ sddl.get(), SDDL_REVISION_1, sd, NULL); |
+ } |
+ |
+ ::LocalFree(sid); |
+ return result; |
+} |
+ |
// Creates a temporary directory under |base_path| and returns the full path |
// of created directory in |work_dir|. If successful return true, otherwise |
// false. When successful, the returned |work_dir| will always have a trailing |
@@ -563,19 +634,23 @@ bool CreateWorkDir(const wchar_t* base_path, PathString* work_dir) { |
if ((work_dir->capacity() - end) < (_countof("fffff.tmp") + 1)) |
return false; |
- // Generate a unique id. We only use the lowest 20 bits, so take the top |
- // 12 bits and xor them with the lower bits. |
- DWORD id = ::GetTickCount(); |
- id ^= (id >> 12); |
+ // Add an ACL if supported by the filesystem. Otherwise system-level installs |
+ // are potentially vulnerable to file squatting attacks. |
+ SECURITY_ATTRIBUTES sa = {}; |
+ sa.nLength = sizeof(SECURITY_ATTRIBUTES); |
+ if (!SetSecurityDescriptor(&sa.lpSecurityDescriptor, base_path)) |
+ return false; |
jschuh
2015/12/05 01:04:39
@grt - I can make this continue on failure if you
grt (UTC plus 2)
2015/12/07 14:56:22
i'm okay with this as-is, but please rejigger so t
jschuh
2015/12/11 04:07:24
Done.
|
- int max_attempts = 10; |
- while (max_attempts--) { |
+ unsigned int id; |
+ RtlGenRandom(&id, sizeof(id)); |
+ bool result = false; |
+ for (int max_attempts = 10; max_attempts; --max_attempts) { |
// This converts 'id' to a string in the format "78563412" on windows |
// because of little endianness, but we don't care since it's just |
// a name. |
if (!HexEncode(&id, sizeof(id), work_dir->get() + end, |
work_dir->capacity() - end)) { |
- return false; |
+ break; |
} |
// We only want the first 5 digits to remain within the 8.3 file name |
@@ -585,14 +660,19 @@ bool CreateWorkDir(const wchar_t* base_path, PathString* work_dir) { |
// for consistency with the previous implementation which relied on |
// GetTempFileName, we append the .tmp extension. |
work_dir->append(L".tmp"); |
- if (::CreateDirectory(work_dir->get(), NULL)) { |
+ |
+ if (::CreateDirectory(work_dir->get(), |
+ sa.lpSecurityDescriptor ? &sa : NULL)) { |
// Yay! Now let's just append the backslash and we're done. |
- return work_dir->append(L"\\"); |
+ result = work_dir->append(L"\\"); |
+ break; |
} |
++id; // Try a different name. |
} |
- return false; |
+ if (sa.lpSecurityDescriptor) |
+ LocalFree(sa.lpSecurityDescriptor); |
+ return result; |
} |
// Creates and returns a temporary directory in |work_dir| that can be used to |