Client certificates are disposed before request can use them

[ghost]

Functional impact

When using Kestrel with HTTPS and requiring client certificates, the client certificate added to context feature collection is disposed after the callback to userCertificateValidationCallback() has completed. In a subsequent controller action, a request to HttpContext.Connection.ClientCertificate returns an instance of the disposed certificate.

Minimal repro steps

1. Setup a basic/empty web project
2. In the Startup class, in UseKestrel(), call options.UseHttps() with ClientCertificateMode = ClientCertificateMode.RequireCertificate, and ClientCertificateValidation = (x, y, z) => x.Verify()
3. Setup a basic controller and action, say Test/Index(), that derives from Microsoft.AspNetCore.Mvc.Controller.
4. In the sample controller and action, make a call to HttpContext.Connection.ClientCertificate.Verify()

Expected result

The client certificate should not be disposed, and can be examined.

Actual result

An exception is thrown when trying to use the certificate:
System.Security.Cryptography.CryptographicException: m_safeCertContext is an invalid handle.

Further technical details

In the userCertificateValidationCallback() method, at the very end of the call, a reference is made to the valid certificate (clientCertificate = certificate2); however, because the caller of userCertificateValidationCallback() calls certificate.Reset() after validation, the local variable (clientCertificate) is now a reference to a disposed certificate. This disposed certificate is then added to the feature collection: features.Set<ITlsConnectionFeature>(new TlsConnectionFeature { ClientCertificate = clientCertificate });

Observations

I observed that simply cloning the certificate did not appear to resolve the issue (such as a call to new X509Certificate2(clientCertificate) from inside userCertificateValidationCallback()); however, if I stored the RawData byte array (from inside userCertificateValidationCallback()), then call features.Set<ITlsConnectionFeature>(new TlsConnectionFeature { ClientCertificate = new X509Certificate2(rawData) });, the certificate was available, but I don't know if this is the correct solution or not.

Related File

https://github.com/aspnet/KestrelHttpServer/blob/c828fafe1b39279d590ad1a18b62844bae76d77b/src/Microsoft.AspNetCore.Server.Kestrel.Https/HttpsConnectionFilter.cs

[ghost]

If it would help, I could put together a PR.

[halter73]

@shanewalters That would be great. I think rather than copying RawData we can just use SslStream.RemoteCertificate. I haven't tested it though. Thanks!

[ghost]

SslStream.RemoteCertificate is a X509Certificate. I can't get the call to new X509Certificate2(sslStream.RemoteCertificate) to compile on net1.3 (works on net451), but does work by calling new X509Certificate2(sslStream.RemoteCertificate.Handle). Is that acceptable?

[Tratcher]

Using Handle is probably a bad idea. Do we know what the handle lifetime is? What happens if we dispose the cert?

[ghost]

I don't see SslStream explicitly disposed, and it's scoped to HttpsConnectionFilter.OnConnectionAsync(), so my guess is GC picks it up; however, SslStream.Dispose() doesn't dispose the RemoteCertificate instance. Just to be clear, X509Certificate doesn't implement IDisposable, but has a Reset() method (which is what clears the main X509Certificate2 instance).

EDIT
X509Certificate in reference assemblies doesn't implement Dispose(), but in mscorlib it does, and in that version, Dispose() ultimately calls Reset(). I also noticed SslStream's reference is maintained through ConnectionFilterContext.Connection property. I apologize for my lack of understanding--I'm not very familiar with all the moving parts here.

[ghost]

I verified that instantiating a new certificate based off the handle, and later disposing the original certificate (from SslStream.RemoteCertificate) does not affect the new X509Certificate2 instance (the m_certContextCloned property was false). I think this could be used safely to create a new instance. Thoughts?

[halter73]

I wouldn't bother Disposing/Resetting SslStream.RemoteCertificate.

We dispose SslStream in Connection.OnSocketClosed, and we never new up RemoteCertificate ourselves. Considering we weren't resetting the property before, I don't see why we'd start now.

[ghost]

