Use permission_broker's OpenPath method to open serial ports.

This new method allows Chrome to open device nodes without having to
chmod them to a group of which Chrome is a member and allows the
permission broker to set opens on the file descriptor before passing it
to Chrome. This is a win for security and future extensibility.

BUG=None

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

Cr-Commit-Position: refs/heads/master@{#338478}

5 files changed, 64 insertions, 69 deletions

diff --git a/device/serial/DEPS b/device/serial/DEPS
index 6a2f02e..fc30a89 100644
--- a/device/serial/DEPS
+++ b/device/serial/DEPS
@@ -1,3 +1,4 @@
 include_rules = [
+  "+dbus",
   "+net/base",
 ]
diff --git a/device/serial/serial_io_handler.cc b/device/serial/serial_io_handler.cc
index 4a244b1..452f91c 100644
--- a/device/serial/serial_io_handler.cc
+++ b/device/serial/serial_io_handler.cc
@@ -9,6 +9,12 @@
 #include "base/message_loop/message_loop.h"
 #include "base/strings/string_util.h"
 
+#if defined(OS_CHROMEOS)
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/permission_broker_client.h"
+#include "dbus/file_descriptor.h"
+#endif  // defined(OS_CHROMEOS)
+
 namespace device {
 
 SerialIoHandler::SerialIoHandler(
@@ -38,34 +44,53 @@ void SerialIoHandler::Open(const std::string& port,
   DCHECK(file_thread_task_runner_.get());
   DCHECK(ui_thread_task_runner_.get());
   MergeConnectionOptions(options);
-  RequestAccess(port, file_thread_task_runner_, ui_thread_task_runner_);
-}
 
-void SerialIoHandler::RequestAccess(
-    const std::string& port,
-    scoped_refptr<base::SingleThreadTaskRunner> file_task_runner,
-    scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner) {
-  OnRequestAccessComplete(port, true /* success */);
+#if defined(OS_CHROMEOS)
+  chromeos::PermissionBrokerClient* client =
+      chromeos::DBusThreadManager::Get()->GetPermissionBrokerClient();
+  DCHECK(client) << "Could not get permission_broker client.";
+  // PermissionBrokerClient should be called on the UI thread.
+  ui_thread_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&chromeos::PermissionBrokerClient::OpenPath,
+                            base::Unretained(client), port,
+                            base::Bind(&SerialIoHandler::OnPathOpened, this,
+                                       file_thread_task_runner_,
+                                       base::ThreadTaskRunnerHandle::Get())));
+#else
+  file_thread_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&SerialIoHandler::StartOpen, this, port,
+                            base::ThreadTaskRunnerHandle::Get()));
+#endif  // defined(OS_CHROMEOS)
 }
 
-void SerialIoHandler::OnRequestAccessComplete(const std::string& port,
-                                              bool success) {
+#if defined(OS_CHROMEOS)
+
+void SerialIoHandler::OnPathOpened(
+    scoped_refptr<base::SingleThreadTaskRunner> file_thread_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> io_thread_task_runner,
+    dbus::FileDescriptor fd) {
   DCHECK(CalledOnValidThread());
-  if (success) {
-    DCHECK(file_thread_task_runner_.get());
-    file_thread_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&SerialIoHandler::StartOpen, this, port,
-                              base::ThreadTaskRunnerHandle::Get()));
-    return;
-  } else {
-    DCHECK(!open_complete_.is_null());
-    OpenCompleteCallback callback = open_complete_;
-    open_complete_.Reset();
-    callback.Run(false);
-    return;
+  file_thread_task_runner->PostTask(
+      FROM_HERE, base::Bind(&SerialIoHandler::ValidateOpenPort, this,
+                            io_thread_task_runner, base::Passed(&fd)));
+}
+
+void SerialIoHandler::ValidateOpenPort(
+    scoped_refptr<base::SingleThreadTaskRunner> io_thread_task_runner,
+    dbus::FileDescriptor fd) {
+  base::File file;
+  fd.CheckValidity();
+  if (fd.is_valid()) {
+    file = base::File(fd.TakeValue());
   }
+
+  io_thread_task_runner->PostTask(
+      FROM_HERE,
+      base::Bind(&SerialIoHandler::FinishOpen, this, base::Passed(&file)));
 }
 
