From ed1f9770d2793a6caced3ba8eb7e532ae178a133 Mon Sep 17 00:00:00 2001 From: water <672684719@qq.com> Date: Wed, 12 Aug 2026 00:02:54 +0800 Subject: [PATCH 1/2] redis: fix uncaught exception in inline command hex escape parser --- .../redis__inline-hex-escape-crash.rst | 9 +++ .../network/common/redis/codec_impl.cc | 34 +++++++--- .../filters/network/common/redis/codec_impl.h | 7 ++ .../network/common/redis/codec_impl_test.cc | 64 +++++++++++++++++++ 4 files changed, 105 insertions(+), 9 deletions(-) create mode 100644 changelogs/current/bug_fixes/redis__inline-hex-escape-crash.rst diff --git a/changelogs/current/bug_fixes/redis__inline-hex-escape-crash.rst b/changelogs/current/bug_fixes/redis__inline-hex-escape-crash.rst new file mode 100644 index 0000000000000..0e756f120ecc5 --- /dev/null +++ b/changelogs/current/bug_fixes/redis__inline-hex-escape-crash.rst @@ -0,0 +1,9 @@ +Fixed a remote, unauthenticated denial-of-service in the Redis proxy filter: a single inline +command containing a literal ``x`` before a ``\xHH`` escape (e.g. ``SET key "x\x41"``) caused the +quoted-string decoder to mis-detect the escape marker and call ``std::stoul`` on a non-hex +string, throwing ``std::invalid_argument`` that escaped the filter and terminated the whole +Envoy process. The decoder no longer scans backwards for the escape marker; it now accumulates +the two hex digits of a ``\xHH`` escape in a dedicated buffer and converts them only once both +are present, matching Redis semantics. A truncated escape (one hex digit before the closing +quote) is emitted literally instead of crashing, and the out-of-bounds read when ``\x`` was the +first content of a quoted argument is eliminated. \ No newline at end of file diff --git a/source/extensions/filters/network/common/redis/codec_impl.cc b/source/extensions/filters/network/common/redis/codec_impl.cc index bd9695cc0fe37..b5870bbd7607e 100644 --- a/source/extensions/filters/network/common/redis/codec_impl.cc +++ b/source/extensions/filters/network/common/redis/codec_impl.cc @@ -704,7 +704,7 @@ void DecoderImpl::parseSlice(const Buffer::RawSlice& slice) { throw ProtocolError("unbalanced quotes in request"); } else if (buffer[0] == 'x') { state_ = State::InlineStringQuotedEscapeHex; - pending_value_stack_.front().value_->asString().push_back(buffer[0]); + inline_hex_digits_.clear(); } else { char c; switch (buffer[0]) { @@ -742,18 +742,34 @@ void DecoderImpl::parseSlice(const Buffer::RawSlice& slice) { case State::InlineStringQuotedEscapeHex: { ENVOY_LOG(trace, "parse slice: InlineStringQuotedEscapeHex: {}", buffer[0]); + auto& s = pending_value_stack_.front().value_->asString(); if (!std::isxdigit(buffer[0])) { + // A non-hex digit terminates a (possibly incomplete) \xHH escape. Redis + // treats the escape marker literally: emit ``x`` followed by any hex digits + // already seen, then reprocess the current byte in the quoted-string state. + s.push_back('x'); + s += inline_hex_digits_; + inline_hex_digits_.clear(); state_ = State::InlineStringQuoted; - break; + break; // do not consume buffer[0]; reprocess in InlineStringQuoted } - auto& s = pending_value_stack_.front().value_->asString(); - ASSERT((!s.empty() && s.back() == 'x') || (s.size() > 1 && s[s.size() - 2] == 'x')); - s.push_back(buffer[0]); - if (s[s.size() - 3] == 'x') { - char c = static_cast(std::stoul(&s[s.size() - 2], nullptr, 16)); - s.resize(s.size() - 3); - s.push_back(c); + inline_hex_digits_.push_back(buffer[0]); + if (inline_hex_digits_.size() == 2) { + // Convert the two hex digits to a single byte. + auto hex_val = [](char c) -> int { + if (c >= '0' && c <= '9') { + return c - '0'; + } + if (c >= 'a' && c <= 'f') { + return c - 'a' + 10; + } + return c - 'A' + 10; + }; + const char byte = static_cast((hex_val(inline_hex_digits_[0]) << 4) | + hex_val(inline_hex_digits_[1])); + s.push_back(byte); + inline_hex_digits_.clear(); state_ = State::InlineStringQuoted; } diff --git a/source/extensions/filters/network/common/redis/codec_impl.h b/source/extensions/filters/network/common/redis/codec_impl.h index 7e3f1f744c408..6bf740e740a59 100644 --- a/source/extensions/filters/network/common/redis/codec_impl.h +++ b/source/extensions/filters/network/common/redis/codec_impl.h @@ -122,6 +122,13 @@ class DecoderImpl : public Decoder, Logger::Loggable { // cap enforced as each byte is accumulated; the terminating CR validates numeric syntax and // moves the buffer into ``RespValue::asString()``. std::string pending_double_buf_; + // Scratch buffer for the one-or-two hex digits of a ``\xHH`` escape currently being + // decoded in a double-quoted inline string. The escape marker ``x`` is NOT appended + // to the value; digits accumulate here and are converted to a single byte only once + // the second hex digit arrives (matching Redis ``\xHH`` semantics). A non-hex digit + // or end-of-quote before the second digit flushes the literal ``x`` followed by the + // digits already seen. + std::string inline_hex_digits_; uint32_t consecutive_attributes_{0}; // counts toward kMaxConsecutiveAttributes // Number of attribute frames currently open on pending_value_stack_. Values completing while // this is non-zero belong to a frame that will itself be discarded, so they must not reset diff --git a/test/extensions/filters/network/common/redis/codec_impl_test.cc b/test/extensions/filters/network/common/redis/codec_impl_test.cc index 1307d2ecf3cb8..051534d7b9bfe 100644 --- a/test/extensions/filters/network/common/redis/codec_impl_test.cc +++ b/test/extensions/filters/network/common/redis/codec_impl_test.cc @@ -836,6 +836,70 @@ TEST_F(RedisEncoderDecoderImplTest, InlineCommandQuotedInvalidEscapes) { EXPECT_EQ(unescaped, decoded_values_[0]->asArray()[0]); } +// Verify that a literal 'x' before an \xHH escape does not cause a false-positive +// hex-digit completion check (issue #46642: the old code scanned 3 characters back +// for the escape marker, colliding with a user-supplied literal 'x'). +TEST_F(RedisEncoderDecoderImplTest, InlineCommandQuotedHexEscapeLiteralXBefore) { + // Input: "x\x41" → literal 'x', then \x41 = 'A' → "xA" + RespValue unescaped; + unescaped.type(RespType::BulkString); + unescaped.asString() = "x\x41"; // "x" + byte 0x41 = 'A' + + buffer_.add("\"x\\x41\"\r\n"); + decoder_.decode(buffer_); + EXPECT_EQ(1UL, decoded_values_.size()); + EXPECT_EQ(RespType::Array, decoded_values_[0]->type()); + EXPECT_EQ(1UL, decoded_values_[0]->asArray().size()); + EXPECT_EQ(unescaped, decoded_values_[0]->asArray()[0]); +} + +// Verify that a truncated \x escape (one hex digit, then end of quoted string) +// does not crash. Regression test for issue #46642. +TEST_F(RedisEncoderDecoderImplTest, InlineCommandQuotedHexEscapeTruncated) { + // Input: "x\x4" → three chars: 'x', 'x' (incomplete escape marker), '4' (only hex digit) + RespValue unescaped; + unescaped.type(RespType::BulkString); + unescaped.asString() = "xx4"; + + buffer_.add("\"x\\x4\"\r\n"); + decoder_.decode(buffer_); + EXPECT_EQ(1UL, decoded_values_.size()); + EXPECT_EQ(RespType::Array, decoded_values_[0]->type()); + EXPECT_EQ(1UL, decoded_values_[0]->asArray().size()); + EXPECT_EQ(unescaped, decoded_values_[0]->asArray()[0]); +} + +// Verify that an \x escape at the start of a quoted string (no preceding chars) +// does not cause an out-of-bounds read. Regression test for issue #46642. +TEST_F(RedisEncoderDecoderImplTest, InlineCommandQuotedHexEscapeAtStart) { + // Input: "\x4" → two chars: 'x' (incomplete escape marker), '4' (only hex digit) + RespValue unescaped; + unescaped.type(RespType::BulkString); + unescaped.asString() = "x4"; + + buffer_.add("\"\\x4\"\r\n"); + decoder_.decode(buffer_); + EXPECT_EQ(1UL, decoded_values_.size()); + EXPECT_EQ(RespType::Array, decoded_values_[0]->type()); + EXPECT_EQ(1UL, decoded_values_[0]->asArray().size()); + EXPECT_EQ(unescaped, decoded_values_[0]->asArray()[0]); +} + +// Verify that multiple consecutive \xHH escapes decode correctly. +TEST_F(RedisEncoderDecoderImplTest, InlineCommandQuotedHexEscapeMultiple) { + // Input: "\x41\x42\x43" → "ABC" + RespValue unescaped; + unescaped.type(RespType::BulkString); + unescaped.asString() = "ABC"; + + buffer_.add("\"\\x41\\x42\\x43\"\r\n"); + decoder_.decode(buffer_); + EXPECT_EQ(1UL, decoded_values_.size()); + EXPECT_EQ(RespType::Array, decoded_values_[0]->type()); + EXPECT_EQ(1UL, decoded_values_[0]->asArray().size()); + EXPECT_EQ(unescaped, decoded_values_[0]->asArray()[0]); +} + TEST_F(RedisEncoderDecoderImplTest, InlineCommandSingleQuotedCommand) { RespValue echo; echo.type(RespType::BulkString); From bd181982554592a0d69c02f8e09542e6cad525de Mon Sep 17 00:00:00 2001 From: water <672684719@qq.com> Date: Wed, 12 Aug 2026 00:06:26 +0800 Subject: [PATCH 2/2] redis: add defense-in-depth catch for unexpected decoder exceptions --- .../filters/network/common/redis/client_impl.cc | 7 +++++++ .../filters/network/redis_proxy/proxy_filter.cc | 15 +++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/source/extensions/filters/network/common/redis/client_impl.cc b/source/extensions/filters/network/common/redis/client_impl.cc index d4f8777bff468..bbd668c9f1614 100644 --- a/source/extensions/filters/network/common/redis/client_impl.cc +++ b/source/extensions/filters/network/common/redis/client_impl.cc @@ -311,6 +311,13 @@ void ClientImpl::onData(Buffer::Instance& data) { host_->cluster().trafficStats()->upstream_cx_protocol_error_.inc(); host_->stats().rq_error_.inc(); connection_->close(Network::ConnectionCloseType::NoFlush); + } catch (std::exception& e) { + ENVOY_LOG(warn, "redis client: unexpected exception while decoding upstream data: {}", + e.what()); + putOutlierEvent(Upstream::Outlier::Result::ExtOriginRequestFailed); + host_->cluster().trafficStats()->upstream_cx_protocol_error_.inc(); + host_->stats().rq_error_.inc(); + connection_->close(Network::ConnectionCloseType::NoFlush); } } diff --git a/source/extensions/filters/network/redis_proxy/proxy_filter.cc b/source/extensions/filters/network/redis_proxy/proxy_filter.cc index e48bc7c3def51..5b1678e0d0301 100644 --- a/source/extensions/filters/network/redis_proxy/proxy_filter.cc +++ b/source/extensions/filters/network/redis_proxy/proxy_filter.cc @@ -477,6 +477,21 @@ Network::FilterStatus ProxyFilter::onData(Buffer::Instance& data, bool) { callbacks_->connection().write(encoder_buffer_, false); callbacks_->connection().close(Network::ConnectionCloseType::NoFlush); return Network::FilterStatus::StopIteration; + } catch (std::exception& e) { + // Defense in depth: the decoder must never let an unexpected exception escape + // (previously a std::invalid_argument from the inline-command hex escape parser + // could terminate the whole process). Treat any other exception as a protocol + // error and close the connection. + ENVOY_LOG(warn, "redis proxy: unexpected exception while decoding downstream data: {}", + e.what()); + config_->stats_.downstream_cx_protocol_error_.inc(); + Common::Redis::RespValue error; + error.type(Common::Redis::RespType::Error); + error.asString() = "downstream protocol error"; + encoder_->encode(error, encoder_buffer_); + callbacks_->connection().write(encoder_buffer_, false); + callbacks_->connection().close(Network::ConnectionCloseType::NoFlush); + return Network::FilterStatus::StopIteration; } }