Skip to content
Permalink
Browse files Browse the repository at this point in the history
Merge pull request from GHSA-9cfh-vx93-84vv
* Ensure empty incoming buffer when TLS negotiation starts

* cleanup

* Use real nio version
  • Loading branch information
fabianfett committed May 4, 2023
1 parent dd1b740 commit 2df54bc
Show file tree
Hide file tree
Showing 7 changed files with 74 additions and 28 deletions.
2 changes: 1 addition & 1 deletion Package.swift
Expand Up @@ -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"),
Expand Down
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand All @@ -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
}
}

Expand Down
6 changes: 6 additions & 0 deletions Sources/PostgresNIO/New/PSQLError.swift
Expand Up @@ -7,6 +7,7 @@ public struct PSQLError: Error {
enum Base: Sendable, Hashable {
case sslUnsupported
case failedToAddSSLHandler
case receivedUnencryptedDataAfterSSLRequest
case server
case messageDecodingFailure
case unexpectedBackendMessage
Expand All @@ -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)
Expand All @@ -51,6 +53,8 @@ public struct PSQLError: Error {
return "sslUnsupported"
case .failedToAddSSLHandler:
return "failedToAddSSLHandler"
case .receivedUnencryptedDataAfterSSLRequest:
return "receivedUnencryptedDataAfterSSLRequest"
case .server:
return "server"
case .messageDecodingFailure:
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion Sources/PostgresNIO/New/PostgresChannelHandler.swift
Expand Up @@ -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()
}
Expand Down
2 changes: 2 additions & 0 deletions Sources/PostgresNIO/Postgres+PSQLCompat.swift
Expand Up @@ -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:
Expand Down
Expand Up @@ -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)))
}
Expand Down
39 changes: 38 additions & 1 deletion Tests/PostgresNIOTests/New/PostgresChannelHandlerTests.swift
Expand Up @@ -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())))

Expand Down

0 comments on commit 2df54bc

Please sign in to comment.