Add TDS token data length bounds checks#4340
Conversation
Prevent unbounded memory allocation from malicious TDS servers by adding runtime bounds checks on token data length fields before heap allocation. Changes: - Add MaxTokenDataLength (1 MB) and MaxPromoteTransactionLength (64 KB) constants to TdsEnums.cs - Add bounds checks in TryProcessFeatureExtAck, TryProcessSessionState, TryProcessFedAuthInfo, ENV_PROMOTETRANSACTION, and TryReadSqlValueInternal - Add bounds check in SqlConnectionInternal SRECOVERY secondary parse - Promote Debug.Assert to runtime checks in TryProcessReturnValue and TryReadSqlValueInternal (binary types), remove now-redundant asserts - Replace conditional heap allocation in TryReadSqlDateTime with fixed stackalloc guarded by a length check Tests: - FeatureExtAckBoundsTests: oversized FeatureExtAck data length, positive boundary test - TdsTokenBoundsTests: oversized SessionState total/inner length, malformed SRECOVERY inner state, oversized FedAuthInfo token length
Inject a malicious PromoteTransaction environment change token (type 15) into the login response with a fraudulently large int32 newLength field. The bounds check in TryProcessEnvChange rejects it before allocation. No simulated server extension needed — env change tokens are part of the standard login response and can be injected via OnAuthenticationResponseCompleted.
The bounds check on valLen must not apply to PLP (Partial Length Prefixed) types because TryReadPlpLength can return chunk lengths that exceed int.MaxValue for legitimate streaming data (e.g., NVARCHAR(MAX) output parameters). PLP types always use int.MaxValue as a sentinel for 'read all data', so the unchecked cast was safe. Restructure the if/else to check IsPlp first, applying the bounds check only to non-PLP return values where an oversized length indicates a corrupted TDS stream.
- Add OnSQLBatchCompleted hook to GenericTdsServer for post-login TDS token injection testing - Add BatchResponse_DateTime test verifying TryReadSqlDateTime rejects oversized length (11 bytes for TimeN, max is 10) - Fix test isolation: all login-phase tests now clear their OnAuthenticationResponseCompleted hook in finally blocks - Add DebugAssertSuppressor helper for clean test teardown after intentional stream corruption
- Test 8: BatchResponse_ReturnValue_OversizedLength injects an IMAGE RETURNVALUE token with data length -1 (huge when cast to ulong), verifying TryProcessReturnValue rejects non-PLP values > int.MaxValue - Test 9: BatchResponse_ReturnValue_PlpUnknownLength sends a valid PLP VARBINARY(MAX) RETURNVALUE with the unknown-length sentinel (0xFFFFFFFFFFFFFFFE) + immediate terminator, confirming the PLP path is not rejected by the bounds check
There was a problem hiding this comment.
Pull request overview
This PR hardens the TDS parser against pre-auth (and post-auth) denial-of-service vectors by adding upper bounds checks on token/data lengths read from the wire, preventing unbounded allocations when a server spoofs lengths.
Changes:
- Added parser-side bounds checks for multiple TDS tokens/fields (SessionState, FedAuthInfo, FeatureExtAck, EnvChange PromoteTransaction, DateTime payloads, ReturnValue non‑PLP, Vector).
- Introduced new protocol max constants in
TdsEnumsand expanded simulated-server infrastructure to inject malicious tokens in both login and batch responses. - Added new simulated-server unit tests covering the new rejection paths and a PLP regression guard.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs | New simulated-server tests covering several oversized-length scenarios (login + post-login injection). |
| src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtAckBoundsTests.cs | New tests for FeatureExtAck oversized length handling and boundary acceptance. |
| src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTdsServer.cs | Adds a post-login SQL batch hook to allow response token injection in tests. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs | Implements the core bounds checks in the TDS parser and fixes PLP/non‑PLP handling in ReturnValue. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsEnums.cs | Adds new max-length constants used by the parser checks. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs | Adds bounds validation for SRECOVERY inner state option parsing in FeatureExtAck handling. |
paulmedynski
changed the title
- ReturnValue: report actual valLen via unchecked((int)valLen) instead of int.MaxValue for more actionable error diagnostics - TryReadSqlDateTime: use TdsEnums.MaxDateTimeLength constant instead of local const for consistency with other bounds checks - SqlConnectionInternal: use ParsingErrorLength for session-recovery check to include the offending length in the error - TdsEnums: broaden MaxTokenDataLength comment scope (not login-only), add MaxDateTimeLength constant - FeatureExtAckBoundsTests: rewrite boundary test to inject token at exactly MaxTokenDataLength and assert bounds check does not fire - TdsTokenBoundsTests: fix DateTime test comment (heap, not stack)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4340 +/- ##
==========================================
- Coverage 66.69% 64.74% -1.95%
==========================================
Files 284 279 -5
Lines 43238 66097 +22859
==========================================
+ Hits 28836 42796 +13960
- Misses 14402 23301 +8899
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The binary bounds check in TryReadSqlValueInternal is only reachable through the sql_variant deserialization path (TryReadSqlVariant). For non-variant binary columns, TryReadSqlValue handles them directly without calling TryReadSqlValueInternal. This test injects a sql_variant column containing a BigVarBinary value whose inner data length (8001) exceeds MAXSIZE (8000), verifying the bounds check fires and prevents unbounded heap allocation. Addresses the 2 uncovered lines flagged by Codecov in PR #4340.
- TryProcessFeatureExtAck: store (int)dataLen in local variable after bounds check to avoid repeated uint-to-int casts - FeatureExtAckBoundsTests: wrap OversizedDataLength test body in try/finally to clear OnAuthenticationResponseCompleted hook, preventing state leakage into other simulated-server tests sharing the same fixture
paulmedynski
left a comment
paulmedynski
left a comment
There was a problem hiding this comment.
Self-review: Test coverage annotations
Each bounds check added to driver code has been annotated with the specific test(s) that exercise it. Summary:
| Driver change | Test(s) |
|---|---|
TdsEnums.cs — MaxTokenDataLength, MaxPromoteTransactionLength, MaxDateTimeLength |
All 12 tests validate these thresholds |
TdsParser.cs — PromoteTransaction bounds check (L3351) |
EnvChange_PromoteTransaction_OversizedLength_ThrowsParsingError, BatchResponse_PromoteTransaction_OversizedLength_ThrowsParsingError |
TdsParser.cs — FeatureExtAck bounds check (L3853) |
FeatureExtAck_OversizedDataLength_ThrowsParsingError, FeatureExtAck_MaxAllowedDataLength_DoesNotThrow |
TdsParser.cs — SessionState total length (L4178) |
SessionState_OversizedTotalLength_ThrowsParsingError |
TdsParser.cs — SessionState inner option (L4234) |
SessionState_OversizedInnerOptionLength_ThrowsParsingError |
TdsParser.cs — FedAuthInfo token length (L4455) |
FedAuthInfo_OversizedTokenLength_ThrowsParsingError |
TdsParser.cs — ReturnValue non-PLP length (L4936) |
BatchResponse_ReturnValue_OversizedLength_ThrowsParsingError, BatchResponse_ReturnValue_PlpUnknownLength_Succeeds |
TdsParser.cs — DateTime length (L7143) |
BatchResponse_DateTime_OversizedLength_ThrowsParsingError |
TdsParser.cs — Binary/Vector sql_variant (L7405) |
BatchResponse_SqlVariantBinary_OversizedLength_ThrowsParsingError |
SqlConnectionInternal.cs — SRECOVERY inner state (L1349) |
SRecovery_MalformedInnerStateLength_ThrowsParsingError |
All 12 tests pass in ~19s.
| if (stateLen < 0 || stateLen > TdsEnums.MaxTokenDataLength) | ||
| { | ||
| throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, stateLen); | ||
| } |
There was a problem hiding this comment.
Test coverage: TdsTokenBoundsTests.SessionState_OversizedInnerOptionLength_ThrowsParsingError — injects a SessionState token with a valid outer length but an inner state option whose 0xFF + DWORD encoded length exceeds MaxTokenDataLength.
| { | ||
| throw SQL.ParsingErrorLength(ParsingErrorState.SessionStateLengthTooShort, length); | ||
| } | ||
| if (length > TdsEnums.MaxTokenDataLength) |
There was a problem hiding this comment.
Test coverage: TdsTokenBoundsTests.SessionState_OversizedTotalLength_ThrowsParsingError — injects a SessionState token (0xE4) whose total length field exceeds MaxTokenDataLength.
| if (env._newLength < 0 || env._newLength > TdsEnums.MaxPromoteTransactionLength) | ||
| { | ||
| throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, env._newLength); | ||
| } |
There was a problem hiding this comment.
Test coverage: TdsTokenBoundsTests.EnvChange_PromoteTransaction_OversizedLength_ThrowsParsingError and TdsTokenBoundsTests.BatchResponse_PromoteTransaction_OversizedLength_ThrowsParsingError — the first injects during login, the second during command execution via OnSQLBatchCompleted.
| if (dataLen > (uint)TdsEnums.MaxTokenDataLength) | ||
| { | ||
| throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, (int)dataLen); | ||
| } | ||
| int dataLength = (int)dataLen; | ||
| byte[] data = new byte[dataLength]; | ||
| if (dataLength > 0) | ||
| { |
There was a problem hiding this comment.
Test coverage: FeatureExtAckBoundsTests.FeatureExtAck_OversizedDataLength_ThrowsParsingError (injects dataLen = MaxTokenDataLength + 1, asserts parsing error state 18) and FeatureExtAckBoundsTests.FeatureExtAck_MaxAllowedDataLength_DoesNotThrow (boundary test confirming no off-by-one).
| if (tokenLen > TdsEnums.MaxTokenDataLength) | ||
| { | ||
| throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, tokenLen); | ||
| } |
There was a problem hiding this comment.
Test coverage: TdsTokenBoundsTests.FedAuthInfo_OversizedTokenLength_ThrowsParsingError — injects a FedAuthInfo token (0xEE) with tokenLen exceeding MaxTokenDataLength. The token type is dispatched unconditionally by TryRun.
| int intlen; | ||
|
|
||
| if (rec.metaType.IsPlp) | ||
| { | ||
| intlen = int.MaxValue; // If plp data, read it all | ||
| } | ||
| else if (valLen > (ulong)int.MaxValue) | ||
| { | ||
| throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, unchecked((int)valLen)); | ||
| } | ||
| else | ||
| { | ||
| intlen = (int)valLen; |
There was a problem hiding this comment.
Test coverage: TdsTokenBoundsTests.BatchResponse_ReturnValue_OversizedLength_ThrowsParsingError (injects IMAGE return value with data length -1, which as ulong exceeds int.MaxValue) and TdsTokenBoundsTests.BatchResponse_ReturnValue_PlpUnknownLength_Succeeds (regression guard: PLP unknown-length sentinel 0xFFFFFFFFFFFFFFFE must NOT trigger the check).
| // DateTimeOffset is the largest datetime type at 10 bytes (5 time + 3 date + 2 offset). | ||
| // Reject anything larger to prevent heap allocation from spoofed metadata. | ||
| if (length < 0 || length > TdsEnums.MaxDateTimeLength) | ||
| { | ||
| throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, length); | ||
| } | ||
|
|
||
| Span<byte> datetimeBuffer = stackalloc byte[TdsEnums.MaxDateTimeLength]; | ||
|
|
||
| TdsOperationStatus result = stateObj.TryReadByteArray(datetimeBuffer, length); | ||
| if (result != TdsOperationStatus.Done) |
There was a problem hiding this comment.
Test coverage: TdsTokenBoundsTests.BatchResponse_DateTime_OversizedLength_ThrowsParsingError — injects a TIME column whose data length byte is 11 (exceeding MaxDateTimeLength = 10). Also validates the elimination of the conditional heap allocation: since all valid datetime types fit in ≤ 10 bytes, this is now an unconditional stackalloc.
| if (len < 0 || len > data.Length - i) | ||
| { | ||
| throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, len); | ||
| } | ||
|
|
There was a problem hiding this comment.
Test coverage: TdsTokenBoundsTests.SRecovery_MalformedInnerStateLength_ThrowsParsingError — injects a FeatureExtAck with SessionRecovery feature (ID=1) whose inner state data contains an option claiming length 999 while only 1 byte follows. The bounds check len < 0 || len > data.Length - i catches this overflow.
| // Note: Better not come here with plp data!! | ||
| Debug.Assert(length <= TdsEnums.MAXSIZE); | ||
| if (length < 0 || length > TdsEnums.MAXSIZE) | ||
| { | ||
| throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, length); |
There was a problem hiding this comment.
Test coverage: TdsTokenBoundsTests.BatchResponse_SqlVariantBinary_OversizedLength_ThrowsParsingError — injects a sql_variant ROW containing a BigVarBinary inner type whose computed lenData (8001) exceeds MAXSIZE (8000). This is the only path to reach this check since normal binary columns go through TryReadSqlValue directly.
| // Maximum allowed data length for token payloads (feature ext ack, | ||
| // session state, fedauth info). Prevents a malicious server from causing | ||
| // unbounded memory allocation via spoofed token length fields. | ||
| internal const int MaxTokenDataLength = 1 << 20; // 1 MB | ||
|
|
||
| // Maximum allowed data length for a DTC promote transaction propagation token. | ||
| internal const int MaxPromoteTransactionLength = 1 << 16; // 64 KB | ||
|
|
||
| // Maximum valid wire size for datetime types (DateTimeOffset = 5 time + 3 date + 2 offset). | ||
| internal const int MaxDateTimeLength = 10; | ||
|
|
||
| // Severity 0 - 10 indicates informational (non-error) messages |
There was a problem hiding this comment.
Test coverage: These constants are validated by all 12 unit tests across FeatureExtAckBoundsTests (2 tests) and TdsTokenBoundsTests (10 tests). Each test verifies that crossing the respective threshold triggers ParsingErrorState.CorruptedTdsStream. The boundary test (MaxAllowedDataLength_DoesNotThrow) also confirms no off-by-one in the MaxTokenDataLength constant.
Summary
Adds bounds checks on data lengths read from the TDS wire to prevent unbounded
byte[]allocation from a malicious server.All checks throw
SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, ...)when a spoofed length exceeds protocol-reasonable maximums.Changes
TdsParser.csL4180TryProcessSessionState— total length checktokenLen > MaxTokenDataLength(1 MB)SessionState_OversizedTotalLengthTdsParser.csL4233TryProcessSessionState— inner option length checkstateLen > MaxTokenDataLengthSessionState_OversizedInnerOptionLengthTdsParser.csL4454TryProcessFedAuthInfo— token length checktokenLen > MaxTokenDataLengthFedAuthInfo_OversizedTokenLengthTdsParser.csL3351TryProcessEnvChange— PromoteTransaction newLength checknewLength > MaxPromoteTransactionLength(64 KB)EnvChange_PromoteTransaction_OversizedLength(login),BatchResponse_PromoteTransaction_OversizedLength(post-login)TdsParser.csL3853TryProcessFeatureExtAck— data length checkdataLen > MaxTokenDataLengthFeatureExtAck_OversizedDataLength,FeatureExtAck_MaxAllowedDataLength_DoesNotThrowTdsParser.csL7144TryReadSqlDateTime— data length checklength > MaxDateTimeLength(10)BatchResponse_DateTime_OversizedLengthTdsParser.csL4937-4948TryProcessReturnValue— non-PLP length checkIsPlptypes skip bounds check; non-PLP rejectsvalLen > int.MaxValueBatchResponse_ReturnValue_OversizedLength,BatchResponse_ReturnValue_PlpUnknownLengthTdsParser.csL7405TryReadSqlValueInternal— binary/vector length checklength > MAXSIZE(8000) for binary types in sql_variant pathBatchResponse_SqlVariantBinary_OversizedLengthSqlConnectionInternal.csL1349OnFeatureExtAck— SRECOVERY inner state lengthSRecovery_MalformedInnerStateLengthTdsEnums.csL90-96MaxTokenDataLength(1 MB),MaxPromoteTransactionLength(64 KB),MaxDateTimeLength(10)Impact analysis
Group 1: New upper-bound constraints
These add hard limits where previously the parser would attempt to allocate whatever the server claimed. A conforming SQL Server will never exceed any of these limits, but a misbehaving proxy, network appliance, or third-party TDS implementation could.
dataLentokenLennewLengthlengthRisk to existing apps: Effectively zero against real SQL Server. Could break connections through a non-conforming TDS proxy that inflates token lengths (e.g., for padding or encapsulation).
Group 2: Debug.Assert → runtime exception (bug fix)
These replaced
Debug.Assert(which is stripped in Release builds, meaning the invalid state was silently accepted) with a throwing check:valLen > int.MaxValueint.MaxValueParsingErrorLengthlength > MAXSIZEParsingErrorLengthRisk to existing apps: Only if the app was previously "surviving" corrupt data by silently reading garbage. In practice these indicate a corrupted stream that would likely cause downstream failures anyway.
Group 3: Buffer overrun prevention (defense-in-depth)
SqlConnectionInternal.OnFeatureExtAckSRECOVERY:len < 0 || len > data.Length - iBuffer.BlockCopyfrom a malformed feature ack payloadRisk to existing apps: None — this path only fires if the server sends internally inconsistent SRECOVERY data (claimed inner length exceeds the outer buffer already validated by the FeatureExtAck check).
Group 4: Behavioral improvement (non-breaking)
TryReadSqlDateTime((uint)length <= 16) ? stackalloc : new byte[length]with unconditionalstackalloc byte[10]Since the bounds check guarantees
length ≤ 10, the conditional heap allocation path is dead code eliminated. Minor perf win. No user-visible behavior change.Impact summary
The only scenario where a real application could be affected is if it connects through a non-conforming TDS intermediary (proxy/load balancer) that rewrites token lengths to values exceeding these bounds. Standard SQL Server, Azure SQL, and conforming TDS 7.x/8.0 implementations will never produce values exceeding any of these limits.
Test infrastructure
OnSQLBatchCompletedhook toGenericTdsServerfor post-login TDS token injectionOnAuthenticationResponseCompletedhook usage for login-phase token injectionChecklist