[PATCH] ContextThread: SCARD_TRANSMIT: work around CT API recv buffer size of 64k

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] ContextThread: SCARD_TRANSMIT: work around CT API recv buffer size of 64k

Marc Kleine-Budde
In commit:

    8eb9ea1b354b SCardTransmit() may return SCARD_E_INSUFFICIENT_BUFFER

the recv buffer size, passed to the SCardTransmit() function, is set
unconditionally to "sizeof pbRecvBuffer", which is 64k + 12. This leads to
problems when the CT API is used in the lower layers, as the CT API implements
a maximum recv buffer size of 64k.

This leads to the truncation of the recv buffer size to 12. If the client has
supplied a buffer of >12 bytes, resulting in truncated reads. This patch tries
to work around the problem, by not unconditionally passing the recv buffer size
of "sizeof pbRecvBuffer" (64k + 12), but increasing the client supplied buffer
by one, keeping the "sizeof pbRecvBuffer" as an upper bound. This way a too
small recv buffer passed by the client can still be detected, but the CT API
limit of 64k is not exceeded if the buffer is below 64k.

Cc: Marcin Cieslak <[hidden email]>
---
 src/winscard_svc.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/src/winscard_svc.c b/src/winscard_svc.c
index 75e4c8e4e8e1..a623fd60f631 100644
--- a/src/winscard_svc.c
+++ b/src/winscard_svc.c
@@ -636,7 +636,20 @@ static void ContextThread(LPVOID newContext)
  ioSendPci.cbPciLength = trStr.ioSendPciLength;
  ioRecvPci.dwProtocol = trStr.ioRecvPciProtocol;
  ioRecvPci.cbPciLength = trStr.ioRecvPciLength;
+ /* The CT API implements a max recv buffer size of 64k,
+ * while "sizeof pbRecvBuffer" is "64k + 12". This leads
+ * to trunkation of max recv buffer size to "12" when
+ * using "sizeof pbRecvBuffer", even if the client
+ * specifies a much smaller recv buffer.
+ *
+ * Here we increase the client buffer by one
+ * (but keeping "sizeof pbRecvBuffer" as maximum),
+ * so that we can detect a too small client buffer
+ * later.
+ */
  cbRecvLength = sizeof pbRecvBuffer;
