[Pcsclite-muscle] Possible generation of duplicate SCARDHANDLE

[Maksim Ivanov]

Hello,

Seems that it's possible for the daemon process to return equal SCARDHANDLE
values for different calls of SCardConnect (with different or the same
context).

The RFCreateReaderHandle function, which is responsible for the generation
of SCARDHANDLE values, is actually based on rand(), which is known to be
non-reentrant and non-thread-safe [1].

Even though RFCreateReaderHandle tries to protect from duplicate handles by
checking each candidate against RFReaderInfoById, it's not enough as this
is done outside mutex locks.

So if two or more SCardConnect requests are handled at the same time,
there's some probability that the same number will be generated and
accepted in some of these requests simultaneously. Considering the typical
rand implementation, the chances to get the same pseudo-random number twice
in two different threads simultaneously are quite high.


I maybe miss some protection against this in the code, but at least it's
possible to reproduce these problems with our fork of PC/SC-Lite for Chrome
OS - just a bunch of dumb clients that "connect"-"sleep"-"disconnect" in a
loop is required.


Having duplicate handles may lead to all sorts of problems within the PC/SC
clients. However, exploiting the bug may be tricky, as just stealing
other's handle doesn't mean that hijacking of data is trivial (though it
seems to be possible in theory).


If the analysis above is correct, then fixing the bug without introducing
some lock for the whole list of readers or introducing a global list of
handles seems to be non-trivial.
Probably, it would involve a spin lock around RFAddReaderHandle with a
check that the added handle produces no duplicates (but the check has to
happen _after_ the addition, as otherwise the gap between the check and the
actual insertion would still exist).

With the "defense-in-depth" concept in mind, it would be also nice to
insert more assertions into RFAddReaderHandle, RFReaderInfoById, and to
make the pseudo-random numbers generation safer (e.g. 1. using the
reentrant rand_r function; 2. not relying on the fact that RAND_MAX is big
enough - it's guaranteed to be only at least 32767; 3. making the condition
around srand to be thread-safe instead of the current use of static int; 4.
not using the naive scaling of the rand result to the required interval, as
this is not a correct transformation).


[1] "The function rand() is not reentrant or thread-safe, since it uses
hidden state that is modified on each call." <
http://linux.die.net/man/3/srand>


Regards,
Maksim Ivanov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/pcsclite-muscle/attachments/20160801/0a9f49fb/attachment.html>