+#endif
+
 void SerialIoHandler::MergeConnectionOptions(
     const serial::ConnectionOptions& options) {
   if (options.bitrate) {
@@ -103,7 +128,7 @@ void SerialIoHandler::StartOpen(
               base::File::FLAG_TERMINAL_DEVICE;
   base::File file(path, flags);
   io_task_runner->PostTask(FROM_HERE, base::Bind(&SerialIoHandler::FinishOpen,
-                                                 this, Passed(file.Pass())));
+                                                 this, base::Passed(&file)));
 }
 
 void SerialIoHandler::FinishOpen(base::File file) {
diff --git a/device/serial/serial_io_handler.h b/device/serial/serial_io_handler.h
index bd8255e..81d7028 100644
--- a/device/serial/serial_io_handler.h
+++ b/device/serial/serial_io_handler.h
@@ -14,6 +14,10 @@
 #include "device/serial/buffer.h"
 #include "device/serial/serial.mojom.h"
 
+namespace dbus {
+class FileDescriptor;
+}
+
 namespace device {
 
 // Provides a simplified interface for performing asynchronous I/O on serial
@@ -35,8 +39,18 @@ class SerialIoHandler : public base::NonThreadSafe,
                     const serial::ConnectionOptions& options,
                     const OpenCompleteCallback& callback);
 
-  // Signals that the access request for |port| is complete.
-  void OnRequestAccessComplete(const std::string& port, bool success);
+#if defined(OS_CHROMEOS)
+  // Signals that the port has been opened.
+  void OnPathOpened(
+      scoped_refptr<base::SingleThreadTaskRunner> file_thread_task_runner,
+      scoped_refptr<base::SingleThreadTaskRunner> io_thread_task_runner,
+      dbus::FileDescriptor fd);
+
+  // Validates the file descriptor provided by the permission broker.
+  void ValidateOpenPort(
+      scoped_refptr<base::SingleThreadTaskRunner> io_thread_task_runner,
+      dbus::FileDescriptor fd);
+#endif  // defined(OS_CHROMEOS)
 
   // Performs an async Read operation. Behavior is undefined if this is called
   // while a Read is already pending. Otherwise, the Done or DoneWithError
@@ -120,12 +134,6 @@ class SerialIoHandler : public base::NonThreadSafe,
   // Platform-specific port configuration applies options_ to the device.
   virtual bool ConfigurePortImpl() = 0;
 
-  // Requests access to the underlying serial device, if needed.
-  virtual void RequestAccess(
-      const std::string& port,
-      scoped_refptr<base::SingleThreadTaskRunner> file_task_runner,
-      scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner);
-
   // Performs platform-specific, one-time port configuration on open.
   virtual bool PostOpen();
 
diff --git a/device/serial/serial_io_handler_posix.cc b/device/serial/serial_io_handler_posix.cc
index 91cd110..b3685e0 100644
--- a/device/serial/serial_io_handler_posix.cc
+++ b/device/serial/serial_io_handler_posix.cc
@@ -11,12 +11,6 @@
 
 #if defined(OS_LINUX)
 #include <linux/serial.h>
-#if defined(OS_CHROMEOS)
-#include "base/bind.h"
-#include "base/sys_info.h"
-#include "chromeos/dbus/dbus_thread_manager.h"
-#include "chromeos/dbus/permission_broker_client.h"
-#endif  // defined(OS_CHROMEOS)
 
 // The definition of struct termios2 is copied from asm-generic/termbits.h
 // because including that header directly conflicts with termios.h.
@@ -153,35 +147,6 @@ scoped_refptr<SerialIoHandler> SerialIoHandler::Create(
                                   ui_thread_task_runner);
 }
 
-void SerialIoHandlerPosix::RequestAccess(
-    const std::string& port,
-    scoped_refptr<base::SingleThreadTaskRunner> file_task_runner,
-    scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner) {
-#if defined(OS_LINUX) && defined(OS_CHROMEOS)
-  if (base::SysInfo::IsRunningOnChromeOS()) {
-    chromeos::PermissionBrokerClient* client =
-        chromeos::DBusThreadManager::Get()->GetPermissionBrokerClient();
-    if (!client) {
-      DVLOG(1) << "Could not get permission_broker client.";
-      OnRequestAccessComplete(port, false /* failure */);
-      return;
-    }
-    // PermissionBrokerClient should be called on the UI thread.
-    ui_task_runner->PostTask(
-        FROM_HERE,
-        base::Bind(
-            &chromeos::PermissionBrokerClient::RequestPathAccess,
-            base::Unretained(client), port, -1,
-            base::Bind(&SerialIoHandler::OnRequestAccessComplete, this, port)));
-  } else {
-    OnRequestAccessComplete(port, true /* success */);
-    return;
-  }
-#else
-  OnRequestAccessComplete(port, true /* success */);
-#endif  // defined(OS_LINUX) && defined(OS_CHROMEOS)
-}
-
 void SerialIoHandlerPosix::ReadImpl() {
   DCHECK(CalledOnValidThread());
   DCHECK(pending_read_buffer());
diff --git a/device/serial/serial_io_handler_posix.h b/device/serial/serial_io_handler_posix.h
index 6d744c7..c9c5fb5 100644
--- a/device/serial/serial_io_handler_posix.h
+++ b/device/serial/serial_io_handler_posix.h
@@ -28,10 +28,6 @@ class SerialIoHandlerPosix : public SerialIoHandler,
   serial::ConnectionInfoPtr GetPortInfo() const override;
   bool SetBreak() override;
   bool ClearBreak() override;
-  void RequestAccess(
-      const std::string& port,
-      scoped_refptr<base::SingleThreadTaskRunner> file_task_runner,
-      scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner) override;
 
  private:
   friend class SerialIoHandler;