Add a restricted ACL to installer temp directory

chrome/installer/mini_installer/mini_installer.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // mini_installer.exe is the first exe that is run when chrome is being
 // installed or upgraded. It is designed to be extremely small (~5KB with no
 // extra resources linked) and it has two main jobs:
 //   1) unpack the resources (possibly decompressing some)
 //   2) run the real installer (setup.exe) with appropriate flags.
 //
 // In order to be really small the app doesn't link against the CRT and
 // defines the following compiler/linker flags:
 //   EnableIntrinsicFunctions="true" compiler: /Oi
 //   BasicRuntimeChecks="0"
 //   BufferSecurityCheck="false" compiler: /GS-
 //   EntryPointSymbol="MainEntryPoint" linker: /ENTRY
 //   IgnoreAllDefaultLibraries="true" linker: /NODEFAULTLIB
 //   OptimizeForWindows98="1"  liker: /OPT:NOWIN98
 //   linker: /SAFESEH:NO
 
 // have the linker merge the sections, saving us ~500 bytes.
 #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>
 
 #include "chrome/installer/mini_installer/appid.h"
 #include "chrome/installer/mini_installer/configuration.h"
 #include "chrome/installer/mini_installer/decompress.h"
 #include "chrome/installer/mini_installer/exit_code.h"
 #include "chrome/installer/mini_installer/mini_installer_constants.h"
 #include "chrome/installer/mini_installer/mini_string.h"
 #include "chrome/installer/mini_installer/pe_resource.h"
@@ skipping 497 matching lines @@
 // Deletes given files and working dir.
 void DeleteExtractedFiles(const wchar_t* base_path,
                           const wchar_t* archive_path,
                           const wchar_t* setup_path) {
   ::DeleteFile(archive_path);
   ::DeleteFile(setup_path);
   // Delete the temp dir (if it is empty, otherwise fail).
   ::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
 // backslash and this function requires that |base_path| always includes a
 // trailing backslash as well.
 // We do not use GetTempFileName here to avoid running into AV software that
 // might hold on to the temp file as soon as we create it and then we can't
 // delete it and create a directory in its place.  So, we use our own mechanism
 // for creating a directory with a hopefully-unique name.  In the case of a
 // collision, we retry a few times with a new name before failing.
 bool CreateWorkDir(const wchar_t* base_path, PathString* work_dir) {
   if (!work_dir->assign(base_path) || !work_dir->append(kTempPrefix))
     return false;
 
   // Store the location where we'll append the id.
   size_t end = work_dir->length();
 
   // Check if we'll have enough buffer space to continue.
   // The name of the directory will use up 11 chars and then we need to append
   // the trailing backslash and a terminator.  We've already added the prefix
   // to the buffer, so let's just make sure we've got enough space for the rest.
   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
     // format (compliant with previous implementation).
     work_dir->truncate_at(end + 5);
 
     // 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
 // extract mini_installer payload. |work_dir| ends with a path separator.
 bool GetWorkDir(HMODULE module, PathString* work_dir) {
   PathString base_path;
   DWORD len = ::GetTempPath(static_cast<DWORD>(base_path.capacity()),
                             base_path.get());
   if (!len || len >= base_path.capacity() ||
       !CreateWorkDir(base_path.get(), work_dir)) {
@@ skipping 228 matching lines @@
 #pragma function(memset)
 void* memset(void* dest, int c, size_t count) {
   void* start = dest;
   while (count--) {
     *reinterpret_cast<char*>(dest) = static_cast<char>(c);
     dest = reinterpret_cast<char*>(dest) + 1;
   }
   return start;
 }
 }  // extern "C"