[Pcsclite-muscle] Possible generation of duplicate SCARDHANDLE

[Ludovic Rousseau]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/pcsclite-muscle/attachments/20160802/7c2b17da/attachment.html>
-------------- next part --------------
colordiff is /usr/bin/colordiff
--- /tmp/s7Nwnu_winscard.c	2016-08-02 15:53:48.263881992 +0200
+++ src/winscard.c	2016-08-02 15:53:44.943827932 +0200
@@ -190,6 +190,8 @@ static void profile_end(const char *f, i
 /** used for backward compatibility */
 #define SCARD_PROTOCOL_ANY_OLD	 0x1000
 
+static pthread_mutex_t LockMutex = PTHREAD_MUTEX_INITIALIZER;
+
 LONG SCardEstablishContext(DWORD dwScope, /*@unused@*/ LPCVOID pvReserved1,
 	/*@unused@*/ LPCVOID pvReserved2, LPSCARDCONTEXT phContext)
 {
@@ -444,6 +446,11 @@ LONG SCardConnect(/*@unused@*/ SCARDCONT
 	/*
 	 * Prepare the SCARDHANDLE identity
 	 */
+
+	/* we need a lock to avoid concurent generation of handles leading
+	 * to a possible hCard handle duplication */
+	(void)pthread_mutex_lock(&LockMutex);
+
 	*phCard = RFCreateReaderHandle(rContext);
 
 	Log2(PCSC_LOG_DEBUG, "hCard Identity: %lx", *phCard);
@@ -467,6 +474,7 @@ LONG SCardConnect(/*@unused@*/ SCARDCONT
 			(void)RFDestroyReaderHandle(*phCard);
 			*phCard = 0;
 			rv = SCARD_E_SHARING_VIOLATION;
+			(void)pthread_mutex_unlock(&LockMutex);
 			goto exit;
 		}
 	}
@@ -483,6 +491,8 @@ LONG SCardConnect(/*@unused@*/ SCARDCONT
 	 */
 	rv = RFAddReaderHandle(rContext, *phCard);
 
+	(void)pthread_mutex_unlock(&LockMutex);
+
 	if (rv != SCARD_S_SUCCESS)
 	{
 		/*