patches/CVE-2019-3552-thrift.patch

   1 From 5a86b29f56048f635abb443ccc9f1a3dc61b4a9c Mon Sep 17 00:00:00 2001
   2 From: Stepan Palamarchuk <stepan@fb.com>
   3 Date: Tue, 22 Jan 2019 09:54:52 -0800
   4 Subject: [PATCH] Throw on bad types during skipping data
   5
   6 Summary:
   7 The current code silently returns on bad types. In case when we have an invalid data, we may get a container of a large size with a bad type, this would lead to us running long loop doing nothing (though we already can say that the data is invalid).
   8
   9 The new code would throw an exception as soon as we try to skip a value of invalid type.
  10
  11 Fixes CVE-2019-3552
  12
  13 Reviewed By: yfeldblum, stevegury
  14
  15 Differential Revision: D8344920
  16 ---
  17  thrift/lib/cpp/protocol/TProtocolException.cpp |  8 +++
  18  thrift/lib/cpp/protocol/TProtocolException.h   |  2 +
  19  thrift/lib/cpp2/protocol/Protocol.h            |  5 +-
  20  thrift/lib/py/protocol/TProtocol.py            |  5 +-
  21  thrift/test/ProtocolSkipTest.cpp               | 68 ++++++++++++++++++++++++++
  22  5 files changed, 85 insertions(+), 3 deletions(-)
  23  create mode 100644 thrift/test/ProtocolSkipTest.cpp
  24
  25 diff --git a/third-party/thrift/src/thrift/lib/cpp/protocol/TProtocolException.cpp b/third-party/thrift/src/thrift/lib/cpp/protocol/TProtocolException.cpp
  26 index d0ad89450..ab84de2a7 100644
  27 --- a/third-party/thrift/src/thrift/lib/cpp/protocol/TProtocolException.cpp
  28 +++ b/third-party/thrift/src/thrift/lib/cpp/protocol/TProtocolException.cpp
  29 @@ -57,4 +57,12 @@ namespace apache { namespace thrift { namespace protocol {
  30            "Attempt to interpret value {} as bool, probably the data is corrupted",
  31            value));
  32  }
  33 +
  34 +[[noreturn]] void TProtocolException::throwInvalidSkipType(TType type) {
  35 +  throw TProtocolException(
  36 +      TProtocolException::INVALID_DATA,
  37 +      folly::sformat(
  38 +          "Encountered invalid field/element type ({}) during skipping",
  39 +          static_cast<uint8_t>(type)));
  40 +}
  41  }}}
  42 diff --git a/third-party/thrift/src/thrift/lib/cpp/protocol/TProtocolException.h b/third-party/thrift/src/thrift/lib/cpp/protocol/TProtocolException.h
  43 index 874654fa4..193d25d78 100644
  44 --- a/third-party/thrift/src/thrift/lib/cpp/protocol/TProtocolException.h
  45 +++ b/third-party/thrift/src/thrift/lib/cpp/protocol/TProtocolException.h
  46 @@ -23,6 +23,7 @@
  47  #define _THRIFT_PROTOCOL_TPROTOCOLEXCEPTION_H_ 1
  48
  49  #include <thrift/lib/cpp/Thrift.h>
  50 +#include <thrift/lib/cpp/protocol/TType.h>
  51
  52  #include <string>
  53
  54 @@ -106,6 +107,7 @@ class TProtocolException : public apache::thrift::TLibraryException {
  55        folly::StringPiece field,
  56        folly::StringPiece type);
  57    [[noreturn]] static void throwBoolValueOutOfRange(uint8_t value);
  58 +  [[noreturn]] static void throwInvalidSkipType(TType type);
  59
  60   protected:
  61    /**
  62 diff --git a/third-party/thrift/src/thrift/lib/cpp2/protocol/Protocol.h b/third-party/thrift/src/thrift/lib/cpp2/protocol/Protocol.h
  63 index d1cc63ed6..033668ac7 100644
  64 --- a/third-party/thrift/src/thrift/lib/cpp2/protocol/Protocol.h
  65 +++ b/third-party/thrift/src/thrift/lib/cpp2/protocol/Protocol.h
  66 @@ -176,8 +176,9 @@ void skip(Protocol_& prot, TType arg_type) {
  67        prot.readListEnd();
  68        return;
  69      }
  70 -    default:
  71 -      return;
  72 +    default: {
  73 +      TProtocolException::throwInvalidSkipType(arg_type);
  74 +    }
  75    }
  76  }
  77
  78 diff --git a/third-party/thrift/src/thrift/lib/py/protocol/TProtocol.py b/third-party/thrift/src/thrift/lib/py/protocol/TProtocol.py
  79 index a229eb025..f7f55bbe2 100644
  80 --- a/third-party/thrift/src/thrift/lib/py/protocol/TProtocol.py
  81 +++ b/third-party/thrift/src/thrift/lib/py/protocol/TProtocol.py
  82 @@ -178,7 +178,10 @@ class TProtocolBase:
  83
  84      def skip(self, type):
  85          if type == TType.STOP:
  86 -            return
  87 +            raise TProtocolException(
  88 +                TProtocolException.INVALID_DATA,
  89 +                "Unexpected type for skipping T_STOP"
  90 +            )
  91          elif type == TType.BOOL:
  92              self.readBool()
  93          elif type == TType.BYTE:
  94 diff --git a/third-party/thrift/src/thrift/test/ProtocolSkipTest.cpp b/third-party/thrift/src/thrift/test/ProtocolSkipTest.cpp
  95 new file mode 100644
  96 index 000000000..762ec64a8
  97 --- /dev/null
  98 +++ b/third-party/thrift/src/thrift/test/ProtocolSkipTest.cpp
  99 @@ -0,0 +1,68 @@
 100 +/*
 101 + * Copyright 2004-present Facebook, Inc.
 102 + *
 103 + * Licensed under the Apache License, Version 2.0 (the "License");
 104 + * you may not use this file except in compliance with the License.
 105 + * You may obtain a copy of the License at
 106 + *
 107 + *   http://www.apache.org/licenses/LICENSE-2.0
 108 + *
 109 + * Unless required by applicable law or agreed to in writing, software
 110 + * distributed under the License is distributed on an "AS IS" BASIS,
 111 + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 112 + * See the License for the specific language governing permissions and
 113 + * limitations under the License.
 114 + */
 115 +
 116 +#include <gtest/gtest.h>
 117 +
 118 +#include <thrift/lib/cpp2/protocol/CompactProtocol.h>
 119 +
 120 +using namespace apache::thrift;
 121 +
 122 +TEST(ProtocolSkipTest, SkipInt) {
 123 +  IOBufQueue queue;
 124 +  CompactProtocolWriter writer;
 125 +  writer.setOutput(&queue);
 126 +  writer.writeI32(123);
 127 +  auto buf = queue.move();
 128 +  CompactProtocolReader reader;
 129 +  reader.setInput(buf.get());
 130 +  reader.skip(TType::T_I32);
 131 +}
 132 +
 133 +TEST(ProtocolSkipTest, SkipStop) {
 134 +  IOBufQueue queue;
 135 +  CompactProtocolWriter writer;
 136 +  writer.setOutput(&queue);
 137 +  writer.writeFieldStop();
 138 +  auto buf = queue.move();
 139 +  CompactProtocolReader reader;
 140 +  reader.setInput(buf.get());
 141 +  bool thrown = false;
 142 +  try {
 143 +    reader.skip(TType::T_STOP);
 144 +  } catch (const TProtocolException& ex) {
 145 +    EXPECT_EQ(TProtocolException::INVALID_DATA, ex.getType());
 146 +    thrown = true;
 147 +  }
 148 +  EXPECT_TRUE(thrown);
 149 +}
 150 +
 151 +TEST(ProtocolSkipTest, SkipStopInContainer) {
 152 +  IOBufQueue queue;
 153 +  CompactProtocolWriter writer;
 154 +  writer.setOutput(&queue);
 155 +  writer.writeListBegin(TType::T_STOP, 1u << 30);
 156 +  auto buf = queue.move();
 157 +  CompactProtocolReader reader;
 158 +  reader.setInput(buf.get());
 159 +  bool thrown = false;
 160 +  try {
 161 +    reader.skip(TType::T_LIST);
 162 +  } catch (const TProtocolException& ex) {
 163 +    EXPECT_EQ(TProtocolException::INVALID_DATA, ex.getType());
 164 +    thrown = true;
 165 +  }
 166 +  EXPECT_TRUE(thrown);
 167 +}
 168 --
 169 2.13.5
 170