crewID/libfreefare
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Prevent mifare_desfire_read_data() overflow.

Browse source 
Depending on the communication settings, mifare_desfire_read_data() may
write more than the provided "length" bytes to the "data" buffer,
possibly causing data corruption or crashes if no special care is taken.

Since the test suite is precisely a "no special care is taken" example,
assume only "length" bytes can be written to the "data" buffer and rely
on a temporary buffer for cryptographic operations.

Fixes  issue 28.
This commit is contained in:
Romain Tartière 2014-04-16 01:54:14 +02:00
parent 8290d3d8cc
commit 57e1fceb6b
3 changed files with 39 additions and 7 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

37
libfreefare/mifare_desfire.c
View file

@ -1542,6 +1542,19 @@ read_data (MifareTag tag, uint8_t command, uint8_t file_no, off_t offset, size_t
BUFFER_APPEND_LE (cmd, offset, 3, sizeof (off_t));
BUFFER_APPEND_LE (cmd, length, 3, sizeof (size_t));
/*
* In case no length is provided (whole file is to be read) get the file's
* length in order to allocate a large enought buffer for crypto
* post-processing.
*/
if (!length) {
struct mifare_desfire_file_settings settings;
int res = mifare_desfire_get_file_settings (tag, file_no, &settings);
if (res < 0)
return res;
length = settings.settings.standard_file.file_size;
}
uint8_t ocs = cs;
if ((MIFARE_DESFIRE (tag)->session_key) && (cs | MDCM_MACED)) {
switch (MIFARE_DESFIRE (tag)->authentication_scheme) {
@ -1556,25 +1569,37 @@ read_data (MifareTag tag, uint8_t command, uint8_t file_no, off_t offset, size_t
cs = ocs;
/*
* FIXME: This is bogus: the user has to provide a data buffer with enougth
* room to store CRC + padding or MAC. If the user wants to read 1 byte,
* there is no reason to provide a 16 bytes buffer.
* Depending on the communication settings, we might read more bytes than
* the actual data length (a MAC or padding padding might follow). This
* can be a problem if the destination buffer is long enouth for the data
* but the MAC / padding overflows.
*
* Create a temporary read buffer to collect all read data, post-process it
* through the cryptography code and copy the actual data to the
* destination buffer.
*/
uint8_t *read_buffer = malloc (enciphered_data_length (tag, length, 0) + 1);
do {
DESFIRE_TRANSCEIVE2 (tag, p, __cmd_n, res);
size_t frame_bytes = BUFFER_SIZE (res) - 1;
memcpy ((uint8_t *)data + bytes_received, res, frame_bytes);
memcpy (read_buffer + bytes_received, res, frame_bytes);
bytes_received += frame_bytes;
p[0] = 0xAF;
__cmd_n = 1;
} while (0xAF == res[__res_n-1]);
((uint8_t *)data)[bytes_received++] = 0x00;
read_buffer[bytes_received++] = 0x00;
ssize_t sr = bytes_received;
p = mifare_cryto_postprocess_data (tag, data, &sr, cs | CMAC_COMMAND | CMAC_VERIFY | MAC_VERIFY);
p = mifare_cryto_postprocess_data (tag, read_buffer, &sr, cs | CMAC_COMMAND | CMAC_VERIFY | MAC_VERIFY);
if (sr > 0)
memcpy(data, read_buffer, sr - 1);
free (read_buffer);
if (!p)
return errno = EINVAL, -1;

2
libfreefare/mifare_desfire_crypto.c
View file

@ -273,7 +273,7 @@ enciphered_data_length (const MifareTag tag, const size_t nbytes, int communicat
}
}
size_t block_size = key_block_size (MIFARE_DESFIRE (tag)->session_key);
size_t block_size = MIFARE_DESFIRE(tag)->session_key ? key_block_size (MIFARE_DESFIRE (tag)->session_key) : 1;
return padded_data_length (nbytes + crc_length, block_size);
}

7
test/test_mifare_desfire_ev1.c
View file

@ -122,6 +122,13 @@ test_mifare_desfire_ev1_aes2 (void)
cut_assert_success ("mifare_desfire_read_data");
cut_assert_equal_memory (buffer, res, sample_data, 27, cut_message ("AES crypto failed"));
char canaries[] = "Canaries Canaries Canaries Canaries Canaries";
res = mifare_desfire_read_data_ex (tag, 1, 0, 1, canaries, MDCM_MACED);
cut_assert_success ("mifare_desfire_read_data");
cut_assert_equal_int (1, res, cut_message ("Reading 1 byte should return 1 byte"));
cut_assert_equal_memory (canaries, 44, "Hanaries Canaries Canaries Canaries Canaries", 44, cut_message ("Canaries got smashed!"));
uint8_t s, c;
res = mifare_desfire_get_key_settings (tag, &s, &c);
cut_assert_success ("mifare_desfire_get__key_settings");