CVE-2021-24028: Release of Invalid Pointer or Reference (CWE-763)
value: An invalid free in Thrift's table-based serialization can cause the application to crash or potentially result in code execution or other undesirable effects. This issue affects Facebook Thrift prior to v2021.02.22.00.
CWE description: The product attempts to return a memory resource to the system, but it calls the wrong release function or calls the appropriate release function incorrectly.
CVSS v3 base score: 9.8
Published: 2021-04-14T00:15Z
Repository: https://github.com/facebook/fbthrift
Language: C++
Files changed: thrift/lib/cpp2/protocol/TableBasedSerializer.cpp, thrift/test/tablebased/SerializerTest.cpp, thrift/test/tablebased/thrift_tablebased.thrift
Commit date: 2021-02-18 17:20:23 -0800
Commit message: Fix handling of invalid union data in table-based serializer

Summary:
Fix handling of invalid union data in the table-based serializer. Previously if the input contained duplicate union data, previous active member of the union was overwritten without calling the destructor of the old object, potentially causing a memory leak. In addition to that, if the second piece of data was incomplete the wrong destructor would be called during stack unwinding causing a segfault, data corruption or other undesirable effects.

Fix the issue by clearing the union if there is an active member.

Also fix the type of the data member that holds the active field id (it's `int`, not `FieldID`).

Reviewed By: yfeldblum

Differential Revision: D26440248

fbshipit-source-id: fae9ab96566cf07e14dabe9663b2beb680a01bb4

Diff: diff --git a/thrift/lib/cpp2/protocol/TableBasedSerializer.cpp b/thrift/lib/cpp2/protocol/TableBasedSerializer.cpp
index 58cfbf1e62..844f59ca1a 100644
--- a/thrift/lib/cpp2/protocol/TableBasedSerializer.cpp
+++ b/thrift/lib/cpp2/protocol/TableBasedSerializer.cpp
@@ -184,13 +184,15 @@ const FieldInfo* FOLLY_NULLABLE findFieldInfo(
     }
   }
   return nullptr;
 }
 
