Possible use-after-free in RFReaderInfoById

[emaxx-google]

After a reader disappears, RFReaderInfoById() (for example, called from SCardDisconnect()) might try dereferencing an READER_CONTEXT::handlesList value that's already destroyed by the hotplug thread in removeReader().

UAF place: https://github.com/LudovicRousseau/PCSC/blob/c35130f2215b75c6d54c4f0162d68548a6de4bab/src/readerfactory.c#L860

Deallocation place: https://github.com/LudovicRousseau/PCSC/blob/c35130f2215b75c6d54c4f0162d68548a6de4bab/src/readerfactory.c#L699

P.S. It should supposedly be a very rare corner case, since normally readers aren't plugged and unplugged very often, and the overlap with another SCard operation on another thread should have right timing in order to trigger the issue.

[emaxx-google]

I'm not entirely clear on the PC/SC-Lite's threading model, so not sure about the proper fix. Maybe we could keep the handlesList_lock locked during the list destruction, but then we need to never destroy the mutex in every READER_CONTEXT.

Or a new mutex has to be introduced, for operations with the whole sReadersContexts.

[LudovicRousseau]

The use case is: a reader is removed, and at the same time a PC/SC application is calling SCardDisconnect() using this reader?

• In RFReaderInfoById() we check the value of sReadersContexts[i]->vHandle (line 856) before accessing the list sReadersContexts[i]->handlesList (line 860)
• In removeReader() we set sContext->vHandle = NULL (line 681) before destroying the list (line 699).

A problem occurs if removeReader() is executed after the check of sReadersContexts[i]->vHandle by RFReaderInfoById() but before the access to the list in RFReaderInfoById().

Is it the problem you are describing?

A very long time ago pcsc-lite used only serial readers so with a static reader configuration and no hotplug. It is possible we missed something during the evolution :-)

[emaxx-google]

Exactly, that's the problematic scenario. I observed it under ASan, so it's a real UaF, even if pretty rare to happen in practice.

I guess that there are also other similar race conditions between different threads accessing sReadersContexts.

I can try preparing a patch that adds a new mutex for this structure, if you think this direction makes sense. (I suppose the only problem is that we should make sure we don't introduce any deadlock - and I myself am still not 100% sure in my understanding of the PC/SC-Lite's threading model.)

[LudovicRousseau]

Yes, I think we need a new pthread_mutex_t to protect the accesses to sReadersContexts[]

Please propose a patch.

[LudovicRousseau]

@emaxx-google any news about a proposed patch?

[emaxx-google]

Sorry, I got distracted by some other high-priority things. Will try to get back to this soonish.

[rohoog]

Maybe issue #165 is a manifestation of this.

[LudovicRousseau]

@rohoog Maybe. Maybe not.

I asked you to upgrade pcsc-lite and try again. This will allow me to know if your bug is already fixed or not.