CVE-2018-16423: Double Free (CWE-415)
value: A double free when handling responses from a smartcard in sc_file_set_sec_attr in libopensc/sc.c in OpenSC before 0.19.0-rc1 could be used by attackers able to supply crafted smartcards to cause a denial of service (application crash) or possibly have unspecified other impact.
CWE description: The product calls free() twice on the same memory address, potentially leading to modification of unexpected memory locations.
CVSS v3 base score: 6.6
Published: 2018-09-04T00:29Z
Repository: https://github.com/OpenSC/OpenSC
Language: C
Files changed: src/libopensc/card-cac.c, src/libopensc/card-epass2003.c, src/libopensc/card-muscle.c, src/libopensc/card-tcos.c, src/libopensc/pkcs15-esteid.c, src/libopensc/pkcs15-gemsafeV1.c, src/libopensc/pkcs15-sc-hsm.c, src/libopensc/sc.c, src/tools/cryptoflex-tool.c, src/tools/egk-tool.c, src/tools/util.c
Commit date: 2018-08-14 15:50:13 +0200
Commit message: fixed out of bounds writes

Thanks to Eric Sesterhenn from X41 D-SEC GmbH
for reporting the problems.

Diff: diff --git a/src/libopensc/card-cac.c b/src/libopensc/card-cac.c
index a30a0438..756788a0 100644
--- a/src/libopensc/card-cac.c
+++ b/src/libopensc/card-cac.c
@@ -792,11 +792,11 @@ static int cac_get_serial_nr_from_CUID(sc_card_t* card, sc_serial_number_t* seri
                 *serial = card->serialnr;
                 SC_FUNC_RETURN(card->ctx, SC_LOG_DEBUG_NORMAL, SC_SUCCESS);
         }
 	if (priv->cac_id_len) {
 		serial->len = MIN(priv->cac_id_len, SC_MAX_SERIALNR);
-		memcpy(serial->value, priv->cac_id, priv->cac_id_len);
+		memcpy(serial->value, priv->cac_id, serial->len);
 		SC_FUNC_RETURN(card->ctx, SC_LOG_DEBUG_NORMAL, SC_SUCCESS);
 	}
 	SC_FUNC_RETURN(card->ctx, SC_LOG_DEBUG_NORMAL, SC_ERROR_FILE_NOT_FOUND);
 }
 
