Add explicit protocol validation when reading RESP messages #332

lmaas · 2024-04-26T04:00:19Z

Overview

This PR adds explicit syntax checks to ensure RESP packages follow the correct RESP syntax, safeguarding against vulnerabilities that result from malicious RESP packages.

Why is this important?

Not validating the complete RESP package syntax can expose vulnerabilities that allow attackers to divert Garnet's control flow and execute malicious actions. It also makes debugging of unintentionally malformed RESP messages much harder.

Take the following invalid RESP message as an example:

*3\r\n$3\r\nSET\r\n$5\r\nmykey\r\n$\r\n1\r\nPING\r\n

Without validation, this message will cause Tsavorite to crash and terminates the client connection without an explicit error message. With the changes in this PR Garnet will return a proper error message back to the client:

-ERR Protocol Error: Unexpected character '\x0d'

Please note that a parsing error will still close the affected RESP session, because it is impossible to safely recover from a malformed package without discarding the remaining read buffer.

Implementation Details

The PR makes the following changes:

Adds a new RespParsingException exception type that is used to indicate parsing errors and terminate the active RESP session. Note that RespParsingExceptions should only be used when the error is expected to be forwarded to the client.
Adds explicit syntax checks in the RESP parsing logic for "non-essential" RESP tokens such as '$','*',':' and '\r\n'
Adds digit and overflow checks for integer parsing.
Adds length checks for length headers (that could be used to offset into the package).

Known limitations:

RESP error messages do not contain a direct indication where the error occurred
The RESP NULL value ($-1\r\n) requires specific opt-in by the caller of ReadLengthHeader() to reduce the risk of unhandled null values.

Performance Impact

Considering the requirement for additional checks in the RESP parsing logic — specifically bounds checking for digits and overflow validation for integers — we observed a noticeable impact on parsing performance. In certain scenarios, this impact extends to overall end-to-end performance as well. However, I believe this trade-off is justifiable given the significant improvements for security and stability.

Below I am listing the performance degradation I have measured for the different RESP parsing functions:

Method	Runtime	Testcase	Before (Mean)	PR (Mean)
ReadLengthHeader	.NET 6.0	$-1\r\n	1.926 ns	2.462 ns
ReadLengthHeader	.NET 8.0	$-1\r\n	1.888 ns	2.755 ns
ReadLengthHeader	.NET 6.0	$-2147483648\r\n	9.317 ns	NA
ReadLengthHeader	.NET 8.0	$-2147483648\r\n	9.067 ns	NA
ReadLengthHeader	.NET 6.0	$0\r\n	1.665 ns	4.321 ns
ReadLengthHeader	.NET 8.0	$0\r\n	1.944 ns	4.608 ns
ReadIntWithLengthHeader	.NET 6.0	$1\r\n0\r\n	9.153 ns	9.072 ns
ReadLongWithLengthHeader	.NET 6.0	$1\r\n0\r\n	7.778 ns	10.861 ns
ReadULongWithLengthHeader	.NET 6.0	$1\r\n0\r\n	6.918 ns	8.177 ns
ReadIntWithLengthHeader	.NET 8.0	$1\r\n0\r\n	8.359 ns	10.863 ns
ReadLongWithLengthHeader	.NET 8.0	$1\r\n0\r\n	6.878 ns	9.337 ns
ReadULongWithLengthHeader	.NET 8.0	$1\r\n0\r\n	6.347 ns	7.955 ns
ReadIntWithLengthHeader	.NET 6.0	$10\r\n2147483647\r\n	12.330 ns	14.044 ns
ReadULongWithLengthHeader	.NET 6.0	$10\r\n2147483647\r\n	12.203 ns	13.767 ns
ReadIntWithLengthHeader	.NET 8.0	$10\r\n2147483647\r\n	14.085 ns	14.555 ns
ReadULongWithLengthHeader	.NET 8.0	$10\r\n2147483647\r\n	12.070 ns	13.565 ns
ReadIntWithLengthHeader	.NET 6.0	$11\r\n-2147483648\r\n	12.685 ns	14.707 ns
ReadIntWithLengthHeader	.NET 8.0	$11\r\n-2147483648\r\n	14.082 ns	14.326 ns
ReadLongWithLengthHeader	.NET 6.0	$19\r\n(...)807\r\n [26]	18.076 ns	19.628 ns
ReadLongWithLengthHeader	.NET 8.0	$19\r\n(...)807\r\n [26]	19.797 ns	19.582 ns
ReadIntWithLengthHeader	.NET 6.0	$2\r\n-1\r\n	7.495 ns	9.146 ns
ReadLongWithLengthHeader	.NET 6.0	$2\r\n-1\r\n	8.030 ns	8.997 ns
ReadIntWithLengthHeader	.NET 8.0	$2\r\n-1\r\n	6.892 ns	9.010 ns
ReadLongWithLengthHeader	.NET 8.0	$2\r\n-1\r\n	6.861 ns	9.094 ns
ReadLongWithLengthHeader	.NET 6.0	$20\r\n(...)808\r\n [27]	18.307 ns	19.433 ns
ReadLongWithLengthHeader	.NET 8.0	$20\r\n(...)808\r\n [27]	19.846 ns	19.310 ns
ReadULongWithLengthHeader	.NET 6.0	$20\r\n(...)615\r\n [27]	18.118 ns	20.328 ns
ReadULongWithLengthHeader	.NET 8.0	$20\r\n(...)615\r\n [27]	17.530 ns	19.560 ns
ReadLengthHeader	.NET 6.0	$2147483647\r\n	8.712 ns	9.340 ns
ReadLengthHeader	.NET 8.0	$2147483647\r\n	8.245 ns	9.361 ns
ReadInt64	.NET 6.0	:-1\r\n	4.028 ns	5.553 ns
ReadInt64	.NET 8.0	:-1\r\n	3.848 ns	5.782 ns
ReadInt64	.NET 6.0	:-922(...)808\r\n [23]	37.710 ns	15.601 ns
ReadInt64	.NET 8.0	:-922(...)808\r\n [23]	39.468 ns	17.011 ns
ReadInt64	.NET 6.0	:0\r\n	3.305 ns	6.120 ns
ReadInt64	.NET 8.0	:0\r\n	2.636 ns	6.544 ns
ReadInt64	.NET 6.0	:9223(...)807\r\n [22]	36.654 ns	16.752 ns
ReadInt64	.NET 8.0	:9223(...)807\r\n [22]	38.434 ns	16.985 ns

