CVE-2023-46852: Buffer Copy without Checking Size of Input ('Classic Buffer Overflow') (CWE-120)
value: In Memcached before 1.6.22, a buffer overflow exists when processing multiget requests in proxy mode, if there are many spaces after the "get" substring.
CWE description: The product copies an input buffer to an output buffer without verifying that the size of the input buffer is less than the size of the output buffer, leading to a buffer overflow.
CVSS v3 base score: 7.5
Published: 2023-10-27T20:15Z
Repository: https://github.com/memcached/memcached
Language: Other
Files changed: proto_proxy.c, t/proxyunits.t
Commit date: 2023-07-28 10:32:16 -0700
Commit message: proxy: fix buffer overflow with multiget syntax

"get[200 spaces]key1 key2\r\n" would overflow a temporary buffer used to
process multiget syntax.

To exploit this you must first pass the check in try_read_command_proxy:
- The request before the first newline must be less than 1024 bytes.
- If it is more than 1024 bytes there is a limit of 100 spaces.
- The key length is still checked at 250 bytes
- Meaning you have up to 772 spaces and then the key to create stack
  corruption.

So the amount of data you can shove in here isn't unlimited.

The fix caps the amount of data pre-key to be reasonable. Something like
GAT needs space for a 32bit TTL which is at most going to be 15 bytes +
spaces, so we limit it to 20 bytes.

I hate hate hate hate hate the multiget syntax. hate it.

Diff: diff --git a/proto_proxy.c b/proto_proxy.c
index 890ed2d3..62f5433c 100644
--- a/proto_proxy.c
+++ b/proto_proxy.c
@@ -719,10 +719,16 @@ int proxy_run_coroutine(lua_State *Lc, mc_resp *resp, io_pending_proxy_t *p, con
     }
 
     return 0;
 }
 
+// basically any data before the first key.
+// max is like 15ish plus spaces. we can be more strict about how many spaces
+// to expect because any client spamming space is being deliberately stupid
+// anyway.
+#define MAX_CMD_PREFIX 20
+
 static void proxy_process_command(conn *c, char *command, size_t cmdlen, bool multiget) {
     assert(c != NULL);
     LIBEVENT_THREAD *thr = c->thread;
     struct proxy_hook *hooks = thr->proxy_hooks;
     lua_State *L = thr->L;
@@ -791,16 +797,22 @@ static void proxy_process_command(conn *c, char *command, size_t cmdlen, bool mu
     // function call, then we don't re-process the above bits in the same way?
     // The way this is detected/passed on is very fragile.
     if (!multiget && pr.cmd_type == CMD_TYPE_GET && pr.has_space) {
         uint32_t keyoff = pr.tokens[pr.keytoken];
         while (pr.klen != 0) {
-            char temp[KEY_MAX_LENGTH + 30];
+            char temp[KEY_MAX_LENGTH + MAX_CMD_PREFIX + 30];
             char *cur = temp;
             // Core daemon can abort the entire command if one key is bad, but
             // we cannot from the proxy. Instead we have to inject errors into
             // the stream. This should, thankfully, be rare at least.
-            if (pr.klen > KEY_MAX_LENGTH) {
+            if (pr.tokens[pr.keytoken] > MAX_CMD_PREFIX) {
+                if (!resp_start(c)) {
+                    conn_set_state(c, conn_closing);
+                    return;
+                }
+                proxy_out_errstring(c->resp, PROXY_CLIENT_ERROR, "malformed request");
+            } else if (pr.klen > KEY_MAX_LENGTH) {
                 if (!resp_start(c)) {
                     conn_set_state(c, conn_closing);
                     return;
                 }
                 proxy_out_errstring(c->resp, PROXY_CLIENT_ERROR, "key too long");
diff --git a/t/proxyunits.t b/t/proxyunits.t
index bebe26a5..3ebf3ed4 100644
--- a/t/proxyunits.t
+++ b/t/proxyunits.t
@@ -149,10 +149,16 @@ sub proxy_test {
 
     for ('get', 'incr', 'decr', 'touch', 'gat', 'gats', 'mg', 'md', 'ma', 'ms') {
         print $ps "$_\r\n";
         is(scalar <$ps>, "CLIENT_ERROR parsing request\r\n", "$_ got CLIENT_ERROR for too few tokens");
     }
+
+    my $space = ' ' x 200;
+    print $ps "get$space key key\r\n";
+    is(scalar <$ps>, "CLIENT_ERROR malformed request\r\n");
+    is(scalar <$ps>, "CLIENT_ERROR malformed request\r\n");
+    is(scalar <$ps>, "END\r\n"); # god damn multiget syntax.
 }
 
 # Basic test with a backend; write a request to the client socket, read it
 # from a backend socket, and write a response to the backend socket.
 #

Vulnerable code:             char temp[KEY_MAX_LENGTH + 30];
            if (pr.klen > KEY_MAX_LENGTH) {
Fixed code: // basically any data before the first key.
// max is like 15ish plus spaces. we can be more strict about how many spaces
// to expect because any client spamming space is being deliberately stupid
// anyway.
#define MAX_CMD_PREFIX 20

            char temp[KEY_MAX_LENGTH + MAX_CMD_PREFIX + 30];
            if (pr.tokens[pr.keytoken] > MAX_CMD_PREFIX) {
                if (!resp_start(c)) {
                    conn_set_state(c, conn_closing);
                    return;
                }
                proxy_out_errstring(c->resp, PROXY_CLIENT_ERROR, "malformed request");
            } else if (pr.klen > KEY_MAX_LENGTH) {

    my $space = ' ' x 200;
    print $ps "get$space key key\r\n";
    is(scalar <$ps>, "CLIENT_ERROR malformed request\r\n");
    is(scalar <$ps>, "CLIENT_ERROR malformed request\r\n");
    is(scalar <$ps>, "END\r\n"); # god damn multiget syntax.