diff --git a/src/brpc/couchbase.cpp b/src/brpc/couchbase.cpp index a707d75abe..7e622b1e65 100644 --- a/src/brpc/couchbase.cpp +++ b/src/brpc/couchbase.cpp @@ -30,6 +30,7 @@ static bool DBUG = false; // Set to true to enable debug logs } \ } while (0) +#include #include #include "brpc/policy/couchbase_protocol.h" @@ -1506,10 +1507,16 @@ bool CouchbaseOperations::CouchbaseResponse::popCollectionId( if (header.status != 0) { // handle error case - _buf.pop_front(sizeof(header) + header.extras_length + header.key_length); // Possibly read error message from value if present - size_t value_size = - header.total_body_length - header.extras_length - header.key_length; + const int64_t value_size = static_cast(header.total_body_length) - + static_cast(header.extras_length) - + static_cast(header.key_length); + if (value_size < 0) { + butil::string_printf(&_err, "value_size=%" PRId64 " is negative", + value_size); + return false; + } + _buf.pop_front(sizeof(header) + header.extras_length + header.key_length); if (value_size > 0) { std::string err_msg; _buf.cutn(&err_msg, value_size); @@ -1583,10 +1590,16 @@ bool CouchbaseOperations::CouchbaseResponse::popManifest( if (header.key_length != 0) { DEBUG_PRINT("Get Collections Manifest response must not have key"); } - _buf.pop_front(sizeof(header) + header.extras_length + header.key_length); // Possibly read error message from value if present - size_t value_size = - header.total_body_length - header.extras_length - header.key_length; + const int64_t value_size = static_cast(header.total_body_length) - + static_cast(header.extras_length) - + static_cast(header.key_length); + if (value_size < 0) { + butil::string_printf(&_err, "value_size=%" PRId64 " is negative", + value_size); + return false; + } + _buf.pop_front(sizeof(header) + header.extras_length + header.key_length); if (value_size > 0) { std::string err_msg; _buf.cutn(&err_msg, value_size); @@ -1599,8 +1612,14 @@ bool CouchbaseOperations::CouchbaseResponse::popManifest( } // Success case: the manifest should be in the value section - size_t value_size = - header.total_body_length - header.extras_length - header.key_length; + const int64_t value_size = static_cast(header.total_body_length) - + static_cast(header.extras_length) - + static_cast(header.key_length); + if (value_size < 0) { + butil::string_printf(&_err, "value_size=%" PRId64 " is negative", + value_size); + return false; + } if (value_size == 0) { butil::string_printf(&_err, "No manifest data in response"); _buf.pop_front(sizeof(header) + header.total_body_length); diff --git a/test/brpc_couchbase_unittest.cpp b/test/brpc_couchbase_unittest.cpp index e2f2fc0ad2..febf31c55c 100644 --- a/test/brpc_couchbase_unittest.cpp +++ b/test/brpc_couchbase_unittest.cpp @@ -98,6 +98,36 @@ TEST_F(CouchbaseUnitTest, ResultStruct) { EXPECT_EQ(0x00, result.status_code); } +// The header fields are attacker-controlled, extras_length + key_length may +// exceed total_body_length. popManifest/popCollectionId must not treat the +// underflowed remainder as a value size and drain the pipelined buffer. +TEST_F(CouchbaseUnitTest, CollectionResponseWithInconsistentBodyLength) { + const std::string next_response(64, 'A'); + + brpc::policy::CouchbaseResponseHeader header = {}; + header.magic = brpc::policy::CB_MAGIC_RESPONSE; + header.command = brpc::policy::CB_GET_COLLECTIONS_MANIFEST; + header.extras_length = 1; + header.total_body_length = 0; + + brpc::CouchbaseOperations::CouchbaseResponse res; + res.rawBuffer().append(&header, sizeof(header)); + res.rawBuffer().append(next_response); + std::string manifest = "untouched"; + EXPECT_FALSE(res.popManifest(&manifest)); + EXPECT_EQ("untouched", manifest); + EXPECT_EQ(sizeof(header) + next_response.size(), res.rawBuffer().size()); + + header.command = brpc::policy::CB_COLLECTIONS_GET_CID; + header.status = 1; + brpc::CouchbaseOperations::CouchbaseResponse res2; + res2.rawBuffer().append(&header, sizeof(header)); + res2.rawBuffer().append(next_response); + uint8_t collection_id = 0; + EXPECT_FALSE(res2.popCollectionId(&collection_id)); + EXPECT_EQ(sizeof(header) + next_response.size(), res2.rawBuffer().size()); +} + TEST_F(CouchbaseUnitTest, EdgeCases) { brpc::CouchbaseOperations::CouchbaseRequest req; req.addRequest("", "value", 0, 0, 0);