Add support for GCM subtypes to desktop Instance ID implementation

components/gcm_driver/gcm_client_impl.cc

Index: components/gcm_driver/gcm_client_impl.cc
diff --git a/components/gcm_driver/gcm_client_impl.cc b/components/gcm_driver/gcm_client_impl.cc
index e5c238dad562ce68743bff49b7c935a4732c05d6..2cd9f6e344ac916751a3e692ded4ae2692fdbf12 100644
--- a/components/gcm_driver/gcm_client_impl.cc
+++ b/components/gcm_driver/gcm_client_impl.cc
@@ -86,6 +86,7 @@ const char kMessageTypeDeletedMessagesKey[] = "deleted_messages";
 const char kMessageTypeKey[] = "message_type";
 const char kMessageTypeSendErrorKey[] = "send_error";
 const char kSendErrorMessageIdKey[] = "google.message_id";
+const char kSubtypeKey[] = "subtype";
 const char kSendMessageFromValue[] = "gcm@chrome.com";
 const int64_t kDefaultUserSerialNumber = 0LL;
 const int kDestroyGCMStoreDelayMS = 5 * 60 * 1000;  // 5 minutes.
@@ -238,6 +239,21 @@ void RecordResetStoreErrorToUMA(ResetStoreError error) {
   UMA_HISTOGRAM_ENUMERATION("GCM.ResetStore", error, RESET_STORE_ERROR_COUNT);
 }
 
+// Helper method for DCHECKs that prevent requesting the same token both with
+// and without subtypes.
+bool InstanceIDsDifferOnlyInUseSubtype(
+    const RegistrationInfo* registration_info_a,
+    const RegistrationInfo* registration_info_b) {
+  const InstanceIDTokenInfo* token_a =
+      InstanceIDTokenInfo::FromRegistrationInfo(registration_info_a);
+  const InstanceIDTokenInfo* token_b =
+      InstanceIDTokenInfo::FromRegistrationInfo(registration_info_b);
+  return token_a && token_b && token_a->use_subtype != token_b->use_subtype &&
+         token_a->app_id == token_b->app_id &&
+         token_a->authorized_entity == token_b->authorized_entity &&
+         token_a->scope == token_b->scope;
+}
+
 }  // namespace
 
 GCMInternalsBuilder::GCMInternalsBuilder() {}
@@ -665,6 +681,23 @@ void GCMClientImpl::GetInstanceIDData(const std::string& app_id,
   *extra_data = iter->second.second;
 }
 
