Strictly check CRLF when parsing querybuf #2872
Conversation
Currently, when parsing querybuf, we are not checking for CRLF, instead we assume the last two characters are CRLF by default, as shown in the following example: ``` telnet 127.0.0.1 6379 Trying 127.0.0.1... Connected to 127.0.0.1. Escape character is '^]'. *3 $3 set $3 key $5 value12 +OK get key $5 value *3 $3 set $3 key $5 value12345 +OK -ERR unknown command '345', with args beginning with: ``` This should actually be considered a protocol error. When a bug occurs in the client-side implementation, we may execute incorrect requests (writing incorrect data is the most serious of these). Signed-off-by: Binbin <[email protected]>
enjoy-binbin
added
the
run-extra-tests
Signed-off-by: Binbin <[email protected]>
| } | ||
|
|
||
| set c 0 | ||
| foreach seq [list "\x00" "*\x00" "$\x00" "*1\r\n$\x00"] { |
There was a problem hiding this comment.
This change seems to be breaking the test *1\r\n$\x00. @ranshid Would you like to take a look?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Very clever! I was staring at this and couldn't understand it.
There was a problem hiding this comment.
wow, thanks, a good explanation, got it!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Backport? If you want, but it's not urgent. It's not a security issue. It's only about how we handle incorrect clients.
| /* 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; | ||
| } | ||
|
|
There was a problem hiding this comment.
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...
| } | ||
|
|
||
| set c 0 | ||
| foreach seq [list "\x00" "*\x00" "$\x00" "*1\r\n$\x00"] { |
There was a problem hiding this comment.
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.
Signed-off-by: Binbin <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #2872 +/- ##
============================================
- Coverage 72.45% 72.43% -0.02%
============================================
Files 128 128
Lines 70485 70491 +6
============================================
- Hits 51068 51063 -5
- Misses 19417 19428 +11
🚀 New features to boost your workflow:
|
|
@enjoy-binbin any idea why the new fails? |
The failures on fedora rawhide is this: |
@enjoy-binbin Can we just merge unstable so we can have a clean run? probably will be fine, but just to play on the safe side |
Signed-off-by: Binbin <[email protected]>
3 participants
Currently, when parsing querybuf, we are not checking for CRLF,
instead we assume the last two characters are CRLF by default,
as shown in the following example:
This should actually be considered a protocol error. When a bug
occurs in the client-side implementation, we may execute incorrect
requests (writing incorrect data is the most serious of these).