Managing the most dangerous constructor ever
The design of the X509Certificate2 is badly broken in terms of safety. If you load a certificate from the disk or a byte buffer, it will go ahead and create a file on the disk behind the scene. If you’ll dispose the instance, the file will be removed. However, if you don’t explicitly dispose the instance, that is too bad. The file remains.
A ticking time bomb, because eventually you’ll have a lot of such files on the disk. Which is then a fun state to try to recover from.
I’m not sure why this design decision was made. I assume that at the time, people didn’t need to work so much with certificates, and a lot of the issues are likely with dealing with the underlying crypto API. Regardless, it is mandatory to dispose the certificate after you use it.
And that leads to a problem. Consider the following code:
The idea is that we want to be able to switch certificates on the fly (since we need to update them before they expire, without interrupting the server). Old connections can still use the old certificate, while new ones will use the updated one.
Practically speaking, the certificate itself shouldn’t be used after the call to AuthenticateAsServerAsync(), but I don’t believe that we have any such promises. Regardless, as the async designation indicates, that can take a while. How would I know to dispose the old certificate? I have to consider multi threading here as well, if I dispose the certificate while it is being used to authenticate a request, that request will likely fail. Given that I’m racing a native API and disposing its resources while it is under use, I may open some severe issues.
Ideally, the X509Certificate2 should manage that for me. If it would have implemented a finalizer, it would dispose itself when the GC made sure that no one was looking at it. That is what I want to happen, but in this case, we have no such support.
Luckily we got options. Behold the following code:
What does this do? It uses several tricks to get what we want, attaching an external finalizer to an object that we don’t control.
First, ConditionalWeakTable will ensure that as long as there is a reference to the certificate, the cleaner will be referenced as well. When there is no reference for the certificate, we’ll need to run the finalizer for the cleaner.
Next, we have the usage of CriticalFinalizerObject, this is done to ensure that the finalizer will be called even when the process terminates. This is the same manner .NET flushes file handles, so we can be sure that we are doing the utmost to ensure that we’ll properly dispose of the files.
Finally, there is the dance with the GetValueOrDefault() call in RegisterForDisposalDuringFinalization(). We need to consider what would happen if we’ll get concurrent requests to register the certificate. If we’ll let it race, one of the cleaners will be discarded, and then the finalizer will be called on that, causing havoc.
In this manner, we let ConditionalWeakTable ensure that there is just one instance, and set the value afterward. Since the value is unique per instance, we can set it multiple times (it will always be set to the same value).
End result, it takes less than 10 lines of code to fix this (and of course, remember to call register whenever you create a certificate instance). But I would really like that to just be the default behavior. Otherwise, that is a very risky trap.
Wow, fun things could happen with, for example a badly configured XML deserializer, as an attacker could cause the creation of those.