From 61e5563ff643ca91e63b6a144cb23bfbdac8a067 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Tue, 28 Feb 2023 15:15:46 +0200 Subject: [PATCH 1/5] Integer Overflow in RAND commands can lead to assertion (CVE-2023-25155) (#11857) Issue happens when passing a negative long value that greater than the max positive value that the long can store. --- src/t_hash.cpp | 6 +++--- src/t_set.cpp | 2 +- src/t_zset.cpp | 6 +++--- tests/unit/type/hash.tcl | 2 ++ tests/unit/type/set.tcl | 5 +++++ tests/unit/type/zset.tcl | 2 ++ 6 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/t_hash.cpp b/src/t_hash.cpp index ed1bb4d63..1d152c463 100644 --- a/src/t_hash.cpp +++ b/src/t_hash.cpp @@ -1221,13 +1221,13 @@ void hrandfieldCommand(client *c) { ziplistEntry ele; if (c->argc >= 3) { - if (getLongFromObjectOrReply(c,c->argv[2],&l,NULL) != C_OK) return; - if (c->argc > 4 || (c->argc == 4 && strcasecmp(szFromObj(c->argv[3]),"withvalues"))) { + if (getRangeLongFromObjectOrReply(c,c->argv[2],-LONG_MAX,LONG_MAX,&l,NULL) != C_OK) return; + if (c->argc > 4 || (c->argc == 4 && strcasecmp(c->argv[3]->ptr,"withvalues"))) { addReplyErrorObject(c,shared.syntaxerr); return; } else if (c->argc == 4) { withvalues = 1; - if (l < LONG_MIN/2 || l > LONG_MAX/2) { + if (l < -LONG_MAX/2 || l > LONG_MAX/2) { addReplyError(c,"value is out of range"); return; } diff --git a/src/t_set.cpp b/src/t_set.cpp index 23b161c10..9d69ca9c7 100644 --- a/src/t_set.cpp +++ b/src/t_set.cpp @@ -672,7 +672,7 @@ void srandmemberWithCountCommand(client *c) { dict *d; - if (getLongFromObjectOrReply(c,c->argv[2],&l,NULL) != C_OK) return; + if (getRangeLongFromObjectOrReply(c,c->argv[2],-LONG_MAX,LONG_MAX,&l,NULL) != C_OK) return; if (l >= 0) { count = (unsigned long) l; } else { diff --git a/src/t_zset.cpp b/src/t_zset.cpp index 4d351da45..c1ceaeb81 100644 --- a/src/t_zset.cpp +++ b/src/t_zset.cpp @@ -4210,13 +4210,13 @@ void zrandmemberCommand(client *c) { ziplistEntry ele; if (c->argc >= 3) { - if (getLongFromObjectOrReply(c,c->argv[2],&l,NULL) != C_OK) return; - if (c->argc > 4 || (c->argc == 4 && strcasecmp(szFromObj(c->argv[3]),"withscores"))) { + if (getRangeLongFromObjectOrReply(c,c->argv[2],-LONG_MAX,LONG_MAX,&l,NULL) != C_OK) return; + if (c->argc > 4 || (c->argc == 4 && strcasecmp(c->argv[3]->ptr,"withscores"))) { addReplyErrorObject(c,shared.syntaxerr); return; } else if (c->argc == 4) { withscores = 1; - if (l < LONG_MIN/2 || l > LONG_MAX/2) { + if (l < -LONG_MAX/2 || l > LONG_MAX/2) { addReplyError(c,"value is out of range"); return; } diff --git a/tests/unit/type/hash.tcl b/tests/unit/type/hash.tcl index 091b1cee1..76a1e53df 100644 --- a/tests/unit/type/hash.tcl +++ b/tests/unit/type/hash.tcl @@ -71,6 +71,8 @@ start_server {tags {"hash"}} { test "HRANDFIELD count overflow" { r hmset myhash a 1 assert_error {*value is out of range*} {r hrandfield myhash -9223372036854770000 withvalues} + assert_error {*value is out of range*} {r hrandfield myhash -9223372036854775808 withvalues} + assert_error {*value is out of range*} {r hrandfield myhash -9223372036854775808} } {} test "HRANDFIELD with against non existing key" { diff --git a/tests/unit/type/set.tcl b/tests/unit/type/set.tcl index ee7b936b5..a24b4c601 100644 --- a/tests/unit/type/set.tcl +++ b/tests/unit/type/set.tcl @@ -588,6 +588,11 @@ start_server { r srandmember nonexisting_key 100 } {} + test "SRANDMEMBER count overflow" { + r sadd myset a + assert_error {*value is out of range*} {r srandmember myset -9223372036854775808} + } {} + # Make sure we can distinguish between an empty array and a null response r readraw 1 diff --git a/tests/unit/type/zset.tcl b/tests/unit/type/zset.tcl index 5ea619ea3..02184fc8c 100644 --- a/tests/unit/type/zset.tcl +++ b/tests/unit/type/zset.tcl @@ -1717,6 +1717,8 @@ start_server {tags {"zset"}} { test "ZRANDMEMBER count overflow" { r zadd myzset 0 a assert_error {*value is out of range*} {r zrandmember myzset -9223372036854770000 withscores} + assert_error {*value is out of range*} {r zrandmember myzset -9223372036854775808 withscores} + assert_error {*value is out of range*} {r zrandmember myzset -9223372036854775808} } {} # Make sure we can distinguish between an empty array and a null response From 6a94b01f814cac6841c97617d35fa37cdc415042 Mon Sep 17 00:00:00 2001 From: chendianqiang Date: Sun, 28 Aug 2022 16:33:41 +0800 Subject: [PATCH 2/5] fix hincrbyfloat not to create a key if the new value is invalid (#11149) Check the validity of the value before performing the create operation, prevents new data from being generated even if the request fails to execute. Co-authored-by: Oran Agra Co-authored-by: chendianqiang Co-authored-by: Binbin --- src/t_hash.cpp | 4 ++++ tests/unit/type/hash.tcl | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/src/t_hash.cpp b/src/t_hash.cpp index 1d152c463..5be55b218 100644 --- a/src/t_hash.cpp +++ b/src/t_hash.cpp @@ -729,6 +729,10 @@ void hincrbyfloatCommand(client *c) { unsigned int vlen; if (getLongDoubleFromObjectOrReply(c,c->argv[3],&incr,NULL) != C_OK) return; + if (isnan(incr) || isinf(incr)) { + addReplyError(c,"value is NaN or Infinity"); + return; + } if ((o = hashTypeLookupWriteOrCreate(c,c->argv[1])) == NULL) return; if (hashTypeGetValue(o,szFromObj(c->argv[2]),&vstr,&vlen,&ll) == C_OK) { if (vstr) { diff --git a/tests/unit/type/hash.tcl b/tests/unit/type/hash.tcl index 76a1e53df..916ebd534 100644 --- a/tests/unit/type/hash.tcl +++ b/tests/unit/type/hash.tcl @@ -833,4 +833,8 @@ start_server {tags {"hash"}} { set _ $k } {ZIP_INT_8B 127 ZIP_INT_16B 32767 ZIP_INT_32B 2147483647 ZIP_INT_64B 9223372036854775808 ZIP_INT_IMM_MIN 0 ZIP_INT_IMM_MAX 12} + test {HINCRBYFLOAT does not allow NaN or Infinity} { + assert_error "*value is NaN or Infinity*" {r hincrbyfloat hfoo field +inf} + assert_equal 0 [r exists hfoo] + } } From 2fccacca4d3f12384fc9d73923cfb7cf7d4782fa Mon Sep 17 00:00:00 2001 From: Malavan Sotheeswaran Date: Mon, 17 Apr 2023 13:46:22 -0700 Subject: [PATCH 3/5] ->ptr to ptrFromObj --- src/t_hash.cpp | 2 +- src/t_zset.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/t_hash.cpp b/src/t_hash.cpp index 5be55b218..f09b06001 100644 --- a/src/t_hash.cpp +++ b/src/t_hash.cpp @@ -1226,7 +1226,7 @@ void hrandfieldCommand(client *c) { if (c->argc >= 3) { if (getRangeLongFromObjectOrReply(c,c->argv[2],-LONG_MAX,LONG_MAX,&l,NULL) != C_OK) return; - if (c->argc > 4 || (c->argc == 4 && strcasecmp(c->argv[3]->ptr,"withvalues"))) { + if (c->argc > 4 || (c->argc == 4 && strcasecmp(ptrFromObj(c->argv[3]),"withvalues"))) { addReplyErrorObject(c,shared.syntaxerr); return; } else if (c->argc == 4) { diff --git a/src/t_zset.cpp b/src/t_zset.cpp index c1ceaeb81..87603577f 100644 --- a/src/t_zset.cpp +++ b/src/t_zset.cpp @@ -4211,7 +4211,7 @@ void zrandmemberCommand(client *c) { if (c->argc >= 3) { if (getRangeLongFromObjectOrReply(c,c->argv[2],-LONG_MAX,LONG_MAX,&l,NULL) != C_OK) return; - if (c->argc > 4 || (c->argc == 4 && strcasecmp(c->argv[3]->ptr,"withscores"))) { + if (c->argc > 4 || (c->argc == 4 && strcasecmp(ptrFromObj(c->argv[3]),"withscores"))) { addReplyErrorObject(c,shared.syntaxerr); return; } else if (c->argc == 4) { From 2f1f101c810dc9b7a9bdd103e8555b29ae5ce322 Mon Sep 17 00:00:00 2001 From: Malavan Sotheeswaran Date: Mon, 17 Apr 2023 13:47:29 -0700 Subject: [PATCH 4/5] ptrFromObj to szFromObj --- src/t_hash.cpp | 2 +- src/t_zset.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/t_hash.cpp b/src/t_hash.cpp index f09b06001..1ca315fec 100644 --- a/src/t_hash.cpp +++ b/src/t_hash.cpp @@ -1226,7 +1226,7 @@ void hrandfieldCommand(client *c) { if (c->argc >= 3) { if (getRangeLongFromObjectOrReply(c,c->argv[2],-LONG_MAX,LONG_MAX,&l,NULL) != C_OK) return; - if (c->argc > 4 || (c->argc == 4 && strcasecmp(ptrFromObj(c->argv[3]),"withvalues"))) { + if (c->argc > 4 || (c->argc == 4 && strcasecmp(szFromObj(c->argv[3]),"withvalues"))) { addReplyErrorObject(c,shared.syntaxerr); return; } else if (c->argc == 4) { diff --git a/src/t_zset.cpp b/src/t_zset.cpp index 87603577f..7aa606bb3 100644 --- a/src/t_zset.cpp +++ b/src/t_zset.cpp @@ -4211,7 +4211,7 @@ void zrandmemberCommand(client *c) { if (c->argc >= 3) { if (getRangeLongFromObjectOrReply(c,c->argv[2],-LONG_MAX,LONG_MAX,&l,NULL) != C_OK) return; - if (c->argc > 4 || (c->argc == 4 && strcasecmp(ptrFromObj(c->argv[3]),"withscores"))) { + if (c->argc > 4 || (c->argc == 4 && strcasecmp(szFromObj(c->argv[3]),"withscores"))) { addReplyErrorObject(c,shared.syntaxerr); return; } else if (c->argc == 4) { From 70612092a162c3590efbbff8aa74ef2c821fcce6 Mon Sep 17 00:00:00 2001 From: Malavan Sotheeswaran Date: Mon, 17 Apr 2023 19:50:22 -0700 Subject: [PATCH 5/5] limit number of random elements returned based on config, exit loop early if client is being closed --- src/config.cpp | 1 + src/server.h | 2 ++ src/t_hash.cpp | 6 +++++- src/t_set.cpp | 6 ++++++ src/t_zset.cpp | 6 +++++- tests/unit/obuf-limits.tcl | 8 ++++++++ 6 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/config.cpp b/src/config.cpp index 073e4aca2..866560d35 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -2912,6 +2912,7 @@ standardConfig configs[] = { createLongLongConfig("repl-backlog-size", NULL, MODIFIABLE_CONFIG, 1, LLONG_MAX, g_pserver->repl_backlog_size, 1024*1024, MEMORY_CONFIG, NULL, updateReplBacklogSize), /* Default: 1mb */ createLongLongConfig("repl-backlog-disk-reserve", NULL, IMMUTABLE_CONFIG, 0, LLONG_MAX, cserver.repl_backlog_disk_size, 0, MEMORY_CONFIG, NULL, NULL), createLongLongConfig("max-snapshot-slip", NULL, MODIFIABLE_CONFIG, 0, 5000, g_pserver->snapshot_slip, 400, 0, NULL, NULL), + createLongLongConfig("max-rand-count", NULL, MODIFIABLE_CONFIG, 0, LONG_MAX/2, g_pserver->rand_total_threshold, LONG_MAX/2, 0, NULL, NULL), /* Unsigned Long Long configs */ createULongLongConfig("maxmemory", NULL, MODIFIABLE_CONFIG, 0, LLONG_MAX, g_pserver->maxmemory, 0, MEMORY_CONFIG, NULL, updateMaxmemory), diff --git a/src/server.h b/src/server.h index eb54f608c..2992a7081 100644 --- a/src/server.h +++ b/src/server.h @@ -2729,6 +2729,8 @@ struct redisServer { long long repl_batch_offStart = -1; long long repl_batch_idxStart = -1; + long long rand_total_threshold; + int config_soft_shutdown = false; bool soft_shutdown = false; diff --git a/src/t_hash.cpp b/src/t_hash.cpp index 1ca315fec..e2d48d91d 100644 --- a/src/t_hash.cpp +++ b/src/t_hash.cpp @@ -1066,6 +1066,8 @@ void hrandfieldWithCountCommand(client *c, long l, int withvalues) { addReplyBulkCBuffer(c, key, sdslen(key)); if (withvalues) addReplyBulkCBuffer(c, value, sdslen(value)); + if (c->flags & CLIENT_CLOSE_ASAP) + break; } } else if (hash->encoding == OBJ_ENCODING_ZIPLIST) { ziplistEntry *keys, *vals = NULL; @@ -1079,6 +1081,8 @@ void hrandfieldWithCountCommand(client *c, long l, int withvalues) { count -= sample_count; ziplistRandomPairs((unsigned char*)ptrFromObj(hash), sample_count, keys, vals); harndfieldReplyWithZiplist(c, sample_count, keys, vals); + if (c->flags & CLIENT_CLOSE_ASAP) + break; } zfree(keys); zfree(vals); @@ -1231,7 +1235,7 @@ void hrandfieldCommand(client *c) { return; } else if (c->argc == 4) { withvalues = 1; - if (l < -LONG_MAX/2 || l > LONG_MAX/2) { + if (l < -g_pserver->rand_total_threshold || l > g_pserver->rand_total_threshold) { addReplyError(c,"value is out of range"); return; } diff --git a/src/t_set.cpp b/src/t_set.cpp index 9d69ca9c7..36342199c 100644 --- a/src/t_set.cpp +++ b/src/t_set.cpp @@ -673,6 +673,10 @@ void srandmemberWithCountCommand(client *c) { dict *d; if (getRangeLongFromObjectOrReply(c,c->argv[2],-LONG_MAX,LONG_MAX,&l,NULL) != C_OK) return; + if (l < -g_pserver->rand_total_threshold || l > g_pserver->rand_total_threshold) { + addReplyError(c,"value is out of range"); + return; + } if (l >= 0) { count = (unsigned long) l; } else { @@ -706,6 +710,8 @@ void srandmemberWithCountCommand(client *c) { } else { addReplyBulkCBuffer(c,ele,sdslen(ele)); } + if (c->flags & CLIENT_CLOSE_ASAP) + break; } return; } diff --git a/src/t_zset.cpp b/src/t_zset.cpp index 7aa606bb3..2ee1d3cc9 100644 --- a/src/t_zset.cpp +++ b/src/t_zset.cpp @@ -4054,6 +4054,8 @@ void zrandmemberWithCountCommand(client *c, long l, int withscores) { addReplyBulkCBuffer(c, key, sdslen(key)); if (withscores) addReplyDouble(c, *(double*)dictGetVal(de)); + if (c->flags & CLIENT_CLOSE_ASAP) + break; } } else if (zsetobj->encoding == OBJ_ENCODING_ZIPLIST) { ziplistEntry *keys, *vals = NULL; @@ -4067,6 +4069,8 @@ void zrandmemberWithCountCommand(client *c, long l, int withscores) { count -= sample_count; ziplistRandomPairs((unsigned char*)ptrFromObj(zsetobj), sample_count, keys, vals); zarndmemberReplyWithZiplist(c, sample_count, keys, vals); + if (c->flags & CLIENT_CLOSE_ASAP) + break; } zfree(keys); zfree(vals); @@ -4216,7 +4220,7 @@ void zrandmemberCommand(client *c) { return; } else if (c->argc == 4) { withscores = 1; - if (l < -LONG_MAX/2 || l > LONG_MAX/2) { + if (l < -g_pserver->rand_total_threshold || l > g_pserver->rand_total_threshold) { addReplyError(c,"value is out of range"); return; } diff --git a/tests/unit/obuf-limits.tcl b/tests/unit/obuf-limits.tcl index 77c8e0bec..0cefb9129 100644 --- a/tests/unit/obuf-limits.tcl +++ b/tests/unit/obuf-limits.tcl @@ -182,4 +182,12 @@ start_server {tags {"obuf-limits"} overrides { server-threads 1 }} { assert_equal "v2" [r get k2] assert_equal "v3" [r get k3] } + + test "Obuf limit, HRANDFIELD with huge count stopped mid-run" { + r config set client-output-buffer-limit {normal 1000000 0 0} + r hset myhash a b + catch {r hrandfield myhash -999999999} e + assert_match "*I/O error*" $e + reconnect + } }