Possible generation of duplicate SCARDHANDLE

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

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


_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
Reply | Threaded
Open this post in threaded view
|

Re: Possible generation of duplicate SCARDHANDLE

Ludovic Rousseau
Hello Maksim,

2016-08-01 19:23 GMT+02:00 Maksim Ivanov <[hidden email]>:
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
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle

patch.txt (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: 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 <[hidden email]> wrote:
Hello Maksim,

2016-08-01 19:23 GMT+02:00 Maksim Ivanov <[hidden email]>:
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
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle



_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
Reply | Threaded
Open this post in threaded view
|

Re: Possible generation of duplicate SCARDHANDLE

Ludovic Rousseau
2016-08-02 19:59 GMT+02:00 Maksim Ivanov <[hidden email]>:
Ludovic,

Thanks for the quick update.

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

Thanks for the feedback.
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.

Good idea.
Fixed in https://github.com/LudovicRousseau/PCSC/commit/8b80aa4900cd60ab075802bdcc5a996027d0c74e and https://github.com/LudovicRousseau/PCSC/commit/8e820796b338cb1048c51ec462446f61a3979835


I also note that the number of different contexts is limited to RAND_MAX.
On my Debian GNU/Linux system I have RAND_MAX = 2147483647 so that should be enough but on some other systems may it be more limited (like 32767).

I opened a new feature request so I can remember to work on this (when time and motivation permits)
https://alioth.debian.org/tracker/index.php?func=detail&aid=315434&group_id=30105&atid=410088

Bye

--
 Dr. Ludovic Rousseau

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle