Browse Source

Merge pull request from GHSA-4rhq-vq24-88gw

Motivation:

HTTP2ToRawGRPCStateMachine has a parse loop that will attempt to read
messages out of the received buffer. This parse loop is sometimes
recursive: if there are bytes still in the message buffer then we'll
recursively attempt to parse more data.

Swift doesn't do tail-recursion in this pass, so we'll add a frame here.
The minimum message size is 5 bytes: a compression byte and then 4
length bytes. It's therefore possible to produce a very deep call stack
in a single read by sending a long sequence of empty messages.

In fuzzing this call stack can get so deep that it blows the stack
entirely. This is harder to do from the actual network, but still
practical. Fortunately, real applications are usually protected by the
fact that an empty proto message is highly unlikely to be valid for the
specific endpoint that a peer hits.

Still, there's no reason to run the risk, and it's good to be
fuzzing-friendly.

Modifications:

Replace the recursion with looping.

Result:

No longer blow the stack when the peer produces a prodigious set of tiny
messages.

Co-authored-by: Cory Benfield <lukasa@apple.com>
George Barnett 4 years ago
parent
commit
60f33250d2

BIN
FuzzTesting/FailCases/clusterfuzz-testcase-minimized-ServerFuzzer-release-5134158417494016


BIN
FuzzTesting/FailCases/clusterfuzz-testcase-minimized-ServerFuzzer-release-5448955772141568


+ 45 - 32
Sources/GRPC/HTTP2ToRawGRPCServerCodec.swift

@@ -212,42 +212,55 @@ internal final class HTTP2ToRawGRPCServerCodec: ChannelInboundHandler, GRPCServe
 
   /// Try to read a request message from the buffer.
   private func tryReadingMessage() {
-    let action = self.state.readNextRequest(maxLength: self.maxReceiveMessageLength)
-    switch action {
-    case .none:
-      ()
+    // This while loop exists to break the recursion in `.forwardMessageThenReadNextMessage`.
+    // Almost all cases return directly out of the loop.
+    while true {
+      let action = self.state.readNextRequest(
+        maxLength: self.maxReceiveMessageLength
+      )
+      switch action {
+      case .none:
+        return
+
+      case let .forwardMessage(buffer):
+        switch self.configurationState {
+        case .notConfigured:
+          preconditionFailure()
+        case let .configured(handler):
+          handler.receiveMessage(buffer)
+        }
 
-    case let .forwardMessage(buffer):
-      switch self.configurationState {
-      case .notConfigured:
-        preconditionFailure()
-      case let .configured(handler):
-        handler.receiveMessage(buffer)
-      }
+        return
 
-    case let .forwardMessageThenReadNextMessage(buffer):
-      switch self.configurationState {
-      case .notConfigured:
-        preconditionFailure()
-      case let .configured(handler):
-        handler.receiveMessage(buffer)
-      }
-      self.tryReadingMessage()
+      case let .forwardMessageThenReadNextMessage(buffer):
+        switch self.configurationState {
+        case .notConfigured:
+          preconditionFailure()
+        case let .configured(handler):
+          handler.receiveMessage(buffer)
+        }
 
-    case .forwardEnd:
-      switch self.configurationState {
-      case .notConfigured:
-        preconditionFailure()
-      case let .configured(handler):
-        handler.receiveEnd()
-      }
+        continue
 
-    case let .errorCaught(error):
-      switch self.configurationState {
-      case .notConfigured:
-        preconditionFailure()
-      case let .configured(handler):
-        handler.receiveError(error)
+      case .forwardEnd:
+        switch self.configurationState {
+        case .notConfigured:
+          preconditionFailure()
+        case let .configured(handler):
+          handler.receiveEnd()
+        }
+
+        return
+
+      case let .errorCaught(error):
+        switch self.configurationState {
+        case .notConfigured:
+          preconditionFailure()
+        case let .configured(handler):
+          handler.receiveError(error)
+        }
+
+        return
       }
     }
   }

+ 10 - 0
Tests/GRPCTests/ServerFuzzingRegressionTests.swift

@@ -67,4 +67,14 @@ final class ServerFuzzingRegressionTests: GRPCTestCase {
     let name = "clusterfuzz-testcase-minimized-ServerFuzzer-release-5077460227063808"
     XCTAssertNoThrow(try self.runTest(withInputNamed: name))
   }
+
+  func testFuzzCase_release_5134158417494016() {
+    let name = "clusterfuzz-testcase-minimized-ServerFuzzer-release-5134158417494016"
+    XCTAssertNoThrow(try self.runTest(withInputNamed: name))
+  }
+
+  func testFuzzCase_release_5448955772141568() {
+    let name = "clusterfuzz-testcase-minimized-ServerFuzzer-release-5448955772141568"
+    XCTAssertNoThrow(try self.runTest(withInputNamed: name))
+  }
 }