Fedict / eid-mw

eID Middleware (main repository)
GNU Lesser General Public License v3.0
198 stars 79 forks source link

SCardGetStatusChange busy wait causes lots of unneeded calls #161

Closed rubendv closed 2 years ago

rubendv commented 2 years ago

It seems that the middleware library is calling the SCardGetStatusChange function with the current state of the special \\?PnP?\Notification reader always set to SCARD_STATE_UNAWARE (= 0). This causes the function to return immediately and causes a lot of calls to SCardGetStatusChangeW. This is probably not that bad for locally connected smartcards (except for high CPU usage perhaps). But we use smartcard redirection over the RDP protocol, which means each of these calls causes additional network traffic to our HTML5 RDP gateway and finally to the client device. When we redirect locally attached smartcard readers it causes a constant stream of smartcard calls over the network when the eid middleware is installed on the remote server.

The Microsoft WinSCard documentation seems to be unclear on this point and even contradicts itself:

https://docs.microsoft.com/en-us/windows/win32/api/winscard/ns-winscard-scard_readerstatea mentions:

Set the value of this member (szReader) to "\?PnP?\Notification" and the values of all other members to zero to be notified of the arrival of a new smart card reader.

But also:

SCARD_STATE_UNAWARE | The application is unaware of the current state, and would like to know. The use of this value results in an immediate return from state transition monitoring services. This is represented by all bits set to zero.

These quotes contradict each other because it implies that the call will wait for new readers, but in practice it returns immediately even with no change.

This stack overflow answer (https://stackoverflow.com/a/16467368) mentions that the proper use of the special \\?PnP?\Notification reader is to set the number of expected readers in the high 16 bits of the current state.

The RDP smartcard redirection documentation confirms this and offers more information about the meaning of the high 16 bits of the other reader states: https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpesc/3a235960-2fec-446b-8ed1-50bcc70e3c5f

If you look at the values that are returned you can indeed see that the high 16 bits are also set to the number of detected readers when returning from SCardGetStatusChangeW. In the below trace the dwEventState of the PnP reader is set to 0x00020002, the last 2 is for SCARD_STATE_CHANGED and the first 2 is the number of readers in the high 16 bits. I expect the proper dwCurrentState for the next call to make sure it blocks until a change is made to the list of readers is either 0x00020000 or 0x00020002?

I expect the high 16 bits of the "normal" readers will also need to be resent in subsequent calls, this is probably a way to detect card inserts/removal events between statuschange calls.

- GetStatusChangeW_Call {
- hContext: 0x010017CD (4)
- dwTimeOut: 0xFFFFFFFF cReaders: 3
-   [0]: szReader: \\?PnP?\Notification cbAtr: 0
-   [0]: dwCurrentState: SCARD_STATE_UNAWARE (0x00000000)
-   [0]: dwEventState: SCARD_STATE_UNAWARE (0x00000000)
-   [1]: szReader: ACS CCID USB Reader 0 cbAtr: 18
-   [1]: dwCurrentState: SCARD_STATE_PRESENT (0x00000020)
-   [1]: dwEventState: SCARD_STATE_UNAWARE (0x00000000)
-   [2]: szReader: Alcor Micro USB Smart Card Reader 0 cbAtr: 13
-   [2]: dwCurrentState: SCARD_STATE_PRESENT (0x00000020)
-   [2]: dwEventState: SCARD_STATE_UNAWARE (0x00000000)
- }
- GetStatusChangeW_Return {
- ReturnCode: SCARD_S_SUCCESS (0x00000000)
- cReaders: 3
-   [0]: dwCurrentState: SCARD_STATE_UNAWARE (0x00000000)
-   [0]: dwEventState: SCARD_STATE_CHANGED (0x00020002)
-   [0]: cbAtr: 0 rgbAtr: 
-   [1]: dwCurrentState: SCARD_STATE_PRESENT (0x00000020)
-   [1]: dwEventState: SCARD_STATE_CHANGED | SCARD_STATE_PRESENT | SCARD_STATE_UNPOWERED (0x00030422)
-   [1]: cbAtr: 18 rgbAtr: 3B6E000080318066B0840C016E0183009000
-   [2]: dwCurrentState: SCARD_STATE_PRESENT (0x00000020)
-   [2]: dwEventState: SCARD_STATE_CHANGED | SCARD_STATE_PRESENT | SCARD_STATE_UNPOWERED (0x00010422)
-   [2]: cbAtr: 13 rgbAtr: 3B9813400AA503010101AD1311
- }
rubendv commented 2 years ago

It seems like we were clearing the high 16 bits of dwCurrentState in our unpacking code, which happens before the trace output I posted above. So I'll be closing the issue. Sorry for the inconvenience!