[7.0.2 Cherry-pick] Add TDS token data length bounds checks (#4340) (#4358)

Co-authored-by: Paul Medynski <31868385+paulmedynski@users.noreply.github.com>

Author: github-actions[bot]

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Connection/SqlConnectionInternal.cs

@@ -1354,6 +1354,11 @@ internal void OnFeatureExtAck(int featureId, byte[] data)
                             len = bLen;
                         }
 
+                        if (len < 0 || len > data.Length - i)
+                        {
+                            throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, len);
+                        }
+
                         byte[] stateData = new byte[len];
                         Buffer.BlockCopy(data, i, stateData, 0, len);
                         i += len;

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs

@@ -746,6 +746,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));

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsEnums.cs

@@ -84,6 +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 token payloads (feature ext ack,
+        // session state, fedauth info). Prevents a malicious server from causing
+        // unbounded memory allocation via spoofed token length fields.
+        internal const int MaxTokenDataLength = 1 << 20; // 1 MB
+
+        // Maximum allowed data length for a DTC promote transaction propagation token.
+        internal const int MaxPromoteTransactionLength = 1 << 16; // 64 KB
+
+        // Maximum valid wire size for datetime types (DateTimeOffset = 5 time + 3 date + 2 offset).
+        internal const int MaxDateTimeLength = 10;
+
         // Severity  0 - 10 indicates informational (non-error) messages
         // 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

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs

@@ -208,7 +208,7 @@ static TdsParser()
         {
             // For CoreCLR, we need to register the ANSI Code Page encoding provider before attempting to get an Encoding from a CodePage
             // For a default installation of SqlServer the encoding exchanged during Login is 1252. This encoding is not loaded by default
-            // See Remarks at https://msdn.microsoft.com/en-us/library/system.text.encodingprovider(v=vs.110).aspx 
+            // See Remarks at https://msdn.microsoft.com/en-us/library/system.text.encodingprovider(v=vs.110).aspx
             // SqlClient needs to register the encoding providers to make sure that even basic scenarios work with Sql Server.
             Encoding.RegisterProvider(CodePagesEncodingProvider.Instance);
         }
@@ -683,7 +683,7 @@ internal void RemoveEncryption()
 
             // create a new packet encryption changes the internal packet size Bug# 228403
             _physicalStateObj.ClearAllWritePackets();
-        }   
+        }
 
         internal void EnableMars()
         {
@@ -1376,11 +1376,11 @@ internal void TdsLogin(
                 int feOffset = length;
                 // calculate and reserve the required bytes for the featureEx
                 length = ApplyFeatureExData(
-                    requestedFeatures, 
-                    recoverySessionData, 
+                    requestedFeatures,
+                    recoverySessionData,
                     fedAuthFeatureExtensionData,
                     UserAgent.Ucs2Bytes,
-                    useFeatureExt, 
+                    useFeatureExt,
                     length
                     );
 
@@ -2792,7 +2792,7 @@ internal TdsOperationStatus TryRun(RunBehavior runBehavior, SqlCommand cmdHandle
                         {
                             _connHandler._federatedAuthenticationInfoReceived = true;
                             SqlFedAuthInfo info;
-                            
+
                             result = TryProcessFedAuthInfo(stateObj, tokenLength, out info);
                             if (result != TdsOperationStatus.Done)
                             {
@@ -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,15 @@ private TdsOperationStatus TryProcessFeatureExtAck(TdsParserStateObject stateObj
                     {
                         return result;
                     }
-                    byte[] data = new byte[dataLen];
-                    if (dataLen > 0)
+                    if (dataLen > (uint)TdsEnums.MaxTokenDataLength)
                     {
-                        result = stateObj.TryReadByteArray(data, checked((int)dataLen));
+                        throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, dataLen);
+                    }
+                    int dataLength = (int)dataLen;
+                    byte[] data = new byte[dataLength];
+                    if (dataLength > 0)
+                    {
+                        result = stateObj.TryReadByteArray(data, dataLength);
                         if (result != TdsOperationStatus.Done)
                         {
                             return result;
@@ -4169,6 +4178,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 +4231,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 +4452,10 @@ private TdsOperationStatus TryProcessFedAuthInfo(TdsParserStateObject stateObj,
                 SqlClientEventSource.Log.TryTraceEvent("<sc.TdsParser.TryProcessFedAuthInfo|ERR> 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,14 +4933,20 @@ internal TdsOperationStatus TryProcessReturnValue(int length,
             }
 
             // always read as sql types
-            Debug.Assert(valLen < (ulong)(int.MaxValue), "ProcessReturnValue received data size > 2Gb");
-
-            int intlen = valLen > (ulong)(int.MaxValue) ? int.MaxValue : (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, unchecked((int)valLen));
+            }
+            else
+            {
+                intlen = (int)valLen;
+            }
 
             if (rec.type == SqlDbTypeExtensions.Vector)
             {
@@ -5790,7 +5817,7 @@ private TdsOperationStatus TryCommonProcessMetaData(TdsParserStateObject stateOb
             {
                 return result;
             }
-         
+
             // read flags and set appropriate flags in structure
             byte flags;
             result = stateObj.TryReadByte(out flags);
@@ -7119,7 +7146,7 @@ internal TdsOperationStatus TryReadSqlValue(SqlBuffer value,
                         return result;
                     }
 
-                    // Internally, we use Sqlbinary to deal with varbinary data and store it in 
+                    // Internally, we use Sqlbinary to deal with varbinary data and store it in
                     // SqlBuffer as SqlBinary value.
 #if NET
                     value.SqlBinary = SqlBinary.WrapBytes(b);
@@ -7188,9 +7215,20 @@ 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)
         {
-            Span<byte> 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.
+            if (length < 0 || length > TdsEnums.MaxDateTimeLength)
+            {
+                throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, length);
+            }
+
+            Span<byte> datetimeBuffer = stackalloc byte[TdsEnums.MaxDateTimeLength];
 
             TdsOperationStatus result = stateObj.TryReadByteArray(datetimeBuffer, length);
             if (result != TdsOperationStatus.Done)
@@ -7446,9 +7484,11 @@ internal TdsOperationStatus TryReadSqlValueInternal(SqlBuffer value, byte tdsTyp
                 case TdsEnums.SQLVECTOR:
                     {
                         // Note: Better not come here with plp data!!
-                        Debug.Assert(length <= TdsEnums.MAXSIZE);
-                        byte[] b = new byte[length];
-                        result = stateObj.TryReadByteArrayWithContinue(length, isPlp: false, out b);
+                        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)
                         {
                             return result;
@@ -9278,7 +9318,7 @@ internal int WriteFedAuthFeatureRequest(FederatedAuthenticationFeatureExtensionD
         /// </remarks>
         internal int WriteVectorSupportFeatureRequest(bool write)
         {
-            const int len = 6; 
+            const int len = 6;
 
             if (write)
             {
@@ -10476,7 +10516,7 @@ private Task TDSExecuteRPCAddParameter(TdsParserStateObject stateObj, SqlParamet
             {
                 isSqlVal = param.ParameterIsSqlType;  // We have to forward the TYPE info, we need to know what type we are returning.  Once we null the parameter we will no longer be able to distinguish what type were seeing.
 
-                // Output parameter of SqlDbType vector are defined through SqlParameter.Value. 
+                // Output parameter of SqlDbType vector are defined through SqlParameter.Value.
                 // This check ensures that we do not discard the parameter value when SqlDbType is vector.
                 if (mt.SqlDbType != SqlDbTypeExtensions.Vector)
                 {
@@ -10761,7 +10801,7 @@ private Task TDSExecuteRPCAddParameter(TdsParserStateObject stateObj, SqlParamet
 
                         Debug.Assert(udtVal != null, "GetBytes returned null instance. Make sure that it always returns non-null value");
                         size = udtVal.Length;
-                        
+
                         if (size >= maxSupportedSize && maxsize != -1)
                         {
                             throw SQL.UDTInvalidSize(maxsize, maxSupportedSize);
@@ -13263,7 +13303,7 @@ private Task WriteUnterminatedValue(object value, MetaType type, byte scale, int
                             {
                                 if (type.NullableType == TdsEnums.SQLJSON)
                                 {
-                                    // TODO : Performance and BOM check. Saurabh 
+                                    // TODO : Performance and BOM check. Saurabh
                                     byte[] jsonAsBytes = Encoding.UTF8.GetBytes((string)value);
                                     WriteInt(jsonAsBytes.Length, stateObj);
                                     return stateObj.WriteByteArray(jsonAsBytes, jsonAsBytes.Length, 0, canAccumulate: false);
@@ -13921,13 +13961,13 @@ internal TdsOperationStatus TryReadPlpUnicodeCharsWithContinue(TdsParserStateObj
             }
 
             TdsOperationStatus result = TryReadPlpUnicodeChars(
-                ref temp, 
-                0, 
-                length >> 1, 
-                stateObj, 
-                out length, 
+                ref temp,
+                0,
+                length >> 1,
+                stateObj,
+                out length,
                 supportRentedBuff: !canContinue, // do not use the arraypool if we are going to keep the buffer in the snapshot
-                rentedBuff: ref buffIsRented, 
+                rentedBuff: ref buffIsRented,
                 startOffset,
                 canContinue
             );
@@ -14137,7 +14177,7 @@ bool writeDataSizeToSnapshot
                         stateObj._longlenleft--;
                         if (writeDataSizeToSnapshot)
                         {
-                            // we need to write the single b1 byte to the array because we may run out of data 
+                            // we need to write the single b1 byte to the array because we may run out of data
                             // and need to wait for another packet
                             buff[offst] = (char)((b1 & 0xff));
                             currentPacketId = IncrementSnapshotDataSize(stateObj, restartingDataSizeCount, currentPacketId, 1);

src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionEnhancedRoutingTests.cs

@@ -14,6 +14,8 @@ namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests;
 /// <summary>
 /// Tests connection routing using the enhanced routing feature extension and envchange token
 /// </summary>
+// 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
 {

src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionFailoverTests.cs

@@ -12,6 +12,8 @@
 namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests
 {
     [Trait("Category", "flaky")]
+    // 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
     {
@@ -173,7 +175,7 @@ public void NetworkTimeout_ShouldFail()
                 InitialCatalog = "master",// Required for failover partner to work
                 ConnectTimeout = 1,
                 ConnectRetryInterval = 1,
-                ConnectRetryCount = 0, // Disable retry 
+                ConnectRetryCount = 0, // Disable retry
                 Encrypt = false,
                 MultiSubnetFailover = false,
 #if NETFRAMEWORK
@@ -460,7 +462,7 @@ public void TransientFault_WithUserProvidedPartner_ShouldConnectToPrimary(uint e
                 FailoverPartner = $"localhost:{failoverServer.EndPoint.Port}", // User provided failover partner
             };
             using SqlConnection connection = new(builder.ConnectionString);
-                
+
             // Act
             connection.Open();
 

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;

src/Microsoft.Data.SqlClient/tests/UnitTests/SimulatedServerTests/ConnectionRoutingTests.cs

@@ -11,6 +11,8 @@
 namespace Microsoft.Data.SqlClient.UnitTests.SimulatedServerTests
 {
     [Trait("Category", "flaky")]
+    // 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
     {
@@ -195,7 +197,7 @@ public void NetworkTimeoutAtRoutedLocation_RetryDisabled_ShouldFail()
             // Act
             var e = Assert.Throws<SqlException>(connection.Open);
 
-            // Assert 
+            // Assert
             Assert.Equal(ConnectionState.Closed, connection.State);
             Assert.Contains("Connection Timeout Expired", e.Message);
         }