Skip to content

Commit

Permalink
Merge pull request from GHSA-xhhr-p2r9-jmm7
Browse files Browse the repository at this point in the history
Motivation:
NIOHTTPRequestDecompressor and HTTPResponseDecompressor are both affected by an issue where the decompression limits defined by their DecompressionLimit property wasn't correctly checked when is was set with DecompressionLimit.size(...), allowing denial of service attacks.

Modifications:
- Update DecompressionLimit.size(...) to correctly check the size of the decompressed data.
- Update test cases to avoid future regressions regarding the size checks.

Result:
Prevents DoS attacks though maliciously crafted compressed data.
  • Loading branch information
Trevör authored May 2, 2020
1 parent 020e322 commit f21a87d
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 40 deletions.
3 changes: 2 additions & 1 deletion Sources/NIOHTTPCompression/HTTPDecompression.swift
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public enum NIOHTTPDecompression {
private var limit: Limit

/// No limit will be set.
/// - warning: Setting `limit` to `.none` leaves you vulnerable to denial of service attacks.
public static let none = DecompressionLimit(limit: .none)
/// Limit will be set on the request body size.
public static func size(_ value: Int) -> DecompressionLimit { return DecompressionLimit(limit: .size(value)) }
Expand All @@ -38,7 +39,7 @@ public enum NIOHTTPDecompression {
case .none:
return false
case .size(let allowed):
return compressed > allowed
return decompressed > allowed
case .ratio(let ratio):
return decompressed > compressed * ratio
}
Expand Down
36 changes: 12 additions & 24 deletions Tests/NIOHTTPCompressionTests/HTTPRequestDecompressorTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,14 @@ class HTTPRequestDecompressorTest: XCTestCase {
func testDecompressionLimitRatio() throws {
let channel = EmbeddedChannel()
try channel.pipeline.addHandler(NIOHTTPRequestDecompressor(limit: .ratio(10))).wait()

let headers = HTTPHeaders([("Content-Encoding", "gzip"), ("Content-Length", "13")])
let decompressed = ByteBuffer.of(bytes: Array(repeating: 0, count: 500))
let compressed = compress(decompressed, "gzip")
let headers = HTTPHeaders([("Content-Encoding", "gzip"), ("Content-Length", "\(compressed.readableBytes)")])
try channel.writeInbound(HTTPServerRequestPart.head(.init(version: .init(major: 1, minor: 1), method: .POST, uri: "https://nio.swift.org/test", headers: headers)))

let buffer = ByteBuffer.of(bytes: [120, 156, 75, 76, 28, 5, 200, 0, 0, 248, 66, 103, 17])
let compressed = compress(buffer, "gzip")


do {
try channel.writeInbound(HTTPServerRequestPart.body(compressed))
XCTFail("writeShouldFail")
} catch let error as NIOHTTPDecompression.DecompressionError {
switch error {
case .limit:
Expand All @@ -78,16 +77,15 @@ class HTTPRequestDecompressorTest: XCTestCase {

func testDecompressionLimitSize() throws {
let channel = EmbeddedChannel()
try channel.pipeline.addHandler(NIOHTTPRequestDecompressor(limit: .size(10))).wait()

let headers = HTTPHeaders([("Content-Encoding", "gzip"), ("Content-Length", "13")])
let decompressed = ByteBuffer.of(bytes: Array(repeating: 0, count: 200))
let compressed = compress(decompressed, "gzip")
try channel.pipeline.addHandler(NIOHTTPRequestDecompressor(limit: .size(decompressed.readableBytes-1))).wait()
let headers = HTTPHeaders([("Content-Encoding", "gzip"), ("Content-Length", "\(compressed.readableBytes)")])
try channel.writeInbound(HTTPServerRequestPart.head(.init(version: .init(major: 1, minor: 1), method: .POST, uri: "https://nio.swift.org/test", headers: headers)))

let buffer = ByteBuffer.of(bytes: [120, 156, 75, 76, 28, 5, 200, 0, 0, 248, 66, 103, 17])
let compressed = compress(buffer, "gzip")


do {
try channel.writeInbound(HTTPServerRequestPart.body(compressed))
XCTFail("writeInbound should fail")
} catch let error as NIOHTTPDecompression.DecompressionError {
switch error {
case .limit:
Expand Down Expand Up @@ -120,17 +118,7 @@ class HTTPRequestDecompressorTest: XCTestCase {
try channel.writeInbound(HTTPServerRequestPart.head(.init(version: .init(major: 1, minor: 1), method: .POST, uri: "https://nio.swift.org/test", headers: headers)))
)

do {
try channel.writeInbound(HTTPServerRequestPart.body(compressed))
} catch let error as NIOHTTPDecompression.DecompressionError {
switch error {
case .limit:
// ok
break
default:
XCTFail("Unexptected error: \(error)")
}
}
XCTAssertNoThrow(try channel.writeInbound(HTTPServerRequestPart.body(compressed)))
}

XCTAssertNoThrow(try channel.writeInbound(HTTPServerRequestPart.end(nil)))
Expand Down
19 changes: 4 additions & 15 deletions Tests/NIOHTTPCompressionTests/HTTPResponseDecompressorTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class HTTPResponseDecompressorTest: XCTestCase {

do {
try channel.writeInbound(HTTPClientResponsePart.body(body))
XCTFail("writeInbound should fail")
} catch let error as NIOHTTPDecompression.DecompressionError {
switch error {
case .limit:
Expand All @@ -56,7 +57,7 @@ class HTTPResponseDecompressorTest: XCTestCase {

func testDecompressionLimitSize() throws {
let channel = EmbeddedChannel()
try channel.pipeline.addHandler(NIOHTTPResponseDecompressor(limit: .size(10))).wait()
try channel.pipeline.addHandler(NIOHTTPResponseDecompressor(limit: .size(15))).wait()

let headers = HTTPHeaders([("Content-Encoding", "deflate"), ("Content-Length", "13")])
try channel.writeInbound(HTTPClientResponsePart.head(.init(version: .init(major: 1, minor: 1), status: .ok, headers: headers)))
Expand All @@ -65,6 +66,7 @@ class HTTPResponseDecompressorTest: XCTestCase {

do {
try channel.writeInbound(HTTPClientResponsePart.body(body))
XCTFail("writeInbound should fail")
} catch let error as NIOHTTPDecompression.DecompressionError {
switch error {
case .limit:
Expand Down Expand Up @@ -99,20 +101,7 @@ class HTTPResponseDecompressorTest: XCTestCase {
headers.add(name: "Content-Length", value: "\(compressed.readableBytes)")

XCTAssertNoThrow(try channel.writeInbound(HTTPClientResponsePart.head(.init(version: .init(major: 1, minor: 1), status: .ok, headers: headers))))

do {
try channel.writeInbound(HTTPClientResponsePart.body(compressed))
} catch let error as NIOHTTPDecompression.DecompressionError {
switch error {
case .limit:
// ok
break
default:
XCTFail("Unexptected error: \(error)")
}
} catch {
XCTFail("Unexptected error: \(error)")
}
XCTAssertNoThrow(try channel.writeInbound(HTTPClientResponsePart.body(compressed)))
}

XCTAssertNoThrow(try channel.writeInbound(HTTPClientResponsePart.end(nil)))
Expand Down

0 comments on commit f21a87d

Please sign in to comment.