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

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

BUG=598073, 504347
Committed: https://crrev.com/c3b69f2871cbdcd8e74b0933b9cdaf765f1cf3c5
Cr-Commit-Position: refs/heads/master@{#416430}

[pfeldman, 2016-09-02 17:59:58]

pfeldman@chromium.org changed reviewers:
+ jam@chromium.org, tsepez@chromium.org

[pfeldman, 2016-09-02 18:01:12]

Thomas: this is for showing arbitrary certificate chains from the DevTools.

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

https://codereview.chromium.org/2302873005/diff/1/chrome/browser/devtools/dev...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/2302873005/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_ui_bindings.cc:705: int cert_id =
content::CertStore::GetInstance()->StoreCert(
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.

[pfeldman, 2016-09-02 18:05:12]

@john: to use this API, call
InspectorFrontendHost.showCertificateViewer(["certificate", "chain", "array"]);
from within the inspector. It does not go over the remote debugging protocol.

[Tom Sepez, 2016-09-02 18:06:25]

Description was changed from

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

BUG=598073, 504347
==========

to

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

BUG=598073, 504347
==========

[Tom Sepez, 2016-09-02 18:06:25]

tsepez@chromium.org changed reviewers:
+ agl@chromium.org, rsleevi@chromium.org

[Tom Sepez, 2016-09-02 18:07:20]

+sleevi, +agl

Do we have a precedent about allowing cert data into the renderer process?  Have
we previously isolated this to browser?

[jam, 2016-09-02 18:12:34]

Pavel: can this be implemented in content/?

On 2016/09/02 18:07:20, Tom Sepez wrote:
> +sleevi, +agl
> 
> Do we have a precedent about allowing cert data into the renderer process? 
Have
> we previously isolated this to browser?

Note it's not used for security checks. It's used only by devtools.

The change that this is part of, https://codereview.chromium.org/2296953004,
undoes trusting the renderer to specify which certificate (id) is used for a
page, and also the security style associated with pages.

[dgozman, 2016-09-02 18:12:54]

dgozman@chromium.org changed reviewers:
+ dgozman@chromium.org

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

devtools rs lgtm

https://codereview.chromium.org/2302873005/diff/1/chrome/browser/devtools/dev...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/2302873005/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_ui_bindings.cc:697: DCHECK(cert);
if (!cert) return;

[Ryan Sleevi, 2016-09-02 18:18:01]

rsleevi@chromium.org changed reviewers:
- dgozman@chromium.org

[Ryan Sleevi, 2016-09-02 18:18:01]

To spawn any certificate viewer UI, it needs to be hosted in the browser
process, because we use native UI, and it cannot be sandboxed (because it
integrates with all the various ways enterprises can and do extend and
customize, especially the US Gov & Edu sectors)

It would be useful to know what the follow-up CL mentioned by pfeldman is, since
I suspect that's where the real challenge is.

Historically, the flow has been:
- Browser process loads something from a TLS site
- Browser process stashes the cert in a CertStore (to keep memory and IPC
overhead as low as possible, as otherwise this could be 12-16K IPCs per-resource
loaded), and IPCs over the ID to the renderer
- When the renderer wants to show UI (e.g. View Frame Info, though removed), it
IPCs back to the browser a "Show this Cert ID" message
- The browser makes sure the CertID was assigned to that renderer (this isn't so
much a security check as a "Renderer refcounting" check), looks up the original
cert based on the ID, and shows it

From what I understand, DevTools is run in a renderer separate than the browsing
context, so this was already 'weird' (DevTools says "Look up this ID", and as
long as the original renderer was still alive, the reference was alive, and it'd
Just Work)

That works fine for local debugging, but if you have remote debugging (e.g. no
local renderer process), there's no cert_id that DevTools can reference. From
what I understand from pfeldman@'s CL, this adds a way for DevTools that, if it
has the full certificate, it wants to be able to create a CertID (by storing the
cert), so then it can pop the UI for remote debugging.

pfeldman@, jam@: Is this correct? Can you provide any additional context?


If so, it's not fundamentally wrong. The biggest concerns here have been
overhead - while changing to passing the actual certs around (to renderers and
to devtools) is arguably clearer and More Correct, it comes with the significant
IPC overhead (12-16K / resource for the full chain), and so in general, it's not
appropriate. For something like DevTools, it "should" be fine, and the browser
must already be hardened in its certificate processing code.

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

https://codereview.chromium.org/2302873005/diff/1/chrome/browser/devtools/dev...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/2302873005/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_ui_bindings.cc:693:
cert_string_piece.push_back(item);
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.

https://codereview.chromium.org/2302873005/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_ui_bindings.cc:697: DCHECK(cert);
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))

