Adds sync brokering to Windows EDK

We use a sync channel on other platforms to facilitate synchronous
shared buffer creation from within a sandbox. We need to do this
on Windows too.

This implements that, and adds a test for sandboxed shared buffer
creation.

BUG=594883

Review-Url: https://codereview.chromium.org/2264543003
Cr-Commit-Position: refs/heads/master@{#418679}

content/browser/mojo_sandbox_browsertest.cc

+// Copyright 2016 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 <stdint.h>
+#include <memory>
+#include <utility>
+
+#include "base/bind.h"
+#include "base/macros.h"
+#include "base/run_loop.h"
+#include "content/browser/utility_process_host_impl.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/utility_process_host_client.h"
+#include "content/public/test/content_browser_test.h"
+#include "content/public/test/test_mojo_service.mojom.h"
+#include "services/shell/public/cpp/interface_provider.h"
+
+namespace content {
+namespace {
+
+const std::string kTestMessage = "My hovercraft is full of eels!";
+
+class MojoSandboxTest : public ContentBrowserTest {
+ public:
+  MojoSandboxTest() {}
+
+  void SetUpOnMainThread() override {
+    base::RunLoop run_loop;
+    BrowserThread::PostTaskAndReply(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&MojoSandboxTest::StartUtilityProcessOnIoThread,
+                   base::Unretained(this)),
+        run_loop.QuitClosure());
+    run_loop.Run();
+  }
+
+  void TearDownOnMainThread() override {
+    base::RunLoop run_loop;
+    BrowserThread::PostTaskAndReply(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&MojoSandboxTest::StopUtilityProcessOnIoThread,
+                   base::Unretained(this)),
+        run_loop.QuitClosure());
+    run_loop.Run();
+  }
+
+ protected:
+  std::unique_ptr<UtilityProcessHostImpl> host_;
+
+ private:
+  void StartUtilityProcessOnIoThread() {
+    host_.reset(new UtilityProcessHostImpl(nullptr, nullptr));
+    ASSERT_TRUE(host_->Start());
+  }
+
+  void StopUtilityProcessOnIoThread() { host_.reset(); }
+
+  DISALLOW_COPY_AND_ASSIGN(MojoSandboxTest);
+};
+
+IN_PROC_BROWSER_TEST_F(MojoSandboxTest, SubprocessSharedBuffer) {
+  // Ensures that a shared buffer can be created within a sandboxed process.
+
+  mojom::TestMojoServicePtr test_service;
+  host_->GetRemoteInterfaces()->GetInterface(&test_service);
+
+  bool got_response = false;
+  base::RunLoop run_loop;
+  test_service.set_connection_error_handler(run_loop.QuitClosure());
+  test_service->CreateSharedBuffer(
+      kTestMessage,
+      base::Bind(
+          [](const base::Closure& quit_closure, bool* got_response,
+             mojo::ScopedSharedBufferHandle buffer) {
+            ASSERT_TRUE(buffer.is_valid());
+            mojo::ScopedSharedBufferMapping mapping =
+                buffer->Map(kTestMessage.size());
+            ASSERT_TRUE(mapping);
+            std::string contents(static_cast<const char*>(mapping.get()),
+                                 kTestMessage.size());
+            EXPECT_EQ(kTestMessage, contents);
+            *got_response = true;
+            quit_closure.Run();
+          },
+          run_loop.QuitClosure(), &got_response));
+  run_loop.Run();
+  EXPECT_TRUE(got_response);
+}
+
+}  //  namespace
+}  //  namespace content

content/browser/service_worker/service_worker_version_unittest.cc

@@ -183,6 +183,11 @@ class TestMojoServiceImpl : public mojom::TestMojoService {
     callback.Run(mojo::String(""));
   }
 
+  void CreateSharedBuffer(const std::string& message,
+                          const CreateSharedBufferCallback& callback) override {
+    NOTREACHED();
+  }
+
  private:
   explicit TestMojoServiceImpl() {}
 };

content/public/test/OWNERS

