From 8bbe31e8cea633027e3b2ba151774aebf8a4d4ee Mon Sep 17 00:00:00 2001
From: "haruki@chromium.org"
<haruki@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Mon, 29 Oct 2012 06:27:33 +0000
Subject: 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
---
dbus/dbus.gyp | 1 +
dbus/object_proxy.cc | 161 ++++++++++++++++++++++---
dbus/object_proxy.h | 21 ++++
dbus/signal_sender_verification_unittest.cc | 180 ++++++++++++++++++++++++++++
dbus/test_service.cc | 13 ++
dbus/test_service.h | 6 +
6 files changed, 365 insertions(+), 17 deletions(-)
create mode 100644 dbus/signal_sender_verification_unittest.cc
(limited to 'dbus')
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.
--
cgit v1.1