From 90b41f569db5cff7a8b8d170b81516dd006f57ba Mon Sep 17 00:00:00 2001 From: jschuh Date: Thu, 22 Jan 2015 17:46:27 -0800 Subject: 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} --- sandbox/win/src/file_policy_test.cc | 45 +++++++++++++++++++++--------- sandbox/win/src/filesystem_dispatcher.cc | 3 ++ sandbox/win/src/filesystem_interception.cc | 4 +++ sandbox/win/src/filesystem_policy.cc | 2 ++ sandbox/win/src/policy_params.h | 1 + 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 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 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 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 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) -- cgit v1.1