mifare_desfire.c: buffer overflow at read_data

If size_t length is greater than 52 and is dividable by 4 but not by 8 (e.g. 60,68,...,876,...) enciphered_data_length() results in wrong padded data length and the read_buffer is to small to keep the content.

Here is a quick and dirty printf debug output for your convenience with 60 as length

+++++++++++++++ ENTER read_data command 189 file_no 7 offset 0 lenght 60 cs 1
############### ENTER padded_data_length: nbytes 8 block_size 16 nbytes % block_size)
############### IF branch : (nbytes / block_size) 0
############### IF branch : ((nbytes / block_size) + 1) 1
############### ((nbytes / block_size) + 1) * block_size 16
=============== ENTER enciphered_data_length
=============== nbytes 60, crc_length 4, block_size 16
############### ENTER padded_data_length: nbytes 64 block_size 16 nbytes % block_size)
############### ELSE: nbytes 64
=============== padded_data_length 64
=============== EXIT enciphered_data_length
+++++++++++++++ enciphered_data_length (tag, length, 0) = 64 rember the + 1 later
+++++++++++++++ bytes_received before memcpy = 0
+++++++++++++++ bytes_received after memcpy = 48
+++++++++++++++ bytes_received before memcpy = 48
+++++++++++++++ bytes_received after memcpy = 68
+++++++++++++++ bytes_received after transfer 68 remvember the ++ later on
+++++++++++++++ sr = 69
############### ENTER padded_data_length: nbytes 61 block_size 16 nbytes % block_size)
############### IF branch : (nbytes / block_size) 3
############### IF branch : ((nbytes / block_size) + 1) 4
############### ((nbytes / block_size) + 1) * block_size 64
*** Error in `/test': malloc(): memory corruption: 0x00e2e238 ***

Increasing the padding by one at

enciphered_data_length (padded_data_length (nbytes + crc_length +1, block_size)

instead of

enciphered_data_length (padded_data_length (nbytes + crc_length, block_size))

solves the problem for me. And I attached a patch for it. But I am not sure if this patch will be right in every case. In the case above the resulting buffer size is 80 which is enough to store the data.

~~While reading the source I also came along that malloc is not checked for errors.~~

Also this part makes me wonder

    uint8_t ocs = cs;
    if ((MIFARE_DESFIRE (tag)->session_key) && (cs | MDCM_MACED)) {
    switch (MIFARE_DESFIRE (tag)->authentication_scheme) {
    case AS_LEGACY:
        break;
    case AS_NEW:
        cs = MDCM_PLAIN;
        break;
    }
    }
    uint8_t *p = mifare_cryto_preprocess_data (tag, cmd, &__cmd_n, 8, MDCM_PLAIN | CMAC_COMMAND);
    cs = ocs;

did you wanted to use :

mifare_cryto_preprocess_data (tag, cmd, &__cmd_n, 8, cs | CMAC_COMMAND)

instead of

mifare_cryto_preprocess_data (tag, cmd, &__cmd_n, 8, MDCM_PLAIN | CMAC_COMMAND)

Thanks in advance, Bernhard

Original issue reported on code.google.com by hale.dev...@gmail.com on 4 Nov 2014 at 3:41

Attachments:

0001-mifare_desfire_crypto.c-fix-wrong-data-length.patch

nfc-tools / libfreefare

mifare_desfire.c: buffer overflow at read_data #33