From bb3e5c4a0aa1ad171f87a82ef402c55956679381 Mon Sep 17 00:00:00 2001 From: Sam Symons Date: Thu, 31 Oct 2024 18:00:30 -0700 Subject: [PATCH 1/6] Validate errors before re-throwing them to the OS. --- .../PacketTunnelProvider.swift | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index fb133916c..bc55e35a9 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -692,6 +692,9 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { throw TunnelError.startingTunnelWithoutAuthToken } } catch { + // Check that the error is valid and able to be re-thrown to the OS before shutting the tunnel down + let error = validated(error: error) + if startupOptions.startupMethod == .automaticOnDemand { // If the VPN was started by on-demand without the basic prerequisites for // it to work we skip firing pixels. This should only be possible if the @@ -723,6 +726,9 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { providerEvents.fire(.tunnelStartAttempt(.success)) } catch { + // Check that the error is valid and able to be re-thrown to the OS before shutting the tunnel down + let error = validated(error: error) + if startupOptions.startupMethod == .automaticOnDemand { // We add a delay when the VPN is started by // on-demand and there's an error, to avoid frenetic ON/OFF @@ -765,6 +771,8 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { private func startTunnel(onDemand: Bool) async throws { do { + throw ErrorThatCrashes.crash + Logger.networkProtection.log("Generating tunnel config") Logger.networkProtection.log("Excluded ranges are: \(String(describing: self.settings.excludedRanges), privacy: .public)") Logger.networkProtection.log("Server selection method: \(self.currentServerSelectionMethod.debugDescription, privacy: .public)") @@ -1815,6 +1823,56 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { snoozeTimingStore.reset() } + // MARK: - Error Validation + + enum InvalidDiagnosticError: Error, CustomNSError { + case errorWithInvalidUnderlyingError(Error) + + var errorCode: Int { + switch self { + case .errorWithInvalidUnderlyingError(let error): + return (error as NSError).code + } + } + + var localizedDescription: String { + switch self { + case .errorWithInvalidUnderlyingError(let error): + return "Error '\(type(of: error))', message: \(error.localizedDescription)" + } + } + + var errorUserInfo: [String: Any] { + switch self { + case .errorWithInvalidUnderlyingError(let error): + let newError = NSError(domain: (error as NSError).domain, code: (error as NSError).code) + return [NSUnderlyingErrorKey: newError] + } + } + } + + /// Validates that an error object is correctly structured; i.e., only uses an `NSError` instances for its underlying error, etc. + private func validated(error: Error) -> Error { + if containsValidUnderlyingError(error) { + return error + } else { + return InvalidDiagnosticError.errorWithInvalidUnderlyingError(error) + } + } + + private func containsValidUnderlyingError(_ error: Error) -> Bool { + let nsError = error as NSError + + if let underlyingError = nsError.userInfo[NSUnderlyingErrorKey] as? Error { + return containsValidUnderlyingError(underlyingError) + } else if nsError.userInfo[NSUnderlyingErrorKey] != nil { + // If `NSUnderlyingErrorKey` exists but is not an `Error`, return false + return false + } + + return true + } + } extension WireGuardAdapterError: LocalizedError, CustomDebugStringConvertible { From aff5f0ca16cc9861157014c96752ec366d4f2c15 Mon Sep 17 00:00:00 2001 From: Sam Symons Date: Thu, 31 Oct 2024 18:01:26 -0700 Subject: [PATCH 2/6] Remove temporary error for testing. --- Sources/NetworkProtection/PacketTunnelProvider.swift | 2 -- 1 file changed, 2 deletions(-) diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index bc55e35a9..9dcd6ff96 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -771,8 +771,6 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { private func startTunnel(onDemand: Bool) async throws { do { - throw ErrorThatCrashes.crash - Logger.networkProtection.log("Generating tunnel config") Logger.networkProtection.log("Excluded ranges are: \(String(describing: self.settings.excludedRanges), privacy: .public)") Logger.networkProtection.log("Server selection method: \(self.currentServerSelectionMethod.debugDescription, privacy: .public)") From 7ab60251fc34ccd98228e317a698542631408b1b Mon Sep 17 00:00:00 2001 From: Sam Symons Date: Thu, 31 Oct 2024 22:25:45 -0700 Subject: [PATCH 3/6] Only validate the error when throwing it. --- .../NetworkProtection/PacketTunnelProvider.swift | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index 9dcd6ff96..2b70ce5a9 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -692,9 +692,6 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { throw TunnelError.startingTunnelWithoutAuthToken } } catch { - // Check that the error is valid and able to be re-thrown to the OS before shutting the tunnel down - let error = validated(error: error) - if startupOptions.startupMethod == .automaticOnDemand { // If the VPN was started by on-demand without the basic prerequisites for // it to work we skip firing pixels. This should only be possible if the @@ -713,7 +710,8 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { Logger.networkProtection.log("🔴 Stopping VPN due to no auth token") await attemptShutdownDueToRevokedAccess() - throw error + // Check that the error is valid and able to be re-thrown to the OS before shutting the tunnel down + throw validated(error: error) } do { @@ -726,9 +724,6 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { providerEvents.fire(.tunnelStartAttempt(.success)) } catch { - // Check that the error is valid and able to be re-thrown to the OS before shutting the tunnel down - let error = validated(error: error) - if startupOptions.startupMethod == .automaticOnDemand { // We add a delay when the VPN is started by // on-demand and there's an error, to avoid frenetic ON/OFF @@ -743,7 +738,9 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { self.knownFailureStore.lastKnownFailure = KnownFailure(error) providerEvents.fire(.tunnelStartAttempt(.failure(error))) - throw error + + // Check that the error is valid and able to be re-thrown to the OS before shutting the tunnel down + throw validated(error: error) } } From 72d59ee0ca070188c56a6bcced19416d01ed0e3a Mon Sep 17 00:00:00 2001 From: Sam Symons Date: Thu, 31 Oct 2024 22:30:50 -0700 Subject: [PATCH 4/6] Fix SwiftLint. --- Sources/NetworkProtection/PacketTunnelProvider.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index 2b70ce5a9..5652030ff 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -738,7 +738,7 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { self.knownFailureStore.lastKnownFailure = KnownFailure(error) providerEvents.fire(.tunnelStartAttempt(.failure(error))) - + // Check that the error is valid and able to be re-thrown to the OS before shutting the tunnel down throw validated(error: error) } From 1f971fb6e8c1e73f981accf4e717f79c07418532 Mon Sep 17 00:00:00 2001 From: Sam Symons Date: Fri, 1 Nov 2024 15:59:05 -0700 Subject: [PATCH 5/6] Wrap malformed error objects before throwing them. --- .../PacketTunnelProvider.swift | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index 5652030ff..a8cfbacd4 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -711,7 +711,12 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { await attemptShutdownDueToRevokedAccess() // Check that the error is valid and able to be re-thrown to the OS before shutting the tunnel down - throw validated(error: error) + if let wrappedError = wrapped(error: error) { + providerEvents.fire(.malformedErrorDetected(error)) + throw wrappedError + } else { + throw error + } } do { @@ -740,7 +745,12 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { providerEvents.fire(.tunnelStartAttempt(.failure(error))) // Check that the error is valid and able to be re-thrown to the OS before shutting the tunnel down - throw validated(error: error) + if let wrappedError = wrapped(error: error) { + providerEvents.fire(.malformedErrorDetected(error)) + throw wrappedError + } else { + throw error + } } } @@ -1846,10 +1856,10 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { } } - /// Validates that an error object is correctly structured; i.e., only uses an `NSError` instances for its underlying error, etc. - private func validated(error: Error) -> Error { + /// Wraps an error instance in a new error type in cases where it is malformed; i.e., doesn't use an `NSError` instance for its underlying error, etc. + private func wrapped(error: Error) -> Error? { if containsValidUnderlyingError(error) { - return error + return nil } else { return InvalidDiagnosticError.errorWithInvalidUnderlyingError(error) } From 5a9de1c0958ddb8127c139dde1926620f45740cf Mon Sep 17 00:00:00 2001 From: Sam Symons Date: Fri, 1 Nov 2024 16:05:58 -0700 Subject: [PATCH 6/6] Check in enum case. --- Sources/NetworkProtection/PacketTunnelProvider.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Sources/NetworkProtection/PacketTunnelProvider.swift b/Sources/NetworkProtection/PacketTunnelProvider.swift index a8cfbacd4..db94999bf 100644 --- a/Sources/NetworkProtection/PacketTunnelProvider.swift +++ b/Sources/NetworkProtection/PacketTunnelProvider.swift @@ -42,6 +42,7 @@ open class PacketTunnelProvider: NEPacketTunnelProvider { case rekeyAttempt(_ step: RekeyAttemptStep) case failureRecoveryAttempt(_ step: FailureRecoveryStep) case serverMigrationAttempt(_ step: ServerMigrationAttemptStep) + case malformedErrorDetected(_ error: Error) } public enum AttemptStep: CustomDebugStringConvertible {