Merge pull request from GHSA-w3f6-pc54-gfw7

* Refactor HPACK integer decoding

Motivation:

The HPACK integer decoding used a number of unchecked operations which
can trap on some inputs. Since it is network reachable code, it should
throw errors if the operations overflow.

Modifications:

- Add a failure case to the fuzz testing
- Refactor the integer decoding to check for overflow on the arithmetic
- This throws a new 'HPACKErrors.UnrepresentableInteger' error on
  overflow
- Add a missing bounds check
- Remove an unnecessary and incorrect path
- Remove the default argument from the function driving the decoding,
  the default was not valid and would cause an assertion to fail if used
- Return the decoded value as an `Int` rather than a `UInt`
- More tests

Result:

Integer decoding is safer.

* Use unchecked shifting

* Use truncatingIfNeeded

* make error internal

Author: glbrntt

FuzzTesting/FailCases/clusterfuzz-testcase-minimized-ServerFuzzer-release-5635720084062208

@@ -23,83 +23,83 @@ public enum NIOHPACKErrors {
     public struct InvalidHeaderIndex : NIOHPACKError {
         /// The offending index.
         public let suppliedIndex: Int
-        
+
         /// The highest index we have available.
         public let availableIndex: Int
     }
-    
+
     /// A header block indicated an indexed header with no accompanying
     /// value, but the index referenced an entry with no value of its own
     /// e.g. one of the many valueless items in the static header table.
     public struct IndexedHeaderWithNoValue : NIOHPACKError {
         /// The offending index.
         public let index: Int
     }
-    
+
     /// An encoded string contained an invalid length that extended
     /// beyond its frame's payload size.
     public struct StringLengthBeyondPayloadSize : NIOHPACKError {
         /// The length supplied.
         public let length: Int
-        
+
         /// The available number of bytes.
         public let available: Int
     }
-    
+
     /// Decoded string data could not be parsed as valid UTF-8.
     public struct InvalidUTF8Data : NIOHPACKError {
         /// The offending bytes.
         public let bytes: ByteBuffer
     }
-    
+
     /// The start byte of a header did not match any format allowed by
     /// the HPACK specification.
     public struct InvalidHeaderStartByte : NIOHPACKError {
         /// The offending byte.
         public let byte: UInt8
     }
-    
+
     /// A dynamic table size update specified an invalid size.
     public struct InvalidDynamicTableSize : NIOHPACKError {
         /// The offending size.
         public let requestedSize: Int
-        
+
         /// The actual maximum size that was set by the protocol.
         public let allowedSize: Int
     }
-    
+
     /// A dynamic table size update was found outside its allowed place.
     /// They may only be included at the start of a header block.
     public struct IllegalDynamicTableSizeChange : NIOHPACKError {}
-    
+
     /// A new header could not be added to the dynamic table. Usually
     /// this means the header itself is larger than the current
     /// dynamic table size.
     public struct FailedToAddIndexedHeader<Name: Collection, Value: Collection> : NIOHPACKError where Name.Element == UInt8, Value.Element == UInt8 {
         /// The table size required to be able to add this header to the table.
         public let bytesNeeded: Int
-        
+
         /// The name of the header that could not be written.
         public let name: Name
-        
+
         /// The value of the header that could not be written.
         public let value: Value
-        
+
         public static func == (lhs: NIOHPACKErrors.FailedToAddIndexedHeader<Name, Value>, rhs: NIOHPACKErrors.FailedToAddIndexedHeader<Name, Value>) -> Bool {
             guard lhs.bytesNeeded == rhs.bytesNeeded else {
                 return false
             }
             return lhs.name.elementsEqual(rhs.name) && lhs.value.elementsEqual(rhs.value)
         }
     }
-    
+
     /// Ran out of input bytes while decoding.
     public struct InsufficientInput : NIOHPACKError {}
-    
+
     /// HPACK encoder asked to begin a new header block while partway through encoding
     /// another block.
     public struct EncoderAlreadyActive : NIOHPACKError {}
-    
+
     /// HPACK encoder asked to append a header without first calling `beginEncoding(allocator:)`.
     public struct EncoderNotStarted : NIOHPACKError {}
 
@@ -118,4 +118,9 @@ public enum NIOHPACKErrors {
     public struct EmptyLiteralHeaderFieldName: NIOHPACKError {
         public init() { }
     }
+
+    /// The integer being decoded is not representable by this implementation.
+    internal struct UnrepresentableInteger: NIOHPACKError {
+        public init() {}
+    }
 }

Sources/NIOHPACK/HPACKErrors.swift

@@ -28,24 +28,24 @@ func encodeInteger(_ value: UInt, to buffer: inout ByteBuffer,
                    prefix: Int, prefixBits: UInt8 = 0) -> Int {
     assert(prefix <= 8)
     assert(prefix >= 1)
-    
+
     let start = buffer.writerIndex
-    
+
     let k = (1 << prefix) - 1
     var initialByte = prefixBits
-    
+
     if value < k {
         // it fits already!
         initialByte |= UInt8(truncatingIfNeeded: value)
         buffer.writeInteger(initialByte)
         return 1
     }
-    
+
     // if it won't fit in this byte altogether, fill in all the remaining bits and move
     // to the next byte.
     initialByte |= UInt8(truncatingIfNeeded: k)
     buffer.writeInteger(initialByte)
-    
+
     // deduct the initial [prefix] bits from the value, then encode it seven bits at a time into
     // the remaining bytes.
     var n = value - UInt(k)
@@ -54,55 +54,88 @@ func encodeInteger(_ value: UInt, to buffer: inout ByteBuffer,
         buffer.writeInteger(nextByte)
         n >>= 7
     }
-    
+
     buffer.writeInteger(UInt8(n))
     return buffer.writerIndex - start
 }
 
+fileprivate let valueMask: UInt8 = 127
+fileprivate let continuationMask: UInt8 = 128
+
 /* private but tests */
-func decodeInteger(from bytes: ByteBufferView, prefix: Int) throws -> (UInt, Int) {
-    assert(prefix <= 8)
-    assert(prefix >= 1)
-    
-    let mask = (1 << prefix) - 1
-    var accumulator: UInt = 0
-    var index = bytes.startIndex
+struct DecodedInteger {
+    var value: Int
+    var bytesRead: Int
+}
 
-    // if the available bits aren't all set, the entire value consists of those bits
-    if bytes[index] & UInt8(mask) != mask {
-        return (UInt(bytes[index] & UInt8(mask)), 1)
+/* private but tests */
+func decodeInteger(from bytes: ByteBufferView, prefix: Int) throws -> DecodedInteger {
+    precondition((1...8).contains(prefix))
+    if bytes.isEmpty {
+        throw NIOHPACKErrors.InsufficientInput()
     }
 
-    accumulator = UInt(mask)
-    index = bytes.index(after: index)
-    if index == bytes.endIndex {
-        return (accumulator, bytes.distance(from: bytes.startIndex, to: index))
+    // See RFC 7541 § 5.1 for details of the encoding/decoding.
+
+    var index = bytes.startIndex
+    // The shifting and arithmetic operate on 'Int' and prefix is 1...8, so these unchecked operations are
+    // fine and the result must fit in a UInt8.
+    let prefixMask = UInt8(truncatingIfNeeded: (1 &<< prefix) &- 1)
+    let prefixBits = bytes[index] & prefixMask
+
+    if prefixBits != prefixMask {
+        // The prefix bits aren't all '1', so they represent the whole value, we're done.
+        return DecodedInteger(value: Int(prefixBits), bytesRead: 1)
     }
-    
+
+    var accumulator = Int(prefixMask)
+    bytes.formIndex(after: &index)
+
     // for the remaining bytes, as long as the top bit is set, consume the low seven bits.
-    var shift: UInt = 0
+    var shift: Int = 0
     var byte: UInt8 = 0
+
     repeat {
         if index == bytes.endIndex {
             throw NIOHPACKErrors.InsufficientInput()
         }
-        
+
         byte = bytes[index]
-        accumulator += UInt(byte & 127) * (1 << shift)
-        shift += 7
-        index = bytes.index(after: index)
-    } while byte & 128 == 128
-    
-    return (accumulator, bytes.distance(from: bytes.startIndex, to: index))
+
+        let value = Int(byte & valueMask)
+
+        // The shift cannot overflow: the value of 'shift' is strictly less than 'Int.bitWidth'.
+        let (multiplicationResult, multiplicationOverflowed) = value.multipliedReportingOverflow(by: (1 &<< shift))
+        if multiplicationOverflowed {
+            throw NIOHPACKErrors.UnrepresentableInteger()
+        }
+
+        let (additionResult, additionOverflowed) = accumulator.addingReportingOverflow(multiplicationResult)
+        if additionOverflowed {
+            throw NIOHPACKErrors.UnrepresentableInteger()
+        }
+
+        accumulator = additionResult
+
+        // Unchecked is fine, there's no chance of it overflowing given the possible values of 'Int.bitWidth'.
+        shift &+= 7
+        if shift >= Int.bitWidth {
+            throw NIOHPACKErrors.UnrepresentableInteger()
+        }
+
+        bytes.formIndex(after: &index)
+    } while byte & continuationMask == continuationMask
+
+    return DecodedInteger(value: accumulator, bytesRead: bytes.distance(from: bytes.startIndex, to: index))
 }
 
 extension ByteBuffer {
-    mutating func readEncodedInteger(withPrefix prefix: Int = 0) throws -> Int {
-        let (result, nread) = try decodeInteger(from: self.readableBytesView, prefix: prefix)
-        self.moveReaderIndex(forwardBy: nread)
-        return Int(result)
+    mutating func readEncodedInteger(withPrefix prefix: Int) throws -> Int {
+        let result = try decodeInteger(from: self.readableBytesView, prefix: prefix)
+        self.moveReaderIndex(forwardBy: result.bytesRead)
+        return result.value
     }
-    
+
     mutating func write(encodedInteger value: UInt, prefix: Int = 0, prefixBits: UInt8 = 0) {
         encodeInteger(value, to: &self, prefix: prefix, prefixBits: prefixBits)
     }

Sources/NIOHPACK/IntegerCoding.swift

@@ -29,6 +29,11 @@ extension IntegerCodingTests {
       return [
                 ("testIntegerEncoding", testIntegerEncoding),
                 ("testIntegerDecoding", testIntegerDecoding),
+                ("testIntegerDecodingMultiplicationDoesNotOverflow", testIntegerDecodingMultiplicationDoesNotOverflow),
+                ("testIntegerDecodingAdditionDoesNotOverflow", testIntegerDecodingAdditionDoesNotOverflow),
+                ("testIntegerDecodingShiftDoesNotOverflow", testIntegerDecodingShiftDoesNotOverflow),
+                ("testIntegerDecodingEmptyInput", testIntegerDecodingEmptyInput),
+                ("testIntegerDecodingNotEnoughBytes", testIntegerDecodingNotEnoughBytes),
            ]
    }
 }

Tests/NIOHPACKTests/IntegerCodingTests+XCTest.swift

@@ -30,11 +30,11 @@ class IntegerCodingTests : XCTestCase {
         return data
     }
 
-    private func decodeInteger(from array: [UInt8], prefix: Int) throws -> UInt {
+    private func decodeInteger(from array: [UInt8], prefix: Int) throws -> Int {
         scratchBuffer.clear()
         scratchBuffer.writeBytes(array)
-        let (r, _) = try NIOHPACK.decodeInteger(from: scratchBuffer.readableBytesView, prefix: prefix)
-        return r
+        let result = try NIOHPACK.decodeInteger(from: scratchBuffer.readableBytesView, prefix: prefix)
+        return result.value
     }
 
     // MARK: - Tests
@@ -108,7 +108,7 @@ class IntegerCodingTests : XCTestCase {
         // something carefully crafted to produce maximum number of output bytes with minimum number of
         // nonzero bits:
         data = encodeIntegerToArray(9223372036854775809, prefix: 1)
-        
+
         // calculations:
         //  subtract prefix:
         //      9223372036854775809 - 1 = 9223372036854775808 or 1000 (0000 x15)
@@ -145,21 +145,64 @@ class IntegerCodingTests : XCTestCase {
 
         XCTAssertEqual(try decodeInteger(from: [0b00011111, 154, 10], prefix: 5), 1337)
         XCTAssertEqual(try decodeInteger(from: [0b11111111, 154, 10], prefix: 5), 1337)
-        
+
         XCTAssertEqual(try decodeInteger(from: [0b00101010], prefix: 8), 42)
-        
+
         // Now some larger numbers:
         XCTAssertEqual(try decodeInteger(from: [255, 129, 254, 255, 255, 255, 255, 255, 255, 33], prefix: 8), 2449958197289549824)
         XCTAssertEqual(try decodeInteger(from: [1, 255, 255, 255, 255, 255, 255, 255, 255, 33], prefix: 1), 2449958197289549824)
-        XCTAssertEqual(try decodeInteger(from: [1, 254, 255, 255, 255, 255, 255, 255, 255, 255, 1], prefix: 1), UInt(UInt64.max))
-        
+        XCTAssertEqual(try decodeInteger(from: [1, 254, 255, 255, 255, 255, 255, 255, 255, 127, 1], prefix: 1), Int.max)
+
         // lots of zeroes: each 128 yields zero
-        XCTAssertEqual(try decodeInteger(from: [1, 128, 128, 128, 128, 128, 128, 128, 128, 128, 1], prefix: 1), 9223372036854775809)
-        
+        XCTAssertEqual(try decodeInteger(from: [1, 128, 128, 128, 128, 128, 128, 128, 128, 127, 1], prefix: 1), 9151314442816847873)
+
         // almost the same bytes, but a different prefix:
-        XCTAssertEqual(try decodeInteger(from: [255, 128, 128, 128, 128, 128, 128, 128, 128, 128, 1], prefix: 8), 9223372036854776063)
-        
+        XCTAssertEqual(try decodeInteger(from: [255, 128, 128, 128, 128, 128, 128, 128, 128, 127, 1], prefix: 8), 9151314442816848127)
+
         // now a silly version which should never have been encoded in so many bytes
         XCTAssertEqual(try decodeInteger(from: [255, 129, 128, 128, 128, 128, 128, 128, 128, 0], prefix: 8), 256)
     }
+
+    func testIntegerDecodingMultiplicationDoesNotOverflow() throws {
+        // Zeros with continuation bits (e.g. 128) to increase the shift value (to 9 * 7 = 63), and then multiply by 127.
+        for `prefix` in 1...8 {
+            XCTAssertThrowsError(try decodeInteger(from: [255, 128, 128, 128, 128, 128, 128, 128, 128, 128, 127], prefix: prefix)) { error in
+                XCTAssert(error is NIOHPACKErrors.UnrepresentableInteger)
+            }
+        }
+    }
+
+    func testIntegerDecodingAdditionDoesNotOverflow() throws {
+        // Zeros with continuation bits (e.g. 128) to increase the shift value (to 9 * 7 = 63), and then multiply by 127.
+        for `prefix` in 1...8 {
+            XCTAssertThrowsError(try decodeInteger(from: [255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 127], prefix: prefix)) { error in
+                XCTAssert(error is NIOHPACKErrors.UnrepresentableInteger)
+            }
+        }
+    }
+
+    func testIntegerDecodingShiftDoesNotOverflow() throws {
+        // With enough iterations we expect the shift to become greater >= 64.
+        for `prefix` in 1...8 {
+            XCTAssertThrowsError(try decodeInteger(from: [255, 128, 128, 128, 128, 128, 128, 128, 128, 128, 128], prefix: prefix)) { error in
+                XCTAssert(error is NIOHPACKErrors.UnrepresentableInteger)
+            }
+        }
+    }
+
+    func testIntegerDecodingEmptyInput() throws {
+        for `prefix` in 1...8 {
+            XCTAssertThrowsError(try decodeInteger(from: [], prefix: prefix)) { error in
+                XCTAssert(error is NIOHPACKErrors.InsufficientInput)
+            }
+        }
+    }
+
+    func testIntegerDecodingNotEnoughBytes() throws {
+        for `prefix` in 1...8 {
+            XCTAssertThrowsError(try decodeInteger(from: [255, 128], prefix: prefix)) { error in
+                XCTAssert(error is NIOHPACKErrors.InsufficientInput)
+            }
+        }
+    }
 }