File CVE-2025-21605.patch of Package valkey.39739
From 507c9eb73e27745069f5259f727fadae99ef3885 Mon Sep 17 00:00:00 2001
From: uriyage <78144248+uriyage@users.noreply.github.com>
Date: Wed, 23 Apr 2025 23:30:56 +0300
Subject: [PATCH] Add output buffer limiting for unauthenticated clients
(#1993)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This commit introduces a mechanism to track client authentication state
with a new `ever_authenticated` flag. It refactors client authentication
handling by adding a `clientSetUser` function that properly sets both
the authenticated and `ever_authenticated` flags.
The implementation limits output buffer size for clients that have never
been authenticated.
Added tests to verify the output buffer limiting behavior for
unauthenticated clients.
---------
Signed-off-by: Uri Yagelnik <uriy@amazon.com>
Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
---
src/acl.c | 7 +++---
src/debug.c | 6 ++++++
src/module.c | 6 ++----
src/networking.c | 26 ++++++++++++++++++++---
src/replication.c | 4 ++--
src/server.c | 1 +
src/server.h | 17 +++++++++------
tests/unit/auth.tcl | 52 +++++++++++++++++++++++++++++++++++++++++++++
8 files changed, 99 insertions(+), 20 deletions(-)
diff --git a/src/acl.c b/src/acl.c
index d266137d72..14087ea2f4 100644
--- a/src/acl.c
+++ b/src/acl.c
@@ -505,8 +505,7 @@ void ACLFreeUserAndKillClients(user *u) {
* this may result in some security hole: it's much
* more defensive to set the default user and put
* it in non authenticated mode. */
- c->user = DefaultUser;
- c->flag.authenticated = 0;
+ clientSetUser(c, DefaultUser, 0);
/* We will write replies to this client later, so we can't
* close it directly even if async. */
if (c == server.current_client) {
@@ -1499,8 +1498,8 @@ void addAuthErrReply(client *c, robj *err) {
* The return value is AUTH_OK on success (valid username / password pair) & AUTH_ERR otherwise. */
int checkPasswordBasedAuth(client *c, robj *username, robj *password) {
if (ACLCheckUserCredentials(username, password) == C_OK) {
- c->flag.authenticated = 1;
- c->user = ACLGetUserByName(username->ptr, sdslen(username->ptr));
+ user *user = ACLGetUserByName(username->ptr, sdslen(username->ptr));
+ clientSetUser(c, user, 1);
moduleNotifyUserChanged(c);
return AUTH_OK;
} else {
diff --git a/src/debug.c b/src/debug.c
index 7314faa2eb..a01ccf9730 100644
--- a/src/debug.c
+++ b/src/debug.c
@@ -503,6 +503,9 @@ void debugCommand(client *c) {
" Grace period in seconds for replica main channel to establish psync.",
"DICT-RESIZING <0|1>",
" Enable or disable the main dict and expire dict resizing.",
+ "CLIENT-ENFORCE-REPLY-LIST <0|1>",
+ " When set to 1, it enforces the use of the client reply list directly",
+ " and avoids using the client's static buffer.",
NULL};
addExtendedReplyHelp(c, help, clusterDebugCommandExtendedHelp());
} else if (!strcasecmp(c->argv[1]->ptr, "segfault")) {
@@ -1014,6 +1017,9 @@ void debugCommand(client *c) {
} else if (!strcasecmp(c->argv[1]->ptr, "dict-resizing") && c->argc == 3) {
server.dict_resizing = atoi(c->argv[2]->ptr);
addReply(c, shared.ok);
+ } else if (!strcasecmp(c->argv[1]->ptr, "client-enforce-reply-list") && c->argc == 3) {
+ server.debug_client_enforce_reply_list = atoi(c->argv[2]->ptr);
+ addReply(c, shared.ok);
} else if (!handleDebugClusterCommand(c)) {
addReplySubcommandSyntaxError(c);
return;
diff --git a/src/module.c b/src/module.c
index 7080293122..93d11b52d4 100644
--- a/src/module.c
+++ b/src/module.c
@@ -9505,8 +9505,7 @@ void revokeClientAuthentication(client *c) {
* is eventually freed we don't rely on the module to still exist. */
moduleNotifyUserChanged(c);
- c->user = DefaultUser;
- c->flag.authenticated = 0;
+ clientSetUser(c, DefaultUser, 0);
/* We will write replies to this client later, so we can't close it
* directly even if async. */
if (c == server.current_client) {
@@ -9827,8 +9826,7 @@ static int authenticateClientWithUser(ValkeyModuleCtx *ctx,
moduleNotifyUserChanged(ctx->client);
- ctx->client->user = user;
- ctx->client->flag.authenticated = 1;
+ clientSetUser(ctx->client, user, 1);
if (clientHasModuleAuthInProgress(ctx->client)) {
ctx->client->flag.module_auth_has_result = 1;
diff --git a/src/networking.c b/src/networking.c
index 9f4506c5fe..ab2df89a59 100644
--- a/src/networking.c
+++ b/src/networking.c
@@ -101,8 +101,22 @@ void linkClient(client *c) {
static void clientSetDefaultAuth(client *c) {
/* If the default user does not require authentication, the user is
* directly authenticated. */
- c->user = DefaultUser;
- c->flag.authenticated = (c->user->flags & USER_FLAG_NOPASS) && !(c->user->flags & USER_FLAG_DISABLED);
+ clientSetUser(c, DefaultUser, (DefaultUser->flags & USER_FLAG_NOPASS) && !(DefaultUser->flags & USER_FLAG_DISABLED));
+}
+
+/* Attach the user u to this client.
+ * Also, mark the client authentication state. In case the client is marked as authenticated,
+ * it will also set the ever_authenticated flag on the client in order to avoid low level
+ * limiting of the client output buffer.*/
+void clientSetUser(client *c, user *u, int authenticated) {
+ c->user = u;
+ c->flag.authenticated = authenticated;
+ if (authenticated)
+ c->flag.ever_authenticated = authenticated;
+}
+
+static int clientEverAuthenticated(client *c) {
+ return c->flag.ever_authenticated;
}
int authRequired(client *c) {
@@ -378,7 +392,8 @@ void deleteCachedResponseClient(client *recording_client) {
VALKEY_NO_SANITIZE("bounds")
size_t _addReplyToBuffer(client *c, const char *s, size_t len) {
size_t available = c->buf_usable_size - c->bufpos;
-
+ /* If the debug enforcing to use the reply list is enabled.*/
+ if (server.debug_client_enforce_reply_list) return 0;
/* If there already are entries in the reply list, we cannot
* add anything more to the static buffer. */
if (listLength(c->reply) > 0) return 0;
@@ -4377,6 +4392,11 @@ int checkClientOutputBufferLimits(client *c) {
int soft = 0, hard = 0, class;
unsigned long used_mem = getClientOutputBufferMemoryUsage(c);
+ /* For unauthenticated clients which were also never authenticated before the output buffer is limited to prevent
+ * them from abusing it by not reading the replies */
+ if (used_mem > REPLY_BUFFER_SIZE_UNAUTHENTICATED_CLIENT && authRequired(c) && !clientEverAuthenticated(c))
+ return 1;
+
class = getClientType(c);
/* For the purpose of output buffer limiting, primaries are handled
* like normal clients. */
diff --git a/src/replication.c b/src/replication.c
index daa975134d..4f87aa7df5 100644
--- a/src/replication.c
+++ b/src/replication.c
@@ -1902,7 +1902,7 @@ void replicationCreatePrimaryClientWithHandler(connection *conn, int dbid, Conne
* execution is done. This is the reason why we allow blocking the replication
* connection. */
server.primary->flag.primary = 1;
- server.primary->flag.authenticated = 1;
+ clientSetUser(server.primary, NULL, 1);
/* Allocate a private query buffer for the primary client instead of using the shared query buffer.
* This is done because the primary's query buffer data needs to be preserved for my sub-replicas to use. */
@@ -4140,7 +4140,7 @@ void establishPrimaryConnection(void) {
connSetPrivateData(server.primary->conn, server.primary);
server.primary->flag.close_after_reply = 0;
server.primary->flag.close_asap = 0;
- server.primary->flag.authenticated = 1;
+ clientSetUser(server.primary, NULL, 1);
server.primary->last_interaction = server.unixtime;
server.repl_state = REPL_STATE_CONNECTED;
server.repl_down_since = 0;
diff --git a/src/server.c b/src/server.c
index 4243185b47..eb9cf04e82 100644
--- a/src/server.c
+++ b/src/server.c
@@ -2621,6 +2621,7 @@ void initServer(void) {
server.reply_buffer_peak_reset_time = REPLY_BUFFER_DEFAULT_PEAK_RESET_TIME;
server.reply_buffer_resizing_enabled = 1;
server.client_mem_usage_buckets = NULL;
+ server.debug_client_enforce_reply_list = 0;
resetReplicationBuffer();
/* Make sure the locale is set on startup based on the config file. */
diff --git a/src/server.h b/src/server.h
index 82cec6c7f5..f2fce0d328 100644
--- a/src/server.h
+++ b/src/server.h
@@ -193,7 +193,8 @@ struct hdr_histogram;
#define PROTO_REPLY_MIN_BYTES (1024) /* the lower limit on reply buffer size */
#define REDIS_AUTOSYNC_BYTES (1024 * 1024 * 4) /* Sync file every 4MB. */
-#define REPLY_BUFFER_DEFAULT_PEAK_RESET_TIME 5000 /* 5 seconds */
+#define REPLY_BUFFER_DEFAULT_PEAK_RESET_TIME 5000 /* 5 seconds */
+#define REPLY_BUFFER_SIZE_UNAUTHENTICATED_CLIENT 1024 /* 1024 bytes */
/* When configuring the server eventloop, we setup it so that the total number
* of file descriptors we can handle are server.maxclients + RESERVED_FDS +
@@ -1224,6 +1225,7 @@ typedef struct ClientFlags {
uint64_t reprocessing_command : 1; /* The client is re-processing the command. */
uint64_t replication_done : 1; /* Indicate that replication has been done on the client */
uint64_t authenticated : 1; /* Indicate a client has successfully authenticated */
+ uint64_t ever_authenticated : 1; /* Indicate a client was ever successfully authenticated during it's lifetime */
uint64_t
protected_rdb_channel : 1; /* Dual channel replication sync: Protects the RDB client from premature \
* release during full sync. This flag is used to ensure that the RDB client, which \
@@ -1244,7 +1246,7 @@ typedef struct ClientFlags {
* knows that it does not need the cache and required a full sync. With this
* flag, we won't cache the primary in freeClient. */
uint64_t fake : 1; /* This is a fake client without a real connection. */
- uint64_t reserved : 5; /* Reserved for future use */
+ uint64_t reserved : 4; /* Reserved for future use */
} ClientFlags;
typedef struct client {
@@ -1771,11 +1773,11 @@ struct valkeyServer {
int events_per_io_thread; /* Number of events on the event loop to trigger IO threads activation. */
int prefetch_batch_max_size; /* Maximum number of keys to prefetch in a single batch */
long long events_processed_while_blocked; /* processEventsWhileBlocked() */
- int enable_protected_configs; /* Enable the modification of protected configs, see PROTECTED_ACTION_ALLOWED_* */
- int enable_debug_cmd; /* Enable DEBUG commands, see PROTECTED_ACTION_ALLOWED_* */
- int enable_module_cmd; /* Enable MODULE commands, see PROTECTED_ACTION_ALLOWED_* */
- int enable_debug_assert; /* Enable debug asserts */
-
+ int enable_protected_configs; /* Enable the modification of protected configs, see PROTECTED_ACTION_ALLOWED_* */
+ int enable_debug_cmd; /* Enable DEBUG commands, see PROTECTED_ACTION_ALLOWED_* */
+ int enable_module_cmd; /* Enable MODULE commands, see PROTECTED_ACTION_ALLOWED_* */
+ int enable_debug_assert; /* Enable debug asserts */
+ int debug_client_enforce_reply_list; /* Force client to always use the reply list */
/* RDB / AOF loading information */
volatile sig_atomic_t loading; /* We are loading data from disk if true */
volatile sig_atomic_t async_loading; /* We are loading data without blocking the db being served */
@@ -2890,6 +2892,7 @@ void initSharedQueryBuf(void);
void freeSharedQueryBuf(void);
client *lookupClientByID(uint64_t id);
int authRequired(client *c);
+void clientSetUser(client *c, user *u, int authenticated);
void putClientInPendingWriteQueue(client *c);
client *createCachedResponseClient(int resp);
void deleteCachedResponseClient(client *recording_client);
diff --git a/tests/unit/auth.tcl b/tests/unit/auth.tcl
index 5654bf7763..0616d310f8 100644
--- a/tests/unit/auth.tcl
+++ b/tests/unit/auth.tcl
@@ -45,6 +45,58 @@ start_server {tags {"auth external:skip"} overrides {requirepass foobar}} {
assert_match {*unauthenticated bulk length*} $e
$rr close
}
+
+ test {For unauthenticated clients output buffer is limited} {
+ set rr [valkey [srv "host"] [srv "port"] 1 $::tls]
+
+ # make sure the client is no longer authenticated
+ $rr SET x 5
+ catch {[$rr read]} e
+ assert_match {*NOAUTH Authentication required*} $e
+
+ # Force clients to use the reply list in order
+ # to make them have a larger than 1K output buffer on any reply
+ assert_match "OK" [r debug client-enforce-reply-list 1]
+
+ # Fill the output buffer without reading it and make
+ # sure the client disconnected.
+ $rr SET x 5
+ catch {[$rr read]} e
+ assert_match {I/O error reading reply} $e
+
+ # Reset the debug
+ assert_match "OK" [r debug client-enforce-reply-list 0]
+ }
+
+ test {For once authenticated clients output buffer is NOT limited} {
+ set rr [valkey [srv "host"] [srv "port"] 1 $::tls]
+
+ # first time authenticate client
+ $rr auth foobar
+ assert_equal {OK} [$rr read]
+
+ # now reset the client so it is not authenticated
+ $rr reset
+ assert_equal {RESET} [$rr read]
+
+ # make sure the client is no longer authenticated
+ $rr SET x 5
+ catch {[$rr read]} e
+ assert_match {*NOAUTH Authentication required*} $e
+
+ # Force clients to use the reply list in order
+ # to make them have a larger than 1K output buffer on any reply
+ assert_match "OK" [r debug client-enforce-reply-list 1]
+
+ # Fill the output buffer without reading it and make
+ # sure the client was NOT disconnected.
+ $rr SET x 5
+ catch {[$rr read]} e
+ assert_match {*NOAUTH Authentication required*} $e
+
+ # Reset the debug
+ assert_match "OK" [r debug client-enforce-reply-list 0]
+ }
}
foreach dualchannel {yes no} {
