Return the right error code when we proxy a call

to the broker.

IIRC we decided to always return access denied because
we did not want to leak the real error code, but this
is bogus for 2 reasons:
1. The broker will return access denied if it's not
allowed in the policy
2. The check to hide the return code is in the renderer,
so it would have been possible for a malicious user to see
it anyway.

I also added a test for it.

BUG:3965

Review URL: http://codereview.chromium.org/10615

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@5388 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 70 insertions, 24 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);