CVE-2023-44386: Incorrect Behavior Order (CWE-696)
value: Vapor is an HTTP web framework for Swift. There is a denial of service vulnerability impacting all users of affected versions of Vapor. The HTTP1 error handler closed connections when HTTP parse errors occur instead of passing them on. The issue is fixed as of Vapor release 4.84.2.
CWE description: The product performs multiple related behaviors, but the behaviors are performed in the wrong order in ways which may produce resultant weaknesses.
CVSS v3 base score: 5.3
Published: 2023-10-05T18:15Z
Repository: https://github.com/vapor/vapor
Language: Swift
Files changed: Sources/Vapor/HTTP/Server/HTTPServer.swift, Sources/Vapor/HTTP/Server/HTTPServerHandler.swift, Sources/Vapor/HTTP/Server/HTTPServerRequestDecoder.swift, Tests/VaporTests/PipelineTests.swift
Commit date: 2023-10-05 06:20:16 -0500
Commit message: Merge pull request from GHSA-3mwq-h3g6-ffhm

* Fix the HTTP1 error handler to close connections when HTTP parse errors occur instead of passing them on.

* Move the error handling for parser errors into HTTPServerRequestDecoder, where it arguably belongs. Don't send 400 bad request for those errors (not required by RFC, difficult to do safely for all cases), fix inbound user event handling in multiple handlers.
Diff: diff --git a/Sources/Vapor/HTTP/Server/HTTPServer.swift b/Sources/Vapor/HTTP/Server/HTTPServer.swift
index c9b406a14..4511ecfd4 100644
--- a/Sources/Vapor/HTTP/Server/HTTPServer.swift
+++ b/Sources/Vapor/HTTP/Server/HTTPServer.swift
@@ -450,77 +450,10 @@ private final class HTTPServerConnection: Sendable {
     deinit {
         assert(!self.channel.isActive, "HTTPServerConnection deinitialized without calling shutdown()")
     }
 }
 