Please note that NA indicates a parsing error being thrown (i.e., length headers must not be negative).

Complete performance before PR

Method	Job	EnvironmentVariables	Runtime	testCase	Mean	Error	StdDev	Median
ReadLengthHeader	.NET 6	Empty	.NET 6.0	$-1\r\n	1.926 ns	0.0027 ns	0.0024 ns	1.925 ns
ReadLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$-1\r\n	1.888 ns	0.0017 ns	0.0016 ns	1.888 ns
ReadLengthHeader	.NET 6	Empty	.NET 6.0	$-2147483648\r\n	9.317 ns	0.1888 ns	0.1766 ns	9.457 ns
ReadLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$-2147483648\r\n	9.067 ns	0.1523 ns	0.1424 ns	9.139 ns
ReadLengthHeader	.NET 6	Empty	.NET 6.0	$0\r\n	1.665 ns	0.0007 ns	0.0006 ns	1.665 ns
ReadLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$0\r\n	1.944 ns	0.0011 ns	0.0009 ns	1.944 ns
ReadIntWithLengthHeader	.NET 6	Empty	.NET 6.0	$1\r\n0\r\n	9.153 ns	0.0058 ns	0.0055 ns	9.154 ns
ReadLongWithLengthHeader	.NET 6	Empty	.NET 6.0	$1\r\n0\r\n	7.778 ns	0.0188 ns	0.0176 ns	7.776 ns
ReadULongWithLengthHeader	.NET 6	Empty	.NET 6.0	$1\r\n0\r\n	6.918 ns	0.0024 ns	0.0023 ns	6.918 ns
ReadIntWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$1\r\n0\r\n	8.359 ns	0.0129 ns	0.0121 ns	8.353 ns
ReadLongWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$1\r\n0\r\n	6.878 ns	0.0120 ns	0.0100 ns	6.877 ns
ReadULongWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$1\r\n0\r\n	6.347 ns	0.0068 ns	0.0057 ns	6.349 ns
ReadIntWithLengthHeader	.NET 6	Empty	.NET 6.0	$10\r\n2147483647\r\n	12.330 ns	0.0299 ns	0.0279 ns	12.324 ns
ReadULongWithLengthHeader	.NET 6	Empty	.NET 6.0	$10\r\n2147483647\r\n	12.203 ns	0.0231 ns	0.0216 ns	12.199 ns
ReadIntWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$10\r\n2147483647\r\n	14.085 ns	0.0601 ns	0.0562 ns	14.090 ns
ReadULongWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$10\r\n2147483647\r\n	12.070 ns	0.0122 ns	0.0114 ns	12.070 ns
ReadIntWithLengthHeader	.NET 6	Empty	.NET 6.0	$11\r\n-2147483648\r\n	12.685 ns	0.0183 ns	0.0153 ns	12.684 ns
ReadIntWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$11\r\n-2147483648\r\n	14.082 ns	0.0636 ns	0.0595 ns	14.081 ns
ReadLongWithLengthHeader	.NET 6	Empty	.NET 6.0	$19\r\n(...)807\r\n [26]	18.076 ns	0.0182 ns	0.0170 ns	18.086 ns
ReadLongWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$19\r\n(...)807\r\n [26]	19.797 ns	0.0231 ns	0.0216 ns	19.796 ns
ReadIntWithLengthHeader	.NET 6	Empty	.NET 6.0	$2\r\n-1\r\n	7.495 ns	0.0027 ns	0.0025 ns	7.495 ns
ReadLongWithLengthHeader	.NET 6	Empty	.NET 6.0	$2\r\n-1\r\n	8.030 ns	0.0088 ns	0.0083 ns	8.031 ns
ReadIntWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$2\r\n-1\r\n	6.892 ns	0.0128 ns	0.0113 ns	6.892 ns
ReadLongWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$2\r\n-1\r\n	6.861 ns	0.0143 ns	0.0134 ns	6.857 ns
ReadLongWithLengthHeader	.NET 6	Empty	.NET 6.0	$20\r\n(...)808\r\n [27]	18.307 ns	0.0043 ns	0.0038 ns	18.307 ns
ReadLongWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$20\r\n(...)808\r\n [27]	19.846 ns	0.0180 ns	0.0159 ns	19.843 ns
ReadULongWithLengthHeader	.NET 6	Empty	.NET 6.0	$20\r\n(...)615\r\n [27]	18.118 ns	0.0130 ns	0.0121 ns	18.118 ns
ReadULongWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$20\r\n(...)615\r\n [27]	17.530 ns	0.0356 ns	0.0333 ns	17.518 ns
ReadLengthHeader	.NET 6	Empty	.NET 6.0	$2147483647\r\n	8.712 ns	0.1950 ns	0.1523 ns	8.712 ns
ReadLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$2147483647\r\n	8.245 ns	0.1511 ns	0.1413 ns	8.297 ns
ReadInt64	.NET 6	Empty	.NET 6.0	:-1\r\n	4.028 ns	0.0222 ns	0.0208 ns	4.030 ns
ReadInt64	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	:-1\r\n	3.848 ns	0.0137 ns	0.0128 ns	3.847 ns
ReadInt64	.NET 6	Empty	.NET 6.0	:-922(...)808\r\n [23]	37.710 ns	0.0223 ns	0.0174 ns	37.705 ns
ReadInt64	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	:-922(...)808\r\n [23]	39.468 ns	0.0391 ns	0.0365 ns	39.474 ns
ReadInt64	.NET 6	Empty	.NET 6.0	:0\r\n	3.305 ns	0.0044 ns	0.0039 ns	3.306 ns
ReadInt64	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	:0\r\n	2.636 ns	0.0094 ns	0.0083 ns	2.635 ns
ReadInt64	.NET 6	Empty	.NET 6.0	:9223(...)807\r\n [22]	36.654 ns	0.0333 ns	0.0312 ns	36.654 ns
ReadInt64	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	:9223(...)807\r\n [22]	38.434 ns	0.0451 ns	0.0400 ns	38.442 ns

