CVE-2023-46853: Off-by-one Error (CWE-193)
value: In Memcached before 1.6.22, an off-by-one error exists when processing proxy requests in proxy mode, if \n is used instead of \r\n.
CWE description: A product calculates or uses an incorrect maximum or minimum value that is 1 more, or 1 less, than the correct value.
CVSS v3 base score: 9.8
Published: 2023-10-27T20:15Z
Repository: https://github.com/memcached/memcached
Language: Other
Files changed: proxy.h, proxy_request.c, t/proxy.t
Commit date: 2023-08-02 15:45:56 -0700
Commit message: proxy: fix off-by-one if \r is missing

A bunch of the parser assumed we only had \r\n, but I didn't actually
have that strictness set. Some commands worked and some broke in subtle
ways when just "\n" was being submitted.

I'm not 100% confident in this change yet so I'm opening a PR to stage
it while I run some more thorough tests.

Diff: diff --git a/proxy.h b/proxy.h
index 90a93370..633be86c 100644
--- a/proxy.h
+++ b/proxy.h
@@ -274,10 +274,11 @@ struct mcp_parser_s {
     uint8_t cmd_type; // command class.
     uint8_t ntokens;
     uint8_t keytoken; // because GAT. sigh. also cmds without a key.
     uint32_t parsed; // how far into the request we parsed already
     uint32_t reqlen; // full length of request buffer.
+    uint32_t endlen; // index to the start of \r\n or \n
     int vlen;
     uint32_t klen; // length of key.
     uint16_t tokens[PARSER_MAX_TOKENS]; // offsets for start of each token
     bool has_space; // a space was found after the last byte parsed.
     bool noreply; // if quiet/noreply mode is set.
diff --git a/proxy_request.c b/proxy_request.c
index b13d815b..c491da40 100644
--- a/proxy_request.c
+++ b/proxy_request.c
@@ -7,11 +7,11 @@
 // Find the starting offsets of each token; ignoring length.
 // This creates a fast small (<= cacheline) index into the request,
 // where we later scan or directly feed data into API's.
 static int _process_tokenize(mcp_parser_t *pr, const size_t max) {
     const char *s = pr->request;
-    int len = pr->reqlen - 2;
+    int len = pr->endlen;
 
     // since multigets can be huge, we can't purely judge reqlen against this
     // limit, but we also can't index past it since the tokens are shorts.
     if (len > PARSER_MAXLEN) {
         len = PARSER_MAXLEN;
@@ -91,11 +91,11 @@ static int _process_request_key(mcp_parser_t *pr) {
 // Just for ascii multiget: search for next "key" beyond where we stopped
 // tokenizing before.
 // Returns the offset for the next key.
 size_t _process_request_next_key(mcp_parser_t *pr) {
     const char *cur = pr->request + pr->parsed;
-    int remain = pr->reqlen - pr->parsed - 2;
+    int remain = pr->endlen - pr->parsed;
 
     // chew off any leading whitespace.
     while (remain) {
         if (*cur == ' ') {
             remain--;
@@ -124,11 +124,11 @@ static int _process_request_metaflags(mcp_parser_t *pr, int token) {
     if (pr->ntokens <= token) {
         pr->t.meta.flags = 0; // no flags found.
         return 0;
     }
     const char *cur = pr->request + pr->tokens[token];
-    const char *end = pr->request + pr->reqlen - 2;
+    const char *end = pr->request + pr->endlen;
 
     // We blindly convert flags into bits, since the range of possible
     // flags is deliberately < 64.
     int state = 0;
     while (cur != end) {
@@ -292,19 +292,29 @@ int process_request(mcp_parser_t *pr, const char *command, size_t cmdlen) {
     // min command length is 2, plus the "\r\n"
     if (cmdlen < 4) {
         return -1;
     }
 
-    const char *s = memchr(command, ' ', cmdlen-2);
+    // Commands can end with bare '\n's. Depressingly I intended to be strict
+    // with a \r\n requirement but never did this and need backcompat.
+    // In this case we _know_ \n is at cmdlen because we can't enter this
+    // function otherwise.
+    if (cm[cmdlen-2] == '\r') {
+        pr->endlen = cmdlen - 2;
+    } else {
+        pr->endlen = cmdlen - 1;
+    }
+
+    const char *s = memchr(command, ' ', pr->endlen);
     if (s != NULL) {
         cl = s - command;
     } else {
-        cl = cmdlen - 2;
+        cl = pr->endlen;
     }
     pr->keytoken = 0;
     pr->has_space = false;
-    pr->parsed = cl + 1;
+    pr->parsed = cl;
     pr->request = command;
     pr->reqlen = cmdlen;
     int token_max = PARSER_MAX_TOKENS;
 
     int cmd = -1;
diff --git a/t/proxy.t b/t/proxy.t
index 4a86d831..c9b2f180 100644
--- a/t/proxy.t
+++ b/t/proxy.t
@@ -149,17 +149,18 @@ my $p_sock = $p_srv->sock;
 
 # command endings
 # NOTE: memcached always allowed [\r]\n for single command lines, but payloads
 # (set/etc) require exactly \r\n as termination.
 # doc/protocol.txt has always specified \r\n for command/response.
-# Proxy is more strict than normal server in this case.
+# Note a bug lead me to believe that the proxy was more strict, we accept any
+# \n or \r\n terminated commands.
 {
     my $s = $srv[0]->sock;
     print $s "version\n";
     like(<$s>, qr/VERSION/, "direct server version cmd with just newline");
     print $p_sock "version\n";
-    like(<$p_sock>, qr/CLIENT_ERROR/, "proxy version cmd with just newline");
+    like(<$p_sock>, qr/VERSION/, "proxy version cmd with just newline");
     print $p_sock "version\r\n";
     like(<$p_sock>, qr/VERSION/, "proxy version cmd with full CRLF");
 }
 
 # set through proxy.

Vulnerable code:     int len = pr->reqlen - 2;
    int remain = pr->reqlen - pr->parsed - 2;
    const char *end = pr->request + pr->reqlen - 2;
    const char *s = memchr(command, ' ', cmdlen-2);
        cl = cmdlen - 2;
    pr->parsed = cl + 1;
# Proxy is more strict than normal server in this case.
    like(<$p_sock>, qr/CLIENT_ERROR/, "proxy version cmd with just newline");
Fixed code:     uint32_t endlen; // index to the start of \r\n or \n
    int len = pr->endlen;
    int remain = pr->endlen - pr->parsed;
    const char *end = pr->request + pr->endlen;
    // Commands can end with bare '\n's. Depressingly I intended to be strict
    // with a \r\n requirement but never did this and need backcompat.
    // In this case we _know_ \n is at cmdlen because we can't enter this
    // function otherwise.
    if (cm[cmdlen-2] == '\r') {
        pr->endlen = cmdlen - 2;
    } else {
        pr->endlen = cmdlen - 1;
    }

    const char *s = memchr(command, ' ', pr->endlen);
        cl = pr->endlen;
    pr->parsed = cl;
# Note a bug lead me to believe that the proxy was more strict, we accept any
# \n or \r\n terminated commands.
    like(<$p_sock>, qr/VERSION/, "proxy version cmd with just newline");