1116439 - Add a way to inspect full certificate chains from the security tab

Status: NEW

(DevTools :: Netmonitor, defect)

Description

[Sami Jaktholm]

Bug 932179 brings per-request security state to the Network Monitor which shows basic information about the certificate.

However, it'd be great if it were possible to inspect the entire chain of certificates with the built-in.

I think this can be achieved by getting the DER of the server side nsIX509Cert[1], transferring it to the client and then reconstructing the nsIX509Cert from the DER data[2].

[1] http://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/public/nsIX509Cert.idl#250
[2] http://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/public/nsIX509CertDB.idl#271

Comment 1

[Sami Jaktholm]

I've got this mostly working but I have a concern about the failedCertChain of nsITransportSecurityInfo.

It seems that the first certificate in that array contains the rest recursively via the .issuer field of the nsIX509Cert. Is this always the case? Is it possible that the certificates in failedCertChain nsIX509CertList are different from the chain you get from recursively looking at .issuer field of the first certificate in failedCertChain?

Thanks.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

nsIX509Cert.issuer works by calling nsIX509Cert.getChain(), which attempts to find a verified certificate chain. This has a number of issues (see bug 867473 and bug 1004580), but for the purposes of this bug the issue is that the verified chain as found by our certificate verification algorithm may not be the same as the one the server sent.

nsITransportSecurityInfo.failedCertChain will always contain the information we want to expose here (given no library failures) in the case where the connection failed and wasn't overridable.

What we need is for that information to always be available, regardless of if the connection failed or not. Unfortunately, due to bug 731478 and related bugs, that information won't always be available (e.g. when a connection is resumed). There may be ways to work around that (e.g. have some sort of connection peer cert chain cache in PSM), but it may just be best to fix that bug.

Comment 3

[Sami Jaktholm]

Thanks for the information. I'll try to work with that.

Comment 4

[Sami Jaktholm]

This turns out to be a little more complicated than I originally thought.

The view cert dialog takes an nsIX509Cert and uses .getChain() to get the chain of certificates it will show in the UI. In order to show the correct chain in case of failure following changes are required: 
1) Make a method like nsNSSDialogs::viewCert that takes an nsIArray of nsIX509Certs instead of a single nsIX509Cert
2) Make the dialog itself work with both single nsIX509Cert and an nsIArray of nsIX509Certs as argument.

I'm quite busy with other things right now so I'll attach work-in-progress patches to this bug and pick this up when I have more time.

Comment 5

[Sami Jaktholm]

Attachment: netmonitor-cert-chains-1-toolkit.patch

The debugger server changes required to show the certificate chains in netmonitor

The packet format probably needs to be changed from a single DER-encoded nsIX509Cert string to an array of strings, one for each certificate in the chain. Also, needs tests for failure cases.

Comment 6

[Sami Jaktholm]

Attachment: netmonitor-cert-chains-2-ui.patch

The frontend changes required to show the certificate chains in netmonitor.

The DER string -> nsIX509Cert conversion needs to be changed to accept an array of DER-encoded nsIX509Cert strings and it needs to call the correct nsICertificateDialogs viewCert method that will take an array of certificates.

Comment 7

[Sami Jaktholm]

Attachment: netmonitor-cert-chains-1-toolkit.patch

This patch contains toolkit/ changes required to show certificate chains in the network monitor. It adds an array of DER-encoded certificates as .chain member to the .cert object of security information.

For successful connections the chain is retrieved by calling nsIX509Cert.getChain(). For failures, the nsITransportSecurityInfo.failedCertChain is used.

Comment 8

[Sami Jaktholm]

Attachment: netmonitor-cert-chains-2-view-cert-chain.patch

This patch contains security/manager/pki changes required to pass a custom chain to the view cert dialog.

It adds a new method nsICertificateDialogs::viewCertChain (implemented by nsNSSDialogs::ViewCertChain) which works like nsICertificateDialogs::viewCert but takes an nsIArray instead of nsIX509Cert. It also makes the viewCertDetails.js to detect if it was given a certificate or a chain and act accordingly.

I also added a test similar to [1] that passes a chain instead of a certificate to the dialog.

[1] https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/mochitest/browser/browser_certViewer.js

Comment 9

[Sami Jaktholm]

Attachment: netmonitor-cert-chains-3-ui.patch

This patch contains browser/ changes required to show certificate chains in the network monitor.

It adds a "View Certificate" button to the security tab that constructs X509Certs from the DER-encoded certificates and opens the chain in the certificate viewer. 

Try run with all patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c871569014f

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8564689 [details] [diff] [review]
netmonitor-cert-chains-1-toolkit.patch

