Skip to content
Permalink
Browse files
[SOUP] webkit_web_view_get_https_status() broken with service workers
https://bugs.webkit.org/show_bug.cgi?id=216038

Patch by Michael Catanzaro <mcatanzaro@gnome.org> 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):

Canonical link: https://commits.webkit.org/230403@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@268394 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
mcatanzaro authored and webkit-commit-queue committed Oct 13, 2020
1 parent a0a78b9 commit 1429617faca8ecf98dbc8edc4525ec9f6371569c
Showing 3 changed files with 88 additions and 2 deletions.
@@ -1,3 +1,46 @@
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 Zalan Bujtas <zalan@apple.com>

[LFC][IFC] Remove fully collapsed trailing run
@@ -45,10 +45,10 @@ class CertificateInfo {
CertificateInfo();
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(); }
void setCertificate(GTlsCertificate* certificate) { m_certificate = certificate; }
@@ -32,6 +32,8 @@
#include <ResourceError.h>
#include <ResourceResponse.h>
#include <libsoup/soup.h>
#include <wtf/glib/GRefPtr.h>
#include <wtf/glib/GUniquePtr.h>

namespace WebCore {

@@ -60,6 +62,47 @@ CertificateInfo::CertificateInfo(GTlsCertificate* certificate, GTlsCertificateFl

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

#endif

0 comments on commit 1429617

Please sign in to comment.