diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/NegotiateStream.cs b/src/libraries/System.Net.Security/src/System/Net/Security/NegotiateStream.cs index b0f25d431b2b90..b5c1cde85be047 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/NegotiateStream.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/NegotiateStream.cs @@ -381,17 +381,22 @@ private async ValueTask ReadAsync(Memory buffer, Cancella // Always pass InternalBuffer for SSPI "in place" decryption. // A user buffer can be shared by many threads in that case decryption/integrity check may fail cause of data corruption. - _readBufferCount = readBytes; - _readBufferOffset = 0; if (_readBuffer.Length < readBytes) { _readBuffer = new byte[readBytes]; } + // Note: do not assign _readBufferCount/_readBufferOffset before the read completes successfully. + // If the read throws (e.g. due to the connection closing), a subsequent Read call would otherwise + // observe a non-zero _readBufferCount and return stale/undecrypted buffer contents. readBytes = await ReadAllAsync(InnerStream, new Memory(_readBuffer, 0, readBytes), allowZeroRead: false, cancellationToken).ConfigureAwait(false); // Decrypt into the same buffer (decrypted data size can be shrunk after decryption). + // Use locals so that on failure we do not leave _readBufferOffset/_readBufferCount in a state that + // would expose stale or undecrypted data on a subsequent Read call. NegotiateAuthenticationStatusCode statusCode; + int decryptedOffset = 0; + int decryptedCount = 0; if (isNtlm && !_context.IsEncrypted) { // Non-encrypted NTLM uses an encoding quirk @@ -404,14 +409,14 @@ private async ValueTask ReadAsync(Memory buffer, Cancella } else { - _readBufferOffset = NtlmSignatureLength; - _readBufferCount = readBytes - NtlmSignatureLength; + decryptedOffset = NtlmSignatureLength; + decryptedCount = readBytes - NtlmSignatureLength; statusCode = NegotiateAuthenticationStatusCode.Completed; } } else { - statusCode = _context.UnwrapInPlace(_readBuffer.AsSpan(0, readBytes), out _readBufferOffset, out _readBufferCount, out _); + statusCode = _context.UnwrapInPlace(_readBuffer.AsSpan(0, readBytes), out decryptedOffset, out decryptedCount, out _); } if (statusCode != NegotiateAuthenticationStatusCode.Completed) @@ -420,6 +425,9 @@ private async ValueTask ReadAsync(Memory buffer, Cancella throw new IOException(SR.net_io_read); } + _readBufferOffset = decryptedOffset; + _readBufferCount = decryptedCount; + // Decrypted data can be shrunk after decryption. if (_readBufferCount == 0 && buffer.Length != 0) { diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs index 5a251058b06388..ad5d18eedee84d 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs @@ -256,6 +256,55 @@ public async Task NegotiateStream_StreamToStream_Authentication_EmptyCredentials } } } + + [ConditionalFact(typeof(NegotiateStreamStreamToStreamTest), nameof(IsNtlmInstalled))] + public async Task NegotiateStream_StreamToStream_ReadFailsMidFrame_DoesNotReturnStaleBufferOnNextRead() + { + (Stream stream1, Stream stream2) = TestHelper.GetConnectedStreams(); + using (var client = new NegotiateStream(stream1)) + { + var server = new NegotiateStream(stream2); + try + { + await TestConfiguration.WhenAllOrAnyFailedWithTimeout( + AuthenticateAsClientAsync(client, CredentialCache.DefaultNetworkCredentials, string.Empty), + AuthenticateAsServerAsync(server)); + + // Inject only a frame header that promises a body, then close the inner stream so the + // client's body read fails mid-frame. With the bug, NegotiateStream pre-populates + // _readBufferCount with the announced body size, and a subsequent Read returns up to + // that many bytes of stale (zero-filled / never populated) buffer contents. + const int FakeFrameSize = 100; + byte[] fakeHeader = new byte[4]; + System.Buffers.Binary.BinaryPrimitives.WriteInt32LittleEndian(fakeHeader, FakeFrameSize); + await stream2.WriteAsync(fakeHeader); + } + finally + { + // Dispose the server NegotiateStream to close its inner stream and force EOF on the + // client side mid-frame. + server.Dispose(); + } + + // First read must observe the mid-frame failure. + byte[] buffer = new byte[200]; + await Assert.ThrowsAsync(() => ReadAsync(client, buffer, 0, buffer.Length)); + + // A subsequent read must NOT return stale buffer data. The inner stream is at EOF so + // the only correct outcomes are zero bytes (graceful EOF) or another IOException. + int read; + try + { + read = await ReadAsync(client, buffer, 0, buffer.Length); + } + catch (IOException) + { + return; + } + + Assert.Equal(0, read); + } + } } public sealed class NegotiateStreamStreamToStreamTest_Async_Array : NegotiateStreamStreamToStreamTest