Re: [opensc-devel] Wrong check for response APDU buffer
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/0x+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 opensc-devel@lists.opensc-project.org http://www.opensc-project.org/mailman/listinfo/opensc-devel -- Douglas E. Engert deeng...@anl.gov Argonne National Laboratory 9700 South Cass Avenue Argonne, Illinois 60439 (630) 252-5444 ___ opensc-devel mailing list opensc-devel@lists.opensc-project.org http://www.opensc-project.org/mailman/listinfo/opensc-devel
Re: [opensc-devel] Wrong check for response APDU buffer
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/0x+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 OpenPACEhttp://openpace.sourceforge.net IFD Handler for libnfc Devices http://sourceforge.net/projects/ifdnfc pgpxRR3Fm3mYi.pgp Description: PGP signature ___ opensc-devel mailing list opensc-devel@lists.opensc-project.org http://www.opensc-project.org/mailman/listinfo/opensc-devel
Re: [opensc-devel] Wrong check for response APDU buffer
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/0x+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 opensc-devel@lists.opensc-project.org http://www.opensc-project.org/mailman/listinfo/opensc-devel -- Douglas E. Engert deeng...@anl.gov Argonne National Laboratory 9700 South Cass Avenue Argonne, Illinois 60439 (630) 252-5444 ___ opensc-devel mailing list opensc-devel@lists.opensc-project.org http://www.opensc-project.org/mailman/listinfo/opensc-devel
[opensc-devel] Wrong check for response APDU buffer
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/0x+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 OpenPACEhttp://openpace.sourceforge.net IFD Handler for libnfc Devices http://sourceforge.net/projects/ifdnfc pgp9fz0CxMiu6.pgp Description: PGP signature ___ opensc-devel mailing list opensc-devel@lists.opensc-project.org http://www.opensc-project.org/mailman/listinfo/opensc-devel