-const FieldID& activeUnionMemberId(const void* object, ptrdiff_t offset) {
-  return *reinterpret_cast<const FieldID*>(
-      offset + static_cast<const char*>(object));
+// Returns a reference to the data member that holds the active field id for a
+// Thrift union object.
+const int& getActiveId(const void* object, const StructInfo& info) {
+  return *reinterpret_cast<const int*>(
+      static_cast<const char*>(object) + info.unionExt->unionTypeOffset);
 }
 
 const bool& fieldIsSet(const void* object, ptrdiff_t offset) {
   return *reinterpret_cast<const bool*>(
       offset + static_cast<const char*>(object));
@@ -589,20 +591,19 @@ void read(Protocol_* iprot, const StructInfo& structInfo, void* object) {
     if (readState.atStop()) {
       structInfo.unionExt->clear(object);
       readState.readStructEnd(iprot);
       return;
     }
-    const auto* fieldInfo = findFieldInfo(iprot, readState, structInfo);
-    // Found it.
-    if (fieldInfo) {
-      void* unionVal = getMember(*fieldInfo, object);
-      // Default construct and placement new into the member union.
-      structInfo.unionExt->initMember[fieldInfo - structInfo.fieldInfos](
-          unionVal);
-      read(iprot, *fieldInfo->typeInfo, readState, unionVal);
-      const_cast<FieldID&>(activeUnionMemberId(
-          object, structInfo.unionExt->unionTypeOffset)) = fieldInfo->id;
+    if (const auto* fieldInfo = findFieldInfo(iprot, readState, structInfo)) {
+      auto& activeId = const_cast<int&>(getActiveId(object, structInfo));
+      if (activeId != 0) {
+        structInfo.unionExt->clear(object);
+      }
+      void* value = getMember(*fieldInfo, object);
+      structInfo.unionExt->initMember[fieldInfo - structInfo.fieldInfos](value);
+      read(iprot, *fieldInfo->typeInfo, readState, value);
+      activeId = fieldInfo->id;
     } else {
       skip(iprot, readState);
     }
     readState.readFieldEnd(iprot);
     readState.readFieldBegin(iprot);
@@ -669,18 +670,17 @@ size_t
 write(Protocol_* iprot, const StructInfo& structInfo, const void* object) {
   DCHECK(object);
   size_t written = iprot->writeStructBegin(structInfo.name);
   if (UNLIKELY(structInfo.unionExt != nullptr)) {
     const FieldInfo* end = structInfo.fieldInfos + structInfo.numFields;
-    const auto& unionId =
-        activeUnionMemberId(object, structInfo.unionExt->unionTypeOffset);
+    const auto& activeId = getActiveId(object, structInfo);
     const FieldInfo* found = std::lower_bound(
         structInfo.fieldInfos,
         end,
-        unionId,
+        activeId,
         [](const FieldInfo& lhs, FieldID rhs) { return lhs.id < rhs; });
-    if (found < end && found->id == unionId) {
+    if (found < end && found->id == activeId) {
       const OptionalThriftValue value = getValue(*found->typeInfo, object);
       if (value.hasValue()) {
         written += writeField(iprot, *found, value.value());
       } else if (found->typeInfo->type == protocol::TType::T_STRUCT) {
         written += iprot->writeFieldBegin(
diff --git a/thrift/test/tablebased/SerializerTest.cpp b/thrift/test/tablebased/SerializerTest.cpp
index 7fdb0c21aa..9f146885cb 100644
--- a/thrift/test/tablebased/SerializerTest.cpp
+++ b/thrift/test/tablebased/SerializerTest.cpp
@@ -354,5 +354,27 @@ TEST(SerializerTest, UnionValueOffsetIsZero) {
   EXPECT_EQ(static_cast<void*>(&u), &*u.fieldA_ref());
 
   u.set_fieldB({});
   EXPECT_EQ(static_cast<void*>(&u), &*u.fieldB_ref());
 }
+
+TEST(SerializerTest, DuplicateUnionData) {
+  // Test that we can handle invalid serialized input with duplicate and
+  // incomplete union data.
+  const char data[] =
+      "\x0c" // type = TType::T_STRUCT
+      "\x00\x01" // fieldId = 1 (unionField)
+      "\x0b" // type = TType::T_STRING
+      "\x00\x01" // fieldId = 1 (stringField)
+      "\x00\x00\x00\x00" // size = 0
+      "\x00" // end of unionField
+
+      "\x0c" // type = TType::T_STRUCT
+      "\x00\x01" // fieldId = 1 (unionField)
+      "\x13" // type = TType::T_FLOAT
+      "\x00\x02"; // fieldId = 2 (floatField), value is missing
+
+  EXPECT_THROW(
+      BinarySerializer::deserialize<tablebased::TestStructWithUnion>(
+          folly::StringPiece(data, sizeof(data))),
+      std::out_of_range);
+}
diff --git a/thrift/test/tablebased/thrift_tablebased.thrift b/thrift/test/tablebased/thrift_tablebased.thrift
index 604f15adaf..16dc17b02e 100644
--- a/thrift/test/tablebased/thrift_tablebased.thrift
+++ b/thrift/test/tablebased/thrift_tablebased.thrift
@@ -77,5 +77,14 @@ union UnionWithRef {
   1: StructA fieldA (cpp2.ref_type = "unique");
   2: StructB fieldB;
   3: StructA fieldC (cpp2.ref_type = "shared_const");
   4: StructA fieldD (cpp2.ref_type = "shared");
 }
+
+union TestUnion {
+  1: string stringField;
+  2: float floatField;
+}
+
+struct TestStructWithUnion {
+  1: TestUnion unionField;
+}

Vulnerable code: const FieldID& activeUnionMemberId(const void* object, ptrdiff_t offset) {
  return *reinterpret_cast<const FieldID*>(
      offset + static_cast<const char*>(object));
    const auto* fieldInfo = findFieldInfo(iprot, readState, structInfo);
    // Found it.
    if (fieldInfo) {
      void* unionVal = getMember(*fieldInfo, object);
      // Default construct and placement new into the member union.
      structInfo.unionExt->initMember[fieldInfo - structInfo.fieldInfos](
          unionVal);
      read(iprot, *fieldInfo->typeInfo, readState, unionVal);
      const_cast<FieldID&>(activeUnionMemberId(
          object, structInfo.unionExt->unionTypeOffset)) = fieldInfo->id;
    const auto& unionId =
        activeUnionMemberId(object, structInfo.unionExt->unionTypeOffset);
        unionId,
    if (found < end && found->id == unionId) {
Fixed code: // Returns a reference to the data member that holds the active field id for a
// Thrift union object.
const int& getActiveId(const void* object, const StructInfo& info) {
  return *reinterpret_cast<const int*>(
      static_cast<const char*>(object) + info.unionExt->unionTypeOffset);
    if (const auto* fieldInfo = findFieldInfo(iprot, readState, structInfo)) {
      auto& activeId = const_cast<int&>(getActiveId(object, structInfo));
      if (activeId != 0) {
        structInfo.unionExt->clear(object);
      }
      void* value = getMember(*fieldInfo, object);
      structInfo.unionExt->initMember[fieldInfo - structInfo.fieldInfos](value);
      read(iprot, *fieldInfo->typeInfo, readState, value);
      activeId = fieldInfo->id;
    const auto& activeId = getActiveId(object, structInfo);
        activeId,
    if (found < end && found->id == activeId) {

TEST(SerializerTest, DuplicateUnionData) {
  // Test that we can handle invalid serialized input with duplicate and
  // incomplete union data.
  const char data[] =
      "\x0c" // type = TType::T_STRUCT
      "\x00\x01" // fieldId = 1 (unionField)
      "\x0b" // type = TType::T_STRING
      "\x00\x01" // fieldId = 1 (stringField)
      "\x00\x00\x00\x00" // size = 0
      "\x00" // end of unionField

      "\x0c" // type = TType::T_STRUCT
      "\x00\x01" // fieldId = 1 (unionField)
      "\x13" // type = TType::T_FLOAT
      "\x00\x02"; // fieldId = 2 (floatField), value is missing

  EXPECT_THROW(
      BinarySerializer::deserialize<tablebased::TestStructWithUnion>(
          folly::StringPiece(data, sizeof(data))),
      std::out_of_range);
}

union TestUnion {
  1: string stringField;
  2: float floatField;
}

struct TestStructWithUnion {
  1: TestUnion unionField;
}