Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[iOS] Url bar falsely showing insecure state (uplift to 1.63.x) #22380

Merged
merged 1 commit into from
Mar 1, 2024
Merged

[iOS] Url bar falsely showing insecure state (uplift to 1.63.x) #22380

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions ios/brave-ios/Sources/Brave/Frontend/Browser/BrowserViewController/BVC+ShareActivity.swift
View file
Open in desktop
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
Open in desktop
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
Loading