Skip to content

Commit

Permalink
proxy: fix buffer overflow with multiget syntax
Browse files Browse the repository at this point in the history
"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.
  • Loading branch information
dormando committed Jul 28, 2023
1 parent bc11696 commit 76a6c36
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 2 deletions.
16 changes: 14 additions & 2 deletions proto_proxy.c
Original file line number Diff line number Diff line change
Expand Up @@ -721,6 +721,12 @@ 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;
Expand Down Expand Up @@ -793,12 +799,18 @@ static void proxy_process_command(conn *c, char *command, size_t cmdlen, bool mu
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;
Expand Down
6 changes: 6 additions & 0 deletions t/proxyunits.t
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,12 @@ sub proxy_test {
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
Expand Down

0 comments on commit 76a6c36

Please sign in to comment.