diff --git a/src/networking.c b/src/networking.c index 3f8bff61ee..277e1690dc 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2996,6 +2996,9 @@ void handleParseError(client *c) { } else if (flags & READ_FLAGS_ERROR_UNBALANCED_QUOTES) { addReplyError(c, "Protocol error: unbalanced quotes in request"); setProtocolError("unbalanced quotes in inline request", c); + } else if (flags & READ_FLAGS_ERROR_INVALID_CRLF) { + addReplyError(c, "Protocol error: invalid CRLF in request"); + setProtocolError("invalid CRLF in request", c); } else if (flags & READ_FLAGS_ERROR_UNEXPECTED_INLINE_FROM_REPLICATED_CLIENT) { if (getClientType(c) == CLIENT_TYPE_SLOT_IMPORT) { serverLog(LL_WARNING, "WARNING: Receiving inline protocol from slot import, import stream corruption? Closing the " @@ -3016,7 +3019,8 @@ int isParsingError(client *c) { READ_FLAGS_ERROR_INVALID_MULTIBULK_LEN | READ_FLAGS_ERROR_UNAUTHENTICATED_MULTIBULK_LEN | READ_FLAGS_ERROR_UNAUTHENTICATED_BULK_LEN | READ_FLAGS_ERROR_MBULK_INVALID_BULK_LEN | READ_FLAGS_ERROR_BIG_BULK_COUNT | READ_FLAGS_ERROR_MBULK_UNEXPECTED_CHARACTER | - READ_FLAGS_ERROR_UNEXPECTED_INLINE_FROM_REPLICATED_CLIENT | READ_FLAGS_ERROR_UNBALANCED_QUOTES); + READ_FLAGS_ERROR_UNEXPECTED_INLINE_FROM_REPLICATED_CLIENT | READ_FLAGS_ERROR_UNBALANCED_QUOTES | + READ_FLAGS_ERROR_INVALID_CRLF); } /* This function is called after the query-buffer was parsed. @@ -3493,6 +3497,11 @@ static int parseMultibulk(client *c, /* Buffer should also contain \n */ if (newline - (c->querybuf + c->qb_pos) > (ssize_t)(sdslen(c->querybuf) - c->qb_pos - 2)) return 0; + /* Check that what follows \r is a real \n */ + if (unlikely(newline[1] != '\n')) { + return READ_FLAGS_ERROR_INVALID_CRLF; + } + /* We know for sure there is a whole line since newline != NULL, * so go ahead and find out the multi bulk length. */ serverAssertWithInfo(c, NULL, c->querybuf[c->qb_pos] == '*'); @@ -3572,6 +3581,11 @@ static int parseMultibulk(client *c, return READ_FLAGS_ERROR_MBULK_UNEXPECTED_CHARACTER; } + /* Check that what follows \r is a real \n */ + if (unlikely(newline[1] != '\n')) { + return READ_FLAGS_ERROR_INVALID_CRLF; + } + size_t bulklen_slen = newline - (c->querybuf + c->qb_pos + 1); ok = string2ll(c->querybuf + c->qb_pos + 1, bulklen_slen, &ll); if (!ok || ll < 0 || (!(is_replicated) && ll > server.proto_max_bulk_len)) { @@ -3627,6 +3641,12 @@ static int parseMultibulk(client *c, *argv = zrealloc(*argv, sizeof(robj *) * (*argv_len)); } + /* Check that what follows argv is a real \r\n */ + if (unlikely(c->querybuf[c->qb_pos + c->bulklen] != '\r' || + c->querybuf[c->qb_pos + c->bulklen + 1] != '\n')) { + return READ_FLAGS_ERROR_INVALID_CRLF; + } + /* Optimization: if a non-replicated client's buffer contains JUST our bulk element * instead of creating a new object by *copying* the sds we * just use the current sds string. */ diff --git a/src/server.h b/src/server.h index 55424b1da3..5821f66751 100644 --- a/src/server.h +++ b/src/server.h @@ -2783,6 +2783,7 @@ void dictVanillaFree(void *val); #define READ_FLAGS_NO_KEYS (1 << 19) #define READ_FLAGS_CROSSSLOT (1 << 20) #define READ_FLAGS_PREFETCHED (1 << 21) +#define READ_FLAGS_ERROR_INVALID_CRLF (1 << 22) /* Write flags for various write errors and states */ #define WRITE_FLAGS_WRITE_ERROR (1 << 0) diff --git a/tests/unit/protocol.tcl b/tests/unit/protocol.tcl index c4f43093e1..eaaf21bd5a 100644 --- a/tests/unit/protocol.tcl +++ b/tests/unit/protocol.tcl @@ -99,6 +99,62 @@ start_server {tags {"protocol network"}} { assert_error "*unbalanced*" {r read} } + test "Check CRLF when parsing the querybuf" { + # Command) SET key value + # RESP) *3\r\n$3\r\nSET\r\n$3\r\nkey\r\n$5\r\nvalue\r\n + # We need to strictly check these \r\n characters. + set proto "*3\r\n\$3\r\nSET\r\n\$3\r\nkey\r\n\$5\r\nvalue\r\n" + reconnect; r write $proto; r flush; assert_equal "OK" [r read] + + # Check if multibulklen `*3\r\n` is followed by `\r\n` + set proto1 "*3x\n\$3\r\nSET\r\n\$3\r\nkey\r\n\$5\r\nvalue\r\n" + set proto2 "*3\rx\$3\r\nSET\r\n\$3\r\nkey\r\n\$5\r\nvalue\r\n" + set proto3 "*3xx\$3\r\nSET\r\n\$3\r\nkey\r\n\$5\r\nvalue\r\n" + r write $proto1; r flush; assert_error "*invalid multibulk length*" {r read}; reconnect + r write $proto2; r flush; assert_error "*invalid CRLF in request*" {r read}; reconnect + r write $proto3; r flush; assert_error "*invalid multibulk length*" {r read}; reconnect + + # Check if bulklen `$3\r\n` is followed by `\r\n` + set proto1 "*3\r\n\$3x\nSET\r\n\$3\r\nkey\r\n\$5\r\nvalue\r\n" + set proto2 "*3\r\n\$3\rxSET\r\n\$3\r\nkey\r\n\$5\r\nvalue\r\n" + set proto3 "*3\r\n\$3xxSET\r\n\$3\r\nkey\r\n\$5\r\nvalue\r\n" + r write $proto1; r flush; assert_error "*invalid bulk length*" {r read}; reconnect + r write $proto2; r flush; assert_error "*invalid CRLF in request*" {r read}; reconnect + r write $proto3; r flush; assert_error "*invalid bulk length*" {r read}; reconnect + + # Check if `SET\r\n` is followed by `\r\n` + set proto1 "*3\r\n\$3\r\nSET\rx\$3\r\nkey\r\n\$5\r\nvalue\r\n" + set proto2 "*3\r\n\$3\r\nSETx\n\$3\r\nkey\r\n\$5\r\nvalue\r\n" + set proto3 "*3\r\n\$3\r\nSETxx\$3\r\nkey\r\n\$5\r\nvalue\r\n" + r write $proto1; r flush; assert_error "*invalid CRLF in request*" {r read}; reconnect + r write $proto2; r flush; assert_error "*invalid CRLF in request*" {r read}; reconnect + r write $proto3; r flush; assert_error "*invalid CRLF in request*" {r read}; reconnect + + # Check if `key\r\n` is followed by `\r\n` + set proto1 "*3\r\n\$3\r\nSET\r\n\$3\r\nkeyx\n\$5\r\nvalue\r\n" + set proto2 "*3\r\n\$3\r\nSET\r\n\$3\r\nkey\rx\$5\r\nvalue\r\n" + set proto3 "*3\r\n\$3\r\nSET\r\n\$3\r\nkeyxx\$5\r\nvalue\r\n" + r write $proto1; r flush; assert_error "*invalid CRLF in request*" {r read}; reconnect + r write $proto2; r flush; assert_error "*invalid CRLF in request*" {r read}; reconnect + r write $proto3; r flush; assert_error "*invalid CRLF in request*" {r read}; reconnect + + # Check if bulklen `$5\r\n` is followed by `\r\n` + set proto1 "*3\r\n\$3\r\nSET\r\n\$3\r\nkey\r\n\$5x\nvalue\r\n" + set proto2 "*3\r\n\$3\r\nSET\r\n\$3\r\nkey\r\n\$5\rxvalue\r\n" + set proto3 "*3\r\n\$3\r\nSET\r\n\$3\r\nkey\r\n\$5xxvalue\r\n" + r write $proto1; r flush; assert_error "*invalid bulk length*" {r read}; reconnect + r write $proto2; r flush; assert_error "*invalid CRLF in request*" {r read}; reconnect + r write $proto3; r flush; assert_error "*invalid bulk length*" {r read}; reconnect + + # Check if `value\r\n` is followed by `\r\n` + set proto1 "*3\r\n\$3\r\nSET\r\n\$3\r\nkey\r\n\$5\r\nvaluex\n" + set proto2 "*3\r\n\$3\r\nSET\r\n\$3\r\nkey\r\n\$5\r\nvalue\rx" + set proto3 "*3\r\n\$3\r\nSET\r\n\$3\r\nkey\r\n\$5\r\nvaluexx" + r write $proto1; r flush; assert_error "*invalid CRLF in request*" {r read}; reconnect + r write $proto2; r flush; assert_error "*invalid CRLF in request*" {r read}; reconnect + r write $proto3; r flush; assert_error "*invalid CRLF in request*" {r read}; reconnect + } + set c 0 foreach seq [list "\x00" "*\x00" "$\x00" "*1\r\n$\x00"] { incr c @@ -108,6 +164,7 @@ start_server {tags {"protocol network"}} { } else { set s [socket [srv 0 host] [srv 0 port]] } + fconfigure $s -translation binary puts -nonewline $s $seq # PROTO_INLINE_MAX_SIZE is hardcoded in Valkey code to 64K. doing the same here # since we would like to validate it is enforced.