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.

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 #22343

Merged
merged 2 commits into from
Feb 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -301,10 +301,13 @@ extension BrowserViewController {
}

// Display Certificate Activity
if let selectedTab = tabManager.selectedTab,
selectedTab.secureContentState != .missingSSL && selectedTab.secureContentState != .unknown
if let tabURL = tabManager.selectedTab?.webView?.url,
tabManager.selectedTab?.webView?.serverTrust != nil
|| ErrorPageHelper.hasCertificates(for: tabURL)
{
logSecureContentState(tab: selectedTab, details: "Display Certificate Activity Settings")
if let selectedTab = tabManager.selectedTab {
logSecureContentState(tab: selectedTab, details: "Display Certificate Activity Settings")
}

activities.append(
BasicMenuActivity(
Expand All @@ -316,7 +319,7 @@ extension BrowserViewController {
)
}

// Report Web-compat Issue Actibity
// Report Web-compat Issue Activity
activities.append(
BasicMenuActivity(
title: Strings.Shields.reportABrokenSite,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,21 +69,6 @@ extension BrowserViewController: WKNavigationDelegate {
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
logSecureContentState(
tab: selectedTab,
details:
"DidStartProvisionalNavigation - Reset secure content state to unknown until page can be evaluated"
)

updateToolbarSecureContentState(.unknown)
}
}

// new site has a different origin, hide wallet icon.
tabManager.selectedTab?.isWalletIconVisible = false
Expand Down Expand Up @@ -746,7 +731,9 @@ extension BrowserViewController: WKNavigationDelegate {
download.delegate = self
}

nonisolated public func webView(
@MainActor

public func webView(
_ webView: WKWebView,
respondTo challenge: URLAuthenticationChallenge
) async -> (URLSession.AuthChallengeDisposition, URLCredential?) {
Expand All @@ -756,11 +743,16 @@ extension BrowserViewController: WKNavigationDelegate {
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)
let trust = challenge.protectionSpace.serverTrust
{
return (.useCredential, URLCredential(trust: trust))

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 @@ -782,13 +774,8 @@ extension BrowserViewController: WKNavigationDelegate {
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)
}

Expand All @@ -802,7 +789,7 @@ extension BrowserViewController: WKNavigationDelegate {
userInfo: ["_kCFStreamErrorCodeKey": Int(errorCode)]
)

let error = await NSError(
let error = NSError(
domain: kCFErrorDomainCFNetwork as String,
code: Int(errorCode),
userInfo: [
Expand All @@ -812,10 +799,8 @@ extension BrowserViewController: WKNavigationDelegate {
]
)

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

return (.cancelAuthenticationChallenge, nil)
}
Expand All @@ -825,39 +810,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)
guard
protectionSpace.authenticationMethod == NSURLAuthenticationMethodHTTPBasic
|| protectionSpace.authenticationMethod == NSURLAuthenticationMethodHTTPDigest
|| protectionSpace.authenticationMethod == NSURLAuthenticationMethodNTLM,
let tab = tab(for: webView)
else {
return (.performDefaultHandling, nil)
}

do {
let credentials = try await Authenticator.handleAuthRequest(
self,
credential: credential,
protectionSpace: protectionSpace,
previousFailureCount: previousFailureCount
)
// The challenge may come from a background tab, so ensure it's the one visible.
tabManager.selectTab(tab)

if BasicAuthCredentialsManager.validDomains.contains(host) {
BasicAuthCredentialsManager.setCredential(
origin: origin,
credential: credentials.credentials
)
}
do {
let credentials = try await Authenticator.handleAuthRequest(
self,
credential: credential,
protectionSpace: protectionSpace,
previousFailureCount: previousFailureCount
)

return (.useCredential, credentials.credentials)
} catch {
return (.rejectProtectionSpace, nil)
if BasicAuthCredentialsManager.validDomains.contains(host) {
BasicAuthCredentialsManager.setCredential(
origin: origin,
credential: credentials.credentials
)
}
}.value

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

public func webView(_ webView: WKWebView, didCommit navigation: WKNavigation!) {
Expand Down Expand Up @@ -991,20 +975,6 @@ extension BrowserViewController: WKNavigationDelegate {
) {
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.
logSecureContentState(tab: tab, details: "ObserveValue trigger in didFailProvisionalNavigation")

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 @@ -1036,23 +1006,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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bad rebase :o

// 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.
// We rely on loading that page to get the restore callback to reset the restoring
Expand Down Expand Up @@ -1118,16 +1075,7 @@ extension BrowserViewController {
// 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 {
if !isVisibleTab {
return false
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2071,8 +2071,10 @@ public class BrowserViewController: UIViewController {
tab.secureContentState = .unknown
logSecureContentState(tab: tab, path: path)

guard let serverTrust = tab.webView?.serverTrust else {
if let url = tab.webView?.url ?? tab.url {
guard let url = webView.url,
let serverTrust = webView.serverTrust
else {
if let url = webView.url {
if InternalURL.isValid(url: url),
let internalUrl = InternalURL(url),
internalUrl.isAboutURL || internalUrl.isAboutHomeURL
Expand Down Expand Up @@ -2166,8 +2168,8 @@ public class BrowserViewController: UIViewController {
break
}

guard let scheme = tab.webView?.url?.scheme,
let host = tab.webView?.url?.host
guard let scheme = url.scheme,
let host = url.host
else {
tab.secureContentState = .unknown
logSecureContentState(tab: tab, path: path, details: "No webview URL host scheme)")
Expand All @@ -2177,7 +2179,7 @@ public class BrowserViewController: UIViewController {
}

let port: Int
if let urlPort = tab.webView?.url?.port {
if let urlPort = url.port {
port = urlPort
} else if scheme == "https" {
port = 443
Expand Down Expand Up @@ -2315,15 +2317,8 @@ public class BrowserViewController: UIViewController {
browser.tabManager.addTabsForURLs([url], zombie: false, isPrivate: isPrivate)
}

public func switchToTabForURLOrOpen(
_ url: URL,
isPrivate: Bool = false,
isPrivileged: Bool,
isExternal: Bool = false
) {
if !isExternal {
popToBVC()
}
public func switchToTabForURLOrOpen(_ url: URL, isPrivate: Bool = false, isPrivileged: Bool) {
popToBVC(isAnimated: false)

if let tab = tabManager.getTabForURL(url, isPrivate: isPrivate) {
tabManager.selectTab(tab)
Expand Down Expand Up @@ -2461,11 +2456,11 @@ public class BrowserViewController: UIViewController {
present(settingsNavigationController, animated: true)
}

func popToBVC(completion: (() -> Void)? = nil) {
func popToBVC(isAnimated: Bool = true, completion: (() -> Void)? = nil) {
guard let currentViewController = navigationController?.topViewController else {
return
}
currentViewController.dismiss(animated: true, completion: completion)
currentViewController.dismiss(animated: isAnimated, completion: completion)

if currentViewController != self {
_ = self.navigationController?.popViewController(animated: true)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,6 @@ class ErrorPageHelper {
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
Expand Down Expand Up @@ -186,6 +181,10 @@ extension ErrorPageHelper {
return 0
}

static func hasCertificates(for url: URL) -> Bool {
return (url as NSURL).valueForQueryParameter(key: "badcerts") != nil
}

static func serverTrust(from errorURL: URL) throws -> SecTrust? {
guard let internalUrl = InternalURL(errorURL),
internalUrl.isErrorPage,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@ class CertificateErrorPageHandler: InterstitialPageHandler {
}

func response(for model: ErrorPageModel) -> (URLResponse, Data)? {
let hasCertificate =
model.components.valueForQuery("certerror") != nil
&& model.components.valueForQuery("peeruntrusted") == nil
let hasCertificate = model.components.valueForQuery("certerror") != nil

guard let asset = Bundle.module.path(forResource: "CertificateError", ofType: "html") else {
assert(false)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,7 @@ public enum NavigationPath: Equatable {

private static func handleURL(url: URL?, isPrivate: Bool, with bvc: BrowserViewController) {
if let newURL = url {
bvc.switchToTabForURLOrOpen(
newURL,
isPrivate: isPrivate,
isPrivileged: false,
isExternal: true
)
bvc.popToBVC()
bvc.switchToTabForURLOrOpen(newURL, isPrivate: isPrivate, isPrivileged: false)
} else {
bvc.openBlankNewTab(attemptLocationFieldFocus: false, isPrivate: isPrivate)
}
Expand Down
1 change: 0 additions & 1 deletion ios/brave-ios/Sources/Brave/Frontend/Browser/Tab.swift
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ class Tab: NSObject {

var secureContentState: TabSecureContentState = .unknown
var sslPinningError: Error?
var sslPinningTrust: SecTrust?

private let _syncTab: BraveSyncTab?
private let _faviconDriver: FaviconDriver?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ extension BraveCertificateUtils {
return serverTrust!
}

/// Verifies ServerTrust using Apple's APIs which validates also the X509 Certificate against the System Trusts
public static func evaluateTrust(_ trust: SecTrust, for host: String?) async throws {
try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<Void, Error>) in
BraveCertificateUtils.evaluationQueue.async {
Expand All @@ -241,6 +242,7 @@ extension BraveCertificateUtils {
}
}

/// Verifies ServerTrust using Brave-Core which verifies only SSL Pinning Status
public static func verifyTrust(_ trust: SecTrust, host: String, port: Int) async -> Int {
return Int(BraveCertificateUtility.verifyTrust(trust, host: host, port: port))
}
Expand Down
Loading