Complete performance after PR

Method	Job	EnvironmentVariables	Runtime	testCase	Mean	Error	StdDev
ReadLengthHeader	.NET 6	Empty	.NET 6.0	$-1\r\n	2.462 ns	0.0072 ns	0.0068 ns
ReadLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$-1\r\n	2.755 ns	0.0048 ns	0.0045 ns
ReadLengthHeader	.NET 6	Empty	.NET 6.0	$-2147483648\r\n	NA	NA	NA
ReadLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$-2147483648\r\n	NA	NA	NA
ReadLengthHeader	.NET 6	Empty	.NET 6.0	$0\r\n	4.321 ns	0.0008 ns	0.0008 ns
ReadLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$0\r\n	4.608 ns	0.0075 ns	0.0070 ns
ReadIntWithLengthHeader	.NET 6	Empty	.NET 6.0	$1\r\n0\r\n	9.072 ns	0.0220 ns	0.0195 ns
ReadLongWithLengthHeader	.NET 6	Empty	.NET 6.0	$1\r\n0\r\n	10.861 ns	0.0052 ns	0.0046 ns
ReadULongWithLengthHeader	.NET 6	Empty	.NET 6.0	$1\r\n0\r\n	8.177 ns	0.0085 ns	0.0076 ns
ReadIntWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$1\r\n0\r\n	10.863 ns	0.0114 ns	0.0095 ns
ReadLongWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$1\r\n0\r\n	9.337 ns	0.0010 ns	0.0009 ns
ReadULongWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$1\r\n0\r\n	7.955 ns	0.0092 ns	0.0081 ns
ReadIntWithLengthHeader	.NET 6	Empty	.NET 6.0	$10\r\n2147483647\r\n	14.044 ns	0.0438 ns	0.0410 ns
ReadULongWithLengthHeader	.NET 6	Empty	.NET 6.0	$10\r\n2147483647\r\n	13.767 ns	0.0366 ns	0.0324 ns
ReadIntWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$10\r\n2147483647\r\n	14.555 ns	0.0181 ns	0.0169 ns
ReadULongWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$10\r\n2147483647\r\n	13.565 ns	0.1113 ns	0.1041 ns
ReadIntWithLengthHeader	.NET 6	Empty	.NET 6.0	$11\r\n-2147483648\r\n	14.707 ns	0.3044 ns	0.2848 ns
ReadIntWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$11\r\n-2147483648\r\n	14.326 ns	0.0291 ns	0.0272 ns
ReadLongWithLengthHeader	.NET 6	Empty	.NET 6.0	$19\r\n(...)807\r\n [26]	19.628 ns	0.0130 ns	0.0116 ns
ReadLongWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$19\r\n(...)807\r\n [26]	19.582 ns	0.0124 ns	0.0110 ns
ReadIntWithLengthHeader	.NET 6	Empty	.NET 6.0	$2\r\n-1\r\n	9.146 ns	0.0199 ns	0.0186 ns
ReadLongWithLengthHeader	.NET 6	Empty	.NET 6.0	$2\r\n-1\r\n	8.997 ns	0.0077 ns	0.0072 ns
ReadIntWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$2\r\n-1\r\n	9.010 ns	0.0022 ns	0.0018 ns
ReadLongWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$2\r\n-1\r\n	9.094 ns	0.0020 ns	0.0019 ns
ReadLongWithLengthHeader	.NET 6	Empty	.NET 6.0	$20\r\n(...)808\r\n [27]	19.433 ns	0.0188 ns	0.0167 ns
ReadLongWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$20\r\n(...)808\r\n [27]	19.310 ns	0.0077 ns	0.0069 ns
ReadULongWithLengthHeader	.NET 6	Empty	.NET 6.0	$20\r\n(...)615\r\n [27]	20.328 ns	0.1163 ns	0.1031 ns
ReadULongWithLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$20\r\n(...)615\r\n [27]	19.560 ns	0.0235 ns	0.0219 ns
ReadLengthHeader	.NET 6	Empty	.NET 6.0	$2147483647\r\n	9.340 ns	0.0372 ns	0.0330 ns
ReadLengthHeader	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	$2147483647\r\n	9.361 ns	0.1513 ns	0.1415 ns
ReadInt64	.NET 6	Empty	.NET 6.0	:-1\r\n	5.553 ns	0.0029 ns	0.0025 ns
ReadInt64	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	:-1\r\n	5.782 ns	0.0045 ns	0.0043 ns
ReadInt64	.NET 6	Empty	.NET 6.0	:-922(...)808\r\n [23]	15.601 ns	0.1549 ns	0.1209 ns
ReadInt64	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	:-922(...)808\r\n [23]	17.011 ns	0.1305 ns	0.1221 ns
ReadInt64	.NET 6	Empty	.NET 6.0	:0\r\n	6.120 ns	0.0109 ns	0.0102 ns
ReadInt64	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	:0\r\n	6.544 ns	0.0035 ns	0.0033 ns
ReadInt64	.NET 6	Empty	.NET 6.0	:9223(...)807\r\n [22]	16.752 ns	0.0662 ns	0.0620 ns
ReadInt64	.NET 8	DOTNET_TieredPGO=0	.NET 8.0	:9223(...)807\r\n [22]	16.985 ns	0.0037 ns	0.0033 ns

