SafeEvpPKeyHandle.DuplicateHandle is not thread safe

[vcsjones]

If a SafeEvpPKeyHandle is disposed by one thread while another thread is calling DuplicateHandle, the DuplicateHandle call can succeed, but the resulting SafeEvpPKeyHandle will not be closed and the handle value will be zero.

To demonstrate:

using System.Security.Cryptography;

using ECDsaOpenSsl ecdsa = new ECDsaOpenSsl();

for (int i = 0; i < int.MaxValue - 1; i++)
{
    SafeEvpPKeyHandle handle = ecdsa.DuplicateKeyHandle();
    SafeEvpPKeyHandle? duplicate = null;

    Thread t1 = new Thread(() => {
        Thread.Sleep(Random.Shared.Next(0, 10));
        try { duplicate = handle.DuplicateHandle(); } catch { }
    });
    Thread t2 = new Thread(() => {
        Thread.Sleep(Random.Shared.Next(0, 10));
        handle.Dispose();
    });

    t1.Start();
    t2.Start();
    t1.Join();
    t2.Join();

    if (duplicate is null)
    {
        continue;
    }

    bool addedRef = false;

    try
    {
        duplicate.DangerousAddRef(ref addedRef);

        IntPtr value = duplicate.DangerousGetHandle();

        if (value == IntPtr.Zero)
        {
            throw new Exception($"We somehow got a handle that was not closed but invalid on iteration {i}.");
        }
    }
    finally
    {
        if (addedRef)
        {
            duplicate.DangerousRelease();
        }
    }

    duplicate.Dispose();
}

Console.WriteLine("Didn't crash?");

After a few thousand runs, this will throw an exception:

Unhandled exception. System.Exception: We somehow got a handle that was not closed but invalid on iteration 1887.
at Program.

$(String[] args) in /home/vcsjones/Projects/scratch-cs/Program.cs:line 39

This should not happen. This means DuplicateHandle returned a SafeEvpPKeyHandle that is not closed, but is invalid.

This is likely the underlying issue that is causing #116307.

In that case, we are calling DuplicateHandle on the key supplied, then passing the safe handle to EvpPKeyBits which gets marshaled as NULL

runtime/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/ECDiffieHellmanOpenSsl.cs

Lines 39 to 40 in f92f9de

	_key = new Lazy<SafeEvpPKeyHandle>(pkeyHandle.DuplicateHandle());
	KeySizeValue = Interop.Crypto.EvpPKeyBits(_key.Value);

The race is likely here:

runtime/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SafeEvpPKeyHandle.OpenSsl.cs

Lines 74 to 86 in f92f9de

	int success = Interop.Crypto.UpRefEvpPkey(this);
	if (success != 1)
	{
	Debug.Fail("Called UpRefEvpPkey on a key which was already marked for destruction");
	Exception e = Interop.Crypto.CreateOpenSslCryptographicException();
	safeHandle.Dispose();
	throw e;
	}
	// Since we didn't actually create a new handle, copy the handle
	// to the new SafeHandle.
	safeHandle.SetHandle(handle);

If the current instance is disposed right before line 86, then handle will be IntPtr.Zero. That is because ReleaseHandle explicitly zeros the handle:

runtime/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/SafeEvpPKeyHandle.OpenSsl.cs

Line 51 in f92f9de

SetHandle(IntPtr.Zero);

The fix is probably that DuplicateHandle should do a DangerousAddRef and DangerousRelease around itself so the handle cannot be destroyed while it is duplicating. This is currently only being done implicitly on line 74 when up-ref-ing the key handle, but if its disposed after the up-ref but before the handle field is read, the resulting duplicated handle will be NULL.

[dotnet-policy-service]

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

[bartonjs]

Hm, I feel like the problem is that it reads the field handle after the call to UpRef, which would probably have been more obvious had handle been named _handle.

If it read it into a local before the call to UpRef, and the thing that did the UpRef is still alive, then there was "no" way that the local is wrong, since we only ever go from 0 to the value then back to 0. (I'm ignoring that someone could Marshal.InitHandle and just overwrite the handle value at any time)

[vcsjones]

@bartonjs I think that "works" but the intention seems less clear. Up to you.

[bartonjs]

Changing the read order avoids a try/finally... but yeah, the try/finally probably shows a much clearer "this atomically succeeds or fails", so, yeah, I'm convinced.

[vcsjones]

@bartonjs this occasionally shows up in CI on release branches, albeit rarely. Should we backport this change?

[bartonjs]

It's a thread-safety problem on a SafeHandle type, so... yeah, probably.