Bug 190789 - [WPE][GTK] Pass full certificate chain in CertificateInfo coder
Summary: [WPE][GTK] Pass full certificate chain in CertificateInfo coder
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Claudio Saavedra
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-10-22 02:33 PDT by Claudio Saavedra
Modified: 2018-10-23 08:52 PDT (History)
3 users (show)

See Also:


Attachments
Patch (6.68 KB, patch)
2018-10-22 02:36 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (7.34 KB, patch)
2018-10-23 01:52 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (7.36 KB, patch)
2018-10-23 02:31 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Claudio Saavedra 2018-10-22 02:33:20 PDT
[WPE][GTK] Pass certificate issuer between processes if present in CertificateInfo
Comment 1 Claudio Saavedra 2018-10-22 02:36:13 PDT
Created attachment 352882 [details]
Patch
Comment 2 Claudio Saavedra 2018-10-22 02:38:28 PDT
This is a bit of a RFC/WIP, but it would be really useful for apps to have at least the issuer certificate in the GTlsCertificate that the WebKitWebView API provides. The idea of the patch is to not lose that information when passing it between process. It could be done more elegantly (or even continue with the whole chain but I think this is good enough for now.
Comment 3 Claudio Saavedra 2018-10-22 02:43:51 PDT
Additionally one could add a signed-certificate/issuer-certificate combo to the tests resources and a test that checks this.
Comment 4 Michael Catanzaro 2018-10-22 07:34:50 PDT
Comment on attachment 352882 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352882&action=review

This seems like a great idea! But I disagree that it's sufficient to pass just the immediate issuer: I'd consider it broken to pass only a single issuer and not the entire chain. The problem is that this will enable manual certificate verification and more complex certificate information dialogs... or it would seem to, and they'll work for most websites, but they will be broken for websites that send more than one intermediate certificate. So can you add a loop here? You'd just need to use a uint32_t instead of a bool to encode the total number of certificates in the chain before decoding the certificates themselves, so it should be an easy enhancement.

> Source/WebKit/Shared/soup/WebCoreArgumentCodersSoup.cpp:129
> +        GRefPtr<GTlsCertificate> issuer = adoptGRef(G_TLS_CERTIFICATE(g_initable_new(
> +            g_tls_backend_get_certificate_type(backend), 0, 0, "certificate", issuerBytes.get(), nullptr)));
> +
> +        certificate = adoptGRef(G_TLS_CERTIFICATE(g_initable_new(
> +            g_tls_backend_get_certificate_type(backend), 0, 0, "certificate", certificateBytes.get(), "issuer", issuer.get(), nullptr)));

It will also be less-confusing if you do this in the opposite order: encode the server certificate first, then the issuer, then the issuer's issuer, etc. until the chain ends.

> Tools/ChangeLog:17
> +        (testSSL): Test that the self-signed certificatt has no bogus

certificate

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestSSL.cpp:77
> +    // Self-signed certificate has a nullptr issuer.
> +    g_assert(!g_tls_certificate_get_issuer(test->m_certificate.get()));

It doesn't really test the change, but it's good enough IMO. Setting up a more complex certificate, as would be needed to test this, is already tested elsewhere in our stack below the WebKit layer, and the likelihood of this particular WebKit code breaking in the future is relatively low, so I think it's OK.
Comment 5 Claudio Saavedra 2018-10-23 01:52:00 PDT
Created attachment 352962 [details]
Patch
Comment 6 Claudio Saavedra 2018-10-23 02:31:38 PDT
Created attachment 352964 [details]
Patch
Comment 7 Michael Catanzaro 2018-10-23 07:39:16 PDT
Comment on attachment 352964 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352964&action=review

Wow, it's a good do-while loop. Don't see those very often.

> Source/WebKit/Shared/soup/WebCoreArgumentCodersSoup.cpp:85
> +    // Encode starting from the root certificate.
> +    for (uint32_t i = chainLength; i > 0; i--) {
> +        GRefPtr<GByteArray> certificate = adoptGRef(certificatesDataList[i - 1]);
> +        encoder.encodeVariableLengthByteArray(IPC::DataReference(certificate->data, certificate->len));
>      }

Eh, maybe it *was* nicer the other way around... whatever, either order is totally fine, as you prefer.
Comment 8 WebKit Commit Bot 2018-10-23 08:52:18 PDT
Comment on attachment 352964 [details]
Patch

Clearing flags on attachment: 352964

Committed r237351: <https://trac.webkit.org/changeset/237351>
Comment 9 WebKit Commit Bot 2018-10-23 08:52:20 PDT
All reviewed patches have been landed.  Closing bug.