mgravell

only got about half-way through so far, but looks promising and much safer; some notes included

libs/common/Parsing/RespParsingException.cs

libs/common/RespReadUtils.cs

libs/server/Resp/RespServerSession.cs

mgravell · 2024-04-27T07:15:57Z

On the magic numbers vs readability topic vs performance topic; the benchmark shows that on net8+ the inline read looks fine for perf, but if you wanted to generalize without compromising, a suggestion: static readonly uint MeaningfulNameHere = Utils.CpuEndianUInt32("$1\r\n"u8); (etc for different widths) where the method would assert the length, do a coerced read, and reverse endianness if needed, without using random numbers in the code.

PaulusParssinen

LGTM A lot of general goodness done here and I'm glad that people are contributing their BDN benchmarks to the repo!

libs/common/RespReadUtils.cs

test/Garnet.test/RespTests.cs

…hecked-resp

lmaas added 3 commits April 25, 2024 18:25

Add explicit protocol validation when reading RESP messages

f6f8e0b

Code cleanup

c6287d8

Merge remote-tracking branch 'origin/main' into checked-resp

de08fc2

lmaas marked this pull request as draft April 26, 2024 04:00

lmaas requested a review from badrishc April 26, 2024 04:01

Add unit tests for RespReadUtils

f9a4f65

