dbus: revamp fd passing support for i/o restrictions

    
Encapsulate file descriptor validity checking and status in the companion FileDescriptor class so callers can do descriptor checking in a context where i/o is allowed.
    
Update the debug daemon client support to validate the pipe descriptors in a worker thread so it is not done on the UI thread.

BUG=126142

TEST=new unit tests + collect trace data on chrome os and verify no assert is triggered

Review URL: https://chromiumcodereview.appspot.com/10382021

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

5 files changed, 116 insertions, 44 deletions

diff --git a/chromeos/dbus/debug_daemon_client.cc b/chromeos/dbus/debug_daemon_client.cc
index c27ca22..7200dd7 100644
--- a/chromeos/dbus/debug_daemon_client.cc
+++ b/chromeos/dbus/debug_daemon_client.cc
@@ -14,6 +14,7 @@
 #include "base/memory/ref_counted_memory.h"
 #include "base/platform_file.h"
 #include "base/string_util.h"
+#include "base/threading/worker_pool.h"
 #include "dbus/bus.h"
 #include "dbus/message.h"
 #include "dbus/object_path.h"
@@ -152,19 +153,19 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
   // DebugDaemonClient override.
   virtual void GetDebugLogs(base::PlatformFile file,
                             const GetDebugLogsCallback& callback) OVERRIDE {
-    dbus::MethodCall method_call(
-        debugd::kDebugdInterface,
-        debugd::kGetDebugLogs);
-    dbus::MessageWriter writer(&method_call);
-    dbus::FileDescriptor fd(file);  // explicit temp for C++ 98
-    writer.AppendFileDescriptor(fd);
 
-    debugdaemon_proxy_->CallMethod(
-        &method_call,
-        dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::Bind(&DebugDaemonClientImpl::OnGetDebugLogs,
+    dbus::FileDescriptor* file_descriptor = new dbus::FileDescriptor(file);
+    // Punt descriptor validity check to a worker thread; on return we'll
+    // issue the D-Bus request to stop tracing and collect results.
+    base::WorkerPool::PostTaskAndReply(
+        FROM_HERE,
+        base::Bind(&DebugDaemonClientImpl::CheckValidity,
+                   file_descriptor),
+        base::Bind(&DebugDaemonClientImpl::OnCheckValidityGetDebugLogs,
                    weak_ptr_factory_.GetWeakPtr(),
-                   callback));
+                   base::Owned(file_descriptor),
+                   callback),
+        false);
   }
 
   virtual void SetDebugMode(const std::string& subsystem,
@@ -216,29 +217,46 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
       write_fd = pipe_reader_->GetWriteFD();
     }
 
-    callback_ = callback;
+    dbus::FileDescriptor* file_descriptor = new dbus::FileDescriptor(write_fd);
+    // Punt descriptor validity check to a worker thread; on return we'll
+    // issue the D-Bus request to stop tracing and collect results.
+    base::WorkerPool::PostTaskAndReply(
+        FROM_HERE,
+        base::Bind(&DebugDaemonClientImpl::CheckValidity,
+                   file_descriptor),
+        base::Bind(&DebugDaemonClientImpl::OnCheckValidityRequestStopSystem,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   base::Owned(file_descriptor),
+                   callback),
+        false);
 
-    // Issue the dbus request to stop system tracing
+    return true;
+  }
+
+ private:
+  // Called to check descriptor validity on a thread where i/o is permitted.
+  static void CheckValidity(dbus::FileDescriptor* file_descriptor) {
+    file_descriptor->CheckValidity();
+  }
+
+  // Called when a CheckValidity response is received.
+  void OnCheckValidityGetDebugLogs(dbus::FileDescriptor* file_descriptor,
+                                   const GetDebugLogsCallback& callback) {
+    // Issue the dbus request to get debug logs.
     dbus::MethodCall method_call(
         debugd::kDebugdInterface,
-        debugd::kSystraceStop);
+        debugd::kGetDebugLogs);
     dbus::MessageWriter writer(&method_call);
-    dbus::FileDescriptor temp(write_fd);  // NB: explicit temp for C++98
-    writer.AppendFileDescriptor(temp);
+    writer.AppendFileDescriptor(*file_descriptor);
 
