Skip to content

Commit 80194e6

Browse files
wfurtstephentoub
wfurt
and
stephentoub
authored
add better handling of SECBUFFER_EXTRA during TLS handshake on Windows (#43475)
* add better handling of SECBUFFER_EXTRA during TLS handshake on Windows (#42427) * add better handling of SECBUFFER_EXTRA during TLS handshake on Windows * fix boundery check * fix spelling * update Authentication_IncorrectServerName_Fail test * feedback from review * Update src/libraries/System.Net.Security/tests/FunctionalTests/CertificateValidationRemoteServer.cs Co-authored-by: Stephen Toub <stoub@microsoft.com> Co-authored-by: Stephen Toub <stoub@microsoft.com>
1 parent effaf28 commit 80194e6

File tree

3 files changed

+183
-42
lines changed

3 files changed

+183
-42
lines changed

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

Lines changed: 153 additions & 40 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-
SafeFreeContextBuffer? outFreeContextBuffer = null;
424+
IntPtr outoutBuffer = IntPtr.Zero;
425425
try
426426
{
427427
Span<Interop.SspiCli.SecBuffer> inUnmanagedBuffer = stackalloc Interop.SspiCli.SecBuffer[3];
@@ -494,11 +494,6 @@ internal static unsafe int InitializeSecurityContext(
494494
IntPtr.Zero :
495495
(IntPtr)(pinnedOutBytes + outSecBuffer.offset);
496496

497-
if (isSspiAllocated)
498-
{
499-
outFreeContextBuffer = SafeFreeContextBuffer.CreateEmptyHandle();
500-
}
501-
502497
if (refContext == null || refContext.IsInvalid)
503498
{
504499
// Previous versions unconditionally built a new "refContext" here, but would pass
@@ -526,23 +521,94 @@ internal static unsafe int InitializeSecurityContext(
526521
refContext!,
527522
ref outSecurityBufferDescriptor,
528523
ref outFlags,
529-
outFreeContextBuffer);
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+
}
530602
}
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;
540603
}
541604
}
542605
}
543606
finally
544607
{
545-
outFreeContextBuffer?.Dispose();
608+
if (outoutBuffer != IntPtr.Zero)
609+
{
610+
Interop.SspiCli.FreeContextBuffer(outoutBuffer);
611+
}
546612
}
547613

548614
return errorCode;
@@ -671,8 +737,6 @@ internal static unsafe int AcceptSecurityContext(
671737
isContextAbsent = refContext._handle.IsZero;
672738
}
673739

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

755-
if (isSspiAllocated)
756-
{
757-
outFreeContextBuffer = SafeFreeContextBuffer.CreateEmptyHandle();
758-
}
759-
760819
if (refContext == null || refContext.IsInvalid)
761820
{
762821
// Previous versions unconditionally built a new "refContext" here, but would pass
@@ -775,31 +834,85 @@ internal static unsafe int AcceptSecurityContext(
775834
refContext!,
776835
ref outSecurityBufferDescriptor,
777836
ref outFlags,
778-
outFreeContextBuffer);
779-
780-
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, "Marshaling OUT buffer");
837+
null);
781838

782839
// No data written out but there is Alert
783-
if (outUnmanagedBuffer[0].cbBuffer == 0 && outUnmanagedBuffer[1].cbBuffer > 0)
784-
{
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
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;
847+
848+
if (inSecBuffers.Count > 1 && inUnmanagedBuffer[1].BufferType == SecurityBufferType.SECBUFFER_EXTRA && inSecBuffers._item1.Type == SecurityBufferType.SECBUFFER_EMPTY)
790849
{
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;
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+
}
796905
}
797906
}
798907
}
799908
}
800909
finally
801910
{
802-
outFreeContextBuffer?.Dispose();
911+
if (isSspiAllocated && outUnmanagedBuffer[0].pvBuffer != IntPtr.Zero)
912+
{
913+
Interop.SspiCli.FreeContextBuffer(outUnmanagedBuffer[0].pvBuffer);
914+
}
915+
803916
if (outUnmanagedBuffer[1].pvBuffer != IntPtr.Zero)
804917
{
805918
Interop.SspiCli.FreeContextBuffer(outUnmanagedBuffer[1].pvBuffer);

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

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,28 @@ public async Task CertificateValidationRemoteServer_EndToEnd_Ok(bool useAsync)
6565
[InlineData("www.apple.com")]
6666
[InlineData("www.icloud.com")]
6767
[PlatformSpecific(TestPlatforms.OSX)]
68-
public async Task CertificateValidationApple_EndToEnd_Ok(string host)
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 because of TLS resume.
86+
await EndToEndHelper(host);
87+
}
88+
89+
private async Task EndToEndHelper(string host)
6990
{
7091
using (var client = new TcpClient())
7192
{

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,14 @@ public async Task SslStream_StreamToStream_Authentication_IncorrectServerName_Fa
9595
Task t2 = server.AuthenticateAsServerAsync(certificate);
9696

9797
await Assert.ThrowsAsync<AuthenticationException>(() => t1);
98-
await t2;
98+
try
99+
{
100+
await t2;
101+
}
102+
catch
103+
{
104+
// Ignore outcome of t2. It can succeed or fail depending on timing.
105+
}
99106
}
100107
}
101108

0 commit comments

Comments
 (0)