Denial of Service (protocol desynchronization / client hang)
Description
The commit adds a guard in smembersCommand for set replies to detect a mismatch between the declared length (via addReplySetLen) and the actual number of elements sent. Previously, if a corrupted or mismatched length occurred (e.g., due to a corrupted dump, a bug in set iteration, or protocol desynchronization), the server could desynchronize the Redis protocol and cause the client to hang while waiting for more elements. The fix introduces a length counter initialized to the actual set size, decrements it as elements are sent, and asserts that the remaining length is zero after finishing. This prevents proceeding with a broken reply and turns a potential hang into a detectable failure (assert), mitigating a Denial-of-Service vector caused by protocol misbehavior. The change is accompanied by a test illustrating a corrupted-dump scenario that previously could hang the smembers response.
Impact: DoS via protocol desynchronization leading to client hang during SMEMBERS when dealing with a set reply whose length did not match the actual element count. The fix ensures mismatches are caught, avoiding a hang at the cost of a crash/assert in corrupted scenarios when built with assertions.
Proof of Concept
PoC (conceptual, executable steps):
1) Environment: Run Redis in a controlled test environment using a vulnerable 8.6.x release (e.g., 8.6.1).
2) Craft a corrupted RDB/RESTORE payload that results in a set key where the protocol length advertised for SMEMBERS does not match the actual number of elements produced when encoding the reply. This typically requires fuzzer-generated or manually crafted payload bytes that affect the internal set encoding and the length that addReplySetLen will emit.
3) Load the corrupted payload into Redis using RESTORE (or load via a fuzzed dump as in the tests) to create a key such as _set with a corrupted internal state.
4) Connect a client and issue SMEMBERS _set.
5) Observe behavior on vulnerable versions: the client may hang due to protocol desynchronization caused by a mismatched length in the crafted reply.
6) Observe behavior on patched versions (8.6.2+): the server detects the mismatch (via the length tracking and final assertion) and may crash/assert rather than leave the client waiting, preventing a longer protocol hang.
Notes:
- The exact corrupted payload bytes are highly dependent on Redis internals (RDB/RESP encoding paths). The repository’s integration test provides an example approach via a crafted restore payload that induces a mismatch and tests for a hanging smembers. Use an isolated test environment and ensure assertions are enabled to observe the crash behavior in patched builds.
- In normal operation without corrupted payloads, no behavior change is expected.
Commit Details
Author: debing.sun
Date: 2024-09-04 09:35 UTC
Message:
Fix set with invalid length causes smembers to hang (#13515)
After https://github.com/redis/redis/pull/13499, If the length set by
`addReplySetLen()` does not match the actual number of elements in the
reply, it will cause protocol broken and result in the client hanging.
Triage Assessment
Vulnerability Type: Denial of Service (hang / protocol handling)
Confidence: MEDIUM
Reasoning:
The change guards against a corrupted or mismatched length in the SET reply which could cause the smembers command to hang, effectively enabling a denial-of-service via protocol misuse. The added length tracking and assertion ensure the response length matches the actual elements, preventing protocol desynchronization and potential DoS scenarios.
Verification Assessment
Vulnerability Type: Denial of Service (protocol desynchronization / client hang)
Confidence: MEDIUM
Affected Versions: 8.6.0, 8.6.1 (any 8.6.x prior to the 8.6.2 patch); fixes applied in 8.6.2
Code Diff
diff --git a/src/t_set.c b/src/t_set.c
index 1aefda0d593..c2bb120a80a 100644
--- a/src/t_set.c
+++ b/src/t_set.c
@@ -1434,17 +1434,20 @@ void smembersCommand(client *c) {
}
/* Prepare the response. */
- addReplySetLen(c,setTypeSize(setobj));
-
+ unsigned long length = setTypeSize(setobj);
+ addReplySetLen(c,length);
/* Iterate through the elements of the set. */
si = setTypeInitIterator(setobj);
+
while (setTypeNext(si, &str, &len, &intobj) != -1) {
if (str != NULL)
addReplyBulkCBuffer(c, str, len);
else
addReplyBulkLongLong(c, intobj);
+ length--;
}
setTypeReleaseIterator(si);
+ serverAssert(length == 0); /* fail on corrupt data */
}
/* SINTERCARD numkeys key [key ...] [LIMIT limit] */
diff --git a/tests/integration/corrupt-dump.tcl b/tests/integration/corrupt-dump.tcl
index 3e644cbb968..273ab4fb946 100644
--- a/tests/integration/corrupt-dump.tcl
+++ b/tests/integration/corrupt-dump.tcl
@@ -885,5 +885,17 @@ test {corrupt payload: fuzzer findings - set with duplicate elements causes sdif
}
} {} {logreqres:skip} ;# This test violates {"uniqueItems": true}
+test {corrupt payload: fuzzer findings - set with invalid length causes smembers to hang} {
+ start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] {
+ # In the past, it generated a broken protocol and left the client hung in smembers
+ r config set sanitize-dump-payload no
+ assert_equal {OK} [r restore _set 0 "\x14\x16\x16\x00\x00\x00\x0c\x00\x81\x61\x02\x81\x62\x02\x81\x63\x02\x01\x01\x02\x01\x03\x01\xff\x0c\x00\x91\x00\x56\x73\xc1\x82\xd5\xbd" replace]
+ assert_encoding listpack _set
+ catch { r SMEMBERS _set } err
+ assert_equal [count_log_message 0 "crashed by signal"] 0
+ assert_equal [count_log_message 0 "ASSERTION FAILED"] 1
+ }
+}
+
} ;# tags