Add sender verification of D-Bus signals.

dbus/object_proxy.cc

Index: dbus/object_proxy.cc
diff --git a/dbus/object_proxy.cc b/dbus/object_proxy.cc
index ba51efaf61ac0305eea09dd55d788010feea049f..4202aa9312067b412c59ae2517851000a4b9957a 100644
--- a/dbus/object_proxy.cc
+++ b/dbus/object_proxy.cc
@@ -24,6 +24,12 @@ 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.
@@ -37,6 +43,12 @@ std::string GetAbsoluteSignalName(
 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 {
@@ -362,25 +374,26 @@ 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 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.
@@ -421,9 +434,28 @@ 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;
   }
 
+  // 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();
 
@@ -437,6 +469,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 +554,160 @@ 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;
+}
+
+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