Skip to content

Strictly check CRLF when parsing querybuf #2872

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: unstable
Choose a base branch
from
+79 −1
Open

Strictly check CRLF when parsing querybuf #2872

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
184a0d5
Strictly check CRLF when parsing querybuf
enjoy-binbin Nov 25, 2025
aff818b
test name
enjoy-binbin Nov 25, 2025
c26acb0
code review
enjoy-binbin Nov 26, 2025
693619b
Merge remote-tracking branch 'upstream/unstable' into check_crlf
enjoy-binbin Nov 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion src/networking.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 "
Expand All @@ -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.
Expand Down Expand Up @@ -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 (newline[1] != '\n') {
return READ_FLAGS_ERROR_INVALID_CRLF;
}

Comment on lines 3493 to 3504
Copy link
Member

@ranshid ranshid Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the first check is for a case when we read up to \r but the \n is not yet read into the buffer right? Maybe we should have made a similar check like we do for inline protocol and just search for the \n and not \r?
not sure if that would simplify things but it seems like it would make the check in line 3490 more accurate...

Copy link
Member Author

@enjoy-binbin enjoy-binbin Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, maybe, there were some history i don't know about, this code is quite critical and i want to change it as little as possible to avoid any unexpected issues, that would be great if have some protocol fuzzer.

5af3020
bf9fd5f

/* 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] == '*');
Expand Down Expand Up @@ -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 (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)) {
Expand Down Expand Up @@ -3627,6 +3641,11 @@ static int parseMultibulk(client *c,
*argv = zrealloc(*argv, sizeof(robj *) * (*argv_len));
}

/* Check that what follows argv is a real \r\n */
if (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. */
Expand Down
1 change: 1 addition & 0 deletions src/server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
56 changes: 56 additions & 0 deletions tests/unit/protocol.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"] {
Copy link
Member Author

@enjoy-binbin enjoy-binbin Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems to be breaking the test *1\r\n$\x00. @ranshid Would you like to take a look?

Copy link
Member

@ranshid ranshid Nov 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The problem is that we allow people like me contribute to this respectable project ;)
The 4th sequence \n will also include carriage return when the socket is not in binary mode.
So the server will actually get:
*1\r\r\n$\x00 which will hit your new validation
So I would suggest to just add:

fconfigure $s -translation binary -encoding binary

after creating the socket in the test.

Copy link
Contributor

@zuiderkwast zuiderkwast Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very clever! I was staring at this and couldn't understand it.

Copy link
Member Author

@enjoy-binbin enjoy-binbin Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow, thanks, a good explanation, got it!

Copy link
Member

@ranshid ranshid Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder of this case should be considered as a missing CRLF error? the bulk does include CRLF...
IMO this does indicate that it might be better to track the '\n' like we do for inline protocol. I understand the need to avoid making changes in this critical path, but we do have time till we release the next version....

Also question: should we backport this fix?

Copy link
Contributor

@zuiderkwast zuiderkwast Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backport? If you want, but it's not urgent. It's not a security issue. It's only about how we handle incorrect clients.

incr c
Expand Down
Loading