Input validation / robustness
Description
The commit tightens input validation for hash field operations (HPERSIST, HPEXPIRE, HEXPIRE, etc.) by requiring that the provided numFields exactly match the number of field tokens given after the FIELDS keyword. Previously the code allowed a mismatch where numFields could be greater than the actual number of arguments, which could lead to inconsistent command handling or misinterpretation of arguments. The fix changes the check from a '>' comparison to an exact '==' and updates error messaging accordingly. This reduces the risk of malformed inputs being processed in a security-relevant manner and strengthens robustness against certain input validation edge cases.
Commit Details
Author: Moti Cohen
Date: 2024-06-26 11:12 UTC
Message:
HFE - count in command must match actual number of fields (#13369)
There was wrong preliminary assumption that we can optionally provide
vector of arguments more than count.
This is error-prone approach that leaded to actual error in that case.
This PR enforce that vector of argument match count.
Also fixed flaky HRANDFIELD test.
Triage Assessment
Vulnerability Type: Input validation / robustness
Confidence: MEDIUM
Reasoning:
The commit tightens input validation by requiring the provided 'numFields' to exactly match the actual number of arguments. This reduces the risk of malformed commands being processed in a way that could lead to security-relevant misbehavior (e.g., incorrect field handling or unexpected command paths). It also changes error messaging to reflect the strict count requirement. While not addressing a concrete exploit like RCE/SQLi, it strengthens robustness against malformed input which has security implications.
Verification Assessment
Vulnerability Type: Input validation / robustness
Confidence: MEDIUM
Affected Versions: 8.6.0 - 8.6.1 (before 8.6.2)
Code Diff
diff --git a/src/t_hash.c b/src/t_hash.c
index b42e3c2593e..45156f46e5d 100644
--- a/src/t_hash.c
+++ b/src/t_hash.c
@@ -2929,8 +2929,8 @@ static void httlGenericCommand(client *c, const char *cmd, long long basetime, i
return;
/* Verify `numFields` is consistent with number of arguments */
- if (numFields > (c->argc - numFieldsAt - 1)) {
- addReplyError(c, "Parameter `numFields` is more than number of arguments");
+ if (numFields != (c->argc - numFieldsAt - 1)) {
+ addReplyError(c, "The `numfields` parameter must match the number of arguments");
return;
}
@@ -3078,6 +3078,7 @@ static void hexpireGenericCommand(client *c, const char *cmd, long long basetime
/* Read the expiry time from command */
if (getLongLongFromObjectOrReply(c, expireArg, &expire, NULL) != C_OK)
return;
+
if (expire < 0) {
addReplyError(c,"invalid expire time, must be >= 0");
return;
@@ -3121,8 +3122,8 @@ static void hexpireGenericCommand(client *c, const char *cmd, long long basetime
return;
/* Verify `numFields` is consistent with number of arguments */
- if (numFields > (c->argc - numFieldsAt - 1)) {
- addReplyError(c, "Parameter `numFields` is more than number of arguments");
+ if (numFields != (c->argc - numFieldsAt - 1)) {
+ addReplyError(c, "The `numfields` parameter must match the number of arguments");
return;
}
@@ -3249,8 +3250,8 @@ void hpersistCommand(client *c) {
return;
/* Verify `numFields` is consistent with number of arguments */
- if (numFields > (c->argc - numFieldsAt - 1)) {
- addReplyError(c, "Parameter `numFields` is more than number of arguments");
+ if (numFields != (c->argc - numFieldsAt - 1)) {
+ addReplyError(c, "The `numfields` parameter must match the number of arguments");
return;
}
diff --git a/tests/unit/type/hash-field-expire.tcl b/tests/unit/type/hash-field-expire.tcl
index 8c71ebed29f..b6dd58043c7 100644
--- a/tests/unit/type/hash-field-expire.tcl
+++ b/tests/unit/type/hash-field-expire.tcl
@@ -149,7 +149,9 @@ start_server {tags {"external:skip needs:debug"}} {
r del myhash
r hset myhash f1 v1
assert_error {*Parameter `numFields` should be greater than 0} {r hpexpire myhash 1000 NX FIELDS 0 f1 f2 f3}
- assert_error {*Parameter `numFields` is more than number of arguments} {r hpexpire myhash 1000 NX FIELDS 4 f1 f2 f3}
+ # <count> not match with actual number of fields
+ assert_error {*parameter must match the number*} {r hpexpire myhash 1000 NX FIELDS 4 f1 f2 f3}
+ assert_error {*parameter must match the number*} {r hpexpire myhash 1000 NX FIELDS 2 f1 f2 f3}
}
test "HPEXPIRE - parameter expire-time near limit of 2^46 ($type)" {
@@ -262,8 +264,9 @@ start_server {tags {"external:skip needs:debug"}} {
foreach cmd {HTTL HPTTL} {
assert_equal [r $cmd myhash FIELDS 2 field2 non_exists_field] "$T_NO_EXPIRY $T_NO_FIELD"
- # Set numFields less than actual number of fields. Fine.
- assert_equal [r $cmd myhash FIELDS 1 non_exists_field1 non_exists_field2] "$T_NO_FIELD"
+ # <count> not match with actual number of fields
+ assert_error {*parameter must match the number*} {r $cmd myhash FIELDS 1 non_exists_field1 non_exists_field2}
+ assert_error {*parameter must match the number*} {r $cmd myhash FIELDS 3 non_exists_field1 non_exists_field2}
}
}
@@ -674,6 +677,9 @@ start_server {tags {"external:skip needs:debug"}} {
assert_error {*wrong number of arguments*} {r hpersist myhash FIELDS 1}
assert_equal [r hpersist myhash FIELDS 2 f1 not-exists-field] "$P_OK $P_NO_FIELD"
assert_equal [r hpersist myhash FIELDS 1 f2] "$P_NO_EXPIRY"
+ # <count> not match with actual number of fields
+ assert_error {*parameter must match the number*} {r hpersist myhash FIELDS 2 f1 f2 f3}
+ assert_error {*parameter must match the number*} {r hpersist myhash FIELDS 4 f1 f2 f3}
}
test "HPERSIST - verify fields with TTL are persisted ($type)" {
@@ -928,13 +934,13 @@ start_server {tags {"external:skip needs:debug"}} {
# Next command won't be propagated to replica
# because XX condition not met or field not exists
- r hexpire h1 10 XX FIELDS 1 f1 f2 non_exists_field
+ r hexpire h1 10 XX FIELDS 3 f1 f2 non_exists_field
r hpexpire h1 20 FIELDS 1 f1
# Next command will be propagate with only field 'f2'
# because NX condition not met for field 'f1'
- r hpexpire h1 30 NX FIELDS 1 f1 f2
+ r hpexpire h1 30 NX FIELDS 2 f1 f2
# Non exists field should be ignored
r hpexpire h1 30 FIELDS 1 non_exists_field
@@ -1035,8 +1041,8 @@ start_server {tags {"external:skip needs:debug"}} {
# Verify HRANDFIELD deletes expired fields and propagates it
r hset h2 f1 v1 f2 v2
- r hpexpire h2 1 FIELDS 1 f1
- r hpexpire h2 50 FIELDS 1 f2
+ r hpexpire h2 1 FIELDS 2 f1 f2
+ after 5
assert_equal [r hrandfield h4 2] ""
after 200
@@ -1048,10 +1054,9 @@ start_server {tags {"external:skip needs:debug"}} {
{hpexpireat h1 * NX FIELDS 3 f3 f4 f5}
{hpexpireat h1 * FIELDS 1 f6}
{hset h2 f1 v1 f2 v2}
- {hpexpireat h2 * FIELDS 1 f1}
- {hpexpireat h2 * FIELDS 1 f2}
- {hdel h2 f1}
- {hdel h2 f2}
+ {hpexpireat h2 * FIELDS 2 f1 f2}
+ {hdel h2 *}
+ {hdel h2 *}
}
array set keyAndFields1 [dumpAllHashes r]
@@ -1099,26 +1104,46 @@ start_server {tags {"external:skip needs:debug"}} {
} {} {needs:repl}
test {HRANDFIELD delete expired fields and propagate DELs to replica} {
+ r debug set-active-expire 0
r flushall
set repl [attach_to_replication_stream]
- r hset h4 f1 v1 f2 v2
- r hpexpire h4 1 FIELDS 1 f1
- r hpexpire h4 2 FIELDS 1 f2
- after 100
- assert_equal [r hrandfield h4 2] ""
+ # HRANDFIELD delete expired fields and propagate MULTI-EXEC DELs. Reply none.
+ r hset h1 f1 v1 f2 v2
+ r hpexpire h1 1 FIELDS 2 f1 f2
+ after 5
+ assert_equal [r hrandfield h1 2] ""
+
+ # HRANDFIELD delete expired field and propagate DEL. Reply non-expired field.
+ r hset h2 f1 v1 f2 v2
+ r hpexpire h2 1 FIELDS 1 f1
+ after 5
+ assert_equal [r hrandfield h2 2] "f2"
+ # HRANDFIELD delete expired field and propagate DEL. Reply none.
+ r hset h3 f1 v1
+ r hpexpire h3 1 FIELDS 1 f1
+ after 5
+ assert_equal [r hrandfield h3 2] ""
assert_replication_stream $repl {
{select *}
- {hset h4 f1 v1 f2 v2}
- {hpexpireat h4 * FIELDS 1 f1}
- {hpexpireat h4 * FIELDS 1 f2}
- {hdel h4 f1}
- {hdel h4 f2}
+ {hset h1 f1 v1 f2 v2}
+ {hpexpireat h1 * FIELDS 2 f1 f2}
+ {multi}
+ {hdel h1 *}
+ {hdel h1 *}
+ {exec}
+ {hset h2 f1 v1 f2 v2}
+ {hpexpireat h2 * FIELDS 1 f1}
+ {hdel h2 f1}
+ {hset h3 f1 v1}
+ {hpexpireat h3 * FIELDS 1 f1}
+ {hdel h3 f1}
}
close_replication_stream $repl
- } {} {needs:repl}
+ r debug set-active-expire 1
+ } {OK} {needs:repl}
# Start another server to test replication of TTLs
start_server {tags {needs:repl external:skip}} {
@@ -1163,4 +1188,3 @@ start_server {tags {"external:skip needs:debug"}} {
}
}
}
-