avoid ArgumentOutOfRangeException while processing invalid or incomplete TLS frame

[wfurt]

This change fixes issue reported in #62109. The issue has long summary describing what is going on - based on dump provided by the reporter.

In essence, FillHandshakeBufferAsync can "silently fail" on EOF and we can create ArrayBuffer with negative length. Added test would throw ArgumentOutOfRangeException on release builds or hit Assert with debug builds.

I modified the FillHandshakeBufferAsync to consistently throw if EOF is reached before request bytes are accumulated. This was already there in few places but we missed the case when read would finish synchronously.

fixes #62109

[ghost]

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

Issue Details

---

This change fixes issue reported in #62109. The issue has long summary describing what is going on - based on dump provided by the reporter.

In essence, FillHandshakeBufferAsync can "silently fail" on EOF and we can create ArrayBuffer with negative length. Added test would throw ArgumentOutOfRangeException on release builds or hit Assert with debug builds.

I modified the FillHandshakeBufferAsync to consistently throw if EOF is reached before request bytes are accumulated. This was already there in few places but we missed the case when read would finish synchronously.

fixes #62109

Author:	wfurt
Assignees:	wfurt
Labels:	area-System.Net.Security
Milestone:	-

[geoffkizer]

Since we are changing this code anyway:

I don't understand why we have the InternalFillHandshakeBuffferAsync helper here. What is this accomplishing, vs just directly awaiting the task returned by ReadAsync? The async machinery already optimizes for the sync completion case.

I suppose perhaps there is some benefit in avoiding the very small cost of async method setup in the case where we don't need to read at all, but (a) I'm quite skeptical that this cost matters, and (b) even if we want to optimize for it, we can achieve the same result much more easily.

Looking back at the history here, this seems to have happened because we basically copied this code from FillBufferAsync, and that's what FillBufferAsync used to do. But FillBufferAsync was changed to not do this a long time ago.

(Also, FillBufferAsync seems to be not used at all anymore, and should be removed.)

[wfurt]

yes, I was thinking about InternalFillHandshakeBuffferAsync, but decided not to change it with this change. It is independent IMHO and would complicate servicing if we would think about it. It seems like was FillBufferAsync was obsoleted by EnsureFullTlsFrameAsync.
Merging the _handshakeBuffer and _internalBuffer is on my todo list for a while.

[wfurt]

Looks like I open #52037 while back for the buffers.

[stephentoub]

LGTM