Changeset 268909 in webkit

Timestamp:

Oct 23, 2020, 1:25:27 AM (6 years ago)

Author:

Carlos Garcia Campos

Message:

Merge r268394 - [SOUP] webkit_web_view_get_https_status() broken with service workers
​https://bugs.webkit.org/show_bug.cgi?id=216038

Patch by Michael Catanzaro <Michael Catanzaro> on 2020-10-13
Reviewed by Carlos Garcia Campos.

This implements CertificateInfo::isolatedCopy for libsoup ports. This is impossible to do
completely, because we cannot copy the private key portion of the GTlsCertificate, because
it is a write-only property, because it might be backed by a hardware token and therefore
really impossible to write software to get it. If we were to implement g_tls_certificate_copy()
in GLib -- which I will probably do eventually, because I need this outside WebKit as well --
then GTlsCertificate implementations could copy the private key or PKCS#11 handle or
whatever.

But anyway, let not perfect be the enemy of the good. We only need this for service workers
currently. (Probably that's all we'll *ever* need it for.) So we are only working with
server certificates, and the private portion of the certificate is guaranteed to not exist
(because servers don't send us their private keys), and we can just forget about it. As
long as nobody tries to copy a client certificate in the future -- where we really would
need the private key portion of the certificate -- this will be perfectly fine.

Actually copying the certificate is kind of annoying, because a chain of certificates is
represented by having the main (server) certificate keep a reference to its issuer, which is
not referenced anywhere else, so we have to reconstruct the chain in reverse order starting
from the final certificate, working back towards the server cert. Fun. Let's also be
careful to construct a completely new GByteArray rather than expecting the GTlsCertificate
implementation to copy it for us. Because GTlsCertificate is implemented by an extension
point and applications and system administrators can -- and do -- implement their own,
implementations could do anything, including keep a reference to the GByteArray that we
pass in.

It would be nice to have a test for this, but writing tests is hard. Also, I don't really
want to learn what service workers are. :)

Drive-by fix: also remove explicit from a constructor that doesn't need it.

• platform/network/soup/CertificateInfo.h:

(WebCore::CertificateInfo::isolatedCopy const):

• platform/network/soup/CertificateInfoSoup.cpp:

(WebCore::createCertificate):
(WebCore::CertificateInfo::isolatedCopy const):

Location:

releases/WebKitGTK/webkit-2.30/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/network/soup/CertificateInfo.h (modified) (1 diff)
• platform/network/soup/CertificateInfoSoup.cpp (modified) (2 diffs)

releases/WebKitGTK/webkit-2.30/Source/WebCore/ChangeLog

@@ +1 @@
+2020-10-13  Michael Catanzaro  <mcatanzaro@gnome.org>
+
+        [SOUP] webkit_web_view_get_https_status() broken with service workers
+        https://bugs.webkit.org/show_bug.cgi?id=216038
+
+        Reviewed by Carlos Garcia Campos.
+
+        This implements CertificateInfo::isolatedCopy for libsoup ports. This is impossible to do
+        completely, because we cannot copy the private key portion of the GTlsCertificate, because
+        it is a write-only property, because it might be backed by a hardware token and therefore
+        really impossible to write software to get it. If we were to implement g_tls_certificate_copy()
+        in GLib -- which I will probably do eventually, because I need this outside WebKit as well --
+        then GTlsCertificate implementations could copy the private key or PKCS#11 handle or
+        whatever.
+
+        But anyway, let not perfect be the enemy of the good. We only need this for service workers
+        currently. (Probably that's all we'll *ever* need it for.) So we are only working with
+        server certificates, and the private portion of the certificate is guaranteed to not exist
+        (because servers don't send us their private keys), and we can just forget about it. As
+        long as nobody tries to copy a client certificate in the future -- where we really would
+        need the private key portion of the certificate -- this will be perfectly fine.
+
+        Actually copying the certificate is kind of annoying, because a chain of certificates is
+        represented by having the main (server) certificate keep a reference to its issuer, which is
+        not referenced anywhere else, so we have to reconstruct the chain in reverse order starting
+        from the final certificate, working back towards the server cert. Fun. Let's also be
+        careful to construct a completely new GByteArray rather than expecting the GTlsCertificate
+        implementation to copy it for us. Because GTlsCertificate is implemented by an extension
+        point and applications and system administrators can -- and do -- implement their own,
+        implementations could do anything, including keep a reference to the GByteArray that we
+        pass in.
+
+        It would be nice to have a test for this, but writing tests is hard. Also, I don't really
+        want to learn what service workers are. :)
+
+        Drive-by fix: also remove explicit from a constructor that doesn't need it.
+
+        * platform/network/soup/CertificateInfo.h:
+        (WebCore::CertificateInfo::isolatedCopy const):
+        * platform/network/soup/CertificateInfoSoup.cpp:
+        (WebCore::createCertificate):
+        (WebCore::CertificateInfo::isolatedCopy const):
+
 2020-10-13  Philippe Normand  <pnormand@igalia.com>
 

