DevTools: allow devtools front-end to show certificate viewer.

chrome/browser/devtools/devtools_ui_bindings.cc

Index: chrome/browser/devtools/devtools_ui_bindings.cc
diff --git a/chrome/browser/devtools/devtools_ui_bindings.cc b/chrome/browser/devtools/devtools_ui_bindings.cc
index 77c639009ecbfdf5b85244e6b23474c6ae5801f3..fb7bd4808aa5c270d744831d9a13a9fcdc4b9059 100644
--- a/chrome/browser/devtools/devtools_ui_bindings.cc
+++ b/chrome/browser/devtools/devtools_ui_bindings.cc
@@ -41,12 +41,14 @@
 #include "components/prefs/scoped_user_pref_update.h"
 #include "components/syncable_prefs/pref_service_syncable.h"
 #include "components/zoom/page_zoom.h"
+#include "content/public/browser/cert_store.h"
 #include "content/public/browser/devtools_external_agent_proxy.h"
 #include "content/public/browser/devtools_external_agent_proxy_delegate.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_contents.h"
@@ -58,6 +60,7 @@
 #include "ipc/ipc_channel.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
+#include "net/cert/x509_certificate.h"
 #include "net/http/http_response_headers.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_fetcher_response_writer.h"
@@ -675,6 +678,36 @@ void DevToolsUIBindings::SetWhitelistedShortcuts(const std::string& message) {
   delegate_->SetWhitelistedShortcuts(message);
 }
 
+void DevToolsUIBindings::ShowCertificateViewer(const std::string& cert_chain) {
+  std::unique_ptr<base::Value> value =
+      base::JSONReader::Read(cert_chain);
+  if (!value || value->GetType() != base::Value::TYPE_LIST)
+    return;
+
+  std::unique_ptr<base::ListValue> list =
+      base::ListValue::From(std::move(value));
+  std::vector<base::StringPiece> cert_string_piece;
+  for (size_t i = 0; i < list->GetSize(); ++i) {
+    std::string item;
+    if (list->GetString(i, &item))
+      cert_string_piece.push_back(item);

[Ryan Sleevi, 2016/09/02 18:20:30]

SECURITY BUG: This is a use-after-free. You're creating a StringPiece from a
string that you deallocate each iteration in this loop, invalidating the memory
the StringPiece is pointing to. You need to keep the actual string alive for as
long as you keep the stringpiece alive.

[pfeldman, 2016/09/02 18:54:43]

Good catch. I did not even pay attention to the fact that it used string pieces
as I was taking code from jam@'s context. He was using it right, I was not.
thanks.

+  }
+  scoped_refptr<net::X509Certificate> cert =
+       net::X509Certificate::CreateFromDERCertChain(cert_string_piece);
+  DCHECK(cert);

[dgozman, 2016/09/02 18:12:55]

if (!cert) return;

[Ryan Sleevi, 2016/09/02 18:20:30]

SECURITY BUG: Why this DCHECK()? You're taking untrusted input that may be a
completely invalid certificate, and potentially causing StoreCert to do a NULL
de-ref.

Because this is the point untrusted input is ingested, you likely need to decide
to either gracefully handle it (if (!cert)), or signal a fatal abort
(CHECK(cert))

[pfeldman, 2016/09/02 18:54:43]

Done.

+
+  // TODO(jam): temporarily add the certificate to the cert store to get an ID
+  // so that we don't have to change the WCD method signature
+  // (will be done in followups).
+  if (!agent_host_ || !agent_host_->GetWebContents())
+    return;
+  content::WebContents* inspected_wc = agent_host_->GetWebContents();
+  int cert_id = content::CertStore::GetInstance()->StoreCert(

[pfeldman, 2016/09/02 18:03:30]

This is temporary, John removes it in a follow up. We are putting certificate
back just to get the ID. I'm using that renderer's process ID as I'm storing the
certificate.

This code will be replaced with the call on the viewer that would simply view
x509 certs, would not touch the store. That would enable us to support remote
debugging scenarios.

+      cert.get(), inspected_wc->GetRenderProcessHost()->GetID());

[Ryan Sleevi, 2016/09/02 18:20:30]

Is it guaranteed that inspected_wc will have an RPH? Is there any
TOCTOU/lifetime issues here?

[pfeldman, 2016/09/02 18:54:43]

It is not. Done.

[Ryan Sleevi, 2016/09/02 19:27:40]

Apologies for not being clearer. The TOCTOU issue was wonderning if
GetWebContents()' value can change (between lines 702 and 704)

The issue with RPH was a null check - that is, if rph can be NULL (if a
WebContents doesn't have an RPH), then GetID() would be a null-deref. If the
timing is fine, then I think you'd want line 705 to be

if (!agent_host_ || !agent_host_->GetWebContents() ||
!agent_host_->GetWebContents()->GetRenderProcessHost())
  return;

+  web_contents_->GetDelegate()->ShowCertificateViewerInDevTools(
+      web_contents_, cert_id);
+}
+
 void DevToolsUIBindings::ZoomIn() {
   zoom::PageZoom::Zoom(web_contents(), content::PAGE_ZOOM_IN);
 }