Fix for context handle management in PCSC reader driver

[misterzed88]

Proposed fix for issue #1999, taken a minimalistic change approach.

Checklist

• Documentation is added or updated
• New files have a LGPL 2.1 license statement
• PKCS#11 module is tested
• Windows minidriver is tested
• macOS tokend is tested

[frankmorgner]

Thanks a lot for tracking this down! It's good as is.

Using -1 for an invalid SCARDCONTEXT (instead of 0) was first introduced in e237574 without justification. @LudovicRousseau do you know any good reason for doing so?

[misterzed88]

So this bug has been lurking there since 2008, over a decade, before surfacing? Interesting!

I am not sure about the motivations for the original code but will try to make a qualified guess.

The original code used -1 since it relied on SCardListReaders to return SCARD_E_INVALID_HANDLE in order to call SCardEstablishContext. Since SCardListReaders accepts 0 as input in Windows, the author chose -1, assuming it would be rejected on all platforms.

This was bad coding practice for multiple reasons:

• Relying on an error code for important functionality.
• Using an arbitary value without API support (a PCSC implementation may chose to return -1 as a valid context).
• Assuming that SCardEstablishContext would leave the output parameter unchanged (as we now know, it was changed to 0, but any value is possible in theory).

The author updated the code in 4fc6c49, explicitly recognizing an uninitialized context instead of relying on an error from SCardListReaders. Now it would have been a good time to switch to 0 as the invalid context value, but the value -1 was kept.

[LudovicRousseau]

0 is not less or more invalid than -1. On pcsc-lite the SCARDCONTEXT is a random number in [0, RAND_MAX] (see rand(3)).
I did not find a constant value that would define an invalid SCARDCONTEXT.

If you want to check a PC/SC context is valid you can/should use the API function SCardIsValidContext().

[misterzed88]

You're right, my bad. (Being too quick when assuming that 0 was reserved in both Windows and pcsc-lite). Thanks for pointing it out.

This means that the proposed solution must be updated to not assume anything about the returned handle. I will get back with an updated fix ASAP.

[misterzed88]

A new fix was proposed, starting fresh, discarding the previous commits in this change branch.

The new fix is even more surgical - changing only the problem with the altered output parameter and adding the zero terminators as a safeguard. The -1 value is kept as the invalid handle value. Even though there are no guarantees from the API for this value being reserved, keeping it for now minimizes the risk for any unexpected side effects.

[frankmorgner]

thanks!