Bug 246379 – REGRESSION(255206@main): [curl] WKProtectionSpaceCopyCertificateInfo is defunct

• Component: Page Loading
• Status: RESOLVED FIXED

[Fujii Hironori]

[WinCairo WK2 MiniBrowser] crashing in bad cert site

1. Start WinCairo WK2 MiniBrowser
2. Go to https://badssl.com/
3. Click "expired"
4. Crash

> WebKit2.dll!WTF::RawPtrTraits<WTF::StringImpl>::unwrap(WTF::StringImpl * const & ptr) Line 44	C++
> WebKit2.dll!WTF::RefPtr<WTF::StringImpl,WTF::RawPtrTraits<WTF::StringImpl>,WTF::DefaultRefDerefTraits<WTF::StringImpl>>::get() Line 76	C++
> WebKit2.dll!WTF::String::impl() Line 115	C++
> WebKit2.dll!WTF::StringView::StringView(const WTF::String & string) Line 410	C++
> WebKit2.dll!API::String::stringView() Line 54	C++
> WebKit2.dll!WKStringGetLength(const OpaqueWKString * stringRef) Line 53	C++
> MiniBrowserLib.dll!createString(const OpaqueWKString * wkString) Line 33	C++
> MiniBrowserLib.dll!WebKitBrowserWindow::canTrustServerCertificate(const OpaqueWKProtectionSpace * protectionSpace) Line 482	C++
> MiniBrowserLib.dll!WebKitBrowserWindow::didReceiveAuthenticationChallenge(const OpaqueWKPage * page, const OpaqueWKAuthenticationChallenge * challenge, const void * clientInfo) Line 455	C++
> WebKit2.dll!`WKPageSetPageNavigationClient'::`2'::NavigationClient::didReceiveAuthenticationChallenge(WebKit::WebPageProxy & page, WebKit::AuthenticationChallengeProxy & authenticationChallenge) Line 2297	C++
> WebKit2.dll!WebKit::WebPageProxy::didReceiveAuthenticationChallengeProxy(WTF::Ref<WebKit::AuthenticationChallengeProxy,WTF::RawPtrTraits<WebKit::AuthenticationChallengeProxy>> && authenticationChallenge, WebKit::NegotiatedLegacyTLS negotiatedLegacyTLS) Line 8818	C++
> WebKit2.dll!WebKit::NetworkProcessProxy::didReceiveAuthenticationChallenge(PAL::SessionID sessionID, WTF::ObjectIdentifier<enum WebKit::WebPageProxyIdentifierType> pageID, const std::optional<WebCore::SecurityOriginData> & topOrigin, WebCore::AuthenticationChallenge && coreChallenge, bool negotiatedLegacyTLS, WTF::ObjectIdentifier<enum WebKit::AuthenticationChallengeIdentifierType> challengeID) Line 512	C++
> WebKit2.dll!IPC::callMemberFunctionImpl<WebKit::NetworkProcessProxy,void (__cdecl WebKit::NetworkProcessProxy::*)(PAL::SessionID,WTF::ObjectIdentifier<enum WebKit::WebPageProxyIdentifierType>,std::optional<WebCore::SecurityOriginData> const &,WebCore::AuthenticationChallenge &&,bool,WTF::ObjectIdentifier<enum WebKit::AuthenticationChallengeIdentifierType>),std::tuple<PAL::SessionID,WTF::ObjectIdentifier<enum WebKit::WebPageProxyIdentifierType>,std::optional<WebCore::SecurityOriginData>,WebCore::AuthenticationChallenge,bool,WTF::ObjectIdentifier<enum WebKit::AuthenticationChallengeIdentifierType>>,0,1,2,3,4,5>(WebKit::NetworkProcessProxy * object, void(WebKit::NetworkProcessProxy::*)(PAL::SessionID, WTF::ObjectIdentifier<enum WebKit::WebPageProxyIdentifierType>, const std::optional<WebCore::SecurityOriginData> &, WebCore::AuthenticationChallenge &&, bool, WTF::ObjectIdentifier<enum WebKit::AuthenticationChallengeIdentifierType>) function, std::tuple<PAL::SessionID,WTF::ObjectIdentifier<enum WebKit::WebPageProxyIdentifierType>,std::optional<WebCore::SecurityOriginData>,WebCore::AuthenticationChallenge,bool,WTF::ObjectIdentifier<enum WebKit::AuthenticationChallengeIdentifierType>> && args, std::integer_sequence<unsigned __int64,0,1,2,3,4,5> __formal) Line 132	C++
> WebKit2.dll!IPC::callMemberFunction<WebKit::NetworkProcessProxy,void (__cdecl WebKit::NetworkProcessProxy::*)(PAL::SessionID,WTF::ObjectIdentifier<enum WebKit::WebPageProxyIdentifierType>,std::optional<WebCore::SecurityOriginData> const &,WebCore::AuthenticationChallenge &&,bool,WTF::ObjectIdentifier<enum WebKit::AuthenticationChallengeIdentifierType>),std::tuple<PAL::SessionID,WTF::ObjectIdentifier<enum WebKit::WebPageProxyIdentifierType>,std::optional<WebCore::SecurityOriginData>,WebCore::AuthenticationChallenge,bool,WTF::ObjectIdentifier<enum WebKit::AuthenticationChallengeIdentifierType>>,std::integer_sequence<unsigned __int64,0,1,2,3,4,5>>(std::tuple<PAL::SessionID,WTF::ObjectIdentifier<enum WebKit::WebPageProxyIdentifierType>,std::optional<WebCore::SecurityOriginData>,WebCore::AuthenticationChallenge,bool,WTF::ObjectIdentifier<enum WebKit::AuthenticationChallengeIdentifierType>> && args, WebKit::NetworkProcessProxy * object, void(WebKit::NetworkProcessProxy::*)(PAL::SessionID, WTF::ObjectIdentifier<enum WebKit::WebPageProxyIdentifierType>, const std::optional<WebCore::SecurityOriginData> &, WebCore::AuthenticationChallenge &&, bool, WTF::ObjectIdentifier<enum WebKit::AuthenticationChallengeIdentifierType>) function) Line 138	C++
> WebKit2.dll!IPC::handleMessage<Messages::NetworkProcessProxy::DidReceiveAuthenticationChallenge,WebKit::NetworkProcessProxy,void (__cdecl WebKit::NetworkProcessProxy::*)(PAL::SessionID,WTF::ObjectIdentifier<enum WebKit::WebPageProxyIdentifierType>,std::optional<WebCore::SecurityOriginData> const &,WebCore::AuthenticationChallenge &&,bool,WTF::ObjectIdentifier<enum WebKit::AuthenticationChallengeIdentifierType>)>(IPC::Connection & connection, IPC::Decoder & decoder, WebKit::NetworkProcessProxy * object, void(WebKit::NetworkProcessProxy::*)(PAL::SessionID, WTF::ObjectIdentifier<enum WebKit::WebPageProxyIdentifierType>, const std::optional<WebCore::SecurityOriginData> &, WebCore::AuthenticationChallenge &&, bool, WTF::ObjectIdentifier<enum WebKit::AuthenticationChallengeIdentifierType>) function) Line 260	C++
> WebKit2.dll!WebKit::NetworkProcessProxy::didReceiveNetworkProcessProxyMessage(IPC::Connection & connection, IPC::Decoder & decoder) Line 344	C++
> WebKit2.dll!WebKit::NetworkProcessProxy::didReceiveMessage(IPC::Connection & connection, IPC::Decoder & decoder) Line 451	C++
> WebKit2.dll!IPC::Connection::dispatchMessage(IPC::Decoder & decoder) Line 1158	C++
> WebKit2.dll!IPC::Connection::dispatchMessage(std::unique_ptr<IPC::Decoder,std::default_delete<IPC::Decoder>> message) Line 1204	C++
> WebKit2.dll!IPC::Connection::dispatchOneIncomingMessage() Line 1272	C++
> WebKit2.dll!IPC::Connection::enqueueIncomingMessage::__l2::<lambda_1>::operator()() Line 1121	C++
> WebKit2.dll!WTF::Detail::CallableWrapper<`IPC::Connection::enqueueIncomingMessage'::`2'::<lambda_1>,void>::call() Line 53	C++
> WTF.dll!WTF::Function<void __cdecl(void)>::operator()() Line 83	C++
> WTF.dll!WTF::RunLoop::performWork() Line 147	C++
> WTF.dll!WTF::RunLoop::wndProc(HWND__ * hWnd, unsigned int message, unsigned __int64 wParam, __int64 lParam) Line 57	C++
> WTF.dll!WTF::RunLoop::RunLoopWndProc(HWND__ * hWnd, unsigned int message, unsigned __int64 wParam, __int64 lParam) Line 39	C++
> [External Code]	
> MiniBrowserLib.dll!wWinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * lpstrCmdLine, int nCmdShow) Line 120	C++
> MiniBrowserLib.dll!dllLauncherEntryPoint(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * lpstrCmdLine, int nCmdShow) Line 140	C++
> MiniBrowser.exe!wWinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * lpstrCmdLine, int nCmdShow) Line 225	C++
> [External Code]	

