Fix NegotiateStream stale read buffer on mid-frame read failure

[rzikm]

Fixes a bug in NegotiateStream.ReadAsync where a failed body read could cause subsequent Read calls to return stale (zero-filled) buffer contents instead of propagating EOF/error.

Problem

In ReadAsync, after reading the 4-byte frame header, the code was assigning _readBufferCount = readBytes (the announced frame size) before actually reading the body:

_readBufferCount = readBytes;   // <-- premature
_readBufferOffset = 0;
if (readBytes > _readBuffer.Length)
{
    _readBuffer = new byte[readBytes];
}
await ReadAllAsync(InnerStream, ..., allowZeroRead: false, ...);  // may throw

If ReadAllAsync threw (e.g. the peer closed the connection mid-frame), _readBufferCount was left non-zero, pointing at a freshly-allocated zero-filled buffer. NegotiateStream does not set any sticky failure flag, so a subsequent Read call would hit the cached-buffer branch (if (_readBufferCount != 0)) and silently return up to readBytes bytes of zeros — the caller has no way to tell the difference between a graceful zero-byte payload and a corrupted stream.

Fix

• Defer the assignment of _readBufferOffset/_readBufferCount until after decryption succeeds.
• As a defensive secondary fix, refactor the decryption block to use locals (decryptedOffset/decryptedCount) and only commit them to the fields after the success check. UnwrapInPlace writes its out-params even on failure, and the original code threw without resetting them.

Test

Added NegotiateStream_StreamToStream_ReadFailsMidFrame_DoesNotReturnStaleBufferOnNextRead to the abstract NegotiateStreamStreamToStreamTest class so all concrete variants (Async_Array, Async_Memory, Async_Memory_NotEncrypted, BeginEnd, Sync, Sync_NotEncrypted) exercise it. The test:

1. Authenticates an NTLM-loopback NegotiateStream pair.
2. Writes a fake 4-byte header (frame size 100) directly to the inner server stream, then disposes the server.
3. The first client read sees the header and tries to read the body, which EOFs → expects IOException.
4. The second client read must return 0 (EOF) — the test asserts that the call does not return 100 stale bytes.

Verified the test fails without the fix (Expected: 0, Actual: 100) and passes with it.

Test is Windows-only (matches the rest of the class), as NTLM loopback works there without explicit credentials/SPNs.

Note

This PR description was drafted by GitHub Copilot.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/ncl, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR fixes a NegotiateStream read-path state bug where an exception during the framed-body read/decrypt could leave internal buffer state inconsistent, allowing subsequent reads to return stale/undecrypted data rather than properly surfacing EOF/error.

Changes:

• Delay committing _readBufferOffset / _readBufferCount until after the framed body is fully read and successfully decrypted.
• Refactor the decrypt/unwrap block to use local decryptedOffset / decryptedCount and only publish them to fields on success.
• Add a functional regression test that forces an EOF mid-frame and verifies the next read doesn’t return stale buffered bytes.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/libraries/System.Net.Security/src/System/Net/Security/NegotiateStream.cs	Defers internal read-buffer state updates until the read+decrypt succeeds to avoid stale-buffer reads after failures.
src/libraries/System.Net.Security/tests/FunctionalTests/NegotiateStreamStreamToStreamTest.cs	Adds a regression test covering mid-frame EOF to ensure subsequent reads don’t return stale data.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[wfurt]

If ReadAllAsync threw (e.g. the peer closed the connection mid-frame), _readBufferCount was left non-zero, pointing at a freshly-allocated zero-filled buffer. NegotiateStream does not set any sticky failure flag, so a subsequent Read call would hit the cached-buffer branch

would we ever go back after throwing? For negotiate and SslStream I would feel the state would be undefined after throwing. And if for example peer closes the socket I see no point of ever trying again.

[rzikm]

would we ever go back after throwing? For negotiate and SslStream I would feel the state would be undefined after throwing. And if for example peer closes the socket I see no point of ever trying again.

Normally no, but this bug has been flagged by an AI scan tool and it seemd as a simple fix