From 0071e2a974493c84d46631ed3a91c88d28c77dbc Mon Sep 17 00:00:00 2001 From: Kartik Kenchi Date: Thu, 10 Sep 2026 01:04:22 +0530 Subject: [PATCH] fix redis: type-check pub/sub reply elements before typed access --- redis/src/storages/redis/impl/sentinel.cpp | 19 ++-- .../src/storages/redis/impl/sentinel_test.cpp | 94 +++++++++++++++++++ 2 files changed, 107 insertions(+), 6 deletions(-) diff --git a/redis/src/storages/redis/impl/sentinel.cpp b/redis/src/storages/redis/impl/sentinel.cpp index 8474f29b4c61..8e32ef2ffe9e 100644 --- a/redis/src/storages/redis/impl/sentinel.cpp +++ b/redis/src/storages/redis/impl/sentinel.cpp @@ -71,11 +71,17 @@ void OnSubscribeImpl( return; } if (!strcasecmp(reply_array[0].GetString().c_str(), subscribe_type.data())) { - subscribe_callback(reply->server_id, reply_array[1].GetString(), reply_array[2].GetInt()); + if (reply_array[1].IsString() && reply_array[2].IsInt()) { + subscribe_callback(reply->server_id, reply_array[1].GetString(), reply_array[2].GetInt()); + } } else if (!strcasecmp(reply_array[0].GetString().c_str(), unsubscribe_type.data())) { - unsubscribe_callback(reply->server_id, reply_array[1].GetString(), reply_array[2].GetInt()); + if (reply_array[1].IsString() && reply_array[2].IsInt()) { + unsubscribe_callback(reply->server_id, reply_array[1].GetString(), reply_array[2].GetInt()); + } } else if (!strcasecmp(reply_array[0].GetString().c_str(), message_type.data())) { - message_callback(reply->server_id, reply_array[1].GetString(), reply_array[2].GetString()); + if (reply_array[1].IsString() && reply_array[2].IsString()) { + message_callback(reply->server_id, reply_array[1].GetString(), reply_array[2].GetString()); + } } } @@ -359,15 +365,16 @@ void Sentinel::OnPsubscribeReply( return; } if (!strcasecmp(reply_array[0].GetString().c_str(), "PSUBSCRIBE")) { - if (reply_array.size() == 3) { + if (reply_array.size() == 3 && reply_array[1].IsString() && reply_array[2].IsInt()) { subscribe_callback(reply->server_id, reply_array[1].GetString(), reply_array[2].GetInt()); } } else if (!strcasecmp(reply_array[0].GetString().c_str(), "PUNSUBSCRIBE")) { - if (reply_array.size() == 3) { + if (reply_array.size() == 3 && reply_array[1].IsString() && reply_array[2].IsInt()) { unsubscribe_callback(reply->server_id, reply_array[1].GetString(), reply_array[2].GetInt()); } } else if (!strcasecmp(reply_array[0].GetString().c_str(), "PMESSAGE")) { - if (reply_array.size() == 4) { + if (reply_array.size() == 4 && reply_array[1].IsString() && reply_array[2].IsString() && + reply_array[3].IsString()) { pmessage_callback( reply->server_id, reply_array[1].GetString(), diff --git a/redis/src/storages/redis/impl/sentinel_test.cpp b/redis/src/storages/redis/impl/sentinel_test.cpp index a464719b0167..d86963f55a8c 100644 --- a/redis/src/storages/redis/impl/sentinel_test.cpp +++ b/redis/src/storages/redis/impl/sentinel_test.cpp @@ -85,4 +85,98 @@ TEST(Sentinel, OnPsubscribeReplyTooShortArray) { } } +namespace { + +// A malicious/compromised server may answer with a well-formed array of the +// expected length whose elements have unexpected types. ReplyData::GetString()/ +// GetInt() only UASSERT the type (a no-op in release) and then dereference the +// pointer returned by std::get_if, which is null on a type mismatch. So each of +// these replies must be ignored, not passed to a typed accessor. +[[nodiscard]] storages::redis::ReplyData::Array MakeReplyArray(std::vector parts) { + storages::redis::ReplyData::Array array; + for (auto& part : parts) { + array.push_back(std::move(part)); + } + return array; +} + +} // namespace + +TEST(Sentinel, OnSubscribeReplyWrongElementTypes) { + using storages::redis::Reply; + using storages::redis::ReplyData; + using storages::redis::impl::Sentinel; + + const auto fail_message = [](storages::redis::ServerId, const std::string&, const std::string&) { + FAIL() << "message callback must not fire on a wrong-typed reply element"; + }; + const auto fail_subscribe = [](storages::redis::ServerId, const std::string&, size_t) { + FAIL() << "subscribe callback must not fire on a wrong-typed reply element"; + }; + const auto fail_unsubscribe = [](storages::redis::ServerId, const std::string&, size_t) { + FAIL() << "unsubscribe callback must not fire on a wrong-typed reply element"; + }; + + std::vector replies; + // channel is expected to be a string, count an integer + replies.push_back(MakeReplyArray({std::string{"SUBSCRIBE"}, ReplyData{42}, ReplyData{1}})); + replies.push_back(MakeReplyArray({std::string{"SUBSCRIBE"}, std::string{"news"}, std::string{"1"}})); + replies.push_back(MakeReplyArray({std::string{"UNSUBSCRIBE"}, ReplyData{42}, ReplyData{0}})); + replies.push_back(MakeReplyArray({std::string{"UNSUBSCRIBE"}, std::string{"news"}, std::string{"0"}})); + // channel and message are both expected to be strings + replies.push_back(MakeReplyArray({std::string{"MESSAGE"}, ReplyData{42}, std::string{"payload"}})); + replies.push_back(MakeReplyArray({std::string{"MESSAGE"}, std::string{"news"}, ReplyData{42}})); + + for (auto& array : replies) { + auto reply = std::make_shared("SUBSCRIBE", ReplyData{std::move(array)}); + Sentinel::OnSubscribeReply(fail_message, fail_subscribe, fail_unsubscribe, reply); + } + + // The sharded pub/sub variant shares the same dispatch (different opcodes). + std::vector sharded_replies; + sharded_replies.push_back(MakeReplyArray({std::string{"SSUBSCRIBE"}, ReplyData{42}, ReplyData{1}})); + sharded_replies.push_back(MakeReplyArray({std::string{"SUNSUBSCRIBE"}, ReplyData{42}, ReplyData{0}})); + sharded_replies.push_back(MakeReplyArray({std::string{"SMESSAGE"}, ReplyData{42}, std::string{"payload"}})); + sharded_replies.push_back(MakeReplyArray({std::string{"SMESSAGE"}, std::string{"news"}, ReplyData{42}})); + for (auto& array : sharded_replies) { + auto reply = std::make_shared("SSUBSCRIBE", ReplyData{std::move(array)}); + Sentinel::OnSsubscribeReply(fail_message, fail_subscribe, fail_unsubscribe, reply); + } +} + +TEST(Sentinel, OnPsubscribeReplyWrongElementTypes) { + using storages::redis::Reply; + using storages::redis::ReplyData; + using storages::redis::impl::Sentinel; + + const auto fail_pmessage = + [](storages::redis::ServerId, const std::string&, const std::string&, const std::string&) { + FAIL() << "pmessage callback must not fire on a wrong-typed reply element"; + }; + const auto fail_subscribe = [](storages::redis::ServerId, const std::string&, size_t) { + FAIL() << "subscribe callback must not fire on a wrong-typed reply element"; + }; + const auto fail_unsubscribe = [](storages::redis::ServerId, const std::string&, size_t) { + FAIL() << "unsubscribe callback must not fire on a wrong-typed reply element"; + }; + + std::vector replies; + replies.push_back(MakeReplyArray({std::string{"PSUBSCRIBE"}, ReplyData{42}, ReplyData{1}})); + replies.push_back(MakeReplyArray({std::string{"PSUBSCRIBE"}, std::string{"news.*"}, std::string{"1"}})); + replies.push_back(MakeReplyArray({std::string{"PUNSUBSCRIBE"}, ReplyData{42}, ReplyData{0}})); + replies.push_back(MakeReplyArray({std::string{"PUNSUBSCRIBE"}, std::string{"news.*"}, std::string{"0"}})); + // pattern, channel and message are all expected to be strings + const std::string pat{"news.*"}; + const std::string chan{"news"}; + const std::string msg{"m"}; + replies.push_back(MakeReplyArray({std::string{"PMESSAGE"}, ReplyData{42}, chan, msg})); + replies.push_back(MakeReplyArray({std::string{"PMESSAGE"}, pat, ReplyData{42}, msg})); + replies.push_back(MakeReplyArray({std::string{"PMESSAGE"}, pat, chan, ReplyData{42}})); + + for (auto& array : replies) { + auto reply = std::make_shared("PSUBSCRIBE", ReplyData{std::move(array)}); + Sentinel::OnPsubscribeReply(fail_pmessage, fail_subscribe, fail_unsubscribe, reply); + } +} + USERVER_NAMESPACE_END