Skip to content

Commit 694aa3f

Browse files
Brandon-Tsoner-yuksel
authored andcommitted
Fix bugs with secureContentState being set in didFailProvisionalNavigation as URL is only ever updated in didCommit so there's a mismatch. This should NEVER be done.
Fix external URLs not working due to inactive tab logic. Fix security certificate display showing when there's no cert at all. Remove serverPinningTrust because it can mismatch in didFailProvisionalNavigation when an AppStore URL is loaded on top of a already secure page URL, then the appstore URL assumes the cert of the page which is wrong. Apple gives us no cert on purpose so we should not store the one from chain evaluation
1 parent 2e771f1 commit 694aa3f

File tree

7 files changed

+77
-137
lines changed

7 files changed

+77
-137
lines changed

ios/brave-ios/Sources/Brave/Frontend/Browser/BrowserViewController/BVC+ShareActivity.swift

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -301,10 +301,10 @@ extension BrowserViewController {
301301
}
302302

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

309309
activities.append(
310310
BasicMenuActivity(

ios/brave-ios/Sources/Brave/Frontend/Browser/BrowserViewController/BVC+WKNavigationDelegate.swift

Lines changed: 56 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -67,24 +67,8 @@ extension BrowserViewController: WKNavigationDelegate {
6767

6868
// check if web view is loading a different origin than the one currently loaded
6969
if let selectedTab = tabManager.selectedTab,
70-
selectedTab.url?.origin != webView.url?.origin
71-
{
72-
if let url = webView.url {
73-
if !InternalURL.isValid(url: url) {
74-
// reset secure content state to unknown until page can be evaluated
75-
selectedTab.sslPinningError = nil
76-
selectedTab.sslPinningTrust = nil
77-
selectedTab.secureContentState = .unknown
78-
logSecureContentState(
79-
tab: selectedTab,
80-
details:
81-
"DidStartProvisionalNavigation - Reset secure content state to unknown until page can be evaluated"
82-
)
83-
84-
updateToolbarSecureContentState(.unknown)
85-
}
86-
}
87-
70+
selectedTab.url?.origin != webView.url?.origin {
71+
8872
// new site has a different origin, hide wallet icon.
8973
tabManager.selectedTab?.isWalletIconVisible = false
9074
// new site, reset connected addresses
@@ -746,21 +730,23 @@ extension BrowserViewController: WKNavigationDelegate {
746730
download.delegate = self
747731
}
748732

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

734+
@MainActor
735+
public func webView(_ webView: WKWebView, respondTo challenge: URLAuthenticationChallenge) async -> (URLSession.AuthChallengeDisposition, URLCredential?) {
754736
// If this is a certificate challenge, see if the certificate has previously been
755737
// accepted by the user.
756738
let host = challenge.protectionSpace.host
757739
let origin = "\(host):\(challenge.protectionSpace.port)"
758740
if challenge.protectionSpace.authenticationMethod == NSURLAuthenticationMethodServerTrust,
759-
let trust = challenge.protectionSpace.serverTrust,
760-
let cert = (SecTrustCopyCertificateChain(trust) as? [SecCertificate])?.first,
761-
profile.certStore.containsCertificate(cert, forOrigin: origin)
762-
{
763-
return (.useCredential, URLCredential(trust: trust))
741+
let trust = challenge.protectionSpace.serverTrust {
742+
743+
let cert = await Task<SecCertificate?, Never>.detached {
744+
return (SecTrustCopyCertificateChain(trust) as? [SecCertificate])?.first
745+
}.value
746+
747+
if let cert = cert, profile.certStore.containsCertificate(cert, forOrigin: origin) {
748+
return (.useCredential, URLCredential(trust: trust))
749+
}
764750
}
765751

766752
// Certificate Pinning
@@ -782,41 +768,28 @@ extension BrowserViewController: WKNavigationDelegate {
782768
if result == Int32.min {
783769
// Cert is POTENTIALLY invalid and cannot be pinned
784770

785-
await MainActor.run {
786-
// Handle the potential error later in `didFailProvisionalNavigation`
787-
self.tab(for: webView)?.sslPinningTrust = serverTrust
788-
}
789-
790771
// Let WebKit handle the request and validate the cert
791-
// This is the same as calling `BraveCertificateUtils.evaluateTrust`
772+
// This is the same as calling `BraveCertificateUtils.evaluateTrust` but with more error info provided by WebKit
792773
return (.performDefaultHandling, nil)
793774
}
794775

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

799-
let underlyingError = NSError(
800-
domain: kCFErrorDomainCFNetwork as String,
801-
code: Int(errorCode),
802-
userInfo: ["_kCFStreamErrorCodeKey": Int(errorCode)]
803-
)
804-
805-
let error = await NSError(
806-
domain: kCFErrorDomainCFNetwork as String,
807-
code: Int(errorCode),
808-
userInfo: [
809-
NSURLErrorFailingURLErrorKey: webView.url as Any,
810-
"NSErrorPeerCertificateChainKey": certificateChain,
811-
NSUnderlyingErrorKey: underlyingError,
812-
]
813-
)
814-
815-
await MainActor.run {
816-
// Handle the error later in `didFailProvisionalNavigation`
817-
self.tab(for: webView)?.sslPinningError = error
818-
}
819-
780+
let underlyingError = NSError(domain: kCFErrorDomainCFNetwork as String,
781+
code: Int(errorCode),
782+
userInfo: ["_kCFStreamErrorCodeKey": Int(errorCode)])
783+
784+
let error = NSError(domain: kCFErrorDomainCFNetwork as String,
785+
code: Int(errorCode),
786+
userInfo: [NSURLErrorFailingURLErrorKey: webView.url as Any,
787+
"NSErrorPeerCertificateChainKey": certificateChain,
788+
NSUnderlyingErrorKey: underlyingError])
789+
790+
// Handle the error later in `didFailProvisionalNavigation`
791+
self.tab(for: webView)?.sslPinningError = error
792+
820793
return (.cancelAuthenticationChallenge, nil)
821794
}
822795
}
@@ -825,39 +798,34 @@ extension BrowserViewController: WKNavigationDelegate {
825798
let protectionSpace = challenge.protectionSpace
826799
let credential = challenge.proposedCredential
827800
let previousFailureCount = challenge.previousFailureCount
828-
return await Task { @MainActor in
829-
guard
830-
protectionSpace.authenticationMethod == NSURLAuthenticationMethodHTTPBasic
831-
|| protectionSpace.authenticationMethod == NSURLAuthenticationMethodHTTPDigest
832-
|| protectionSpace.authenticationMethod == NSURLAuthenticationMethodNTLM,
833-
let tab = tab(for: webView)
834-
else {
835-
return (.performDefaultHandling, nil)
836-
}
837801

838-
// The challenge may come from a background tab, so ensure it's the one visible.
839-
tabManager.selectTab(tab)
840-
841-
do {
842-
let credentials = try await Authenticator.handleAuthRequest(
843-
self,
844-
credential: credential,
845-
protectionSpace: protectionSpace,
846-
previousFailureCount: previousFailureCount
847-
)
848-
849-
if BasicAuthCredentialsManager.validDomains.contains(host) {
850-
BasicAuthCredentialsManager.setCredential(
851-
origin: origin,
852-
credential: credentials.credentials
853-
)
854-
}
855-
856-
return (.useCredential, credentials.credentials)
857-
} catch {
858-
return (.rejectProtectionSpace, nil)
802+
guard protectionSpace.authenticationMethod == NSURLAuthenticationMethodHTTPBasic ||
803+
protectionSpace.authenticationMethod == NSURLAuthenticationMethodHTTPDigest ||
804+
protectionSpace.authenticationMethod == NSURLAuthenticationMethodNTLM,
805+
let tab = tab(for: webView)
806+
else {
807+
return (.performDefaultHandling, nil)
808+
}
809+
810+
// The challenge may come from a background tab, so ensure it's the one visible.
811+
tabManager.selectTab(tab)
812+
813+
do {
814+
let credentials = try await Authenticator.handleAuthRequest(
815+
self,
816+
credential: credential,
817+
protectionSpace: protectionSpace,
818+
previousFailureCount: previousFailureCount
819+
)
820+
821+
if BasicAuthCredentialsManager.validDomains.contains(host) {
822+
BasicAuthCredentialsManager.setCredential(origin: origin, credential: credentials.credentials)
859823
}
860-
}.value
824+
825+
return (.useCredential, credentials.credentials)
826+
} catch {
827+
return (.rejectProtectionSpace, nil)
828+
}
861829
}
862830

863831
public func webView(_ webView: WKWebView, didCommit navigation: WKNavigation!) {
@@ -991,20 +959,6 @@ extension BrowserViewController: WKNavigationDelegate {
991959
) {
992960
guard let tab = tab(for: webView) else { return }
993961

994-
// WebKit does not update certs on cancellation of a frame load
995-
// So manually trigger the notification with the current cert
996-
// Also, when Chromium cert validation passes, BUT Apple cert validation fails, the request is cancelled automatically by WebKit
997-
// In such a case, the webView.serverTrust is `nil`. The only time we have a valid trust is when we received the challenge
998-
// so we need to update the URL-Bar to show that serverTrust when WebKit's is nil.
999-
logSecureContentState(tab: tab, details: "ObserveValue trigger in didFailProvisionalNavigation")
1000-
1001-
observeValue(
1002-
forKeyPath: KVOConstants.serverTrust.keyPath,
1003-
of: webView,
1004-
change: [.newKey: webView.serverTrust ?? tab.sslPinningTrust as Any, .kindKey: 1],
1005-
context: nil
1006-
)
1007-
1008962
// Ignore the "Frame load interrupted" error that is triggered when we cancel a request
1009963
// to open an external application and hand it over to UIApplication.openURL(). The result
1010964
// will be that we switch to the external app, for example the app store, while keeping the
@@ -1036,23 +990,10 @@ extension BrowserViewController: WKNavigationDelegate {
1036990

1037991
if let url = error.userInfo[NSURLErrorFailingURLErrorKey] as? URL {
1038992

1039-
// The certificate came from the WebKit SSL Handshake validation and the cert is untrusted
1040-
if webView.serverTrust == nil, let serverTrust = tab.sslPinningTrust,
1041-
error.userInfo["NSErrorPeerCertificateChainKey"] == nil
1042-
{
1043-
// Build a cert chain error to display in the cert viewer in such cases, as we aren't given one by WebKit
1044-
var userInfo = error.userInfo
1045-
userInfo["NSErrorPeerCertificateChainKey"] =
1046-
SecTrustCopyCertificateChain(serverTrust) as? [SecCertificate] ?? []
1047-
userInfo["NSErrorPeerUntrustedByApple"] = true
1048-
error = NSError(domain: error.domain, code: error.code, userInfo: userInfo)
993+
if tab == self.tabManager.selectedTab {
994+
self.topToolbar.hideProgressBar()
1049995
}
1050-
1051-
ErrorPageHelper(certStore: profile.certStore).loadPage(error, forUrl: url, inWebView: webView)
1052-
// Submitting same errornous URL using toolbar will cause progress bar get stuck
1053-
// Reseting the progress bar in case there is an error is necessary
1054-
topToolbar.hideProgressBar()
1055-
996+
1056997
// If the local web server isn't working for some reason (Brave cellular data is
1057998
// disabled in settings, for example), we'll fail to load the session restore URL.
1058999
// We rely on loading that page to get the restore callback to reset the restoring

ios/brave-ios/Sources/Brave/Frontend/Browser/BrowserViewController/BrowserViewController.swift

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2071,8 +2071,9 @@ public class BrowserViewController: UIViewController {
20712071
tab.secureContentState = .unknown
20722072
logSecureContentState(tab: tab, path: path)
20732073

2074-
guard let serverTrust = tab.webView?.serverTrust else {
2075-
if let url = tab.webView?.url ?? tab.url {
2074+
guard let url = webView.url,
2075+
let serverTrust = webView.serverTrust else {
2076+
if let url = webView.url {
20762077
if InternalURL.isValid(url: url),
20772078
let internalUrl = InternalURL(url),
20782079
internalUrl.isAboutURL || internalUrl.isAboutHomeURL
@@ -2165,10 +2166,9 @@ public class BrowserViewController: UIViewController {
21652166
}
21662167
break
21672168
}
2168-
2169-
guard let scheme = tab.webView?.url?.scheme,
2170-
let host = tab.webView?.url?.host
2171-
else {
2169+
2170+
guard let scheme = url.scheme,
2171+
let host = url.host else {
21722172
tab.secureContentState = .unknown
21732173
logSecureContentState(tab: tab, path: path, details: "No webview URL host scheme)")
21742174

@@ -2177,7 +2177,7 @@ public class BrowserViewController: UIViewController {
21772177
}
21782178

21792179
let port: Int
2180-
if let urlPort = tab.webView?.url?.port {
2180+
if let urlPort = url.port {
21812181
port = urlPort
21822182
} else if scheme == "https" {
21832183
port = 443

ios/brave-ios/Sources/Brave/Frontend/Browser/Helpers/ErrorPageHelper.swift

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -104,11 +104,6 @@ class ErrorPageHelper {
104104
URLQueryItem(name: "timestamp", value: "\(Int(Date().timeIntervalSince1970 * 1000))"),
105105
]
106106

107-
// The error came from WebKit's internal validation and the cert is untrusted
108-
if error.userInfo["NSErrorPeerUntrustedByApple"] as? Bool == true {
109-
queryItems.append(URLQueryItem(name: "peeruntrusted", value: "true"))
110-
}
111-
112107
// If this is an invalid certificate, show a certificate error allowing the
113108
// user to go back or continue. The certificate itself is encoded and added as
114109
// a query parameter to the error page URL; we then read the certificate from
@@ -186,6 +181,10 @@ extension ErrorPageHelper {
186181
return 0
187182
}
188183

184+
static func hasCertificates(for url: URL) -> Bool {
185+
return (url as NSURL).valueForQueryParameter(key: "badcerts") != nil
186+
}
187+
189188
static func serverTrust(from errorURL: URL) throws -> SecTrust? {
190189
guard let internalUrl = InternalURL(errorURL),
191190
internalUrl.isErrorPage,

ios/brave-ios/Sources/Brave/Frontend/Browser/Interstitial Pages/CertificateErrorPageHandler.swift

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,7 @@ class CertificateErrorPageHandler: InterstitialPageHandler {
1717
}
1818

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

2422
guard let asset = Bundle.module.path(forResource: "CertificateError", ofType: "html") else {
2523
assert(false)

ios/brave-ios/Sources/Brave/Frontend/Browser/Tab.swift

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,6 @@ class Tab: NSObject {
103103

104104
var secureContentState: TabSecureContentState = .unknown
105105
var sslPinningError: Error?
106-
var sslPinningTrust: SecTrust?
107106

108107
private let _syncTab: BraveSyncTab?
109108
private let _faviconDriver: FaviconDriver?

ios/brave-ios/Sources/CertificateUtilities/BraveCertificateUtils.swift

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,8 @@ extension BraveCertificateUtils {
220220
return serverTrust!
221221
}
222222

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

244-
public static func verifyTrust(_ trust: SecTrust, host: String, port: Int) async -> Int {
245+
/// Verifies ServerTrust using Brave-Core which verifies only SSL Pinning Status
246+
static func verifyTrust(_ trust: SecTrust, host: String, port: Int) async -> Int {
247+
245248
return Int(BraveCertificateUtility.verifyTrust(trust, host: host, port: port))
246249
}
247250
}

0 commit comments

Comments
 (0)