Implementation for chrome.certificateProvider.requestPin/stopPinRequest

chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.cc

 // Copyright 2015 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 "chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.h"
 
 #include <memory>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/singleton.h"
+#include "base/rand_util.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/certificate_provider/certificate_provider_service.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
 #include "chrome/common/extensions/api/certificate_provider.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "extensions/browser/event_listener_map.h"
 #include "extensions/browser/event_router.h"
 #include "extensions/browser/event_router_factory.h"
 #include "extensions/browser/extension_event_histogram_value.h"
 #include "extensions/browser/extension_registry.h"
@@ skipping 78 matching lines @@
     const std::string& extension_id,
     int request_id,
     net::SSLPrivateKey::Hash hash,
     const scoped_refptr<net::X509Certificate>& certificate,
     const std::string& digest) {
   const std::string event_name(api_cp::OnSignDigestRequested::kEventName);
   if (!event_router_->ExtensionHasEventListener(extension_id, event_name))
     return false;
 
   api_cp::SignRequest request;
+  // Generate a random request id so that a malicious extension can not guess a
+  // valid id and spam the user.
+  uint64_t sign_request_id = base::RandUint64();

[emaxx, 2016/09/06 15:02:10]

I'm afraid we can put the client extensions into a bad situation by generating
the number that cannot be stored as a JavaScript number precisely.

My understanding is that the JavaScript number can store precisely 53-bit
integers, but I may be wrong here (and in any way hardcoding this here would be
quite strange).

So maybe, for simplicity reasons, we should generate the numbers that fit into
the usual 4-byte int range.

[emaxx, 2016/09/06 15:02:10]

There is already another "sign_request_id" in the existing code that is
generated by CertificateProviderService and passed to this function - here it is
named as "request_id". Can we reuse it instead of introducing a new one
identifier?

There is one difference between them: the existing ids are generated simply by
increasing a counter instead of the random used here (see the generation code
here:
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/certificate_prov...
). But I think it should be fine to modify the SignRequests class implementation
accordingly.

[igorcov, 2016/09/07 09:12:28]

Done.

[igorcov, 2016/09/07 09:12:28]

Done.

+  // If we hit the improbable case when generated ID exists already, we
+  // generate another ID.
+  while (!service_->pin_dialog_manager()->AddSignRequestId(sign_request_id)) {
+    sign_request_id = base::RandUint64();
+  }
+  request.sign_request_id = sign_request_id;
   switch (hash) {
     case net::SSLPrivateKey::Hash::MD5_SHA1:
       request.hash = api_cp::HASH_MD5_SHA1;
       break;
     case net::SSLPrivateKey::Hash::SHA1:
       request.hash = api_cp::HASH_SHA1;
       break;
     case net::SSLPrivateKey::Hash::SHA256:
       request.hash = api_cp::HASH_SHA256;
       break;
@@ skipping 70 matching lines @@
     content::BrowserContext* context) const {
   CertificateProviderService* const service = new CertificateProviderService();
   service->SetDelegate(base::WrapUnique(new DefaultDelegate(
       service,
       extensions::ExtensionRegistryFactory::GetForBrowserContext(context),
       extensions::EventRouterFactory::GetForBrowserContext(context))));
   return service;
 }
 
 }  // namespace chromeos