Add sender verification of D-Bus signals.

The CL does the following:
- Add a match rule for NameOwnerChanged signal from org.freedesktop.DBus.
- Update the owner of the well-known bus name when a NameOwnerChanged comes.
- Call GetNameOwner method to update the latest if ObjectProxy instance does not know the owner.
- Verify the sender of the signal and reject the unknown senders.
- Add UMA_HISTOGRAM_COUNTS "DBus.RejectedSignalCount" for rejected signals.

and a unittest.

BUG=140938
TEST=manual, unittests

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

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

6 files changed, 365 insertions, 17 deletions

diff --git a/dbus/dbus.gyp b/dbus/dbus.gyp
index a902b07..25317de 100644
--- a/dbus/dbus.gyp
+++ b/dbus/dbus.gyp
@@ -92,6 +92,7 @@
         'message_unittest.cc',
         'mock_unittest.cc',
         'property_unittest.cc',
+        'signal_sender_verification_unittest.cc',
         'string_util_unittest.cc',
         'test_service.cc',
         'test_service.h',
diff --git a/dbus/object_proxy.cc b/dbus/object_proxy.cc
index ba51efa..c8f00d7 100644
--- a/dbus/object_proxy.cc
+++ b/dbus/object_proxy.cc
@@ -24,6 +24,9 @@ const char kErrorServiceUnknown[] = "org.freedesktop.DBus.Error.ServiceUnknown";
 // Used for success ratio histograms. 1 for success, 0 for failure.
 const int kSuccessRatioHistogramMaxValue = 2;
 
+// The path of D-Bus Object sending NameOwnerChanged signal.
+const char kDbusSystemObjectPath[] = "/org/freedesktop/DBus";
+
 // Gets the absolute signal name by concatenating the interface name and
 // the signal name. Used for building keys for method_table_ in
 // ObjectProxy.
