Correctly test for canonicalized path in the CreateNamedPipe policy engine.

BUG=334897
TEST=sbox_integration_tests.exe

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

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

2 files changed, 96 insertions, 7 deletions

diff --git a/sandbox/win/src/named_pipe_dispatcher.cc b/sandbox/win/src/named_pipe_dispatcher.cc
index 54b3162..da4045c 100644
--- a/sandbox/win/src/named_pipe_dispatcher.cc
+++ b/sandbox/win/src/named_pipe_dispatcher.cc
@@ -5,6 +5,7 @@
 #include "sandbox/win/src/named_pipe_dispatcher.h"
 
 #include "base/basictypes.h"
+#include "base/strings/string_split.h"
 
 #include "sandbox/win/src/crosscall_client.h"
 #include "sandbox/win/src/interception.h"
@@ -43,6 +44,23 @@ bool NamedPipeDispatcher::CreateNamedPipe(
     IPCInfo* ipc, base::string16* name, DWORD open_mode, DWORD pipe_mode,
     DWORD max_instances, DWORD out_buffer_size, DWORD in_buffer_size,
     DWORD default_timeout) {
+  ipc->return_info.win32_result = ERROR_ACCESS_DENIED;
+  ipc->return_info.handle = INVALID_HANDLE_VALUE;
+
+  std::vector<base::string16> paths;
+  std::vector<base::string16> innerpaths;
+  base::SplitString(*name, '/', &paths);
+
+  for (std::vector<base::string16>::const_iterator iter = paths.begin();
+       iter != paths.end(); ++iter) {
+    base::SplitString(*iter, '\\', &innerpaths);
+    for (std::vector<base::string16>::const_iterator iter2 = innerpaths.begin();
+          iter2 != innerpaths.end(); ++iter2) {
+      if (*iter2 == L"..")
+          return true;
+    }
+  }
+
   const wchar_t* pipe_name = name->c_str();
   CountedParameterSet<NameBased> params;
   params[NameBased::NAME] = ParamPickerMake(pipe_name);
@@ -50,6 +68,16 @@ bool NamedPipeDispatcher::CreateNamedPipe(
   EvalResult eval = policy_base_->EvalPolicy(IPC_CREATENAMEDPIPEW_TAG,
                                              params.GetBase());
 
+  // "For file I/O, the "\\?\" prefix to a path string tells the Windows APIs to
+  // disable all string parsing and to send the string that follows it straight
+  // to the file system."
+  // http://msdn.microsoft.com/en-us/library/aa365247(VS.85).aspx
+  // This ensures even if there is a path traversal in the pipe name, and it is
+  // able to get past the checks above, it will still not be allowed to escape
+  // our whitelisted namespace.
+  if (name->compare(0, 4, L"\\\\.\\") == 0)
+    name->replace(0, 4, L"\\\\\?\\");
+
   HANDLE pipe;
   DWORD ret = NamedPipePolicy::CreateNamedPipeAction(eval, *ipc->client_info,
                                                      *name, open_mode,
diff --git a/sandbox/win/src/named_pipe_policy_test.cc b/sandbox/win/src/named_pipe_policy_test.cc
index b89a191..fe8c71f 100644
--- a/sandbox/win/src/named_pipe_policy_test.cc
+++ b/sandbox/win/src/named_pipe_policy_test.cc
@@ -1,18 +1,20 @@
-// Copyright (c) 2006-2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "testing/gtest/include/gtest/gtest.h"
+#include "base/win/windows_version.h"
+#include "sandbox/win/src/handle_closer.h"
 #include "sandbox/win/src/sandbox.h"
 #include "sandbox/win/src/sandbox_policy.h"
 #include "sandbox/win/src/sandbox_factory.h"
 #include "sandbox/win/tests/common/controller.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
 namespace sandbox {
 
 
 SBOX_TESTS_COMMAND int NamedPipe_Create(int argc, wchar_t **argv) {
-  if (argc != 1) {
+  if (argc < 1 || argc > 2) {
     return SBOX_TEST_FAILED_TO_EXECUTE_COMMAND;
   }
   if ((NULL == argv) || (NULL == argv[0])) {
@@ -26,6 +28,18 @@ SBOX_TESTS_COMMAND int NamedPipe_Create(int argc, wchar_t **argv) {
   if (INVALID_HANDLE_VALUE == pipe)
     return SBOX_TEST_DENIED;
 
+  // The second parameter allows us to enforce a whitelist for where the
+  // pipe should be in the object namespace after creation.
+  if (argc == 2) {
+    base::string16 handle_name;
+    if (GetHandleName(pipe, &handle_name)) {
+      if (handle_name.compare(0, wcslen(argv[1]), argv[1]) != 0)
+        return SBOX_TEST_FAILED;
+    } else {
+      return SBOX_TEST_FAILED;
+    }
+  }
+
   OVERLAPPED overlapped = {0};
   overlapped.hEvent = ::CreateEvent(NULL, TRUE, TRUE, NULL);
   BOOL result = ::ConnectNamedPipe(pipe, &overlapped);
@@ -45,19 +59,59 @@ SBOX_TESTS_COMMAND int NamedPipe_Create(int argc, wchar_t **argv) {
   return SBOX_TEST_SUCCEEDED;
 }
 
-// Tests if we can create a pipe in the sandbox. On XP, the sandbox can create
-// a pipe without any help but it fails on Vista, this is why we do not test
-// the "denied" case.
+// Tests if we can create a pipe in the sandbox.
 TEST(NamedPipePolicyTest, CreatePipe) {
   TestRunner runner;
   // TODO(nsylvain): This policy is wrong because "*" is a valid char in a
   // namedpipe name. Here we apply it like a wildcard. http://b/893603
   EXPECT_TRUE(runner.AddRule(TargetPolicy::SUBSYS_NAMED_PIPES,
                              TargetPolicy::NAMEDPIPES_ALLOW_ANY,
-                              L"\\\\.\\pipe\\test*"));
+                             L"\\\\.\\pipe\\test*"));
 
   EXPECT_EQ(SBOX_TEST_SUCCEEDED,
             runner.RunTest(L"NamedPipe_Create \\\\.\\pipe\\testbleh"));
+
+  // On XP, the sandbox can create a pipe without any help but it fails on
+  // Vista+, this is why we do not test the "denied" case.
+  if (base::win::OSInfo::GetInstance()->version() >= base::win::VERSION_VISTA) {
+    EXPECT_EQ(SBOX_TEST_DENIED,
+              runner.RunTest(L"NamedPipe_Create \\\\.\\pipe\\bleh"));
+  }
+}
+
+// Tests if we can create a pipe with a path traversal in the sandbox.
+TEST(NamedPipePolicyTest, CreatePipeTraversal) {
+  TestRunner runner;
+  // TODO(nsylvain): This policy is wrong because "*" is a valid char in a
+  // namedpipe name. Here we apply it like a wildcard. http://b/893603
+  EXPECT_TRUE(runner.AddRule(TargetPolicy::SUBSYS_NAMED_PIPES,
+                             TargetPolicy::NAMEDPIPES_ALLOW_ANY,
+                              L"\\\\.\\pipe\\test*"));
+
+  // On XP, the sandbox can create a pipe without any help but it fails on
+  // Vista+, this is why we do not test the "denied" case.
+  if (base::win::OSInfo::GetInstance()->version() >= base::win::VERSION_VISTA) {
+    EXPECT_EQ(SBOX_TEST_DENIED,
+              runner.RunTest(L"NamedPipe_Create \\\\.\\pipe\\test\\..\\bleh"));
+    EXPECT_EQ(SBOX_TEST_DENIED,
+              runner.RunTest(L"NamedPipe_Create \\\\.\\pipe\\test/../bleh"));
+    EXPECT_EQ(SBOX_TEST_DENIED,
+              runner.RunTest(L"NamedPipe_Create \\\\.\\pipe\\test\\../bleh"));
+    EXPECT_EQ(SBOX_TEST_DENIED,
+              runner.RunTest(L"NamedPipe_Create \\\\.\\pipe\\test/..\\bleh"));
+  }
+}
+
+// This tests that path canonicalization is actually disabled if we use \\?\
+// syntax.
+TEST(NamedPipePolicyTest, CreatePipeCanonicalization) {
+  // "For file I/O, the "\\?\" prefix to a path string tells the Windows APIs to
+  // disable all string parsing and to send the string that follows it straight
+  // to the file system."
+  // http://msdn.microsoft.com/en-us/library/aa365247(VS.85).aspx
+  wchar_t* argv[2] = { L"\\\\?\\pipe\\test\\..\\bleh",
+                       L"\\Device\\NamedPipe\\test" };
+  EXPECT_EQ(SBOX_TEST_SUCCEEDED, NamedPipe_Create(2, argv));
 }
 
 // The same test as CreatePipe but this time using strict interceptions.
@@ -73,6 +127,13 @@ TEST(NamedPipePolicyTest, CreatePipeStrictInterceptions) {
 
   EXPECT_EQ(SBOX_TEST_SUCCEEDED,
             runner.RunTest(L"NamedPipe_Create \\\\.\\pipe\\testbleh"));
+
+  // On XP, the sandbox can create a pipe without any help but it fails on
+  // Vista+, this is why we do not test the "denied" case.
+  if (base::win::OSInfo::GetInstance()->version() >= base::win::VERSION_VISTA) {
+    EXPECT_EQ(SBOX_TEST_DENIED,
+              runner.RunTest(L"NamedPipe_Create \\\\.\\pipe\\bleh"));
+  }
 }
 
 }  // namespace sandbox