I was going to put together a PR, but when I update if (clientCertificate != null) to if (sslStream.RemoteCertificate?.Handle != IntPtr.Zero), the tests fail during cleanup. If I preserve the original if condition, test cleanup succeeds, but feels like the wrong conditional check. Do you want me to continue looking into it, or are you (@halter73) taking it from here?

Microsoft.AspNetCore.Server.KestrelTests.HttpsConnectionFilterTests.CanReadAndWriteWithHttpsConnectionFilter [FAIL]
      System.AggregateException : One or more errors occurred. (Object reference not set to an instance of an object.)
      ---- System.NullReferenceException : Object reference not set to an instance of an object.
      Stack Trace:
           at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
           at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
           at System.Threading.Tasks.Task.Wait(TimeSpan timeout)
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\KestrelEngine.cs(117,0): at Microsoft.AspNetCore.Server.Kestrel.KestrelEngine.DisposeListeners(List`1 listeners)
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\KestrelEngine.cs(97,0): at Microsoft.AspNetCore.Server.Kestrel.KestrelEngine.<>c__DisplayClass12_0.<CreateServer>b__0()
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Infrastructure\Disposable.cs(27,0): at Microsoft.AspNetCore.Server.Kestrel.Disposable.Dispose(Boolean disposing)
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Infrastructure\Disposable.cs(39,0): at Microsoft.AspNetCore.Server.Kestrel.Disposable.Dispose()
        C:\dev\KestrelHttpServer\test\Microsoft.AspNetCore.Server.KestrelTests\TestServer.cs(57,0): at Microsoft.AspNetCore.Server.KestrelTests.TestServer.Dispose()
        C:\dev\KestrelHttpServer\test\Microsoft.AspNetCore.Server.KestrelTests\HttpsConnectionFilterTests.cs(88,0): at Microsoft.AspNetCore.Server.KestrelTests.HttpsConnectionFilterTests.<CanReadAndWriteWithHttpsConnectionFilter>d__1.MoveNext()
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
        ----- Inner Stack Trace -----
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Http\Connection.cs(147,0): at Microsoft.AspNetCore.Server.Kestrel.Http.Connection.StopAsync()
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Http\ConnectionManager.cs(38,0): at Microsoft.AspNetCore.Server.Kestrel.Http.ConnectionManager.<WalkConnectionsAndClose>b__3_0(IntPtr ptr)
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Infrastructure\KestrelThread.cs(192,0): at Microsoft.AspNetCore.Server.Kestrel.KestrelThread.<>c__DisplayClass38_0.<Walk>b__0(IntPtr ptr, IntPtr arg)
           at Microsoft.AspNetCore.Server.Kestrel.Networking.Libuv.NativeMethods.uv_walk(UvLoopHandle loop, uv_walk_cb walk_cb, IntPtr arg)
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Networking\Libuv.cs(397,0): at Microsoft.AspNetCore.Server.Kestrel.Networking.Libuv.walk(UvLoopHandle loop, uv_walk_cb walk_cb, IntPtr arg)
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Infrastructure\KestrelThread.cs(188,0): at Microsoft.AspNetCore.Server.Kestrel.KestrelThread.Walk(Action`1 callback)
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Http\ConnectionManager.cs(31,0): at Microsoft.AspNetCore.Server.Kestrel.Http.ConnectionManager.WalkConnectionsAndClose()
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Http\Listener.cs(100,0): at Microsoft.AspNetCore.Server.Kestrel.Http.Listener.<>c.<DisposeAsync>b__11_0(Object state)
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Infrastructure\KestrelThread.cs(27,0): at Microsoft.AspNetCore.Server.Kestrel.KestrelThread.<>c.<.cctor>b__47_1(Object callback, Object state)
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Infrastructure\KestrelThread.cs(286,0): at Microsoft.AspNetCore.Server.Kestrel.KestrelThread.DoPostWork()
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
           at System.Runtime.CompilerServices.ConfiguredTaskAwaitable.ConfiguredTaskAwaiter.GetResult()
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Http\Listener.cs(93,0): at Microsoft.AspNetCore.Server.Kestrel.Http.Listener.<DisposeAsync>d__11.MoveNext()
    Microsoft.AspNetCore.Server.KestrelTests.HttpsConnectionFilterTests.HttpsSchemePassedToRequestFeature [FAIL]
      System.AggregateException : One or more errors occurred. (Object reference not set to an instance of an object.)
      ---- System.NullReferenceException : Object reference not set to an instance of an object.
      Stack Trace:
           at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
           at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
           at System.Threading.Tasks.Task.Wait(TimeSpan timeout)
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\KestrelEngine.cs(117,0): at Microsoft.AspNetCore.Server.Kestrel.KestrelEngine.DisposeListeners(List`1 listeners)
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\KestrelEngine.cs(97,0): at Microsoft.AspNetCore.Server.Kestrel.KestrelEngine.<>c__DisplayClass12_0.<CreateServer>b__0()
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Infrastructure\Disposable.cs(27,0): at Microsoft.AspNetCore.Server.Kestrel.Disposable.Dispose(Boolean disposing)
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Infrastructure\Disposable.cs(39,0): at Microsoft.AspNetCore.Server.Kestrel.Disposable.Dispose()
        C:\dev\KestrelHttpServer\test\Microsoft.AspNetCore.Server.KestrelTests\TestServer.cs(57,0): at Microsoft.AspNetCore.Server.KestrelTests.TestServer.Dispose()
        C:\dev\KestrelHttpServer\test\Microsoft.AspNetCore.Server.KestrelTests\HttpsConnectionFilterTests.cs(292,0): at Microsoft.AspNetCore.Server.KestrelTests.HttpsConnectionFilterTests.<HttpsSchemePassedToRequestFeature>d__5.MoveNext()
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
        ----- Inner Stack Trace -----
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Http\Connection.cs(147,0): at Microsoft.AspNetCore.Server.Kestrel.Http.Connection.StopAsync()
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Http\ConnectionManager.cs(38,0): at Microsoft.AspNetCore.Server.Kestrel.Http.ConnectionManager.<WalkConnectionsAndClose>b__3_0(IntPtr ptr)
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Infrastructure\KestrelThread.cs(192,0): at Microsoft.AspNetCore.Server.Kestrel.KestrelThread.<>c__DisplayClass38_0.<Walk>b__0(IntPtr ptr, IntPtr arg)
           at Microsoft.AspNetCore.Server.Kestrel.Networking.Libuv.NativeMethods.uv_walk(UvLoopHandle loop, uv_walk_cb walk_cb, IntPtr arg)
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Networking\Libuv.cs(397,0): at Microsoft.AspNetCore.Server.Kestrel.Networking.Libuv.walk(UvLoopHandle loop, uv_walk_cb walk_cb, IntPtr arg)
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Infrastructure\KestrelThread.cs(188,0): at Microsoft.AspNetCore.Server.Kestrel.KestrelThread.Walk(Action`1 callback)
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Http\ConnectionManager.cs(31,0): at Microsoft.AspNetCore.Server.Kestrel.Http.ConnectionManager.WalkConnectionsAndClose()
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Http\Listener.cs(100,0): at Microsoft.AspNetCore.Server.Kestrel.Http.Listener.<>c.<DisposeAsync>b__11_0(Object state)
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Infrastructure\KestrelThread.cs(27,0): at Microsoft.AspNetCore.Server.Kestrel.KestrelThread.<>c.<.cctor>b__47_1(Object callback, Object state)
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Infrastructure\KestrelThread.cs(286,0): at Microsoft.AspNetCore.Server.Kestrel.KestrelThread.DoPostWork()
        --- End of stack trace from previous location where exception was thrown ---
           at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
           at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
           at System.Runtime.CompilerServices.ConfiguredTaskAwaitable.ConfiguredTaskAwaiter.GetResult()
        C:\dev\KestrelHttpServer\src\Microsoft.AspNetCore.Server.Kestrel\Http\Listener.cs(93,0): at Microsoft.AspNetCore.Server.Kestrel.Http.Listener.<DisposeAsync>d__11.MoveNext()