Review of attachment 8564689 [details] [diff] [review]:
-----------------------------------------------------------------

Good stuff! r=me

::: toolkit/devtools/webconsole/network-helper.js
@@ +805,5 @@
> +   * @param nsIX509Cert cert
> +   *        The certificate to serialize.
> +   *
> +   * @return string
> +   *         The DER encoded certificate as a binary String.

Nit: "binary String"? I think you mean just "string".

Comment 11

[Sami Jaktholm]

Attachment: netmonitor-cert-chains-1-toolkit.patch

Thanks for the review. Here's a patch with the comment fixed.

Comment 12

[Victor Porof [:vporof][:vp]]

Comment on attachment 8564692 [details] [diff] [review]
netmonitor-cert-chains-3-ui.patch

Review of attachment 8564692 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/netmonitor/test/browser_net_security-view-cert.js
@@ +117,5 @@
> +        ok(true, "Cert Viewer opened.");
> +        Services.ww.unregisterNotification(observer);
> +
> +        // executeSoon to give certViewer a chance to finish its async
> +        // initialization.

This might be fragile in tests. Can't we add an event instead?

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8564691 [details] [diff] [review]
netmonitor-cert-chains-2-view-cert-chain.patch

Review of attachment 8564691 [details] [diff] [review]:
-----------------------------------------------------------------

This seems like a good opportunity to fix a main-thread-IO issue in the certificate viewer. We never want to call getChain() in that code because it can cause the main thread to wait on network IO. So, wherever something calls nsICertificateDialog.viewCert, it should have already obtained (hopefully off the main thread) a certificate chain to examine. There are only a few places to fix, but a few of them are in comm-central, just as a head-up. Fixing bug 1023621 might be helpful here.

::: security/manager/ssl/public/nsICertificateDialogs.idl
@@ +89,5 @@
> +   *  @param ctx A user interface context.
> +   *  @param chain The certificate chain as an array of nsIX509Certs to be
> +   *               shown to the user.
> +   */
> +  void viewCertChain(in nsIInterfaceRequestor ctx,

We should just modify viewCert to take an nsIX509CertList rather than adding a new function.

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8565037 [details] [diff] [review]
netmonitor-cert-chains-1-toolkit.patch

Review of attachment 8565037 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/webconsole/network-helper.js
@@ +688,5 @@
>          sha1: cert.sha1Fingerprint,
>          sha256: cert.sha256Fingerprint,
>        };
> +
> +      info.chain = this.getRawDERStringsForChain(cert.getChain());

getChain() should be avoided in part because of the main-thread IO issue and in part because its results aren't guaranteed to be either a verified path to a root or even the list of certificates the peer sent (so it's basically useless). See bug 867473, bug 1004580, etc.

Comment 15

[Sami Jaktholm]

(In reply to Victor Porof [:vporof][:vp] from comment #12)
> Comment on attachment 8564692 [details] [diff] [review]
> netmonitor-cert-chains-3-ui.patch
> 
> Review of attachment 8564692 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/netmonitor/test/browser_net_security-view-cert.js
> @@ +117,5 @@
> > +        ok(true, "Cert Viewer opened.");
> > +        Services.ww.unregisterNotification(observer);
> > +
> > +        // executeSoon to give certViewer a chance to finish its async
> > +        // initialization.
> 
> This might be fragile in tests. Can't we add an event instead?

executeSoon is there only to suppress an error message that would be printed to the logs if the certificate viewer window was closed before its onload handler is executed. The test works with or without it.

Comment 16

[Sami Jaktholm]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #13)
> Comment on attachment 8564691 [details] [diff] [review]
> netmonitor-cert-chains-2-view-cert-chain.patch
> 
> Review of attachment 8564691 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems like a good opportunity to fix a main-thread-IO issue in the
> certificate viewer. We never want to call getChain() in that code because it
> can cause the main thread to wait on network IO. So, wherever something
> calls nsICertificateDialog.viewCert, it should have already obtained
> (hopefully off the main thread) a certificate chain to examine. There are
> only a few places to fix, but a few of them are in comm-central, just as a
> head-up. Fixing bug 1023621 might be helpful here.
That sounds like a complicated problem and I don't have the knowledge or time to figure out how everything works now and how to fix the issues you mentioned.

Comment 17

[Sami Jaktholm]

However, I think I'll be able to fix bug 1049110 which should make it possible to implement this without having to use getChain() & friends.

Comment 18

[Patrick Brosset <:pbro>]

This bug has not been updated in the last 3 months. Resetting the assignee field.
Please, feel free to pick it up again and add a comment outlining your plans for it if you do still intend to work on it.
This is just trying to clean our backlog of bugs and make bugs available for people.