Skip to content

Commit b9fbdd0

Browse files
stephentoubstephentoub
authored
Revert "add better handling of SECBUFFER_EXTRA during TLS handshake on Windows (#42427)"
This reverts commit 51f6b8b.
1 parent 7557a0a commit b9fbdd0

2 files changed

Lines changed: 41 additions & 175 deletions

File tree

‎src/libraries/Common/src/Interop/Windows/SspiCli/SecuritySafeHandles.cs‎

Lines changed: 40 additions & 153 deletions
Original file line numberDiff line numberDiff line change
@@ -421,7 +421,7 @@ internal static unsafe int InitializeSecurityContext(
421421
}
422422

423423
// Optional output buffer that may need to be freed.
424-
IntPtr outoutBuffer = IntPtr.Zero;
424+
SafeFreeContextBuffer? outFreeContextBuffer = null;
425425
try
426426
{
427427
Span<Interop.SspiCli.SecBuffer> inUnmanagedBuffer = stackalloc Interop.SspiCli.SecBuffer[3];
@@ -494,6 +494,11 @@ internal static unsafe int InitializeSecurityContext(
494494
IntPtr.Zero :
495495
(IntPtr)(pinnedOutBytes + outSecBuffer.offset);
496496

497+
if (isSspiAllocated)
498+
{
499+
outFreeContextBuffer = SafeFreeContextBuffer.CreateEmptyHandle();
500+
}
501+
497502
if (refContext == null || refContext.IsInvalid)
498503
{
499504
// Previous versions unconditionally built a new "refContext" here, but would pass
@@ -521,94 +526,23 @@ internal static unsafe int InitializeSecurityContext(
521526
refContext!,
522527
ref outSecurityBufferDescriptor,
523528
ref outFlags,
524-
null);
525-
526-
if (isSspiAllocated)
527-
{
528-
outoutBuffer = outUnmanagedBuffer.pvBuffer;
529-
}
530-
531-
// Get unmanaged buffer with index 0 as the only one passed into PInvoke.
532-
outSecBuffer.size = outUnmanagedBuffer.cbBuffer;
533-
outSecBuffer.type = outUnmanagedBuffer.BufferType;
534-
outSecBuffer.token = outSecBuffer.size > 0 ?
535-
new Span<byte>((byte*)outUnmanagedBuffer.pvBuffer, outUnmanagedBuffer.cbBuffer).ToArray() :
536-
null;
537-
538-
if (inSecBuffers.Count > 1 && inUnmanagedBuffer[1].BufferType == SecurityBufferType.SECBUFFER_EXTRA && inSecBuffers._item1.Type == SecurityBufferType.SECBUFFER_EMPTY)
539-
{
540-
// OS function did not use all provided data and turned EMPTY to EXTRA
541-
// https://docs.microsoft.com/en-us/windows/win32/secauthn/extra-buffers-returned-by-schannel
542-
543-
int leftover = inUnmanagedBuffer[1].cbBuffer;
544-
int processed = inSecBuffers._item0.Token.Length - inUnmanagedBuffer[1].cbBuffer;
545-
546-
/* skip over processed data and try it again. */
547-
inUnmanagedBuffer[0].cbBuffer = leftover;
548-
inUnmanagedBuffer[0].pvBuffer = inUnmanagedBuffer[0].pvBuffer + processed;
549-
inUnmanagedBuffer[1].BufferType = SecurityBufferType.SECBUFFER_EMPTY;
550-
inUnmanagedBuffer[1].cbBuffer = 0;
551-
552-
outUnmanagedBuffer.cbBuffer = 0;
553-
554-
if (outoutBuffer != IntPtr.Zero)
555-
{
556-
Interop.SspiCli.FreeContextBuffer(outoutBuffer);
557-
outoutBuffer = IntPtr.Zero;
558-
}
559-
560-
errorCode = MustRunInitializeSecurityContext(
561-
ref inCredentials,
562-
isContextAbsent,
563-
(byte*)(((object)targetName == (object)dummyStr) ? null : namePtr),
564-
inFlags,
565-
endianness,
566-
&inSecurityBufferDescriptor,
567-
refContext!,
568-
ref outSecurityBufferDescriptor,
569-
ref outFlags,
570-
null);
571-
572-
if (isSspiAllocated)
573-
{
574-
outoutBuffer = outUnmanagedBuffer.pvBuffer;
575-
}
576-
577-
if (outUnmanagedBuffer.cbBuffer > 0)
578-
{
579-
if (outSecBuffer.size == 0)
580-
{
581-
// We did not get anything in the first round.
582-
outSecBuffer.size = outUnmanagedBuffer.cbBuffer;
583-
outSecBuffer.type = outUnmanagedBuffer.BufferType;
584-
outSecBuffer.token = new Span<byte>((byte*)outUnmanagedBuffer.pvBuffer, outUnmanagedBuffer.cbBuffer).ToArray();
585-
}
586-
else
587-
{
588-
byte[] buffer = new byte[outSecBuffer.size + outUnmanagedBuffer.cbBuffer];
589-
Buffer.BlockCopy(outSecBuffer.token!, 0, buffer, 0, outSecBuffer.size);
590-
new Span<byte>((byte*)outUnmanagedBuffer.pvBuffer, outUnmanagedBuffer.cbBuffer).CopyTo(new Span<byte>(buffer, outSecBuffer.size, outUnmanagedBuffer.cbBuffer));
591-
outSecBuffer.size = buffer.Length;
592-
outSecBuffer.token = buffer;
593-
}
594-
}
595-
596-
if (inUnmanagedBuffer[1].BufferType == SecurityBufferType.SECBUFFER_EXTRA)
597-
{
598-
// we are left with unprocessed data again. fail with SEC_E_INCOMPLETE_MESSAGE hResult.
599-
errorCode = unchecked((int)0x80090318);
600-
}
601-
}
529+
outFreeContextBuffer);
602530
}
531+
532+
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, "Marshalling OUT buffer");
533+
534+
// Get unmanaged buffer with index 0 as the only one passed into PInvoke.
535+
outSecBuffer.size = outUnmanagedBuffer.cbBuffer;
536+
outSecBuffer.type = outUnmanagedBuffer.BufferType;
537+
outSecBuffer.token = outSecBuffer.size > 0 ?
538+
new Span<byte>((byte*)outUnmanagedBuffer.pvBuffer, outUnmanagedBuffer.cbBuffer).ToArray() :
539+
null;
603540
}
604541
}
605542
}
606543
finally
607544
{
608-
if (outoutBuffer != IntPtr.Zero)
609-
{
610-
Interop.SspiCli.FreeContextBuffer(outoutBuffer);
611-
}
545+
outFreeContextBuffer?.Dispose();
612546
}
613547

