[libfreefare] 2 new revisions pushed by romain.t...@gmail.com on 2014-04-16 00:03 GMT

[libfr...@googlecode.com]

master moved from 8290d3d8cc16 to 27d33775dc2a

2 new revisions:

Revision: 57e1fceb6b06
Author: Romain Tartière <rom...@blogreen.org>
Date: Tue Apr 15 23:54:14 2014 UTC
Log: Prevent mifare_desfire_read_data() overflow....
http://code.google.com/p/libfreefare/source/detail?r=57e1fceb6b06

Revision: 27d33775dc2a
Author: Romain Tartière <rom...@blogreen.org>
Date: Wed Apr 16 00:02:50 2014 UTC
Log: Don't make me think!
http://code.google.com/p/libfreefare/source/detail?r=27d33775dc2a

==============================================================================
Revision: 57e1fceb6b06
Author: Romain Tartière <rom...@blogreen.org>
Date: Tue Apr 15 23:54:14 2014 UTC
Log: Prevent mifare_desfire_read_data() overflow.

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.

http://code.google.com/p/libfreefare/source/detail?r=57e1fceb6b06

Modified:
/libfreefare/mifare_desfire.c
/libfreefare/mifare_desfire_crypto.c
/test/test_mifare_desfire_ev1.c

=======================================
--- /libfreefare/mifare_desfire.c Fri Mar 14 21:49:27 2014 UTC
+++ /libfreefare/mifare_desfire.c Tue Apr 15 23:54:14 2014 UTC
@@ -1549,6 +1549,19 @@
BUFFER_APPEND (cmd, file_no);
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)) {
@@ -1564,25 +1577,37 @@
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;
=======================================
--- /libfreefare/mifare_desfire_crypto.c Mon Jan 27 17:24:55 2014 UTC
+++ /libfreefare/mifare_desfire_crypto.c Tue Apr 15 23:54:14 2014 UTC
@@ -273,7 +273,7 @@
}
}

- 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);
}
=======================================
--- /test/test_mifare_desfire_ev1.c Fri Dec 24 13:59:28 2010 UTC
+++ /test/test_mifare_desfire_ev1.c Tue Apr 15 23:54:14 2014 UTC
@@ -122,6 +122,13 @@
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");

==============================================================================
Revision: 27d33775dc2a
Author: Romain Tartière <rom...@blogreen.org>
Date: Wed Apr 16 00:02:50 2014 UTC
Log: Don't make me think!

http://code.google.com/p/libfreefare/source/detail?r=27d33775dc2a

Modified:
/libfreefare/freefare_internal.h
/libfreefare/mifare_desfire.c

=======================================
--- /libfreefare/freefare_internal.h Sun Apr 13 18:29:57 2014 UTC
+++ /libfreefare/freefare_internal.h Wed Apr 16 00:02:50 2014 UTC
@@ -286,8 +286,10 @@
#define TB_AB(ab) ((ab == C_DEFAULT) ? C_100 : ab)

#ifdef WITH_DEBUG
+#define DEBUG_FUNCTION() do { printf("*** \033[033;1m%s\033[0m ***\n",
__FUNCTION__); } while (0)
#define DEBUG_XFER(data, nbytes, hint) do { hexdump (data, nbytes, hint,
0); } while (0)
#else
+#define DEBUG_FUNCTION() do {} while (0)
#define DEBUG_XFER(data, nbytes, hint) do {} while (0)
#endif

=======================================
--- /libfreefare/mifare_desfire.c Tue Apr 15 23:54:14 2014 UTC
+++ /libfreefare/mifare_desfire.c Wed Apr 16 00:02:50 2014 UTC
@@ -167,6 +167,7 @@
*/
#define DESFIRE_TRANSCEIVE2(tag, msg, msg_len, res) \
do { \
+ DEBUG_FUNCTION(); \
static uint8_t __msg[MAX_CAPDU_SIZE + 5] = { 0x90, 0x00, 0x00, 0x00,
0x00, /* ..., */ 0x00 }; \
/* CLA INS P1 P2 Lc
PAYLOAD LE*/ \
static uint8_t __res[MAX_RAPDU_SIZE + 1]; \