Skip to content

Commit 937c458

Browse files
LukasaLukasa
committed
Forbid HTTP protocols other than 1.
Motivation: Our HTTP code handles only HTTP/1.X. There is no reason to support HTTP/0.9, and we cannot safely handle a major protocol higher than 1 in this code, so we should simply treat requests/responses claiming to be of those protocols as errors. Modifications: HTTPDecoder now checks the major version is equal to 1 before it continues with parsing. If it hits an error, that error will be propagated out to the user. Result: Better resilience against bad HTTP messages.
1 parent d58fa23 commit 937c458

File tree

4 files changed

+248
-0
lines changed

4 files changed

+248
-0
lines changed

‎Sources/NIOHTTP1/HTTPDecoder.swift

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ private struct HTTPParserState {
2525
var readerIndexAdjustment = 0
2626
// This is set before http_parser_execute(...) is called and set to nil again after it finish
2727
var baseAddress: UnsafePointer<UInt8>?
28+
var currentError: HTTPParserError?
2829

2930
enum DataAwaitingState {
3031
case messageBegin
@@ -235,10 +236,20 @@ public class HTTPDecoder<HTTPMessageT>: ByteToMessageDecoder, AnyHTTPDecoder {
235236
switch handler {
236237
case let handler as HTTPRequestDecoder:
237238
let head = handler.newRequestHead(parser)
239+
guard head.version.major == 1 else {
240+
handler.state.currentError = HTTPParserError.invalidVersion
241+
return -1
242+
}
243+
238244
handler.pendingInOut.append(handler.wrapInboundOut(HTTPServerRequestPart.head(head)))
239245
return 0
240246
case let handler as HTTPResponseDecoder:
241247
let head = handler.newResponseHead(parser)
248+
guard head.version.major == 1 else {
249+
handler.state.currentError = HTTPParserError.invalidVersion
250+
return -1
251+
}
252+
242253
handler.pendingInOut.append(handler.wrapInboundOut(HTTPClientResponsePart.head(head)))
243254

244255
// http_parser doesn't correctly handle responses to HEAD requests. We have to do something
@@ -368,6 +379,10 @@ public class HTTPDecoder<HTTPMessageT>: ByteToMessageDecoder, AnyHTTPDecoder {
368379
c_nio_http_parser_execute(&parser, &settings, p.advanced(by: buffer.readerIndex), buffer.readableBytes)
369380
})
370381

382+
if let error = state.currentError {
383+
throw error
384+
}
385+
371386
state.baseAddress = nil
372387

373388
let errno = parser.http_errno

‎Tests/LinuxMain.swift

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ import XCTest
5252
testCase(EventLoopTest.allTests),
5353
testCase(FileRegionTest.allTests),
5454
testCase(GetaddrinfoResolverTest.allTests),
55+
testCase(HTTPDecoderTest.allTests),
5556
testCase(HTTPHeadersTest.allTests),
5657
testCase(HTTPRequestEncoderTests.allTests),
5758
testCase(HTTPResponseCompressorTest.allTests),

‎Tests/NIOHTTP1Tests/HTTPDecoderTest+XCTest.swift

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
//===----------------------------------------------------------------------===//
2+
//
3+
// This source file is part of the SwiftNIO open source project
4+
//
5+
// Copyright (c) 2017-2018 Apple Inc. and the SwiftNIO project authors
6+
// Licensed under Apache License v2.0
7+
//
8+
// See LICENSE.txt for license information
9+
// See CONTRIBUTORS.txt for the list of SwiftNIO project authors
10+
//
11+
// SPDX-License-Identifier: Apache-2.0
12+
//
13+
//===----------------------------------------------------------------------===//
14+
//
15+
// HTTPDecoderTest+XCTest.swift
16+
//
17+
import XCTest
18+
19+
///
20+
/// NOTE: This file was generated by generate_linux_tests.rb
21+
///
22+
/// Do NOT edit this file directly as it will be regenerated automatically when needed.
23+
///
24+
25+
extension HTTPDecoderTest {
26+
27+
static var allTests : [(String, (HTTPDecoderTest) -> () throws -> Void)] {
28+
return [
29+
("testDoesNotDecodeRealHTTP09Request", testDoesNotDecodeRealHTTP09Request),
30+
("testDoesNotDecodeFakeHTTP09Request", testDoesNotDecodeFakeHTTP09Request),
31+
("testDoesNotDecodeHTTP2XRequest", testDoesNotDecodeHTTP2XRequest),
32+
("testToleratesHTTP13Request", testToleratesHTTP13Request),
33+
("testDoesNotDecodeRealHTTP09Response", testDoesNotDecodeRealHTTP09Response),
34+
("testDoesNotDecodeFakeHTTP09Response", testDoesNotDecodeFakeHTTP09Response),
35+
("testDoesNotDecodeHTTP2XResponse", testDoesNotDecodeHTTP2XResponse),
36+
("testToleratesHTTP13Response", testToleratesHTTP13Response),
37+
]
38+
}
39+
}
40+

‎Tests/NIOHTTP1Tests/HTTPDecoderTest.swift

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,192 @@
1+
//===----------------------------------------------------------------------===//
2+
//
3+
// This source file is part of the SwiftNIO open source project
4+
//
5+
// Copyright (c) 2017-2018 Apple Inc. and the SwiftNIO project authors
6+
// Licensed under Apache License v2.0
7+
//
8+
// See LICENSE.txt for license information
9+
// See CONTRIBUTORS.txt for the list of SwiftNIO project authors
10+
//
11+
// SPDX-License-Identifier: Apache-2.0
12+
//
13+
//===----------------------------------------------------------------------===//
14+
15+
import XCTest
16+
import Dispatch
17+
import NIO
18+
import NIOHTTP1
19+
20+
class HTTPDecoderTest: XCTestCase {
21+
private var channel: EmbeddedChannel!
22+
private var loop: EmbeddedEventLoop!
23+
24+
override func setUp() {
25+
self.channel = EmbeddedChannel()
26+
self.loop = channel.eventLoop as! EmbeddedEventLoop
27+
}
28+
29+
override func tearDown() {
30+
self.channel = nil
31+
self.loop = nil
32+
}
33+
34+
func testDoesNotDecodeRealHTTP09Request() throws {
35+
XCTAssertNoThrow(try channel.pipeline.add(handler: HTTPRequestDecoder()).wait())
36+
37+
// This is an invalid HTTP/0.9 simple request (too many CRLFs), but we need to
38+
// trigger https://github.com/nodejs/http-parser/issues/386 or http_parser won't
39+
// actually parse this at all.
40+
var buffer = channel.allocator.buffer(capacity: 64)
41+
buffer.write(staticString: "GET /a-file\r\n\r\n")
42+
43+
do {
44+
try channel.writeInbound(buffer)
45+
XCTFail("Did not error")
46+
} catch HTTPParserError.invalidVersion {
47+
// ok
48+
} catch {
49+
XCTFail("Unexpected error: \(error)")
50+
}
51+
52+
loop.run()
53+
}
54+
55+
func testDoesNotDecodeFakeHTTP09Request() throws {
56+
XCTAssertNoThrow(try channel.pipeline.add(handler: HTTPRequestDecoder()).wait())
57+
58+
// This is a HTTP/1.1-formatted request that claims to be HTTP/0.9.
59+
var buffer = channel.allocator.buffer(capacity: 64)
60+
buffer.write(staticString: "GET / HTTP/0.9\r\nHost: whatever\r\n\r\n")
61+
62+
do {
63+
try channel.writeInbound(buffer)
64+
XCTFail("Did not error")
65+
} catch HTTPParserError.invalidVersion {
66+
// ok
67+
} catch {
68+
XCTFail("Unexpected error: \(error)")
69+
}
70+
71+
loop.run()
72+
}
73+
74+
func testDoesNotDecodeHTTP2XRequest() throws {
75+
XCTAssertNoThrow(try channel.pipeline.add(handler: HTTPRequestDecoder()).wait())
76+
77+
// This is a hypothetical HTTP/2.0 protocol request, assuming it is
78+
// byte for byte identical (which such a protocol would never be).
79+
var buffer = channel.allocator.buffer(capacity: 64)
80+
buffer.write(staticString: "GET / HTTP/2.0\r\nHost: whatever\r\n\r\n")
81+
82+
do {
83+
try channel.writeInbound(buffer)
84+
XCTFail("Did not error")
85+
} catch HTTPParserError.invalidVersion {
86+
// ok
87+
} catch {
88+
XCTFail("Unexpected error: \(error)")
89+
}
90+
91+
loop.run()
92+
}
93+
94+
func testToleratesHTTP13Request() throws {
95+
XCTAssertNoThrow(try channel.pipeline.add(handler: HTTPRequestDecoder()).wait())
96+
97+
// We tolerate higher versions of HTTP/1 than we know about because RFC 7230
98+
// says that these should be treated like HTTP/1.1 by our users.
99+
var buffer = channel.allocator.buffer(capacity: 64)
100+
buffer.write(staticString: "GET / HTTP/1.3\r\nHost: whatever\r\n\r\n")
101+
102+
XCTAssertNoThrow(try channel.writeInbound(buffer))
103+
XCTAssertNoThrow(try channel.finish())
104+
}
105+
106+
func testDoesNotDecodeRealHTTP09Response() throws {
107+
XCTAssertNoThrow(try channel.pipeline.add(handler: HTTPRequestEncoder()).wait())
108+
XCTAssertNoThrow(try channel.pipeline.add(handler: HTTPResponseDecoder()).wait())
109+
110+
// We need to prime the decoder by seeing a GET request.
111+
try channel.writeOutbound(HTTPClientRequestPart.head(HTTPRequestHead(version: .init(major: 0, minor: 9), method: .GET, uri: "/")))
112+
113+
// The HTTP parser has no special logic for HTTP/0.9 simple responses, but we'll send
114+
// one anyway just to prove it explodes.
115+
var buffer = channel.allocator.buffer(capacity: 64)
116+
buffer.write(staticString: "This is file data\n")
117+
118+
do {
119+
try channel.writeInbound(buffer)
120+
XCTFail("Did not error")
121+
} catch HTTPParserError.invalidConstant {
122+
// ok
123+
} catch {
124+
XCTFail("Unexpected error: \(error)")
125+
}
126+
127+
loop.run()
128+
}
129+
130+
func testDoesNotDecodeFakeHTTP09Response() throws {
131+
XCTAssertNoThrow(try channel.pipeline.add(handler: HTTPRequestEncoder()).wait())
132+
XCTAssertNoThrow(try channel.pipeline.add(handler: HTTPResponseDecoder()).wait())
133+
134+
// We need to prime the decoder by seeing a GET request.
135+
try channel.writeOutbound(HTTPClientRequestPart.head(HTTPRequestHead(version: .init(major: 0, minor: 9), method: .GET, uri: "/")))
136+
137+
// The HTTP parser rejects HTTP/1.1-formatted responses claiming 0.9 as a version.
138+
var buffer = channel.allocator.buffer(capacity: 64)
139+
buffer.write(staticString: "HTTP/0.9 200 OK\r\nServer: whatever\r\n\r\n")
140+
141+
do {
142+
try channel.writeInbound(buffer)
143+
XCTFail("Did not error")
144+
} catch HTTPParserError.invalidVersion {
145+
// ok
146+
} catch {
147+
XCTFail("Unexpected error: \(error)")
148+
}
149+
150+
loop.run()
151+
}
152+
153+
func testDoesNotDecodeHTTP2XResponse() throws {
154+
XCTAssertNoThrow(try channel.pipeline.add(handler: HTTPRequestEncoder()).wait())
155+
XCTAssertNoThrow(try channel.pipeline.add(handler: HTTPResponseDecoder()).wait())
156+
157+
// We need to prime the decoder by seeing a GET request.
158+
try channel.writeOutbound(HTTPClientRequestPart.head(HTTPRequestHead(version: .init(major: 2, minor: 0), method: .GET, uri: "/")))
159+
160+
// This is a hypothetical HTTP/2.0 protocol response, assuming it is
161+
// byte for byte identical (which such a protocol would never be).
162+
var buffer = channel.allocator.buffer(capacity: 64)
163+
buffer.write(staticString: "HTTP/2.0 200 OK\r\nServer: whatever\r\n\r\n")
164+
165+
do {
166+
try channel.writeInbound(buffer)
167+
XCTFail("Did not error")
168+
} catch HTTPParserError.invalidVersion {
169+
// ok
170+
} catch {
171+
XCTFail("Unexpected error: \(error)")
172+
}
173+
174+
loop.run()
175+
}
176+
177+
func testToleratesHTTP13Response() throws {
178+
XCTAssertNoThrow(try channel.pipeline.add(handler: HTTPRequestEncoder()).wait())
179+
XCTAssertNoThrow(try channel.pipeline.add(handler: HTTPResponseDecoder()).wait())
180+
181+
// We need to prime the decoder by seeing a GET request.
182+
try channel.writeOutbound(HTTPClientRequestPart.head(HTTPRequestHead(version: .init(major: 2, minor: 0), method: .GET, uri: "/")))
183+
184+
// We tolerate higher versions of HTTP/1 than we know about because RFC 7230
185+
// says that these should be treated like HTTP/1.1 by our users.
186+
var buffer = channel.allocator.buffer(capacity: 64)
187+
buffer.write(staticString: "HTTP/1.3 200 OK\r\nServer: whatever\r\n\r\n")
188+
189+
XCTAssertNoThrow(try channel.writeInbound(buffer))
190+
XCTAssertNoThrow(try channel.finish())
191+
}
192+
}

0 commit comments

Comments
 (0)