Skip to content
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.

Sign up for GitHub

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

Jump to bottom

TdsParser.ProcessSSPI in NetCore uses ArrayPool.Shared.Rent which might return an non zero-initialized array #2441

Closed
stuka120 opened this issue Apr 3, 2024 · 3 comments · Fixed by #2447
Closed

TdsParser.ProcessSSPI in NetCore uses ArrayPool.Shared.Rent which might return an non zero-initialized array #2441

stuka120 opened this issue Apr 3, 2024 · 3 comments · Fixed by #2447
Projects
SqlClient Triage Board

Comments

@stuka120
Copy link

stuka120 commented Apr 3, 2024 •
edited

Describe the bug

TdsParser for NetCore uses ArrayPool.Shared.Rent(...) which might return an non-zero-initialized array.

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

Lines 8654 to 8683 in faf9b95

byte[] receivedBuff = ArrayPool<byte>.Shared.Rent(receivedLength);
// read SSPI data received from server
Debug.Assert(_physicalStateObj._syncOverAsync, "Should not attempt pends in a synchronous call");
bool result = _physicalStateObj.TryReadByteArray(receivedBuff, receivedLength);
if (!result)
{
throw SQL.SynchronousCallMayNotPend();
}
// allocate send buffer and initialize length
byte[] rentedSendBuff = ArrayPool<byte>.Shared.Rent((int)s_maxSSPILength);
byte[] sendBuff = rentedSendBuff;
uint sendLength = s_maxSSPILength;
// make call for SSPI data
SSPIData(receivedBuff, (uint)receivedLength, ref sendBuff, ref sendLength);
// DO NOT SEND LENGTH - TDS DOC INCORRECT! JUST SEND SSPI DATA!
_physicalStateObj.WriteByteArray(sendBuff, (int)sendLength, 0);
ArrayPool<byte>.Shared.Return(rentedSendBuff, clearArray: true);
ArrayPool<byte>.Shared.Return(receivedBuff, clearArray: true);
// set message type so server knows its a SSPI response
_physicalStateObj._outputMessageType = TdsEnums.MT_SSPI;
// send to server
_physicalStateObj.WritePacket(TdsEnums.HARDFLUSH);
_physicalStateObj.SniContext = outerContext;

The first bytes of that array are then replaced with the negTokenResp.

But as the array has some bytes set from the beginning, there might be some additional data after the actual negTokenResp, which causes the MIT-krb5 implementation to fail with a DEFECTIVE-TOKEN here.

Exception message:
Cannot authenticate using Kerberos. Ensure Kerberos has been initialized on the client with 'kinit' and a Service Principal Name has been registered for the SQL Server to allow Kerberos authentication.\nErrorCode=InternalError, Exception=Interop+NetSecurityNative+GssApiException: GSSAPI operation failed with error - Invalid token was supplied (Unknown error).\n  

Stack trace:
   at System.Net.Security.NegotiateStreamPal.GssInitSecurityContext(SafeGssContextHandle& context, SafeGssCredHandle credential, Boolean isNtlm, SafeGssNameHandle targetName, GssFlags inFlags, Byte[] buffer, Byte[]& outputBuffer, UInt32& outFlags, Int32& isNtlmUsed)\n  
   at System.Net.Security.NegotiateStreamPal.EstablishSecurityContext(SafeFreeNegoCredentials credential, SafeDeleteContext& context, String targetName, ContextFlagsPal inFlags, SecurityBuffer inputBuffer, SecurityBuffer outputBuffer, ContextFlagsPal& outFlags)\n  
   at Microsoft.Data.SqlClient.SNI.SNIProxy.GenSspiClientContext(SspiClientContextStatus sspiClientContextStatus, Byte[] receivedBuff, Byte[]& sendBuff, Byte[][] serverName)\n  
   at Microsoft.Data.SqlClient.SNI.TdsParserStateObjectManaged.GenerateSspiClientContext(Byte[] receivedBuff, UInt32 receivedLength, Byte[]& sendBuff, UInt32& sendLength, Byte[][] _sniSpnBuffer)\n  
   at Microsoft.Data.SqlClient.TdsParser.SSPIData(Byte[] receivedBuff, UInt32 receivedLength, Byte[]& sendBuff, UInt32& sendLength)

