Skip to content

Commit 1b3dc9e

Browse files
danieleggertdanieleggert
authored
FramingParser can crash when single CR is inside DQUOTE string (#779)
1 parent 5b2a96a commit 1b3dc9e

File tree

6 files changed

+103
-54
lines changed

6 files changed

+103
-54
lines changed

Diff for: ‎Sources/CLI/main.swift

+11-7
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,17 @@ logger.info("Great! Connecting to \(hostname)")
3131

3232
let sslContext = try NIOSSLContext(configuration: .clientDefault)
3333
let group = MultiThreadedEventLoopGroup(numberOfThreads: 1)
34-
let channel = try ClientBootstrap(group: group).channelInitializer { (channel) -> EventLoopFuture<Void> in
35-
channel.pipeline.addHandlers([
36-
try! NIOSSLClientHandler(context: sslContext, serverHostname: hostname),
37-
ResponseRoundtripHandler(logger: logger),
38-
CommandRoundtripHandler(logger: logger),
39-
])
40-
}.connect(host: hostname, port: 993).wait()
34+
let channel = try ClientBootstrap(group: group)
35+
.channelInitializer { (channel) -> EventLoopFuture<Void> in
36+
channel.eventLoop.makeCompletedFuture {
37+
try! channel.pipeline.syncOperations.addHandlers([
38+
try! NIOSSLClientHandler(context: sslContext, serverHostname: hostname),
39+
ResponseRoundtripHandler(logger: logger),
40+
CommandRoundtripHandler(logger: logger),
41+
])
42+
}
43+
}
44+
.connect(host: hostname, port: 993).wait()
4145

4246
_ = channel.closeFuture.always { result in
4347
switch result {

Diff for: ‎Sources/NIOIMAP/Client/FramingParser.swift

+35-17
Original file line numberDiff line numberDiff line change
@@ -343,22 +343,11 @@ extension FramingParser {
343343
private mutating func readByte_state_normalTraversal(lineFeedStrategy: LineFeedByteStrategy) -> FrameStatus {
344344
let byte = self.readByte()
345345
switch byte {
346-
case CR:
347-
self.readByte_state_foundCR()
348-
return .parsedCompleteFrame
349-
350-
case LF:
351-
switch lineFeedStrategy {
352-
case .ignoreFirst:
353-
precondition(self.frameLength == 1)
354-
self.stepBackAndIgnoreByte()
355-
self.state = .normalTraversal(.includeInFrame)
356-
return .continueParsing
357-
case .includeInFrame:
358-
// if we weren't meant to ignore the LF then it
359-
// must be the end of the current frame
360-
return .parsedCompleteFrame
361-
}
346+
case CR, LF:
347+
return processLineBreakByte_state(
348+
byte: byte,
349+
lineFeedStrategy: lineFeedStrategy
350+
)
362351

363352
case LITERAL_HEADER_START:
364353
self.state = .searchingForLiteralHeader(.findingBinaryFlag)
@@ -384,7 +373,10 @@ extension FramingParser {
384373
// parts (notably `text`) is allowed to have _any_ character
385374
// (except CR or LF).
386375
// Thus, fall through to “normal” state:
387-
return readByte_state_normalTraversal(lineFeedStrategy: .includeInFrame)
376+
return processLineBreakByte_state(
377+
byte: byte,
378+
lineFeedStrategy: .includeInFrame
379+
)
388380

389381
case (ESCAPE, .normal):
390382
self.state = .insideQuoted(.escaped)
@@ -403,6 +395,32 @@ extension FramingParser {
403395
}
404396
}
405397

398+
private mutating func processLineBreakByte_state(
399+
byte: UInt8,
400+
lineFeedStrategy: LineFeedByteStrategy
401+
) -> FrameStatus {
402+
switch byte {
403+
case CR:
404+
self.readByte_state_foundCR()
405+
return .parsedCompleteFrame
406+
407+
case LF:
408+
switch lineFeedStrategy {
409+
case .ignoreFirst:
410+
precondition(self.frameLength == 1)
411+
self.stepBackAndIgnoreByte()
412+
self.state = .normalTraversal(.includeInFrame)
413+
return .continueParsing
414+
case .includeInFrame:
415+
// if we weren't meant to ignore the LF then it
416+
// must be the end of the current frame
417+
return .parsedCompleteFrame
418+
}
419+
default:
420+
fatalError()
421+
}
422+
}
423+
406424
private mutating func readByte_state_foundCR() {
407425
guard let byte = self.peekByte() else {
408426
// As we don't yet have the next byte we have to assume

Diff for: ‎Sources/Proxy/MailClientToProxyHandler.swift

+13-11
Original file line numberDiff line numberDiff line change
@@ -37,17 +37,19 @@ class MailClientToProxyHandler: ChannelInboundHandler {
3737
let boundContext = NIOLoopBound(context, eventLoop: context.eventLoop)
3838
let boundSelf = NIOLoopBound(self, eventLoop: context.eventLoop)
3939
ClientBootstrap(group: context.eventLoop).channelInitializer { channel in
40-
let sslHandler = try! NIOSSLClientHandler(
41-
context: NIOSSLContext(configuration: .clientDefault),
42-
serverHostname: serverHost
43-
)
44-
return channel.pipeline.addHandlers([
45-
sslHandler,
46-
OutboundPrintHandler(type: "CLIENT (Encoded)"),
47-
InboundPrintHandler(type: "SERVER (Original)"),
48-
IMAPClientHandler(encodingChangeCallback: { _, _ in }),
49-
ProxyToMailServerHandler(mailAppToProxyChannel: mailClientToProxyChannel),
50-
])
40+
channel.eventLoop.makeCompletedFuture {
41+
let sslHandler = try! NIOSSLClientHandler(
42+
context: NIOSSLContext(configuration: .clientDefault),
43+
serverHostname: serverHost
44+
)
45+
try! channel.pipeline.syncOperations.addHandlers([
46+
sslHandler,
47+
OutboundPrintHandler(type: "CLIENT (Encoded)"),
48+
InboundPrintHandler(type: "SERVER (Original)"),
49+
IMAPClientHandler(encodingChangeCallback: { _, _ in }),
50+
ProxyToMailServerHandler(mailAppToProxyChannel: mailClientToProxyChannel),
51+
])
52+
}
5153
}.connect(host: serverHost, port: self.serverPort).map { channel in
5254
boundSelf.value.clientChannel = channel
5355
channel.closeFuture.whenSuccess {

Diff for: ‎Sources/Proxy/main.swift

+17-14
Original file line numberDiff line numberDiff line change
@@ -40,27 +40,30 @@ guard CommandLine.arguments.count == 5 else {
4040

4141
let host = CommandLine.arguments[1]
4242
guard let port = Int(CommandLine.arguments[2]) else {
43-
print("Invalid port, coudln't convert to an integer")
43+
print("Invalid port, couldn't convert to an integer")
4444
exit(1)
4545
}
4646

4747
let serverHost = CommandLine.arguments[3]
4848
guard let serverPort = Int(CommandLine.arguments[4]) else {
49-
print("Invalid server port, coudln't convert to an integer")
49+
print("Invalid server port, couldn't convert to an integer")
5050
exit(1)
5151
}
5252

5353
// MARK: - Run
5454

55-
try ServerBootstrap(group: eventLoopGroup).childChannelInitializer { channel -> EventLoopFuture<Void> in
56-
channel.pipeline.addHandlers([
57-
InboundPrintHandler(type: "CLIENT (Original)"),
58-
OutboundPrintHandler(type: "SERVER (Decoded)"),
59-
ByteToMessageHandler(FrameDecoder()),
60-
IMAPServerHandler(),
61-
MailClientToProxyHandler(serverHost: serverHost, serverPort: serverPort),
62-
])
63-
}
64-
.serverChannelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEADDR), value: 1)
65-
.bind(host: host, port: port).wait()
66-
.closeFuture.wait()
55+
try ServerBootstrap(group: eventLoopGroup)
56+
.childChannelInitializer { channel -> EventLoopFuture<Void> in
57+
channel.eventLoop.makeCompletedFuture {
58+
try! channel.pipeline.syncOperations.addHandlers([
59+
InboundPrintHandler(type: "CLIENT (Original)"),
60+
OutboundPrintHandler(type: "SERVER (Decoded)"),
61+
ByteToMessageHandler(FrameDecoder()),
62+
IMAPServerHandler(),
63+
MailClientToProxyHandler(serverHost: serverHost, serverPort: serverPort),
64+
])
65+
}
66+
}
67+
.serverChannelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEADDR), value: 1)
68+
.bind(host: host, port: port).wait()
69+
.closeFuture.wait()

Diff for: ‎Tests/NIOIMAPTests/FramingParserTests.swift

+20
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,26 @@ extension FramingParserTests {
5858
}
5959
}
6060

61+
func testSingleQuoteInLine_CR_lineBreak() {
62+
// RFC 3501 allows un-matched `"` in `text` parts, such as the text of an untagged OK response.
63+
//
64+
// Test that this:
65+
//
66+
// S: * OK Hello " foo
67+
// S: * NO Hello bar
68+
//
69+
// gets parsed into the corresponding two frames / lines.
70+
do {
71+
var buffer: ByteBuffer = "* OK Hello \" foo\r"
72+
XCTAssertEqual(try self.parser.appendAndFrameBuffer(&buffer), [.complete("* OK Hello \" foo\r")])
73+
}
74+
// Check that the next line parses:
75+
do {
76+
var buffer: ByteBuffer = "* NO Hello bar\r"
77+
XCTAssertEqual(try self.parser.appendAndFrameBuffer(&buffer), [.complete("* NO Hello bar\r")])
78+
}
79+
}
80+
6181
func testCommandWithQuoted() {
6282
var buffer: ByteBuffer = "A1 LOGIN \"foo\" \"bar\"\r\n"
6383
XCTAssertEqual(try self.parser.appendAndFrameBuffer(&buffer), [.complete("A1 LOGIN \"foo\" \"bar\"\r\n")])

Diff for: ‎Tests/NIOIMAPTests/IntegrationTests.swift

+7-5
Original file line numberDiff line numberDiff line change
@@ -62,11 +62,13 @@ final class ParserIntegrationTests: XCTestCase {
6262
server = try ServerBootstrap(group: group)
6363
.serverChannelOption(ChannelOptions.socket(.init(SOL_SOCKET), .init(SO_REUSEADDR)), value: 1)
6464
.childChannelInitializer { channel in
65-
channel.pipeline.addHandlers(
66-
ByteToMessageHandler(FrameDecoder()),
67-
IMAPServerHandler(),
68-
CollectEverythingHandler(collectionDonePromise: collectionDonePromise)
69-
)
65+
channel.eventLoop.makeCompletedFuture {
66+
try! channel.pipeline.syncOperations.addHandlers([
67+
ByteToMessageHandler(FrameDecoder()),
68+
IMAPServerHandler(),
69+
CollectEverythingHandler(collectionDonePromise: collectionDonePromise),
70+
])
71+
}
7072
}
7173
.bind(to: .init(ipAddress: "127.0.0.1", port: 0))
7274
.wait()

0 commit comments

Comments
 (0)