WKProtectionSpaceCopyCertificateInfo returned nullptr.

[Fujii Hironori]

Since 255206@main (bug#245997)

[Fujii Hironori]

Created attachment 462932 [details]
Patch

[Fujii Hironori]

Created attachment 462934 [details]
Patch

[Fujii Hironori]

Created attachment 462935 [details]
Patch

[Darin Adler]

Comment on attachment 462935 [details]
Patch

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

> COMMIT_MESSAGE:8
> +255206@main deprecated WKProtectionSpaceCopyCertificateInfo API and
> +WKCertificateInfoRef. But, WinCairo and PlayStation ports are using
> +it. Reverted the part of the change.

Can we find a way to preserve deprecation warnings for uses of these on Apple ports? This is quite valuable to the Apple WebKit team. I suggest we create a variant of WK_C_API_DEPRECATED for cases like this where we want this on some ports but not others. Maybe a "Windows-only" version, or a "non-Apple-ports-only" version, depending on what people maintaining other ports want.

[Alex Christensen]

Comment on attachment 462935 [details]
Patch

Rather than re-introduce the abstraction of WebCertificateInfo, let's add a function like this instead:
WKArrayRef WKProtectionSpaceCopyCertificateChain(WKProtectionSpaceRef protectionSpace);

[Alex Christensen]

Something like this.  I haven't gotten it compiling and it has a small piece that still needs implementing, but I think this is a better design:

diff --git a/Source/WebKit/UIProcess/API/C/curl/WKProtectionSpaceCurl.cpp b/Source/WebKit/UIProcess/API/C/curl/WKProtectionSpaceCurl.cpp
index 03ae616a37c6..18e795cf5573 100644
--- a/Source/WebKit/UIProcess/API/C/curl/WKProtectionSpaceCurl.cpp
+++ b/Source/WebKit/UIProcess/API/C/curl/WKProtectionSpaceCurl.cpp
@@ -36,3 +36,22 @@ WKCertificateInfoRef WKProtectionSpaceCopyCertificateInfo(WKProtectionSpaceRef p
 {
     return nullptr;
 }
+
+WKArrayRef WKProtectionSpaceCopyCertificateChain(WKProtectionSpaceRef protectionSpace)
+{
+    auto& certificateInfo = toImpl(protectionSpaceRef)->protectionSpace().certificateInfo();
+    (void)certificateInfo; // FIXME: Get the data of each certificate here.
+    return nullptr;
+}
+
+int WKProtectionSpaceGetCertificateVerificationError(WKProtectionSpaceRef protectionSpace)
+{
+    auto& certificateInfo = toImpl(protectionSpaceRef)->protectionSpace().certificateInfo();
+    return certificateInfo.verificationError();
+}
+
+WK_EXPORT WKStringRef WKProtectionSpaceCopyCertificateVerificationErrorDescription(WKProtectionSpaceRef protectionSpace)
+{
+    auto& certificateInfo = toImpl(protectionSpaceRef)->protectionSpace().certificateInfo();
+    return WebKit::toCopiedAPI(certificateInfo.verificationErrorDescription());
+}
diff --git a/Source/WebKit/UIProcess/API/C/curl/WKProtectionSpaceCurl.h b/Source/WebKit/UIProcess/API/C/curl/WKProtectionSpaceCurl.h
index 3161fc290cdf..c7d88217a738 100644
--- a/Source/WebKit/UIProcess/API/C/curl/WKProtectionSpaceCurl.h
+++ b/Source/WebKit/UIProcess/API/C/curl/WKProtectionSpaceCurl.h
@@ -33,6 +33,9 @@ extern "C" {
 #endif
 
 WK_EXPORT WKCertificateInfoRef WKProtectionSpaceCopyCertificateInfo(WKProtectionSpaceRef) WK_C_API_DEPRECATED;
+WK_EXPORT WKArrayRef WKProtectionSpaceCopyCertificateChain(WKProtectionSpaceRef protectionSpace);
+WK_EXPORT int WKProtectionSpaceGetCertificateVerificationError(WKProtectionSpaceRef protectionSpace);
+WK_EXPORT WKStringRef WKProtectionSpaceCopyCertificateVerificationErrorDescription(WKProtectionSpaceRef protectionSpace);
 
 #ifdef __cplusplus
 }
diff --git a/Tools/MiniBrowser/win/WebKitBrowserWindow.cpp b/Tools/MiniBrowser/win/WebKitBrowserWindow.cpp
index 50565a2e540a..c312d116ec97 100644
--- a/Tools/MiniBrowser/win/WebKitBrowserWindow.cpp
+++ b/Tools/MiniBrowser/win/WebKitBrowserWindow.cpp
@@ -61,16 +61,16 @@ std::string createUTF8String(const wchar_t* src, size_t srcLength)
     return { buffer.data(), actualLength };
 }
 
-std::wstring createPEMString(WKCertificateInfoRef certificateInfo)
+std::wstring createPEMString(WKProtectionSpaceRef protectionSpace)
 {
-    auto chainSize = WKCertificateInfoGetCertificateChainSize(certificateInfo);
+    auto chain = adoptWK(WKProtectionSpaceCopyCertificateChain(protectionSpace));
 
     std::wstring pems;
 
-    for (auto i = 0; i < chainSize; i++) {
-        auto certificate = adoptWK(WKCertificateInfoCopyCertificateAtIndex(certificateInfo, i));
-        auto size = WKDataGetSize(certificate.get());
-        auto data = WKDataGetBytes(certificate.get());
+    for (auto i = 0; i < WKArrayGetSize(chain.get()); i++) {
+        auto certificate = WKArrayGetItemAtIndex(chain.get(), i);
+        auto size = WKDataGetSize(certificate);
+        auto data = WKDataGetBytes(certificate);
 
         for (size_t i = 0; i < size; i++)
             pems.push_back(data[i]);
@@ -477,9 +477,9 @@ void WebKitBrowserWindow::didReceiveAuthenticationChallenge(WKPageRef page, WKAu
 bool WebKitBrowserWindow::canTrustServerCertificate(WKProtectionSpaceRef protectionSpace)
 {
     auto host = createString(adoptWK(WKProtectionSpaceCopyHost(protectionSpace)).get());
-    auto certificateInfo = adoptWK(WKProtectionSpaceCopyCertificateInfo(protectionSpace));
-    auto verificationError = WKCertificateInfoGetVerificationError(certificateInfo.get());
-    auto description = createString(adoptWK(WKCertificateInfoCopyVerificationErrorDescription(certificateInfo.get())).get());
+    auto verificationError = WKProtectionSpaceGetCertificateVerificationError(protectionSpace);
+    auto verificationError = WKProtectionSpaceCopyCertificateVerificationErrorDescription(protectionSpace);
+    auto description = createString(adoptWK(WKProtectionSpaceCopyCertificateVerificationErrorDescription(protectionSpace)).get());
     auto pem = createPEMString(certificateInfo.get());
 
     auto it = m_acceptedServerTrustCerts.find(host);

[Fujii Hironori]

Created attachment 462947 [details]
Patch

[Alex Christensen]

Thanks!  This is closer to the API shape of certificate chains on other platforms.

[EWS]

Committed 255505@main (9fa349553d3f): <https://commits.webkit.org/255505@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/101143266>