Skip to content

Commit

Permalink
[iOS] Url bar falsely showing insecure state (uplift to 1.63.x) (#22380)
Browse files Browse the repository at this point in the history 
Merge pull request #22343 from brave/bugfix/iOS-Certs-And-Secure-State
  • Loading branch information
@soner-yuksel
soner-yuksel authored Mar 1, 2024
1 parent 48b9717 commit 09f5cd3
Show file tree
Hide file tree
Showing 8 changed files with 146 additions and 164 deletions.
7 changes: 5 additions & 2 deletions ios/brave-ios/Sources/Brave/Frontend/Browser/BrowserViewController/BVC+ShareActivity.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,10 @@ extension BrowserViewController {
}

// Display Certificate Activity
if let secureState = tabManager.selectedTab?.secureContentState, secureState != .missingSSL && secureState != .unknown {
if let tabURL = tabManager.selectedTab?.webView?.url,
tabManager.selectedTab?.webView?.serverTrust != nil
|| ErrorPageHelper.hasCertificates(for: tabURL)
{
activities.append(
BasicMenuActivity(
title: Strings.displayCertificate,
Expand All @@ -278,7 +281,7 @@ extension BrowserViewController {
)
}

// Report Web-compat Issue Actibity
// Report Web-compat Issue Activity
activities.append(
BasicMenuActivity(
title: Strings.Shields.reportABrokenSite,
Expand Down
253 changes: 116 additions & 137 deletions ...e-ios/Sources/Brave/Frontend/Browser/BrowserViewController/BVC+WKNavigationDelegate.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,17 +66,9 @@ extension BrowserViewController: WKNavigationDelegate {

// check if web view is loading a different origin than the one currently loaded
if let selectedTab = tabManager.selectedTab,
selectedTab.url?.origin != webView.url?.origin {
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)
}
}

selectedTab.url?.origin != webView.url?.origin
{

// new site has a different origin, hide wallet icon.
tabManager.selectedTab?.isWalletIconVisible = false
// new site, reset connected addresses
Expand Down Expand Up @@ -606,16 +598,28 @@ extension BrowserViewController: WKNavigationDelegate {
download.delegate = self
}

nonisolated public func webView(_ webView: WKWebView, respondTo challenge: URLAuthenticationChallenge) async -> (URLSession.AuthChallengeDisposition, URLCredential?) {

@MainActor

public func webView(
_ webView: WKWebView,
respondTo challenge: URLAuthenticationChallenge
) async -> (URLSession.AuthChallengeDisposition, URLCredential?) {

// If this is a certificate challenge, see if the certificate has previously been
// accepted by the user.
let host = challenge.protectionSpace.host
let origin = "\(host):\(challenge.protectionSpace.port)"
if challenge.protectionSpace.authenticationMethod == NSURLAuthenticationMethodServerTrust,
let trust = challenge.protectionSpace.serverTrust,
let cert = (SecTrustCopyCertificateChain(trust) as? [SecCertificate])?.first, profile.certStore.containsCertificate(cert, forOrigin: origin) {
return (.useCredential, URLCredential(trust: trust))
let trust = challenge.protectionSpace.serverTrust
{
let cert = await Task<SecCertificate?, Never>.detached {
return (SecTrustCopyCertificateChain(trust) as? [SecCertificate])?.first
}.value

if let cert = cert, profile.certStore.containsCertificate(cert, forOrigin: origin) {
return (.useCredential, URLCredential(trust: trust))
}
}

// Certificate Pinning
Expand All @@ -636,36 +640,35 @@ extension BrowserViewController: WKNavigationDelegate {
// 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`
// This is the same as calling `BraveCertificateUtils.evaluateTrust` but with more error info provided by WebKit
return (.performDefaultHandling, nil)
}

// Cert is invalid and cannot be pinned
Logger.module.error("CERTIFICATE_INVALID")
let errorCode = CFNetworkErrors.braveCertificatePinningFailed.rawValue

let underlyingError = NSError(domain: kCFErrorDomainCFNetwork as String,
code: Int(errorCode),
userInfo: ["_kCFStreamErrorCodeKey": Int(errorCode)])

let error = await NSError(domain: kCFErrorDomainCFNetwork as String,
code: Int(errorCode),
userInfo: [NSURLErrorFailingURLErrorKey: webView.url as Any,
"NSErrorPeerCertificateChainKey": certificateChain,
NSUnderlyingErrorKey: underlyingError])

await MainActor.run {
// Handle the error later in `didFailProvisionalNavigation`
self.tab(for: webView)?.sslPinningError = error
}


let underlyingError = NSError(
domain: kCFErrorDomainCFNetwork as String,
code: Int(errorCode),
userInfo: ["_kCFStreamErrorCodeKey": Int(errorCode)]
)

let error = NSError(
domain: kCFErrorDomainCFNetwork as String,
code: Int(errorCode),
userInfo: [
NSURLErrorFailingURLErrorKey: webView.url as Any,
"NSErrorPeerCertificateChainKey": certificateChain,
NSUnderlyingErrorKey: underlyingError,
]
)

// Handle the error later in `didFailProvisionalNavigation`
self.tab(for: webView)?.sslPinningError = error

return (.cancelAuthenticationChallenge, nil)
}
}
Expand All @@ -674,35 +677,38 @@ extension BrowserViewController: WKNavigationDelegate {
let protectionSpace = challenge.protectionSpace
let credential = challenge.proposedCredential
let previousFailureCount = challenge.previousFailureCount
return await Task { @MainActor in
guard protectionSpace.authenticationMethod == NSURLAuthenticationMethodHTTPBasic ||
protectionSpace.authenticationMethod == NSURLAuthenticationMethodHTTPDigest ||
protectionSpace.authenticationMethod == NSURLAuthenticationMethodNTLM,
let tab = tab(for: webView)
else {
return (.performDefaultHandling, nil)
}

// The challenge may come from a background tab, so ensure it's the one visible.
tabManager.selectTab(tab)

do {
let credentials = try await Authenticator.handleAuthRequest(
self,
credential: credential,
protectionSpace: protectionSpace,
previousFailureCount: previousFailureCount

guard
protectionSpace.authenticationMethod == NSURLAuthenticationMethodHTTPBasic
|| protectionSpace.authenticationMethod == NSURLAuthenticationMethodHTTPDigest
|| protectionSpace.authenticationMethod == NSURLAuthenticationMethodNTLM,
let tab = tab(for: webView)
else {
return (.performDefaultHandling, nil)
}

// The challenge may come from a background tab, so ensure it's the one visible.
tabManager.selectTab(tab)

do {
let credentials = try await Authenticator.handleAuthRequest(
self,
credential: credential,
protectionSpace: protectionSpace,
previousFailureCount: previousFailureCount
)

if BasicAuthCredentialsManager.validDomains.contains(host) {
BasicAuthCredentialsManager.setCredential(
origin: origin,
credential: credentials.credentials
)

if BasicAuthCredentialsManager.validDomains.contains(host) {
BasicAuthCredentialsManager.setCredential(origin: origin, credential: credentials.credentials)
}

return (.useCredential, credentials.credentials)
} catch {
return (.rejectProtectionSpace, nil)
}
}.value

return (.useCredential, credentials.credentials)
} catch {
return (.rejectProtectionSpace, nil)
}
}

public func webView(_ webView: WKWebView, didCommit navigation: WKNavigation!) {
Expand Down Expand Up @@ -817,17 +823,7 @@ 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.
observeValue(forKeyPath: KVOConstants.serverTrust.keyPath,
of: webView,
change: [.newKey: webView.serverTrust ?? tab.sslPinningTrust 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
Expand Down Expand Up @@ -858,20 +854,10 @@ 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 webView.serverTrust == nil, let serverTrust = tab.sslPinningTrust, 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)

if tab == self.tabManager.selectedTab {
self.topToolbar.hideProgressBar()
}

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
topToolbar.hideProgressBar()

// If the local web server isn't working for some reason (Brave cellular data is
// disabled in settings, for example), we'll fail to load the session restore URL.
Expand Down Expand Up @@ -912,55 +898,48 @@ extension BrowserViewController {
private func handleExternalURL(
_ url: URL,
tab: Tab?,
navigationAction: WKNavigationAction) async -> Bool {
// Do not open external links for child tabs automatically
// The user must tap on the link to open it.
if tab?.parent != nil && navigationAction.navigationType != .linkActivated {
return false
}

// Check if the current url of the caller has changed
if let domain = tab?.url?.baseDomain,
domain != tab?.externalAppURLDomain {
tab?.externalAppAlertCounter = 0
tab?.isExternalAppAlertSuppressed = false
}

tab?.externalAppURLDomain = tab?.url?.baseDomain

// Do not try to present over existing warning
if tab?.isExternalAppAlertPresented == true || tab?.isExternalAppAlertSuppressed == true {
return false
}

// External dialog should not be shown for non-active tabs #6687 - #7835
let isVisibleTab = tab?.isTabVisible() == true

// Check user trying to open on NTP like external link browsing
var isAboutHome = false
if let url = tab?.url {
isAboutHome = InternalURL(url)?.isAboutHomeURL == true
}

// Finally check non-active tab
let isNonActiveTab = isAboutHome ? false : tab?.url?.host != topToolbar.currentURL?.host

if !isVisibleTab || isNonActiveTab {
return false
}

var alertTitle = Strings.openExternalAppURLGenericTitle

if let displayHost = tab?.url?.withoutWWW.host {
alertTitle = String(format: Strings.openExternalAppURLTitle, displayHost)
}

// Handling condition when Tab is empty when handling an external URL we should remove the tab once the user decides
let removeTabIfEmpty = { [weak self] in
if let tab = tab, tab.url == nil {
self?.tabManager.removeTab(tab)
}
navigationAction: WKNavigationAction
) async -> Bool {
// Do not open external links for child tabs automatically
// The user must tap on the link to open it.
if tab?.parent != nil && navigationAction.navigationType != .linkActivated {
return false
}

// Check if the current url of the caller has changed
if let domain = tab?.url?.baseDomain,
domain != tab?.externalAppURLDomain
{
tab?.externalAppAlertCounter = 0
tab?.isExternalAppAlertSuppressed = false
}

tab?.externalAppURLDomain = tab?.url?.baseDomain

// Do not try to present over existing warning
if tab?.isExternalAppAlertPresented == true || tab?.isExternalAppAlertSuppressed == true {
return false
}

// External dialog should not be shown for non-active tabs #6687 - #7835
let isVisibleTab = tab?.isTabVisible() == true

if !isVisibleTab {
return false
}

var alertTitle = Strings.openExternalAppURLGenericTitle

if let displayHost = tab?.url?.withoutWWW.host {
alertTitle = String(format: Strings.openExternalAppURLTitle, displayHost)
}

// Handling condition when Tab is empty when handling an external URL we should remove the tab once the user decides
let removeTabIfEmpty = { [weak self] in
if let tab = tab, tab.url == nil {
self?.tabManager.removeTab(tab)
}
}

// Show the external sceheme invoke alert
@MainActor
Expand Down
Loading

0 comments on commit 09f5cd3

Please sign in to comment.