+bool GCMClientImpl::CategoryHasSubtype(const std::string& category) {
+  // SECURITY NOTE: Subtypes that we receive from GCM *cannot* be trusted for
+  // registrations without a subtype (as the sender can pass any subtype they
+  // want). They can however be trusted for registrations that are known to have
+  // a subtype (as GCM overwrites anything passed by the sender).
+  //
+  // So a given Chrome profile always passes the same string (e.g.
+  // "com.chrome.stable.macosx") as the category when registering with a
+  // subtypes, and incoming subtypes are only trusted for that category.
+  //
+  // (On Android, all registrations made by Chrome on behalf of third-party
+  // apps/extensions/websites have always had a subtype, so we don't need such a
+  // check, indeed we cannot, since category is fixed to the true package name).
+
+  return category == chrome_build_info_.category_for_subtypes;
+}
+
 void GCMClientImpl::AddHeartbeatInterval(const std::string& scope,
                                          int interval_ms) {
   DCHECK(mcs_client_);
@@ -850,6 +883,11 @@ void GCMClientImpl::Register(
     const linked_ptr<RegistrationInfo>& registration_info) {
   DCHECK_EQ(state_, READY);
 
+  // Registrations should never pass as an app_id the special category used
+  // internally when registering with a subtype. See security note in
+  // GCMClient::CategoryHasSubtype.
+  CHECK(!CategoryHasSubtype(registration_info->app_id));
+
   // Find and use the cached registration ID.
   RegistrationInfoMap::const_iterator registrations_iter =
       registrations_.find(registration_info);
@@ -870,6 +908,11 @@ void GCMClientImpl::Register(
               cached_gcm_registration_info->sender_ids) {
         matched = false;
       }
+    } else {
+      DCHECK(!InstanceIDsDifferOnlyInUseSubtype(
+                  registration_info.get(), registrations_iter->first.get()))
+          << "Registering the same Instance ID token both with and without "
+             "subtypes is not supported";
     }
 
     if (matched) {
@@ -917,9 +960,10 @@ void GCMClientImpl::Register(
   }
 
   RegistrationRequest::RequestInfo request_info(
-      device_checkin_info_.android_id,
-      device_checkin_info_.secret,
-      registration_info->app_id);
+      device_checkin_info_.android_id, device_checkin_info_.secret,
+      registration_info->app_id,
+      instance_id_token_info && instance_id_token_info->use_subtype,

[Peter Beverloo, 2016/07/22 12:17:03]

nit: extract to a local bool w/ an appropriate name

[johnme, 2016/07/26 17:11:55]

Done.

+      chrome_build_info_.category_for_subtypes);
 
   std::unique_ptr<RegistrationRequest> registration_request(
       new RegistrationRequest(
@@ -1002,6 +1046,7 @@ void GCMClientImpl::Unregister(
       NOTREACHED();
       return;
     }
+
     request_handler.reset(new InstanceIDDeleteTokenRequestHandler(
         instance_id_iter->second.first,
         instance_id_token_info->authorized_entity,
@@ -1049,6 +1094,8 @@ void GCMClientImpl::Unregister(
       delegate_->OnUnregisterFinished(registration_info, INVALID_PARAMETER);
       return;
     }
+    DCHECK(!InstanceIDsDifferOnlyInUseSubtype(registration_info.get(),
+                                              iter->first.get()));
     registrations_.erase(iter);
 
     gcm_store_->RemoveRegistration(
@@ -1058,9 +1105,10 @@ void GCMClientImpl::Unregister(
   }
 
   UnregistrationRequest::RequestInfo request_info(
-      device_checkin_info_.android_id,
-      device_checkin_info_.secret,
-      registration_info->app_id);
+      device_checkin_info_.android_id, device_checkin_info_.secret,
+      registration_info->app_id,
+      instance_id_token_info && instance_id_token_info->use_subtype,

[Peter Beverloo, 2016/07/22 12:17:03]

nit: extract to a local bool w/ an appropriate name

[johnme, 2016/07/26 17:11:55]

Done.

+      chrome_build_info_.category_for_subtypes);
 
   std::unique_ptr<UnregistrationRequest> unregistration_request(
       new UnregistrationRequest(
@@ -1260,49 +1308,62 @@ void GCMClientImpl::HandleIncomingMessage(const gcm::MCSMessage& message) {
           message.GetProtobuf());
   DCHECK_EQ(data_message_stanza.device_user_id(), kDefaultUserSerialNumber);
 
-  // Copying all the data from the stanza to a MessageData object. When present,
-  // keys like kMessageTypeKey or kSendErrorMessageIdKey will be filtered out
-  // later.
+  // Copy all the data from the stanza to a MessageData object. When present,
+  // keys like kSubtypeKey, kMessageTypeKey or kSendErrorMessageIdKey will be
+  // filtered out later.
   MessageData message_data;
   for (int i = 0; i < data_message_stanza.app_data_size(); ++i) {
     std::string key = data_message_stanza.app_data(i).key();
     message_data[key] = data_message_stanza.app_data(i).value();
   }
 
+  std::string subtype = "";

[Peter Beverloo, 2016/07/22 12:17:03]

nit: no init

[johnme, 2016/07/26 17:11:55]

Done.

+  auto subtype_iter = message_data.find(kSubtypeKey);

[Peter Beverloo, 2016/07/22 12:17:03]

This is using "subtype", which is potentially untrusted, while we agreed to
start using "gcm.subtype", which is trusted. This is not yet supported by the
server, though, so we should probably keep this code, but include a TODO for
simply deferring to the "gcm.subtype" value when present?

[johnme, 2016/07/26 17:11:55]

Added TODO.

+  if (subtype_iter != message_data.end()) {
+    subtype = subtype_iter->second;
+    message_data.erase(subtype_iter);
+  }
+
+  // If CategoryHasSubtype returns false, the subtype is untrusted input set by
+  // the sender, and we should ignore it.
+  bool use_subtype =
+      !subtype.empty() && CategoryHasSubtype(data_message_stanza.category());
+  std::string app_id = use_subtype ? subtype : data_message_stanza.category();
+
   MessageType message_type = DATA_MESSAGE;
-  MessageData::iterator iter = message_data.find(kMessageTypeKey);
-  if (iter != message_data.end()) {
-    message_type = DecodeMessageType(iter->second);
-    message_data.erase(iter);
+  MessageData::iterator type_iter = message_data.find(kMessageTypeKey);
+  if (type_iter != message_data.end()) {
+    message_type = DecodeMessageType(type_iter->second);
+    message_data.erase(type_iter);
   }
 
   switch (message_type) {
     case DATA_MESSAGE:
-      HandleIncomingDataMessage(data_message_stanza, message_data);
+      HandleIncomingDataMessage(app_id, use_subtype, data_message_stanza,
+                                message_data);
       break;
     case DELETED_MESSAGES:
-      recorder_.RecordDataMessageReceived(data_message_stanza.category(),
-                                          data_message_stanza.from(),
-                                          data_message_stanza.ByteSize(),
-                                          true,
+      recorder_.RecordDataMessageReceived(app_id, data_message_stanza.from(),
+                                          data_message_stanza.ByteSize(), true,
                                           GCMStatsRecorder::DELETED_MESSAGES);
-      delegate_->OnMessagesDeleted(data_message_stanza.category());
+      delegate_->OnMessagesDeleted(app_id);
       break;
     case SEND_ERROR:
-      HandleIncomingSendError(data_message_stanza, message_data);
+      HandleIncomingSendError(app_id, data_message_stanza, message_data);
       break;
     case UNKNOWN:
     default:  // Treat default the same as UNKNOWN.
       DVLOG(1) << "Unknown message_type received. Message ignored. "
-               << "App ID: " << data_message_stanza.category() << ".";
+               << "App ID: " << app_id << ".";
       break;
   }
 }
 
 void GCMClientImpl::HandleIncomingDataMessage(
+    const std::string& app_id,
+    bool use_subtype,
     const mcs_proto::DataMessageStanza& data_message_stanza,
     MessageData& message_data) {
-  std::string app_id = data_message_stanza.category();
   std::string sender = data_message_stanza.from();
 
   // Drop the message when the app is not registered for the sender of the
@@ -1329,15 +1390,23 @@ void GCMClientImpl::HandleIncomingDataMessage(
 
   // Then, find among all InstanceID registrations.
   if (!registered) {
-    std::unique_ptr<InstanceIDTokenInfo> instance_id_token(
-        new InstanceIDTokenInfo);
+    linked_ptr<InstanceIDTokenInfo> instance_id_token(new InstanceIDTokenInfo);
     instance_id_token->app_id = app_id;
+    instance_id_token->use_subtype = use_subtype;
     instance_id_token->authorized_entity = sender;
     instance_id_token->scope = kGCMScope;
-    auto instance_id_token_iter = registrations_.find(
-        make_linked_ptr<RegistrationInfo>(instance_id_token.release()));
-    if (instance_id_token_iter != registrations_.end())
-      registered = true;
+
+    auto instance_id_token_iter = registrations_.find(instance_id_token);
+    if (instance_id_token_iter != registrations_.end()) {
+      if (InstanceIDsDifferOnlyInUseSubtype(
+              instance_id_token.get(), instance_id_token_iter->first.get())) {
+        DLOG(ERROR) << "Incoming GCM message for " << app_id

[Peter Beverloo, 2016/07/22 12:17:03]

Is this DLOG necessary? It would only happen when the server sends an invalid
message to a debug build of Chrome, so (a) how would anyone ever see this
message, and (b) what are they supposed to do when it happens?

[johnme, 2016/07/26 17:11:55]

a) it's most likely to happen if the value of use_subtype is incorrect when we
register (or if the GCM server swaps the fields round, but that's very
unlikely).

b) hopefully file a bug?

+                    << " had use_subtype = " << use_subtype
+                    << " which conflicts with stored registration";
+      } else {
+        registered = true;
+      }
+    }
   }
 
   recorder_.RecordDataMessageReceived(app_id, sender,
@@ -1357,6 +1426,7 @@ void GCMClientImpl::HandleIncomingDataMessage(
 }
 
 void GCMClientImpl::HandleIncomingSendError(
+    const std::string& app_id,
     const mcs_proto::DataMessageStanza& data_message_stanza,
     MessageData& message_data) {
   SendErrorDetails send_error_details;
@@ -1370,12 +1440,9 @@ void GCMClientImpl::HandleIncomingSendError(
     send_error_details.additional_data.erase(iter);
   }
 
-  recorder_.RecordIncomingSendError(
-      data_message_stanza.category(),
-      data_message_stanza.to(),
-      data_message_stanza.id());
-  delegate_->OnMessageSendError(data_message_stanza.category(),
-                                send_error_details);
+  recorder_.RecordIncomingSendError(app_id, data_message_stanza.to(),
+                                    data_message_stanza.id());
+  delegate_->OnMessageSendError(app_id, send_error_details);
 }
 
 bool GCMClientImpl::HasStandaloneRegisteredApp() const {