Add a restricted ACL to installer temp directory

chrome/installer/mini_installer/mini_installer.cc

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 Owner and User from a DACL
perspective. However it'll probably only be an issue in UAC scenarios, where
normal user is their own SID but elevated is admin group SID. As this directory
is ephemeral anyway and we probably don't care too much about UAC issues no
reason to change.

[jschuh, 2015/12/05 18:37:38]

This is why I said from the beginning that I *hate* the Windows security
descriptor APIs. It's like they're intentionally designed to be error prone and
painful to use.

Accepting that, you did use TokenOwner in your original comment, and it's a
trivial switch. So, I'll fix it.

[jschuh, 2015/12/05 18:49:14]

Just to clarify -- since I've mucked this up a few times already -- using
TokenOwner means that under UAC elevation the ACL grants access to the admin but
not the normal user, correct? So, it seems worth making that small change to
ensure the directory is always restricted to the most privileged context (even
if that is redundant in the scenarios we care about).

[grt (UTC plus 2), 2015/12/07 14:56:22]

I'm far from an expert at the NT security model. Be aware that there are three
or four ways the installer can be run:
1. as a normal user for per-user installs
2. as an elevated user for per-machine installs
3. as SYSTEM for per-machine installs and updates
4. as whatever msiexec uses (SYSTEM? something else?) when it runs us for
per-machine installs

[jschuh, 2015/12/11 04:07:24]

Yup, we should be good on all cases now. The redundant scenario I was referring
to is the normal user case, where this basically duplicates the existing ACL
assuming that %TEMP% lives under the user's profile.

+  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 find it preferable.

[grt (UTC plus 2), 2015/12/07 14:56:22]

i'm okay with this as-is, but please rejigger so that there's a distinct
ExitCode value for "failed to set security descriptor on work dir" so that we
can see the extent to which this is happening in the wild.

[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