To reproduce

I'm using an application with multiple connection-strings which - for testing purposes - queries the database very often, afterwards it invokes SqlConnection.ClearAllPools();. And this is done in an endless loop -> This causes the client to establish new security-contexts heavily which causes the error to appear.

Expected behavior

Only the negTokenResp bytes are sent to the underlying GSSAPI implementation

Further technical details

Microsoft.Data.SqlClient version: (found on the nuget or Microsoft.Data.SqlClient.dll)
.NET target: (e.g. .NET6)
SQL Server version: Microsoft SQL Server 2019
Operating system: aspnet:7.0-bookworm-slim

Additional context

I event tried the latest debian distro, but there the latest krb5 version is 1.20 which still does that check. In 1.21 they kind of revamped that check, but it's not available for debian as it stands now.
@vonzshik
Copy link

vonzshik commented Apr 3, 2024

The problem is in GenerateSspiClientContext, which while does receive the size of the array (receivedLength), but completely ignores it.

SqlClient/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParserStateObjectManaged.cs

Lines 392 to 414 in faf9b95

internal override uint GenerateSspiClientContext(byte[] receivedBuff,
uint receivedLength,
ref byte[] sendBuff,
ref uint sendLength,
byte[][] _sniSpnBuffer)
{
#if NET7_0_OR_GREATER
_negotiateAuth ??= new(new NegotiateAuthenticationClientOptions { Package = "Negotiate", TargetName = Encoding.Unicode.GetString(_sniSpnBuffer[0]) });
sendBuff = _negotiateAuth.GetOutgoingBlob(receivedBuff, out NegotiateAuthenticationStatusCode statusCode)!;
SqlClientEventSource.Log.TryTraceEvent("TdsParserStateObjectManaged.GenerateSspiClientContext | Info | Session Id {0}, StatusCode={1}", _sessionHandle?.ConnectionId, statusCode);
if (statusCode is not NegotiateAuthenticationStatusCode.Completed and not NegotiateAuthenticationStatusCode.ContinueNeeded)
{
throw new InvalidOperationException(SQLMessage.SSPIGenerateError() + Environment.NewLine + statusCode);
}
#else
_sspiClientContextStatus ??= new SspiClientContextStatus();
SNIProxy.GenSspiClientContext(_sspiClientContextStatus, receivedBuff, ref sendBuff, _sniSpnBuffer);
SqlClientEventSource.Log.TryTraceEvent("TdsParserStateObjectManaged.GenerateSspiClientContext | Info | Session Id {0}", _sessionHandle?.ConnectionId);
#endif
sendLength = (uint)(sendBuff != null ? sendBuff.Length : 0);
return 0;
}

@stuka120
Copy link
Author

stuka120 commented Apr 3, 2024

If we need to respect the actual size of the negTokenResp, which is available in receivedLength, does it then make sense to use the ArrayPool.Shared.Rent(...) mechanism at all?

Maybe a byte[] receivedBuff = new byte[receivedLength];, like it has been done in the old System.Data.SqlClient#TdsParser would be a better fit here?

@JRahnama JRahnama added this to Needs triage in SqlClient Triage Board via automation Apr 3, 2024
@stuka120 stuka120 changed the title TdsParser in NetCore uses ArrayPool.Shared.Rent which might return an non zero-initialized array TdsParser.ProcessSSPI in NetCore uses ArrayPool.Shared.Rent which might return an non zero-initialized array Apr 3, 2024
This was referenced Apr 5, 2024
Closed
Merged
@JRahnama JRahnama removed the untriaged label Apr 9, 2024
@JRahnama JRahnama moved this from Needs triage to Under Investigation in SqlClient Triage Board Apr 9, 2024
@JRahnama
Copy link
Member

JRahnama commented Apr 9, 2024

PR #2447 is under review to solve this issue.

SqlClient Triage Board automation moved this from Under Investigation to Closed May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
SqlClient Triage Board
  
Closed
Milestone
No milestone
3 participants
@stuka120 @vonzshik @JRahnama