@@ -3,3 +3,7 @@ sky@chromium.org
 
 # For WebRTC files.
 tommi@chromium.org
+
+# For security presubmit checks, though test mojom really don't need review.
+per-file *.mojom=set noparent
+per-file *.mojom=file://ipc/SECURITY_OWNERS

content/public/test/test_mojo_app.cc

@@ -53,4 +53,10 @@ void TestMojoApp::GetRequestorName(const GetRequestorNameCallback& callback) {
   callback.Run(requestor_name_);
 }
 
+void TestMojoApp::CreateSharedBuffer(
+    const std::string& message,
+    const CreateSharedBufferCallback& callback) {
+  NOTREACHED();
+}
+
 }  // namespace content

content/public/test/test_mojo_app.h

@@ -40,6 +40,8 @@ class TestMojoApp : public shell::Service,
   void DoTerminateProcess(const DoTerminateProcessCallback& callback) override;
   void CreateFolder(const CreateFolderCallback& callback) override;
   void GetRequestorName(const GetRequestorNameCallback& callback) override;
+  void CreateSharedBuffer(const std::string& message,
+                          const CreateSharedBufferCallback& callback) override;
 
   mojo::Binding<mojom::TestMojoService> service_binding_;
 

content/public/test/test_mojo_service.mojom

@@ -17,4 +17,8 @@ interface TestMojoService {
 
   // Retrieve the requestor name as seen by the test app providing this service.
   GetRequestorName() => (string name);
+
+  // Requests that a new shared buffer be created and returned. If successful,
+  // |buffer| is non-null and its contents match |message|'s bytes exactly.
+  CreateSharedBuffer(string message) => (handle<shared_buffer>? buffer);
 };

content/shell/renderer/shell_content_renderer_client.cc

@@ -71,6 +71,11 @@ class TestMojoServiceImpl : public mojom::TestMojoService {
     callback.Run("Not implemented.");
   }
 
+  void CreateSharedBuffer(const std::string& message,
+                          const CreateSharedBufferCallback& callback) override {
+    NOTREACHED();
+  }
+
   mojo::Binding<mojom::TestMojoService> binding_;
 
   DISALLOW_COPY_AND_ASSIGN(TestMojoServiceImpl);

content/shell/utility/shell_content_utility_client.cc

@@ -13,6 +13,7 @@
 #include "content/public/test/test_mojo_app.h"
 #include "content/public/test/test_mojo_service.mojom.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
