diff --git a/src/brpc/ubshm/ub_ring.cpp b/src/brpc/ubshm/ub_ring.cpp index cdc346edf4..ea34267332 100644 --- a/src/brpc/ubshm/ub_ring.cpp +++ b/src/brpc/ubshm/ub_ring.cpp @@ -424,6 +424,12 @@ int UBRing::UbrTrxRecvBlockMode(uint8_t *dest, uint32_t buf_len) } uint8_t chunk_msg_len = current_chunk->header[UBR_MSG_LEN_INDEX]; uint8_t cur_index = current_chunk->header[UBR_MSG_CUR_INDEX]; + if (UNLIKELY(!IsRecvChunkHeaderValid(chunk_msg_len, cur_index))) { + LOG(ERROR) << "Trx recv failed, invalid chunk header msg_len=" + << (uint32_t)chunk_msg_len << " cur_index=" << (uint32_t)cur_index; + errno = EBADMSG; + return UBRING_ERR; + } uint8_t available_data = chunk_msg_len - cur_index; int32_t copy_len = (remaining_len < available_data) ? remaining_len : available_data; @@ -1081,6 +1087,12 @@ ssize_t UBRing::StartReadv(UbrTrx *trx, const struct iovec *iov, int iovcnt, siz } uint8_t chunk_msg_len = current_chunk->header[UBR_MSG_LEN_INDEX]; uint8_t cur_index = current_chunk->header[UBR_MSG_CUR_INDEX]; + if (UNLIKELY(!IsRecvChunkHeaderValid(chunk_msg_len, cur_index))) { + LOG(ERROR) << "Trx readv failed, invalid chunk header msg_len=" + << (uint32_t)chunk_msg_len << " cur_index=" << (uint32_t)cur_index; + errno = EBADMSG; + return UBRING_ERR; + } uint8_t recv_len = remain_buf_len > (size_t)(chunk_msg_len - cur_index) ? (chunk_msg_len - cur_index) : (uint8_t)remain_buf_len; while (iov_index < iovcnt && recv_len > 0) { diff --git a/src/brpc/ubshm/ub_ring.h b/src/brpc/ubshm/ub_ring.h index 412c759885..0a02dd2fde 100644 --- a/src/brpc/ubshm/ub_ring.h +++ b/src/brpc/ubshm/ub_ring.h @@ -160,6 +160,21 @@ class UBRing : public butil::IReader { return UBRING_OK; } + // The message length and current offset of a received chunk are read out + // of the receive ring, which the remote peer writes. The peer is trusted + // to keep msg_len within the payload capacity and cur_index within msg_len + // (the send path clamps msg_len to UBR_MSG_PAYLOAD_LEN), but nothing + // enforces that on the receive side. When msg_len > UBR_MSG_PAYLOAD_LEN or + // cur_index > msg_len, reading `payload.inner + cur_index' for + // `msg_len - cur_index' bytes runs past the 60-byte payload (the uint8_t + // subtraction also wraps when cur_index > msg_len), so validate the header + // before using it. + static inline bool IsRecvChunkHeaderValid(uint8_t chunk_msg_len, + uint8_t cur_index) + { + return chunk_msg_len <= UBR_MSG_PAYLOAD_LEN && cur_index <= chunk_msg_len; + } + static inline void UpdateDataQTail(UbrTrx *trx) { ((UbrDataStatusQMsg *)trx->ubr_rx.remote_data_status_q.addr)->tail = trx->ubr_rx.read_pos; diff --git a/test/brpc_ubring_unittest.cpp b/test/brpc_ubring_unittest.cpp index 53bb0a9bd4..d3d0a7ce98 100644 --- a/test/brpc_ubring_unittest.cpp +++ b/test/brpc_ubring_unittest.cpp @@ -28,6 +28,8 @@ #include "brpc/ubshm/shm/shm_def.h" #include "brpc/ubshm/shm/shm_mgr.h" #include "brpc/ubshm/ub_ring_manager.h" +#include "brpc/ubshm/ub_ring.h" +#include "brpc/ubshm/ubr_msg.h" namespace brpc { namespace ubring { @@ -245,6 +247,46 @@ TEST_F(UBShmEndpointTest, reset_is_idempotent) { _ep->Reset(); } +// The receive paths (UbrTrxRecvBlockMode / StartReadv) read `msg_len' and +// `cur_index' out of a chunk header the remote peer writes into the ring, then +// copy `msg_len - cur_index' bytes from the 60-byte `payload.inner'. A peer +// that writes msg_len > 60, or cur_index > msg_len (which underflows the +// uint8_t subtraction), makes that copy over-read the payload into adjacent +// shared memory. IsRecvChunkHeaderValid is the guard both paths now apply. +TEST(UBRingRecvChunkHeaderTest, reject_out_of_range_len_and_index) { + using brpc::ubring::UBRing; + // Legitimate values a well-formed peer produces: full payload, partial + // consume, and the fully-consumed boundary. + EXPECT_TRUE(UBRing::IsRecvChunkHeaderValid(UBR_MSG_PAYLOAD_LEN, 0)); + EXPECT_TRUE(UBRing::IsRecvChunkHeaderValid(10, 5)); + EXPECT_TRUE(UBRing::IsRecvChunkHeaderValid(0, 0)); + EXPECT_TRUE(UBRing::IsRecvChunkHeaderValid(UBR_MSG_PAYLOAD_LEN, + UBR_MSG_PAYLOAD_LEN)); + // msg_len past the payload capacity -> over-read source. + EXPECT_FALSE(UBRing::IsRecvChunkHeaderValid(UBR_MSG_PAYLOAD_LEN + 1, 0)); + EXPECT_FALSE(UBRing::IsRecvChunkHeaderValid(255, 0)); + // cur_index past msg_len -> `msg_len - cur_index' underflows to a large + // uint8_t. + EXPECT_FALSE(UBRing::IsRecvChunkHeaderValid(0, 1)); + EXPECT_FALSE(UBRing::IsRecvChunkHeaderValid(10, 20)); +} + +// A crafted chunk laid out exactly like one in the ring: the guard rejects it +// so the recv loop never reaches the over-reading memcpy. +TEST(UBRingRecvChunkHeaderTest, crafted_chunk_is_rejected) { + brpc::ubring::UbrMsgFormat chunk; + memset(&chunk, 0xAB, sizeof(chunk)); + chunk.header[UBR_MSG_LEN_INDEX] = 255; // peer claims 255 bytes in a 60-byte payload + chunk.header[UBR_MSG_CUR_INDEX] = 0; + EXPECT_FALSE(brpc::ubring::UBRing::IsRecvChunkHeaderValid( + chunk.header[UBR_MSG_LEN_INDEX], chunk.header[UBR_MSG_CUR_INDEX])); + + chunk.header[UBR_MSG_LEN_INDEX] = UBR_MSG_PAYLOAD_LEN; + chunk.header[UBR_MSG_CUR_INDEX] = 0; + EXPECT_TRUE(brpc::ubring::UBRing::IsRecvChunkHeaderValid( + chunk.header[UBR_MSG_LEN_INDEX], chunk.header[UBR_MSG_CUR_INDEX])); +} + #else TEST(UbringDisabledTest, skip) {