Merge pull request from GHSA-9cfh-vx93-84vv

* Ensure empty incoming buffer when TLS negotiation starts

* cleanup

* Use real nio version

Author: fabianfett

Package.swift

@@ -14,7 +14,7 @@ let package = Package(
     ],
     dependencies: [
         .package(url: "https://github.com/apple/swift-atomics.git", from: "1.1.0"),
-        .package(url: "https://github.com/apple/swift-nio.git", from: "2.51.1"),
+        .package(url: "https://github.com/apple/swift-nio.git", from: "2.52.0"),
         .package(url: "https://github.com/apple/swift-nio-transport-services.git", from: "1.16.0"),
         .package(url: "https://github.com/apple/swift-nio-ssl.git", from: "2.23.1"),
         .package(url: "https://github.com/apple/swift-crypto.git", "1.0.0" ..< "3.0.0"),

Sources/PostgresNIO/New/Connection State Machine/ConnectionStateMachine.swift

@@ -228,9 +228,12 @@ struct ConnectionStateMachine {
         }
     }
 
-    mutating func sslSupportedReceived() -> ConnectionAction {
+    mutating func sslSupportedReceived(unprocessedBytes: Int) -> ConnectionAction {
         switch self.state {
         case .sslRequestSent:
+            if unprocessedBytes > 0 {
+                return self.closeConnectionAndCleanup(.receivedUnencryptedDataAfterSSLRequest)
+            }
             self.state = .sslNegotiated
             return .establishSSLConnection
 
@@ -1079,9 +1082,18 @@ extension ConnectionStateMachine {
 extension ConnectionStateMachine {
     func shouldCloseConnection(reason error: PSQLError) -> Bool {
         switch error.code.base {
-        case .sslUnsupported:
-            return true
-        case .failedToAddSSLHandler:
+        case .failedToAddSSLHandler,
+             .receivedUnencryptedDataAfterSSLRequest,
+             .sslUnsupported,
+             .messageDecodingFailure,
+             .unexpectedBackendMessage,
+             .unsupportedAuthMechanism,
+             .authMechanismRequiresPassword,
+             .saslError,
+             .tooManyParameters,
+             .invalidCommandTag,
+             .connectionError,
+             .uncleanShutdown:
             return true
         case .queryCancelled:
             return false
@@ -1097,28 +1109,10 @@ extension ConnectionStateMachine {
             }
 
             return false
-        case .messageDecodingFailure:
-            return true
-        case .unexpectedBackendMessage:
-            return true
-        case .unsupportedAuthMechanism:
-            return true
-        case .authMechanismRequiresPassword:
-            return true
-        case .saslError:
-            return true
-        case .tooManyParameters:
-            return true
-        case .invalidCommandTag:
-            return true
         case .connectionQuiescing:
             preconditionFailure("Pure client error, that is thrown directly in PostgresConnection")
         case .connectionClosed:
             preconditionFailure("Pure client error, that is thrown directly and should never ")
-        case .connectionError:
-            return true
-        case .uncleanShutdown:
-            return true
         }
     }
 

Sources/PostgresNIO/New/PSQLError.swift

@@ -7,6 +7,7 @@ public struct PSQLError: Error {
         enum Base: Sendable, Hashable {
             case sslUnsupported
             case failedToAddSSLHandler
+            case receivedUnencryptedDataAfterSSLRequest
             case server
             case messageDecodingFailure
             case unexpectedBackendMessage
@@ -31,6 +32,7 @@ public struct PSQLError: Error {
 
         public static let sslUnsupported = Self.init(.sslUnsupported)
         public static let failedToAddSSLHandler = Self(.failedToAddSSLHandler)
+        public static let receivedUnencryptedDataAfterSSLRequest = Self(.receivedUnencryptedDataAfterSSLRequest)
         public static let server = Self(.server)
         public static let messageDecodingFailure = Self(.messageDecodingFailure)
         public static let unexpectedBackendMessage = Self(.unexpectedBackendMessage)
@@ -51,6 +53,8 @@ public struct PSQLError: Error {
                 return "sslUnsupported"
             case .failedToAddSSLHandler:
                 return "failedToAddSSLHandler"
+            case .receivedUnencryptedDataAfterSSLRequest:
+                return "receivedUnencryptedDataAfterSSLRequest"
             case .server:
                 return "server"
             case .messageDecodingFailure:
@@ -343,6 +347,8 @@ public struct PSQLError: Error {
 
     static var uncleanShutdown: PSQLError { PSQLError(code: .uncleanShutdown) }
 
+    static var receivedUnencryptedDataAfterSSLRequest: PSQLError { PSQLError(code: .receivedUnencryptedDataAfterSSLRequest) }
+
     static func server(_ response: PostgresBackendMessage.ErrorResponse) -> PSQLError {
         var error = PSQLError(code: .server)
         error.serverInfo = .init(response)

Sources/PostgresNIO/New/PostgresChannelHandler.swift

@@ -139,7 +139,7 @@ final class PostgresChannelHandler: ChannelDuplexHandler {
                 case .rowDescription(let rowDescription):
                     action = self.state.rowDescriptionReceived(rowDescription)
                 case .sslSupported:
-                    action = self.state.sslSupportedReceived()
+                    action = self.state.sslSupportedReceived(unprocessedBytes: self.decoder.unprocessedBytes)
                 case .sslUnsupported:
                     action = self.state.sslUnsupportedReceived()
                 }

Sources/PostgresNIO/Postgres+PSQLCompat.swift

@@ -31,6 +31,8 @@ extension PSQLError {
             return PostgresError.protocol("Unsupported auth scheme: \(message)")
         case .authMechanismRequiresPassword:
             return PostgresError.protocol("Unable to authenticate without password")
+        case .receivedUnencryptedDataAfterSSLRequest:
+            return PostgresError.protocol("Received unencrypted data after SSL request")
         case .saslError:
             return self.underlying ?? self
         case .tooManyParameters, .invalidCommandTag:

Tests/PostgresNIOTests/New/Connection State Machine/ConnectionStateMachineTests.swift

@@ -19,20 +19,27 @@ class ConnectionStateMachineTests: XCTestCase {
         let authContext = AuthContext(username: "test", password: "abc123", database: "test")
         var state = ConnectionStateMachine(requireBackendKeyData: true)
         XCTAssertEqual(state.connected(tls: .require), .sendSSLRequest)
-        XCTAssertEqual(state.sslSupportedReceived(), .establishSSLConnection)
+        XCTAssertEqual(state.sslSupportedReceived(unprocessedBytes: 0), .establishSSLConnection)
         XCTAssertEqual(state.sslHandlerAdded(), .wait)
         XCTAssertEqual(state.sslEstablished(), .provideAuthenticationContext)
         XCTAssertEqual(state.provideAuthenticationContext(authContext), .sendStartupMessage(authContext))
         let salt: (UInt8, UInt8, UInt8, UInt8) = (0,1,2,3)
         XCTAssertEqual(state.authenticationMessageReceived(.md5(salt: salt)), .sendPasswordMessage(.md5(salt: salt), authContext))
     }
-    
+
+    func testSSLStartupFailureTooManyBytesRemaining() {
+        var state = ConnectionStateMachine(requireBackendKeyData: true)
+        XCTAssertEqual(state.connected(tls: .require), .sendSSLRequest)
+        let failError = PSQLError.receivedUnencryptedDataAfterSSLRequest
+        XCTAssertEqual(state.sslSupportedReceived(unprocessedBytes: 1), .closeConnectionAndCleanup(.init(action: .close, tasks: [], error: failError, closePromise: nil)))
+    }
+
     func testSSLStartupFailHandler() {
         struct SSLHandlerAddError: Error, Equatable {}
 
         var state = ConnectionStateMachine(requireBackendKeyData: true)
         XCTAssertEqual(state.connected(tls: .require), .sendSSLRequest)
-        XCTAssertEqual(state.sslSupportedReceived(), .establishSSLConnection)
+        XCTAssertEqual(state.sslSupportedReceived(unprocessedBytes: 0), .establishSSLConnection)
         let failError = PSQLError.failedToAddSSLHandler(underlying: SSLHandlerAddError())
         XCTAssertEqual(state.errorHappened(failError), .closeConnectionAndCleanup(.init(action: .close, tasks: [], error: failError, closePromise: nil)))
     }

Tests/PostgresNIOTests/New/PostgresChannelHandlerTests.swift

@@ -77,7 +77,44 @@ class PostgresChannelHandlerTests: XCTestCase {
         XCTAssertEqual(startupMessage.parameters.database, config.database)
         XCTAssertEqual(startupMessage.parameters.replication, .false)
     }
-    
+
+    func testEstablishSSLCallbackIsNotCalledIfSSLIsSupportedButAnotherMEssageIsSentAsWell() {
+        var config = self.testConnectionConfiguration()
+        XCTAssertNoThrow(config.tls = .require(try NIOSSLContext(configuration: .makeClientConfiguration())))
+        var addSSLCallbackIsHit = false
+        let handler = PostgresChannelHandler(configuration: config) { channel in
+            addSSLCallbackIsHit = true
+        }
+        let eventHandler = TestEventHandler()
+        let embedded = EmbeddedChannel(handlers: [
+            ReverseByteToMessageHandler(PSQLFrontendMessageDecoder()),
+            handler,
+            eventHandler
+        ])
+
+        var maybeMessage: PostgresFrontendMessage?
+        XCTAssertNoThrow(embedded.connect(to: try .init(ipAddress: "0.0.0.0", port: 5432), promise: nil))
+        XCTAssertNoThrow(maybeMessage = try embedded.readOutbound(as: PostgresFrontendMessage.self))
+        guard case .sslRequest(let request) = maybeMessage else {
+            return XCTFail("Unexpected message")
+        }
+
+        XCTAssertEqual(request.code, 80877103)
+
+        var responseBuffer = ByteBuffer()
+        responseBuffer.writeInteger(UInt8(ascii: "S"))
+        responseBuffer.writeInteger(UInt8(ascii: "1"))
+        XCTAssertNoThrow(try embedded.writeInbound(responseBuffer))
+
+        XCTAssertFalse(addSSLCallbackIsHit)
+
+        // the event handler should have seen an error
+        XCTAssertEqual(eventHandler.errors.count, 1)
+
+        // the connections should be closed
+        XCTAssertFalse(embedded.isActive)
+    }
+
     func testSSLUnsupportedClosesConnection() throws {
         let config = self.testConnectionConfiguration(tls: .require(try NIOSSLContext(configuration: .makeClientConfiguration())))