Skip to content
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

rzikm
Copy link
Member

@rzikm rzikm commented Jun 13, 2024

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 via SECBUFFER_EXTRA), but the existing code would attempt to feed it back into ISC, when the right thing to do is to pass it to DecryptMessage (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).

@rzikm rzikm marked this pull request as ready for review June 14, 2024 07:55
@rzikm rzikm requested a review from wfurt June 14, 2024 07:55
@rzikm
Copy link
Member Author

rzikm commented Jun 14, 2024

/azp run runtime-libraries coreclr-outerloop

Copy link

No pipelines are associated with this pull request.

@rzikm
Copy link
Member Author

rzikm commented Jun 14, 2024

/azp run runtime-libraries-coreclr outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rzikm
Copy link
Member Author

rzikm commented Jun 14, 2024

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wfurt
Copy link
Member

wfurt commented Jun 14, 2024

I was originally wondering if we can do something here:

extraBuffer = new byte[_buffer.DecryptedLength];
_buffer.DecryptedSpan.CopyTo(extraBuffer);
_buffer.Discard(_buffer.DecryptedLength);
}
if (NetEventSource.Log.IsEnabled())
NetEventSource.Info(null, $"***Processing an error Status = {status}");
if (status.ErrorCode == SecurityStatusPalErrorCode.Renegotiate)
{
// We determined above that we will not process it.
if (_handshakeWaiter == null)
{
throw new IOException(SR.net_ssl_io_renego);
}
await ReplyOnReAuthenticationAsync<TIOAdapter>(extraBuffer, cancellationToken).ConfigureAwait(false);

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.

@rzikm
Copy link
Member Author

rzikm commented Jun 17, 2024

we treat it as opaque buffer but it is probably sequence of TLS frames...?

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 (ChangeCipherSpec, Finished and such).

This issue impacts only Windows IMHO so it would be nice to keep is scoped down.

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.

@rzikm
Copy link
Member Author

rzikm commented Jun 19, 2024

/azp run runtime-libraries-coreclr outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rzikm rzikm closed this Jul 8, 2024
@rzikm rzikm reopened this Jul 8, 2024
rzikm added 2 commits July 8, 2024 13:14
…renegotiation-is-requested-immediately-after-handshake
…if-renegotiation-is-requested-immediately-after-handshake
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AuthenticateAsClientAsync can fail if renegotiation is requested immediately after handshake
3 participants