[6.1.6] Add TDS token data length bounds checks (#4340)#4359
Open
github-actions[bot] wants to merge 1 commit into
Open
[6.1.6] Add TDS token data length bounds checks (#4340)#4359github-actions[bot] wants to merge 1 commit into
github-actions[bot] wants to merge 1 commit into
Conversation
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.
528f903 to
81947eb
Compare
Contributor
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
paulmedynski
approved these changes
Jun 15, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens TDS token parsing in Microsoft.Data.SqlClient by adding upper-bound validation to length fields read from the wire, preventing unbounded allocations (and related OOB risks) when connected to a malicious or corrupted server.
Changes:
- Added maximum length constants (1 MB token payload cap, 64 KB promote-transaction cap, 10-byte datetime cap) and enforced them during token parsing.
- Converted several debug-only assertions into runtime validation that throws
SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, ...). - Added simulated-server infrastructure and new unit tests covering the new bounds checks.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsEnums.cs | Introduces new max-length constants used by the parser. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs | Adds ParsingErrorLength overload for uint lengths. |
| src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs | Adds bounds checks to multiple token parsing paths (netcore). |
| src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs | Adds bounds checks to multiple token parsing paths (netfx). |
| src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs | Validates SRECOVERY inner-state lengths before copying state buffers (netcore). |
| src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cs | Validates SRECOVERY inner-state lengths before copying state buffers (netfx). |
| src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTdsServer.cs | Adds a hook to mutate SQL batch responses for simulated server tests. |
| src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/Fixtures/TdsServerFixture.cs | New fixture to manage per-test TDS server lifecycle. |
| src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtAckBoundsTests.cs | New tests for FeatureExtAck payload length bounds. |
| src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs | New comprehensive tests for bounds checks across multiple token types. |
| using Microsoft.SqlServer.TDS.Done; | ||
| using Microsoft.SqlServer.TDS.FeatureExtAck; | ||
| using Microsoft.SqlServer.TDS.Servers; | ||
| using Microsoft.SqlServer.TDS.SessionState; |
Comment on lines
+8
to
+11
| /// <summary> | ||
| /// An xunit test fixture that manages the lifecycle of a TdsServer. | ||
| /// </summary> | ||
| public class TdsServerFixture : IDisposable |
| /// sent to the client. Tests can use this to inject or replace tokens in the | ||
| /// response message. | ||
| /// </summary> | ||
| public Action<TDSMessage> OnSQLBatchCompleted { get; set; } |
Comment on lines
+82
to
+83
| Assert.Contains("Error state: 18", ex.Message); // ParsingErrorState.CorruptedTdsStream = 18 | ||
| Assert.Contains($"Length: {TdsEnums.MaxTokenDataLength + 1}", ex.Message); |
| // The connection will fail (insufficient data for the claimed length), | ||
| // but the failure must NOT be the bounds-check error. | ||
| Exception ex = Assert.ThrowsAny<Exception>(connection.Open); | ||
| Assert.DoesNotContain("Error state: 18", ex.Message); |
Comment on lines
+75
to
+76
| Assert.Contains("Error state: 18", ex.Message); // CorruptedTdsStream | ||
| Assert.Contains($"Length: {TdsEnums.MaxTokenDataLength + 1}", ex.Message); |
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.
Description
Re-implementation of #4340 (
b6c05695a) for therelease/6.1branch layout.Adds bounds checks to TDS token parsing to prevent unbounded memory allocation from a malicious server spoofing length fields.
Bounds checks added
ENV_PROMOTETRANSACTIONTryProcessFeatureExtAckdata lengthTryProcessSessionStatetotal lengthTryProcessSessionStateper-state lengthTryProcessFedAuthInfotoken lengthTryProcessReturnValuevalue lengthTryReadSqlDateTimelengthSQLVECTOR/binary readSqlInternalConnectionTdssession recoveryNew constants
TdsEnums.MaxTokenDataLength= 1 MBTdsEnums.MaxPromoteTransactionLength= 64 KBTdsEnums.MaxDateTimeLength= 10New tests (14 total)
FeatureExtAckBoundsTests(2 tests) — oversized feature ext ack dataTdsTokenBoundsTests(12 tests) — all other bounds checks via simulated TDS serverTest infrastructure additions
GenericTdsServer.OnSQLBatchCompleteddelegate hookTdsServerFixturexunit fixture classFiles changed
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsEnums.cssrc/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cssrc/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cssrc/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cssrc/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cssrc/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/SqlInternalConnectionTds.cssrc/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTdsServer.cstests/UnitTests/SimulatedServerTests/Fixtures/TdsServerFixture.cstests/UnitTests/SimulatedServerTests/FeatureExtAckBoundsTests.cstests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.csVerification
Fixes #4340