-    DVLOG(1) << "Requesting a systrace stop";
     debugdaemon_proxy_->CallMethod(
         &method_call,
         dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
-        base::Bind(&DebugDaemonClientImpl::OnRequestStopSystemTracing,
-                   weak_ptr_factory_.GetWeakPtr()));
-
-    pipe_reader_->CloseWriteFD();  // close our copy of fd after send
-
-    return true;
+        base::Bind(&DebugDaemonClientImpl::OnGetDebugLogs,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   callback));
   }
 
- private:
   // Called when a response for GetDebugLogs() is received.
   void OnGetDebugLogs(const GetDebugLogsCallback& callback,
                       dbus::Response* response) {
@@ -269,6 +287,29 @@ class DebugDaemonClientImpl : public DebugDaemonClient {
     }
   }
 
+  // Called when a CheckValidity response is received.
+  void OnCheckValidityRequestStopSystem(
+      dbus::FileDescriptor* file_descriptor,
+      const StopSystemTracingCallback& callback) {
+    // Issue the dbus request to stop system tracing
+    dbus::MethodCall method_call(
+        debugd::kDebugdInterface,
+        debugd::kSystraceStop);
+    dbus::MessageWriter writer(&method_call);
+    writer.AppendFileDescriptor(*file_descriptor);
+
+    callback_ = callback;
+
+    DVLOG(1) << "Requesting a systrace stop";
+    debugdaemon_proxy_->CallMethod(
+        &method_call,
+        dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+        base::Bind(&DebugDaemonClientImpl::OnRequestStopSystemTracing,
+                   weak_ptr_factory_.GetWeakPtr()));
+
+    pipe_reader_->CloseWriteFD();  // close our copy of fd after send
+  }
+
   // Called when a response for RequestStopSystemTracing() is received.
   void OnRequestStopSystemTracing(dbus::Response* response) {
     if (!response) {
diff --git a/dbus/file_descriptor.cc b/dbus/file_descriptor.cc
index fc240f5..91b089f 100644
--- a/dbus/file_descriptor.cc
+++ b/dbus/file_descriptor.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/logging.h"
 #include "base/platform_file.h"
 #include "dbus/file_descriptor.h"
 
@@ -12,4 +13,21 @@ FileDescriptor::~FileDescriptor() {
     base::ClosePlatformFile(value_);
 }
 
+int FileDescriptor::value() const {
+  CHECK(valid_);
+  return value_;
+}
+
+int FileDescriptor::TakeValue() {
+  CHECK(valid_);  // NB: check first so owner_ is unchanged if this triggers
+  owner_ = false;
+  return value_;
+}
+
+void FileDescriptor::CheckValidity() {
+  base::PlatformFileInfo info;
+  bool ok = base::GetPlatformFileInfo(value_, &info);
+  valid_ = (ok && !info.is_directory);
+}
+
 }  // namespace dbus
diff --git a/dbus/file_descriptor.h b/dbus/file_descriptor.h
index 12f1a2e..7034617 100644
--- a/dbus/file_descriptor.h
+++ b/dbus/file_descriptor.h
@@ -23,33 +23,47 @@ namespace dbus {
 // the descriptor in fd will be closed if the PopUint32 fails.  But
 //   writer.AppendFileDescriptor(dbus::FileDescriptor(1));
 // will not automatically close "1" because it is not owned.
+//
+// Descriptors must be validated before marshalling in a D-Bus message
+// or using them after unmarshalling.  We disallow descriptors to a
+// directory to reduce the security risks.  Splitting out validation
+// also allows the caller to do this work on the File thread to conform
+// with i/o restrictions.
 class FileDescriptor {
  public:
   // Permits initialization without a value for passing to
   // dbus::MessageReader::PopFileDescriptor to fill in and from int values.
-  FileDescriptor() : value_(-1), owner_(false) {}
-  explicit FileDescriptor(int value) : value_(value), owner_(false) {}
+  FileDescriptor() : value_(-1), owner_(false), valid_(false) {}
+  explicit FileDescriptor(int value) : value_(value), owner_(false),
+      valid_(false) {}
 
   virtual ~FileDescriptor();
 
   // Retrieves value as an int without affecting ownership.
-  int value() const { return value_; }
+  int value() const;
+
+  // Retrieves whether or not the descriptor is ok to send/receive.
+  int is_valid() const { return valid_; }
 
   // Sets the value and assign ownership.
   void PutValue(int value) {
     value_ = value;
     owner_ = true;
+    valid_ = false;
   }
 
   // Takes the value and ownership.
-  int TakeValue() {
-    owner_ = false;
-    return value_;
-  }
+  int TakeValue();
+
+  // Checks (and records) validity of the file descriptor.
+  // We disallow directories to avoid potential sandbox escapes.
+  // Note this call must be made on a thread where file i/o is allowed.
+  void CheckValidity();
 
  private:
   int value_;
   bool owner_;
+  bool valid_;
 
   DISALLOW_COPY_AND_ASSIGN(FileDescriptor);
 };
diff --git a/dbus/message.cc b/dbus/message.cc
index d525912..400c1bc 100644
--- a/dbus/message.cc
+++ b/dbus/message.cc
@@ -9,7 +9,6 @@
 #include "base/basictypes.h"
 #include "base/format_macros.h"
 #include "base/logging.h"
-#include "base/platform_file.h"
 #include "base/stringprintf.h"
 #include "dbus/object_path.h"
 #include "third_party/protobuf/src/google/protobuf/message_lite.h"
@@ -691,13 +690,11 @@ void MessageWriter::AppendVariantOfBasic(int dbus_type, const void* value) {
 void MessageWriter::AppendFileDescriptor(const FileDescriptor& value) {
   CHECK(kDBusTypeUnixFdIsSupported);
 
-  base::PlatformFileInfo info;
-  int fd = value.value();
-  bool ok = base::GetPlatformFileInfo(fd, &info);
-  if (!ok || info.is_directory) {
+  if (!value.is_valid()) {
     // NB: sending a directory potentially enables sandbox escape
     LOG(FATAL) << "Attempt to pass invalid file descriptor";
   }
+  int fd = value.value();
   AppendBasic(DBUS_TYPE_UNIX_FD, &fd);
 }
 
@@ -968,15 +965,8 @@ bool MessageReader::PopFileDescriptor(FileDescriptor* value) {
   if (!success)
     return false;
 
-  base::PlatformFileInfo info;
-  bool ok = base::GetPlatformFileInfo(fd, &info);
-  if (!ok || info.is_directory) {
-    base::ClosePlatformFile(fd);
-    // NB: receiving a directory potentially enables sandbox escape
-    LOG(FATAL) << "Attempt to receive invalid file descriptor";
-    return false;  // NB: not reached
-  }
   value->PutValue(fd);
+  // NB: the caller must check validity before using the value
   return true;
 }
 
diff --git a/dbus/message_unittest.cc b/dbus/message_unittest.cc
index 424b50a..9d87c2a 100644
--- a/dbus/message_unittest.cc
+++ b/dbus/message_unittest.cc
@@ -107,6 +107,11 @@ TEST(MessageTest, AppendAndPopFileDescriptor) {
 
   // Append stdout.
   dbus::FileDescriptor temp(1);
+  // Descriptor should not be valid until checked.
+  ASSERT_FALSE(temp.is_valid());
+  // NB: thread IO requirements not relevant for unit tests.
+  temp.CheckValidity();
+  ASSERT_TRUE(temp.is_valid());
   writer.AppendFileDescriptor(temp);
 
   dbus::FileDescriptor fd_value;
@@ -115,6 +120,10 @@ TEST(MessageTest, AppendAndPopFileDescriptor) {
   ASSERT_TRUE(reader.HasMoreData());
   ASSERT_TRUE(reader.PopFileDescriptor(&fd_value));
   ASSERT_FALSE(reader.HasMoreData());
+  // Descriptor is not valid until explicitly checked.
+  ASSERT_FALSE(fd_value.is_valid());
+  fd_value.CheckValidity();
+  ASSERT_TRUE(fd_value.is_valid());
 
   // Stdout should be returned but we cannot check the descriptor
   // value because stdout will be dup'd.  Instead check st_rdev