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..41afccbfd9fbb9376b0a75493e00312df1910184 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,70 @@ void DeleteExtractedFiles(const wchar_t* base_path,
   ::RemoveDirectory(base_path);
 }
 
+// Returns true if the 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,

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

did "git cl format" do this indentation? i would expect either six spaces (four
more than the previous line) or aligned with ::GetVolumePathName.

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

VS keeps trying to cleverly reformat things after git-cl-format. And I'm moving
between machines often enough that I don't have them all set up correctly.

+    0) || (flags & FILE_PERSISTENT_ACLS);

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

|| -> &&

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

Done. Forgot to reverse all the conditionals when I moved it out to its own
function that.

+}
+
+// Retrieves the SID of the default owner for objects created by this user
+// token (accounting for different behavior under UAC elevation, etc.).
+// NOTE: On success the |sid| parameter must be freed with LocalFree().
+bool GetCurrentOwnerSid(wchar_t** sid) {
+  HANDLE token;
+  if (!::OpenProcessToken(::GetCurrentProcess(), TOKEN_QUERY, &token))
+    return false;
+
+  DWORD size = 0;
+  TOKEN_OWNER* owner = NULL;
+  bool result = false;
+  // We get the TokenOwner rather than the TokenUser because e.g. under UAC
+  // elevation we want the admin to own the directory rather than the user.
+  ::GetTokenInformation(token, TokenOwner, &owner, 0, &size);
+  if (size && GetLastError() == ERROR_INSUFFICIENT_BUFFER) {
+    if (owner = reinterpret_cast<TOKEN_OWNER*>(::LocalAlloc(LPTR, size))) {
+      if (GetTokenInformation(token, TokenOwner, owner, size, &size))

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

nit: ::GetTokenInformation

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

Done.

+        result = ::ConvertSidToStringSid(owner->Owner, sid);
+      ::LocalFree(owner);
+    }
+  }
+  ::CloseHandle(token);
+  return result;
+}
+
+// Sets an ACL allowing only the current owner, admin, and system.

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

suggested comment (assuming it's correct):
// Populates |sd| suitable for use when creating directories within |path| with
ACLs allowing access to only the current owner, admin, and system.

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

Done.

+// NOTE: On success the |sd| parameter must be freed with LocalFree().
+bool SetSecurityDescriptor(PSECURITY_DESCRIPTOR* sd, const wchar_t* path) {

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

swap sd and path since in params should precede out params

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

Done.

+  *sd = NULL;
+  if (!IsAclSupportedForPath(path))
+    return true;
+
+  wchar_t* sid = NULL;
+  if (!GetCurrentOwnerSid(&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.

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

nit: break this line just before the open paren so that only the D:PAI is on
this line, and all subsequent lines contain the individual ACE strings. then the
comment for this line could be "Protected, auto-inherited DACL"

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

i think the "ID" flag should be removed here and below. msdn says it "Indicates
that the ACE was inherited. The system sets this bit when it propagates an
inherited ACE to a child object."

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

to be utterly pedantic, each of these should have a trailing semicolon to fit
the format shown in
https://msdn.microsoft.com/library/windows/desktop/aa374928.aspx:

"ace_type;ace_flags;rights;object_guid;inherit_object_guid;account_sid;(resource_attribute)"

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

D'oh. cargo cult. Done.

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

Done.

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

The documentation is a lie! At least, it went all kablooey when I tried to leave
the trailing semicolons.

[grt (UTC plus 2), 2015/12/14 18:59:26]

Ah, I meant like this:
  bool result = sddl.append(L"D:PAI"  // Protected, auto-inherited DACL.
                    L"(A;;FA;;;BA)"  // Admin: Full control.
so that all lines but the first specify a single ACE string each

[jschuh, 2015/12/15 20:58:58]

I've now adjusted the first line, but I think it makes sense to keep the inherit
and non-inherit permissions grouped by the principal, as it currently is.

+    L"(A;OICIIOID;GA;;;BA)"

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

nit: each of these should be four-space indented

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

IIUC:
OI - files in this directory will inherit the ACE
CI - directories in this directory will inherit the ACE
IO - the ace does not apply to this directory (only to the contained files and
directories)
is this what you desire? should the ACE to apply to |path| itself?

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

Oops. My brain keeps thinking "temp directory" rather than "directory inside
system temp". So, the IO is wrong.

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

"Visual Studio!!!!!!!"

[grt (UTC plus 2), 2015/12/14 18:59:26]

Give up and use a real editor. :-)

+    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 +636,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;
 
-  int max_attempts = 10;
-  while (max_attempts--) {
+  unsigned int id;
+  RtlGenRandom(&id, sizeof(id));

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

nit: ::RtlGenRandom

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

Done.

+  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 +662,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