summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--sandbox/src/file_policy_test.cc35
-rw-r--r--sandbox/src/filesystem_interception.cc13
-rw-r--r--sandbox/src/named_pipe_interception.cc4
-rw-r--r--sandbox/src/process_thread_interception.cc20
-rw-r--r--sandbox/src/registry_interception.cc14
-rw-r--r--sandbox/src/sync_interception.cc8
-rw-r--r--sandbox/tests/common/controller.h3
7 files changed, 72 insertions, 25 deletions
diff --git a/sandbox/src/file_policy_test.cc b/sandbox/src/file_policy_test.cc
index 145d03a..e4d1985 100644
--- a/sandbox/src/file_policy_test.cc
+++ b/sandbox/src/file_policy_test.cc
@@ -176,6 +176,8 @@ SBOX_TESTS_COMMAND int File_CreateSys32(int argc, wchar_t **argv) {
return SBOX_TEST_SUCCEEDED;
} else if (STATUS_ACCESS_DENIED == status) {
return SBOX_TEST_DENIED;
+ } else if (STATUS_OBJECT_NAME_NOT_FOUND == status) {
+ return SBOX_TEST_NOT_FOUND;
}
return SBOX_TEST_FAILED;
}
@@ -208,6 +210,8 @@ SBOX_TESTS_COMMAND int File_OpenSys32(int argc, wchar_t **argv) {
return SBOX_TEST_SUCCEEDED;
} else if (STATUS_ACCESS_DENIED == status) {
return SBOX_TEST_DENIED;
+ } else if (STATUS_OBJECT_NAME_NOT_FOUND == status) {
+ return SBOX_TEST_NOT_FOUND;
}
return SBOX_TEST_FAILED;
}
@@ -293,6 +297,8 @@ SBOX_TESTS_COMMAND int File_QueryAttributes(int argc, wchar_t **argv) {
return SBOX_TEST_SUCCEEDED;
} else if (STATUS_ACCESS_DENIED == status1) {
return SBOX_TEST_DENIED;
+ } else if (STATUS_OBJECT_NAME_NOT_FOUND == status1) {
+ return SBOX_TEST_NOT_FOUND;
}
return SBOX_TEST_FAILED;
@@ -384,10 +390,25 @@ TEST(FilePolicyTest, AllowNtCreatePatternRule) {
EXPECT_EQ(SBOX_TEST_SUCCEEDED, runner.RunTest(L"File_OpenSys32 appwiz.cpl"));
}
+TEST(FilePolicyTest, CheckNotFound) {
+ TestRunner runner;
+ EXPECT_TRUE(runner.AddRuleSys32(TargetPolicy::FILES_ALLOW_ANY, L"n*.dll"));
+
+ EXPECT_EQ(SBOX_TEST_NOT_FOUND,
+ runner.RunTest(L"File_OpenSys32 notfound.dll"));
+}
+
+TEST(FilePolicyTest, CheckNoLeak) {
+ TestRunner runner;
+ EXPECT_EQ(SBOX_TEST_DENIED, runner.RunTest(L"File_CreateSys32 notfound.exe"));
+}
+
TEST(FilePolicyTest, TestQueryAttributesFile) {
TestRunner runner;
EXPECT_TRUE(runner.AddRuleSys32(TargetPolicy::FILES_ALLOW_ANY,
L"appmgmts.dll"));
+ EXPECT_TRUE(runner.AddRuleSys32(TargetPolicy::FILES_ALLOW_ANY,
+ L"notfound.exe"));
EXPECT_TRUE(runner.AddRuleSys32(TargetPolicy::FILES_ALLOW_ANY, L"drivers"));
EXPECT_TRUE(runner.AddRuleSys32(TargetPolicy::FILES_ALLOW_QUERY,
L"ipconfig.exe"));
@@ -403,6 +424,20 @@ TEST(FilePolicyTest, TestQueryAttributesFile) {
EXPECT_EQ(SBOX_TEST_DENIED,
runner.RunTest(L"File_QueryAttributes ftp.exe f"));
+
+ EXPECT_EQ(SBOX_TEST_NOT_FOUND,
+ runner.RunTest(L"File_QueryAttributes notfound.exe f"));
+}
+
+// Makes sure that we don't leak information when there is not policy to allow
+// a path.
+TEST(FilePolicyTest, TestQueryAttributesFileNoPolicy) {
+ TestRunner runner;
+ EXPECT_EQ(SBOX_TEST_DENIED,
+ runner.RunTest(L"File_QueryAttributes ftp.exe f"));
+
+ EXPECT_EQ(SBOX_TEST_DENIED,
+ runner.RunTest(L"File_QueryAttributes notfound.exe f"));
}
TEST(FilePolicyTest, TestRename) {
diff --git a/sandbox/src/filesystem_interception.cc b/sandbox/src/filesystem_interception.cc
index 5f1a253..8183f12 100644
--- a/sandbox/src/filesystem_interception.cc
+++ b/sandbox/src/filesystem_interception.cc
@@ -76,7 +76,7 @@ NTSTATUS WINAPI TargetNtCreateFile(NtCreateFileFunction orig_CreateFile,
break;
if (!NT_SUCCESS(answer.nt_status))
- break;
+ return answer.nt_status;
__try {
*file = answer.handle;
@@ -144,7 +144,7 @@ NTSTATUS WINAPI TargetNtOpenFile(NtOpenFileFunction orig_OpenFile, PHANDLE file,
break;
if (!NT_SUCCESS(answer.nt_status))
- break;
+ return answer.nt_status;
__try {
*file = answer.handle;
@@ -208,9 +208,6 @@ NTSTATUS WINAPI TargetNtQueryAttributesFile(
if (SBOX_ALL_OK != code)
break;
- if (!NT_SUCCESS(answer.nt_status))
- break;
-
return answer.nt_status;
} while (false);
@@ -269,9 +266,6 @@ NTSTATUS WINAPI TargetNtQueryFullAttributesFile(
if (SBOX_ALL_OK != code)
break;
- if (!NT_SUCCESS(answer.nt_status))
- break;
-
return answer.nt_status;
} while (false);
@@ -348,9 +342,6 @@ NTSTATUS WINAPI TargetNtSetInformationFile(
if (SBOX_ALL_OK != code)
break;
- if (!NT_SUCCESS(answer.nt_status))
- break;
-
status = answer.nt_status;
} while (false);
diff --git a/sandbox/src/named_pipe_interception.cc b/sandbox/src/named_pipe_interception.cc
index 0c3c653..fedb58e 100644
--- a/sandbox/src/named_pipe_interception.cc
+++ b/sandbox/src/named_pipe_interception.cc
@@ -57,8 +57,10 @@ HANDLE WINAPI TargetCreateNamedPipeW(
if (SBOX_ALL_OK != code)
break;
+ ::SetLastError(answer.win32_result);
+
if (ERROR_SUCCESS != answer.win32_result)
- break;
+ return INVALID_HANDLE_VALUE;
return answer.handle;
} while (false);
diff --git a/sandbox/src/process_thread_interception.cc b/sandbox/src/process_thread_interception.cc
index 79809e1..50b36b7 100644
--- a/sandbox/src/process_thread_interception.cc
+++ b/sandbox/src/process_thread_interception.cc
@@ -74,6 +74,14 @@ NTSTATUS WINAPI TargetNtOpenThread(NtOpenThreadFunction orig_OpenThread,
break;
if (!NT_SUCCESS(answer.nt_status))
+ // The nt_status here is most likely STATUS_INVALID_CID because
+ // in the broker we set the process id in the CID (client ID) param
+ // to be the current process. If you try to open a thread from another
+ // process you will get this INVALID_CID error. On the other hand, if you
+ // try to open a thread in your own process, it should return success.
+ // We don't want to return STATUS_INVALID_CID here, so we return the
+ // return of the original open thread status, which is most likely
+ // STATUS_ACCESS_DENIED.
break;
__try {
@@ -144,7 +152,7 @@ NTSTATUS WINAPI TargetNtOpenProcess(NtOpenProcessFunction orig_OpenProcess,
break;
if (!NT_SUCCESS(answer.nt_status))
- break;
+ return answer.nt_status;
__try {
// Write the output parameters.
@@ -189,7 +197,7 @@ NTSTATUS WINAPI TargetNtOpenProcessToken(
break;
if (!NT_SUCCESS(answer.nt_status))
- break;
+ return answer.nt_status;
__try {
// Write the output parameters.
@@ -234,7 +242,7 @@ NTSTATUS WINAPI TargetNtOpenProcessTokenEx(
break;
if (!NT_SUCCESS(answer.nt_status))
- break;
+ return answer.nt_status;
__try {
// Write the output parameters.
@@ -296,8 +304,9 @@ BOOL WINAPI TargetCreateProcessW(CreateProcessWFunction orig_CreateProcessW,
if (SBOX_ALL_OK != code)
break;
+ ::SetLastError(answer.win32_result);
if (ERROR_SUCCESS != answer.win32_result)
- break;
+ return FALSE;
return TRUE;
} while (false);
@@ -376,8 +385,9 @@ BOOL WINAPI TargetCreateProcessA(CreateProcessAFunction orig_CreateProcessA,
if (SBOX_ALL_OK != code)
break;
+ ::SetLastError(answer.win32_result);
if (ERROR_SUCCESS != answer.win32_result)
- break;
+ return FALSE;
return TRUE;
} while (false);
diff --git a/sandbox/src/registry_interception.cc b/sandbox/src/registry_interception.cc
index 21cea08..adbbf6e 100644
--- a/sandbox/src/registry_interception.cc
+++ b/sandbox/src/registry_interception.cc
@@ -65,6 +65,12 @@ NTSTATUS WINAPI TargetNtCreateKey(NtCreateKeyFunction orig_CreateKey,
break;
if (!NT_SUCCESS(answer.nt_status))
+ // TODO(nsylvain): We should return answer.nt_status here instead
+ // of status. We can do this only after we checked the policy.
+ // otherwise we will returns ACCESS_DENIED for all paths
+ // that are not specified by a policy, even though your token allows
+ // access to that path, and the original call had a more meaningful
+ // error. Bug 4369
break;
__try {
@@ -121,7 +127,13 @@ NTSTATUS WINAPI TargetNtOpenKey(NtOpenKeyFunction orig_OpenKey, PHANDLE key,
break;
if (!NT_SUCCESS(answer.nt_status))
- break;
+ // TODO(nsylvain): We should return answer.nt_status here instead
+ // of status. We can do this only after we checked the policy.
+ // otherwise we will returns ACCESS_DENIED for all paths
+ // that are not specified by a policy, even though your token allows
+ // access to that path, and the original call had a more meaningful
+ // error. Bug 4369
+ break;
__try {
*key = answer.handle;
diff --git a/sandbox/src/sync_interception.cc b/sandbox/src/sync_interception.cc
index 715b2a8..aa7112a 100644
--- a/sandbox/src/sync_interception.cc
+++ b/sandbox/src/sync_interception.cc
@@ -52,9 +52,7 @@ HANDLE WINAPI TargetCreateEventW(CreateEventWFunction orig_CreateEvent,
if (SBOX_ALL_OK != code)
break;
- if (NULL == answer.handle)
- break;
-
+ ::SetLastError(answer.win32_result);
return answer.handle;
} while (false);
@@ -98,9 +96,7 @@ HANDLE WINAPI TargetOpenEventW(OpenEventWFunction orig_OpenEvent,
if (SBOX_ALL_OK != code)
break;
- if (NULL == answer.handle)
- break;
-
+ ::SetLastError(answer.win32_result);
return answer.handle;
} while (false);
diff --git a/sandbox/tests/common/controller.h b/sandbox/tests/common/controller.h
index 8d9038f..1584f57 100644
--- a/sandbox/tests/common/controller.h
+++ b/sandbox/tests/common/controller.h
@@ -25,7 +25,8 @@ enum SboxTestResult {
SBOX_TEST_SUCCEEDED,
SBOX_TEST_PING_OK,
SBOX_TEST_FIRST_INFO = SBOX_TEST_FIRST_RESULT | SEVERITY_INFO_FLAGS,
- SBOX_TEST_DENIED,
+ SBOX_TEST_DENIED, // Access was denied.
+ SBOX_TEST_NOT_FOUND, // The resource was not found.
SBOX_TEST_FIRST_ERROR = SBOX_TEST_FIRST_RESULT | SEVERITY_ERROR_FLAGS,
SBOX_TEST_INVALID_PARAMETER,
SBOX_TEST_FAILED_TO_RUN_TEST,
generated by cgit v1.1 at 2025-01-16 20:15:50 +0000