Merge pull request from GHSA-jchv-x857-q8fq

* Forbid excessive sending of empty DATA frames

Motivation:

One vector for DoS can be sending a large number of essentially "empty"
frames, such as empty DATA frames without END_STREAM set. We should
forbid peers from doing so excessively, but we want to tolerate
implementations that occasionally emit such a frame for buggy reasons.

Modifications:

- Added verification that too many empty DATA frames are not sent.

Result:

Better resistance to DoS attacks.

* Improved buffering in CompoundOutboundBuffer

Motivation:

In HTTP/2 it is possible for remote peers to submit work in a way that
cause the NIOHTTP2Handler to generate a substantial number of control
frames. If the peer is not consuming these frames, we can eventually run
out of memory and die, which is not exactly ideal.

It's hard for us to know how the buffering is going if we always submit
all frames directly to the Channel, so we need to pay attention to
whether the Channel is actually writable. If it isn't, rather than issue
further writes we need to delay those writes so we can keep track of how
big that buffer is getting. If the buffer gets too large we need a
circuit breaker to drop the connection, as we're clearly not able to get
work done.

This patch adds a new buffer for outbound control frames. This new
buffer is the last step in the buffering sequence, and the first to be
drained when a Channel becomes writable.

Additionally, enhancements need to be made to ensure that other buffers
behave correctly in the face of this new model. The
ConcurrentStreamsBuffer needs to be more aggressive about storing
HEADERS frames that cannot be emitted to the wire because it does not
tolerate emitting a HEADERS frame that cannot be immediately written to
the wire. Additionally, the buffers must not be polled when a Channel is
unwritable.

This is a large patch, but it does substantially change the buffering
logic, and protects NIO against a wide range of denial of service
attacks, while providing a clear and coherent defense surface.

Note that one thing this patch does not do is constrain the size of the
buffers in the ConcurrentStreamsBuffer. This is on purpose: the size of
this buffer is controlled entirely by user action. Users that do not
wish to buffer excessively in this buffer can avoid doing so simply by
respecting the peer's value of SETTINGS_MAX_CONCURRENT_STREAMS. As the
entire purpose of this buffer is to tolerate users that do not wish to
abide by this setting, it would be unreasonable to provide an upper
bound on the value used in this setting.

Modifications:

- Moved the various buffers into a directory.
- Built and implemented ControlFrameBuffer.
- Integrated ControlFrameBuffer into CompoundOutboundBuffer.
- Added awareness around channel writability to HTTP2Handler
- Enhanced ConcurrentStreamsBuffer to be writability-aware.

Result:

We'll have somewhere to put control frames.

* Cleanup API for merged security fixes.

Motivation:

After merging the security fixes we ended up with a slightly awkward
API: the argument names differed slightly for no good reason, and it
became impossible to override only one of the two security settings.

Modifications:

- Renamed `max` to `maximum`.
- Gave default values for the security settings.

Result:

Better user experience.

* Prevent DoS with large header lists.

Motivation:

There are two ways malicious peers could attempt to DoS a SwiftNIO
HTTP/2 implementation with header lists. The first would be to attempt
to send an enormous header list, ideally with an amplification factor.
This approach was possible because SwiftNIO never actually enforced the
value of the max header list size it advertised.

In principle the peer can still get a good amplification attack effect
by sending a long header list that contains zero-length headers. We may
as well close this door as well: zero length header field names are
unacceptable, and so we should forbid them.

Modifications:

- Added max header list enforcement.
- Forbade zero-length header field names.

Result:

Peers cannot DoS a SwiftNIO HTTP/2 implementation using large header
lists.

Author: Lukasa

Sources/NIOHPACK/HPACKDecoder.swift

