Restrict file creation disposition in Windows sandbox

Fixing a test bug as well.

BUG=438157
R=wfh@chromium.org

Review URL: https://codereview.chromium.org/771313002

Cr-Commit-Position: refs/heads/master@{#312738}

5 files changed, 42 insertions, 13 deletions

diff --git a/sandbox/win/src/file_policy_test.cc b/sandbox/win/src/file_policy_test.cc
index e47c6e5..199dd4c 100644
--- a/sandbox/win/src/file_policy_test.cc
+++ b/sandbox/win/src/file_policy_test.cc
@@ -28,34 +28,47 @@ namespace sandbox {
 const ULONG kSharing = FILE_SHARE_WRITE | FILE_SHARE_READ | FILE_SHARE_DELETE;
 
 // Creates a file using different desired access. Returns if the call succeeded
-// or not.  The first argument in argv is the filename. If the second argument
-// is "read", we try read only access. Otherwise we try read-write access.
+// or not.  The first argument in argv is the filename. The second argument
+// determines the type of access and the dispositino of the file.
 SBOX_TESTS_COMMAND int File_Create(int argc, wchar_t **argv) {
   if (argc != 2)
     return SBOX_TEST_FAILED_TO_EXECUTE_COMMAND;
 
-  bool read = (_wcsicmp(argv[0], L"Read") == 0);
+  std::wstring operation(argv[0]);
 
-  if (read) {
+  if (operation == L"Read") {
     base::win::ScopedHandle file1(CreateFile(
         argv[1], GENERIC_READ, kSharing, NULL, OPEN_EXISTING, 0, NULL));
     base::win::ScopedHandle file2(CreateFile(
         argv[1], FILE_EXECUTE, kSharing, NULL, OPEN_EXISTING, 0, NULL));
 
-    if (file1.Get() && file2.Get())
-      return SBOX_TEST_SUCCEEDED;
-    return SBOX_TEST_DENIED;
-  } else {
+    if (file1.IsValid() == file2.IsValid())
+      return file1.IsValid() ? SBOX_TEST_SUCCEEDED : SBOX_TEST_DENIED;
+    return file1.IsValid() ? SBOX_TEST_FIRST_ERROR : SBOX_TEST_SECOND_ERROR;
+
+  } else if (operation == L"Write") {
     base::win::ScopedHandle file1(CreateFile(
         argv[1], GENERIC_ALL, kSharing, NULL, OPEN_EXISTING, 0, NULL));
     base::win::ScopedHandle file2(CreateFile(
         argv[1], GENERIC_READ | FILE_WRITE_DATA, kSharing, NULL, OPEN_EXISTING,
         0, NULL));
 
-    if (file1.Get() && file2.Get())
-      return SBOX_TEST_SUCCEEDED;
-    return SBOX_TEST_DENIED;
+    if (file1.IsValid() == file2.IsValid())
+      return file1.IsValid() ? SBOX_TEST_SUCCEEDED : SBOX_TEST_DENIED;
+    return file1.IsValid() ? SBOX_TEST_FIRST_ERROR : SBOX_TEST_SECOND_ERROR;
+
+  } else if (operation == L"ReadCreate") {
+    base::win::ScopedHandle file2(CreateFile(
+        argv[1], GENERIC_READ, kSharing, NULL, CREATE_NEW, 0, NULL));
+    base::win::ScopedHandle file1(CreateFile(
+        argv[1], GENERIC_READ, kSharing, NULL, CREATE_ALWAYS, 0, NULL));
+
+    if (file1.IsValid() == file2.IsValid())
+      return file1.IsValid() ? SBOX_TEST_SUCCEEDED : SBOX_TEST_DENIED;
+    return file1.IsValid() ? SBOX_TEST_FIRST_ERROR : SBOX_TEST_SECOND_ERROR;
   }
+
+  return SBOX_TEST_INVALID_PARAMETER;
 }
 
 SBOX_TESTS_COMMAND int File_Win32Create(int argc, wchar_t **argv) {
@@ -295,15 +308,21 @@ TEST(FilePolicyTest, AllowReadOnly) {
 
   wchar_t command_read[MAX_PATH + 20] = {0};
   wsprintf(command_read, L"File_Create Read \"%ls\"", temp_file_name);
+  wchar_t command_read_create[MAX_PATH + 20] = {0};
+  wsprintf(command_read_create, L"File_Create ReadCreate \"%ls\"",
+           temp_file_name);
   wchar_t command_write[MAX_PATH + 20] = {0};
   wsprintf(command_write, L"File_Create Write \"%ls\"", temp_file_name);
 
-  // Verify that we have read access after revert.
-  EXPECT_EQ(SBOX_TEST_SUCCEEDED, runner.RunTest(command_read));
+  // Verify that we cannot create the file after revert.
+  EXPECT_EQ(SBOX_TEST_DENIED, runner.RunTest(command_read_create));
 
   // Verify that we don't have write access after revert.
   EXPECT_EQ(SBOX_TEST_DENIED, runner.RunTest(command_write));
 
+  // Verify that we have read access after revert.
+  EXPECT_EQ(SBOX_TEST_SUCCEEDED, runner.RunTest(command_read));
+
   // Verify that we really have write access to the file.
   runner.SetTestState(BEFORE_REVERT);
   EXPECT_EQ(SBOX_TEST_SUCCEEDED, runner.RunTest(command_write));
diff --git a/sandbox/win/src/filesystem_dispatcher.cc b/sandbox/win/src/filesystem_dispatcher.cc
index 354fe4f..dcd30a7 100644
--- a/sandbox/win/src/filesystem_dispatcher.cc
+++ b/sandbox/win/src/filesystem_dispatcher.cc
@@ -102,6 +102,7 @@ bool FilesystemDispatcher::NtCreateFile(IPCInfo* ipc,
   CountedParameterSet<OpenFile> params;
   params[OpenFile::NAME] = ParamPickerMake(filename);
   params[OpenFile::ACCESS] = ParamPickerMake(desired_access);
+  params[OpenFile::DISPOSITION] = ParamPickerMake(create_disposition);
   params[OpenFile::OPTIONS] = ParamPickerMake(create_options);
   params[OpenFile::BROKER] = ParamPickerMake(broker);
 
@@ -144,9 +145,11 @@ bool FilesystemDispatcher::NtOpenFile(IPCInfo* ipc,
   const wchar_t* filename = name->c_str();
 
   uint32 broker = TRUE;
+  uint32 create_disposition = 0;
   CountedParameterSet<OpenFile> params;
   params[OpenFile::NAME] = ParamPickerMake(filename);
   params[OpenFile::ACCESS] = ParamPickerMake(desired_access);
+  params[OpenFile::DISPOSITION] = ParamPickerMake(create_disposition);
   params[OpenFile::OPTIONS] = ParamPickerMake(open_options);
   params[OpenFile::BROKER] = ParamPickerMake(broker);
 
diff --git a/sandbox/win/src/filesystem_interception.cc b/sandbox/win/src/filesystem_interception.cc
index 043e1fa..459f7ac 100644
--- a/sandbox/win/src/filesystem_interception.cc
+++ b/sandbox/win/src/filesystem_interception.cc
@@ -54,10 +54,12 @@ NTSTATUS WINAPI TargetNtCreateFile(NtCreateFileFunction orig_CreateFile,
 
     uint32 desired_access_uint32 = desired_access;
     uint32 options_uint32 = options;
+    uint32 disposition_uint32 = disposition;
     uint32 broker = FALSE;
     CountedParameterSet<OpenFile> params;
     params[OpenFile::NAME] = ParamPickerMake(name);
     params[OpenFile::ACCESS] = ParamPickerMake(desired_access_uint32);
+    params[OpenFile::DISPOSITION] = ParamPickerMake(disposition_uint32);
     params[OpenFile::OPTIONS] = ParamPickerMake(options_uint32);
     params[OpenFile::BROKER] = ParamPickerMake(broker);
 
@@ -128,10 +130,12 @@ NTSTATUS WINAPI TargetNtOpenFile(NtOpenFileFunction orig_OpenFile, PHANDLE file,
 
     uint32 desired_access_uint32 = desired_access;
     uint32 options_uint32 = options;
+    uint32 disposition_uint32 = FILE_OPEN;
     uint32 broker = FALSE;
     CountedParameterSet<OpenFile> params;
     params[OpenFile::NAME] = ParamPickerMake(name);
     params[OpenFile::ACCESS] = ParamPickerMake(desired_access_uint32);
+    params[OpenFile::DISPOSITION] = ParamPickerMake(disposition_uint32);
     params[OpenFile::OPTIONS] = ParamPickerMake(options_uint32);
     params[OpenFile::BROKER] = ParamPickerMake(broker);
 
diff --git a/sandbox/win/src/filesystem_policy.cc b/sandbox/win/src/filesystem_policy.cc
index 65e8f47..1ce21c8 100644
--- a/sandbox/win/src/filesystem_policy.cc
+++ b/sandbox/win/src/filesystem_policy.cc
@@ -127,7 +127,9 @@ bool FileSystemPolicy::GenerateRules(const wchar_t* name,
                             GENERIC_READ | GENERIC_EXECUTE | READ_CONTROL;
       DWORD restricted_flags = ~allowed_flags;
       open.AddNumberMatch(IF_NOT, OpenFile::ACCESS, restricted_flags, AND);
+      open.AddNumberMatch(IF, OpenFile::DISPOSITION, FILE_OPEN, EQUAL);
       create.AddNumberMatch(IF_NOT, OpenFile::ACCESS, restricted_flags, AND);
+      create.AddNumberMatch(IF, OpenFile::DISPOSITION, FILE_OPEN, EQUAL);
 
       // Read only access don't work for rename.
       rule_to_add &= ~kCallNtSetInfoRename;
diff --git a/sandbox/win/src/policy_params.h b/sandbox/win/src/policy_params.h
index f50cf1e..e051d2b 100644
--- a/sandbox/win/src/policy_params.h
+++ b/sandbox/win/src/policy_params.h
@@ -23,6 +23,7 @@ POLPARAMS_BEGIN(OpenFile)
   POLPARAM(NAME)
   POLPARAM(BROKER)   // TRUE if called from the broker.
   POLPARAM(ACCESS)
+  POLPARAM(DISPOSITION)
   POLPARAM(OPTIONS)
 POLPARAMS_END(OpenFile)