CVE-2019-11937: Uncontrolled Recursion (CWE-674)
value: In Mcrouter prior to v0.41.0, a large struct input provided to the Carbon protocol reader could result in stack exhaustion and denial of service.
CWE description: The product does not properly control the amount of recursion that takes place,  consuming excessive resources, such as allocated memory or the program stack.
CVSS v3 base score: 7.5
Published: 2019-12-04T16:15Z
Repository: https://github.com/facebook/mcrouter
Language: C++
Files changed: mcrouter/lib/carbon/CarbonProtocolReader.cpp, mcrouter/lib/carbon/CarbonProtocolReader.h
Commit date: 2019-11-04 07:04:09 -0800
Commit message: Attempt to make CarbonProtocolReader::skip tail recursive

Reviewed By: edenzik

Differential Revision: D17967570

fbshipit-source-id: fdc32e190a521349c7c8f4d6081902fa18eb0284

Diff: diff --git a/mcrouter/lib/carbon/CarbonProtocolReader.cpp b/mcrouter/lib/carbon/CarbonProtocolReader.cpp
index a4cc007c..67f89c9c 100644
--- a/mcrouter/lib/carbon/CarbonProtocolReader.cpp
+++ b/mcrouter/lib/carbon/CarbonProtocolReader.cpp
@@ -76,17 +76,15 @@ void CarbonProtocolReader::skip(const FieldType ft) {
       skipLinearContainer();
       break;
     }
     case FieldType::Struct: {
       readStructBegin();
-      while (true) {
-        const auto fieldType = readFieldHeader().first;
-        if (fieldType == FieldType::Stop) {
-          break;
-        }
-        skip(fieldType);
-      }
+      const auto next = readFieldHeader().first;
+      skip(next);
+      break;
+    }
+    case FieldType::Stop: {
       readStructEnd();
       break;
     }
     case FieldType::Set: {
       skipLinearContainer();
@@ -94,10 +92,12 @@ void CarbonProtocolReader::skip(const FieldType ft) {
     }
     case FieldType::Map: {
       skipKVContainer();
       break;
     }
-    default: { break; }
+    default: {
+      break;
+    }
   }
 }
 
-} // carbon
+} // namespace carbon
diff --git a/mcrouter/lib/carbon/CarbonProtocolReader.h b/mcrouter/lib/carbon/CarbonProtocolReader.h
index 5ec74365..083283f1 100644
--- a/mcrouter/lib/carbon/CarbonProtocolReader.h
+++ b/mcrouter/lib/carbon/CarbonProtocolReader.h
@@ -243,12 +243,14 @@ class CarbonProtocolReader {
     nestedStructFieldIds_.push_back(lastFieldId_);
     lastFieldId_ = 0;
   }
 
   void readStructEnd() {
-    lastFieldId_ = nestedStructFieldIds_.back();
-    nestedStructFieldIds_.pop_back();
+    if (!nestedStructFieldIds_.empty()) {
+      lastFieldId_ = nestedStructFieldIds_.back();
+      nestedStructFieldIds_.pop_back();
+    }
   }
 
   std::pair<std::pair<FieldType, FieldType>, uint32_t>
   readKVContainerFieldSizeAndInnerTypes() {
     std::pair<std::pair<FieldType, FieldType>, uint32_t> pr;

Vulnerable code:       while (true) {
        const auto fieldType = readFieldHeader().first;
        if (fieldType == FieldType::Stop) {
          break;
        }
        skip(fieldType);
      }
    default: { break; }
} // carbon
    lastFieldId_ = nestedStructFieldIds_.back();
    nestedStructFieldIds_.pop_back();
Fixed code:       const auto next = readFieldHeader().first;
      skip(next);
      break;
    }
    case FieldType::Stop: {
    default: {
      break;
    }
} // namespace carbon
    if (!nestedStructFieldIds_.empty()) {
      lastFieldId_ = nestedStructFieldIds_.back();
      nestedStructFieldIds_.pop_back();
    }