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 507 matching lines @@
 // 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) {

[grt (UTC plus 2), 2015/12/04 16:51:43]

regarding testing, i think it's appropriate to move this utility function into a
new .{cc,h} pair (mini_installer_util?) and create a corresponding _unittest.cc
file. these new sources and the test can then be linked into
setup_unittests.exe. be warned that you'll have to land a change to the signing
script that adds the new files to |extra_files|, |input_headers|, and
|input_files| so that the official build doesn't tip over.

[jschuh, 2015/12/05 01:04:39]

In principle I agree. In practice, this was how I spent my one day of coding for
the next few weeks, so it's going to be quite a while before I can get back to
it and do that. How about I file a bug on it and promise to get it taken care
of?

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

sgtm provided that you satisfy my request below.

   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);
+  wchar_t volume[MAX_PATH];

[grt (UTC plus 2), 2015/12/04 15:37:35]

use PathString here rather than a wchar_t array:
  PathString volume;
  DWORD flags = 0;
  if (::GetVolumePathName(base_path, volume.capacity(), volume.get()) &&
      ::GetVolumeInformation(volume.get(), ...))

(and prefix win32 calls with :: since that seems to be the style of the file)

[jschuh, 2015/12/05 01:04:39]

Done.

+  DWORD flags = 0;
+  if (GetVolumePathName(base_path, volume, sizeof(volume)) &&
+      GetVolumeInformation(volume, nullptr, 0, nullptr, nullptr, &flags,

[grt (UTC plus 2), 2015/12/04 15:37:35]

sadly, we can't use C++-11 yet in mini_installer since (last i checked) the
signing bots are using an old toolchain. for now, use NULL everywhere rather
than nullptr. mmoss is working on resolving this.

[jschuh, 2015/12/05 01:04:39]

Done.

+                           nullptr, 0) &&
+      (flags & FILE_PERSISTENT_ACLS)) {
+    static const wchar_t sddl[] =
+        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;ID;FA;;;CO)"  // Owner: Full control.
+        L"(A;OICIIOID;GA;;;CO)";
+    if (!ConvertStringSecurityDescriptorToSecurityDescriptor(
+            sddl, SDDL_REVISION, &sa.lpSecurityDescriptor, NULL)) {

[grt (UTC plus 2), 2015/12/04 15:37:35]

is it safer to explicitly use SDDL_REVISION_1? MSDN says StringSDRevision must
be that value. i see that sddl.h defines SDDL_REVISION to SDDL_REVISION_1 for
now, but would we necessarily want to pick up a change in a future version of
the SDK automatically?

[jschuh, 2015/12/05 01:04:39]

Done.

+      return false;
+    }
+  }
 
-  int max_attempts = 10;
-  while (max_attempts--) {
+  bool result = false;
+  unsigned int id;
+  RtlGenRandom(&id, sizeof(id));
+  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 : nullptr)) {
       // 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"