Skip to content

guard underflowed value size in couchbase collection responses #3402

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ubeddulla wants to merge 2 commits into
base: master
Choose a base branch
from
+57 −8
Open

guard underflowed value size in couchbase collection responses #3402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df0e14b
guard underflowed value size in couchbase collection responses
ubeddulla Jul 22, 2026
02b864d
Compute couchbase value_size as int64_t to avoid int overflow
ubeddulla Jul 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 27 additions & 8 deletions src/brpc/couchbase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ static bool DBUG = false; // Set to true to enable debug logs
} \
} while (0)

#include <cinttypes>
#include <iostream>

#include "brpc/policy/couchbase_protocol.h"
Expand Down Expand Up @@ -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<int64_t>(header.total_body_length) -
static_cast<int64_t>(header.extras_length) -
static_cast<int64_t>(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);
Expand Down Expand Up @@ -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<int64_t>(header.total_body_length) -
static_cast<int64_t>(header.extras_length) -
static_cast<int64_t>(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);
Expand All @@ -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<int64_t>(header.total_body_length) -
static_cast<int64_t>(header.extras_length) -
static_cast<int64_t>(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);
Expand Down
30 changes: 30 additions & 0 deletions test/brpc_couchbase_unittest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Comment on lines +107 to +111

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());
Comment on lines +126 to +128
}

TEST_F(CouchbaseUnitTest, EdgeCases) {
brpc::CouchbaseOperations::CouchbaseRequest req;
req.addRequest("", "value", 0, 0, 0);
Expand Down
Loading