Skip to content

Commit

Permalink
Police correct content-length headers. (apple#98)
Browse files Browse the repository at this point in the history
Motivation:

RFC 7540 requires that compliant specifications emit errors if content-length has been incorrectly
set. We should have this behaviour by default, though we should allow it to be disabled if necessary.

Modifications:

- Added content-length checking.
- Added flag to disable content-length checking.

Result:

Better RFC 7540 compliance.
  • Loading branch information
Lukasa authored and weissi committed Apr 15, 2019
1 parent 08db9d8 commit aaaeca7
Show file tree
Hide file tree
Showing 12 changed files with 908 additions and 377 deletions.

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ protocol ReceivingDataState: HasFlowControlWindows {

extension ReceivingDataState {
/// Called to receive a DATA frame.
mutating func receiveData(streamID: HTTP2StreamID, flowControlledBytes: Int, isEndStreamSet endStream: Bool) -> StateMachineResultWithEffect {
mutating func receiveData(streamID: HTTP2StreamID, contentLength: Int, flowControlledBytes: Int, isEndStreamSet endStream: Bool) -> StateMachineResultWithEffect {
do {
try self.inboundFlowControlWindow.consume(flowControlledBytes: flowControlledBytes)
} catch let error where error is NIOHTTP2Errors.FlowControlViolation {
Expand All @@ -34,7 +34,7 @@ extension ReceivingDataState {
}

let result = self.streamState.modifyStreamState(streamID: streamID, ignoreRecentlyReset: true) {
$0.receiveData(flowControlledBytes: flowControlledBytes, isEndStreamSet: endStream)
$0.receiveData(contentLength: contentLength, flowControlledBytes: flowControlledBytes, isEndStreamSet: endStream)
}

// We need to be a bit careful here. The backing code may have triggered either an ignoreFrame or streamError. While both of these
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ protocol ReceivingHeadersState: HasFlowControlWindows {

var headerBlockValidation: HTTP2ConnectionStateMachine.ValidationState { get }

var contentLengthValidation: HTTP2ConnectionStateMachine.ValidationState { get }

var streamState: ConnectionStreamState { get set }

var localInitialWindowSize: UInt32 { get }
Expand All @@ -34,19 +36,20 @@ extension ReceivingHeadersState {
mutating func receiveHeaders(streamID: HTTP2StreamID, headers: HPACKHeaders, isEndStreamSet endStream: Bool) -> StateMachineResultWithEffect {
let result: StateMachineResultWithStreamEffect
let validateHeaderBlock = self.headerBlockValidation == .enabled
let validateContentLength = self.contentLengthValidation == .enabled

if self.role == .server && streamID.mayBeInitiatedBy(.client) {
do {
result = try self.streamState.modifyStreamStateCreateIfNeeded(streamID: streamID, localRole: .server, localInitialWindowSize: self.localInitialWindowSize, remoteInitialWindowSize: self.remoteInitialWindowSize) {
$0.receiveHeaders(headers: headers, validateHeaderBlock: validateHeaderBlock, isEndStreamSet: endStream)
$0.receiveHeaders(headers: headers, validateHeaderBlock: validateHeaderBlock, validateContentLength: validateContentLength, isEndStreamSet: endStream)
}
} catch {
return StateMachineResultWithEffect(result: .connectionError(underlyingError: error, type: .protocolError), effect: nil)
}
} else {
// HEADERS cannot create streams for servers, so this must be for a stream we already know about.
result = self.streamState.modifyStreamState(streamID: streamID, ignoreRecentlyReset: true) {
$0.receiveHeaders(headers: headers, validateHeaderBlock: validateHeaderBlock, isEndStreamSet: endStream)
$0.receiveHeaders(headers: headers, validateHeaderBlock: validateHeaderBlock, validateContentLength: validateContentLength, isEndStreamSet: endStream)
}
}

Expand All @@ -63,14 +66,15 @@ extension ReceivingHeadersState where Self: LocallyQuiescingState {
/// new streams.
mutating func receiveHeaders(streamID: HTTP2StreamID, headers: HPACKHeaders, isEndStreamSet endStream: Bool) -> StateMachineResultWithEffect {
let validateHeaderBlock = self.headerBlockValidation == .enabled
let validateContentLength = self.contentLengthValidation == .enabled

if streamID.mayBeInitiatedBy(.client) && streamID > self.lastRemoteStreamID {
return StateMachineResultWithEffect(result: .ignoreFrame, effect: nil)
}

// At this stage we've quiesced, so the remote peer is not allowed to create new streams.
let result = self.streamState.modifyStreamState(streamID: streamID, ignoreRecentlyReset: true) {
$0.receiveHeaders(headers: headers, validateHeaderBlock: validateHeaderBlock, isEndStreamSet: endStream)
$0.receiveHeaders(headers: headers, validateHeaderBlock: validateHeaderBlock, validateContentLength: validateContentLength, isEndStreamSet: endStream)
}
return StateMachineResultWithEffect(result, connectionState: self)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ protocol SendingDataState: HasFlowControlWindows {

extension SendingDataState {
/// Called to send a DATA frame.
mutating func sendData(streamID: HTTP2StreamID, flowControlledBytes: Int, isEndStreamSet endStream: Bool) -> StateMachineResultWithEffect {
mutating func sendData(streamID: HTTP2StreamID, contentLength: Int, flowControlledBytes: Int, isEndStreamSet endStream: Bool) -> StateMachineResultWithEffect {
do {
try self.outboundFlowControlWindow.consume(flowControlledBytes: flowControlledBytes)
} catch let error where error is NIOHTTP2Errors.FlowControlViolation {
Expand All @@ -34,7 +34,7 @@ extension SendingDataState {
}

let result = self.streamState.modifyStreamState(streamID: streamID, ignoreRecentlyReset: false) {
$0.sendData(flowControlledBytes: flowControlledBytes, isEndStreamSet: endStream)
$0.sendData(contentLength: contentLength, flowControlledBytes: flowControlledBytes, isEndStreamSet: endStream)
}
return StateMachineResultWithEffect(result, connectionState: self)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ protocol SendingHeadersState: HasFlowControlWindows {

var headerBlockValidation: HTTP2ConnectionStateMachine.ValidationState { get }

var contentLengthValidation: HTTP2ConnectionStateMachine.ValidationState { get }

var streamState: ConnectionStreamState { get set }

var localInitialWindowSize: UInt32 { get }
Expand All @@ -35,22 +37,23 @@ extension SendingHeadersState {
mutating func sendHeaders(streamID: HTTP2StreamID, headers: HPACKHeaders, isEndStreamSet endStream: Bool) -> StateMachineResultWithEffect {
let result: StateMachineResultWithStreamEffect
let validateHeaderBlock = self.headerBlockValidation == .enabled
let validateContentLength = self.contentLengthValidation == .enabled

if self.role == .client && streamID.mayBeInitiatedBy(.client) {
do {
result = try self.streamState.modifyStreamStateCreateIfNeeded(streamID: streamID,
localRole: .client,
localInitialWindowSize: self.localInitialWindowSize,
remoteInitialWindowSize: self.remoteInitialWindowSize) {
$0.sendHeaders(headers: headers, validateHeaderBlock: validateHeaderBlock, isEndStreamSet: endStream)
$0.sendHeaders(headers: headers, validateHeaderBlock: validateHeaderBlock, validateContentLength: validateContentLength, isEndStreamSet: endStream)
}
} catch {
return StateMachineResultWithEffect(result: .connectionError(underlyingError: error, type: .protocolError), effect: nil)
}
} else {
// HEADERS cannot create streams for servers, so this must be for a stream we already know about.
result = self.streamState.modifyStreamState(streamID: streamID, ignoreRecentlyReset: false) {
$0.sendHeaders(headers: headers, validateHeaderBlock: validateHeaderBlock, isEndStreamSet: endStream)
$0.sendHeaders(headers: headers, validateHeaderBlock: validateHeaderBlock, validateContentLength: validateContentLength, isEndStreamSet: endStream)
}
}

Expand All @@ -64,9 +67,10 @@ extension SendingHeadersState where Self: RemotelyQuiescingState {
/// be modifying an existing one.
mutating func sendHeaders(streamID: HTTP2StreamID, headers: HPACKHeaders, isEndStreamSet endStream: Bool) -> StateMachineResultWithEffect {
let validateHeaderBlock = self.headerBlockValidation == .enabled
let validateContentLength = self.contentLengthValidation == .enabled

let result = self.streamState.modifyStreamState(streamID: streamID, ignoreRecentlyReset: false) {
$0.sendHeaders(headers: headers, validateHeaderBlock: validateHeaderBlock, isEndStreamSet: endStream)
$0.sendHeaders(headers: headers, validateHeaderBlock: validateHeaderBlock, validateContentLength: validateContentLength, isEndStreamSet: endStream)
}
return StateMachineResultWithEffect(result, connectionState: self)
}
Expand Down
65 changes: 65 additions & 0 deletions Sources/NIOHTTP2/ContentLengthVerifier.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
//===----------------------------------------------------------------------===//
//
// 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 NIOHPACK

/// An object that verifies that a content-length field on a HTTP request or
/// response is respected.
struct ContentLengthVerifier {
private var expectedContentLength: Int?
}

extension ContentLengthVerifier {
/// A chunk of data has been received from the network.
mutating func receivedDataChunk(length: Int) throws {
assert(length >= 0, "received data chunks must be positive")

// If there was no content-length, don't keep track.
guard let expectedContentLength = self.expectedContentLength else {
return
}

let newContentLength = expectedContentLength - length
if newContentLength < 0 {
throw NIOHTTP2Errors.ContentLengthViolated()
}
self.expectedContentLength = newContentLength
}

/// Called when end of stream has been received. Validates that the complete body was received.
func endOfStream() throws {
switch self.expectedContentLength {
case .none, .some(0):
break
default:
throw NIOHTTP2Errors.ContentLengthViolated()
}
}
}

extension ContentLengthVerifier {
internal init(_ headers: HPACKHeaders) {
self.expectedContentLength = headers[canonicalForm: "content-length"].first.flatMap { Int($0, radix: 10) }
}

/// The verifier for use when content length verification is disabled.
internal static var disabled: ContentLengthVerifier {
return ContentLengthVerifier(expectedContentLength: nil)
}
}

extension ContentLengthVerifier: CustomStringConvertible {
var description: String {
return "ContentLengthVerifier(length: \(String(describing: self.expectedContentLength)))"
}
}
8 changes: 4 additions & 4 deletions Sources/NIOHTTP2/HTTP2ChannelHandler.swift
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ public final class NIOHTTP2Handler: ChannelDuplexHandler {
case disabled
}

public init(mode: ParserMode, initialSettings: HTTP2Settings = nioDefaultSettings, headerBlockValidation: ValidationState = .enabled) {
self.stateMachine = HTTP2ConnectionStateMachine(role: .init(mode), headerBlockValidation: .init(headerBlockValidation))
public init(mode: ParserMode, initialSettings: HTTP2Settings = nioDefaultSettings, headerBlockValidation: ValidationState = .enabled, contentLengthValidation: ValidationState = .enabled) {
self.stateMachine = HTTP2ConnectionStateMachine(role: .init(mode), headerBlockValidation: .init(headerBlockValidation), contentLengthValidation: .init(contentLengthValidation))
self.mode = mode
self.initialSettings = initialSettings
self.outboundBuffer = CompoundOutboundBuffer(mode: mode, initialMaxOutboundStreams: 100)
Expand Down Expand Up @@ -220,7 +220,7 @@ extension NIOHTTP2Handler {
// TODO(cory): Implement
fatalError("Currently some frames are unhandled.")
case .data(let dataBody):
result = self.stateMachine.receiveData(streamID: frame.streamID, flowControlledBytes: flowControlledLength, isEndStreamSet: dataBody.endStream)
result = self.stateMachine.receiveData(streamID: frame.streamID, contentLength: dataBody.data.readableBytes, flowControlledBytes: flowControlledLength, isEndStreamSet: dataBody.endStream)
case .goAway(let lastStreamID, _, _):
result = self.stateMachine.receiveGoaway(lastStreamID: lastStreamID)
case .headers(let headerBody):
Expand Down Expand Up @@ -368,7 +368,7 @@ extension NIOHTTP2Handler {
fatalError("Currently some frames are unhandled.")
case .data(let data):
// TODO(cory): Correctly account for padding data.
result = self.stateMachine.sendData(streamID: frame.streamID, flowControlledBytes: data.data.readableBytes, isEndStreamSet: data.endStream)
result = self.stateMachine.sendData(streamID: frame.streamID, contentLength: data.data.readableBytes, flowControlledBytes: data.data.readableBytes, isEndStreamSet: data.endStream)
case .goAway(let lastStreamID, _, _):
result = self.stateMachine.sendGoaway(lastStreamID: lastStreamID)
case .headers(let headerContent):
Expand Down
5 changes: 5 additions & 0 deletions Sources/NIOHTTP2/HTTP2Error.swift
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,11 @@ public enum NIOHTTP2Errors {
self.value = value
}
}

/// A request or response has violated the expected content length, either exceeding or falling beneath it.
public struct ContentLengthViolated: NIOHTTP2Error {
public init() { }
}
}


Expand Down
Loading

0 comments on commit aaaeca7

Please sign in to comment.