Client certificate provided for request isn't cloned when using Kestrel

mconnew commented 4 years ago

Describe the bug

The certificate returned from HttpContext.Connection.ClientCertificate when using Kestrel is the original certificate returned from SslStream.RemoteCertificate and is the same instance for every request from that underlying connection. This means if one request calls Dispose() on the certificate, a following request made on the same connection will get an invalid certificate as the same object instance is returned. When using the HttpSys based server, a new instance of X509Certificate is returned for every request so calling Dispose() in one request doesn't affect the following requests on the same connection. Exposing/producing a mutable object via a public api should not produce the same instance to multiple requests as otherwise you risk leaking side effects between multiple independent requests. This is also not thread safe because X509Certificate2 is mutable and not thread safe. For example, if two threads each fetch the Extensions property (not at the same time as it's lazily created, so different times to ensure same instance) and then both threads try to modify the X509ExtensionCollection at the same time, you could corrupt the underlying List object.

To Reproduce

Executing the following code with a service configured to require client certificates and using the Kestrel server. Use the following code in Startup.Configure:

app.Run(async context => { 
    var cert = context.Connection.ClientCertificate;        
    await context.Response.WriteAsync($"<div>You used a certificate with thumbprint {cert.Thumbprint}</div>")
    cert.Dispose(); 
});

Exceptions (if any)

On the second request using the same connection, you will get the following exception:

Unhandled exception: System.Security.Cryptography.CryptographicException: m_safeCertContext is an invalid handle.
   at System.Security.Cryptography.X509Certificates.X509Certificate.ThrowIfInvalid()
   at System.Security.Cryptography.X509Certificates.X509Certificate.GetCertHashString()
   at System.Security.Cryptography.X509Certificates.X509Certificate2.get_Thumbprint()

Further technical details

ASP.NET Core version - At least 2.1, looks like the same code in the latest codebase too

ghost commented 4 years ago

Thanks for contacting us. We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

ghost commented 4 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

halter73 commented 4 years ago

dotnet / aspnetcore