jinchengyang98 opened a new issue, #3493:
URL: https://github.com/apache/kvrocks/issues/3493
# [Security] RESP Protocol Injection via Lua `redis.error_reply()` in kvrocks
## Summary
kvrocks converts `redis.error_reply()` results from Lua to Redis protocol
without sanitizing embedded `\r\n` sequences. This allows an authenticated user
to inject arbitrary RESP messages into the response stream via crafted error
strings, potentially corrupting subsequent command responses in a shared
connection pool.
## Vulnerability Details
- **Affected code**: `src/storage/scripting.cc` — `ReplyToRedisReply()`
(lines 1293–1300)
- **Vulnerability type**: RESP Protocol Injection (CWE-116 — Improper
Encoding or Escaping of Output)
- **Related CVE**: CVE-2025-67733 (same class in Valkey/Redis)
## Root Cause
In `ReplyToRedisReply()`, when Lua returns a table with an `err` field
(i.e., from `redis.error_reply()`), the error string is passed directly to
`redis::Error()` without sanitizing embedded `\r\n`:
```cpp
// src/storage/scripting.cc line 1297-1300
if (t == LUA_TSTRING) {
output = redis::Error({Status::RedisErrorNoPrefix, lua_tostring(lua, -1)});
lua_pop(lua, 1);
return output;
}
```
`redis::Error()` constructs a RESP error reply by concatenation:
```cpp
// src/server/redis_reply.cc line 49
std::string Error(const Status &s) { return RESP_PREFIX_ERROR +
StatusToRedisErrorMsg(s) + CRLF; }
```
With `Status::RedisErrorNoPrefix`, `StatusToRedisErrorMsg()` returns the raw
message string. If the message contains `\r\n`, the RESP output includes
embedded delimiters, splitting the response into multiple messages:
**Attack input:**
```lua
return redis.error_reply("ERR injected\r\n+INJECTED_SIMPLE_STRING\r\n")
```
**Wire output:**
```
-ERR injected\r\n+INJECTED_SIMPLE_STRING\r\n\r\n
```
A RESP client parses this as:
1. `-ERR injected` → error reply
2. `+INJECTED_SIMPLE_STRING` → simple string reply (injected)
3. `\r\n` → (spurious)
## Attack Scenario
1. An authenticated client with access to `EVAL` or `FCALL` sends:
```
EVAL "return redis.error_reply('ERR x\r\n+PONG\r\n')" 0
```
2. The server responds with:
```
-ERR x
+PONG
```
3. If the connection is part of a connection pool, the injected `+PONG` is
consumed as the response to the next pooled command (e.g., if a pool `PING`
follows), causing response desynchronization.
## Affected Code
```cpp
// src/storage/scripting.cc (kvrocks HEAD 9c411259, 2026-05-17)
// Line 1297-1300: error_reply field handling
if (t == LUA_TSTRING) {
output = redis::Error({Status::RedisErrorNoPrefix, lua_tostring(lua,
-1)}); // <-- unsanitized
lua_pop(lua, 1);
return output;
}
```
## Suggested Fix
Strip `\r\n` from the error string before constructing the RESP error reply:
```cpp
if (t == LUA_TSTRING) {
std::string err_msg = lua_tostring(lua, -1);
// Strip embedded CRLF to prevent RESP protocol injection
err_msg.erase(std::remove_if(err_msg.begin(), err_msg.end(),
[](char c){ return c == '\r' || c == '\n'; }),
err_msg.end());
output = redis::Error({Status::RedisErrorNoPrefix, err_msg});
lua_pop(lua, 1);
return output;
}
```
Or add sanitization in `redis::Error()` / `StatusToRedisErrorMsg()` for the
`RedisErrorNoPrefix` case.
## References
- Similar fix in Valkey:
[CVE-2025-67733](https://github.com/valkey-io/valkey/security/advisories/GHSA-p876-p7q5-hv2m)
— replaced `addReplyErrorSdsEx` with `addReplyErrorSdsExSafe` which strips
control characters
- Related: [RESP protocol specification](https://redis.io/topics/protocol)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
