-
Notifications
You must be signed in to change notification settings - Fork 329
Add TDS token data length bounds checks #4340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
f7e272d
656316a
24ea6ff
0f11e87
3bb0e4c
54a2817
d062a39
9f4bcf0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
|
|
@@ -1346,6 +1346,11 @@ internal void OnFeatureExtAck(int featureId, byte[] data) | |
| len = bLen; | ||
| } | ||
|
|
||
| if (len < 0 || len > data.Length - i) | ||
| { | ||
| throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, len); | ||
| } | ||
|
|
||
|
Comment on lines
+1349
to
+1353
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test coverage: |
||
| byte[] stateData = new byte[len]; | ||
| Buffer.BlockCopy(data, i, stateData, 0, len); | ||
| i += len; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
||
|
paulmedynski marked this conversation as resolved.
|
||
| // 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 | ||
|
Comment on lines
+87
to
98
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test coverage: These constants are validated by all 12 unit tests across |
||
| // 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
| } | ||
|
Comment on lines
+3351
to
+3354
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test coverage: |
||
| // 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) | ||
| { | ||
| throw SQL.ParsingErrorLength(ParsingErrorState.CorruptedTdsStream, (int)dataLen); | ||
| } | ||
| int dataLength = (int)dataLen; | ||
| byte[] data = new byte[dataLength]; | ||
| if (dataLength > 0) | ||
| { | ||
|
Comment on lines
+3853
to
3860
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test coverage: |
||
| result = stateObj.TryReadByteArray(data, checked((int)dataLen)); | ||
| 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) | ||
|
Comment on lines
4178
to
+4181
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test coverage: |
||
| { | ||
| 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); | ||
| } | ||
|
Comment on lines
+4234
to
+4237
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test coverage: |
||
| 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); | ||
| } | ||
|
Comment on lines
+4455
to
+4458
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test coverage: |
||
|
|
||
| // 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)); | ||
| } | ||
|
paulmedynski marked this conversation as resolved.
|
||
| else | ||
| { | ||
| intlen = (int)valLen; | ||
|
Comment on lines
+4936
to
+4948
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test coverage: |
||
| } | ||
|
|
||
| if (rec.type == SqlDbTypeExtensions.Vector) | ||
| { | ||
|
|
@@ -7113,7 +7140,14 @@ internal TdsOperationStatus TryReadSqlValue(SqlBuffer value, | |
|
|
||
| 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) | ||
|
Comment on lines
+7143
to
7153
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test coverage: |
||
|
|
@@ -7369,7 +7403,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); | ||
|
Comment on lines
7405
to
+7408
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test coverage: |
||
| } | ||
| result = stateObj.TryReadByteArrayWithContinue(length, isPlp: false, out byte[] b); | ||
| if (result != TdsOperationStatus.Done) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,172 @@ | ||
| // 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; | ||
|
|
||
| /// <summary> | ||
| /// 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. | ||
| /// </summary> | ||
| [Collection("SimulatedServerTests")] | ||
| public class FeatureExtAckBoundsTests : IClassFixture<TdsServerFixture> | ||
| { | ||
| 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; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Verifies that the TDS parser rejects a FeatureExtAck token whose data length | ||
| /// field exceeds <see cref="TdsEnums.MaxTokenDataLength"/> (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. | ||
| /// </summary> | ||
| [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<TDSFeatureExtAckToken>().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))); | ||
| }; | ||
|
|
||
| try | ||
| { | ||
| // Act & Assert: connection should fail with a parsing error, NOT an OutOfMemoryException | ||
| using SqlConnection connection = new(_connectionString); | ||
| Exception ex = Assert.ThrowsAny<InvalidOperationException>(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; | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// 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. | ||
| /// </summary> | ||
| [Fact] | ||
| public void FeatureExtAck_MaxAllowedDataLength_DoesNotThrow() | ||
| { | ||
| // 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 => | ||
| { | ||
| var existing = responseMessage.OfType<TDSFeatureExtAckToken>().FirstOrDefault(); | ||
| if (existing != null) | ||
| { | ||
| 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)); | ||
| }; | ||
|
|
||
| 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<Exception>(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; | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// 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. | ||
| /// </summary> | ||
| 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); | ||
| } | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.