[Pcsclite-muscle] Possible generation of duplicate SCARDHANDLE

[Maksim Ivanov]

Ludovic,

Thanks for the quick update.

Yes, looks like the suggested patch fixes the problem. Great that the
resulting patch is so small.


I am not sure to understand your concern about srand (point 3).
> I agree the code is not thread safe. So srand could be called twice. But
> that should not be an issue. srand would be called at least once in all
> cases. Or I am missing something?


The effect of calling srand twice is the possible overlaps of the obtained
pseudo-random numbers. Also, srand is not guaranteed to work correctly at
all when used from multiple threads simultaneously.

Generally, why not just call srand once somewhere around the daemon's entry
point (int main)? Then there would be no questions regarding which
component and how should perform this initialization.


Regards,
Maksim


On Tue, Aug 2, 2016 at 4:00 PM, Ludovic Rousseau <ludovic.rousseau at gmail.com
> wrote:

> Hello Maksim,
>
> 2016-08-01 19:23 GMT+02:00 Maksim Ivanov <emaxx at google.com>:
>
>> 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.
>>
>>
> Exact. I have not tried to reproduced the problem on my side.
> Please try the attached patch and confirm the problem is fixed with the
> patch.
>
>
>>
>> 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).
>>
>
> The patch adds a lock to have the hCard generation and store in the same
> critical section.
>
>
>> 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).
>>
>
> That are good suggestions.
>
> I am not sure to understand your concern about srand (point 3).
> I agree the code is not thread safe. So srand could be called twice. But
> that should not be an issue. srand would be called at least once in all
> cases. Or I am missing something?
>
> Thanks
>
> --
>  Dr. Ludovic Rousseau
>
> _______________________________________________
> Pcsclite-muscle mailing list
> Pcsclite-muscle at lists.alioth.debian.org
> http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/pcsclite-muscle/attachments/20160802/3d96d522/attachment.html>