Skip to content

Don't return SslStreamBuffer when disposing during handshake #68956

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.

Sign up for GitHub

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

Jump to bottom
Merged
rzikm merged 2 commits into from
May 11, 2022
Merged

Don't return SslStreamBuffer when disposing during handshake #68956

rzikm merged 2 commits into from
May 11, 2022

Conversation

@rzikm
Copy link
Member

@rzikm rzikm commented May 6, 2022 •
edited
Loading

Fixes #66818.

This PR makes sure we don't dispose buffers which may be still in use by ongoing handshake, which on debug builds led to an assert firing and crashing entire test sutite (see #65455 for example).

Dispose during ongoing read operations was handled correctly before.

@wfurt you mentioned we should have some tests which dispose the SslStream mid-operation on purpose. Would something like below suffice? It is essentially what we did before #66682.

Details 
        SslProtocols clientProtocol;
        SslProtocols serverProtocol;

        clientProtocol = SslProtocols.Tls13;
        serverProtocol = SslProtocols.Tls12;

        TcpListener listener = new TcpListener(IPAddress.Loopback, 0);
        listener.Start();

        string certPath = Path.Join(new FileInfo(Assembly.GetExecutingAssembly().Location).Directory.FullName, "contoso.com.pfx");
        System.Console.WriteLine(certPath);
        using X509Certificate2 serverCertificate = new X509Certificate2(File.ReadAllBytes(certPath), "testcertificate");

        while (true)
        {
            System.Console.Write('.');
            using (TcpClient client = new TcpClient())
            {
                Task clientConnectTask = client.ConnectAsync(IPAddress.Loopback, ((IPEndPoint)listener.LocalEndpoint).Port);
                Task<TcpClient> listenerAcceptTask = listener.AcceptTcpClientAsync();

                await Task.WhenAll(clientConnectTask, listenerAcceptTask);

                TcpClient server = listenerAcceptTask.Result;
                using (SslStream clientStream = new SslStream(
                    client.GetStream(),
                    false,
                    new RemoteCertificateValidationCallback(delegate { return true; }),
                    null,
                    EncryptionPolicy.RequireEncryption))
                using (SslStream serverStream = new SslStream(
                    server.GetStream(),
                    false,
                    null,
                    null,
                    EncryptionPolicy.RequireEncryption))
                {

                    Task clientAuthenticationTask = clientStream.AuthenticateAsClientAsync(
                        serverCertificate.GetNameInfo(X509NameType.SimpleName, false),
                        null,
                        clientProtocol,
                        false);

                    try
                    {
                        await serverStream.AuthenticateAsServerAsync(
                           serverCertificate,
                           false,
                           serverProtocol,
                           false);
                    }
                    catch (AuthenticationException)
                    {
                    }

                    // e = await Assert.ThrowsAsync<AuthenticationException>(() => clientAuthenticationTask);
                }
            }
        }

</details/

@ghost ghost assigned rzikm May 6, 2022
@ghost
Copy link

ghost commented May 6, 2022

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

Issue Details

Fixes #66818.

This PR makes sure we don't dispose buffers which may be still in use by ongoing handshake, which on debug builds led to an assert firing and crashing entire test sutite (see #65455 for example).

Dispose during ongoing read operations was handled correctly before.

@wfurt you mentioned we should have some tests which dispose the SslStream mid-operation on purpose. Would something like below suffice? It is essentially what we did before #66682.

Details ```cs SslProtocols clientProtocol; SslProtocols serverProtocol; 
    clientProtocol = SslProtocols.Tls13;
    serverProtocol = SslProtocols.Tls12;

    TcpListener listener = new TcpListener(IPAddress.Loopback, 0);
    listener.Start();

    string certPath = Path.Join(new FileInfo(Assembly.GetExecutingAssembly().Location).Directory.FullName, "contoso.com.pfx");
    System.Console.WriteLine(certPath);
    using X509Certificate2 serverCertificate = new X509Certificate2(File.ReadAllBytes(certPath), "testcertificate");

    while (true)
    {
        System.Console.Write('.');
        using (TcpClient client = new TcpClient())
        {
            Task clientConnectTask = client.ConnectAsync(IPAddress.Loopback, ((IPEndPoint)listener.LocalEndpoint).Port);
            Task<TcpClient> listenerAcceptTask = listener.AcceptTcpClientAsync();

            await Task.WhenAll(clientConnectTask, listenerAcceptTask);

            TcpClient server = listenerAcceptTask.Result;
            using (SslStream clientStream = new SslStream(
                client.GetStream(),
                false,
                new RemoteCertificateValidationCallback(delegate { return true; }),
                null,
                EncryptionPolicy.RequireEncryption))
            using (SslStream serverStream = new SslStream(
                server.GetStream(),
                false,
                null,
                null,
                EncryptionPolicy.RequireEncryption))
            {

                Task clientAuthenticationTask = clientStream.AuthenticateAsClientAsync(
                    serverCertificate.GetNameInfo(X509NameType.SimpleName, false),
                    null,
                    clientProtocol,
                    false);

                try
                {
                    await serverStream.AuthenticateAsServerAsync(
                       serverCertificate,
                       false,
                       serverProtocol,
                       false);
                }
                catch (AuthenticationException)
                {
                }

                // e = await Assert.ThrowsAsync<AuthenticationException>(() => clientAuthenticationTask);
            }
        }
    }

</details/

<table>
  <tr>
    <th align="left">Author:</th>
    <td>rzikm</td>
  </tr>
  <tr>
    <th align="left">Assignees:</th>
    <td>-</td>
  </tr>
  <tr>
    <th align="left">Labels:</th>
    <td>

`area-System.Net.Security`

</td>
  </tr>
  <tr>
    <th align="left">Milestone:</th>
    <td>-</td>
  </tr>
</table>
</details>

@rzikm rzikm requested a review from wfurt May 6, 2022 11:13
wfurt
wfurt approved these changes May 6, 2022
View reviewed changes
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.
We will not return buffer to the shared pool in that case but it seems ok since that should be rare. (it was not issue prior #64747 as we did not rent buffer for handshake)
cc @stephentoub for any additional concerns.

@rzikm rzikm force-pushed the 66818-Possible-Assert-when-disposing-SslStream-during-handshake branch from f080bd2 to 3e24569 Compare May 9, 2022 08:34
@rzikm @stephentoub
Update src/libraries/System.Net.Security/src/System/Net/Security/SslS…
5e97526 
…tream.IO.cs

Co-authored-by: Stephen Toub <stoub@microsoft.com>
@rzikm
Copy link
Member Author

rzikm commented May 11, 2022

Test failures are unrelated

@rzikm rzikm merged commit 24587d1 into dotnet:main May 11, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 10, 2022
@karelz karelz added this to the 7.0.0 milestone Jul 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub stephentoub left review comments

@wfurt wfurt wfurt approved these changes

Assignees

@rzikm rzikm

Labels

area-System.Net.Security

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

Possible Assert when disposing SslStream during transfer/handshake

4 participants

@rzikm @stephentoub @wfurt @karelz