Add sender verification of D-Bus signals.

dbus/object_proxy.cc

 // 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 "dbus/bus.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/metrics/histogram.h"
 #include "base/string_piece.h"
 #include "base/stringprintf.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
 #include "dbus/message.h"
 #include "dbus/object_path.h"
 #include "dbus/object_proxy.h"
 #include "dbus/scoped_dbus_error.h"
 
 namespace {
 
 const char kErrorServiceUnknown[] = "org.freedesktop.DBus.Error.ServiceUnknown";
 
 // Used for success ratio histograms. 1 for success, 0 for failure.
 const int kSuccessRatioHistogramMaxValue = 2;
 
+const dbus::ObjectPath kDbusSystemObjectPath("/org/freedesktop/DBus");

[satorux1, 2012/10/22 04:50:42]

ObjectPath may be POD but we should avoid doing this. Please use const char[]
instead.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static...

[Haruki Sato, 2012/10/24 08:28:05]

Done.
Thanks!

+
 // Gets the absolute signal name by concatenating the interface name and
 // the signal name. Used for building keys for method_table_ in
 // ObjectProxy.
 std::string GetAbsoluteSignalName(
     const std::string& interface_name,
     const std::string& signal_name) {
   return interface_name + "." + signal_name;
 }
 
 // An empty function used for ObjectProxy::EmptyResponseCallback().
@@ skipping 313 matching lines @@
   if (bus_->Connect() && bus_->SetUpAsyncOperations()) {
     // We should add the filter only once. Otherwise, HandleMessage() will
     // be called more than once.
     if (!filter_added_) {
       if (bus_->AddFilterFunction(&ObjectProxy::HandleMessageThunk, this)) {
         filter_added_ = true;
       } else {
         LOG(ERROR) << "Failed to add filter function";
       }
     }
+
+    // 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());
     // Add a match rule so the signal goes through HandleMessage().
     const std::string match_rule =
         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;
-      success = true;
-    }
+    success = AddMatchRuleAndCallback(match_rule, absolute_signal_name,
+                                      signal_callback) &&

[satorux1, 2012/10/22 04:50:42]

when parameters don't fit in one line, please align them vertically.

[Haruki Sato, 2012/10/24 08:28:05]

Done.

+              AddMatchRuleWithoutCallback(
+                  name_owner_changed_match_rule,
+                  "org.freedesktop.DBus.NameOwnerChanged");

[satorux1, 2012/10/22 04:50:42]

Looks a bit tricky. Please do

if (AddMatchRuleAndCallback(..) &&
    AddMatchRuleWithoutCallback(...)) {
  success = true;
}

[Haruki Sato, 2012/10/24 08:28:05]

Done.

   }
 
   // Run on_connected_callback in the origin thread.
   bus_->PostTaskToOriginThread(
       FROM_HERE,
       base::Bind(&ObjectProxy::OnConnected,
                  this,
                  on_connected_callback,
                  interface_name,
                  signal_name,
@@ skipping 20 matching lines @@
   // reference so we can use it in Signal.
   dbus_message_ref(raw_message);
   scoped_ptr<Signal> signal(
       Signal::FromRawMessage(raw_message));
 
   // Verify the signal comes from the object we're proxying for, this is
   // our last chance to return DBUS_HANDLER_RESULT_NOT_YET_HANDLED and
   // allow other object proxies to handle instead.
   const dbus::ObjectPath path = signal->GetPath();
   if (path != object_path_) {
+    if (path == kDbusSystemObjectPath &&
+        signal->GetMember() == "NameOwnerChanged") {
+      // Handle NameOwnerChanged separately
+      return HandleNameOwnerChanged(signal.get());
+    }
     return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
   }
 
   const std::string interface = signal->GetInterface();
   const std::string member = signal->GetMember();
 
   // Check if we know about the signal.
   const std::string absolute_signal_name = GetAbsoluteSignalName(
       interface, member);
   MethodTable::const_iterator iter = method_table_.find(absolute_signal_name);
   if (iter == method_table_.end()) {
     // Don't know about the signal.
     return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
   }
   VLOG(1) << "Signal received: " << signal->ToString();
 
+  std::string sender = signal->GetSender();
+  if (service_name_owner_.empty()) {
+    UpdateNameOwner();
+  }
+
+  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.
     // Transfer the ownership of |signal| to RunMethod().
     // |released_signal| will be deleted in RunMethod().
     Signal* released_signal = signal.release();
     bus_->PostTaskToOriginThread(FROM_HERE,
                                  base::Bind(&ObjectProxy::RunMethod,
                                             this,
                                             start_time,
@@ skipping 58 matching lines @@
     std::string error_message;
     reader.PopString(&error_message);
     LogMethodCallFailure(interface_name,
                          method_name,
                          error_response->GetErrorName(),
                          error_message);
   }
   response_callback.Run(NULL);
 }
 
+bool ObjectProxy::AddMatchRuleWithoutCallback(

[satorux1, 2012/10/22 05:23:32]

The order does not match with .h file?

[Haruki Sato, 2012/10/24 08:28:05]

Done.

+    std::string match_rule, std::string absolute_signal_name) {

[satorux1, 2012/10/22 05:23:32]

Please add this:

bus_->AssertOnDBusThread();

[Haruki Sato, 2012/10/24 08:28:05]

Done.
Thanks.

+  if (match_rules_.find(match_rule) == match_rules_.end()) {

[satorux1, 2012/10/22 05:23:32]

maybe:

if (match_rules_.find(match_rule) == match_rules_.end())
  return true;

This will save one-indentation level.

[Haruki Sato, 2012/10/24 08:28:05]

Done.
Thanks!

+    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);
+    }
+  }
+  return true;
+}
+
+bool ObjectProxy::AddMatchRuleAndCallback(std::string match_rule,

[satorux1, 2012/10/22 05:23:32]

And -> With?

[Haruki Sato, 2012/10/24 08:28:05]

Done.

+                                          std::string absolute_signal_name,
+                                          SignalCallback signal_callback) {

[satorux1, 2012/10/22 05:23:32]

Please add this:

bus_->AssertOnDBusThread();

[Haruki Sato, 2012/10/24 08:28:05]

Done.

+  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::UpdateNameOwner() {

[satorux1, 2012/10/22 05:23:32]

Please add this:

bus_->AssertOnDBusThread();

[Haruki Sato, 2012/10/24 08:28:05]

Done.

+  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 false;
+  }
+
+  DBusMessage* response_message = bus_->SendWithReplyAndBlock(
+      get_name_owner_call.raw_message(), 1000, error.get());

[satorux1, 2012/10/22 05:23:32]

what's 1000? please define a constant.

[Haruki Sato, 2012/10/24 08:28:05]

Done.
Thanks.

+  if (!response_message) {
+    LOG(ERROR) << "Failed to get name owner. Got " << error.name() << ": " <<
+        error.message();
+    return false;
+  }
+  scoped_ptr<Response> response(Response::FromRawMessage(response_message));
+  MessageReader reader(response.get());
+  reader.PopString(&service_name_owner_);

[satorux1, 2012/10/22 05:23:32]

It's not guaranteed that PopString() succeeds. Please check the return value:

return reader.PopString(&service_name_owner_);

[Haruki Sato, 2012/10/24 08:28:05]

Done.

+  return true;
+}
+
+DBusHandlerResult ObjectProxy::HandleNameOwnerChanged(Signal* signal) {

[satorux1, 2012/10/22 05:23:32]

Please put:

bus_->AssertOnDBusThread();
DCHECK(signal);

[Haruki Sato, 2012/10/24 08:28:05]

Done.

+  // 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;
+    reader.PopString(&name);
+    reader.PopString(&old_owner);
+    reader.PopString(&new_owner);

[satorux1, 2012/10/22 05:23:32]

Please check return value of PopString()

[Haruki Sato, 2012/10/24 08:28:05]

Done.

+    if (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