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;
 
+// The path of D-Bus Object sending NameOwnerChanged signal.
+const char kDbusSystemObjectPath[] = "/org/freedesktop/DBus";
+
+// Timeout in milliseconds used for GetNameOwner method call.
+const int kGetNameOwnerCallTimeoutMs = 1000;
+
 // 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().
 void EmptyResponseCallbackBody(dbus::Response* unused_response) {
 }
 
+// Creates and returns a copy of the pointed Signal instance.
+dbus::Signal* CloneSignalMessage(dbus::Signal* signal) {
+  DBusMessage* clone = dbus_message_copy(signal->raw_message());
+  return dbus::Signal::FromRawMessage(clone);
+}
+
 }  // namespace
 
 namespace dbus {
 
 ObjectProxy::ObjectProxy(Bus* bus,
                          const std::string& service_name,
                          const ObjectPath& object_path,
                          int options)
     : bus_(bus),
       service_name_(service_name),
@@ skipping 305 matching lines @@
         filter_added_ = true;
       } else {
         LOG(ERROR) << "Failed to add filter function";
       }
     }
     // 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;
+    // 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 latest name owner. No pending signal.
+    // Passing NULL as the pending signal so that it only call GetNameOwner to
+    // update |service_name_owner_|.

[satorux1, 2012/10/25 08:56:23]

We might want to add:

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.

[Haruki Sato, 2012/10/26 05:03:24]

Done.

+    UpdateNameOwnerAsync(scoped_ptr<Signal>(NULL));
   }
 
   // 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.value() == kDbusSystemObjectPath &&
+        signal->GetMember() == "NameOwnerChanged") {
+      // Handle NameOwnerChanged separately
+      return HandleNameOwnerChanged(signal.get());
+    }
     return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
   }
 
+  // If the name owner is not known yet, get it before verifying the sender.
+  if (service_name_owner_.empty()) {
+    return UpdateNameOwnerAsync(signal.Pass());
+  }

[satorux1, 2012/10/25 08:56:23]

I just realized that this would be unnecessary

1) if we are lucky, we can get the name owner when connecting to a signal
2) otherwise, we'll receive the NameOwnerChanged as soon as the service is
started

If my understanding is correct, we can significantly simplify the code here.
What do you think?

[Haruki Sato, 2012/10/26 05:03:24]

Discussed offline.
I confirmed that 
There's still very small timeframe where the owner of the name can send signal
while ObjectProxy has not yet GetNameOwner, but that's very unlikely and happens
only in browser's initialization. The services should not send such signal at
that stage.

Restored sync version of UpdateNameOwner
and removed tricky signal handling.

Thank you for the advice!

+
+  // Verify the sender, check if we know about the signal, and invoke the
+  // callback if any.
+  return VerifySenderAndDispatch(signal.Pass());
+}
+
+DBusHandlerResult ObjectProxy::VerifySenderAndDispatch(
+    scoped_ptr<dbus::Signal> signal) {
+  DCHECK(signal);
+
   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_ != 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::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;
+}
+
+DBusHandlerResult ObjectProxy::UpdateNameOwnerAsync(
+    scoped_ptr<Signal> pending_signal) {
+  bus_->AssertOnDBusThread();
+
+  scoped_ptr<MethodCall> get_name_owner_call(
+      new MethodCall("org.freedesktop.DBus", "GetNameOwner"));
+  MessageWriter writer(get_name_owner_call.get());
+  writer.AppendString(service_name_);
+  VLOG(1) << "Method call: " << get_name_owner_call->ToString();
+
+  const dbus::ObjectPath object_path("/org/freedesktop/DBus");
+  ScopedDBusError error;
+  if (!get_name_owner_call->SetDestination("org.freedesktop.DBus") ||
+      !get_name_owner_call->SetPath(object_path)) {
+    LOG(ERROR) << "Failed to get name owner.";
+    return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+  }
+
+  Signal* clone = NULL;
+  if (pending_signal) {
+    // This depends on the incoming reply to GetNameOwner method call.
+    // Copy the pending signal and delete the original in order to free
+    // libdbus's messaging queue and let it accept the next incoming message.
+    // The limitation of the queue size seems to be the one described in
+    // ObjectProxy::RunResponseCallback()
+    clone = CloneSignalMessage(pending_signal.get());
+    pending_signal.reset();
+  }
+  DBusMessage* request_message = get_name_owner_call->raw_message();
+  dbus_message_ref(request_message);
+
+  const base::TimeTicks start_time = base::TimeTicks::Now();
+  StartAsyncMethodCall(
+      kGetNameOwnerCallTimeoutMs,
+      request_message,
+      base::Bind(&ObjectProxy::OnGetNameOwner, this, clone),
+      base::Bind(&ObjectProxy::OnGetNameOwnerError, this, clone),
+      start_time);
+  return DBUS_HANDLER_RESULT_HANDLED;
+}
+
+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;
+}
+
+void ObjectProxy::OnGetNameOwner(Signal* pending_signal, Response* response) {
+  bus_->PostTaskToDBusThread(
+      FROM_HERE,
+      base::Bind(&ObjectProxy::OnGetNameOwnerOnDbusThread,
+                 this,
+                 pending_signal,
+                 response));
+}
+
+void ObjectProxy::OnGetNameOwnerOnDbusThread(
+    Signal* pending_signal, Response* response) {
+  bus_->AssertOnDBusThread();
+
+  MessageReader reader(response);
+  if (!reader.PopString(&service_name_owner_)) {
+    LOG(ERROR) << "Can not interpret the result of GetNameOwner: "
+               << response->ToString();
+  }
+
+  if (pending_signal)
+    VerifySenderAndDispatch(scoped_ptr<Signal>(pending_signal));
+}
+
+void ObjectProxy::OnGetNameOwnerError(
+    Signal* pending_signal, ErrorResponse* response) {
+  bus_->PostTaskToDBusThread(
+      FROM_HERE,
+      base::Bind(&ObjectProxy::OnGetNameOwnerErrorOnDbusThread,
+                 this,
+                 pending_signal,
+                 response));
+}
+
+void ObjectProxy::OnGetNameOwnerErrorOnDbusThread(
+    Signal* pending_signal, ErrorResponse* response) {
+  bus_->AssertOnDBusThread();
+
+  if (pending_signal)
+    VerifySenderAndDispatch(scoped_ptr<Signal>(pending_signal));
+}
+
 }  // namespace dbus