From 14048123a2b00ed2bf530e4a4701ba249733310f Mon Sep 17 00:00:00 2001 From: Zhiqiang Liu Date: Thu, 29 Oct 2020 22:09:48 +0800 Subject: [PATCH] pcsc-lite: backport some patches to solve some upstream problems backport some patches to solve some upstream problems Signed-off-by: Zhiqiang Liu --- ...lock-a-reader-if-allocating-hCard-fa.patch | 78 +++++++++++++++++++ 0002-Fix-a-hang-in-SCardTransmit.patch | 56 +++++++++++++ ...-always-initialize-the-return-values.patch | 52 +++++++++++++ ...ntForEvent-correctly-handle-EHTryToU.patch | 36 +++++++++ pcsc-lite.spec | 9 ++- 5 files changed, 230 insertions(+), 1 deletion(-) create mode 100644 0001-Do-not-possibly-lock-a-reader-if-allocating-hCard-fa.patch create mode 100644 0002-Fix-a-hang-in-SCardTransmit.patch create mode 100644 0003-ATRDecodeAtr-always-initialize-the-return-values.patch create mode 100644 0004-EHUnregisterClientForEvent-correctly-handle-EHTryToU.patch diff --git a/0001-Do-not-possibly-lock-a-reader-if-allocating-hCard-fa.patch b/0001-Do-not-possibly-lock-a-reader-if-allocating-hCard-fa.patch new file mode 100644 index 0000000..ae4574c --- /dev/null +++ b/0001-Do-not-possibly-lock-a-reader-if-allocating-hCard-fa.patch @@ -0,0 +1,78 @@ +From 36bc9446b40fa3c6ac12312b934f4d7131659087 Mon Sep 17 00:00:00 2001 +From: Ludovic Rousseau +Date: Wed, 5 Aug 2020 17:59:41 +0200 +Subject: [PATCH 01/13] Do not (possibly) lock a reader if allocating hCard + fails + +In case of SCardConnect() the reader may be locked in +SCARD_SHARE_EXCLUSIVE mode if internal SCardConnect() works but +MSGAddHandle() fails because the list of handle is full. + +You need to start pcscd with "--max-card-handle-per-reader n" with +n > 200 or the 200 limit (default value) will be hit in internal +SCardConnect() and MSGAddHandle() would not be called. + +Thanks to Maksim Ivanov for the bug report +"[Pcsclite-muscle] SCardConnect behavior with invalid contexts" +http://lists.infradead.org/pipermail/pcsclite-muscle/2020-July/001095.html + +" Hello, + +I believe that there's a potential problem with the SCardConnect +implementation that it doesn't check the received SCARDCONTEXT +*before* executing the command. This might result in an unexpected +state, where the SCardConnect() caller receives an error code +meanwhile the connection to the card is actually established (which, +for example, might be an exclusive connection that prevents anyone +else from connecting to the card). + +In detail, the ContextThread() function in winscard_svc.c, when +receiving the SCARD_CONNECT command, calls first SCardConnect() from +winscard.c, and then MSGAddHandle(). The former ignores SCARDCONTEXT +and, if possible, establishes a connection to the card. The latter +does check the SCARDCONTEXT value, but this happens after the +connection is already established, and its error is just returned to +the caller (without closing the just-opened connection). + +Would it make sense to add a check of SCARDCONTEXT before calling +SCardConnect(), and/or to call SCardDisconnect() if MSGAddHandle() +fails? + +Regards, +Maksim " +--- + src/winscard_svc.c | 8 +++++++- + 1 file changed, 7 insertions(+), 1 deletion(-) + +diff --git a/src/winscard_svc.c b/src/winscard_svc.c +index cdeac33..c0df008 100644 +--- a/src/winscard_svc.c ++++ b/src/winscard_svc.c +@@ -507,9 +507,15 @@ static void * ContextThread(LPVOID newContext) + coStr.dwActiveProtocol = dwActiveProtocol; + + if (coStr.rv == SCARD_S_SUCCESS) ++ { + coStr.rv = MSGAddHandle(coStr.hContext, coStr.hCard, + threadContext); + ++ /* if storing the hCard fails we disconnect */ ++ if (coStr.rv != SCARD_S_SUCCESS) ++ SCardDisconnect(coStr.hCard, SCARD_LEAVE_CARD); ++ } ++ + WRITE_BODY(coStr); + } + break; +@@ -963,7 +969,7 @@ static LONG MSGAddHandle(SCARDCONTEXT hContext, SCARDHANDLE hCard, + if (listLength >= contextMaxCardHandles) + { + Log4(PCSC_LOG_DEBUG, +- "Too many card handles for thread context @%p: %d (max is %d)" ++ "Too many card handles for thread context @%p: %d (max is %d). " + "Restart pcscd with --max-card-handle-per-thread value", + threadContext, listLength, contextMaxCardHandles); + retval = SCARD_E_NO_MEMORY; +-- +1.8.3.1 + diff --git a/0002-Fix-a-hang-in-SCardTransmit.patch b/0002-Fix-a-hang-in-SCardTransmit.patch new file mode 100644 index 0000000..00fa499 --- /dev/null +++ b/0002-Fix-a-hang-in-SCardTransmit.patch @@ -0,0 +1,56 @@ +From 38dfe5c1f474db519e1f7e31cf714ba5d4c6cfa4 Mon Sep 17 00:00:00 2001 +From: Ludovic Rousseau +Date: Wed, 5 Aug 2020 18:57:30 +0200 +Subject: [PATCH 02/13] Fix a hang in SCardTransmit() + +In some special conditions it is possible to make SCardTransmit() to +hang forever in pcscd and generates a denial of service. + +I was able to reproduce the problem using a sample C code. + +Thanks to Maksim Ivanov for the bug report +"[Pcsclite-muscle] Potential hang in SCardTransmit" +http://lists.infradead.org/pipermail/pcsclite-muscle/2020-July/001096.html + +" Hello, + +It seems that there's (at least half-hypothetical) scenario when +SCardTransmit may hang. + +The combination is: +the service's |readerState| is (SCARD_PRESENT | SCARD_POWERED | +SCARD_NEGOTIABLE); +the service's |cardProtocol| is SCARD_PROTOCOL_UNDEFINED (right after +power-up); +the caller's |pioSendPci->dwProtocol| is SCARD_PROTOCOL_ANY_OLD. + +In that case, the hang happens in the loop that attempts to find the +highest bit in the |cardProtocol| value; it doesn't handle the case +when the latter is zero: +https://salsa.debian.org/rousseau/PCSC/-/blob/467df10d439f6d739cd48a51f2b3dd543b1a64ce/src/winscard.c#L1583 + +P.S. Sorry if I misunderstood something and this case can never occur +in practice. + +Regards, +Maksim " +--- + src/winscard.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/src/winscard.c b/src/winscard.c +index 9f24cd7..3b88554 100644 +--- a/src/winscard.c ++++ b/src/winscard.c +@@ -1580,7 +1580,7 @@ LONG SCardTransmit(SCARDHANDLE hCard, const SCARD_IO_REQUEST *pioSendPci, + unsigned long i; + unsigned long prot = rContext->readerState->cardProtocol; + +- for (i = 0 ; prot != 1 ; i++) ++ for (i = 0 ; prot != 1 && i < 16; i++) + prot >>= 1; + + sSendPci.Protocol = i; +-- +1.8.3.1 + diff --git a/0003-ATRDecodeAtr-always-initialize-the-return-values.patch b/0003-ATRDecodeAtr-always-initialize-the-return-values.patch new file mode 100644 index 0000000..05a4594 --- /dev/null +++ b/0003-ATRDecodeAtr-always-initialize-the-return-values.patch @@ -0,0 +1,52 @@ +From a706455f31178ab35f07e3e6e76bd4a35d7ef3da Mon Sep 17 00:00:00 2001 +From: Ludovic Rousseau +Date: Sat, 8 Aug 2020 15:11:53 +0200 +Subject: [PATCH 03/13] ATRDecodeAtr: always initialize the return values + +Always set a value to availableProtocols and currentProtocol before any +return in error. + +Thanks to Maksim Ivanov for the bug report +"[Pcsclite-muscle] Missing checks of ATRDecodeAtr returns" +http://lists.infradead.org/pipermail/pcsclite-muscle/2020-July/001097.html + +" Hello, + +The callers of the ATRDecodeAtr() function (SCardConnect() and +SCardReconnect() in winscard.c) don't check its return value, which +might potentially cause reads of uninitialized variables +|availableProtocols| and |defaultProtocol| and unexpected side +effects. + +Regards, +Maksim " +--- + src/atrhandler.c | 6 +++--- + 1 file changed, 3 insertions(+), 3 deletions(-) + +diff --git a/src/atrhandler.c b/src/atrhandler.c +index 2ebc440..1e0654d 100644 +--- a/src/atrhandler.c ++++ b/src/atrhandler.c +@@ -75,15 +75,15 @@ short ATRDecodeAtr(int *availableProtocols, int *currentProtocol, + LogXxd(PCSC_LOG_DEBUG, "ATR: ", pucAtr, dwLength); + #endif + +- if (dwLength < 2) +- return 0; /** @retval 0 Atr must have TS and T0 */ +- + /* + * Zero out the bitmasks + */ + *availableProtocols = SCARD_PROTOCOL_UNDEFINED; + *currentProtocol = SCARD_PROTOCOL_UNDEFINED; + ++ if (dwLength < 2) ++ return 0; /** @retval 0 Atr must have TS and T0 */ ++ + /* + * Decode the TS byte + */ +-- +1.8.3.1 + diff --git a/0004-EHUnregisterClientForEvent-correctly-handle-EHTryToU.patch b/0004-EHUnregisterClientForEvent-correctly-handle-EHTryToU.patch new file mode 100644 index 0000000..7ffeb33 --- /dev/null +++ b/0004-EHUnregisterClientForEvent-correctly-handle-EHTryToU.patch @@ -0,0 +1,36 @@ +From 278b55a87a5f4b9bd86513f7d8f9ab7d66558602 Mon Sep 17 00:00:00 2001 +From: Ludovic Rousseau +Date: Sat, 8 Aug 2020 17:37:40 +0200 +Subject: [PATCH 05/13] EHUnregisterClientForEvent: correctly handle + EHTryToUnregisterClientForEvent + +EHTryToUnregisterClientForEvent() returns SCARD_S_SUCCESS or +SCARD_F_INTERNAL_ERROR but never a negative value. + +Thanks to Valerii Zapodovnikov for the bug report +"Code cleanup" +https://salsa.debian.org/rousseau/PCSC/-/issues/19 + +" https://salsa.debian.org/rousseau/PCSC/-/blob/master/src/eventhandler.c#L107 +rv < 0 is always false, because on line 94 there SCARD_F_INTERNAL_ERROR +is ((LONG)0x80100001 and SCARD_S_SUCCESS is ((LONG)0x00000000). " +--- + src/eventhandler.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/src/eventhandler.c b/src/eventhandler.c +index 932d30b..8d450d5 100644 +--- a/src/eventhandler.c ++++ b/src/eventhandler.c +@@ -104,7 +104,7 @@ LONG EHUnregisterClientForEvent(int32_t filedes) + { + LONG rv = EHTryToUnregisterClientForEvent(filedes); + +- if (rv < 0) ++ if (rv != SCARD_S_SUCCESS) + Log2(PCSC_LOG_ERROR, "Can't remove client: %d", filedes); + + return rv; +-- +1.8.3.1 + diff --git a/pcsc-lite.spec b/pcsc-lite.spec index 12f73ca..9dc1456 100644 --- a/pcsc-lite.spec +++ b/pcsc-lite.spec @@ -1,6 +1,6 @@ Name: pcsc-lite Version: 1.9.0 -Release: 1 +Release: 2 Summary: Middleware to access a smart card using SCard API (PC/SC) License: BSD URL: https://pcsclite.apdu.fr/ @@ -19,6 +19,10 @@ Provides: pcsc-lite-libs%{?_isa} pcsc-lite-libs Obsoletes: pcsc-lite-libs Patch0: 0000-pcsc-lite-change-to-use-python3-for-pcsc-spy.patch +Patch1: 0001-Do-not-possibly-lock-a-reader-if-allocating-hCard-fa.patch +Patch2: 0002-Fix-a-hang-in-SCardTransmit.patch +Patch3: 0003-ATRDecodeAtr-always-initialize-the-return-values.patch +Patch4: 0004-EHUnregisterClientForEvent-correctly-handle-EHTryToU.patch %description PC/SC Lite is a middleware to access a smart card using SCard API (PC/SC). @@ -112,6 +116,9 @@ mkdir -p %{buildroot}/%{_localstatedir}/run/pcscd %changelog +* Thu Oct 29 2020 Zhiqiang Liu - 1.9.0-2 +- backport some patches to solve some upstream problems + * Tue Jul 21 2020 jixinjie - 1.9.0-1 - update package to 1.9.0