From b2e88316ceeb2a3d6388190fb6d4c92f325c07f5 Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Thu, 4 Jun 2026 10:39:52 -0300 Subject: [PATCH 01/11] Add bounds checks for TDS token data lengths (pre-auth DoS fix) 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 --- .../Connection/SqlConnectionInternal.cs | 5 + .../src/Microsoft/Data/SqlClient/TdsEnums.cs | 8 + .../src/Microsoft/Data/SqlClient/TdsParser.cs | 44 ++- .../FeatureExtAckBoundsTests.cs | 159 ++++++++ .../TdsTokenBoundsTests.cs | 344 ++++++++++++++++++ 5 files changed, 555 insertions(+), 5 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtAckBoundsTests.cs create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs index 295286b1e5..4bfb229745 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs @@ -1346,6 +1346,11 @@ internal void OnFeatureExtAck(int featureId, byte[] data) len = bLen; } + if (len < 0 || len > data.Length - i) + { + throw SQL.ParsingError(ParsingErrorState.CorruptedTdsStream); + } + byte[] stateData = new byte[len]; Buffer.BlockCopy(data, i, stateData, 0, len); i += len; diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsEnums.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsEnums.cs index 25a489d91d..c63c33c384 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsEnums.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsEnums.cs @@ -84,6 +84,14 @@ internal static class TdsEnums public const int MAX_PACKET_SIZE = 32768; public const int MAX_SERVER_USER_NAME = 256; // obtained from luxor + // Maximum allowed data length for login-phase token payloads (feature ext ack, + // session state, fedauth info). Prevents a malicious server from causing + // unbounded memory allocation on the client before authentication completes. + 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 + // Severity 0 - 10 indicates informational (non-error) messages // Severity 11 - 16 indicates errors that can be corrected by user (syntax errors, etc...) // Severity 17 - 19 indicates failure due to insufficient resources in the server diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs index bb625d563d..2b3d64f8df 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs @@ -3348,6 +3348,10 @@ private TdsOperationStatus TryProcessEnvChange(int tokenLength, TdsParserStateOb // new value has 4 byte length return result; } + if (env._newLength < 0 || env._newLength > TdsEnums.MaxPromoteTransactionLength) + { + throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, env._newLength); + } // read new value with 4 byte length env._newBinValue = new byte[env._newLength]; result = stateObj.TryReadByteArray(env._newBinValue, env._newLength); @@ -3846,10 +3850,14 @@ private TdsOperationStatus TryProcessFeatureExtAck(TdsParserStateObject stateObj { return result; } + if (dataLen > (uint)TdsEnums.MaxTokenDataLength) + { + throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, (int)dataLen); + } byte[] data = new byte[dataLen]; if (dataLen > 0) { - result = stateObj.TryReadByteArray(data, checked((int)dataLen)); + result = stateObj.TryReadByteArray(data, (int)dataLen); if (result != TdsOperationStatus.Done) { return result; @@ -4169,6 +4177,10 @@ private TdsOperationStatus TryProcessSessionState(TdsParserStateObject stateObj, { throw SQL.ParsingErrorLength(ParsingErrorState.SessionStateLengthTooShort, length); } + if (length > TdsEnums.MaxTokenDataLength) + { + throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, length); + } uint seqNum; TdsOperationStatus result = stateObj.TryReadUInt32(out seqNum); if (result != TdsOperationStatus.Done) @@ -4218,6 +4230,10 @@ private TdsOperationStatus TryProcessSessionState(TdsParserStateObject stateObj, return result; } } + if (stateLen < 0 || stateLen > TdsEnums.MaxTokenDataLength) + { + throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, stateLen); + } byte[] buffer = null; lock (sdata._delta) { @@ -4435,6 +4451,10 @@ private TdsOperationStatus TryProcessFedAuthInfo(TdsParserStateObject stateObj, SqlClientEventSource.Log.TryTraceEvent(" FEDAUTHINFO token stream length too short for CountOfInfoIDs."); throw SQL.ParsingErrorLength(ParsingErrorState.FedAuthInfoLengthTooShortForCountOfInfoIds, tokenLen); } + if (tokenLen > TdsEnums.MaxTokenDataLength) + { + throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, tokenLen); + } // read how many FedAuthInfo options there are uint optionsCount; @@ -4912,9 +4932,12 @@ internal TdsOperationStatus TryProcessReturnValue(int length, } // always read as sql types - Debug.Assert(valLen < (ulong)(int.MaxValue), "ProcessReturnValue received data size > 2Gb"); + if (valLen > (ulong)int.MaxValue) + { + throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, int.MaxValue); + } - int intlen = valLen > (ulong)(int.MaxValue) ? int.MaxValue : (int)valLen; + int intlen = (int)valLen; if (rec.metaType.IsPlp) { @@ -7113,7 +7136,15 @@ internal TdsOperationStatus TryReadSqlValue(SqlBuffer value, private TdsOperationStatus TryReadSqlDateTime(SqlBuffer value, byte tdsType, int length, byte scale, TdsParserStateObject stateObj) { - Span datetimeBuffer = ((uint)length <= 16) ? stackalloc byte[16] : new byte[length]; + // 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. + const int MaxDateTimeLength = 10; + if (length < 0 || length > MaxDateTimeLength) + { + throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, length); + } + + Span datetimeBuffer = stackalloc byte[MaxDateTimeLength]; TdsOperationStatus result = stateObj.TryReadByteArray(datetimeBuffer, length); if (result != TdsOperationStatus.Done) @@ -7369,7 +7400,10 @@ internal TdsOperationStatus TryReadSqlValueInternal(SqlBuffer value, byte tdsTyp case TdsEnums.SQLVECTOR: { // 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); + } result = stateObj.TryReadByteArrayWithContinue(length, isPlp: false, out byte[] b); if (result != TdsOperationStatus.Done) { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtAckBoundsTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtAckBoundsTests.cs new file mode 100644 index 0000000000..d5adc3b4c1 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtAckBoundsTests.cs @@ -0,0 +1,159 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.IO; +using System.Linq; +using Microsoft.SqlServer.TDS; +using Microsoft.SqlServer.TDS.FeatureExtAck; +using Microsoft.SqlServer.TDS.Servers; +using TDSDoneToken = global::Microsoft.SqlServer.TDS.Done.TDSDoneToken; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests; + +/// +/// Tests that the TDS parser rejects feature extension acknowledgment tokens +/// with data lengths exceeding protocol-reasonable bounds. This prevents a +/// malicious server from causing unbounded memory allocation on the client. +/// +[Collection("SimulatedServerTests")] +public class FeatureExtAckBoundsTests : IClassFixture +{ + private readonly TdsServer _server; + private readonly string _connectionString; + + public FeatureExtAckBoundsTests(TdsServerFixture fixture) + { + _server = fixture.TdsServer; + SqlConnectionStringBuilder builder = new() + { + DataSource = $"localhost,{_server.EndPoint.Port}", + Encrypt = SqlConnectionEncryptOption.Optional, + Pooling = false + }; + _connectionString = builder.ConnectionString; + } + + /// + /// Verifies that the TDS parser rejects a FeatureExtAck token whose data length + /// field exceeds (1 MB), throwing a + /// parsing error instead of attempting an unbounded heap allocation. + /// This guards against CVE denial-of-service via pre-auth memory exhaustion. + /// + [Fact] + public void FeatureExtAck_OversizedDataLength_ThrowsParsingError() + { + // Arrange: inject a malicious FeatureExtAck token with an absurdly large data length + _server.OnAuthenticationResponseCompleted = responseMessage => + { + // Remove any existing FeatureExtAck token + var existing = responseMessage.OfType().FirstOrDefault(); + if (existing != null) + { + responseMessage.Remove(existing); + } + + // Insert a malicious token with oversized data length before the DONE token + int doneIndex = responseMessage.FindIndex(t => t is TDSDoneToken); + if (doneIndex < 0) + { + doneIndex = responseMessage.Count; + } + + responseMessage.Insert(doneIndex, new MaliciousFeatureExtAckToken( + featureId: (TDSFeatureID)TdsEnums.FEATUREEXT_GLOBALTRANSACTIONS, + claimedDataLen: (uint)(TdsEnums.MaxTokenDataLength + 1))); + }; + + // Act & Assert: connection should fail with a parsing error, NOT an OutOfMemoryException + using SqlConnection connection = new(_connectionString); + Exception ex = Assert.ThrowsAny(connection.Open); + + // The exception message should indicate a corrupted TDS stream (parsing error state 18) + // with the oversized length value, not an OOM from attempting the allocation. + Assert.Contains("18", ex.Message); // ParsingErrorState.CorruptedTdsStream = 18 + Assert.Contains((TdsEnums.MaxTokenDataLength + 1).ToString(), ex.Message); + } + + /// + /// Verifies that a FeatureExtAck token with a data length at or below the + /// allowed maximum is accepted without error, confirming there is no + /// off-by-one in the bounds check. + /// + [Fact] + public void FeatureExtAck_MaxAllowedDataLength_DoesNotThrow() + { + // Arrange: inject a FeatureExtAck token at exactly the maximum allowed size. + // We use a small real payload to avoid actually allocating 1 MB in the test; + // the server will write a data length that equals the actual data length. + // This verifies that the boundary value is accepted (not off-by-one). + byte[] legitimateData = new byte[] { 0x01 }; // 1-byte GlobalTransactions ack + + _server.OnAuthenticationResponseCompleted = responseMessage => + { + // Find existing FeatureExtAck or create one + var existing = responseMessage.OfType().FirstOrDefault(); + if (existing == null) + { + int doneIndex = responseMessage.FindIndex(t => t is TDSDoneToken); + if (doneIndex < 0) + { + doneIndex = responseMessage.Count; + } + + var token = new TDSFeatureExtAckToken( + new TDSFeatureExtAckGenericOption( + (TDSFeatureID)TdsEnums.FEATUREEXT_GLOBALTRANSACTIONS, + (uint)legitimateData.Length, + legitimateData)); + responseMessage.Insert(doneIndex, token); + } + }; + + // Act & Assert: connection should open successfully + using SqlConnection connection = new(_connectionString); + connection.Open(); + Assert.Equal(System.Data.ConnectionState.Open, connection.State); + } + + /// + /// A custom TDS packet token that writes a FEATUREEXTACK token with a fraudulently + /// large data length field. This simulates a malicious server attempting to cause + /// the client to allocate an unbounded byte array. + /// + private sealed class MaliciousFeatureExtAckToken : TDSPacketToken + { + private readonly TDSFeatureID _featureId; + private readonly uint _claimedDataLen; + + public MaliciousFeatureExtAckToken(TDSFeatureID featureId, uint claimedDataLen) + { + _featureId = featureId; + _claimedDataLen = claimedDataLen; + } + + public override bool Inflate(Stream source) => throw new NotSupportedException(); + + public override void Deflate(Stream destination) + { + // Write the FEATUREEXTACK token type (0xAE) + destination.WriteByte((byte)TDSTokenType.FeatureExtAck); + + // Write the feature ID byte + destination.WriteByte((byte)_featureId); + + // Write the claimed data length (uint32, little-endian) — this is the lie + byte[] lenBytes = BitConverter.GetBytes(_claimedDataLen); + destination.Write(lenBytes, 0, 4); + + // Write only 1 byte of actual data (the client will try to read _claimedDataLen bytes + // but we only provide 1 — the bounds check should fire before the read attempt) + destination.WriteByte(0x01); + + // Write terminator + destination.WriteByte((byte)TDSFeatureID.Terminator); + } + } +} diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs new file mode 100644 index 0000000000..ee1150c5dc --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs @@ -0,0 +1,344 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.IO; +using System.Linq; +using Microsoft.SqlServer.TDS; +using Microsoft.SqlServer.TDS.Done; +using Microsoft.SqlServer.TDS.FeatureExtAck; +using Microsoft.SqlServer.TDS.Servers; +using Microsoft.SqlServer.TDS.SessionState; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests; + +/// +/// Tests that the TDS parser rejects various token types with data lengths +/// exceeding protocol-reasonable bounds, preventing unbounded memory allocation +/// from a malicious server. +/// +[Collection("SimulatedServerTests")] +public class TdsTokenBoundsTests : IClassFixture +{ + private readonly TdsServer _server; + private readonly string _connectionString; + + public TdsTokenBoundsTests(TdsServerFixture fixture) + { + _server = fixture.TdsServer; + SqlConnectionStringBuilder builder = new() + { + DataSource = $"localhost,{_server.EndPoint.Port}", + Encrypt = SqlConnectionEncryptOption.Optional, + Pooling = false + }; + _connectionString = builder.ConnectionString; + } + + // ────────────────────────────────────────────────────────────────────────── + // Test 1: SessionState token with oversized total length + // ────────────────────────────────────────────────────────────────────────── + + /// + /// Verifies that TryProcessSessionState rejects a SessionState token (0xE4) + /// whose total length field exceeds (1 MB), + /// preventing unbounded memory allocation from a spoofed token length. + /// + [Fact] + public void SessionState_OversizedTotalLength_ThrowsParsingError() + { + _server.OnAuthenticationResponseCompleted = responseMessage => + { + int doneIndex = responseMessage.FindIndex(t => t is TDSDoneToken); + if (doneIndex < 0) + { + doneIndex = responseMessage.Count; + } + + // Inject a SessionState token claiming a total length exceeding MaxTokenDataLength + responseMessage.Insert(doneIndex, new MaliciousSessionStateToken( + claimedTotalLength: (uint)(TdsEnums.MaxTokenDataLength + 1))); + }; + + using SqlConnection connection = new(_connectionString); + Exception ex = Assert.ThrowsAny(connection.Open); + Assert.Contains("18", ex.Message); // CorruptedTdsStream + } + + // ────────────────────────────────────────────────────────────────────────── + // Test 2: SessionState token with oversized inner option length + // ────────────────────────────────────────────────────────────────────────── + + /// + /// Verifies that TryProcessSessionState rejects an individual session state + /// option whose inner data length (encoded via the 0xFF + DWORD path) exceeds + /// , even when the outer token length is valid. + /// + [Fact] + public void SessionState_OversizedInnerOptionLength_ThrowsParsingError() + { + _server.OnAuthenticationResponseCompleted = responseMessage => + { + int doneIndex = responseMessage.FindIndex(t => t is TDSDoneToken); + if (doneIndex < 0) + { + doneIndex = responseMessage.Count; + } + + // Inject a SessionState token with a valid outer length but an inner + // state option claiming a huge data length + responseMessage.Insert(doneIndex, new MaliciousSessionStateInnerLenToken( + innerClaimedLength: TdsEnums.MaxTokenDataLength + 1)); + }; + + using SqlConnection connection = new(_connectionString); + Exception ex = Assert.ThrowsAny(connection.Open); + Assert.Contains("18", ex.Message); // CorruptedTdsStream + } + + // ────────────────────────────────────────────────────────────────────────── + // Test 3: SRECOVERY feature ack with malformed inner state data + // ────────────────────────────────────────────────────────────────────────── + + /// + /// Verifies that the secondary parse of FEATUREEXT_SRECOVERY data in + /// SqlConnectionInternal.OnFeatureExtAck rejects inner state options + /// whose claimed length exceeds the remaining buffer, preventing an + /// out-of-bounds read or over-allocation. + /// + [Fact] + public void SRecovery_MalformedInnerStateLength_ThrowsParsingError() + { + _server.OnAuthenticationResponseCompleted = responseMessage => + { + // Remove existing FeatureExtAck if present + var existing = responseMessage.OfType().FirstOrDefault(); + if (existing != null) + { + responseMessage.Remove(existing); + } + + int doneIndex = responseMessage.FindIndex(t => t is TDSDoneToken); + if (doneIndex < 0) + { + doneIndex = responseMessage.Count; + } + + // Inject a FeatureExtAck with SessionRecovery feature containing + // inner state data where a state option claims a length exceeding the buffer + responseMessage.Insert(doneIndex, new MaliciousSRecoveryFeatureExtAckToken()); + }; + + using SqlConnection connection = new(_connectionString); + Exception ex = Assert.ThrowsAny(connection.Open); + Assert.Contains("18", ex.Message); // CorruptedTdsStream + } + + // ────────────────────────────────────────────────────────────────────────── + // Test 4: FedAuthInfo token with oversized total length + // ────────────────────────────────────────────────────────────────────────── + + /// + /// Verifies that TryProcessFedAuthInfo rejects a FedAuthInfo token (0xEE) + /// whose total length exceeds (1 MB). + /// The token type is dispatched unconditionally by TryRun, so this check + /// fires regardless of whether federated authentication was negotiated. + /// + [Fact] + public void FedAuthInfo_OversizedTokenLength_ThrowsParsingError() + { + _server.OnAuthenticationResponseCompleted = responseMessage => + { + int doneIndex = responseMessage.FindIndex(t => t is TDSDoneToken); + if (doneIndex < 0) + { + doneIndex = responseMessage.Count; + } + + // Inject a FedAuthInfo token with a total length exceeding MaxTokenDataLength. + // The parser dispatches on token type regardless of whether FedAuth was negotiated. + responseMessage.Insert(doneIndex, new MaliciousFedAuthInfoToken( + claimedLength: TdsEnums.MaxTokenDataLength + 1)); + }; + + using SqlConnection connection = new(_connectionString); + Exception ex = Assert.ThrowsAny(connection.Open); + Assert.Contains("18", ex.Message); // CorruptedTdsStream + } + + // ══════════════════════════════════════════════════════════════════════════ + // Malicious token helpers + // ══════════════════════════════════════════════════════════════════════════ + + /// + /// Writes a SessionState token (0xE4) with a fraudulently large total length. + /// Wire format: [0xE4][uint32 totalLen][uint32 seqNum][byte status][...] + /// The bounds check fires on the totalLen value before any data is read. + /// + private sealed class MaliciousSessionStateToken : TDSPacketToken + { + private readonly uint _claimedTotalLength; + + public MaliciousSessionStateToken(uint claimedTotalLength) + { + _claimedTotalLength = claimedTotalLength; + } + + public override bool Inflate(Stream source) => throw new NotSupportedException(); + + public override void Deflate(Stream destination) + { + // Token type + destination.WriteByte((byte)TDSTokenType.SessionState); // 0xE4 + + // Total token length (uint32) — this is the fraudulent value + byte[] lenBytes = BitConverter.GetBytes(_claimedTotalLength); + destination.Write(lenBytes, 0, 4); + + // Write minimal valid-looking data: seqNum (4 bytes) + status (1 byte) + // The bounds check should fire before trying to process option data. + destination.Write(new byte[] { 0x01, 0x00, 0x00, 0x00 }, 0, 4); // seqNum = 1 + destination.WriteByte(0x01); // status = recoverable + } + } + + /// + /// Writes a SessionState token (0xE4) with a valid outer length but an inner + /// state option that claims a huge data length (using the 0xFF + DWORD encoding). + /// Wire format: [0xE4][uint32 totalLen][uint32 seqNum][byte status] + /// [byte stateId][0xFF][int32 innerLen][...data...] + /// The inner bounds check fires on the innerLen value. + /// + private sealed class MaliciousSessionStateInnerLenToken : TDSPacketToken + { + private readonly int _innerClaimedLength; + + public MaliciousSessionStateInnerLenToken(int innerClaimedLength) + { + _innerClaimedLength = innerClaimedLength; + } + + public override bool Inflate(Stream source) => throw new NotSupportedException(); + + public override void Deflate(Stream destination) + { + // Token type + destination.WriteByte((byte)TDSTokenType.SessionState); // 0xE4 + + // Calculate total token length: + // seqNum(4) + status(1) + stateId(1) + lenMarker(1) + innerLen(4) + minimal data(1) + uint totalLength = 4 + 1 + 1 + 1 + 4 + 1; + + byte[] lenBytes = BitConverter.GetBytes(totalLength); + destination.Write(lenBytes, 0, 4); + + // Sequence number + destination.Write(new byte[] { 0x01, 0x00, 0x00, 0x00 }, 0, 4); + + // Status (recoverable) + destination.WriteByte(0x01); + + // State option: stateId + destination.WriteByte(0x00); // UserOptions state ID + + // Length marker: 0xFF means next 4 bytes are the DWORD length + destination.WriteByte(0xFF); + + // Inner claimed length — fraudulently large + byte[] innerLenBytes = BitConverter.GetBytes(_innerClaimedLength); + destination.Write(innerLenBytes, 0, 4); + + // Write only 1 byte of actual data + destination.WriteByte(0x42); + } + } + + /// + /// Writes a FeatureExtAck token (0xAE) with a SessionRecovery feature (ID=1) + /// that carries inner state data where a state option claims a length exceeding + /// the remaining buffer. This exercises the bounds check in + /// SqlConnectionInternal.OnFeatureExtAck for FEATUREEXT_SRECOVERY. + /// + private sealed class MaliciousSRecoveryFeatureExtAckToken : TDSPacketToken + { + public override bool Inflate(Stream source) => throw new NotSupportedException(); + + public override void Deflate(Stream destination) + { + // Token type: FEATUREEXTACK + destination.WriteByte((byte)TDSTokenType.FeatureExtAck); // 0xAE + + // Feature ID: SessionRecovery = 0x01 + destination.WriteByte(0x01); + + // The feature data for SRECOVERY is parsed by SqlConnectionInternal.OnFeatureExtAck: + // It reads pairs of [stateId(1)][lenByte(1)][data(len)] or [stateId(1)][0xFF][int32 len][data(len)] + // We'll craft inner data where one option claims length > remaining buffer. + + // Inner data layout: + // stateId(1) + 0xFF marker(1) + int32 len(4) + 1 byte actual data + byte[] innerData; + using (var ms = new MemoryStream()) + { + ms.WriteByte(0x00); // stateId = 0 + + // Use 0xFF marker to indicate DWORD length + ms.WriteByte(0xFF); + + // Claim a length that exceeds the remaining buffer + // The remaining buffer after reading stateId + 0xFF + 4-byte-len will be 1 byte, + // but we claim 999 bytes + byte[] claimedLen = BitConverter.GetBytes(999); + ms.Write(claimedLen, 0, 4); + + // Only provide 1 byte of actual data + ms.WriteByte(0x42); + + innerData = ms.ToArray(); + } + + // Feature data length (uint32) + byte[] featureDataLen = BitConverter.GetBytes((uint)innerData.Length); + destination.Write(featureDataLen, 0, 4); + + // Feature data + destination.Write(innerData, 0, innerData.Length); + + // Terminator + destination.WriteByte((byte)TDSFeatureID.Terminator); // 0xFF + } + } + + /// + /// Writes a FedAuthInfo token (0xEE) with a fraudulently large total length. + /// Wire format: [0xEE][int32 tokenLen][...data...] + /// The bounds check fires on tokenLen before any data is read. + /// + private sealed class MaliciousFedAuthInfoToken : TDSPacketToken + { + private readonly int _claimedLength; + + public MaliciousFedAuthInfoToken(int claimedLength) + { + _claimedLength = claimedLength; + } + + public override bool Inflate(Stream source) => throw new NotSupportedException(); + + public override void Deflate(Stream destination) + { + // Token type + destination.WriteByte(0xEE); // SQLFEDAUTHINFO + + // Token length (int32) — fraudulently large + byte[] lenBytes = BitConverter.GetBytes(_claimedLength); + destination.Write(lenBytes, 0, 4); + + // Write minimal data (at least sizeof(uint) to pass the lower bound check, + // but the upper bound check should fire first) + destination.Write(new byte[] { 0x01, 0x00, 0x00, 0x00 }, 0, 4); // optionsCount = 1 + } + } +} From 7bd1a52e18f9f9b7254178dc928ed3ff251fc19d Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Thu, 4 Jun 2026 10:46:35 -0300 Subject: [PATCH 02/11] Add test for ENV_PROMOTETRANSACTION bounds check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../Connection/SqlConnectionInternal.cs | 2 +- .../TdsTokenBoundsTests.cs | 77 +++++++++++++++++++ 2 files changed, 78 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs index 4bfb229745..2c531b5809 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs @@ -400,7 +400,7 @@ internal SqlConnectionInternal( try { - // If we want to consider pool operations against the overall connect timeout, + // If we want to consider pool operations against the overall connect timeout, // use the provided timeout. Otherwise, start a fresh timeout to receive the full // connect timeout. _timeout = ResolveLoginTimeout(timeout, connectionOptions.ConnectTimeout); diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs index ee1150c5dc..94e35f0fd7 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs @@ -168,6 +168,38 @@ public void FedAuthInfo_OversizedTokenLength_ThrowsParsingError() Assert.Contains("18", ex.Message); // CorruptedTdsStream } + // ────────────────────────────────────────────────────────────────────────── + // Test 5: ENV_PROMOTETRANSACTION with oversized newLength + // ────────────────────────────────────────────────────────────────────────── + + /// + /// Verifies that TryProcessEnvChange rejects a PromoteTransaction + /// environment change token (type 15) whose inner newLength field exceeds + /// (64 KB). A malicious server + /// can set the outer uint16 token length to a small value while writing an + /// int32 inner length claiming gigabytes, causing unbounded allocation. + /// + [Fact] + public void EnvChange_PromoteTransaction_OversizedLength_ThrowsParsingError() + { + _server.OnAuthenticationResponseCompleted = responseMessage => + { + int doneIndex = responseMessage.FindIndex(t => t is TDSDoneToken); + if (doneIndex < 0) + { + doneIndex = responseMessage.Count; + } + + // Inject a PromoteTransaction env change with a fraudulently large newLength + responseMessage.Insert(doneIndex, new MaliciousPromoteTransactionEnvChangeToken( + claimedNewLength: TdsEnums.MaxPromoteTransactionLength + 1)); + }; + + using SqlConnection connection = new(_connectionString); + Exception ex = Assert.ThrowsAny(connection.Open); + Assert.Contains("18", ex.Message); // CorruptedTdsStream + } + // ══════════════════════════════════════════════════════════════════════════ // Malicious token helpers // ══════════════════════════════════════════════════════════════════════════ @@ -341,4 +373,49 @@ public override void Deflate(Stream destination) destination.Write(new byte[] { 0x01, 0x00, 0x00, 0x00 }, 0, 4); // optionsCount = 1 } } + + /// + /// Writes an EnvChange token (0xE3) with type PromoteTransaction (15) whose + /// inner int32 newLength exceeds . + /// Wire format: [0xE3][uint16 tokenLen][byte type=15][int32 newLen][data...][byte oldLen=0] + /// The outer uint16 tokenLen is set to accommodate the header but the int32 + /// newLength claims far more data than actually follows, triggering the bounds check. + /// + private sealed class MaliciousPromoteTransactionEnvChangeToken : TDSPacketToken + { + private readonly int _claimedNewLength; + + public MaliciousPromoteTransactionEnvChangeToken(int claimedNewLength) + { + _claimedNewLength = claimedNewLength; + } + + public override bool Inflate(Stream source) => throw new NotSupportedException(); + + public override void Deflate(Stream destination) + { + // Token type: ENVCHANGE + destination.WriteByte((byte)TDSTokenType.EnvironmentChange); // 0xE3 + + // Outer token length (uint16): type(1) + newLength(4) + 1 byte data + oldLength(1) + // We write just enough to contain the header fields the parser reads before + // hitting the bounds check. The parser reads: type(1) + int32 newLen(4) = 5 bytes min. + ushort outerLength = 1 + 4 + 1 + 1; // type + newLen + 1 fake byte + oldLen + byte[] outerLenBytes = BitConverter.GetBytes(outerLength); + destination.Write(outerLenBytes, 0, 2); + + // Env change type: PromoteTransaction = 15 + destination.WriteByte(15); + + // newLength (int32) — fraudulently large + byte[] newLenBytes = BitConverter.GetBytes(_claimedNewLength); + destination.Write(newLenBytes, 0, 4); + + // Write 1 byte of fake data (the bounds check fires before attempting to read this much) + destination.WriteByte(0x00); + + // Old value length (byte) = 0 + destination.WriteByte(0x00); + } + } } From 44b9dcdc7c9feac3f074b64ccbed4033bc32ddda Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Thu, 4 Jun 2026 10:52:50 -0300 Subject: [PATCH 03/11] Fix TryProcessReturnValue bounds check to skip PLP types 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. --- .../src/Microsoft/Data/SqlClient/TdsParser.cs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs index 2b3d64f8df..337b0de49b 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs @@ -4932,17 +4932,20 @@ internal TdsOperationStatus TryProcessReturnValue(int length, } // always read as sql types - if (valLen > (ulong)int.MaxValue) - { - throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, int.MaxValue); - } - - int intlen = (int)valLen; + 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, int.MaxValue); + } + else + { + intlen = (int)valLen; + } if (rec.type == SqlDbTypeExtensions.Vector) { From fa372aa126590a74614f3c7e0b6cbfd4de48b794 Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Thu, 4 Jun 2026 12:52:52 -0300 Subject: [PATCH 04/11] Add SQL batch response injection hook and DateTime bounds test - 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 --- .../TdsTokenBoundsTests.cs | 222 ++++++++++++++++-- .../tools/TDS/TDS.Servers/GenericTdsServer.cs | 10 + 2 files changed, 217 insertions(+), 15 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs index 94e35f0fd7..067ca06f09 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs @@ -6,6 +6,7 @@ using System.IO; using System.Linq; using Microsoft.SqlServer.TDS; +using Microsoft.SqlServer.TDS.ColMetadata; using Microsoft.SqlServer.TDS.Done; using Microsoft.SqlServer.TDS.FeatureExtAck; using Microsoft.SqlServer.TDS.Servers; @@ -62,9 +63,16 @@ public void SessionState_OversizedTotalLength_ThrowsParsingError() claimedTotalLength: (uint)(TdsEnums.MaxTokenDataLength + 1))); }; - using SqlConnection connection = new(_connectionString); - Exception ex = Assert.ThrowsAny(connection.Open); - Assert.Contains("18", ex.Message); // CorruptedTdsStream + try + { + using SqlConnection connection = new(_connectionString); + Exception ex = Assert.ThrowsAny(connection.Open); + Assert.Contains("18", ex.Message); // CorruptedTdsStream + } + finally + { + _server.OnAuthenticationResponseCompleted = null; + } } // ────────────────────────────────────────────────────────────────────────── @@ -93,9 +101,16 @@ public void SessionState_OversizedInnerOptionLength_ThrowsParsingError() innerClaimedLength: TdsEnums.MaxTokenDataLength + 1)); }; - using SqlConnection connection = new(_connectionString); - Exception ex = Assert.ThrowsAny(connection.Open); - Assert.Contains("18", ex.Message); // CorruptedTdsStream + try + { + using SqlConnection connection = new(_connectionString); + Exception ex = Assert.ThrowsAny(connection.Open); + Assert.Contains("18", ex.Message); // CorruptedTdsStream + } + finally + { + _server.OnAuthenticationResponseCompleted = null; + } } // ────────────────────────────────────────────────────────────────────────── @@ -131,9 +146,16 @@ public void SRecovery_MalformedInnerStateLength_ThrowsParsingError() responseMessage.Insert(doneIndex, new MaliciousSRecoveryFeatureExtAckToken()); }; - using SqlConnection connection = new(_connectionString); - Exception ex = Assert.ThrowsAny(connection.Open); - Assert.Contains("18", ex.Message); // CorruptedTdsStream + try + { + using SqlConnection connection = new(_connectionString); + Exception ex = Assert.ThrowsAny(connection.Open); + Assert.Contains("18", ex.Message); // CorruptedTdsStream + } + finally + { + _server.OnAuthenticationResponseCompleted = null; + } } // ────────────────────────────────────────────────────────────────────────── @@ -163,9 +185,16 @@ public void FedAuthInfo_OversizedTokenLength_ThrowsParsingError() claimedLength: TdsEnums.MaxTokenDataLength + 1)); }; - using SqlConnection connection = new(_connectionString); - Exception ex = Assert.ThrowsAny(connection.Open); - Assert.Contains("18", ex.Message); // CorruptedTdsStream + try + { + using SqlConnection connection = new(_connectionString); + Exception ex = Assert.ThrowsAny(connection.Open); + Assert.Contains("18", ex.Message); // CorruptedTdsStream + } + finally + { + _server.OnAuthenticationResponseCompleted = null; + } } // ────────────────────────────────────────────────────────────────────────── @@ -195,9 +224,16 @@ public void EnvChange_PromoteTransaction_OversizedLength_ThrowsParsingError() claimedNewLength: TdsEnums.MaxPromoteTransactionLength + 1)); }; - using SqlConnection connection = new(_connectionString); - Exception ex = Assert.ThrowsAny(connection.Open); - Assert.Contains("18", ex.Message); // CorruptedTdsStream + try + { + using SqlConnection connection = new(_connectionString); + Exception ex = Assert.ThrowsAny(connection.Open); + Assert.Contains("18", ex.Message); // CorruptedTdsStream + } + finally + { + _server.OnAuthenticationResponseCompleted = null; + } } // ══════════════════════════════════════════════════════════════════════════ @@ -418,4 +454,160 @@ public override void Deflate(Stream destination) destination.WriteByte(0x00); } } + + // ────────────────────────────────────────────────────────────────────────── + // Test 6: Post-login batch response injection — PromoteTransaction + // ────────────────────────────────────────────────────────────────────────── + + /// + /// Verifies that bounds checks fire during command execution (post-login) + /// by injecting a malicious PromoteTransaction env change token into the + /// SQL batch response. This exercises the OnSQLBatchCompleted hook + /// on the simulated server and proves the same bounds check fires regardless + /// of whether the token arrives during login or command execution. + /// + [Fact] + public void BatchResponse_PromoteTransaction_OversizedLength_ThrowsParsingError() + { + _server.OnSQLBatchCompleted = responseMessage => + { + int doneIndex = responseMessage.FindIndex(t => t is TDSDoneToken); + if (doneIndex < 0) + { + doneIndex = responseMessage.Count; + } + + responseMessage.Insert(doneIndex, new MaliciousPromoteTransactionEnvChangeToken( + claimedNewLength: TdsEnums.MaxPromoteTransactionLength + 1)); + }; + + try + { + using SqlConnection connection = new(_connectionString); + connection.Open(); + + using SqlCommand command = connection.CreateCommand(); + command.CommandText = "SELECT 1"; + + Exception ex = Assert.ThrowsAny( + () => command.ExecuteNonQuery()); + Assert.Contains("18", ex.Message); // CorruptedTdsStream + } + finally + { + _server.OnSQLBatchCompleted = null; + } + } + + // ────────────────────────────────────────────────────────────────────────── + // Test 7: Post-login batch response — DateTime oversized length + // ────────────────────────────────────────────────────────────────────────── + + /// + /// Verifies that TryReadSqlDateTime rejects a TIME column value whose + /// data length byte exceeds the maximum datetime wire size (10 bytes). A + /// malicious server could set this length to a large value, causing the + /// parser to attempt an oversized stackalloc. + /// + [Fact] + public void BatchResponse_DateTime_OversizedLength_ThrowsParsingError() + { + _server.OnSQLBatchCompleted = responseMessage => + { + // Replace the entire response with a crafted result set containing + // a TIME column whose ROW data length exceeds the maximum. + responseMessage.Clear(); + + // Use proper library tokens for COLMETADATA so framing is correct + var metadata = new TDSColMetadataToken(); + var col = new TDSColumnData(); + col.DataType = TDSDataType.TimeN; + col.DataTypeSpecific = (byte)7; // scale = 7 + col.Flags.IsNullable = true; + col.Name = string.Empty; + metadata.Columns.Add(col); + responseMessage.Add(metadata); + + // Add a malicious ROW token with oversized data length + responseMessage.Add(new MaliciousTimeRowToken()); + + // Add DONE token + responseMessage.Add(new TDSDoneToken(TDSDoneTokenStatusType.Final | TDSDoneTokenStatusType.Count, TDSDoneTokenCommandType.Select, 1)); + }; + + try + { + using SqlConnection connection = new(_connectionString); + connection.Open(); + + using SqlCommand command = connection.CreateCommand(); + command.CommandText = "MALICIOUS_QUERY_NOT_RECOGNIZED"; + + SqlDataReader reader = command.ExecuteReader(); + try + { + // Read() returns true (data is ready) but doesn't actually parse the + // column values — that happens on GetValue/GetTimeSpan. Force the read. + Assert.True(reader.Read()); + Exception ex = Assert.ThrowsAny( + () => reader.GetValue(0)); + Assert.Contains("18", ex.Message); // CorruptedTdsStream + } + finally + { + // After corrupting the stream, dispose may trigger additional + // parsing errors. Suppress them for test stability. + using (new DebugAssertSuppressor()) + { + try { reader.Dispose(); } catch { } + } + } + } + finally + { + _server.OnSQLBatchCompleted = null; + } + } + + /// + /// Writes a ROW token (0xD1) with a single TimeN column whose data length + /// byte is set to 11 (exceeding the maximum datetime wire size of 10 bytes). + /// + private sealed class MaliciousTimeRowToken : TDSPacketToken + { + public override bool Inflate(Stream source) => throw new NotSupportedException(); + + public override void Deflate(Stream destination) + { + // ROW token type + destination.WriteByte(0xD1); + + // TimeN data: length prefix (1 byte) = 11 (INVALID — max for time is 5, max datetime overall is 10) + destination.WriteByte(11); + + // Write 11 bytes of dummy data + destination.Write(new byte[11], 0, 11); + } + } + + /// + /// Temporarily suppresses Debug.Assert failures by clearing trace listeners. + /// Used when disposing resources after intentionally corrupting a TDS stream. + /// + private sealed class DebugAssertSuppressor : IDisposable + { + private readonly System.Diagnostics.TraceListener[] _listeners; + + public DebugAssertSuppressor() + { + _listeners = new System.Diagnostics.TraceListener[System.Diagnostics.Trace.Listeners.Count]; + System.Diagnostics.Trace.Listeners.CopyTo(_listeners, 0); + System.Diagnostics.Trace.Listeners.Clear(); + } + + public void Dispose() + { + System.Diagnostics.Trace.Listeners.AddRange(_listeners); + } + } } diff --git a/src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTdsServer.cs b/src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTdsServer.cs index 1eb8bdf8c6..c3fadc3e0a 100644 --- a/src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTdsServer.cs +++ b/src/Microsoft.Data.SqlClient/tests/tools/TDS/TDS.Servers/GenericTdsServer.cs @@ -156,6 +156,13 @@ public GenericTdsServer(T arguments, QueryEngine queryEngine) public OnAuthenticationCompletedDelegate OnAuthenticationResponseCompleted { private get; set; } + /// + /// Delegate invoked after a SQL batch response is prepared but before it is + /// sent to the client. Tests can use this to inject or replace tokens in the + /// response message. + /// + public Action OnSQLBatchCompleted { get; set; } + public OnLogin7ValidatedDelegate OnLogin7Validated { private get; set; } @@ -473,6 +480,9 @@ public virtual TDSMessageCollection OnSQLBatchRequest(ITDSServerSession session, session.PacketSize = (uint)Arguments.PacketSize; } + // Allow tests to modify or inject tokens into the response + OnSQLBatchCompleted?.Invoke(responseMessage[0]); + return responseMessage; } From c66f152110a110109ad480b06e7c92372dc06043 Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Thu, 4 Jun 2026 13:20:12 -0300 Subject: [PATCH 05/11] Add ReturnValue bounds check tests (oversized + PLP regression) - 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 --- .../TdsTokenBoundsTests.cs | 231 ++++++++++++++++++ 1 file changed, 231 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs index 067ca06f09..4bf0716766 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs @@ -590,6 +590,237 @@ public override void Deflate(Stream destination) } } + // ────────────────────────────────────────────────────────────────────────── + // Test 8: Post-login batch response — ReturnValue with oversized data length + // ────────────────────────────────────────────────────────────────────────── + + /// + /// Verifies that TryProcessReturnValue rejects a RETURNVALUE token (0xAC) + /// whose inner data length (for a non-PLP IMAGE type) exceeds int.MaxValue. + /// A malicious server can craft a TEXT/IMAGE return value with a spoofed int32 + /// data length that becomes a huge value when cast to ulong, triggering unbounded + /// allocation. + /// + [Fact] + public void BatchResponse_ReturnValue_OversizedLength_ThrowsParsingError() + { + _server.OnSQLBatchCompleted = responseMessage => + { + int doneIndex = responseMessage.FindIndex(t => t is TDSDoneToken); + if (doneIndex < 0) + { + doneIndex = responseMessage.Count; + } + + responseMessage.Insert(doneIndex, new MaliciousReturnValueToken()); + }; + + try + { + using SqlConnection connection = new(_connectionString); + connection.Open(); + + using SqlCommand command = connection.CreateCommand(); + command.CommandText = "SELECT 1"; + + using (new DebugAssertSuppressor()) + { + Exception ex = Assert.ThrowsAny( + () => command.ExecuteNonQuery()); + Assert.Contains("18", ex.Message); // CorruptedTdsStream + } + } + finally + { + _server.OnSQLBatchCompleted = null; + } + } + + /// + /// Writes a RETURNVALUE token (0xAC) with an IMAGE (0x22) type whose data + /// length field is set to -1 (0xFFFFFFFF). When cast to ulong this exceeds + /// int.MaxValue, triggering the bounds check in TryProcessReturnValue. + /// Wire layout: + /// [0xAC] token + /// [uint16] ordinal + /// [byte] param name length (0) + /// [byte] status + /// [uint32] user type + /// [byte] flags1 + /// [byte] flags2 + /// [byte] tds type = 0x22 (IMAGE) + /// [int32] max length + /// [byte] textPtrLen = 16 + /// [16 bytes] textPtr + /// [8 bytes] timestamp + /// [int32] data length = -1 (INVALID) + /// + private sealed class MaliciousReturnValueToken : TDSPacketToken + { + public override bool Inflate(Stream source) => throw new NotSupportedException(); + + public override void Deflate(Stream destination) + { + // RETURNVALUE token + destination.WriteByte(0xAC); + + // Ordinal (uint16 LE) + destination.WriteByte(0x00); + destination.WriteByte(0x00); + + // Param name length (byte) = 0 + destination.WriteByte(0x00); + + // Status (byte) = 0x01 (output parameter) + destination.WriteByte(0x01); + + // UserType (uint32 LE) = 0 + destination.Write(new byte[4], 0, 4); + + // Flags byte 1 (ignored) + destination.WriteByte(0x00); + + // Flags byte 2 + destination.WriteByte(0x00); + + // TDS type = SQLIMAGE (0x22) + destination.WriteByte(0x22); + + // MaxLen (int32 LE) — for IMAGE this is read via TryGetTokenLength + // which for 0x22 reads int32. Value doesn't matter much, just needs + // to be valid for MetaType lookup. + destination.Write(new byte[] { 0x10, 0x00, 0x00, 0x00 }, 0, 4); // 16 + + // -- TryProcessColumnHeaderNoNBC: IsLong && !IsPlp path -- + // TextPtr length (byte) = 16 + destination.WriteByte(0x10); + + // TextPtr data (16 bytes) + destination.Write(new byte[16], 0, 16); + + // Timestamp (8 bytes) + destination.Write(new byte[8], 0, 8); + + // Data length (int32 LE) = -1 (0xFFFFFFFF) + // (ulong)(-1) = 0xFFFFFFFFFFFFFFFF > int.MaxValue → triggers bounds check + destination.WriteByte(0xFF); + destination.WriteByte(0xFF); + destination.WriteByte(0xFF); + destination.WriteByte(0xFF); + } + } + + // ────────────────────────────────────────────────────────────────────────── + // Test 9: Post-login batch response — PLP ReturnValue (regression guard) + // ────────────────────────────────────────────────────────────────────────── + + /// + /// Verifies that TryProcessReturnValue correctly handles PLP + /// (Partially Length-Prefixed) return values without triggering the bounds + /// check. PLP types use the unknown-length sentinel (0xFFFFFFFFFFFFFFFE) + /// which must NOT be rejected by the non-PLP bounds check. + /// + [Fact] + public void BatchResponse_ReturnValue_PlpUnknownLength_Succeeds() + { + _server.OnSQLBatchCompleted = responseMessage => + { + int doneIndex = responseMessage.FindIndex(t => t is TDSDoneToken); + if (doneIndex < 0) + { + doneIndex = responseMessage.Count; + } + + responseMessage.Insert(doneIndex, new ValidPlpReturnValueToken()); + }; + + try + { + using SqlConnection connection = new(_connectionString); + connection.Open(); + + using SqlCommand command = connection.CreateCommand(); + command.CommandText = "SELECT 1"; + + // Should NOT throw — PLP unknown-length sentinel is valid + command.ExecuteNonQuery(); + } + finally + { + _server.OnSQLBatchCompleted = null; + } + } + + /// + /// Writes a valid RETURNVALUE token (0xAC) with a PLP VARBINARY(MAX) type + /// using the unknown-length sentinel (0xFFFFFFFFFFFFFFFE) followed by an + /// immediate PLP terminator (chunk length = 0). This exercises the IsPlp + /// branch in TryProcessReturnValue and must NOT trigger the bounds check. + /// Wire layout: + /// [0xAC] token + /// [uint16] ordinal + /// [byte] param name length (0) + /// [byte] status + /// [uint32] user type + /// [byte] flags1 + /// [byte] flags2 + /// [byte] tds type = 0xA5 (BIGVARBINARY) + /// [uint16] max length = 0xFFFF (PLP marker) + /// [uint64] PLP length = 0xFFFFFFFFFFFFFFFE (unknown) + /// [uint32] chunk length = 0 (terminator) + /// + private sealed class ValidPlpReturnValueToken : TDSPacketToken + { + public override bool Inflate(Stream source) => throw new NotSupportedException(); + + public override void Deflate(Stream destination) + { + // RETURNVALUE token + destination.WriteByte(0xAC); + + // Ordinal (uint16 LE) + destination.WriteByte(0x00); + destination.WriteByte(0x00); + + // Param name length (byte) = 0 + destination.WriteByte(0x00); + + // Status (byte) = 0x01 (output parameter) + destination.WriteByte(0x01); + + // UserType (uint32 LE) = 0 + destination.Write(new byte[4], 0, 4); + + // Flags byte 1 (ignored) + destination.WriteByte(0x00); + + // Flags byte 2 + destination.WriteByte(0x00); + + // TDS type = SQLBIGVARBINARY (0xA5) + destination.WriteByte(0xA5); + + // MaxLen (uint16 LE) = 0xFFFF — PLP marker + destination.WriteByte(0xFF); + destination.WriteByte(0xFF); + + // -- TryProcessColumnHeaderNoNBC: non-IsLong path -- + // TryGetDataLength → TryReadPlpLength: + // reads uint64 = 0xFFFFFFFFFFFFFFFE (unknown length sentinel) + destination.WriteByte(0xFE); + destination.WriteByte(0xFF); + destination.WriteByte(0xFF); + destination.WriteByte(0xFF); + destination.WriteByte(0xFF); + destination.WriteByte(0xFF); + destination.WriteByte(0xFF); + destination.WriteByte(0xFF); + + // PLP chunk terminator (uint32 = 0) — empty data + destination.Write(new byte[4], 0, 4); + } + } + /// /// Temporarily suppresses Debug.Assert failures by clearing trace listeners. /// Used when disposing resources after intentionally corrupting a TDS stream. From aad03713fcb12764db97fb3f291f502506015c08 Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Thu, 4 Jun 2026 16:07:15 -0300 Subject: [PATCH 06/11] Address Copilot review feedback - 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) --- .../Connection/SqlConnectionInternal.cs | 2 +- .../src/Microsoft/Data/SqlClient/TdsEnums.cs | 7 ++- .../src/Microsoft/Data/SqlClient/TdsParser.cs | 7 +-- .../FeatureExtAckBoundsTests.cs | 56 ++++++++++--------- .../TdsTokenBoundsTests.cs | 2 +- 5 files changed, 41 insertions(+), 33 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs index 2c531b5809..3d67df236e 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs @@ -1348,7 +1348,7 @@ internal void OnFeatureExtAck(int featureId, byte[] data) if (len < 0 || len > data.Length - i) { - throw SQL.ParsingError(ParsingErrorState.CorruptedTdsStream); + throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, len); } byte[] stateData = new byte[len]; diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsEnums.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsEnums.cs index c63c33c384..9b71afc350 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsEnums.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsEnums.cs @@ -84,14 +84,17 @@ internal static class TdsEnums public const int MAX_PACKET_SIZE = 32768; public const int MAX_SERVER_USER_NAME = 256; // obtained from luxor - // Maximum allowed data length for login-phase token payloads (feature ext ack, + // Maximum allowed data length for token payloads (feature ext ack, // session state, fedauth info). Prevents a malicious server from causing - // unbounded memory allocation on the client before authentication completes. + // 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 // Severity 11 - 16 indicates errors that can be corrected by user (syntax errors, etc...) // Severity 17 - 19 indicates failure due to insufficient resources in the server diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs index 337b0de49b..8e7233fba7 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs @@ -4940,7 +4940,7 @@ internal TdsOperationStatus TryProcessReturnValue(int length, } else if (valLen > (ulong)int.MaxValue) { - throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, int.MaxValue); + throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, unchecked((int)valLen)); } else { @@ -7141,13 +7141,12 @@ private TdsOperationStatus TryReadSqlDateTime(SqlBuffer value, byte tdsType, int { // 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. - const int MaxDateTimeLength = 10; - if (length < 0 || length > MaxDateTimeLength) + if (length < 0 || length > TdsEnums.MaxDateTimeLength) { throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, length); } - Span datetimeBuffer = stackalloc byte[MaxDateTimeLength]; + Span datetimeBuffer = stackalloc byte[TdsEnums.MaxDateTimeLength]; TdsOperationStatus result = stateObj.TryReadByteArray(datetimeBuffer, length); if (result != TdsOperationStatus.Done) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtAckBoundsTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtAckBoundsTests.cs index d5adc3b4c1..5e6385321b 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtAckBoundsTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtAckBoundsTests.cs @@ -78,44 +78,50 @@ public void FeatureExtAck_OversizedDataLength_ThrowsParsingError() } /// - /// Verifies that a FeatureExtAck token with a data length at or below the + /// Verifies that a FeatureExtAck token with a data length at exactly the /// allowed maximum is accepted without error, confirming there is no /// off-by-one in the bounds check. /// [Fact] public void FeatureExtAck_MaxAllowedDataLength_DoesNotThrow() { - // Arrange: inject a FeatureExtAck token at exactly the maximum allowed size. - // We use a small real payload to avoid actually allocating 1 MB in the test; - // the server will write a data length that equals the actual data length. - // This verifies that the boundary value is accepted (not off-by-one). - byte[] legitimateData = new byte[] { 0x01 }; // 1-byte GlobalTransactions ack - + // Arrange: inject a FeatureExtAck token whose declared data length equals + // MaxTokenDataLength exactly. The bounds check should NOT fire for this + // value. The connection will fail for other reasons (not enough data on + // the wire), but the error must NOT be state 18 (CorruptedTdsStream). _server.OnAuthenticationResponseCompleted = responseMessage => { - // Find existing FeatureExtAck or create one var existing = responseMessage.OfType().FirstOrDefault(); - if (existing == null) + if (existing != null) { - int doneIndex = responseMessage.FindIndex(t => t is TDSDoneToken); - if (doneIndex < 0) - { - doneIndex = responseMessage.Count; - } - - var token = new TDSFeatureExtAckToken( - new TDSFeatureExtAckGenericOption( - (TDSFeatureID)TdsEnums.FEATUREEXT_GLOBALTRANSACTIONS, - (uint)legitimateData.Length, - legitimateData)); - responseMessage.Insert(doneIndex, token); + responseMessage.Remove(existing); + } + + int doneIndex = responseMessage.FindIndex(t => t is TDSDoneToken); + if (doneIndex < 0) + { + doneIndex = responseMessage.Count; } + + // Insert token with dataLen = MaxTokenDataLength (at boundary, not over) + responseMessage.Insert(doneIndex, new MaliciousFeatureExtAckToken( + featureId: (TDSFeatureID)TdsEnums.FEATUREEXT_GLOBALTRANSACTIONS, + claimedDataLen: (uint)TdsEnums.MaxTokenDataLength)); }; - // Act & Assert: connection should open successfully - using SqlConnection connection = new(_connectionString); - connection.Open(); - Assert.Equal(System.Data.ConnectionState.Open, connection.State); + try + { + using SqlConnection connection = new(_connectionString); + // The connection will fail (insufficient data for the claimed length), + // but the failure must NOT be the bounds check (state 18 with MaxTokenDataLength+1). + Exception ex = Assert.ThrowsAny(connection.Open); + // Confirm it's NOT the bounds-check error (which would report MaxTokenDataLength+1) + Assert.DoesNotContain((TdsEnums.MaxTokenDataLength + 1).ToString(), ex.Message); + } + finally + { + _server.OnAuthenticationResponseCompleted = null; + } } /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs index 4bf0716766..a03a316e5f 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs @@ -507,7 +507,7 @@ public void BatchResponse_PromoteTransaction_OversizedLength_ThrowsParsingError( /// Verifies that TryReadSqlDateTime rejects a TIME column value whose /// data length byte exceeds the maximum datetime wire size (10 bytes). A /// malicious server could set this length to a large value, causing the - /// parser to attempt an oversized stackalloc. + /// parser to attempt an unbounded heap allocation. /// [Fact] public void BatchResponse_DateTime_OversizedLength_ThrowsParsingError() From e21e90ccac8280cbf61932511b193596e37c1b5d Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Fri, 5 Jun 2026 09:16:14 -0300 Subject: [PATCH 07/11] Add test for TryReadSqlValueInternal binary bounds check via sql_variant 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. --- .../TdsTokenBoundsTests.cs | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs index a03a316e5f..b90a1e01d0 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs @@ -821,6 +821,112 @@ public override void Deflate(Stream destination) } } + // ────────────────────────────────────────────────────────────────────────── + // Test 10: Post-login batch response — sql_variant with oversized binary + // ────────────────────────────────────────────────────────────────────────── + + /// + /// Verifies that TryReadSqlValueInternal rejects a binary value inside + /// a sql_variant column whose inner data length exceeds + /// (8000 bytes). The bounds check in the + /// sql_variant deserialization path prevents unbounded heap allocation. + /// + [Fact] + public void BatchResponse_SqlVariantBinary_OversizedLength_ThrowsParsingError() + { + _server.OnSQLBatchCompleted = responseMessage => + { + responseMessage.Clear(); + + // COLMETADATA: one SSVariant column + var metadata = new TDSColMetadataToken(); + var col = new TDSColumnData(); + col.DataType = TDSDataType.SSVariant; + col.DataTypeSpecific = (uint)8009; // max length for SSVariant + col.Flags.IsNullable = true; + col.Name = string.Empty; + metadata.Columns.Add(col); + responseMessage.Add(metadata); + + // ROW with a sql_variant containing oversized binary data + responseMessage.Add(new MaliciousSqlVariantBinaryRowToken()); + + // DONE + responseMessage.Add(new TDSDoneToken(TDSDoneTokenStatusType.Final | TDSDoneTokenStatusType.Count, TDSDoneTokenCommandType.Select, 1)); + }; + + try + { + using SqlConnection connection = new(_connectionString); + connection.Open(); + + using SqlCommand command = connection.CreateCommand(); + command.CommandText = "MALICIOUS_QUERY_NOT_RECOGNIZED"; + + SqlDataReader reader = command.ExecuteReader(); + try + { + Assert.True(reader.Read()); + Exception ex = Assert.ThrowsAny( + () => reader.GetValue(0)); + Assert.Contains("18", ex.Message); // CorruptedTdsStream + } + finally + { + using (new DebugAssertSuppressor()) + { + try { reader.Dispose(); } catch { } + } + } + } + finally + { + _server.OnSQLBatchCompleted = null; + } + } + + /// + /// Writes a ROW token (0xD1) with a single SSVariant column containing a + /// BigVarBinary variant whose inner data length exceeds MAXSIZE (8000). + /// Wire layout for the variant: + /// [int32] total variant length = 8005 + /// [byte] inner type = 0xA5 (BigVarBinary) + /// [byte] cbPropBytes = 2 + /// [ushort] maxLen (property) = 8001 + /// [8001 bytes would be data, but we only write 4 to trigger the check] + /// lenData = 8005 - 2(SQLVARIANT_SIZE) - 2(cbProps) = 8001 > MAXSIZE → throws + /// + private sealed class MaliciousSqlVariantBinaryRowToken : TDSPacketToken + { + public override bool Inflate(Stream source) => throw new NotSupportedException(); + + public override void Deflate(Stream destination) + { + // ROW token type + destination.WriteByte(0xD1); + + // SSVariant column data: total length (int32 LE) + // lenData = totalLength - SQLVARIANT_SIZE(2) - cbPropBytes(2) = totalLength - 4 + // We want lenData = 8001, so totalLength = 8005 + int totalLength = 8005; + byte[] lenBytes = BitConverter.GetBytes(totalLength); + destination.Write(lenBytes, 0, 4); + + // Inner type: BigVarBinary = 0xA5 + destination.WriteByte(0xA5); + + // cbPropBytes = 2 + destination.WriteByte(0x02); + + // Properties: maxLen (ushort) = 8001 + destination.WriteByte(0x41); // 8001 & 0xFF = 0x41 + destination.WriteByte(0x1F); // 8001 >> 8 = 0x1F + + // Write 4 bytes of dummy data (bounds check fires before trying to read 8001) + destination.Write(new byte[4], 0, 4); + } + } + /// /// Temporarily suppresses Debug.Assert failures by clearing trace listeners. /// Used when disposing resources after intentionally corrupting a TDS stream. From 9f3b490253cff88ecb61d81f0d67deff50200e69 Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Fri, 5 Jun 2026 11:27:33 -0300 Subject: [PATCH 08/11] Address second round of Copilot review feedback - 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 --- .../src/Microsoft/Data/SqlClient/TdsParser.cs | 7 +++--- .../FeatureExtAckBoundsTests.cs | 23 ++++++++++++------- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs index 8e7233fba7..a1fad331f3 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs @@ -3854,10 +3854,11 @@ private TdsOperationStatus TryProcessFeatureExtAck(TdsParserStateObject stateObj { throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, (int)dataLen); } - byte[] data = new byte[dataLen]; - if (dataLen > 0) + int dataLength = (int)dataLen; + byte[] data = new byte[dataLength]; + if (dataLength > 0) { - result = stateObj.TryReadByteArray(data, (int)dataLen); + result = stateObj.TryReadByteArray(data, dataLength); if (result != TdsOperationStatus.Done) { return result; diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtAckBoundsTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtAckBoundsTests.cs index 5e6385321b..56a2597d95 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtAckBoundsTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtAckBoundsTests.cs @@ -67,14 +67,21 @@ public void FeatureExtAck_OversizedDataLength_ThrowsParsingError() claimedDataLen: (uint)(TdsEnums.MaxTokenDataLength + 1))); }; - // Act & Assert: connection should fail with a parsing error, NOT an OutOfMemoryException - using SqlConnection connection = new(_connectionString); - Exception ex = Assert.ThrowsAny(connection.Open); - - // The exception message should indicate a corrupted TDS stream (parsing error state 18) - // with the oversized length value, not an OOM from attempting the allocation. - Assert.Contains("18", ex.Message); // ParsingErrorState.CorruptedTdsStream = 18 - Assert.Contains((TdsEnums.MaxTokenDataLength + 1).ToString(), ex.Message); + try + { + // Act & Assert: connection should fail with a parsing error, NOT an OutOfMemoryException + using SqlConnection connection = new(_connectionString); + Exception ex = Assert.ThrowsAny(connection.Open); + + // The exception message should indicate a corrupted TDS stream (parsing error state 18) + // with the oversized length value, not an OOM from attempting the allocation. + Assert.Contains("18", ex.Message); // ParsingErrorState.CorruptedTdsStream = 18 + Assert.Contains((TdsEnums.MaxTokenDataLength + 1).ToString(), ex.Message); + } + finally + { + _server.OnAuthenticationResponseCompleted = null; + } } /// From c93035386a00151dc818df5f04cfa5c4b64b8b7a Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Mon, 8 Jun 2026 12:23:45 -0300 Subject: [PATCH 09/11] Improve test quality: instance fixtures, tighter assertions, coverage 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 --- .../src/Microsoft/Data/SqlClient/TdsParser.cs | 4 + .../ConnectionEnhancedRoutingTests.cs | 2 + .../ConnectionFailoverTests.cs | 2 + .../ConnectionReadOnlyRoutingTests.cs | 8 +- .../ConnectionRoutingTests.cs | 4 +- .../ConnectionRoutingTestsAzure.cs | 8 +- .../FeatureExtAckBoundsTests.cs | 53 ++- .../FeatureExtensionNegotiationTests.cs | 2 + .../TdsTokenBoundsTests.cs | 361 +++++++++++------- 9 files changed, 266 insertions(+), 178 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs index a1fad331f3..fffe3339db 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs @@ -7138,6 +7138,10 @@ internal TdsOperationStatus TryReadSqlValue(SqlBuffer value, return TdsOperationStatus.Done; } + // length originates as a single byte on the wire (nullable datetime length prefix), + // but is kept as int to match the TDS parsing API surface where all lengths are int. + // Using byte here would require casts at all call sites and silently truncate values + // from the sql_variant path where lenData is computed arithmetically. private TdsOperationStatus TryReadSqlDateTime(SqlBuffer value, byte tdsType, int length, byte scale, TdsParserStateObject stateObj) { // DateTimeOffset is the largest datetime type at 10 bytes (5 time + 3 date + 2 offset). diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionEnhancedRoutingTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionEnhancedRoutingTests.cs index 7d8941a07d..c85365d98e 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionEnhancedRoutingTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionEnhancedRoutingTests.cs @@ -14,6 +14,8 @@ namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests; /// /// Tests connection routing using the enhanced routing feature extension and envchange token /// +// TODO: Do we need this collection? It serializes all tests within it, which we probably don't +// need since each test uses its own TDS Server with ephemeral listen port. [Collection("SimulatedServerTests")] public class ConnectionEnhancedRoutingTests { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionFailoverTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionFailoverTests.cs index ea3e2d3da7..183c557292 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionFailoverTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionFailoverTests.cs @@ -12,6 +12,8 @@ namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests { + // TODO: Do we need this collection? It serializes all tests within it, which we probably don't + // need since each test uses its own TDS Server with ephemeral listen port. [Collection("SimulatedServerTests")] public class ConnectionFailoverTests { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionReadOnlyRoutingTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionReadOnlyRoutingTests.cs index f0618ac269..8220871f22 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionReadOnlyRoutingTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionReadOnlyRoutingTests.cs @@ -11,6 +11,8 @@ namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests { + // TODO: Do we need this collection? It serializes all tests within it, which we probably don't + // need since each test uses its own TDS Server with ephemeral listen port. [Collection("SimulatedServerTests")] public class ConnectionReadOnlyRoutingTests { @@ -71,7 +73,7 @@ public void RecursivelyRoutedConnection(int layers) router.Start(); routingLayers.Push(router); lastEndpoint = router.EndPoint; - lastConnectionString = (new SqlConnectionStringBuilder() { + lastConnectionString = (new SqlConnectionStringBuilder() { DataSource = $"localhost,{lastEndpoint.Port}", ApplicationIntent = ApplicationIntent.ReadOnly, Encrypt = false @@ -114,8 +116,8 @@ public async Task RecursivelyRoutedAsyncConnection(int layers) router.Start(); routingLayers.Push(router); lastEndpoint = router.EndPoint; - lastConnectionString = (new SqlConnectionStringBuilder() { - DataSource = $"localhost,{lastEndpoint.Port}", + lastConnectionString = (new SqlConnectionStringBuilder() { + DataSource = $"localhost,{lastEndpoint.Port}", ApplicationIntent = ApplicationIntent.ReadOnly, Encrypt = false }).ConnectionString; diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionRoutingTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionRoutingTests.cs index 0eff08139f..3065f780bc 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionRoutingTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionRoutingTests.cs @@ -10,6 +10,8 @@ namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests { + // TODO: Do we need this collection? It serializes all tests within it, which we probably don't + // need since each test uses its own TDS Server with ephemeral listen port. [Collection("SimulatedServerTests")] public class ConnectionRoutingTests { @@ -194,7 +196,7 @@ public void NetworkTimeoutAtRoutedLocation_RetryDisabled_ShouldFail() // Act var e = Assert.Throws(connection.Open); - // Assert + // Assert Assert.Equal(ConnectionState.Closed, connection.State); Assert.Contains("Connection Timeout Expired", e.Message); } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionRoutingTestsAzure.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionRoutingTestsAzure.cs index 3cd2628300..642d54f5b2 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionRoutingTestsAzure.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionRoutingTestsAzure.cs @@ -10,6 +10,8 @@ namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests { + // TODO: Do we need this collection? It serializes all tests within it, which we probably don't + // need since each test uses its own TDS Server with ephemeral listen port. [Collection("SimulatedServerTests")] public class ConnectionRoutingTestsAzure : IDisposable { @@ -21,8 +23,8 @@ public ConnectionRoutingTestsAzure() adpHelper.AddAzureSqlServerEndpoint("localhost"); } - public void Dispose() - { + public void Dispose() + { adpHelper.Dispose(); } @@ -192,7 +194,7 @@ public void NetworkTimeoutAtRoutedLocation_RetryDisabled_ShouldFail() // Act var e = Assert.Throws(connection.Open); - // Assert + // Assert Assert.Equal(ConnectionState.Closed, connection.State); Assert.Contains("Connection Timeout Expired", e.Message); } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtAckBoundsTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtAckBoundsTests.cs index 56a2597d95..20c94c0361 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtAckBoundsTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtAckBoundsTests.cs @@ -18,15 +18,19 @@ namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests; /// with data lengths exceeding protocol-reasonable bounds. This prevents a /// malicious server from causing unbounded memory allocation on the client. /// +// TODO: Do we need this collection? It serializes all tests within it, which we probably don't +// need since each test uses its own TDS Server with ephemeral listen port. [Collection("SimulatedServerTests")] -public class FeatureExtAckBoundsTests : IClassFixture +public class FeatureExtAckBoundsTests : IDisposable { + private readonly TdsServerFixture _fixture; private readonly TdsServer _server; private readonly string _connectionString; - public FeatureExtAckBoundsTests(TdsServerFixture fixture) + public FeatureExtAckBoundsTests() { - _server = fixture.TdsServer; + _fixture = new TdsServerFixture(); + _server = _fixture.TdsServer; SqlConnectionStringBuilder builder = new() { DataSource = $"localhost,{_server.EndPoint.Port}", @@ -36,6 +40,8 @@ public FeatureExtAckBoundsTests(TdsServerFixture fixture) _connectionString = builder.ConnectionString; } + public void Dispose() => _fixture.Dispose(); + /// /// Verifies that the TDS parser rejects a FeatureExtAck token whose data length /// field exceeds (1 MB), throwing a @@ -67,21 +73,14 @@ public void FeatureExtAck_OversizedDataLength_ThrowsParsingError() claimedDataLen: (uint)(TdsEnums.MaxTokenDataLength + 1))); }; - try - { - // Act & Assert: connection should fail with a parsing error, NOT an OutOfMemoryException - using SqlConnection connection = new(_connectionString); - Exception ex = Assert.ThrowsAny(connection.Open); - - // The exception message should indicate a corrupted TDS stream (parsing error state 18) - // with the oversized length value, not an OOM from attempting the allocation. - Assert.Contains("18", ex.Message); // ParsingErrorState.CorruptedTdsStream = 18 - Assert.Contains((TdsEnums.MaxTokenDataLength + 1).ToString(), ex.Message); - } - finally - { - _server.OnAuthenticationResponseCompleted = null; - } + // Act & Assert: connection should fail with a parsing error, NOT an OutOfMemoryException + using SqlConnection connection = new(_connectionString); + Exception ex = Assert.ThrowsAny(connection.Open); + + // The exception message should indicate a corrupted TDS stream + // with the oversized length value, not an OOM from attempting the allocation. + Assert.Contains("Error state: 18", ex.Message); // ParsingErrorState.CorruptedTdsStream = 18 + Assert.Contains($"Length: {TdsEnums.MaxTokenDataLength + 1}", ex.Message); } /// @@ -116,19 +115,11 @@ public void FeatureExtAck_MaxAllowedDataLength_DoesNotThrow() claimedDataLen: (uint)TdsEnums.MaxTokenDataLength)); }; - try - { - using SqlConnection connection = new(_connectionString); - // The connection will fail (insufficient data for the claimed length), - // but the failure must NOT be the bounds check (state 18 with MaxTokenDataLength+1). - Exception ex = Assert.ThrowsAny(connection.Open); - // Confirm it's NOT the bounds-check error (which would report MaxTokenDataLength+1) - Assert.DoesNotContain((TdsEnums.MaxTokenDataLength + 1).ToString(), ex.Message); - } - finally - { - _server.OnAuthenticationResponseCompleted = null; - } + using SqlConnection connection = new(_connectionString); + // The connection will fail (insufficient data for the claimed length), + // but the failure must NOT be the bounds-check error. + Exception ex = Assert.ThrowsAny(connection.Open); + Assert.DoesNotContain("Error state: 18", ex.Message); } /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtensionNegotiationTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtensionNegotiationTests.cs index e6342c4fa8..0141d44000 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtensionNegotiationTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/FeatureExtensionNegotiationTests.cs @@ -13,6 +13,8 @@ namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests; +// Serializes execution with other SimulatedServerTests classes to avoid port/resource conflicts. +// Required here because IClassFixture shares a single TdsServer instance across all tests in this class. [Collection("SimulatedServerTests")] public class FeatureExtensionNegotiationTests : IClassFixture { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs index b90a1e01d0..fc4fc89a1d 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs @@ -20,15 +20,19 @@ namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests; /// exceeding protocol-reasonable bounds, preventing unbounded memory allocation /// from a malicious server. /// +// TODO: Do we need this collection? It serializes all tests within it, which we probably don't +// need since each test uses its own TDS Server with ephemeral listen port. [Collection("SimulatedServerTests")] -public class TdsTokenBoundsTests : IClassFixture +public class TdsTokenBoundsTests : IDisposable { + private readonly TdsServerFixture _fixture; private readonly TdsServer _server; private readonly string _connectionString; - public TdsTokenBoundsTests(TdsServerFixture fixture) + public TdsTokenBoundsTests() { - _server = fixture.TdsServer; + _fixture = new TdsServerFixture(); + _server = _fixture.TdsServer; SqlConnectionStringBuilder builder = new() { DataSource = $"localhost,{_server.EndPoint.Port}", @@ -38,6 +42,8 @@ public TdsTokenBoundsTests(TdsServerFixture fixture) _connectionString = builder.ConnectionString; } + public void Dispose() => _fixture.Dispose(); + // ────────────────────────────────────────────────────────────────────────── // Test 1: SessionState token with oversized total length // ────────────────────────────────────────────────────────────────────────── @@ -63,16 +69,10 @@ public void SessionState_OversizedTotalLength_ThrowsParsingError() claimedTotalLength: (uint)(TdsEnums.MaxTokenDataLength + 1))); }; - try - { - using SqlConnection connection = new(_connectionString); - Exception ex = Assert.ThrowsAny(connection.Open); - Assert.Contains("18", ex.Message); // CorruptedTdsStream - } - finally - { - _server.OnAuthenticationResponseCompleted = null; - } + using SqlConnection connection = new(_connectionString); + Exception ex = Assert.ThrowsAny(connection.Open); + Assert.Contains("Error state: 18", ex.Message); // CorruptedTdsStream + Assert.Contains($"Length: {TdsEnums.MaxTokenDataLength + 1}", ex.Message); } // ────────────────────────────────────────────────────────────────────────── @@ -101,16 +101,42 @@ public void SessionState_OversizedInnerOptionLength_ThrowsParsingError() innerClaimedLength: TdsEnums.MaxTokenDataLength + 1)); }; - try - { - using SqlConnection connection = new(_connectionString); - Exception ex = Assert.ThrowsAny(connection.Open); - Assert.Contains("18", ex.Message); // CorruptedTdsStream - } - finally + using SqlConnection connection = new(_connectionString); + Exception ex = Assert.ThrowsAny(connection.Open); + Assert.Contains("Error state: 18", ex.Message); // CorruptedTdsStream + Assert.Contains($"Length: {TdsEnums.MaxTokenDataLength + 1}", ex.Message); + } + + // ────────────────────────────────────────────────────────────────────────── + // Test 2b: SessionState token with negative inner option length + // ────────────────────────────────────────────────────────────────────────── + + /// + /// Verifies that TryProcessSessionState rejects an individual session state + /// option whose inner data length (encoded via the 0xFF + DWORD path) is negative, + /// which would be interpreted as a huge unsigned value if not bounds-checked. + /// + [Fact] + public void SessionState_NegativeInnerOptionLength_ThrowsParsingError() + { + _server.OnAuthenticationResponseCompleted = responseMessage => { - _server.OnAuthenticationResponseCompleted = null; - } + int doneIndex = responseMessage.FindIndex(t => t is TDSDoneToken); + if (doneIndex < 0) + { + doneIndex = responseMessage.Count; + } + + // Inject a SessionState token with an inner state option claiming + // a negative data length (-1 = 0xFFFFFFFF as uint32) + responseMessage.Insert(doneIndex, new MaliciousSessionStateInnerLenToken( + innerClaimedLength: -1)); + }; + + using SqlConnection connection = new(_connectionString); + Exception ex = Assert.ThrowsAny(connection.Open); + Assert.Contains("Error state: 18", ex.Message); // CorruptedTdsStream + Assert.Contains("Length: -1", ex.Message); } // ────────────────────────────────────────────────────────────────────────── @@ -146,16 +172,10 @@ public void SRecovery_MalformedInnerStateLength_ThrowsParsingError() responseMessage.Insert(doneIndex, new MaliciousSRecoveryFeatureExtAckToken()); }; - try - { - using SqlConnection connection = new(_connectionString); - Exception ex = Assert.ThrowsAny(connection.Open); - Assert.Contains("18", ex.Message); // CorruptedTdsStream - } - finally - { - _server.OnAuthenticationResponseCompleted = null; - } + using SqlConnection connection = new(_connectionString); + Exception ex = Assert.ThrowsAny(connection.Open); + Assert.Contains("Error state: 18", ex.Message); // CorruptedTdsStream + Assert.Contains("Length: 999", ex.Message); } // ────────────────────────────────────────────────────────────────────────── @@ -185,16 +205,10 @@ public void FedAuthInfo_OversizedTokenLength_ThrowsParsingError() claimedLength: TdsEnums.MaxTokenDataLength + 1)); }; - try - { - using SqlConnection connection = new(_connectionString); - Exception ex = Assert.ThrowsAny(connection.Open); - Assert.Contains("18", ex.Message); // CorruptedTdsStream - } - finally - { - _server.OnAuthenticationResponseCompleted = null; - } + using SqlConnection connection = new(_connectionString); + Exception ex = Assert.ThrowsAny(connection.Open); + Assert.Contains("Error state: 18", ex.Message); // CorruptedTdsStream + Assert.Contains($"Length: {TdsEnums.MaxTokenDataLength + 1}", ex.Message); } // ────────────────────────────────────────────────────────────────────────── @@ -224,16 +238,10 @@ public void EnvChange_PromoteTransaction_OversizedLength_ThrowsParsingError() claimedNewLength: TdsEnums.MaxPromoteTransactionLength + 1)); }; - try - { - using SqlConnection connection = new(_connectionString); - Exception ex = Assert.ThrowsAny(connection.Open); - Assert.Contains("18", ex.Message); // CorruptedTdsStream - } - finally - { - _server.OnAuthenticationResponseCompleted = null; - } + using SqlConnection connection = new(_connectionString); + Exception ex = Assert.ThrowsAny(connection.Open); + Assert.Contains("Error state: 18", ex.Message); // CorruptedTdsStream + Assert.Contains($"Length: {TdsEnums.MaxPromoteTransactionLength + 1}", ex.Message); } // ══════════════════════════════════════════════════════════════════════════ @@ -481,22 +489,16 @@ public void BatchResponse_PromoteTransaction_OversizedLength_ThrowsParsingError( claimedNewLength: TdsEnums.MaxPromoteTransactionLength + 1)); }; - try - { - using SqlConnection connection = new(_connectionString); - connection.Open(); + using SqlConnection connection = new(_connectionString); + connection.Open(); - using SqlCommand command = connection.CreateCommand(); - command.CommandText = "SELECT 1"; + using SqlCommand command = connection.CreateCommand(); + command.CommandText = "SELECT 1"; - Exception ex = Assert.ThrowsAny( - () => command.ExecuteNonQuery()); - Assert.Contains("18", ex.Message); // CorruptedTdsStream - } - finally - { - _server.OnSQLBatchCompleted = null; - } + Exception ex = Assert.ThrowsAny( + () => command.ExecuteNonQuery()); + Assert.Contains("Error state: 18", ex.Message); // CorruptedTdsStream + Assert.Contains($"Length: {TdsEnums.MaxPromoteTransactionLength + 1}", ex.Message); } // ────────────────────────────────────────────────────────────────────────── @@ -535,37 +537,32 @@ public void BatchResponse_DateTime_OversizedLength_ThrowsParsingError() responseMessage.Add(new TDSDoneToken(TDSDoneTokenStatusType.Final | TDSDoneTokenStatusType.Count, TDSDoneTokenCommandType.Select, 1)); }; - try - { - using SqlConnection connection = new(_connectionString); - connection.Open(); + using SqlConnection connection = new(_connectionString); + connection.Open(); - using SqlCommand command = connection.CreateCommand(); - command.CommandText = "MALICIOUS_QUERY_NOT_RECOGNIZED"; + using SqlCommand command = connection.CreateCommand(); + command.CommandText = "MALICIOUS_QUERY_NOT_RECOGNIZED"; - SqlDataReader reader = command.ExecuteReader(); - try - { - // Read() returns true (data is ready) but doesn't actually parse the - // column values — that happens on GetValue/GetTimeSpan. Force the read. - Assert.True(reader.Read()); - Exception ex = Assert.ThrowsAny( - () => reader.GetValue(0)); - Assert.Contains("18", ex.Message); // CorruptedTdsStream - } - finally - { - // After corrupting the stream, dispose may trigger additional - // parsing errors. Suppress them for test stability. - using (new DebugAssertSuppressor()) - { - try { reader.Dispose(); } catch { } - } - } + SqlDataReader reader = command.ExecuteReader(); + try + { + // Read() returns true (data is ready) but doesn't actually parse the + // column values — that happens on GetValue/GetTimeSpan. Force the read. + Assert.True(reader.Read()); + Exception ex = Assert.ThrowsAny( + () => reader.GetValue(0)); + Assert.Contains("Error state: 18", ex.Message); // CorruptedTdsStream + Assert.Contains("Length: 11", ex.Message); } finally { - _server.OnSQLBatchCompleted = null; + // Disposing the reader after a corrupted stream causes the driver to + // attempt further TDS parsing during teardown, which can trip unrelated + // Debug.Assert calls in TdsParser. + using (new DebugAssertSuppressor()) + { + try { reader.Dispose(); } catch { } + } } } @@ -615,24 +612,21 @@ public void BatchResponse_ReturnValue_OversizedLength_ThrowsParsingError() responseMessage.Insert(doneIndex, new MaliciousReturnValueToken()); }; - try - { - using SqlConnection connection = new(_connectionString); - connection.Open(); + using SqlConnection connection = new(_connectionString); + connection.Open(); - using SqlCommand command = connection.CreateCommand(); - command.CommandText = "SELECT 1"; + using SqlCommand command = connection.CreateCommand(); + command.CommandText = "SELECT 1"; - using (new DebugAssertSuppressor()) - { - Exception ex = Assert.ThrowsAny( - () => command.ExecuteNonQuery()); - Assert.Contains("18", ex.Message); // CorruptedTdsStream - } - } - finally + // The malicious RETURNVALUE token corrupts parser state before the + // exception propagates. Suppress Debug.Assert calls that fire in + // TdsParser during error handling and connection teardown. + using (new DebugAssertSuppressor()) { - _server.OnSQLBatchCompleted = null; + Exception ex = Assert.ThrowsAny( + () => command.ExecuteNonQuery()); + Assert.Contains("Error state: 18", ex.Message); // CorruptedTdsStream + Assert.Contains("Length: -1", ex.Message); } } @@ -734,21 +728,14 @@ public void BatchResponse_ReturnValue_PlpUnknownLength_Succeeds() responseMessage.Insert(doneIndex, new ValidPlpReturnValueToken()); }; - try - { - using SqlConnection connection = new(_connectionString); - connection.Open(); + using SqlConnection connection = new(_connectionString); + connection.Open(); - using SqlCommand command = connection.CreateCommand(); - command.CommandText = "SELECT 1"; + using SqlCommand command = connection.CreateCommand(); + command.CommandText = "SELECT 1"; - // Should NOT throw — PLP unknown-length sentinel is valid - command.ExecuteNonQuery(); - } - finally - { - _server.OnSQLBatchCompleted = null; - } + // Should NOT throw — PLP unknown-length sentinel is valid + command.ExecuteNonQuery(); } /// @@ -855,33 +842,91 @@ public void BatchResponse_SqlVariantBinary_OversizedLength_ThrowsParsingError() responseMessage.Add(new TDSDoneToken(TDSDoneTokenStatusType.Final | TDSDoneTokenStatusType.Count, TDSDoneTokenCommandType.Select, 1)); }; - try - { - using SqlConnection connection = new(_connectionString); - connection.Open(); + using SqlConnection connection = new(_connectionString); + connection.Open(); - using SqlCommand command = connection.CreateCommand(); - command.CommandText = "MALICIOUS_QUERY_NOT_RECOGNIZED"; + using SqlCommand command = connection.CreateCommand(); + command.CommandText = "MALICIOUS_QUERY_NOT_RECOGNIZED"; - SqlDataReader reader = command.ExecuteReader(); - try - { - Assert.True(reader.Read()); - Exception ex = Assert.ThrowsAny( - () => reader.GetValue(0)); - Assert.Contains("18", ex.Message); // CorruptedTdsStream - } - finally + SqlDataReader reader = command.ExecuteReader(); + try + { + Assert.True(reader.Read()); + Exception ex = Assert.ThrowsAny( + () => reader.GetValue(0)); + Assert.Contains("Error state: 18", ex.Message); // CorruptedTdsStream + Assert.Contains("Length: 8001", ex.Message); + } + finally + { + // Disposing the reader after a corrupted stream causes the driver to + // attempt further TDS parsing during teardown, which can trip unrelated + // Debug.Assert calls in TdsParser. + using (new DebugAssertSuppressor()) { - using (new DebugAssertSuppressor()) - { - try { reader.Dispose(); } catch { } - } + try { reader.Dispose(); } catch { } } } + } + + // ────────────────────────────────────────────────────────────────────────── + // Test 11: Post-login batch response — sql_variant with negative inner length + // ────────────────────────────────────────────────────────────────────────── + + /// + /// Verifies that TryReadSqlValueInternal rejects a sql_variant binary + /// value whose declared total length is too small for the type overhead, + /// causing the computed inner data length to be negative. The bounds check + /// if (length < 0 || length > TdsEnums.MAXSIZE) catches this. + /// + [Fact] + public void BatchResponse_SqlVariantBinary_NegativeLength_ThrowsParsingError() + { + _server.OnSQLBatchCompleted = responseMessage => + { + responseMessage.Clear(); + + // COLMETADATA: one SSVariant column + var metadata = new TDSColMetadataToken(); + var col = new TDSColumnData(); + col.DataType = TDSDataType.SSVariant; + col.DataTypeSpecific = (uint)8009; // max length for SSVariant + col.Flags.IsNullable = true; + col.Name = string.Empty; + metadata.Columns.Add(col); + responseMessage.Add(metadata); + + // ROW with a sql_variant claiming a total length too small for its overhead + responseMessage.Add(new NegativeLenSqlVariantBinaryRowToken()); + + // DONE + responseMessage.Add(new TDSDoneToken(TDSDoneTokenStatusType.Final | TDSDoneTokenStatusType.Count, TDSDoneTokenCommandType.Select, 1)); + }; + + using SqlConnection connection = new(_connectionString); + connection.Open(); + + using SqlCommand command = connection.CreateCommand(); + command.CommandText = "MALICIOUS_QUERY_NOT_RECOGNIZED"; + + SqlDataReader reader = command.ExecuteReader(); + try + { + Assert.True(reader.Read()); + Exception ex = Assert.ThrowsAny( + () => reader.GetValue(0)); + Assert.Contains("Error state: 18", ex.Message); // CorruptedTdsStream + Assert.Contains("Length: -1", ex.Message); + } finally { - _server.OnSQLBatchCompleted = null; + // Disposing the reader after a corrupted stream causes the driver to + // attempt further TDS parsing during teardown, which can trip unrelated + // Debug.Assert calls in TdsParser. + using (new DebugAssertSuppressor()) + { + try { reader.Dispose(); } catch { } + } } } @@ -927,6 +972,42 @@ public override void Deflate(Stream destination) } } + /// + /// Writes a ROW token (0xD1) with a single SSVariant column containing a + /// BigVarBinary variant whose total length (3) is too small to cover the + /// type overhead (SQLVARIANT_SIZE=2 + cbPropBytes=2 = 4), producing a + /// negative computed inner data length: lenData = 3 - 4 = -1. + /// + private sealed class NegativeLenSqlVariantBinaryRowToken : TDSPacketToken + { + public override bool Inflate(Stream source) => throw new NotSupportedException(); + + public override void Deflate(Stream destination) + { + // ROW token type + destination.WriteByte(0xD1); + + // SSVariant column data: total length (int32 LE) + // lenConsumed = SQLVARIANT_SIZE(2) + cbPropBytes(2) = 4 + // lenData = totalLength - lenConsumed = 3 - 4 = -1 → triggers < 0 check + int totalLength = 3; + byte[] lenBytes = BitConverter.GetBytes(totalLength); + destination.Write(lenBytes, 0, 4); + + // Inner type: BigVarBinary = 0xA5 + destination.WriteByte(0xA5); + + // cbPropBytes = 2 + destination.WriteByte(0x02); + + // Properties: maxLen (ushort) = 100 (arbitrary, just need 2 bytes) + destination.WriteByte(0x64); // 100 & 0xFF + destination.WriteByte(0x00); // 100 >> 8 + + // No data bytes — the bounds check fires before attempting to read + } + } + /// /// Temporarily suppresses Debug.Assert failures by clearing trace listeners. /// Used when disposing resources after intentionally corrupting a TDS stream. From 3bb59e9f675903e4935e1ab753867dc27d0db256 Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Mon, 8 Jun 2026 12:41:47 -0300 Subject: [PATCH 10/11] Address Copilot review: uint overload for ParsingErrorLength, fix DebugAssertSuppressor - 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). --- .../src/Microsoft/Data/SqlClient/SqlUtil.cs | 4 ++++ .../src/Microsoft/Data/SqlClient/TdsParser.cs | 2 +- .../SimulatedServerTests/TdsTokenBoundsTests.cs | 14 ++++++++++---- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs index 96312a491d..84667d99e7 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs @@ -731,6 +731,10 @@ internal static Exception ParsingErrorLength(ParsingErrorState state, int length { return ADP.InvalidOperation(StringsHelper.GetString(Strings.SQL_ParsingErrorLength, ((int)state).ToString(CultureInfo.InvariantCulture), length)); } + internal static Exception ParsingErrorLength(ParsingErrorState state, uint length) + { + return ADP.InvalidOperation(StringsHelper.GetString(Strings.SQL_ParsingErrorLength, ((int)state).ToString(CultureInfo.InvariantCulture), length)); + } internal static Exception ParsingErrorStatus(ParsingErrorState state, int status) { return ADP.InvalidOperation(StringsHelper.GetString(Strings.SQL_ParsingErrorStatus, ((int)state).ToString(CultureInfo.InvariantCulture), status)); diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs index fffe3339db..79c725c074 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs @@ -3852,7 +3852,7 @@ private TdsOperationStatus TryProcessFeatureExtAck(TdsParserStateObject stateObj } if (dataLen > (uint)TdsEnums.MaxTokenDataLength) { - throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, (int)dataLen); + throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, dataLen); } int dataLength = (int)dataLen; byte[] data = new byte[dataLength]; diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs index fc4fc89a1d..e701631369 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs @@ -20,8 +20,9 @@ namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests; /// exceeding protocol-reasonable bounds, preventing unbounded memory allocation /// from a malicious server. /// -// TODO: Do we need this collection? It serializes all tests within it, which we probably don't -// need since each test uses its own TDS Server with ephemeral listen port. +// Serializes execution with other SimulatedServerTests classes. Required here because +// DebugAssertSuppressor mutates the global Trace.Listeners collection, which is not +// safe to do concurrently with other tests that may trigger Debug.Assert. [Collection("SimulatedServerTests")] public class TdsTokenBoundsTests : IDisposable { @@ -1009,15 +1010,18 @@ public override void Deflate(Stream destination) } /// - /// Temporarily suppresses Debug.Assert failures by clearing trace listeners. - /// Used when disposing resources after intentionally corrupting a TDS stream. + /// Temporarily suppresses Debug.Assert failures by clearing trace listeners. Used when + /// disposing resources after intentionally corrupting a TDS stream. A static lock serializes + /// access for the lifetime of the instance because Trace.Listeners is a global collection. /// private sealed class DebugAssertSuppressor : IDisposable { + private static readonly object s_listenerLock = new(); private readonly System.Diagnostics.TraceListener[] _listeners; public DebugAssertSuppressor() { + System.Threading.Monitor.Enter(s_listenerLock); _listeners = new System.Diagnostics.TraceListener[System.Diagnostics.Trace.Listeners.Count]; System.Diagnostics.Trace.Listeners.CopyTo(_listeners, 0); System.Diagnostics.Trace.Listeners.Clear(); @@ -1025,7 +1029,9 @@ public DebugAssertSuppressor() public void Dispose() { + System.Diagnostics.Trace.Listeners.Clear(); System.Diagnostics.Trace.Listeners.AddRange(_listeners); + System.Threading.Monitor.Exit(s_listenerLock); } } } From 1918e49feb554f61bcf4aee73cb24642e1218f21 Mon Sep 17 00:00:00 2001 From: Paul Medynski <31868385+paulmedynski@users.noreply.github.com> Date: Mon, 8 Jun 2026 12:48:20 -0300 Subject: [PATCH 11/11] Make DebugAssertSuppressor exception-safe 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. --- .../TdsTokenBoundsTests.cs | 26 ++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs index e701631369..ef975dccb3 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/TdsTokenBoundsTests.cs @@ -1022,16 +1022,30 @@ private sealed class DebugAssertSuppressor : IDisposable public DebugAssertSuppressor() { System.Threading.Monitor.Enter(s_listenerLock); - _listeners = new System.Diagnostics.TraceListener[System.Diagnostics.Trace.Listeners.Count]; - System.Diagnostics.Trace.Listeners.CopyTo(_listeners, 0); - System.Diagnostics.Trace.Listeners.Clear(); + try + { + _listeners = new System.Diagnostics.TraceListener[System.Diagnostics.Trace.Listeners.Count]; + System.Diagnostics.Trace.Listeners.CopyTo(_listeners, 0); + System.Diagnostics.Trace.Listeners.Clear(); + } + catch + { + System.Threading.Monitor.Exit(s_listenerLock); + throw; + } } public void Dispose() { - System.Diagnostics.Trace.Listeners.Clear(); - System.Diagnostics.Trace.Listeners.AddRange(_listeners); - System.Threading.Monitor.Exit(s_listenerLock); + try + { + System.Diagnostics.Trace.Listeners.Clear(); + System.Diagnostics.Trace.Listeners.AddRange(_listeners); + } + finally + { + System.Threading.Monitor.Exit(s_listenerLock); + } } } }