From 27ef4ca34eeb423c3416460a9a7043e88ea9ba7a Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 12 May 2026 10:23:17 +0200 Subject: [PATCH 1/3] Fix NegotiateStream stale read buffer on mid-frame read failure ReadAsync was assigning _readBufferCount = readBytes (announced frame size) before the body read completed. If the body read failed (e.g. connection close), _readBufferCount stayed non-zero pointing at a freshly-allocated zero-filled buffer, and the next Read call returned that stale data instead of propagating EOF/error. Defer assignment of _readBufferOffset/_readBufferCount until after decryption succeeds. UnwrapInPlace also writes those fields via out-params even on failure, so we use locals and only commit to the fields after the success check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../System/Net/Security/NegotiateStream.cs | 18 +++++-- .../NegotiateStreamStreamToStreamTest.cs | 51 +++++++++++++++++++ 2 files changed, 64 insertions(+), 5 deletions(-) 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..cd689a0b56dfd1 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs @@ -256,6 +256,57 @@ 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(new[] + { + 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 (uninitialized / zeroed) 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 first so it can flush before its inner stream is closed, + // then close the inner stream to 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 From 9650d172bdc6af6d1ef126f331a365ef74426d19 Mon Sep 17 00:00:00 2001 From: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Date: Tue, 12 May 2026 10:42:04 +0200 Subject: [PATCH 2/3] Code review feedback Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../FunctionalTests/NegotiateStreamStreamToStreamTest.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs index cd689a0b56dfd1..8b41b3de25e3f2 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs @@ -283,8 +283,8 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout(new[] } finally { - // Dispose the server NegotiateStream first so it can flush before its inner stream is closed, - // then close the inner stream to force EOF on the client side mid-frame. + // Dispose the server NegotiateStream to close its inner stream and force EOF on the + // client side mid-frame. server.Dispose(); } From d36dce6cad68bd499451d45ac4f890c6827dd5f4 Mon Sep 17 00:00:00 2001 From: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Date: Tue, 12 May 2026 11:51:54 +0200 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../FunctionalTests/NegotiateStreamStreamToStreamTest.cs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs index 8b41b3de25e3f2..ad5d18eedee84d 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs @@ -266,16 +266,14 @@ public async Task NegotiateStream_StreamToStream_ReadFailsMidFrame_DoesNotReturn var server = new NegotiateStream(stream2); try { - await TestConfiguration.WhenAllOrAnyFailedWithTimeout(new[] - { + await TestConfiguration.WhenAllOrAnyFailedWithTimeout( AuthenticateAsClientAsync(client, CredentialCache.DefaultNetworkCredentials, string.Empty), - AuthenticateAsServerAsync(server), - }); + 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 (uninitialized / zeroed) buffer contents. + // 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);