lmaas marked this pull request as ready for review April 26, 2024 05:27

lmaas linked an issue Apr 26, 2024 that may be closed by this pull request

Debug.Asserts don't detect protocol violations reliably #199

Closed

lmaas requested a review from mgravell April 26, 2024 05:36

mgravell reviewed Apr 26, 2024

View reviewed changes

PaulusParssinen reviewed Apr 26, 2024

View reviewed changes

libs/server/Resp/RespServerSession.cs Outdated Show resolved Hide resolved

lmaas added 6 commits April 26, 2024 13:02

Add length header fast-path for booleans

1ec9efb

Improve readability of RespParsingExceptions

99dd598

Add context to resp parsing integer overflow exceptions

b946569

Code cleanup

2086e1b

Misc. code cleanup for RESP read utils

1e02c4b

Merge branch 'main' into checked-resp

a9e5174

lmaas requested review from PaulusParssinen and mgravell April 27, 2024 01:05

PaulusParssinen approved these changes Apr 27, 2024

View reviewed changes

libs/common/RespReadUtils.cs Show resolved Hide resolved

Merge branch 'main' into checked-resp

abbb59e

vazois reviewed Apr 29, 2024

View reviewed changes

lmaas and others added 2 commits April 30, 2024 14:34

Allow empty string values for INCRBY and SET commands.

28f7cd1

Merge branch 'main' into checked-resp

1060dde

badrishc approved these changes May 1, 2024

View reviewed changes

lmaas added 4 commits May 1, 2024 00:02

Allow empty keys in RESP messages.

34a0b31

Merge branch 'checked-resp' of https://github.com/lmaas/garnet into c…

a099141

…hecked-resp

Merge remote-tracking branch 'origin/main' into checked-resp

a3eb5cb

Code cleanup

89b7c4f

lmaas added 2 commits May 1, 2024 14:47

Merge branch 'main' into checked-resp

8ba839f

Make RESP null parsing opt-in

0bf437d

lmaas added the parser label May 2, 2024

lmaas merged commit 3a4b349 into microsoft:main May 2, 2024
23 checks passed

lmaas deleted the checked-resp branch May 2, 2024 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add explicit protocol validation when reading RESP messages #332

Add explicit protocol validation when reading RESP messages #332

lmaas commented Apr 26, 2024 •

edited

mgravell left a comment

mgravell commented Apr 27, 2024 via email

PaulusParssinen left a comment

Add explicit protocol validation when reading RESP messages #332

Add explicit protocol validation when reading RESP messages #332

Conversation

lmaas commented Apr 26, 2024 • edited

Overview

Why is this important?

Implementation Details

Performance Impact

mgravell left a comment

Choose a reason for hiding this comment

mgravell commented Apr 27, 2024 via email

PaulusParssinen left a comment

Choose a reason for hiding this comment

lmaas commented Apr 26, 2024 •

edited