+#include "mojo/public/cpp/system/buffer.h"
 #include "services/shell/public/cpp/interface_registry.h"
 
 namespace content {
@@ -45,6 +46,20 @@ class TestMojoServiceImpl : public mojom::TestMojoService {
     NOTREACHED();
   }
 
+  void CreateSharedBuffer(const std::string& message,
+                          const CreateSharedBufferCallback& callback) override {
+    mojo::ScopedSharedBufferHandle buffer =
+        mojo::SharedBufferHandle::Create(message.size());
+    CHECK(buffer.is_valid());
+
+    mojo::ScopedSharedBufferMapping mapping = buffer->Map(message.size());
+    CHECK(mapping);
+    std::copy(message.begin(), message.end(),
+              reinterpret_cast<char*>(mapping.get()));
+
+    callback.Run(std::move(buffer));
+  }
+
  private:
   explicit TestMojoServiceImpl() {}
 

content/test/BUILD.gn

@@ -598,6 +598,7 @@ test("content_browsertests") {
     "../browser/memory/memory_coordinator_browsertest.cc",
     "../browser/memory/memory_pressure_controller_impl_browsertest.cc",
     "../browser/message_port_provider_browsertest.cc",
+    "../browser/mojo_sandbox_browsertest.cc",
     "../browser/net_info_browsertest.cc",
     "../browser/renderer_host/input/composited_scrolling_browsertest.cc",
     "../browser/renderer_host/input/main_thread_event_queue_browsertest.cc",

mojo/edk/embedder/platform_shared_buffer.cc

@@ -253,21 +253,28 @@ bool PlatformSharedBuffer::InitFromPlatformHandle(
 bool PlatformSharedBuffer::InitFromPlatformHandlePair(
     ScopedPlatformHandle rw_platform_handle,
     ScopedPlatformHandle ro_platform_handle) {
-#if defined(OS_WIN) || defined(OS_MACOSX)
+#if defined(OS_MACOSX)
   NOTREACHED();
   return false;
-#else
-  DCHECK(!shared_memory_);
+#else  // defined(OS_MACOSX)
 
+#if defined(OS_WIN)
+  base::SharedMemoryHandle handle(rw_platform_handle.release().handle,
+                                  base::GetCurrentProcId());
+  base::SharedMemoryHandle ro_handle(ro_platform_handle.release().handle,
+                                     base::GetCurrentProcId());
+#else  // defined(OS_WIN)
   base::SharedMemoryHandle handle(rw_platform_handle.release().handle, false);
-  shared_memory_.reset(new base::SharedMemory(handle, false));
-
   base::SharedMemoryHandle ro_handle(ro_platform_handle.release().handle,
                                      false);
-  ro_shared_memory_.reset(new base::SharedMemory(ro_handle, true));
+#endif  // defined(OS_WIN)
 
+  DCHECK(!shared_memory_);
+  shared_memory_.reset(new base::SharedMemory(handle, false));
+  ro_shared_memory_.reset(new base::SharedMemory(ro_handle, true));
   return true;
-#endif
+
+#endif  // defined(OS_MACOSX)
 }
 
 void PlatformSharedBuffer::InitFromSharedMemoryHandle(

mojo/edk/system/BUILD.gn

@@ -20,9 +20,10 @@ component("system") {
     "awakable_list.cc",
     "awakable_list.h",
     "broker.h",
+    "broker_host.cc",
     "broker_host.h",
-    "broker_host_posix.cc",
     "broker_posix.cc",
+    "broker_win.cc",
     "channel.cc",
     "channel.h",
     "channel_posix.cc",
@@ -106,7 +107,7 @@ component("system") {
 
   if (is_nacl && !is_nacl_nonsfi) {
     sources -= [
-      "broker_host_posix.cc",
+      "broker_host.cc",
       "broker_posix.cc",
       "channel_posix.cc",
       "remote_message_pipe_bootstrap.cc",

mojo/edk/system/broker_host_posix.cc → mojo/edk/system/broker_host.cc

@@ -4,17 +4,12 @@
 
 #include "mojo/edk/system/broker_host.h"
 
-#include <fcntl.h>
-#include <unistd.h>
-
 #include <utility>
 
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/message_loop/message_loop.h"
 #include "base/threading/thread_task_runner_handle.h"
-#include "mojo/edk/embedder/embedder_internal.h"
-#include "mojo/edk/embedder/platform_channel_utils_posix.h"
 #include "mojo/edk/embedder/platform_handle_vector.h"
 #include "mojo/edk/embedder/platform_shared_buffer.h"
 #include "mojo/edk/system/broker_messages.h"
@@ -23,18 +18,25 @@ namespace mojo {
 namespace edk {
 
 namespace {
+
 // To prevent abuse, limit the maximum size of shared memory buffers.
-// TODO(amistry): Re-consider this limit, or do something smarter.
-const size_t kMaxSharedBufferSize = 16 * 1024 * 1024;
-}
+// TODO(rockot): Re-consider this limit, or do something smarter.
+const uint32_t kMaxSharedBufferSize = 16 * 1024 * 1024;
+
+}  // namespace
 
-BrokerHost::BrokerHost(ScopedPlatformHandle platform_handle) {
+BrokerHost::BrokerHost(base::ProcessHandle client_process,
+                       ScopedPlatformHandle platform_handle)
+#if defined(OS_WIN)
+    : client_process_(client_process)
+#endif
+{
   CHECK(platform_handle.is_valid());
 
   base::MessageLoop::current()->AddDestructionObserver(this);
 
-  channel_ = Channel::Create(this, std::move(platform_handle),
-                             base::ThreadTaskRunnerHandle::Get());
+  channel_ = Channel::Create(
+      this, std::move(platform_handle), base::ThreadTaskRunnerHandle::Get());
   channel_->Start();
 }
 
@@ -46,6 +48,18 @@ BrokerHost::~BrokerHost() {
     channel_->ShutDown();
 }
 
+void BrokerHost::PrepareHandlesForClient(PlatformHandleVector* handles) {
+#if defined(OS_WIN)
+  if (!Channel::Message::RewriteHandles(
+      base::GetCurrentProcessHandle(), client_process_, handles)) {
+    // NOTE: We only log an error here. We do not signal a logical error or
+    // prevent any message from being sent. The client should handle unexpected
+    // invalid handles appropriately.
+    DLOG(ERROR) << "Failed to rewrite one or more handles to broker client.";
+  }
+#endif
+}
+
 void BrokerHost::SendChannel(ScopedPlatformHandle handle) {
   CHECK(handle.is_valid());
   CHECK(channel_);
@@ -55,12 +69,12 @@ void BrokerHost::SendChannel(ScopedPlatformHandle handle) {
   ScopedPlatformHandleVectorPtr handles;
   handles.reset(new PlatformHandleVector(1));
   handles->at(0) = handle.release();
+  PrepareHandlesForClient(handles.get());
   message->SetHandles(std::move(handles));
-
   channel_->Write(std::move(message));
 }
 
-void BrokerHost::OnBufferRequest(size_t num_bytes) {
+void BrokerHost::OnBufferRequest(uint32_t num_bytes) {
   scoped_refptr<PlatformSharedBuffer> buffer;
   scoped_refptr<PlatformSharedBuffer> read_only_buffer;
   if (num_bytes <= kMaxSharedBufferSize) {
@@ -80,6 +94,7 @@ void BrokerHost::OnBufferRequest(size_t num_bytes) {
     handles.reset(new PlatformHandleVector(2));
     handles->at(0) = buffer->PassPlatformHandle().release();
     handles->at(1) = read_only_buffer->PassPlatformHandle().release();
+    PrepareHandlesForClient(handles.get());
     message->SetHandles(std::move(handles));
   }
 
@@ -101,29 +116,18 @@ void BrokerHost::OnChannelMessage(const void* payload,
         const BufferRequestData* request =
             reinterpret_cast<const BufferRequestData*>(header + 1);
         OnBufferRequest(request->size);
-        return;
       }
       break;
 
     default:
+      LOG(ERROR) << "Unexpected broker message type: " << header->type;
       break;
   }
-
-  LOG(ERROR) << "Unexpected broker message type: " << header->type;
 }
 
-void BrokerHost::OnChannelError() {
-  if (channel_) {
-    channel_->ShutDown();
-    channel_ = nullptr;
-  }
+void BrokerHost::OnChannelError() { delete this; }
 
-  delete this;
-}
-
-void BrokerHost::WillDestroyCurrentMessageLoop() {
-  delete this;
-}
+void BrokerHost::WillDestroyCurrentMessageLoop() { delete this; }
 
 }  // namespace edk
 }  // namespace mojo

mojo/edk/system/broker_host.h

@@ -5,8 +5,12 @@
 #ifndef MOJO_EDK_SYSTEM_BROKER_HOST_H_
 #define MOJO_EDK_SYSTEM_BROKER_HOST_H_
 
+#include <stdint.h>
+
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
+#include "base/process/process_handle.h"
+#include "mojo/edk/embedder/platform_handle_vector.h"
 #include "mojo/edk/embedder/scoped_platform_handle.h"
 #include "mojo/edk/system/channel.h"
 
@@ -18,7 +22,7 @@ namespace edk {
 class BrokerHost : public Channel::Delegate,
                    public base::MessageLoop::DestructionObserver {
  public:
-  explicit BrokerHost(ScopedPlatformHandle platform_handle);
+  BrokerHost(base::ProcessHandle client_process, ScopedPlatformHandle handle);
 
   // Send |handle| to the child, to be used to establish a NodeChannel to us.
   void SendChannel(ScopedPlatformHandle handle);
@@ -26,6 +30,8 @@ class BrokerHost : public Channel::Delegate,
  private:
   ~BrokerHost() override;
 
+  void PrepareHandlesForClient(PlatformHandleVector* handles);
+
   // Channel::Delegate:
   void OnChannelMessage(const void* payload,
                         size_t payload_size,
@@ -35,7 +41,11 @@ class BrokerHost : public Channel::Delegate,
   // base::MessageLoop::DestructionObserver:
   void WillDestroyCurrentMessageLoop() override;
 
-  void OnBufferRequest(size_t num_bytes);
+  void OnBufferRequest(uint32_t num_bytes);
+
+#if defined(OS_WIN)
+  base::ProcessHandle client_process_;
+#endif
 
   scoped_refptr<Channel> channel_;
 

mojo/edk/system/broker_win.cc

+// Copyright 2016 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 <windows.h>
+
+#include <limits>
+#include <utility>
+
+#include "base/numerics/safe_conversions.h"
+#include "mojo/edk/embedder/platform_handle.h"
+#include "mojo/edk/embedder/platform_handle_vector.h"
+#include "mojo/edk/embedder/platform_shared_buffer.h"
+#include "mojo/edk/system/broker.h"
+#include "mojo/edk/system/broker_messages.h"
+#include "mojo/edk/system/channel.h"
+
+namespace mojo {
+namespace edk {
+
+namespace {
+
+// 256 bytes should be enough for anyone!
+const size_t kMaxBrokerMessageSize = 256;
+
+bool WaitForBrokerMessage(PlatformHandle platform_handle,
+                          BrokerMessageType expected_type,
+                          size_t expected_num_handles,
+                          ScopedPlatformHandle* out_handles) {
+  char buffer[kMaxBrokerMessageSize];
+  DWORD bytes_read = 0;
+  BOOL result = ::ReadFile(platform_handle.handle, buffer,
+                           kMaxBrokerMessageSize, &bytes_read, nullptr);
+  if (!result) {
+    PLOG(ERROR) << "Error reading broker pipe";
+    return false;
+  }
+
+  Channel::MessagePtr message =
+      Channel::Message::Deserialize(buffer, static_cast<size_t>(bytes_read));
+  if (!message || message->payload_size() < sizeof(BrokerMessageHeader)) {
+    LOG(ERROR) << "Invalid broker message";
+    return false;
+  }
+
+  if (message->num_handles() != expected_num_handles) {
+    LOG(ERROR) << "Received unexpected number of handles in broker message";
+    return false;
+  }
+
+  const BrokerMessageHeader* header =
+      reinterpret_cast<const BrokerMessageHeader*>(message->payload());
+  if (header->type != expected_type) {
+    LOG(ERROR) << "Unknown broker message type";
+    return false;
+  }
+
+  ScopedPlatformHandleVectorPtr handles = message->TakeHandles();
+  DCHECK(handles);
+  DCHECK_EQ(handles->size(), expected_num_handles);
+  DCHECK(out_handles);
+
+  for (size_t i = 0; i < expected_num_handles; ++i)
+    out_handles[i] = ScopedPlatformHandle(handles->at(i));
+  handles->clear();
+  return true;
+}
+
+}  // namespace
+
+Broker::Broker(ScopedPlatformHandle handle) : sync_channel_(std::move(handle)) {
+  CHECK(sync_channel_.is_valid());
+  bool result = WaitForBrokerMessage(
+      sync_channel_.get(), BrokerMessageType::INIT, 1, &parent_channel_);
+  DCHECK(result);
+}
+
+Broker::~Broker() {}
+
+ScopedPlatformHandle Broker::GetParentPlatformHandle() {
+  return std::move(parent_channel_);
+}
+
+scoped_refptr<PlatformSharedBuffer> Broker::GetSharedBuffer(size_t num_bytes) {
+  base::AutoLock lock(lock_);
+  BufferRequestData* buffer_request;
+  Channel::MessagePtr out_message = CreateBrokerMessage(
+      BrokerMessageType::BUFFER_REQUEST, 0, &buffer_request);
+  buffer_request->size = base::checked_cast<uint32_t>(num_bytes);
+  DWORD bytes_written = 0;
+  BOOL result = ::WriteFile(sync_channel_.get().handle, out_message->data(),
+                            static_cast<DWORD>(out_message->data_num_bytes()),
+                            &bytes_written, nullptr);
+  if (!result ||
+      static_cast<size_t>(bytes_written) != out_message->data_num_bytes()) {
+    LOG(ERROR) << "Error sending sync broker message";
+    return nullptr;
+  }
+
+  ScopedPlatformHandle handles[2];
+  if (WaitForBrokerMessage(sync_channel_.get(),
+                           BrokerMessageType::BUFFER_RESPONSE, 2,
+                           &handles[0])) {
+    return PlatformSharedBuffer::CreateFromPlatformHandlePair(
+        num_bytes, std::move(handles[0]), std::move(handles[1]));
+  }
+
+  return nullptr;
+}
+
+}  // namespace edk
+}  // namespace mojo

mojo/edk/system/node_controller.cc

@@ -228,10 +228,9 @@ void NodeController::CloseChildPorts(const std::string& child_token) {
 }
 
 void NodeController::ConnectToParent(ScopedPlatformHandle platform_handle) {
-// TODO(amistry): Consider the need for a broker on Windows.
-#if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_NACL_SFI)
-  // On posix, use the bootstrap channel for the broker and receive the node's
-  // channel synchronously as the first message from the broker.
+#if !defined(OS_MACOSX) && !defined(OS_NACL_SFI)
+  // Use the bootstrap channel for the broker and receive the node's channel
+  // synchronously as the first message from the broker.
   base::ElapsedTimer timer;
   broker_.reset(new Broker(std::move(platform_handle)));
   platform_handle = broker_->GetParentPlatformHandle();
@@ -356,7 +355,7 @@ int NodeController::MergeLocalPorts(const ports::PortRef& port0,
 
 scoped_refptr<PlatformSharedBuffer> NodeController::CreateSharedBuffer(
     size_t num_bytes) {
-#if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_NACL_SFI)
+#if !defined(OS_MACOSX) && !defined(OS_NACL_SFI)
   // Shared buffer creation failure is fatal, so always use the broker when we
   // have one. This does mean that a non-root process that has children will use
   // the broker for shared buffer creation even though that process is
@@ -392,10 +391,11 @@ void NodeController::ConnectToChildOnIOThread(
     const ProcessErrorCallback& process_error_callback) {
   DCHECK(io_task_runner_->RunsTasksOnCurrentThread());
 
-#if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_NACL)
+#if !defined(OS_MACOSX) && !defined(OS_NACL)
   PlatformChannelPair node_channel;
   // BrokerHost owns itself.
-  BrokerHost* broker_host = new BrokerHost(std::move(platform_handle));
+  BrokerHost* broker_host =
+      new BrokerHost(process_handle, std::move(platform_handle));
   broker_host->SendChannel(node_channel.PassClientHandle());
   scoped_refptr<NodeChannel> channel = NodeChannel::Create(
       this, node_channel.PassServerHandle(), io_task_runner_,

mojo/edk/system/node_controller.h

@@ -331,8 +331,8 @@ class NodeController : public ports::NodeDelegate,
   // Must only be accessed from the IO thread.
   bool destroy_on_io_thread_shutdown_ = false;
 
-#if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_NACL_SFI)
-  // Broker for sync shared buffer creation (non-Mac posix-only) in children.
+#if !defined(OS_MACOSX) && !defined(OS_NACL_SFI)
+  // Broker for sync shared buffer creation in children.
   std::unique_ptr<Broker> broker_;
 #endif