614548
return errorCode;
@@ -737,6 +671,8 @@ internal static unsafe int AcceptSecurityContext(
737671
isContextAbsent = refContext._handle.IsZero;
738672
}
739673

674+
// Optional output buffer that may need to be freed.
675+
SafeFreeContextBuffer? outFreeContextBuffer = null;
740676
Span<Interop.SspiCli.SecBuffer> outUnmanagedBuffer = stackalloc Interop.SspiCli.SecBuffer[2];
741677
outUnmanagedBuffer[1].pvBuffer = IntPtr.Zero;
742678
try
@@ -816,6 +752,11 @@ internal static unsafe int AcceptSecurityContext(
816752
outUnmanagedBuffer[1].cbBuffer = 0;
817753
outUnmanagedBuffer[1].BufferType = SecurityBufferType.SECBUFFER_ALERT;
818754

755+
if (isSspiAllocated)
756+
{
757+
outFreeContextBuffer = SafeFreeContextBuffer.CreateEmptyHandle();
758+
}
759+
819760
if (refContext == null || refContext.IsInvalid)
820761
{
821762
// Previous versions unconditionally built a new "refContext" here, but would pass
@@ -834,85 +775,31 @@ internal static unsafe int AcceptSecurityContext(
834775
refContext!,
835776
ref outSecurityBufferDescriptor,
836777
ref outFlags,
837-
null);
778+
outFreeContextBuffer);
838779

839-
// No data written out but there is Alert
840-
int index = outUnmanagedBuffer[0].cbBuffer == 0 && outUnmanagedBuffer[1].cbBuffer > 0 ? 1 : 0;
841-
842-
outSecBuffer.size = outUnmanagedBuffer[index].cbBuffer;
843-
outSecBuffer.type = outUnmanagedBuffer[index].BufferType;
844-
outSecBuffer.token = outSecBuffer.size > 0 ?
845-
new Span<byte>((byte*)outUnmanagedBuffer[index].pvBuffer, outUnmanagedBuffer[0].cbBuffer).ToArray() :
846-
null;
780+
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, "Marshaling OUT buffer");
847781

