LudovicRousseau / CCID

CCID driver
https://ccid.apdu.fr/
GNU Lesser General Public License v2.1
235 stars 79 forks source link

Avoid buffer overrun of we got too short response #139

Closed Jakuje closed 5 months ago

Jakuje commented 5 months ago

This issue was pointed out by coverity that the memmove could be called with underflow value, if we get small answer from CCID_Receive(). My proposal would be to add a sanity check to prevent that, but I am not that much familiar with all the code so I am open to other proposals or suggestions.

"Error: OVERRUN (CWE-119):
ccid-1.5.5/src/commands.c:594: write_constant: Write the value 2 into ""*RxLength"".
ccid-1.5.5/src/commands.c:603: overrun-buffer-arg: Calling ""memmove"" with ""RxBuffer"" and ""*RxLength - 4U"" is suspicious because of the very large index, 4294967294. The index may be due to a negative parameter being interpreted as unsigned. [Note: The source code implementation of the function has been overridden by a builtin model.]
#  601|   
#  602|             /* get only the T=1 data */
#  603|->           memmove(RxBuffer, RxBuffer+3, *RxLength -4);
#  604|             *RxLength -= 4; /* remove NAD, PCB, LEN and CRC */
#  605|         }"
LudovicRousseau commented 5 months ago

Fixed in https://github.com/LudovicRousseau/CCID/commit/346c3aa78debfdbd89a97e8af0c7531a118ac795

I modified your patch to use *RxLength instead of RxLength.

Jakuje commented 5 months ago

Thank you! Should have double-checked the code before pushing.