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.

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

Fix | Fix ArrayPool usages in TdsParser.ProcessSSPI #2449

Closed
wants to merge 1 commit into from

Conversation

stuka120
Copy link

@stuka120 stuka120 commented Apr 5, 2024

Reapply the implementation of TdsParser.ProcessSSPI from the netfx implementation which does not utilize the ArrayPool.Shared.Rent(...) mechanism, as the ArrayPool received byte[] is not guaranteed to be zero-initialized as specified in the docs, and also has not the correct size, as the given receivedLength is only specifying the minimum-size of the returned byte[].

Old netfx implementation in version 5.2.0:

private void ProcessSSPI(int receivedLength)
{
SniContext outerContext = _physicalStateObj.SniContext;
_physicalStateObj.SniContext = SniContext.Snix_ProcessSspi;
// allocate received buffer based on length from SSPI message
byte[] receivedBuff = new byte[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[] sendBuff = new byte[s_maxSSPILength];
UInt32 sendLength = s_maxSSPILength;
// make call for SSPI data
SSPIData(receivedBuff, (UInt32)receivedLength, sendBuff, ref sendLength);
// DO NOT SEND LENGTH - TDS DOC INCORRECT! JUST SEND SSPI DATA!
_physicalStateObj.WriteByteArray(sendBuff, (int)sendLength, 0);
// 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;
}

If the ArrayPool.Shared.Rent(...) array is non-zero-initialized and oversized, this further causes invalid genTokenResp to be passed to the underlying GSSAPI#InitSecContext implementation.

fixes #2441

@twsouthwick
Copy link
Member

I'm currently reworking the sspi generation in general to enable a public api around it. One of my changes in #2447 fixes this in the direction I need for the larger item #2253.

Part of the changes I'm trying to push in moves toward more memory/span operations (and helps consolidate the implementations between framework and core). There are currently 4 implementations of generating SSPI contexts and the others work fine with a ReadOnlyMemory<byte>. Forcing all of them to use an array because a single one requires it will cause undue memory allocations.

@stuka120
Copy link
Author

stuka120 commented Apr 9, 2024

I changed the implementation to don't do another allocation but instead clear the "overflowing" bytes inside the receivedBuff by doing receivedBuff.AsSpan(receivedLength, receivedBuff.Length - receivedLength).Clear();. That should at least for the time being do the trick.
Maybe we can use that until your rework of the SSPI implementations is complete @twsouthwick?
Or is the PR you opened planned to being part of the next patch-release for version 5.2 (e.g. 5.2.1)?

@stuka120 stuka120 changed the title Fix | Remove ArrayPool usages in TdsParser.ProcessSSPI Fix | Fix ArrayPool usages in TdsParser.ProcessSSPI Apr 9, 2024
@twsouthwick
Copy link
Member

@stuka120 my change for sspi with this fix has been merged so this can probably be closed

@David-Engel
Copy link
Contributor

Fixed via #2447

@David-Engel David-Engel closed this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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