https://codereview.chromium.org/2302873005/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_ui_bindings.cc:706: cert.get(),
inspected_wc->GetRenderProcessHost()->GetID());
Is it guaranteed that inspected_wc will have an RPH? Is there any
TOCTOU/lifetime issues here?

[pfeldman, 2016-09-02 18:34:15]

On 2016/09/02 18:18:01, Ryan Sleevi (slow) wrote:
> To spawn any certificate viewer UI, it needs to be hosted in the browser
> process, because we use native UI, and it cannot be sandboxed (because it
> integrates with all the various ways enterprises can and do extend and
> customize, especially the US Gov & Edu sectors)
> 
> It would be useful to know what the follow-up CL mentioned by pfeldman is,
since
> I suspect that's where the real challenge is.
> 

I'll defer this to jam@.

> Historically, the flow has been:
> - Browser process loads something from a TLS site
> - Browser process stashes the cert in a CertStore (to keep memory and IPC
> overhead as low as possible, as otherwise this could be 12-16K IPCs
per-resource
> loaded), and IPCs over the ID to the renderer
> - When the renderer wants to show UI (e.g. View Frame Info, though removed),
it
> IPCs back to the browser a "Show this Cert ID" message
> - The browser makes sure the CertID was assigned to that renderer (this isn't
so
> much a security check as a "Renderer refcounting" check), looks up the
original
> cert based on the ID, and shows it
> 
> From what I understand, DevTools is run in a renderer separate than the
browsing
> context, so this was already 'weird' (DevTools says "Look up this ID", and as
> long as the original renderer was still alive, the reference was alive, and
it'd
> Just Work)
> 
> That works fine for local debugging, but if you have remote debugging (e.g. no
> local renderer process), there's no cert_id that DevTools can reference. From
> what I understand from pfeldman@'s CL, this adds a way for DevTools that, if
it
> has the full certificate, it wants to be able to create a CertID (by storing
the
> cert), so then it can pop the UI for remote debugging.
> 
> pfeldman@, jam@: Is this correct? Can you provide any additional context?

That's exactly right. jam@ has additional scenario where this is helpful that
was the motivation for the change.

> 
> 
> If so, it's not fundamentally wrong. The biggest concerns here have been
> overhead - while changing to passing the actual certs around (to renderers and
> to devtools) is arguably clearer and More Correct, it comes with the
significant
> IPC overhead (12-16K / resource for the full chain), and so in general, it's
not
> appropriate. For something like DevTools, it "should" be fine, and the browser
> must already be hardened in its certificate processing code.

That exactly matches our thought process.

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

https://codereview.chromium.org/2302873005/diff/1/chrome/browser/devtools/dev...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/2302873005/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_ui_bindings.cc:693:
cert_string_piece.push_back(item);
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.

https://codereview.chromium.org/2302873005/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_ui_bindings.cc:697: DCHECK(cert);
On 2016/09/02 18:20:30, Ryan Sleevi (slow) wrote:
> 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))

Done.

https://codereview.chromium.org/2302873005/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_ui_bindings.cc:706: cert.get(),
inspected_wc->GetRenderProcessHost()->GetID());
On 2016/09/02 18:20:30, Ryan Sleevi (slow) wrote:
> Is it guaranteed that inspected_wc will have an RPH? Is there any
> TOCTOU/lifetime issues here?

It is not. Done.

[jam, 2016-09-02 19:05:23]

On 2016/09/02 18:34:15, pfeldman wrote:
> On 2016/09/02 18:18:01, Ryan Sleevi (slow) wrote:
> > To spawn any certificate viewer UI, it needs to be hosted in the browser
> > process, because we use native UI, and it cannot be sandboxed (because it
> > integrates with all the various ways enterprises can and do extend and
> > customize, especially the US Gov & Edu sectors)
> > 
> > It would be useful to know what the follow-up CL mentioned by pfeldman is,
> since
> > I suspect that's where the real challenge is.
> > 
> 
> I'll defer this to jam@.

The cl is linked in comment 9, along with a brief description. That cl has a
more indepth explanation of the motivations (for both plznavigate and for
network service). let me know if anything is unclear and i'm happy to expand.

