From 27ef4ca34eeb423c3416460a9a7043e88ea9ba7a Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 12 May 2026 10:23:17 +0200 Subject: [PATCH 1/4] 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/4] 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/4] 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); From 7fd39b79626dad11727e77fc30a0543cc4876b27 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Fri, 15 May 2026 09:57:46 +0200 Subject: [PATCH 4/4] Skip ReadFailsMidFrame body when NegotiateStream is unencrypted/unsigned With ProtectionLevel.None, NegotiateStream.Read forwards directly to the inner stream and never populates _readBufferCount, so the mid-frame stale-buffer scenario the test asserts on does not apply. The _NotEncrypted test class variants currently fail the IOException assertion on Windows because the fake frame header is returned verbatim as the read payload. Bail out after authentication when neither encryption nor signing is in effect. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../FunctionalTests/NegotiateStreamStreamToStreamTest.cs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs index ad5d18eedee84d..046e7b287b3d04 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs @@ -270,6 +270,15 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( AuthenticateAsClientAsync(client, CredentialCache.DefaultNetworkCredentials, string.Empty), AuthenticateAsServerAsync(server)); + // The mid-frame failure scenario only applies when NegotiateStream is actually + // framing the payload (i.e., encryption or signing is in effect). With + // ProtectionLevel.None, NegotiateStream.Read forwards directly to the inner stream + // and there is no _readBufferCount to leave stale. + if (!client.IsEncrypted && !client.IsSigned) + { + return; + } + // 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