848-
if (inSecBuffers.Count > 1 && inUnmanagedBuffer[1].BufferType == SecurityBufferType.SECBUFFER_EXTRA && inSecBuffers._item1.Type == SecurityBufferType.SECBUFFER_EMPTY)
782+
// No data written out but there is Alert
783+
if (outUnmanagedBuffer[0].cbBuffer == 0 && outUnmanagedBuffer[1].cbBuffer > 0)
849784
{
850-
// OS function did not use all provided data and turned EMPTY to EXTRA
851-
// https://docs.microsoft.com/en-us/windows/win32/secauthn/extra-buffers-returned-by-schannel
852-
853-
int leftover = inUnmanagedBuffer[1].cbBuffer;
854-
int processed = inSecBuffers._item0.Token.Length - inUnmanagedBuffer[1].cbBuffer;
855-
856-
/* skip over processed data and try it again. */
857-
inUnmanagedBuffer[0].cbBuffer = leftover;
858-
inUnmanagedBuffer[0].pvBuffer = inUnmanagedBuffer[0].pvBuffer + processed;
859-
inUnmanagedBuffer[1].BufferType = SecurityBufferType.SECBUFFER_EMPTY;
860-
inUnmanagedBuffer[1].cbBuffer = 0;
861-
862-
outUnmanagedBuffer[0].cbBuffer = 0;
863-
if (isSspiAllocated && outUnmanagedBuffer[0].pvBuffer != IntPtr.Zero)
864-
{
865-
Interop.SspiCli.FreeContextBuffer(outUnmanagedBuffer[0].pvBuffer);
866-
outUnmanagedBuffer[0].pvBuffer = IntPtr.Zero;
867-
}
868-
869-
errorCode = MustRunAcceptSecurityContext_SECURITY(
870-
ref inCredentials,
871-
isContextAbsent,
872-
&inSecurityBufferDescriptor,
873-
inFlags,
874-
endianness,
875-
refContext!,
876-
ref outSecurityBufferDescriptor,
877-
ref outFlags,
878-
null);
879-
880-
index = outUnmanagedBuffer[0].cbBuffer == 0 && outUnmanagedBuffer[1].cbBuffer > 0 ? 1 : 0;
881-
if (outUnmanagedBuffer[index].cbBuffer > 0)
882-
{
883-
if (outSecBuffer.size == 0)
884-
{
885-
// We did not get anything in the first round.
886-
outSecBuffer.size = outUnmanagedBuffer[index].cbBuffer;
887-
outSecBuffer.type = outUnmanagedBuffer[index].BufferType;
888-
outSecBuffer.token = new Span<byte>((byte*)outUnmanagedBuffer[index].pvBuffer, outUnmanagedBuffer[index].cbBuffer).ToArray();
889-
}
890-
else
891-
{
892-
byte[] buffer = new byte[outSecBuffer.size + outUnmanagedBuffer[index].cbBuffer];
893-
Buffer.BlockCopy(outSecBuffer.token!, 0, buffer, 0, outSecBuffer.size);
894-
new Span<byte>((byte*)outUnmanagedBuffer[index].pvBuffer, outUnmanagedBuffer[index].cbBuffer).CopyTo(new Span<byte>(buffer, outSecBuffer.size, outUnmanagedBuffer[index].cbBuffer));
895-
outSecBuffer.size = buffer.Length;
896-
outSecBuffer.token = buffer;
897-
}
898-
}
899-
900-
if (inUnmanagedBuffer[1].BufferType == SecurityBufferType.SECBUFFER_EXTRA)
901-
{
902-
// we are left with unprocessed data again. fail with SEC_E_INCOMPLETE_MESSAGE hResult.
903-
errorCode = unchecked((int)0x80090318);
904-
}
785+
outSecBuffer.size = outUnmanagedBuffer[1].cbBuffer;
786+
outSecBuffer.type = outUnmanagedBuffer[1].BufferType;
787+
outSecBuffer.token = new Span<byte>((byte*)outUnmanagedBuffer[1].pvBuffer, outUnmanagedBuffer[1].cbBuffer).ToArray();
788+
}
789+
else
790+
{
791+
outSecBuffer.size = outUnmanagedBuffer[0].cbBuffer;
792+
outSecBuffer.type = outUnmanagedBuffer[0].BufferType;
793+
outSecBuffer.token = outUnmanagedBuffer[0].cbBuffer > 0 ?
794+
new Span<byte>((byte*)outUnmanagedBuffer[0].pvBuffer, outUnmanagedBuffer[0].cbBuffer).ToArray() :
795+
null;
905796
}
906797
}
907798
}
908799
}
909800
finally
910801
{
911-
if (isSspiAllocated && outUnmanagedBuffer[0].pvBuffer != IntPtr.Zero)
912-
{
913-
Interop.SspiCli.FreeContextBuffer(outUnmanagedBuffer[0].pvBuffer);
914-
}
915-
802+
outFreeContextBuffer?.Dispose();
916803
if (outUnmanagedBuffer[1].pvBuffer != IntPtr.Zero)
917804
{
918805
Interop.SspiCli.FreeContextBuffer(outUnmanagedBuffer[1].pvBuffer);

‎src/libraries/System.Net.Security/tests/FunctionalTests/CertificateValidationRemoteServer.cs‎

Lines changed: 1 addition & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -65,28 +65,7 @@ public async Task CertificateValidationRemoteServer_EndToEnd_Ok(bool useAsync)
6565
[InlineData("www.apple.com")]
6666
[InlineData("www.icloud.com")]
6767
[PlatformSpecific(TestPlatforms.OSX)]
68-
public Task CertificateValidationApple_EndToEnd_Ok(string host)
69-
{
70-
return EndToEndHelper(host);
71-
}
72-
73-
[ConditionalTheory]
74-
[OuterLoop("Uses external servers")]
75-
[InlineData("api.nuget.org")]
76-
[InlineData("")]
77-
public async Task DefaultConnect_EndToEnd_Ok(string host)
78-
{
79-
if (string.IsNullOrEmpty(host))
80-
{
81-
host = Configuration.Security.TlsServer.IdnHost;
82-
}
83-
84-
await EndToEndHelper(host);
85-
// Second try may change the handshake becasue of TLS resume.
86-
await EndToEndHelper(host);
87-
}
88-
89-
private async Task EndToEndHelper(string host)
68+
public async Task CertificateValidationApple_EndToEnd_Ok(string host)
9069
{
9170
using (var client = new TcpClient())
9271
{

0 commit comments

Comments
 (0)