@@ -29,6 +29,14 @@ public struct HPACKDecoder {
     public static var maxDynamicTableSize: Int {
         return DynamicHeaderTable.defaultSize
     }
+
+    /// The default value of the maximum header list size for the decoder.
+    ///
+    /// This value is somewhat arbitrary, but 16kB should be sufficiently large to decode all reasonably
+    /// sized header lists.
+    public static var defaultMaxHeaderListSize: Int {
+        return 1<<14
+    }
 
     // private but tests
     var headerTable: IndexedHeaderTable
@@ -43,7 +51,10 @@ public struct HPACKDecoder {
         get { return self.headerTable.dynamicTableAllowedLength }
         set { self.headerTable.dynamicTableAllowedLength = newValue }
     }
-    
+
+    /// The maximum size of the header list.
+    public var maxHeaderListSize: Int
+
     /// A string value discovered in a HPACK buffer. The value can either indicate an entry
     /// in the header table index or the start of an inline literal string.
     enum HPACKString {
@@ -73,7 +84,17 @@ public struct HPACKDecoder {
     ///
     /// - Parameter maxDynamicTableSize: Maximum allowed size of the dynamic header table.
     public init(allocator: ByteBufferAllocator, maxDynamicTableSize: Int = HPACKDecoder.maxDynamicTableSize) {
+        self.init(allocator: allocator, maxDynamicTableSize: maxDynamicTableSize, maxHeaderListSize: HPACKDecoder.defaultMaxHeaderListSize)
+    }
+
+    /// Creates a new decoder
+    ///
+    /// - Parameter maxDynamicTableSize: Maximum allowed size of the dynamic header table.
+    /// - Parameter maxHeaderListSize: Maximum allowed size of a decoded header list.
+    public init(allocator: ByteBufferAllocator, maxDynamicTableSize: Int, maxHeaderListSize: Int) {
+        precondition(maxHeaderListSize > 0, "Max header list size must be positive!")
         self.headerTable = IndexedHeaderTable(allocator: allocator, maxDynamicTableSize: maxDynamicTableSize)
+        self.maxHeaderListSize = maxHeaderListSize
     }
 
     /// Reads HPACK encoded header data from a `ByteBuffer`.
@@ -91,6 +112,8 @@ public struct HPACKDecoder {
         var headers: [HPACKHeader] = []
         headers.reserveCapacity(16)
 
+        var listSize = 0
+
         while bufCopy.readableBytes > 0 {
             switch try self.decodeHeader(from: &bufCopy) {
             case .tableSizeChange:
@@ -104,6 +127,11 @@ public struct HPACKDecoder {
                     throw NIOHPACKErrors.IllegalDynamicTableSizeChange()
                 }
             case .header(let header):
+                listSize += header.size
+                guard listSize <= self.maxHeaderListSize else {
+                    throw NIOHPACKErrors.MaxHeaderListSizeViolation()
+                }
+
                 headers.append(header)
             }
         }
@@ -191,6 +219,10 @@ public struct HPACKDecoder {
             (name, _) = try self.headerTable.header(at: idx)
         case .literal:
             name = try self.readEncodedString(from: &buffer)
+            guard name.utf8.count > 0 else {
+                // This isn't explicitly forbidden by RFC 7541, but it *is* forbidden by RFC 7230.
+                throw NIOHPACKErrors.EmptyLiteralHeaderFieldName()
+            }
         }
 
         let value = try self.readEncodedString(from: &buffer)

Sources/NIOHPACK/HPACKErrors.swift

@@ -107,4 +107,15 @@ public enum NIOHPACKErrors {
     public struct ZeroHeaderIndex: NIOHPACKError {
         public init() { }
     }
+
+    /// HPACK decoder asked to decode a header list that would violate the configured
+    /// max header list size.
+    public struct MaxHeaderListSizeViolation: NIOHPACKError {
+        public init() { }
+    }
+
+    /// HPACK decoder asked to decode a header field name that was empty.
+    public struct EmptyLiteralHeaderFieldName: NIOHPACKError {
+        public init() { }
+    }
 }

Sources/NIOHPACK/HPACKHeader.swift

@@ -280,6 +280,20 @@ internal struct HPACKHeader {
 }
 
 
+extension HPACKHeader {
+    internal var size: Int {
+        // RFC 7541 § 4.1:
+        //
+        //      The size of an entry is the sum of its name's length in octets (as defined in
+        //      Section 5.2), its value's length in octets, and 32.
+        //
+        //      The size of an entry is calculated using the length of its name and value
+        //      without any Huffman encoding applied.
+        return name.utf8.count + value.utf8.count + 32
+    }
+}
+
+
 internal extension UInt8 {
     var isASCII: Bool {
         return self <= 127

Sources/NIOHTTP2/ConnectionStateMachine/HasLocalSettings.swift

@@ -63,7 +63,9 @@ extension HasLocalSettings {
                     // respect that possibility.
                     effect.streamWindowSizeChange += Int(delta)
                 case .maxFrameSize:
-                   effect.newMaxFrameSize = newValue
+                    effect.newMaxFrameSize = newValue
+                case .maxHeaderListSize:
+                    effect.newMaxHeaderListSize = newValue
                 default:
                     // No operation required
                     return

Sources/NIOHTTP2/DOSHeuristics.swift

@@ -0,0 +1,60 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the SwiftNIO open source project
+//
+// Copyright (c) 2019 Apple Inc. and the SwiftNIO project authors
+// Licensed under Apache License v2.0
+//
+// See LICENSE.txt for license information
+// See CONTRIBUTORS.txt for the list of SwiftNIO project authors
+//
+// SPDX-License-Identifier: Apache-2.0
+//
+//===----------------------------------------------------------------------===//
+
+
+/// Implements some simple denial of service heuristics on inbound frames.
+struct DOSHeuristics {
+    /// The number of "empty" (zero bytes of useful payload) DATA frames we've received since the
+    /// last useful frame.
+    ///
+    /// We reset this count each time we see END_STREAM, or a HEADERS frame, both of which we count
+    /// as doing useful work. We have a small budget for these because we want to tolerate buggy
+    /// implementations that occasionally emit empty DATA frames, but don't want to drown in them.
+    private var receivedEmptyDataFrames: Int
+
+    /// The maximum number of "empty" data frames we're willing to tolerate.
+    private let maximumSequentialEmptyDataFrames: Int
+
+    internal init(maximumSequentialEmptyDataFrames: Int) {
+        precondition(maximumSequentialEmptyDataFrames >= 0,
+                     "maximum sequential empty data frames must be positive, got \(maximumSequentialEmptyDataFrames)")
+        self.maximumSequentialEmptyDataFrames = maximumSequentialEmptyDataFrames
+        self.receivedEmptyDataFrames = 0
+    }
+}
+
+
+extension DOSHeuristics {
+    mutating func process(_ frame: HTTP2Frame) throws {
+        switch frame.payload {
+        case .data(let payload):
+            if payload.data.readableBytes == 0 {
+                self.receivedEmptyDataFrames += 1
+            }
+
+            if payload.endStream {
+                self.receivedEmptyDataFrames = 0
+            }
+        case .headers:
+            self.receivedEmptyDataFrames = 0
+        case .alternativeService, .goAway, .origin, .ping, .priority, .pushPromise, .rstStream, .settings, .windowUpdate:
+            // Currently we don't assess these for DoS risk.
+            ()
+        }
+
+        if self.receivedEmptyDataFrames > self.maximumSequentialEmptyDataFrames {
+            throw NIOHTTP2Errors.ExcessiveEmptyDataFrames()
+        }
+    }
+}

Sources/NIOHTTP2/ConcurrentStreamBuffer.swift renamed to Sources/NIOHTTP2/Frame Buffers/ConcurrentStreamBuffer.swift

@@ -105,8 +105,9 @@ struct ConcurrentStreamBuffer {
         self.bufferedFrames.markFlushPoint()
     }
 
-    mutating func processOutboundFrame(_ frame: HTTP2Frame, promise: EventLoopPromise<Void>?) throws -> OutboundFrameAction {
-        // If this frame is not for a locally initiated stream, then that's fine, just pass it on.
+    mutating func processOutboundFrame(_ frame: HTTP2Frame, promise: EventLoopPromise<Void>?, channelWritable: Bool) throws -> OutboundFrameAction {
+        // If this frame is not for a locally initiated stream, then that's fine, just pass it on. Even if the channel isn't
+        // writable, one of the other two buffers should catch this.
         guard frame.streamID != .rootStream && frame.streamID.mayBeInitiatedBy(self.mode) else {
             return .forward
         }
@@ -119,29 +120,44 @@ struct ConcurrentStreamBuffer {
         //
         // Before we search our buffers for this stream we do a quick sanity check: if its stream ID is lower than the first element in the
         // array, it won't be there.
+        //
+        // Again, we choose to ignore channel writability here because one of the other buffers should catch this frame.
         if let firstElement = self.bufferedFrames.first,
             frame.streamID >= firstElement.streamID,
             let bufferIndex = self.bufferedFrames.binarySearch(key: { $0.streamID }, needle: frame.streamID) {
             return self.bufferFrame(frame, promise: promise, bufferIndex: bufferIndex)
         }
 
         // Ok, we're not currently buffering frames for this stream.
+        //
         // Now we need to check if this is for a stream that has already been opened. If it is, and we aren't buffering it, pass
-        // the frame through.
+        // the frame through. Again, we ignore channel writability here because we don't need to delay state changes: one of the
+        // other buffers will catch this and it'll be fine.
         if frame.streamID <= self.lastOutboundStream {
             return .forward
         }
 
         // Now we want to see whether we're allowed to initiate a new stream. If we aren't, then we will buffer this stream.
-        if self.currentOutboundStreams >= self.maxOutboundStreams {
-            // Ok, we can't create a new stream. In this case we need to buffer this. We can only have gotten this far if either this stream ID is lower
-            // than the first stream ID, or if it's higher but doesn't match something in the buffer. As a result, it is an error for this frame to have
-            // a stream ID lower than or equal to the highest stream ID in the buffer: if it did, we should have found it when we searched above. If that
-            // constraint is breached, fail the write.
+        if self.currentOutboundStreams >= self.maxOutboundStreams || !channelWritable {
+            // Ok, we can't create a new stream, either due to MAX_CONCURRENT_STREAMS limits or because the channel isn't writable. In this case we
+            // need to buffer this. We can only have gotten this far if either this stream ID is lower than the first stream ID, or if it's higher
+            // but doesn't match something in the buffer. As a result, it is an error for this frame to have a stream ID lower than or equal to the
+            // highest stream ID in the buffer: if it did, we should have found it when we searched above. If that constraint is breached, fail the write.
             if let lastElement = self.bufferedFrames.last, frame.streamID <= lastElement.streamID {
                 throw NIOHTTP2Errors.StreamIDTooSmall()
             }
 
+            // Ok, the stream ID is fine: buffer this frame.
+            self.bufferFrameForNewStream(frame, promise: promise)
+            return .nothing
+        } else if let lastElement = self.bufferedFrames.last, !lastElement.currentlyUnblocking {
+            // In principle we can create a new stream, and the channel is writable. However, we have at least one stream that is currently buffered and not unblocking.
+            // This buffer probably has a HEADERS frame in it, and we really don't want to violate the ordering requirements that implies, so we'll buffer this anyway.
+            // We still want StreamIDTooSmall protection here.
+            if frame.streamID <= lastElement.streamID {
+                throw NIOHTTP2Errors.StreamIDTooSmall()
+            }
+
             // Ok, the stream ID is fine: buffer this frame.
             self.bufferFrameForNewStream(frame, promise: promise)
             return .nothing

Sources/NIOHTTP2/Frame Buffers/ControlFrameBuffer.swift

@@ -0,0 +1,108 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the SwiftNIO open source project
+//
+// Copyright (c) 2019 Apple Inc. and the SwiftNIO project authors
+// Licensed under Apache License v2.0
+//
+// See LICENSE.txt for license information
+// See CONTRIBUTORS.txt for the list of SwiftNIO project authors
+//
+// SPDX-License-Identifier: Apache-2.0
+//
+//===----------------------------------------------------------------------===//
+import NIO
+
+
+/// A buffer that stores outbound control frames.
+///
+/// In general it is preferential to buffer outbound frames instead of passing them into the channel.
+/// This is because once the frame has left the HTTP2 handler and moved into the Channel it is no longer
+/// easy for us to tell how much data has been buffered. The larger the buffer grows, the more likely it
+/// is that the peer is consuming resources of ours that we need for other use-cases, and in some cases
+/// this may amount to an actual denial of service attack.
+///
+/// We have a number of buffers that handle data frames, but a similar concern applies to control frames
+/// too. Control frames need to be emitted with relatively high priority, but they should only be emitted
+/// when it will be reasonably possible to write them to the network. As long as it is not possible, we
+/// want to store them where we can see them, and use the buffer size to make choices about the connection.
+struct ControlFrameBuffer {
+    /// Any control frame writes that may need to be emitted.
+    private var pendingControlFrames: MarkedCircularBuffer<PendingControlFrame>
+
+    /// The maximum size of the buffer. If we have to buffer more frames than this,
+    /// we'll kill the connection.
+    internal var maximumBufferSize: Int
+}
+
+
+// MARK:- ControlFrameBuffer initializers
+extension ControlFrameBuffer {
+    internal init(maximumBufferSize: Int) {
+        // We allocate a circular buffer of reasonable size to ensure that if we ever do have to
+        // buffer control frames that we won't need to resize this too aggressively.
+        self.pendingControlFrames = MarkedCircularBuffer(initialCapacity: 16)
+        self.maximumBufferSize = maximumBufferSize
+    }
+}
+
+
+// MARK:- ControlFrameBuffer frame processing
+extension ControlFrameBuffer {
+    internal mutating func processOutboundFrame(_ frame: HTTP2Frame, promise: EventLoopPromise<Void>?, channelWritable: Bool) throws -> OutboundFrameAction {
+        switch frame.payload {
+        case .data:
+            // These frames are not buffered here. If it reached us, it's because we believe the channel is writable,
+            // and we must have no control frames buffered.
+            assert(channelWritable, "Received flushed data frame without writable channel")
+            assert(self.pendingControlFrames.count == 0, "Received flush data frames while buffering control frames")
+            return .forward
+        default:
+            // Control frames. These are what we buffer. We buffer them in two cases: either we have something else
+            // already buffered (to ensure ordering stays correct), or the channel isn't writable.
+            if !channelWritable || self.pendingControlFrames.count > 0 {
+                try self.bufferFrame(frame, promise: promise)
+                return .nothing
+            } else {
+                return .forward
+            }
+        }
+    }
+
+    internal mutating func flushReceived() {
+        self.pendingControlFrames.mark()
+    }
+
+    internal mutating func nextFlushedWritableFrame() -> PendingControlFrame? {
+        if self.pendingControlFrames.hasMark && self.pendingControlFrames.count > 0 {
+            return self.pendingControlFrames.removeFirst()
+        } else {
+            return nil
+        }
+    }
+
+    internal func invalidateBuffer(reason: ChannelError) {
+        for write in self.pendingControlFrames {
+            write.promise?.fail(reason)
+        }
+    }
+
+    private mutating func bufferFrame(_ frame: HTTP2Frame, promise: EventLoopPromise<Void>?) throws {
+        guard self.pendingControlFrames.count < self.maximumBufferSize else {
+            // Appending another frame would violate the maximum buffer size. We're storing too many frames here,
+            // we gotta move on.
+            throw NIOHTTP2Errors.ExcessiveOutboundFrameBuffering()
+        }
+        self.pendingControlFrames.append(PendingControlFrame(frame: frame, promise: promise))
+    }
+}
+
+
+// MARK:- ControlFrameBuffer.PendingControlFrame definition
+extension ControlFrameBuffer {
+    /// A buffered control frame write and its associated promise.
+    struct PendingControlFrame {
+        var frame: HTTP2Frame
+        var promise: EventLoopPromise<Void>?
+    }
+}

Sources/NIOHTTP2/OutboundFlowControlBuffer.swift renamed to Sources/NIOHTTP2/Frame Buffers/OutboundFlowControlBuffer.swift

@@ -143,6 +143,7 @@ internal struct OutboundFlowControlBuffer {
     }
 
     internal mutating func nextFlushedWritableFrame() -> (HTTP2Frame, EventLoopPromise<Void>?)? {
+        // If the channel isn't writable, we don't want to send anything.
         guard let nextStreamID = self.nextStreamToSend(), self.connectionWindowSize > 0 else {
             return nil
         }