<div dir="ltr"><div>Ludovic,</div><div><br></div><div>Thanks for the quick update.</div><div><br></div><div>Yes, looks like the suggested patch fixes the problem. Great that the resulting patch is so small.</div><div><br></div><div><br></div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">I am not sure to understand your concern about srand (point 3).<br>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?</blockquote></div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div><br></div><div>Regards,</div><div>Maksim</div><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 2, 2016 at 4:00 PM, Ludovic Rousseau <span dir="ltr"><<a href="mailto:ludovic.rousseau@gmail.com" target="_blank">ludovic.rousseau@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hello Maksim,<br><div><div class="gmail_extra"><br><div class="gmail_quote"><span class="gmail-">2016-08-01 19:23 GMT+02:00 Maksim Ivanov <span dir="ltr"><<a href="mailto:emaxx@google.com" target="_blank">emaxx@google.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hello,<div><br></div><div>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).</div><div><br></div><div>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].</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div><br></div><div>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.</div><div><br></div></div></blockquote><div><br></div></span><div>Exact. I have not tried to reproduced the problem on my side.<br></div><div>Please try the attached patch and confirm the problem is fixed with the patch.<br> <br></div><span class="gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div><br></div><div>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).</div><div><br></div><div><br></div><div>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.</div><div>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).</div></div></blockquote><div><br></div></span><div>The patch adds a lock to have the hCard generation and store in the same critical section.<br><br></div><span class="gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><br></div><div>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).</div></div></blockquote><div><br></div></span><div>That are good suggestions.<br><br></div><div>I am not sure to understand your concern about srand (point 3).<br></div><div>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?<br> <br></div>Thanks<span class="gmail-HOEnZb"><font color="#888888"><br></font></span></div><span class="gmail-HOEnZb"><font color="#888888"><br>-- <br><div class="gmail-m_-3701386476503025520gmail_signature"> Dr. Ludovic Rousseau</div>
</font></span></div></div></div>
<br>_______________________________________________<br>
Pcsclite-muscle mailing list<br>
<a href="mailto:Pcsclite-muscle@lists.alioth.debian.org">Pcsclite-muscle@lists.alioth.debian.org</a><br>
<a href="http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle" rel="noreferrer" target="_blank">http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle</a><br></blockquote></div></div><p dir="ltr" style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:12.8px;line-height:1.38;margin-top:0pt;margin-bottom:0pt;background-color:rgb(255,255,255)"><br></p></div>