-
Notifications
You must be signed in to change notification settings - Fork 5k
Fix handling SECBUFFER_EXTRA with renegotiation after handshake. #103419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix handling SECBUFFER_EXTRA with renegotiation after handshake. #103419
Conversation
/azp run runtime-libraries coreclr-outerloop |
No pipelines are associated with this pull request. |
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
I was originally wondering if we can do something here: runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs Lines 850 to 866 in f8f4509
we treat it as opaque buffer but it is probably sequence of TLS frames...? This issue impacts only Windows IMHO so it would be nice to keep is scoped down. |
The Renegotiate frame which causes the problem in this case is of type Handshake, so it cannot be distinguished from the tail end of the TLS handshake (
I tried to scope it down as much as possible, the Unix/OSX paths should behave the same way as before, the only change over there is the added out parameter which always reports that entire buffer passed in was processed. I have some ideas how to refactor and simplify the Windows/Unix logic split, but I want to postpone them until early .NET 10. |
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…renegotiation-is-requested-immediately-after-handshake
…if-renegotiation-is-requested-immediately-after-handshake
Fixes #91409.
In case Renegotiation request arrives immediately after handshake, we may read it together with the ServerDone message, passing the Renegotiate message to
InitializeSecurityContext
would leave it unprocessed (and reported viaSECBUFFER_EXTRA
), but the existing code would attempt to feed it back into ISC, when the right thing to do is to pass it toDecryptMessage
(as normal post-handshake data).This PR bubbles up the amount of consumed bytes from InitializeSecurityContext and AcceptSecurityContext, so that the upper layer can decide where the extra data should go next (back to ISC/ASC if handshake is not yet finished,
DecryptMessage
if handshake is finished). It also adds unit tests (one of which fails without this change).