CVE-2021-24028 (C++, published 2021-04-14T00:15Z)
Description: 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.
Severity: HIGH (CVSS v3 base score: 9.8)
Weakness: CWE-763 \u2014 Release of Invalid Pointer or Reference
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.
Repository: https://github.com/facebook/fbthrift
Files changed:
thrift/lib/cpp2/protocol/TableBasedSerializer.cpp
thrift/test/tablebased/SerializerTest.cpp
thrift/test/tablebased/thrift_tablebased.thrift
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


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;
}