diff --git a/src/libopensc/card-epass2003.c b/src/libopensc/card-epass2003.c
index cbe417cf..65a58f67 100644
--- a/src/libopensc/card-epass2003.c
+++ b/src/libopensc/card-epass2003.c
@@ -949,11 +949,11 @@ decrypt_response(struct sc_card *card, unsigned char *in, size_t inlen, unsigned
 
 	/* unpadding */
 	while (0x80 != plaintext[cipher_len - 2] && (cipher_len - 2 > 0))
 		cipher_len--;
 
-	if (2 == cipher_len)
+	if (2 == cipher_len || *out_len < cipher_len - 2)
 		return -1;
 
 	memcpy(out, plaintext, cipher_len - 2);
 	*out_len = cipher_len - 2;
 	return 0;
@@ -975,10 +975,11 @@ epass2003_sm_unwrap_apdu(struct sc_card *card, struct sc_apdu *sm, struct sc_apd
 	LOG_FUNC_CALLED(card->ctx);
 
 	r = sc_check_sw(card, sm->sw1, sm->sw2);
 	if (r == SC_SUCCESS) {
 		if (exdata->sm) {
+			len = plain->resplen;
 			if (0 != decrypt_response(card, sm->resp, sm->resplen, plain->resp, &len))
 				return SC_ERROR_CARD_CMD_FAILED;
 		}
 		else {
 			memcpy(plain->resp, sm->resp, sm->resplen);
diff --git a/src/libopensc/card-muscle.c b/src/libopensc/card-muscle.c
index fd148e30..d247efab 100644
--- a/src/libopensc/card-muscle.c
+++ b/src/libopensc/card-muscle.c
@@ -516,20 +516,23 @@ static int muscle_list_files(sc_card_t *card, u8 *buf, size_t bufLen)
 	int count = 0;
 
 	mscfs_check_cache(priv->fs);
 
 	for(x = 0; x < fs->cache.size; x++) {
-		u8* oid= fs->cache.array[x].objectId.id;
+		u8* oid = fs->cache.array[x].objectId.id;
+		if (bufLen < 2)
+			break;
 		sc_debug(card->ctx, SC_LOG_DEBUG_NORMAL,
 			"FILE: %02X%02X%02X%02X\n",
 			oid[0],oid[1],oid[2],oid[3]);
 		if(0 == memcmp(fs->currentPath, oid, 2)) {
 			buf[0] = oid[2];
 			buf[1] = oid[3];
 			if(buf[0] == 0x00 && buf[1] == 0x00) continue; /* No directories/null names outside of root */
 			buf += 2;
-			count+=2;
+			count += 2;
+			bufLen -= 2;
 		}
 	}
 	return count;
 }
 
diff --git a/src/libopensc/card-tcos.c b/src/libopensc/card-tcos.c
index 40eac877..3c86b1f4 100644
--- a/src/libopensc/card-tcos.c
+++ b/src/libopensc/card-tcos.c
@@ -406,11 +406,11 @@ static int tcos_select_file(sc_card_t *card,
 	if (file == NULL) SC_FUNC_RETURN(ctx, SC_LOG_DEBUG_NORMAL, SC_ERROR_OUT_OF_MEMORY);
 	*file_out = file;
 	file->path = *in_path;
 
 	for(i=2; i+1<apdu.resplen && i+1+apdu.resp[i+1]<apdu.resplen; i+=2+apdu.resp[i+1]){
-		int j, len=apdu.resp[i+1];
+		size_t j, len=apdu.resp[i+1];
 		unsigned char type=apdu.resp[i], *d=apdu.resp+i+2;
 
 		switch (type) {
 		case 0x80:
 		case 0x81:
@@ -430,12 +430,12 @@ static int tcos_select_file(sc_card_t *card,
 			break;
 		case 0x83:
 			file->id = (d[0]<<8) | d[1];
 			break;
 		case 0x84:
-			memcpy(file->name, d, len);
-			file->namelen = len;
+			file->namelen = MIN(sizeof file->name, len);
+			memcpy(file->name, d, file->namelen);
 			break;
 		case 0x86:
 			sc_file_set_sec_attr(file, d, len); 
 			break;
 		default:
diff --git a/src/libopensc/pkcs15-esteid.c b/src/libopensc/pkcs15-esteid.c
index b3cf5178..2b8d66b9 100644
--- a/src/libopensc/pkcs15-esteid.c
+++ b/src/libopensc/pkcs15-esteid.c
@@ -77,11 +77,11 @@ sc_pkcs15emu_esteid_init (sc_pkcs15_card_t * p15card)
 	SC_TEST_RET(card->ctx, SC_LOG_DEBUG_NORMAL, r, "select esteid PD failed");
 
 	/* read the serial (document number) */
 	r = sc_read_record (card, SC_ESTEID_PD_DOCUMENT_NR, buff, sizeof(buff), SC_RECORD_BY_REC_NR);
 	SC_TEST_RET(card->ctx, SC_LOG_DEBUG_NORMAL, r, "read document number failed");
-	buff[r] = '\0';
+	buff[MIN((size_t) r, (sizeof buff)-1)] = '\0';
 	set_string (&p15card->tokeninfo->serial_number, (const char *) buff);
 
 	p15card->tokeninfo->flags = SC_PKCS15_TOKEN_PRN_GENERATION
 				  | SC_PKCS15_TOKEN_EID_COMPLIANT
 				  | SC_PKCS15_TOKEN_READONLY;
diff --git a/src/libopensc/pkcs15-gemsafeV1.c b/src/libopensc/pkcs15-gemsafeV1.c
index a8162b9d..3b220f7a 100644
--- a/src/libopensc/pkcs15-gemsafeV1.c
+++ b/src/libopensc/pkcs15-gemsafeV1.c
@@ -206,11 +206,11 @@ static int gemsafe_get_cert_len(sc_card_t *card)
 	 * For allocated keys, the fourth byte seems to indicate the
 	 * default key and the fifth byte indicates the key_ref of
 	 * the private key.
 	 */
 	ind = 2; /* skip length */
-	while (ibuf[ind] == 0x01) {
+	while (ibuf[ind] == 0x01 && i < gemsafe_cert_max) {
 		if (ibuf[ind+1] == 0xFE) {
 			gemsafe_prkeys[i].ref = ibuf[ind+4];
 			sc_log(card->ctx, "Key container %d is allocated and uses key_ref %d",
 					i+1, gemsafe_prkeys[i].ref);
 			ind += 9;
diff --git a/src/libopensc/pkcs15-sc-hsm.c b/src/libopensc/pkcs15-sc-hsm.c
index 305572cb..1391bea1 100644
--- a/src/libopensc/pkcs15-sc-hsm.c
+++ b/src/libopensc/pkcs15-sc-hsm.c
@@ -835,16 +835,18 @@ static int sc_pkcs15emu_sc_hsm_init (sc_pkcs15_card_t * p15card)
 	} else {
 		len = sizeof efbin;
 		r = read_file(p15card, (u8 *) "\x2F\x02", efbin, &len, 1);
 		LOG_TEST_RET(card->ctx, r, "Skipping optional EF.C_DevAut");
 
-		/* save EF_C_DevAut for further use */
-		ptr = realloc(priv->EF_C_DevAut, len);
-		if (ptr) {
-			memcpy(ptr, efbin, len);
-			priv->EF_C_DevAut = ptr;
-			priv->EF_C_DevAut_len = len;
+		if (len > 0) {
+			/* save EF_C_DevAut for further use */
+			ptr = realloc(priv->EF_C_DevAut, len);
+			if (ptr) {
+				memcpy(ptr, efbin, len);
+				priv->EF_C_DevAut = ptr;
+				priv->EF_C_DevAut_len = len;
+			}
 		}
 
 		ptr = efbin;
 	}
 
diff --git a/src/libopensc/sc.c b/src/libopensc/sc.c
index af71cafa..53fefb3e 100644
--- a/src/libopensc/sc.c
+++ b/src/libopensc/sc.c
@@ -626,11 +626,11 @@ int sc_file_set_sec_attr(sc_file_t *file, const u8 *sec_attr,
 	u8 *tmp;
 	if (!sc_file_valid(file)) {
 		return SC_ERROR_INVALID_ARGUMENTS;
 	}
 
-	if (sec_attr == NULL) {
+	if (sec_attr == NULL || sec_attr_len) {
 		if (file->sec_attr != NULL)
 			free(file->sec_attr);
 		file->sec_attr = NULL;
 		file->sec_attr_len = 0;
 		return 0;
diff --git a/src/tools/cryptoflex-tool.c b/src/tools/cryptoflex-tool.c
index 21b0baa1..662a0282 100644
--- a/src/tools/cryptoflex-tool.c
+++ b/src/tools/cryptoflex-tool.c
@@ -19,10 +19,11 @@
  */
 
 #include "config.h"
 
 #include "libopensc/sc-ossl-compat.h"
+#include "libopensc/internal.h"
 #include <openssl/bn.h>
 #include <openssl/rsa.h>
 #include <openssl/x509.h>
 #include <openssl/pem.h>
 #include <openssl/err.h>
@@ -329,11 +330,11 @@ static int read_public_key(RSA *rsa)
 	r = sc_select_file(card, &path, &file);
 	if (r) {
 		fprintf(stderr, "Unable to select public key file: %s\n", sc_strerror(r));
 		return 2;
 	}
-	bufsize = file->size;
+	bufsize = MIN(file->size, sizeof buf);
 	sc_file_free(file);
 	r = sc_read_binary(card, 0, buf, bufsize, 0);
 	if (r < 0) {
 		fprintf(stderr, "Unable to read public key file: %s\n", sc_strerror(r));
 		return 2;
@@ -380,11 +381,11 @@ static int read_private_key(RSA *rsa)
 		return 2;
 	}
 	e = sc_file_get_acl_entry(file, SC_AC_OP_READ);
 	if (e == NULL || e->method == SC_AC_NEVER)
 		return 10;
-	bufsize = file->size;
+	bufsize = MIN(file->size, sizeof buf);
 	sc_file_free(file);
 	r = sc_read_binary(card, 0, buf, bufsize, 0);
 	if (r < 0) {
 		fprintf(stderr, "Unable to read private key file: %s\n", sc_strerror(r));
 		return 2;
diff --git a/src/tools/egk-tool.c b/src/tools/egk-tool.c
index 0a8834b6..31360833 100644
--- a/src/tools/egk-tool.c
+++ b/src/tools/egk-tool.c
@@ -147,11 +147,11 @@ int read_file(struct sc_card *card, char *str_path, unsigned char **data, size_t
 	sc_format_path(str_path, &path);
 	if (SC_SUCCESS != sc_select_file(card, &path, &file)) {
 		goto err;
 	}
 
-	len = file ? file->size : 4096;
+	len = file && file->size > 0 ? file->size : 4096;
 	p = realloc(*data, len);
 	if (!p) {
 		goto err;
 	}
 	*data = p;
diff --git a/src/tools/util.c b/src/tools/util.c
index 6b8743e2..e49647a4 100644
--- a/src/tools/util.c
+++ b/src/tools/util.c
@@ -337,14 +337,15 @@ const char * util_acl_to_str(const sc_acl_entry_t *e)
 			break;
 		default:
 			strcpy(buf, "????");
 			break;
 		}
-		strcat(line, buf);
-		strcat(line, " ");
+		strncat(line, buf, sizeof line);
+		strncat(line, " ", sizeof line);
 		e = e->next;
 	}
+	line[(sizeof line)-1] = '\0'; /* make sure it's NUL terminated */
 	line[strlen(line)-1] = 0; /* get rid of trailing space */
 	return line;
 }
 
 NORETURN void

Vulnerable code: 		memcpy(serial->value, priv->cac_id, priv->cac_id_len);
	if (2 == cipher_len)
		u8* oid= fs->cache.array[x].objectId.id;
			count+=2;
		int j, len=apdu.resp[i+1];
			memcpy(file->name, d, len);
			file->namelen = len;
	buff[r] = '\0';
	while (ibuf[ind] == 0x01) {
		/* save EF_C_DevAut for further use */
		ptr = realloc(priv->EF_C_DevAut, len);
		if (ptr) {
			memcpy(ptr, efbin, len);
			priv->EF_C_DevAut = ptr;
			priv->EF_C_DevAut_len = len;
	if (sec_attr == NULL) {
	bufsize = file->size;
	bufsize = file->size;
	len = file ? file->size : 4096;
		strcat(line, buf);
		strcat(line, " ");
Fixed code: 		memcpy(serial->value, priv->cac_id, serial->len);
	if (2 == cipher_len || *out_len < cipher_len - 2)
			len = plain->resplen;
		u8* oid = fs->cache.array[x].objectId.id;
		if (bufLen < 2)
			break;
			count += 2;
			bufLen -= 2;
		size_t j, len=apdu.resp[i+1];
			file->namelen = MIN(sizeof file->name, len);
			memcpy(file->name, d, file->namelen);
	buff[MIN((size_t) r, (sizeof buff)-1)] = '\0';
	while (ibuf[ind] == 0x01 && i < gemsafe_cert_max) {
		if (len > 0) {
			/* save EF_C_DevAut for further use */
			ptr = realloc(priv->EF_C_DevAut, len);
			if (ptr) {
				memcpy(ptr, efbin, len);
				priv->EF_C_DevAut = ptr;
				priv->EF_C_DevAut_len = len;
			}
	if (sec_attr == NULL || sec_attr_len) {
#include "libopensc/internal.h"
	bufsize = MIN(file->size, sizeof buf);
	bufsize = MIN(file->size, sizeof buf);
	len = file && file->size > 0 ? file->size : 4096;
		strncat(line, buf, sizeof line);
		strncat(line, " ", sizeof line);
	line[(sizeof line)-1] = '\0'; /* make sure it's NUL terminated */