-/// A simple channel handler that catches errors emitted by parsing HTTP requests
-/// and sends 400 Bad Request responses.
-///
-/// This channel handler provides the basic behaviour that the majority of simple HTTP
-/// servers want. This handler does not suppress the parser errors: it allows them to
-/// continue to pass through the pipeline so that other handlers (e.g. logging ones) can
-/// deal with the error.
-/// 
-/// adapted from: https://github.com/apple/swift-nio/blob/00341c92770e0a7bebdc5fda783f08765eb3ff56/Sources/NIOHTTP1/HTTPServerProtocolErrorHandler.swift
-final class HTTP1ServerErrorHandler: ChannelDuplexHandler, RemovableChannelHandler {
-    typealias InboundIn = Never
-    typealias InboundOut = Never
-    typealias OutboundIn = HTTPServerResponsePart
-    typealias OutboundOut = HTTPServerResponsePart
-    let logger: Logger
-    private var hasUnterminatedResponse: Bool = false
-    
-    init(logger: Logger) {
-        self.logger = logger
-    }
-    
-    func errorCaught(context: ChannelHandlerContext, error: Error) {
-        if let error = error as? HTTPParserError {
-            self.makeHTTPParserErrorResponse(context: context, error: error)
-        }
-
-        // Now pass the error on in case someone else wants to see it.
-        // In the Vapor ChannelPipeline the connection will eventually 
-        // be closed by the NIOCloseOnErrorHandler
-        context.fireErrorCaught(error)
-    }
-
-    private func makeHTTPParserErrorResponse(context: ChannelHandlerContext, error: HTTPParserError) {
-        // Any HTTPParserError is automatically fatal, and we don't actually need (or want) to
-        // provide that error to the client: we just want to inform them something went wrong
-        // and then close off the pipeline. However, we can only send an
-        // HTTP error response if another response hasn't started yet.
-        //
-        // A side note here: we cannot block or do any delayed work. 
-        // The channel might be closed right after we return from this function.
-        if !self.hasUnterminatedResponse {
-            self.logger.debug("Bad Request - Invalid HTTP: \(error)")
-            let headers = HTTPHeaders([("Connection", "close"), ("Content-Length", "0")])
-            let head = HTTPResponseHead(version: .http1_1, status: .badRequest, headers: headers)
-            context.write(self.wrapOutboundOut(.head(head)), promise: nil)
-            context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil)
-        }
-    }
-
-    public func write(context: ChannelHandlerContext, data: NIOAny, promise: EventLoopPromise<Void>?) {
-        let res = self.unwrapOutboundIn(data)
-        switch res {
-        case .head(let head) where head.isInformational:
-            precondition(!self.hasUnterminatedResponse)
-        case .head:
-            precondition(!self.hasUnterminatedResponse)
-            self.hasUnterminatedResponse = true
-        case .body:
-            precondition(self.hasUnterminatedResponse)
-        case .end:
-            precondition(self.hasUnterminatedResponse)
-            self.hasUnterminatedResponse = false
-        }
-        context.write(data, promise: promise)
-    }
-}
-
 extension HTTPResponseHead {
     /// Determines if the head is purely informational. If a head is informational another head will follow this
     /// head eventually.
     /// 
     /// This is also from SwiftNIO
@@ -606,13 +539,10 @@ extension ChannelPipeline {
             handlers.append(requestDecompressionHandler)
         case .disabled:
             break
         }
 
-        let errorHandler = HTTP1ServerErrorHandler(logger: configuration.logger)
-        handlers.append(errorHandler)
-
         // add NIO -> HTTP response encoder
         let serverResEncoder = HTTPServerResponseEncoder(
             serverHeader: configuration.serverName,
             dateCache: .eventLoop(self.eventLoop)
         )
diff --git a/Sources/Vapor/HTTP/Server/HTTPServerHandler.swift b/Sources/Vapor/HTTP/Server/HTTPServerHandler.swift
index 323d2c22c..5a76db194 100644
--- a/Sources/Vapor/HTTP/Server/HTTPServerHandler.swift
+++ b/Sources/Vapor/HTTP/Server/HTTPServerHandler.swift
@@ -69,11 +69,11 @@ final class HTTPServerHandler: ChannelInboundHandler, RemovableChannelHandler {
         switch event {
         case is ChannelShouldQuiesceEvent:
             self.logger.trace("HTTP handler will no longer respect keep-alive")
             self.isShuttingDown = true
         default:
-            self.logger.trace("Unhandled user event: \(event)")
+            context.fireUserInboundEventTriggered(event)
         }
     }
 }
 
 struct ErrorBodyStreamWriter: BodyStreamWriter {
diff --git a/Sources/Vapor/HTTP/Server/HTTPServerRequestDecoder.swift b/Sources/Vapor/HTTP/Server/HTTPServerRequestDecoder.swift
index b3ad32709..ddd65d3a7 100644
--- a/Sources/Vapor/HTTP/Server/HTTPServerRequestDecoder.swift
+++ b/Sources/Vapor/HTTP/Server/HTTPServerRequestDecoder.swift
@@ -131,10 +131,14 @@ final class HTTPServerRequestDecoder: ChannelDuplexHandler, RemovableChannelHand
                 stream: stream
             )
         default:
             break
         }
+
+        if error is HTTPParserError {
+            self.logger.debug("Invalid HTTP request, will close connection: \(String(reflecting: error))")
+        }
         context.fireErrorCaught(error)
     }
 
     func channelInactive(context: ChannelHandlerContext) {
         switch self.requestState {
@@ -216,11 +220,11 @@ final class HTTPServerRequestDecoder: ChannelDuplexHandler, RemovableChannelHand
             default:
                 self.logger.debug("A request is currently in-flight")
                 context.fireUserInboundEventTriggered(event)
             }
         default:
-            self.logger.trace("Unhandled user event: \(event)")
+            context.fireUserInboundEventTriggered(event)
         }
     }
 }
 
 extension HTTPPart: CustomStringConvertible {
diff --git a/Tests/VaporTests/PipelineTests.swift b/Tests/VaporTests/PipelineTests.swift
index 962e178ab..9f7c25351 100644
--- a/Tests/VaporTests/PipelineTests.swift
+++ b/Tests/VaporTests/PipelineTests.swift
@@ -136,11 +136,10 @@ final class PipelineTests: XCTestCase {
             } else {
                 XCTFail("Caught error \"\(error)\"")
             }
         }
         XCTAssertEqual(channel.isActive, false)
