Add TDS token data length bounds checks#4340
Merged
Merged
Conversation
Contributor
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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4340 +/- ##
==========================================
- Coverage 66.50% 64.84% -1.67%
==========================================
Files 285 280 -5
Lines 43311 66191 +22880
==========================================
+ Hits 28806 42923 +14117
- Misses 14505 23268 +8763
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:
|
paulmedynski
added a commit
that referenced
this pull request
Jun 5, 2026
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.
paulmedynski
commented
Jun 5, 2026
paulmedynski
left a comment
Contributor
Author
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.
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
- 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)
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
… gaps - Convert FeatureExtAckBoundsTests and TdsTokenBoundsTests to per-test instance fixtures (IDisposable) for proper test isolation - Remove unnecessary try/finally cleanup patterns - Tighten assertion messages to check specific error state/length values instead of loose numeric matches - Add test for sql_variant binary negative inner length (length < 0 path) - Add test for SessionState negative inner option length - Add DebugAssertSuppressor comments explaining their purpose - Add TODO comments on sibling test classes questioning Collection usage - Add explanatory comment on TryReadSqlDateTime length parameter
34b23b4 to
c930353
Compare
…ugAssertSuppressor - Add uint overload of SQL.ParsingErrorLength() so the FeatureExtAck bounds check passes the raw uint dataLen without truncating to int. This prevents misleading negative values in diagnostics when a wire length exceeds int.MaxValue. - Fix DebugAssertSuppressor: add a static lock to serialize access to the global Trace.Listeners collection, and clear before restore in Dispose to prevent duplicate listeners. - Update [Collection] comment to explain why serialization is required (DebugAssertSuppressor mutates global state).
Wrap constructor body in try/catch so Monitor.Exit is called if the listener snapshot allocation or CopyTo throws. Wrap Dispose restore in try/finally so the lock is always released even if AddRange fails.
mdaigle
approved these changes
Jun 9, 2026
apoorvdeshmukh
approved these changes
Jun 11, 2026
cheenamalhotra
approved these changes
Jun 11, 2026
This was referenced Jun 11, 2026
paulmedynski
added a commit
that referenced
this pull request
Jun 15, 2026
paulmedynski
pushed a commit
that referenced
this pull request
Jun 15, 2026
Re-implementation of b6c0569 for the release/6.1 branch layout. Adds bounds checks to TDS token parsing to prevent unbounded memory allocation from a malicious server spoofing length fields: - ENV_PROMOTETRANSACTION: reject > 64 KB - FeatureExtAck data: reject > 1 MB - SessionState total + per-state: reject > 1 MB - FedAuthInfo: reject > 1 MB - ReturnValue: reject > int.MaxValue (was silent truncation) - TryReadSqlDateTime: reject > 10 bytes (max DateTimeOffset wire size) - SQLVECTOR/binary: reject > MAXSIZE Also adds bounds check in SqlInternalConnectionTds session recovery parsing to prevent buffer overread from spoofed state lengths. New constants: TdsEnums.MaxTokenDataLength (1 MB), MaxPromoteTransactionLength (64 KB), MaxDateTimeLength (10). New tests: FeatureExtAckBoundsTests (2 tests), TdsTokenBoundsTests (12 tests) using simulated TDS server.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 < 0orstateLen > MaxTokenDataLengthSessionState_OversizedInnerOptionLength,SessionState_NegativeInnerOptionLengthTdsParser.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 < 0orlength > MAXSIZE(8000) for binary types in sql_variant pathBatchResponse_SqlVariantBinary_OversizedLength,BatchResponse_SqlVariantBinary_NegativeLengthSqlConnectionInternal.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.
dataLentokenLennewLengthlengthlenDataresults from a total variant length smaller than the type overheadRisk 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 injectionIDisposable) for proper test isolation — each test gets a freshTdsServerFixtureChecklist