> 
> > Historically, the flow has been:
> > - Browser process loads something from a TLS site
> > - Browser process stashes the cert in a CertStore (to keep memory and IPC
> > overhead as low as possible, as otherwise this could be 12-16K IPCs
> per-resource
> > loaded), and IPCs over the ID to the renderer
> > - When the renderer wants to show UI (e.g. View Frame Info, though removed),
> it
> > IPCs back to the browser a "Show this Cert ID" message
> > - The browser makes sure the CertID was assigned to that renderer (this
isn't
> so
> > much a security check as a "Renderer refcounting" check), looks up the
> original
> > cert based on the ID, and shows it
> > 
> > From what I understand, DevTools is run in a renderer separate than the
> browsing
> > context, so this was already 'weird' (DevTools says "Look up this ID", and
as
> > long as the original renderer was still alive, the reference was alive, and
> it'd
> > Just Work)
> > 
> > That works fine for local debugging, but if you have remote debugging (e.g.
no
> > local renderer process), there's no cert_id that DevTools can reference.
From
> > what I understand from pfeldman@'s CL, this adds a way for DevTools that, if
> it
> > has the full certificate, it wants to be able to create a CertID (by storing
> the
> > cert), so then it can pop the UI for remote debugging.
> > 
> > pfeldman@, jam@: Is this correct? Can you provide any additional context?
> 
> That's exactly right. jam@ has additional scenario where this is helpful that
> was the motivation for the change.
> 
> > 
> > 
> > If so, it's not fundamentally wrong. The biggest concerns here have been
> > overhead - while changing to passing the actual certs around (to renderers
and
> > to devtools) is arguably clearer and More Correct, it comes with the
> significant
> > IPC overhead (12-16K / resource for the full chain), and so in general, it's
> not
> > appropriate. For something like DevTools, it "should" be fine, and the
browser
> > must already be hardened in its certificate processing code.
> 
> That exactly matches our thought process.

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

lgtm

https://codereview.chromium.org/2302873005/diff/1/chrome/browser/devtools/dev...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/2302873005/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_ui_bindings.cc:706: cert.get(),
inspected_wc->GetRenderProcessHost()->GetID());
On 2016/09/02 18:54:43, pfeldman wrote:
> On 2016/09/02 18:20:30, Ryan Sleevi (slow) wrote:
> > Is it guaranteed that inspected_wc will have an RPH? Is there any
> > TOCTOU/lifetime issues here?
> 
> It is not. Done.

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;

[pfeldman, 2016-09-02 19:35:26]

On 2016/09/02 19:27:40, Ryan Sleevi (slow) wrote:
> lgtm
> 
>
https://codereview.chromium.org/2302873005/diff/1/chrome/browser/devtools/dev...
> File chrome/browser/devtools/devtools_ui_bindings.cc (right):
> 
>
https://codereview.chromium.org/2302873005/diff/1/chrome/browser/devtools/dev...
> chrome/browser/devtools/devtools_ui_bindings.cc:706: cert.get(),
> inspected_wc->GetRenderProcessHost()->GetID());
> On 2016/09/02 18:54:43, pfeldman wrote:
> > On 2016/09/02 18:20:30, Ryan Sleevi (slow) wrote:
> > > Is it guaranteed that inspected_wc will have an RPH? Is there any
> > > TOCTOU/lifetime issues here?
> > 
> > It is not. Done.
> 
> 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;

The timing is fine, I was saving on characters when I was introducing "rph".

[pfeldman, 2016-09-02 19:37:01]

@tsepez: requesting the security stamp!

[Tom Sepez, 2016-09-02 20:19:04]

stamp LGTM

[pfeldman, 2016-09-02 20:20:10]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-09-02 20:20:11]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2302873005/#ps20001
(title: "review comments.")

[commit-bot: I haz the power, 2016-09-02 20:20:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2016-09-02 20:22:49]

lgtm

(Pavel explained in person that we only care about Chrome as devtools front end)

[commit-bot: I haz the power, 2016-09-02 23:46:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 23:46:06]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jam, 2016-09-02 23:51:23]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2016-09-02 23:51:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-03 02:51:13]

Description was changed from

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

BUG=598073, 504347
==========

to

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

BUG=598073, 504347
==========

[commit-bot: I haz the power, 2016-09-03 02:51:15]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-03 02:53:40]

Description was changed from

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

BUG=598073, 504347
==========

to

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

BUG=598073, 504347

Committed: https://crrev.com/c3b69f2871cbdcd8e74b0933b9cdaf765f1cf3c5
Cr-Commit-Position: refs/heads/master@{#416430}
==========

[commit-bot: I haz the power, 2016-09-03 02:53:41]

Patchset 2 (id:??) landed as
https://crrev.com/c3b69f2871cbdcd8e74b0933b9cdaf765f1cf3c5
Cr-Commit-Position: refs/heads/master@{#416430}