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, 0) && (flags & FILE_PERSISTENT_ACLS);
+}
+
+// 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))
+        result = ::ConvertSidToStringSid(owner->Owner, sid);
+      ::LocalFree(owner);
+    }
+  }
+  ::CloseHandle(token);
+  return result;
+}
+
+// Populates |sd| suitable for use when creating directories within |path| with
+// ACLs allowing access to only the current owner, admin, and system.
+// NOTE: On success the |sd| parameter must be freed with LocalFree().
+bool SetSecurityDescriptor(const wchar_t* path, PSECURITY_DESCRIPTOR* sd) {
+  *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( // Protected, auto-inherited DACL.
+                    L"D:PAI(A;;FA;;;BA)"  // Admin: Full control.
+                    L"(A;OICI;GA;;;BA)"
+                    L"(A;;FA;;;SY)"  // System: Full control.
+                    L"(A;OICI;GA;;;SY)"
+                    L"(A;OICI;GA;;;CO)"  // Owner: Full control.

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

I haven't digested the nuances around "Creator owner" vs the current owner, so
I'll take this detail on faith + forshaw's stamp of approval.

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

Thinking about it, I'm going to keep the inheritable ACEs inherit-only, which
forshaw@ signed off on earlier. The truth is that they're redundant to the other
ACEs, and that the way I have it now is the normal pattern used throughout
Windows.

Since forshaw@ is on vacation, hopefully I can clarify. The subtlety of the
creator owner relationship is that UAC elevated users and members of the
administrators group will not be granted explicit access by the creator owner
ACEs. Rather, the appropriate privileged account is going to be set as the
creator/owner in those scenarios. The idea here being that when elevated you
don't want to leak a privileged object to your unelevated context.

The other obnoxiously confusing detail (that forshaw@ explained to me earlier)
is that the well-known SID for creator owner doesn't actually have any meaning
in the context of explicit ACEs (it's used only for inheritance). Hence, all the
annoying work to extract the owner from the user's token.

+                    L"(A;;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) {
+bool CreateWorkDir(const wchar_t* base_path, PathString* work_dir,
+                   ProcessExitResult* exit_code) {
   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(base_path, &sa.lpSecurityDescriptor)) {
+    *exit_code = ProcessExitResult(UNABLE_TO_SET_DIRECTORY_ACL,
+                                   ::GetLastError());
+    return false;
+  }
 
-  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
     // 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) {
+bool GetWorkDir(HMODULE module, PathString* work_dir,
+                ProcessExitResult* exit_code) {
   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)) {
+      !CreateWorkDir(base_path.get(), work_dir, exit_code)) {
+    *exit_code = ProcessExitResult(UNABLE_TO_GET_WORK_DIRECTORY);
     // Problem creating the work dir under TEMP path, so try using the
     // current directory as the base path.
     len = ::GetModuleFileName(module, base_path.get(),
                               static_cast<DWORD>(base_path.capacity()));
     if (len >= base_path.capacity() || !len)
       return false;  // Can't even get current directory? Return an error.
 
     wchar_t* name = GetNameFromPathExt(base_path.get(), len);
     if (name == base_path.get())
       return false;  // There was no directory in the string!  Bail out.
 
     *name = L'\0';
 
-    return CreateWorkDir(base_path.get(), work_dir);
+    *exit_code = ProcessExitResult(SUCCESS_EXIT_CODE);

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

I don't think this is quite right since CreateWorkDir has codepaths that return
false without modifying |exit_code|. I think CreateWorkDir needs to be adjusted
so that it populates a value in all cases. For example, the first "return
false;" up there should probably be "*exit_code =
ProcessExitResult(PATH_STRING_OVERFLOW); return false;" Maybe it's simpler for
CreateWorkDir (and this function?) to return a ProcessExitResult rather than a
bool. WDYT?

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

I think I checked that all code paths return correctly, but it is ugly and very
fragile. So, yeah, you're right, I'll fix it up.

Also, I totally missed PATH_STRING_OVERFLOW, which is definitely more
informative.

+    return CreateWorkDir(base_path.get(), work_dir, exit_code);
   }
   return true;
 }
 
 // Returns true for ".." and "." directories.
 bool IsCurrentOrParentDirectory(const wchar_t* dir) {
   return dir &&
          dir[0] == L'.' &&
          (dir[1] == L'\0' || (dir[1] == L'.' && dir[2] == L'\0'));
 }
@@ skipping 144 matching lines @@
   if (!configuration.Initialize(module))
     return ProcessExitResult(GENERIC_INITIALIZATION_FAILURE);
 
   // If the --cleanup switch was specified on the command line, then that means
   // we should only do the cleanup and then exit.
   if (ProcessNonInstallOperations(configuration, &exit_code))
     return exit_code;
 
   // First get a path where we can extract payload
   PathString base_path;
-  if (!GetWorkDir(module, &base_path))
-    return ProcessExitResult(UNABLE_TO_GET_WORK_DIRECTORY);
+  if (!GetWorkDir(module, &base_path, &exit_code))
+    return exit_code;
 
 #if defined(GOOGLE_CHROME_BUILD)
   // Set the magic suffix in registry to try full installer next time. We ignore
   // any errors here and we try to set the suffix for user level unless
   // --system-level is on the command line in which case we set it for system
   // level instead. This only applies to the Google Chrome distribution.
   SetInstallerFlags(configuration);
 #endif
 
   PathString archive_path;
@@ skipping 38 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"