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 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
 // 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;

[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
     // 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"