Skip to content
Permalink
Browse files Browse the repository at this point in the history
Better handling of truncated data when reading strings
Summary:
Currently we read string size and blindly pre-allocate it. This allows malicious attacker to send a few bytes message and cause server to allocate huge amount of memory (>1GB).

This diff changes the logic to check if we have enough data in the buffer before allocating the string.

This is a second part of a fix for CVE-2019-3553.

Reviewed By: vitaut

Differential Revision: D14393393

fbshipit-source-id: e2046d2f5b087d3abc9a9d2c6c107cf088673057
  • Loading branch information
spalamarchuk authored and facebook-github-bot committed Jan 28, 2020
1 parent 075748f commit c9a903e
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 0 deletions.
3 changes: 3 additions & 0 deletions thrift/lib/cpp2/protocol/BinaryProtocol-inl.h
Expand Up @@ -553,6 +553,9 @@ void BinaryProtocolReader::readStringBody(StrType& str, int32_t size) {
}

if (static_cast<int32_t>(in_.length()) < size) {
if (!in_.canAdvance(size)) {
protocol::TProtocolException::throwTruncatedData();
}
str.reserve(size); // only reserve for multi iter case below
}
str.clear();
Expand Down
3 changes: 3 additions & 0 deletions thrift/lib/cpp2/protocol/CompactProtocol-inl.h
Expand Up @@ -672,6 +672,9 @@ void CompactProtocolReader::readStringSize(int32_t& size) {
template <typename StrType>
void CompactProtocolReader::readStringBody(StrType& str, int32_t size) {
if (static_cast<int32_t>(in_.length()) < size) {
if (!in_.canAdvance(size)) {
protocol::TProtocolException::throwTruncatedData();
}
str.reserve(size); // only reserve for multi iter case below
}
str.clear();
Expand Down
1 change: 1 addition & 0 deletions thrift/test/ProtocolTruncatedData.thrift
Expand Up @@ -20,4 +20,5 @@ struct TestStruct {
1: optional list<i64> i64_list,
2: optional set<i32> i32_set,
3: optional map<i32,i16> i32_i16_map,
4: optional string a_string,
}
16 changes: 16 additions & 0 deletions thrift/test/ProtocolTruncatedDataTest.cpp
Expand Up @@ -77,3 +77,19 @@ TEST(ProtocolTruncatedDataTest, TruncatedMap) {
testPartialDataHandling<CompactSerializer>(
s, 3 /* headers */ + 30 * 2 /* 2b / kv pair */);
}

TEST(ProtocolTruncatedDataTest, TuncatedString_Compact) {
TestStruct s;
s.a_string_ref() = "foobarbazstring";

testPartialDataHandling<CompactSerializer>(
s, 2 /* field & length header */ + s.a_string_ref()->size());
}

TEST(ProtocolTruncatedDataTest, TuncatedString_Binary) {
TestStruct s;
s.a_string_ref() = "foobarbazstring";

testPartialDataHandling<BinarySerializer>(
s, 7 /* field & length header */ + s.a_string_ref()->size());
}

0 comments on commit c9a903e

Please sign in to comment.