releases/WebKitGTK/webkit-2.30/Source/WebCore/platform/network/soup/CertificateInfo.h

@@ -46 +46 @@
     explicit CertificateInfo(const WebCore::ResourceResponse&);
     explicit CertificateInfo(const WebCore::ResourceError&);
-    explicit CertificateInfo(GTlsCertificate*, GTlsCertificateFlags);
+    CertificateInfo(GTlsCertificate*, GTlsCertificateFlags);
     WEBCORE_EXPORT ~CertificateInfo();
 
-    CertificateInfo isolatedCopy() const { notImplemented(); return { }; }
+    CertificateInfo isolatedCopy() const;
 
     GTlsCertificate* certificate() const { return m_certificate.get(); }

releases/WebKitGTK/webkit-2.30/Source/WebCore/platform/network/soup/CertificateInfoSoup.cpp

@@ -33 +33 @@
 #include <ResourceResponse.h>
 #include <libsoup/soup.h>
+#include <wtf/glib/GRefPtr.h>
+#include <wtf/glib/GUniquePtr.h>
 
 namespace WebCore {
@@ -61 +63 @@
 CertificateInfo::~CertificateInfo() = default;
 
+static GRefPtr<GTlsCertificate> createCertificate(GByteArray* bytes, GTlsCertificate* issuer)
+{
+    gpointer cert = g_initable_new(g_tls_backend_get_certificate_type(g_tls_backend_get_default()),
+        nullptr, nullptr,
+        "certificate", bytes,
+        "issuer", issuer,
+        nullptr);
+    RELEASE_ASSERT(cert);
+    return adoptGRef(G_TLS_CERTIFICATE(cert));
+}
+
+CertificateInfo CertificateInfo::isolatedCopy() const
+{
+    // We can only copy the public portions, so this can only be used for server certificates, not
+    // for client certificates. Sadly, other ports don't have this restriction, and there is no way
+    // to assert that we are not messing up here because we can't know how callers are using the
+    // certificate. So be careful?
+    //
+    // We should add g_tls_certificate_copy() to GLib so that we can copy the private portion too.
+
+    Vector<GRefPtr<GByteArray>> certificateBytes;
+    GTlsCertificate* cert = m_certificate.get();
+    if (!cert)
+        return CertificateInfo();
+
+    do {
+        GRefPtr<GByteArray> der;
+        g_object_get(cert, "certificate", &der.outPtr(), nullptr);
+
+        GRefPtr<GByteArray> copy = adoptGRef(g_byte_array_new());
+        g_byte_array_append(copy.get(), der->data, der->len);
+        certificateBytes.append(WTFMove(copy));
+    } while ((cert = g_tls_certificate_get_issuer(cert)));
+
+    auto finalCertificateIndex = certificateBytes.size() - 1;
+    GRefPtr<GTlsCertificate> copy = createCertificate(certificateBytes[finalCertificateIndex].get(), nullptr);
+    for (ssize_t i = finalCertificateIndex - 1; i >= 0; i--)
+        copy = createCertificate(certificateBytes[i].get(), copy.get());
+    return CertificateInfo(copy.get(), m_tlsErrors);
+}
+
 } // namespace WebCore