sandbox::TouchMemory now doesn't destroy input buffer

Windows 10 RS4 (maybe older as well) x64 implementation of CopyFile expects
that the HANDLE passed to NtCreateFile is not modified upon error. Old
implementation of TouchMemory 'destroys' the buffer before passing it to
the broker. This can be fixed by first reading the original value and then
restoring back at the original place. This has to be done carefully so
optimizer won't remove the code.

R=wfh@chromium.org

Bug: 839775
Change-Id: Iff80f3a5f145b85da2f0ba500e74387fce4e0e4b
Reviewed-on: https://chromium-review.googlesource.com/1061460
Reviewed-by: Will Harris <wfh@chromium.org>
Commit-Queue: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559279}

AUTHORS

@@ -690,6 +690,7 @@ Refael Ackermann <refack@gmail.com>
 Renata Hodovan <rhodovan.u-szeged@partner.samsung.com>
 Rene Bolldorf <rb@radix.io>
 Rene Ladan <r.c.ladan@gmail.com>
+Richard Baranyi <lordprotector@gmail.com>
 Rijubrata Bhaumik <rijubrata.bhaumik@intel.com>
 Riku Voipio <riku.voipio@linaro.org>
 Rob Buis <rob.buis@samsung.com>

sandbox/win/src/file_policy_test.cc

@@ -259,6 +259,21 @@ SBOX_TESTS_COMMAND int File_QueryAttributes(int argc, wchar_t** argv) {
   return SBOX_TEST_FAILED;
 }
 
+// Tries to create a backup of calc.exe in system32 folder. This should fail
+// with ERROR_ACCESS_DENIED if everything is working as expected.
+SBOX_TESTS_COMMAND int File_CopyFile(int argc, wchar_t** argv) {
+  base::string16 calc_path = MakePathToSys(L"calc.exe", false);
+  base::string16 calc_backup_path = MakePathToSys(L"calc.exe.bak", false);
+
+  if (::CopyFile(calc_path.c_str(), calc_backup_path.c_str(), FALSE))
+    return SBOX_TEST_FAILED_TO_EXECUTE_COMMAND;
+
+  if (::GetLastError() != ERROR_ACCESS_DENIED)
+    return SBOX_TEST_FAILED;
+
+  return SBOX_TEST_SUCCEEDED;
+}
+
 TEST(FilePolicyTest, DenyNtCreateCalc) {
   TestRunner runner;
   EXPECT_TRUE(
@@ -664,4 +679,27 @@ TEST(FilePolicyTest, CheckMissingNTPrefixEscape) {
   EXPECT_STREQ(result.c_str(), L"\\/?/?\\C:\\NAME");
 }
 
+TEST(FilePolicyTest, TestCopyFile) {
+  // Check if the test is running Win8 or newer since
+  // MITIGATION_STRICT_HANDLE_CHECKS is not supported on older systems.
+  if (base::win::GetVersion() < base::win::VERSION_WIN8)
+    return;
+
+  TestRunner runner;
+  runner.SetTimeout(2000);
+
+  // Allow read access to calc.exe, this should be on all Windows versions.
+  ASSERT_TRUE(
+      runner.AddRuleSys32(TargetPolicy::FILES_ALLOW_READONLY, L"calc.exe"));
+
+  sandbox::TargetPolicy* policy = runner.GetPolicy();
+
+  // Set proper mitigation.
+  EXPECT_EQ(
+      policy->SetDelayedProcessMitigations(MITIGATION_STRICT_HANDLE_CHECKS),
+      SBOX_ALL_OK);
+
+  ASSERT_EQ(SBOX_TEST_SUCCEEDED, runner.RunTest(L"File_CopyFile"));
+}
+
 }  // namespace sandbox

sandbox/win/src/sandbox_nt_util.cc

@@ -190,14 +190,14 @@ bool InitHeap() {
 int TouchMemory(void* buffer, size_t size_bytes, RequiredAccess intent) {
   const int kPageSize = 4096;
   int dummy = 0;
-  char* start = reinterpret_cast<char*>(buffer);
-  char* end = start + size_bytes - 1;
+  volatile char* start = reinterpret_cast<char*>(buffer);
+  volatile char* end = start + size_bytes - 1;
 
   if (WRITE == intent) {
     for (; start < end; start += kPageSize) {
-      *start = 0;
+      *start = *start;
     }
-    *end = 0;
+    *end = *end;
   } else {
     for (; start < end; start += kPageSize) {
       dummy += *start;

sandbox/win/src/sandbox_nt_util.h

@@ -100,8 +100,6 @@ enum RequiredAccess { READ, WRITE };
 // Performs basic user mode buffer validation. In any case, buffers access must
 // be protected by SEH. intent specifies if the buffer should be tested for read
 // or write.
-// Note that write intent implies destruction of the buffer content (we actually
-// write)
 bool ValidParameter(void* buffer, size_t size, RequiredAccess intent);
 
 // Copies data from a user buffer to our buffer. Returns the operation status.