+ if (cbRecvLength > trStr.pcbRecvLength + 1)
+ cbRecvLength = trStr.pcbRecvLength + 1;
 
  trStr.rv = SCardTransmit(trStr.hCard, &ioSendPci,
  pbSendBuffer, trStr.cbSendLength, &ioRecvPci,
--
2.6.2


_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ContextThread: SCARD_TRANSMIT: work around CT API recv buffer size of 64k

Ludovic Rousseau
Hello,

Why can't you fix the problem in the CT-API ifdhandler?
You can use the same patch in IFDHTransmitToICC() [1] of your driver.

I don't think the problem is on the pcsc-lite side.
Please fix your IFDHandler driver.

Regards,

[1] https://pcsclite.alioth.debian.org/api/group__IFDHandler.html#gac86e07f01d11accda93fb80d3935eeed

2015-12-08 13:39 GMT+01:00 Marc Kleine-Budde <[hidden email]>:
In commit:

    8eb9ea1b354b SCardTransmit() may return SCARD_E_INSUFFICIENT_BUFFER

the recv buffer size, passed to the SCardTransmit() function, is set
unconditionally to "sizeof pbRecvBuffer", which is 64k + 12. This leads to
problems when the CT API is used in the lower layers, as the CT API implements
a maximum recv buffer size of 64k.

This leads to the truncation of the recv buffer size to 12. If the client has
supplied a buffer of >12 bytes, resulting in truncated reads. This patch tries
to work around the problem, by not unconditionally passing the recv buffer size
of "sizeof pbRecvBuffer" (64k + 12), but increasing the client supplied buffer
by one, keeping the "sizeof pbRecvBuffer" as an upper bound. This way a too
small recv buffer passed by the client can still be detected, but the CT API
limit of 64k is not exceeded if the buffer is below 64k.

Cc: Marcin Cieslak <[hidden email]>
---
 src/winscard_svc.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/src/winscard_svc.c b/src/winscard_svc.c
index 75e4c8e4e8e1..a623fd60f631 100644
--- a/src/winscard_svc.c
+++ b/src/winscard_svc.c
@@ -636,7 +636,20 @@ static void ContextThread(LPVOID newContext)
                                ioSendPci.cbPciLength = trStr.ioSendPciLength;
                                ioRecvPci.dwProtocol = trStr.ioRecvPciProtocol;
                                ioRecvPci.cbPciLength = trStr.ioRecvPciLength;
+                               /* The CT API implements a max recv buffer size of 64k,
+                                * while "sizeof pbRecvBuffer" is "64k + 12". This leads
+                                * to trunkation of max recv buffer size to "12" when
+                                * using "sizeof pbRecvBuffer", even if the client
+                                * specifies a much smaller recv buffer.
+                                *
+                                * Here we increase the client buffer by one
+                                * (but keeping "sizeof pbRecvBuffer" as maximum),
+                                * so that we can detect a too small client buffer
+                                * later.
+                                */
                                cbRecvLength = sizeof pbRecvBuffer;
+                               if (cbRecvLength > trStr.pcbRecvLength + 1)
+                                       cbRecvLength = trStr.pcbRecvLength + 1;

                                trStr.rv = SCardTransmit(trStr.hCard, &ioSendPci,
                                        pbSendBuffer, trStr.cbSendLength, &ioRecvPci,
--
2.6.2


_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle



--
 Dr. Ludovic Rousseau

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ContextThread: SCARD_TRANSMIT: work around CT API recv buffer size of 64k

Marc Kleine-Budde
On 12/08/2015 09:04 PM, Ludovic Rousseau wrote:
> Why can't you fix the problem in the CT-API ifdhandler?
> You can use the same patch in IFDHTransmitToICC() [1] of your driver.

I'm using openct. Which internally uses unsigned short for the length of
the recv buffer. But Marcin has prepared a patch for it [1]

> I don't think the problem is on the pcsc-lite side.

Hmm, from my point of view commit "8eb9ea1b354b SCardTransmit() may
return SCARD_E_INSUFFICIENT_BUFFER" breaks all existing openct based
drivers.

> Please fix your IFDHandler driver.

I hope someone in the openct teams listens.

regards,
Marc

[1] https://github.com/OpenSC/openct/pull/5
--
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle

signature.asc (465 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ContextThread: SCARD_TRANSMIT: work around CT API recv buffer size of 64k

Marcin Cieslak-3
In reply to this post by Ludovic Rousseau
On Tue, 8 Dec 2015, Ludovic Rousseau wrote:

> Hello,
>
> Why can't you fix the problem in the CT-API ifdhandler?
> You can use the same patch in IFDHTransmitToICC() [1] of your driver.
>
> I don't think the problem is on the pcsc-lite side.
> Please fix your IFDHandler driver.

Well, of course the source of the problem is the CT-API and
its openct (currently dead) implementation. There is no
doubt about this.

But the change in pcscd, while technically correct, has
introduced a serious interoperability problem.

I wish there would be a way for the IFD driver to say
"I support extended APDUs and large transfers". That
would be a much better solution. (Maybe there is a way?
I'm not an expert on the PC/SC family of standards).

It reminds me of the situation in the packet networks -
- in the old times there were links that could not push
packets larger than, say, 1002 bytes and nobody insists
(even today!) that Ethernet jumboframes have to be pushed via
typical serial links.

I believe that good engineering should take such factors
into account.

Marcin

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ContextThread: SCARD_TRANSMIT: work around CT API recv buffer size of 64k

Ludovic Rousseau
Hello,

2015-12-29 21:21 GMT+01:00 Marcin Cieslak <[hidden email]>:
On Tue, 8 Dec 2015, Ludovic Rousseau wrote:

> Hello,
>
> Why can't you fix the problem in the CT-API ifdhandler?
> You can use the same patch in IFDHTransmitToICC() [1] of your driver.
>
> I don't think the problem is on the pcsc-lite side.
> Please fix your IFDHandler driver.

Well, of course the source of the problem is the CT-API and
its openct (currently dead) implementation. There is no
doubt about this.

I can add you (or someone else) in the OpenCT maintainers [1] github group so that you can fix OpenCT and make a new release.
Would that help you?

Regards,

[1] https://github.com/orgs/OpenSC/teams/openct-maintainers

--
 Dr. Ludovic Rousseau

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ContextThread: SCARD_TRANSMIT: work around CT API recv buffer size of 64k

Marcin Cieslak-3
On Tue, 29 Dec 2015, Ludovic Rousseau wrote:

> Hello,
>
> I can add you (or someone else) in the OpenCT maintainers [1] github group
> so that you can fix OpenCT and make a new release.
> Would that help you?
>
> Regards,
>
> [1] https://github.com/orgs/OpenSC/teams/openct-maintainers

Sure, that helps. I am "saper" on GitHub. I also have some
early hacks to get an obscure non-CCID OminKey USB reader
to work with openct.

My patch to openct currently just truncates the buffer size.

Speaking of a right fix:

What is the proper way for the IFD to say to pcscd
"your buffer is too large, please back off"?

Marcin

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] ContextThread: SCARD_TRANSMIT: work around CT API recv buffer size of 64k

Ludovic Rousseau


2015-12-31 15:06 GMT+01:00 Marcin Cieslak <[hidden email]>:
On Tue, 29 Dec 2015, Ludovic Rousseau wrote:

> Hello,
>
> I can add you (or someone else) in the OpenCT maintainers [1] github group
> so that you can fix OpenCT and make a new release.
> Would that help you?
>
> Regards,
>
> [1] https://github.com/orgs/OpenSC/teams/openct-maintainers

Sure, that helps. I am "saper" on GitHub. I also have some
early hacks to get an obscure non-CCID OminKey USB reader
to work with openct.

I just sent you an invitation to join the openct-maintainers team
https://github.com/orgs/OpenSC/teams/openct-maintainers

 
My patch to openct currently just truncates the buffer size.

Speaking of a right fix:

What is the proper way for the IFD to say to pcscd
"your buffer is too large, please back off"?

You can't.
It is the job of the IFDHandler to adapt and use a smaller buffer to please OpenCT.

Bye

--
 Dr. Ludovic Rousseau

_______________________________________________
Pcsclite-muscle mailing list
[hidden email]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/pcsclite-muscle