@@ -362,25 +365,27 @@ void ObjectProxy::ConnectToSignalInternal(
         base::StringPrintf("type='signal', interface='%s', path='%s'",
                            interface_name.c_str(),
                            object_path_.value().c_str());
-
-    // Add the match rule if we don't have it.
-    if (match_rules_.find(match_rule) == match_rules_.end()) {
-      ScopedDBusError error;
-      bus_->AddMatch(match_rule, error.get());;
-      if (error.is_set()) {
-        LOG(ERROR) << "Failed to add match rule: " << match_rule;
-      } else {
-        // Store the match rule, so that we can remove this in Detach().
-        match_rules_.insert(match_rule);
-        // Add the signal callback to the method table.
-        method_table_[absolute_signal_name] = signal_callback;
-        success = true;
-      }
-    } else {
-      // We already have the match rule.
-      method_table_[absolute_signal_name] = signal_callback;
+    // Add a match_rule listening NameOwnerChanged for the well-known name
+    // |service_name_|.
+    const std::string name_owner_changed_match_rule =
+        base::StringPrintf(
+            "type='signal',interface='org.freedesktop.DBus',"
+            "member='NameOwnerChanged',path='/org/freedesktop/DBus',"
+            "sender='org.freedesktop.DBus',arg0='%s'",
+            service_name_.c_str());
+    if (AddMatchRuleWithCallback(match_rule,
+                                 absolute_signal_name,
+                                 signal_callback) &&
+        AddMatchRuleWithoutCallback(name_owner_changed_match_rule,
+                                    "org.freedesktop.DBus.NameOwnerChanged")) {
       success = true;
     }
+
+    // Try getting the current name owner. It's not guaranteed that we can get
+    // the name owner at this moment, as the service may not yet be started. If
+    // that's the case, we'll get the name owner via NameOwnerChanged signal,
+    // as soon as the service is started.
+    UpdateNameOwnerAndBlock();
   }
 
   // Run on_connected_callback in the origin thread.
@@ -407,6 +412,7 @@ DBusHandlerResult ObjectProxy::HandleMessage(
     DBusConnection* connection,
     DBusMessage* raw_message) {
   bus_->AssertOnDBusThread();
+
   if (dbus_message_get_type(raw_message) != DBUS_MESSAGE_TYPE_SIGNAL)
     return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 
@@ -421,6 +427,11 @@ DBusHandlerResult ObjectProxy::HandleMessage(
   // allow other object proxies to handle instead.
   const dbus::ObjectPath path = signal->GetPath();
   if (path != object_path_) {
+    if (path.value() == kDbusSystemObjectPath &&
+        signal->GetMember() == "NameOwnerChanged") {
+      // Handle NameOwnerChanged separately
+      return HandleNameOwnerChanged(signal.get());
+    }
     return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
   }
 
@@ -437,6 +448,13 @@ DBusHandlerResult ObjectProxy::HandleMessage(
   }
   VLOG(1) << "Signal received: " << signal->ToString();
 
+  std::string sender = signal->GetSender();
+  if (service_name_owner_ != sender) {
+    LOG(ERROR) << "Rejecting a message from a wrong sender.";
+    UMA_HISTOGRAM_COUNTS("DBus.RejectedSignalCount", 1);
+    return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+  }
+
   const base::TimeTicks start_time = base::TimeTicks::Now();
   if (bus_->HasDBusThread()) {
     // Post a task to run the method in the origin thread.
@@ -515,4 +533,113 @@ void ObjectProxy::OnCallMethodError(const std::string& interface_name,
   response_callback.Run(NULL);
 }
 
+bool ObjectProxy::AddMatchRuleWithCallback(
+    const std::string& match_rule,
+    const std::string& absolute_signal_name,
+    SignalCallback signal_callback) {
+  DCHECK(!match_rule.empty());
+  DCHECK(!absolute_signal_name.empty());
+  bus_->AssertOnDBusThread();
+
+  if (match_rules_.find(match_rule) == match_rules_.end()) {
+    ScopedDBusError error;
+    bus_->AddMatch(match_rule, error.get());
+    if (error.is_set()) {
+      LOG(ERROR) << "Failed to add match rule \"" << match_rule << "\". Got " <<
+          error.name() << ": " << error.message();
+      return false;
+    } else {
+      // Store the match rule, so that we can remove this in Detach().
+      match_rules_.insert(match_rule);
+      // Add the signal callback to the method table.
+      method_table_[absolute_signal_name] = signal_callback;
+      return true;
+    }
+  } else {
+    // We already have the match rule.
+    method_table_[absolute_signal_name] = signal_callback;
+    return true;
+  }
+}
+
+bool ObjectProxy::AddMatchRuleWithoutCallback(
+    const std::string& match_rule,
+    const std::string& absolute_signal_name) {
+  DCHECK(!match_rule.empty());
+  DCHECK(!absolute_signal_name.empty());
+  bus_->AssertOnDBusThread();
+
+  if (match_rules_.find(match_rule) != match_rules_.end())
+    return true;
+
+  ScopedDBusError error;
+  bus_->AddMatch(match_rule, error.get());
+  if (error.is_set()) {
+    LOG(ERROR) << "Failed to add match rule \"" << match_rule << "\". Got " <<
+        error.name() << ": " << error.message();
+    return false;
+  }
+  // Store the match rule, so that we can remove this in Detach().
+  match_rules_.insert(match_rule);
+  return true;
+}
+
+void ObjectProxy::UpdateNameOwnerAndBlock() {
+  bus_->AssertOnDBusThread();
+
+  MethodCall get_name_owner_call("org.freedesktop.DBus", "GetNameOwner");
+  MessageWriter writer(&get_name_owner_call);
+  writer.AppendString(service_name_);
+  VLOG(1) << "Method call: " << get_name_owner_call.ToString();
+
+  const dbus::ObjectPath obj_path("/org/freedesktop/DBus");
+  ScopedDBusError error;
+  if (!get_name_owner_call.SetDestination("org.freedesktop.DBus") ||
+      !get_name_owner_call.SetPath(obj_path)) {
+    LOG(ERROR) << "Failed to get name owner.";
+    return;
+  }
+
+  DBusMessage* response_message = bus_->SendWithReplyAndBlock(
+      get_name_owner_call.raw_message(),
+      TIMEOUT_USE_DEFAULT,
+      error.get());
+  if (!response_message) {
+    LOG(ERROR) << "Failed to get name owner. Got " << error.name() << ": " <<
+        error.message();
+    return;
+  }
+  scoped_ptr<Response> response(Response::FromRawMessage(response_message));
+  MessageReader reader(response.get());
+
+  std::string new_service_name_owner;
+  if (reader.PopString(&new_service_name_owner))
+    service_name_owner_ = new_service_name_owner;
+  else
+    service_name_owner_.clear();
+}
+
+DBusHandlerResult ObjectProxy::HandleNameOwnerChanged(Signal* signal) {
+  DCHECK(signal);
+  bus_->AssertOnDBusThread();
+
+  // Confirm the validity of the NameOwnerChanged signal.
+  if (signal->GetMember() == "NameOwnerChanged" &&
+      signal->GetInterface() == "org.freedesktop.DBus" &&
+      signal->GetSender() == "org.freedesktop.DBus") {
+    MessageReader reader(signal);
+    std::string name, old_owner, new_owner;
+    if (reader.PopString(&name) &&
+        reader.PopString(&old_owner) &&
+        reader.PopString(&new_owner) &&
+        name == service_name_) {
+      service_name_owner_ = new_owner;
+      return DBUS_HANDLER_RESULT_HANDLED;
+    }
+  }
+
+  // Untrusted or uninteresting signal
+  return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+}
+
 }  // namespace dbus
diff --git a/dbus/object_proxy.h b/dbus/object_proxy.h
index 9642262..71ec0f1 100644
--- a/dbus/object_proxy.h
+++ b/dbus/object_proxy.h
@@ -236,6 +236,24 @@ class ObjectProxy : public base::RefCountedThreadSafe<ObjectProxy> {
                          ResponseCallback response_callback,
                          ErrorResponse* error_response);
 
+  // Adds the match rule to the bus and associate the callback with the signal.
+  bool AddMatchRuleWithCallback(const std::string& match_rule,
+                                const std::string& absolute_signal_name,
+                                SignalCallback signal_callback);
+
+  // Adds the match rule to the bus so that HandleMessage can see the signal.
+  bool AddMatchRuleWithoutCallback(const std::string& match_rule,
+                                   const std::string& absolute_signal_name);
+
+  // Calls D-Bus's GetNameOwner method synchronously to update
+  // |service_name_owner_| with the current owner of |service_name_|.
+  //
+  // BLOCKING CALL.
+  void UpdateNameOwnerAndBlock();
+
+  // Handles NameOwnerChanged signal from D-Bus's special message bus.
+  DBusHandlerResult HandleNameOwnerChanged(dbus::Signal* signal);
+
   scoped_refptr<Bus> bus_;
   std::string service_name_;
   ObjectPath object_path_;
@@ -252,6 +270,9 @@ class ObjectProxy : public base::RefCountedThreadSafe<ObjectProxy> {
 
   const bool ignore_service_unknown_errors_;
 
+  // Known name owner of the well-known bus name represnted by |service_name_|.
+  std::string service_name_owner_;
+
   DISALLOW_COPY_AND_ASSIGN(ObjectProxy);
 };
 
diff --git a/dbus/signal_sender_verification_unittest.cc b/dbus/signal_sender_verification_unittest.cc
new file mode 100644
index 0000000..5179fff
--- /dev/null
+++ b/dbus/signal_sender_verification_unittest.cc
@@ -0,0 +1,180 @@
+// Copyright (c) 2012 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 "base/bind.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop.h"
+#include "base/metrics/histogram.h"
+#include "base/metrics/histogram_samples.h"
+#include "base/metrics/statistics_recorder.h"
+#include "base/test/test_timeouts.h"
+#include "base/threading/platform_thread.h"
+#include "base/threading/thread_restrictions.h"
+#include "dbus/bus.h"
+#include "dbus/message.h"
+#include "dbus/object_proxy.h"
+#include "dbus/test_service.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+// The test for sender verification in ObjectProxy.
+class SignalSenderVerificationTest : public testing::Test {
+ public:
+  SignalSenderVerificationTest() {
+  }
+
+  virtual void SetUp() {
+    base::StatisticsRecorder::Initialize();
+
+    // Make the main thread not to allow IO.
+    base::ThreadRestrictions::SetIOAllowed(false);
+
+    // Start the D-Bus thread.
+    dbus_thread_.reset(new base::Thread("D-Bus Thread"));
+    base::Thread::Options thread_options;
+    thread_options.message_loop_type = MessageLoop::TYPE_IO;
+    ASSERT_TRUE(dbus_thread_->StartWithOptions(thread_options));
+
+    // Start the test service, using the D-Bus thread.
+    dbus::TestService::Options options;
+    options.dbus_thread_message_loop_proxy = dbus_thread_->message_loop_proxy();
+    test_service_.reset(new dbus::TestService(options));
+    ASSERT_TRUE(test_service_->StartService());
+    ASSERT_TRUE(test_service_->WaitUntilServiceIsStarted());
+    ASSERT_TRUE(test_service_->HasDBusThread());
+
+    // Same setup for the second TestService. This service should not have the
+    // ownership of the name at this point.
+    test_service2_.reset(new dbus::TestService(options));
+    ASSERT_TRUE(test_service2_->StartService());
+    ASSERT_TRUE(test_service2_->WaitUntilServiceIsStarted());
+    ASSERT_TRUE(test_service2_->HasDBusThread());
+
+    // Create the client, using the D-Bus thread.
+    dbus::Bus::Options bus_options;
+    bus_options.bus_type = dbus::Bus::SESSION;
+    bus_options.connection_type = dbus::Bus::PRIVATE;
+    bus_options.dbus_thread_message_loop_proxy =
+        dbus_thread_->message_loop_proxy();
+    bus_ = new dbus::Bus(bus_options);
+    object_proxy_ = bus_->GetObjectProxy(
+        "org.chromium.TestService",
+        dbus::ObjectPath("/org/chromium/TestObject"));
+    ASSERT_TRUE(bus_->HasDBusThread());
+
+    // Connect to the "Test" signal of "org.chromium.TestInterface" from
+    // the remote object.
+    object_proxy_->ConnectToSignal(
+        "org.chromium.TestInterface",
+        "Test",
+        base::Bind(&SignalSenderVerificationTest::OnTestSignal,
+                   base::Unretained(this)),
+        base::Bind(&SignalSenderVerificationTest::OnConnected,
+                   base::Unretained(this)));
+    // Wait until the object proxy is connected to the signal.
+    message_loop_.Run();
+  }
+
+  virtual void TearDown() {
+    bus_->ShutdownOnDBusThreadAndBlock();
+
+    // Shut down the service.
+    test_service_->ShutdownAndBlock();
+    test_service2_->ShutdownAndBlock();
+
+    // Reset to the default.
+    base::ThreadRestrictions::SetIOAllowed(true);
+
+    // Stopping a thread is considered an IO operation, so do this after
+    // allowing IO.
+    test_service_->Stop();
+    test_service2_->Stop();
+  }
+
+ protected:
+
+  // Called when the "Test" signal is received, in the main thread.
+  // Copy the string payload to |test_signal_string_|.
+  void OnTestSignal(dbus::Signal* signal) {
+    dbus::MessageReader reader(signal);
+    ASSERT_TRUE(reader.PopString(&test_signal_string_));
+    message_loop_.Quit();
+  }
+
+  // Called when connected to the signal.
+  void OnConnected(const std::string& interface_name,
+                   const std::string& signal_name,
+                   bool success) {
+    ASSERT_TRUE(success);
+    message_loop_.Quit();
+  }
+
+  // Wait for the hey signal to be received.
+  void WaitForTestSignal() {
+    // OnTestSignal() will quit the message loop.
+    message_loop_.Run();
+  }
+
+  MessageLoop message_loop_;
+  scoped_ptr<base::Thread> dbus_thread_;
+  scoped_refptr<dbus::Bus> bus_;
+  dbus::ObjectProxy* object_proxy_;
+  scoped_ptr<dbus::TestService> test_service_;
+  scoped_ptr<dbus::TestService> test_service2_;
+  // Text message from "Test" signal.
+  std::string test_signal_string_;
+};
+
+TEST_F(SignalSenderVerificationTest, TestSignalAccepted) {
+  const char kMessage[] = "hello, world";
+  // Send the test signal from the exported object.
+  test_service_->SendTestSignal(kMessage);
+  // Receive the signal with the object proxy. The signal is handled in
+  // SignalSenderVerificationTest::OnTestSignal() in the main thread.
+  WaitForTestSignal();
+  ASSERT_EQ(kMessage, test_signal_string_);
+}
+
+TEST_F(SignalSenderVerificationTest, TestSignalRejected) {
+  // To make sure the histogram instance is created.
+  UMA_HISTOGRAM_COUNTS("DBus.RejectedSignalCount", 0);
+  base::Histogram* reject_signal_histogram =
+        base::StatisticsRecorder::FindHistogram("DBus.RejectedSignalCount");
+  scoped_ptr<base::HistogramSamples> samples1(
+      reject_signal_histogram->SnapshotSamples());
+
+  const char kNewMessage[] = "hello, new world";
+  test_service2_->SendTestSignal(kNewMessage);
+
+  // This test tests that our callback is NOT called by the ObjectProxy.
+  // Sleep to have message delivered to the client via the D-Bus service.
+  base::PlatformThread::Sleep(TestTimeouts::action_timeout());
+
+  scoped_ptr<base::HistogramSamples> samples2(
+      reject_signal_histogram->SnapshotSamples());
+
+  ASSERT_EQ("", test_signal_string_);
+  EXPECT_EQ(samples1->TotalCount() + 1, samples2->TotalCount());
+}
+
+TEST_F(SignalSenderVerificationTest, TestOwnerChanged) {
+  const char kMessage[] = "hello, world";
+
+  // Send the test signal from the exported object.
+  test_service_->SendTestSignal(kMessage);
+  // Receive the signal with the object proxy. The signal is handled in
+  // SignalSenderVerificationTest::OnTestSignal() in the main thread.
+  WaitForTestSignal();
+  ASSERT_EQ(kMessage, test_signal_string_);
+
+  // Release and aquire the name ownership.
+  test_service_->ShutdownAndBlock();
+  test_service2_->RequestOwnership();
+
+  // Now the second service owns the name.
+  const char kNewMessage[] = "hello, new world";
+
+  test_service2_->SendTestSignal(kNewMessage);
+  WaitForTestSignal();
+  ASSERT_EQ(kNewMessage, test_signal_string_);
+}
diff --git a/dbus/test_service.cc b/dbus/test_service.cc
index ee1befb..87c6e01 100644
--- a/dbus/test_service.cc
+++ b/dbus/test_service.cc
@@ -103,6 +103,19 @@ void TestService::SendTestSignalFromRootInternal(const std::string& message) {
   root_object->SendSignal(&signal);
 }
 
+void TestService::RequestOwnership() {
+  message_loop()->PostTask(
+      FROM_HERE,
+      base::Bind(&TestService::RequestOwnershipInternal,
+                 base::Unretained(this)));
+}
+
+void TestService::RequestOwnershipInternal() {
+  bus_->RequestOwnership("org.chromium.TestService",
+                         base::Bind(&TestService::OnOwnership,
+                                    base::Unretained(this)));
+}
+
 void TestService::OnOwnership(const std::string& service_name,
                               bool success) {
   LOG_IF(ERROR, !success) << "Failed to own: " << service_name;
diff --git a/dbus/test_service.h b/dbus/test_service.h
index 871aed6..17f27ca 100644
--- a/dbus/test_service.h
+++ b/dbus/test_service.h
@@ -65,6 +65,9 @@ class TestService : public base::Thread {
   // This function emulates dbus-send's behavior.
   void SendTestSignalFromRoot(const std::string& message);
 
+  // Request the ownership of a well-known name "TestService".
+  void RequestOwnership();
+
  private:
   // Helper function for SendTestSignal().
   void SendTestSignalInternal(const std::string& message);
@@ -127,6 +130,9 @@ class TestService : public base::Thread {
   // Helper function for SendPropertyChangedSignal().
   void SendPropertyChangedSignalInternal(const std::string& name);
 
+  // Helper function for RequestOwnership().
+  void RequestOwnershipInternal();
+
   scoped_refptr<base::MessageLoopProxy> dbus_thread_message_loop_proxy_;
   base::WaitableEvent on_all_methods_exported_;
   // The number of methods actually exported.