From 56c60a29b1207a731be62fb68cc98246942145ed Mon Sep 17 00:00:00 2001 From: George Barnett Date: Fri, 11 Feb 2022 09:18:39 +0000 Subject: [PATCH] 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 --- ...ized-ServerFuzzer-release-5635720084062208 | Bin 0 -> 109 bytes Sources/NIOHPACK/HPACKErrors.swift | 37 ++++--- Sources/NIOHPACK/IntegerCoding.swift | 101 ++++++++++++------ .../IntegerCodingTests+XCTest.swift | 5 + Tests/NIOHPACKTests/IntegerCodingTests.swift | 67 +++++++++--- 5 files changed, 148 insertions(+), 62 deletions(-) create mode 100644 FuzzTesting/FailCases/clusterfuzz-testcase-minimized-ServerFuzzer-release-5635720084062208 diff --git a/FuzzTesting/FailCases/clusterfuzz-testcase-minimized-ServerFuzzer-release-5635720084062208 b/FuzzTesting/FailCases/clusterfuzz-testcase-minimized-ServerFuzzer-release-5635720084062208 new file mode 100644 index 0000000000000000000000000000000000000000..66bb8c076ca45ff2806db90246cca8287e8148be GIT binary patch literal 109 zcmWFt@>I}L@CXSB&^OXE;N{}w3ibt&3=E2lEm*;SIACC4`1SvP%N7O(bp{T0#{ch` Qv@`Q|!KI-Lh$=7#0Er|yw*UYD literal 0 HcmV?d00001 diff --git a/Sources/NIOHPACK/HPACKErrors.swift b/Sources/NIOHPACK/HPACKErrors.swift index dca8249f..8c42e13b 100644 --- a/Sources/NIOHPACK/HPACKErrors.swift +++ b/Sources/NIOHPACK/HPACKErrors.swift @@ -23,11 +23,11 @@ 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. @@ -35,56 +35,56 @@ public enum NIOHPACKErrors { /// 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 : 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, rhs: NIOHPACKErrors.FailedToAddIndexedHeader) -> Bool { guard lhs.bytesNeeded == rhs.bytesNeeded else { return false @@ -92,14 +92,14 @@ public enum NIOHPACKErrors { 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() {} + } } diff --git a/Sources/NIOHPACK/IntegerCoding.swift b/Sources/NIOHPACK/IntegerCoding.swift index 6f754135..6ff5a311 100644 --- a/Sources/NIOHPACK/IntegerCoding.swift +++ b/Sources/NIOHPACK/IntegerCoding.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) } diff --git a/Tests/NIOHPACKTests/IntegerCodingTests+XCTest.swift b/Tests/NIOHPACKTests/IntegerCodingTests+XCTest.swift index e78a8b38..ca8f20a0 100644 --- a/Tests/NIOHPACKTests/IntegerCodingTests+XCTest.swift +++ b/Tests/NIOHPACKTests/IntegerCodingTests+XCTest.swift @@ -29,6 +29,11 @@ extension IntegerCodingTests { return [ ("testIntegerEncoding", testIntegerEncoding), ("testIntegerDecoding", testIntegerDecoding), + ("testIntegerDecodingMultiplicationDoesNotOverflow", testIntegerDecodingMultiplicationDoesNotOverflow), + ("testIntegerDecodingAdditionDoesNotOverflow", testIntegerDecodingAdditionDoesNotOverflow), + ("testIntegerDecodingShiftDoesNotOverflow", testIntegerDecodingShiftDoesNotOverflow), + ("testIntegerDecodingEmptyInput", testIntegerDecodingEmptyInput), + ("testIntegerDecodingNotEnoughBytes", testIntegerDecodingNotEnoughBytes), ] } } diff --git a/Tests/NIOHPACKTests/IntegerCodingTests.swift b/Tests/NIOHPACKTests/IntegerCodingTests.swift index 448e613d..504b719f 100644 --- a/Tests/NIOHPACKTests/IntegerCodingTests.swift +++ b/Tests/NIOHPACKTests/IntegerCodingTests.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) + } + } + } }