From 6a8cc0870c7a9c26021269f692fe80baaed6c613 Mon Sep 17 00:00:00 2001 From: Brandon-T Date: Thu, 25 Jan 2024 04:47:48 -0500 Subject: [PATCH] Fix #8688: Fix URL-Bar not updating sometimes to show the latest cert status (#8698) Trigger cert validation on request failure/cancellation Added a certificate trust error handling when WebKit throws an error internally and we receive no certs! This can mess with the URL Bar, interstitial pages, and cert viewer. Move evaluation of certs to a serial queue. --- .../BVC+WKNavigationDelegate.swift | 43 ++++++++++++++++--- .../BrowserViewController.swift | 2 +- .../Browser/Helpers/ErrorPageHelper.swift | 11 +++-- .../CertificateErrorPageHandler.swift | 2 +- Sources/Brave/Frontend/Browser/Tab.swift | 1 + .../BraveCertificateUtils.swift | 24 +++++------ 6 files changed, 59 insertions(+), 24 deletions(-) diff --git a/Sources/Brave/Frontend/Browser/BrowserViewController/BVC+WKNavigationDelegate.swift b/Sources/Brave/Frontend/Browser/BrowserViewController/BVC+WKNavigationDelegate.swift index 72d4f444943..3279569b46d 100644 --- a/Sources/Brave/Frontend/Browser/BrowserViewController/BVC+WKNavigationDelegate.swift +++ b/Sources/Brave/Frontend/Browser/BrowserViewController/BVC+WKNavigationDelegate.swift @@ -70,6 +70,8 @@ extension BrowserViewController: WKNavigationDelegate { if let url = webView.url { if !InternalURL.isValid(url: url) { // reset secure content state to unknown until page can be evaluated + selectedTab.sslPinningError = nil + selectedTab.sslPinningTrust = nil selectedTab.secureContentState = .unknown updateToolbarSecureContentState(.unknown) } @@ -612,6 +614,15 @@ extension BrowserViewController: WKNavigationDelegate { // Cert is valid and should not be pinned // Let the system handle it and we'll show an error if the system cannot validate it if result == Int32.min { + // Cert is POTENTIALLY invalid and cannot be pinned + + await MainActor.run { + // Handle the potential error later in `didFailProvisionalNavigation` + self.tab(for: webView)?.sslPinningTrust = serverTrust + } + + // Let WebKit handle the request and validate the cert + // This is the same as calling `BraveCertificateUtils.evaluateTrust` return (.performDefaultHandling, nil) } @@ -718,7 +729,6 @@ extension BrowserViewController: WKNavigationDelegate { public func webView(_ webView: WKWebView, didFinish navigation: WKNavigation!) { if let tab = tabManager[webView] { - // Deciding whether to inject app's IAP receipt for Brave SKUs or not if let url = tab.url, let braveSkusHelper = BraveSkusWebHelper(for: url), @@ -785,13 +795,26 @@ extension BrowserViewController: WKNavigationDelegate { /// Invoked when an error occurs while starting to load data for the main frame. public func webView(_ webView: WKWebView, didFailProvisionalNavigation navigation: WKNavigation!, withError error: Error) { + guard let tab = tab(for: webView) else { return } + + // WebKit does not update certs on cancellation of a frame load + // So manually trigger the notification with the current cert + // Also, when Chromium cert validation passes, BUT Apple cert validation fails, the request is cancelled automatically by WebKit + // In such a case, the webView.serverTrust is `nil`. The only time we have a valid trust is when we received the challenge + // so we need to update the URL-Bar to show that serverTrust when WebKit's is nil. + let serverTrust = webView.serverTrust ?? tab.sslPinningTrust + observeValue(forKeyPath: KVOConstants.serverTrust.keyPath, + of: webView, + change: [.newKey: serverTrust as Any, .kindKey: 1], + context: nil) + // Ignore the "Frame load interrupted" error that is triggered when we cancel a request // to open an external application and hand it over to UIApplication.openURL(). The result // will be that we switch to the external app, for example the app store, while keeping the // original web page in the tab instead of replacing it with an error page. var error = error as NSError if error.domain == "WebKitErrorDomain" && error.code == 102 { - if let tab = tabManager[webView], tab === tabManager.selectedTab { + if tab === tabManager.selectedTab { updateToolbarCurrentURL(tab.url?.displayURL) updateWebViewPageZoom(tab: tab) } @@ -802,12 +825,12 @@ extension BrowserViewController: WKNavigationDelegate { return } - if let tab = tabManager[webView], let sslPinningError = tab.sslPinningError { + if let sslPinningError = tab.sslPinningError { error = sslPinningError as NSError } if error.code == Int(CFNetworkErrors.cfurlErrorCancelled.rawValue) { - if let tab = tabManager[webView], tab === tabManager.selectedTab { + if tab === tabManager.selectedTab { updateToolbarCurrentURL(tab.url?.displayURL) updateWebViewPageZoom(tab: tab) } @@ -815,6 +838,16 @@ extension BrowserViewController: WKNavigationDelegate { } if let url = error.userInfo[NSURLErrorFailingURLErrorKey] as? URL { + + // The certificate came from the WebKit SSL Handshake validation and the cert is untrusted + if let serverTrust = serverTrust, error.userInfo["NSErrorPeerCertificateChainKey"] == nil { + // Build a cert chain error to display in the cert viewer in such cases, as we aren't given one by WebKit + var userInfo = error.userInfo + userInfo["NSErrorPeerCertificateChainKey"] = SecTrustCopyCertificateChain(serverTrust) as? [SecCertificate] ?? [] + userInfo["NSErrorPeerUntrustedByApple"] = true + error = NSError(domain: error.domain, code: error.code, userInfo: userInfo) + } + ErrorPageHelper(certStore: profile.certStore).loadPage(error, forUrl: url, inWebView: webView) // Submitting same errornous URL using toolbar will cause progress bar get stuck // Reseting the progress bar in case there is an error is necessary @@ -825,7 +858,7 @@ extension BrowserViewController: WKNavigationDelegate { // We rely on loading that page to get the restore callback to reset the restoring // flag, so if we fail to load that page, reset it here. if InternalURL(url)?.aboutComponent == "sessionrestore" { - tabManager.allTabs.filter { $0.webView == webView }.first?.restoring = false + tab.restoring = false } } } diff --git a/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController.swift b/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController.swift index e575ec52b4a..c4eef2baeb6 100644 --- a/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController.swift +++ b/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController.swift @@ -3286,7 +3286,7 @@ extension BrowserViewController: IAPObserverDelegate { extension BrowserViewController { func displayPageCertificateInfo() { - guard let webView = tabManager.selectedTab?.webView else { + guard let tab = tabManager.selectedTab, let webView = tab.webView else { Logger.module.error("Invalid WebView") return } diff --git a/Sources/Brave/Frontend/Browser/Helpers/ErrorPageHelper.swift b/Sources/Brave/Frontend/Browser/Helpers/ErrorPageHelper.swift index 5a01cb4ddc1..69991c63379 100644 --- a/Sources/Brave/Frontend/Browser/Helpers/ErrorPageHelper.swift +++ b/Sources/Brave/Frontend/Browser/Helpers/ErrorPageHelper.swift @@ -105,15 +105,20 @@ class ErrorPageHelper { // 'timestamp' is used for the js reload logic URLQueryItem(name: "timestamp", value: "\(Int(Date().timeIntervalSince1970 * 1000))"), ] + + // The error came from WebKit's internal validation and the cert is untrusted + if error.userInfo["NSErrorPeerUntrustedByApple"] as? Bool == true { + queryItems.append(URLQueryItem(name: "peeruntrusted", value: "true")) + } // If this is an invalid certificate, show a certificate error allowing the // user to go back or continue. The certificate itself is encoded and added as // a query parameter to the error page URL; we then read the certificate from // the URL if the user wants to continue. if CertificateErrorPageHandler.isValidCertificateError(error: error), - let certChain = error.userInfo["NSErrorPeerCertificateChainKey"] as? [SecCertificate], - let underlyingError = error.userInfo[NSUnderlyingErrorKey] as? NSError, - let certErrorCode = underlyingError.userInfo["_kCFStreamErrorCodeKey"] as? Int { + let certChain = error.userInfo["NSErrorPeerCertificateChainKey"] as? [SecCertificate], + let underlyingError = error.userInfo[NSUnderlyingErrorKey] as? NSError, + let certErrorCode = underlyingError.userInfo["_kCFStreamErrorCodeKey"] as? Int { let encodedCerts = ErrorPageHelper.encodeCertChain(certChain) queryItems.append(URLQueryItem(name: "badcerts", value: encodedCerts)) diff --git a/Sources/Brave/Frontend/Browser/Interstitial Pages/CertificateErrorPageHandler.swift b/Sources/Brave/Frontend/Browser/Interstitial Pages/CertificateErrorPageHandler.swift index f2a599bc9f3..f0f882663eb 100644 --- a/Sources/Brave/Frontend/Browser/Interstitial Pages/CertificateErrorPageHandler.swift +++ b/Sources/Brave/Frontend/Browser/Interstitial Pages/CertificateErrorPageHandler.swift @@ -17,7 +17,7 @@ class CertificateErrorPageHandler: InterstitialPageHandler { } func response(for model: ErrorPageModel) -> (URLResponse, Data)? { - let hasCertificate = model.components.valueForQuery("certerror") != nil + let hasCertificate = model.components.valueForQuery("certerror") != nil && model.components.valueForQuery("peeruntrusted") == nil guard let asset = Bundle.module.path(forResource: "CertificateError", ofType: "html") else { assert(false) diff --git a/Sources/Brave/Frontend/Browser/Tab.swift b/Sources/Brave/Frontend/Browser/Tab.swift index 261b154ed7e..8e1a621fd04 100644 --- a/Sources/Brave/Frontend/Browser/Tab.swift +++ b/Sources/Brave/Frontend/Browser/Tab.swift @@ -95,6 +95,7 @@ class Tab: NSObject { var secureContentState: TabSecureContentState = .unknown var sslPinningError: Error? + var sslPinningTrust: SecTrust? private let _syncTab: BraveSyncTab? private let _faviconDriver: FaviconDriver? diff --git a/Sources/CertificateUtilities/BraveCertificateUtils.swift b/Sources/CertificateUtilities/BraveCertificateUtils.swift index 3a21e8103da..1efd9acc228 100644 --- a/Sources/CertificateUtilities/BraveCertificateUtils.swift +++ b/Sources/CertificateUtilities/BraveCertificateUtils.swift @@ -190,6 +190,8 @@ public enum BraveCertificateUtilError: LocalizedError { } public extension BraveCertificateUtils { + private static let evaluationQueue = DispatchQueue(label: "com.brave.cert-utils-evaluation-queue", qos: .userInitiated) + static func createServerTrust(_ certificates: [SecCertificate], for host: String?) throws -> SecTrust { if certificates.isEmpty { throw BraveCertificateUtilError.noCertificatesProvided @@ -211,25 +213,19 @@ public extension BraveCertificateUtils { } static func evaluateTrust(_ trust: SecTrust, for host: String?) async throws { - let policies = [ - SecPolicyCreateSSL(true, host as CFString?), - ] - - SecTrustSetPolicies(trust, policies as CFTypeRef) try await withCheckedThrowingContinuation { (continuation: CheckedContinuation) in - let queue = DispatchQueue.global() - queue.async { - let result = SecTrustEvaluateAsyncWithError(trust, queue) { _, isTrusted, error in - if let error = error { - continuation.resume(throwing: error as Error) + BraveCertificateUtils.evaluationQueue.async { + SecTrustEvaluateAsyncWithError(trust, BraveCertificateUtils.evaluationQueue) { _, isTrusted, error in + if !isTrusted { + if let error = error { + continuation.resume(throwing: error as Error) + } else { + continuation.resume(throwing: BraveCertificateUtilError.trustEvaluationFailed) + } } else { continuation.resume() } } - - if result != errSecSuccess { - continuation.resume(throwing: BraveCertificateUtilError.trustEvaluationFailed) - } } } }