Bug 216038 – [SOUP] webkit_web_view_get_https_status() broken with service workers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

Bug #206403 is probably not fixed for libsoup ports because CertificateInfo::isolated_copy is not implemented.

I don't know how to test service workers. If anybody knows of a test page to see if it works or not, that would be cool.

[Alex Christensen]

https://twitter.com

[Michael Catanzaro]

OK thanks. We just got a report that was broken in https://gitlab.gnome.org/GNOME/epiphany/-/issues/1323, which is how I wound up here, although I had incorrectly assumed it was using offline web application cache rather than service workers.

I can reproduce this problem on twitter.com in WebKitGTK 2.28.4, but not in 2.29.91, which indicates it is probably fixed. HOWEVER... CertificateInfo::isolated_copy is still not implemented, so that's weird. Magic?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #2)
> I can reproduce this problem on twitter.com in WebKitGTK 2.28.4, but not in
> 2.29.91, which indicates it is probably fixed. HOWEVER...
> CertificateInfo::isolated_copy is still not implemented, so that's weird.
> Magic?

So without having bisected it, I would guess that you (Alex) fixed it in r264724, but I don't understand how it's possible with that function unimplemented. WebKit is actually somehow loading the correct TLS certificate -- I can inspect it in Epiphany and view its properties -- whereas previously it reported that the website had no security (webkit_web_view_get_tls_info() returned NULL), indicating that the main resource was loaded via service workers. So there *should* be no way for WebKit to get the CertificateInfo if it's not properly encoded. But the encoding depends on CertificateInfo::isolated_copy, which just returns an empty struct, so ??????

I did think about how to implement CertificateInfo::isolated_copy. It's currently impossible to do without adding new GLib API, because the GTlsCertificate:private-key property is not readable. Internally to GTlsCertificate, it will be a PKCS#11 token, which is just a string that is simple to copy for use across threads. But no way to get it without new API. I've been considering adding it anyway for an unrelated project.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #3)
> But no way to get it without new API. I've been considering adding it anyway for an unrelated project.

(It would be: g_tls_certificate_copy().)

[Michael Catanzaro]

Definitely still broken, I can reproduce on https://twitter.com/othermaciej. We will either need to add new GLib API to allow implementing CertificateInfo::isolated_copy, or else reconsider other possible solutions (e.g. we might be able to use a partial isolated copy that doesn't copy everything).

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #5)
> Definitely still broken, I can reproduce on https://twitter.com/othermaciej.

I'm using 2.30.1 currently. Not sure why I wasn't able to reproduce in 2.29.91 last month.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #6)
> I'm using 2.30.1 currently. Not sure why I wasn't able to reproduce in
> 2.29.91 last month.

I probably didn't refresh the page.

[Michael Catanzaro]

Created attachment 411087 [details]
Patch

[Michael Catanzaro]

Reference: https://discourse.gnome.org/t/epiphany-gnome-web-says-this-site-insecure/4443/8

[Michael Catanzaro]

Created attachment 411088 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 411088 [details]
Patch

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

> Source/WebCore/platform/network/soup/CertificateInfoSoup.cpp:73
> +    RELEASE_ASSERT(!error);

Since you are using the GError just to assert it should never fail, why not just pass nullptr and assert cert is not nullptr?

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #11)
> Since you are using the GError just to assert it should never fail, why not
> just pass nullptr and assert cert is not nullptr?

Good idea.

[Michael Catanzaro]

Created attachment 411220 [details]
Patch

[EWS]

Committed r268394: <https://trac.webkit.org/changeset/268394>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411220 [details].

[Alex Christensen]

Comment on attachment 411220 [details]
Patch

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

> Source/WebCore/platform/network/soup/CertificateInfoSoup.cpp:81
> +    // certificate. So be careful?

CertificateInfo::isolatedCopy should only be used for server certificates.  We need better abstractions.

> Source/WebCore/platform/network/soup/CertificateInfoSoup.cpp:83
> +    // We should add g_tls_certificate_copy() to GLib so that we can copy the private portion too.

I wonder how you would handle the case where the private key is on a USB device that does not allow access to the private key.

[Michael Catanzaro]

(In reply to Alex Christensen from comment #15)
> > Source/WebCore/platform/network/soup/CertificateInfoSoup.cpp:83
> > +    // We should add g_tls_certificate_copy() to GLib so that we can copy the private portion too.
> 
> I wonder how you would handle the case where the private key is on a USB
> device that does not allow access to the private key.

That's no problem for GLib, because GTlsCertificate knows its PKCS#11 URI. (Actually, that is already going to be public API in GLib 2.68, since https://gitlab.gnome.org/GNOME/glib/-/issues/1809.) But that doesn't help WebKit, since if it's not a PKCS#11 certificate, then we really do need to copy the private key to copy the entire certificate, but the private-key property cannot ever be made readable (because then it would be impossible for PKCS#11-backed certificates to implement the API).