Skip to content

Commit

Permalink
Fix brave/brave-ios#8688: Fix URL-Bar not updating sometimes to show …
Browse files Browse the repository at this point in the history
…the latest cert status (brave/brave-ios#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.
  • Loading branch information
Brandon-T authored Jan 25, 2024
1 parent 5fe1b24 commit 761049d
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 24 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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)
}
Expand All @@ -802,19 +825,29 @@ 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)
}
return
}

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
Expand All @@ -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
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
11 changes: 8 additions & 3 deletions Sources/Brave/Frontend/Browser/Helpers/ErrorPageHelper.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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))

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions Sources/Brave/Frontend/Browser/Tab.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand Down
24 changes: 10 additions & 14 deletions Sources/CertificateUtilities/BraveCertificateUtils.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<Void, Error>) 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)
}
}
}
}
Expand Down

0 comments on commit 761049d

Please sign in to comment.