Fix integer overflow in McAsciiParser uint action (T267172046)

Reviewed By: alikhtarov

Differential Revision: D111235612

fbshipit-source-id: 5a99db12bc80c5443a127b6c014687fc0b0d55f4

Author: generatedunixname1383054420177565

mcrouter/lib/network/McAsciiParser.rl

@@ -7,6 +7,8 @@
 
 #include "mcrouter/lib/network/McAsciiParser.h"
 
+#include <limits>
+
 #include "mcrouter/lib/mc/msg.h"
 #include "mcrouter/lib/network/gen/MemcacheMessages.h"
 #include "mcrouter/lib/network/gen/MemcacheRoutingGroups.h"
@@ -109,6 +111,18 @@ multi_token = (print+ -- ( '\r' | '\n' )) >key_start %key_end %{
 
 # Unsigned integer value.
 uint = digit+ > { currentUInt_ = 0; } ${
+  // `digit+` is unbounded; without this guard the accumulator wraps modulo
+  // 2^64 and a 20+-digit field decodes to an attacker-chosen value (e.g. a
+  // value_bytes count that then slips past the maxValueBytes check).
+  if (FOLLY_UNLIKELY(
+          currentUInt_ >
+          (std::numeric_limits<uint64_t>::max() -
+           static_cast<uint64_t>(fc - '0')) /
+              10)) {
+    state_ = State::ERROR;
+    currentErrorDescription_ = "Integer field overflows uint64.";
+    fbreak;
+  }
   currentUInt_ = currentUInt_ * 10 + (fc - '0');
 };
 
@@ -1245,6 +1259,14 @@ McAsciiParserBase::State McServerAsciiParser::consume(folly::IOBuf& buffer) {
       appendKeyPiece(buffer, currentKey_, keyPieceStart_, p_);
     }
 
+    // A parser action may set State::ERROR directly (e.g. integer-field
+    // overflow) on grammatically valid input, without driving the machine to
+    // errorCs_. Honor it here so the request is rejected now, before a later
+    // finishReq() resets state_ and the truncated request is dispatched.
+    if (state_ == State::ERROR) {
+      break;
+    }
+
     if (savedCs_ == errorCs_) {
       handleError(buffer);
       break;

mcrouter/lib/network/test/McAsciiParserTest.cpp

@@ -270,6 +270,25 @@ TYPED_TEST(McAsciiParserTestGet, GetHit_Error) {
   h.runTest(1);
 }
 
+// A value-size field of exactly 2^64 overflows the uint64 accumulator. Before
+// the overflow guard it wrapped to 0 and the reply parsed as an empty value;
+// the parser must now reject it as a protocol error.
+TYPED_TEST(McAsciiParserTestGet, GetHit_ValueBytesOverflow_Error) {
+  McAsciiParserHarness h("VALUE test 10 18446744073709551616\r\n\r\nEND\r\n");
+  h.expectNext<TypeParam>(ReplyT<TypeParam>(), true);
+  h.runTest(1);
+}
+
+// The largest in-range uint64 (2^64 - 1) must still parse, confirming the
+// overflow guard rejects only genuine overflow and not the boundary value.
+TYPED_TEST(McAsciiParserTestGet, GetHit_MaxUint64Flags) {
+  McAsciiParserHarness h("VALUE test 18446744073709551615 2\r\nte\r\nEND\r\n");
+  h.expectNext<TypeParam>(setFlags(
+      setValue(ReplyT<TypeParam>(carbon::Result::FOUND), "te"),
+      18446744073709551615ULL));
+  h.runTest(1);
+}
+
 TYPED_TEST(McAsciiParserTestGet, GetMiss) {
   McAsciiParserHarness h("END\r\n");
   h.expectNext<TypeParam>(ReplyT<TypeParam>(carbon::Result::NOTFOUND));

mcrouter/lib/network/test/McServerAsciiParserTest.cpp

@@ -406,6 +406,14 @@ void setLikeTest(std::string opCmd) {
       strlen(kTestValue),
       kTestValue);
 
+  // A value-size field that overflows uint64 must be rejected rather than
+  // wrapping to a small size and truncating the value (which would let the
+  // trailing bytes be reparsed as a second, smuggled command).
+  TestRunner().expectError().run(
+      "{} test:stepan:1 123 651342 18446744073709551616\r\n{}\r\n",
+      opCmd,
+      kTestValue);
+
   // Test noreply.
   TestRunner()
       .expectNext(
@@ -478,6 +486,11 @@ void arithmeticTest(std::string opCmd) {
       .run(opCmd + " test:stepan:1 noreply\r\n")
       .run(opCmd + "     test:stepan:1       noreply  \r\n")
       .run(opCmd + " test:stepan:1   noreply \r\n");
+
+  // A delta that overflows uint64 must be rejected, not silently truncated to
+  // a wrapped value and dispatched.
+  TestRunner().expectError().run(
+      opCmd + " test:stepan:1 18446744073709551616\r\n");
 }
 
 } // namespace