Wrong check for response APDU buffer

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

Wrong check for response APDU buffer

Frank Morgner
Hi!

Currently, sc_check_apdu checks the length of an R-APDU buffer using
SC_MAX_APDU_BUFFER_SIZE, which defines the maximum length for a C-APDU.
https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/apdu.c#L415
https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/apdu.c#L392

A quick fix would be to use 0xff+1/0xffff+1 instead. However, in
multiple files I have seen this wrong usage of SC_MAX_APDU_BUFFER_SIZE
(eg, see `grep rbuf *.c | grep SC_MAX_APDU_BUFFER_SIZE`). Unfortunately
I dont have time to check libopensc in depth, so I leave this up to the
community.

--
Frank Morgner

Virtual Smart Card Architecture http://vsmartcard.sourceforge.net
OpenPACE                        http://openpace.sourceforge.net
IFD Handler for libnfc Devices  http://sourceforge.net/projects/ifdnfc

_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc-project.org/mailman/listinfo/opensc-devel

attachment0 (501 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Wrong check for response APDU buffer

Douglas E. Engert


On 12/7/2012 5:15 PM, Frank Morgner wrote:
> Hi!
>
> Currently, sc_check_apdu checks the length of an R-APDU buffer using
> SC_MAX_APDU_BUFFER_SIZE, which defines the maximum length for a C-APDU.
> https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/apdu.c#L415
> https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/apdu.c#L392

Yes this looks like a bug as SC_MAX_APDU_BUFFER_SIZE is for max size of ADPU
that can be sent, not size of receive buffer:
#define SC_MAX_APDU_BUFFER_SIZE         261 /* takes account of: CLA INS P1 P2 Lc [255 byte of data] Le */


>
> A quick fix would be to use 0xff+1/0xffff+1 instead. However, in
> multiple files I have seen this wrong usage of SC_MAX_APDU_BUFFER_SIZE
> (eg, see `grep rbuf *.c | grep SC_MAX_APDU_BUFFER_SIZE`). Unfortunately
> I dont have time to check libopensc in depth, so I leave this up to the
> community.
>

Do you mean something like this:

--- ,apdu.c     Tue Dec  4 08:43:40 2012
+++ apdu.c      Tue Dec 11 09:50:50 2012
@@ -389,7 +389,7 @@
                 if (apdu->resplen == 0 || apdu->resp == NULL)
                         goto error;
                 /* return buffer to small */
-               if ((apdu->le == 0 && apdu->resplen < SC_MAX_APDU_BUFFER_SIZE-2)
+               if ((apdu->le == 0 && apdu->resplen < ((apdu->cse & SC_APDU_EXT) ? 65536 : 256))
                                 || (apdu->resplen < apdu->le))
                         goto error;
                 break;
@@ -412,7 +412,7 @@
                 if (apdu->resplen == 0 || apdu->resp == NULL)
                         goto error;
                 /* return buffer to small */
-               if ((apdu->le == 0 && apdu->resplen < SC_MAX_APDU_BUFFER_SIZE-2)
+       if ((apdu->le == 0 && apdu->resplen < ((apdu->cse & SC_APDU_EXT) ? 65536 : 256)
                                 || (apdu->resplen < apdu->le))
                         goto error;
                 /* inconsistent datalen   */



>
>
> _______________________________________________
> opensc-devel mailing list
> [hidden email]
> http://www.opensc-project.org/mailman/listinfo/opensc-devel
>

--

  Douglas E. Engert  <[hidden email]>
  Argonne National Laboratory
  9700 South Cass Avenue
  Argonne, Illinois  60439
  (630) 252-5444
_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc-project.org/mailman/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: Wrong check for response APDU buffer

Frank Morgner
On Tuesday, December 11 at 09:59AM, Douglas E. Engert wrote:

>
>
>
> On 12/7/2012 5:15 PM, Frank Morgner wrote:
> > Hi!
> >
> > Currently, sc_check_apdu checks the length of an R-APDU buffer using
> > SC_MAX_APDU_BUFFER_SIZE, which defines the maximum length for a C-APDU.
> > https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/apdu.c#L415
> > https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/apdu.c#L392
>
> Yes this looks like a bug as SC_MAX_APDU_BUFFER_SIZE is for max size of ADPU
> that can be sent, not size of receive buffer:
> #define SC_MAX_APDU_BUFFER_SIZE         261 /* takes account of: CLA INS P1 P2 Lc [255 byte of data] Le */
>
>
> >
> > A quick fix would be to use 0xff+1/0xffff+1 instead. However, in
> > multiple files I have seen this wrong usage of SC_MAX_APDU_BUFFER_SIZE
> > (eg, see `grep rbuf *.c | grep SC_MAX_APDU_BUFFER_SIZE`). Unfortunately
> > I dont have time to check libopensc in depth, so I leave this up to the
> > community.
> >
>
> Do you mean something like this:
>
> --- ,apdu.c     Tue Dec  4 08:43:40 2012
> +++ apdu.c      Tue Dec 11 09:50:50 2012
> @@ -389,7 +389,7 @@
>                  if (apdu->resplen == 0 || apdu->resp == NULL)
>                          goto error;
>                  /* return buffer to small */
> -               if ((apdu->le == 0 && apdu->resplen < SC_MAX_APDU_BUFFER_SIZE-2)
> +               if ((apdu->le == 0 && apdu->resplen < ((apdu->cse & SC_APDU_EXT) ? 65536 : 256))
>                                  || (apdu->resplen < apdu->le))
>                          goto error;
>                  break;
> @@ -412,7 +412,7 @@
>                  if (apdu->resplen == 0 || apdu->resp == NULL)
>                          goto error;
>                  /* return buffer to small */
> -               if ((apdu->le == 0 && apdu->resplen < SC_MAX_APDU_BUFFER_SIZE-2)
> +       if ((apdu->le == 0 && apdu->resplen < ((apdu->cse & SC_APDU_EXT) ? 65536 : 256)
>                                  || (apdu->resplen < apdu->le))
>                          goto error;
>                  /* inconsistent datalen   */
Yes, but I would use a define instead of hard coded values. Please have
in mind that the rest of the source code should be checked, too. The
following grep shows 65 hits which should be changed to use the new
define:

    grep -R SC_MAX * |egrep '(rbuf|recvbuf)'

--
Frank Morgner

Virtual Smart Card Architecture http://vsmartcard.sourceforge.net
OpenPACE                        http://openpace.sourceforge.net
IFD Handler for libnfc Devices  http://sourceforge.net/projects/ifdnfc

_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc-project.org/mailman/listinfo/opensc-devel

attachment0 (501 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Wrong check for response APDU buffer

Douglas E. Engert


On 12/11/2012 3:27 PM, Frank Morgner wrote:

> On Tuesday, December 11 at 09:59AM, Douglas E. Engert wrote:
>>
>>
>>
>> On 12/7/2012 5:15 PM, Frank Morgner wrote:
>>> Hi!
>>>
>>> Currently, sc_check_apdu checks the length of an R-APDU buffer using
>>> SC_MAX_APDU_BUFFER_SIZE, which defines the maximum length for a C-APDU.
>>> https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/apdu.c#L415
>>> https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/apdu.c#L392
>>
>> Yes this looks like a bug as SC_MAX_APDU_BUFFER_SIZE is for max size of ADPU
>> that can be sent, not size of receive buffer:
>> #define SC_MAX_APDU_BUFFER_SIZE         261 /* takes account of: CLA INS P1 P2 Lc [255 byte of data] Le */
>>
>>
>>>
>>> A quick fix would be to use 0xff+1/0xffff+1 instead. However, in
>>> multiple files I have seen this wrong usage of SC_MAX_APDU_BUFFER_SIZE
>>> (eg, see `grep rbuf *.c | grep SC_MAX_APDU_BUFFER_SIZE`). Unfortunately
>>> I dont have time to check libopensc in depth, so I leave this up to the
>>> community.
>>>
>>
>> Do you mean something like this:
>>
>> --- ,apdu.c     Tue Dec  4 08:43:40 2012
>> +++ apdu.c      Tue Dec 11 09:50:50 2012
>> @@ -389,7 +389,7 @@
>>                   if (apdu->resplen == 0 || apdu->resp == NULL)
>>                           goto error;
>>                   /* return buffer to small */
>> -               if ((apdu->le == 0 && apdu->resplen < SC_MAX_APDU_BUFFER_SIZE-2)
>> +               if ((apdu->le == 0 && apdu->resplen < ((apdu->cse & SC_APDU_EXT) ? 65536 : 256))
>>                                   || (apdu->resplen < apdu->le))
>>                           goto error;
>>                   break;
>> @@ -412,7 +412,7 @@
>>                   if (apdu->resplen == 0 || apdu->resp == NULL)
>>                           goto error;
>>                   /* return buffer to small */
>> -               if ((apdu->le == 0 && apdu->resplen < SC_MAX_APDU_BUFFER_SIZE-2)
>> +       if ((apdu->le == 0 && apdu->resplen < ((apdu->cse & SC_APDU_EXT) ? 65536 : 256)
>>                                   || (apdu->resplen < apdu->le))
>>                           goto error;
>>                   /* inconsistent datalen   */
>
> Yes, but I would use a define instead of hard coded values.

The 65536 and 256 are also used in other places in the module, so I used
the numbers. That not to say that defines could not be used.

Did you just notice the code looked wrong or do you have a problem
caused by the original code?

Could you test a change?


> Please have
> in mind that the rest of the source code should be checked, too. The
> following grep shows 65 hits which should be changed to use the new
> define:
>
>      grep -R SC_MAX * |egrep '(rbuf|recvbuf)'

Fortunately these buffers are 261 bytes long, as the define was meant
to define the max size that could be sent, and this is larger then the
size that can be received. So although the 65 places could be changed,
the use of the buffers in every instance would need to be reviewed.

There may be other locations that use the SC_MAX_APDU_BUFFER_SIZE
that don't use rbuf or recvbuf.

Can you provide a change?

>
>
>
> _______________________________________________
> opensc-devel mailing list
> [hidden email]
> http://www.opensc-project.org/mailman/listinfo/opensc-devel
>

--

  Douglas E. Engert  <[hidden email]>
  Argonne National Laboratory
  9700 South Cass Avenue
  Argonne, Illinois  60439
  (630) 252-5444
_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc-project.org/mailman/listinfo/opensc-devel