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..85891ac5a0e201c2ac01a6df467305f9928c0fdf 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";

[satorux1, 2012/10/25 01:55:16]

nit: please do:

const char kDbusSystemObjectPath[] = "/org/freedesktop/DBus";

[Haruki Sato, 2012/10/25 07:29:52]

Done.
Thanks.

+
+// Timeout in milliseconds used for GetNameOwner method call.
+const static int kGetNameOwnerCallTimeoutMs = 1000;

[satorux1, 2012/10/25 01:55:16]

remove static.

[Haruki Sato, 2012/10/25 07:29:52]

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.
@@ -37,6 +43,11 @@ std::string GetAbsoluteSignalName(
 void EmptyResponseCallbackBody(dbus::Response* unused_response) {
 }
 
+dbus::Signal* CloneSignalMessage(dbus::Signal* orig) {

[satorux1, 2012/10/25 01:55:16]

function comment is missing.

orig -> signal ?

[Haruki Sato, 2012/10/25 07:29:52]

Done.
Thanks.

+  DBusMessage* clone = dbus_message_copy(orig->raw_message());
+  return dbus::Signal::FromRawMessage(clone);
+}
+
 }  // namespace
 
 namespace dbus {
@@ -362,25 +373,24 @@ 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.

[satorux1, 2012/10/25 01:55:16]

What does "no pending signal" mean? Please describe more.

[Haruki Sato, 2012/10/25 07:29:52]

Done.

+    UpdateNameOwnerAsync(scoped_ptr<Signal>(NULL));
   }
 
   // Run on_connected_callback in the origin thread.
@@ -421,9 +431,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());
+  }
+
+  // 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 +466,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 +551,157 @@ 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();

[satorux1, 2012/10/25 01:55:16]

please put a blank line here.

[Haruki Sato, 2012/10/25 07:29:52]

Done.
Thanks.

+  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 obj_path("/org/freedesktop/DBus");

[satorux1, 2012/10/25 01:55:16]

object_path in favor of less abbreviation.

[Haruki Sato, 2012/10/25 07:29:52]

Done.

+  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 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()
+    Signal* released_signal = pending_signal.release();
+    clone = CloneSignalMessage(released_signal);

[satorux1, 2012/10/25 01:55:16]

maybe:

clone =  CloneSignalMessage(pending_signal.get()) ?
pending_signal.reset();

[Haruki Sato, 2012/10/25 07:29:52]

Done.
Thanks!

+    delete released_signal;
+    bus_->ProcessAllIncomingDataIfAny();

[satorux1, 2012/10/25 01:55:16]

why do we need to call this? We rarely need to call this.

[Haruki Sato, 2012/10/25 07:29:52]

Ah! I somehow added this call here. Confirmed that this is unnecessary.
Removed.
Thanks!

+  }
+  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();

[satorux1, 2012/10/25 01:55:16]

put a blank line here.

[Haruki Sato, 2012/10/25 07:29:52]

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;
+    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* signal, Response* response) {

[satorux1, 2012/10/25 01:55:16]

signal -> pending_signal?

DCHECK(pending_signal);

[Haruki Sato, 2012/10/25 07:29:52]

Done.

+  bus_->PostTaskToDBusThread(
+      FROM_HERE,
+      base::Bind(&ObjectProxy::OnGetNameOwnerOnDbusThread,
+                 this,
+                 signal,
+                 response));
+}
+
+void ObjectProxy::OnGetNameOwnerOnDbusThread(
+    Signal* signal, Response* response) {

[satorux1, 2012/10/25 01:55:16]

ditto. 

[Haruki Sato, 2012/10/25 07:29:52]

Done.

+  bus_->AssertOnDBusThread();

[satorux1, 2012/10/25 01:55:16]

put a blank line here.

[Haruki Sato, 2012/10/25 07:29:52]

Done.

+  MessageReader reader(response);
+  if (!reader.PopString(&service_name_owner_)) {
+    LOG(ERROR) << "Can not interpret the result of GetNameOwner: " <<

[satorux1, 2012/10/25 01:55:16]

LOG(ERROR) << "Can not interpret the result of GetNameOwner: "
           << response->ToString();

[Haruki Sato, 2012/10/25 07:29:52]

Done.
Thanks.

+        response->ToString();
+  }
+
+  if (signal)

[satorux1, 2012/10/25 01:55:16]

signal should always be non-null here?

[Haruki Sato, 2012/10/25 07:29:52]

I am passing NULL as pending_signal in LINE 393,
where I intended to retrieve the name owner earlier in ConnectToSignalInternal.
Do you have any other idea, or think we don't need to try GetNameOwner in
ConnectToSignalInternal?

+    VerifySenderAndDispatch(scoped_ptr<Signal>(signal));
+}
+
+void ObjectProxy::OnGetNameOwnerError(Signal* signal, ErrorResponse* response) {
+  bus_->PostTaskToDBusThread(
+      FROM_HERE,
+      base::Bind(&ObjectProxy::OnGetNameOwnerErrorOnDbusThread,
+                 this,
+                 signal,
+                 response));
+}
+
+void ObjectProxy::OnGetNameOwnerErrorOnDbusThread(
+    Signal* signal, ErrorResponse* response) {
+  bus_->AssertOnDBusThread();
+  if (signal)

[satorux1, 2012/10/25 01:55:16]

ditto.

+    VerifySenderAndDispatch(scoped_ptr<Signal>(signal));
+}
+
 }  // namespace dbus