[BUG] PivSession.Dispose doesn't close Connection if Yubikey unplugged

[canton7]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

PivSession.Dispose attempts to send a SelectApplicationCommand command, before it does other tidying up.

However, if the Yubikey has been disconnected, this Connection.SendCommand throws an exception, and the rest of the Dispose method does not run.

This means that Connection.Dispose() isn't run. This also means that a Dispose method throws an exception, which is not normally expected behaviour.

Expected Behavior

PivSession.Dispose should make a best-effort attempt to clean up all resources, even if the Yubikey has been disconnected.

Steps To Reproduce

1. Connect a Yubikey, open a PivSession
2. Disconnect the Yubikey
3. Dispose the PivSession
4. Observe that an exception is thrown

Version

1.10.0

Version

5.4.3

Anything else?

No response

[canton7]

I initially thought this was the cause of a SCARD_E_SHARING_VIOLATION when trying to reconnect to a Yubikey which had been unplugged and then plugged in again, but I still got that even after manually disposing the Connection. Downgrading to v1.9.0 fixed that issue, but I suspect that the Dispose behaviour here is still undesirable.

[GregDomzalski]

Yup. That seems like an issue regardless.

Regarding the sharing violation: Typically it's caused by another Windows service racing with your app to make a connection to the YubiKey. I had attempted to "fix" this in 1.9.1 by opening the handle to the smart card exclusively. I did not realize that it would instead cause further issues upstream. This has now been changed back to the previous (1.9.0) behavior and the exclusive locking guarded behind an app compat flag. Hopefully this will be made available in the next release. A more comprehensive fix to the race is still needed, but it'll be more involved too. At least the SDK will be back to behaving how it has been.

[canton7]

Yep, I gathered that from reading the other issues here, thanks! (Which is why I downgraded, pending a new release with your fix in).

[DennisDyallo]

Relevant code

[DennisDyallo]

Hi @canton7 ! Just to gather more info, why do you think that the exception shouldn't be thrown?
The SDK expects the Yubikey to be connected when working with them. Calling the Dispose to clean up resources/reset state when the Yubikey is disconnected is not supported. There's still stuff left to do, but the user unplugged the Yubikey. What should we do then? Why did the user unplug and not wait for the work to be done? Is there perhaps a particular use case to this where this 'best effort' treatment is necessary? And since we're throwing an exception, could you not handle it on your end?

Thanks, trying to understand.

[canton7]

Hi @DennisDyallo, thanks for picking this up!

why do you think that the exception shouldn't be thrown?

First and foremost, Dispose methods are not expected to throw exceptions in .NET. See for example CA1065.

When dealing with a class which implements IDisposable, it's common to wrap it in a using statement, which calls Dispose automatically. It's not expected that the user has to manually check whether certain preconditions are met before calling Dispose manually.

In addition, I don't think there's a way to check whether a Yubikey is connected? If so, there's no way to write the following code anyway:

IYubiKeyDevice yubiKeyToUse = SelectYubiKey();
var piv = new PivSession(yubiKeyToUse)
try
{
    /* Perform PIV operations. */
}
finally
{
    if (piv.Connection.IsConnected) // I can't find a property such as this? How do you check this?
    {
        piv.Dispose();
    }
}

There's still stuff left to do, but the user unplugged the Yubikey. What should we do then?

PivSession.Dispose() calls PivSession.Connection.Dispose(), but only if the prior call to Connection.SendCommand(new SelectApplicationCommand(YubiKeyApplication.Management)) succeeded (which it doesn't if the Yubikey has been disconnected).

PivSession.Connection is an IDisposable object, which needs to be disposed in its own right. To take an example, DesktopSmartCardConnection's Dispose method looks like this:

if (!_disposed)
{
	if (disposing)
	{
		_cardHandle.Dispose();
		_context.Dispose();
	}
	_disposed = true;
}

_cardHandle.Dispose() calls NativeMethods.SCardDisconnect, passing in a native handle. _context.Dispose() calls NativeMethods.SCardReleaseContext, passing in a handle. SCardReleaseContext in particular is required in order to release resources allocated under the context (and calling SCardDisconnect is just good practice).

Thus, if the Yubikey has been disconnected by the user, disposing the PivSession does not correctly release unmanaged resources, and instead we fall back to relying on the finalizer to release them at some indeterminate point in the futer.

.NET best practice is to allow unmanaged resources to be released explicitly by a call to Dispose. The current implementation of PivSession.Dispose does not allow this.

It's worth noting that PivSession also does:

KeyCollector = null;
ResetAuthenticationStatus();
_disposed = true;

None of this happens if the Yubikey has become disconnected.

Calling the Dispose to clean up resources/reset state when the Yubikey is disconnected is not supported

As discussed this is a very odd decision, which it not documented and goes against the way that the rest of .NET works. Elsewhere, it is always permitted to call Dispose, and that method should do whatever is appropriate to clean up as much as possible. If Yubikey has made an explicit decision to deviate from this norm, this needs to be explicitly and loudly documented.

It is very much not expected that Yubikey leaks native resources (Finalizer notwithstanding) if the user disconnects a Yubikey!

There's still stuff left to do, but the user unplugged the Yubikey. What should we do then

Sure, but you should at least let the programmer release native resources in the normal way.

And since we're throwing an exception, could you not handle it on your end?

Yes, but that's very unusual in .NET (you never see a using statement wrapped in a try/catch -- it's just not a thing which is ever needed). Additionally as discussed, you do not correctly release unmanaged resources in this case.

---

I also note that your own documentation says:

Because this class implements IDisposable, use the using keyword. For example,

    IYubiKeyDevice yubiKeyToUse = SelectYubiKey();
    using (var piv = new PivSession(yubiKeyToUse))
    {
        /* Perform PIV operations. */
    }

If this class is used as part of a using expression, when the session goes out of scope, the Dispose method will be called to dispose the active PIV session, clearing any authenticated state, and ultimately releasing the connection to the YubiKey.

According to your comment, this needs to be reworded, and the code sample changed. Although what it should be changed to I'm not sure! I currently have:

var piv = new PivSession(yubiKeyToUse);
try
{
    /* Perform PIV operations. */
}
finally
{
    try
    {
        piv.Dispose();
    }
    catch
    {
        // If the Yubikey is no longer connected, then PivSession.Dispose will throw and won't call Connection.Close, which leaves the underlying Windows connection open.
        // This means that subsequent attempts to open a connection to the same device (once reconnected) will fail with SCARD_E_SHARING_VIOLATION.
        // See https://github.com/Yubico/Yubico.NET.SDK/issues/104
        piv.Connection.Dispose();
    }
}

---

In my view, a corrected implementation of PivSession would just be:

public void Dispose()
{
    if (!_disposed)
    {
        // This could fail, e.g. if the Yubikey has been disconnected. Continue freeing resources if so.
        try
        {
            Connection.SendCommand(new SelectApplicationCommand(YubiKeyApplication.Management));
        }
        catch { }

        KeyCollector = null;
        ResetAuthenticationStatus();
        Connection.Dispose();
        _disposed = true;
    }
}

[DennisDyallo]

Hi again @canton7 !
I agree with all of your comments. A Dispose method that throws exceptions is indeed a rare sight (and I don't think I've ever experienced one before myself).
I'm about to push a PR that addresses this in the way you suggested together with some logging for the user. That being said, I want to set expectations by letting you know that our next SDK release isn't planned until Q4. Will you survive until then?

[canton7]

That's great, thank you!

Q4 is fine for me thanks, I have a functional workaround.