-        try XCTAssertContains(channel.readOutbound(as: ByteBuffer.self)?.string, "HTTP/1.1 400 Bad Request")
         try XCTAssertNil(channel.readOutbound(as: ByteBuffer.self)?.string)
     }
 
     override class func setUp() {
         XCTAssert(isLoggingConfigured)

Vulnerable code: /// A simple channel handler that catches errors emitted by parsing HTTP requests
/// and sends 400 Bad Request responses.
///
/// This channel handler provides the basic behaviour that the majority of simple HTTP
/// servers want. This handler does not suppress the parser errors: it allows them to
/// continue to pass through the pipeline so that other handlers (e.g. logging ones) can
/// deal with the error.
/// 
/// adapted from: https://github.com/apple/swift-nio/blob/00341c92770e0a7bebdc5fda783f08765eb3ff56/Sources/NIOHTTP1/HTTPServerProtocolErrorHandler.swift
final class HTTP1ServerErrorHandler: ChannelDuplexHandler, RemovableChannelHandler {
    typealias InboundIn = Never
    typealias InboundOut = Never
    typealias OutboundIn = HTTPServerResponsePart
    typealias OutboundOut = HTTPServerResponsePart
    let logger: Logger
    private var hasUnterminatedResponse: Bool = false
    
    init(logger: Logger) {
        self.logger = logger
    }
    
    func errorCaught(context: ChannelHandlerContext, error: Error) {
        if let error = error as? HTTPParserError {
            self.makeHTTPParserErrorResponse(context: context, error: error)
        }

        // Now pass the error on in case someone else wants to see it.
        // In the Vapor ChannelPipeline the connection will eventually 
        // be closed by the NIOCloseOnErrorHandler
        context.fireErrorCaught(error)
    }

    private func makeHTTPParserErrorResponse(context: ChannelHandlerContext, error: HTTPParserError) {
        // Any HTTPParserError is automatically fatal, and we don't actually need (or want) to
        // provide that error to the client: we just want to inform them something went wrong
        // and then close off the pipeline. However, we can only send an
        // HTTP error response if another response hasn't started yet.
        //
        // A side note here: we cannot block or do any delayed work. 
        // The channel might be closed right after we return from this function.
        if !self.hasUnterminatedResponse {
            self.logger.debug("Bad Request - Invalid HTTP: \(error)")
            let headers = HTTPHeaders([("Connection", "close"), ("Content-Length", "0")])
            let head = HTTPResponseHead(version: .http1_1, status: .badRequest, headers: headers)
            context.write(self.wrapOutboundOut(.head(head)), promise: nil)
            context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil)
        }
    }

    public func write(context: ChannelHandlerContext, data: NIOAny, promise: EventLoopPromise<Void>?) {
        let res = self.unwrapOutboundIn(data)
        switch res {
        case .head(let head) where head.isInformational:
            precondition(!self.hasUnterminatedResponse)
        case .head:
            precondition(!self.hasUnterminatedResponse)
            self.hasUnterminatedResponse = true
        case .body:
            precondition(self.hasUnterminatedResponse)
        case .end:
            precondition(self.hasUnterminatedResponse)
            self.hasUnterminatedResponse = false
        }
        context.write(data, promise: promise)
    }
}

        let errorHandler = HTTP1ServerErrorHandler(logger: configuration.logger)
        handlers.append(errorHandler)

            self.logger.trace("Unhandled user event: \(event)")
            self.logger.trace("Unhandled user event: \(event)")
        try XCTAssertContains(channel.readOutbound(as: ByteBuffer.self)?.string, "HTTP/1.1 400 Bad Request")
Fixed code:             context.fireUserInboundEventTriggered(event)

        if error is HTTPParserError {
            self.logger.debug("Invalid HTTP request, will close connection: \(String(reflecting: error))")
        }
            context.fireUserInboundEventTriggered(event)