From 456ac566938a1da774db06126a2fa6c0cba514b3 Mon Sep 17 00:00:00 2001
From: Doug Engert <deengert@gmail.com>
Date: Wed, 14 Jul 2021 11:15:10 -0500
Subject: [PATCH] PIV Improved parsing of data from the card

Based on Fuzz testing, many of the calls to sc_asn1_find_tag were replaced
with sc_asn1_read_tag. The input is also tested that the
expected tag is  the first byte. Additional tests are also add.

sc_asn1_find_tag will skip 0X00 or 0Xff if found. NIST sp800-73-x specs
do not allow these extra bytes.

 On branch PIV-improved-parsing
 Changes to be committed:
	modified:   card-piv.c
---
 src/libopensc/card-piv.c | 112 +++++++++++++++++++++------------------
 1 file changed, 60 insertions(+), 52 deletions(-)

diff --git a/src/libopensc/card-piv.c b/src/libopensc/card-piv.c
index f144b2ccef..77e4864f66 100644
--- a/src/libopensc/card-piv.c
+++ b/src/libopensc/card-piv.c
@@ -608,14 +608,12 @@ static int piv_generate_key(sc_card_t *card,
 		const u8 *cp;
 		keydata->exponent = 0;
 
-		/* expected tag is 7f49.  */
-		/* we will whatever tag is present */
-
 		cp = rbuf;
 		in_len = r;
 
+		/* expected tag is 0x7f49,returned as cla_out == 0x60 and tag_out = 0x1F49 */
 		r = sc_asn1_read_tag(&cp, in_len, &cla_out, &tag_out, &in_len);
-		if (cp == NULL) {
+		if (cp == NULL || in_len == 0 || cla_out != 0x60 || tag_out != 0x1f49) {
 			r = SC_ERROR_ASN1_OBJECT_NOT_FOUND;
 		}
 		if (r != SC_SUCCESS) {
@@ -1032,7 +1030,7 @@ piv_cache_internal_data(sc_card_t *card, int enumtag)
 			priv->obj_cache[enumtag].obj_len,
 			0x53, &bodylen);
 
-	if (body == NULL)
+	if (body == NULL || priv->obj_cache[enumtag].obj_data[0] != 0x53)
 		LOG_FUNC_RETURN(card->ctx, SC_ERROR_OBJECT_NOT_VALID);
 
 	/* get the certificate out */
@@ -1611,7 +1609,7 @@ static int piv_general_mutual_authenticate(sc_card_t *card,
 	/* Remove the encompassing outer TLV of 0x7C and get the data */
 	body = sc_asn1_find_tag(card->ctx, rbuf,
 		r, 0x7C, &body_len);
-	if (!body) {
+	if (!body || rbuf[0] != 0x7C) {
 		sc_debug(card->ctx, SC_LOG_DEBUG_VERBOSE, "Invalid Witness Data response of NULL\n");
 		r =  SC_ERROR_INVALID_DATA;
 		goto err;
@@ -1753,7 +1751,7 @@ static int piv_general_mutual_authenticate(sc_card_t *card,
 	/* Remove the encompassing outer TLV of 0x7C and get the data */
 	body = sc_asn1_find_tag(card->ctx, rbuf,
 		r, 0x7C, &body_len);
-	if(!body) {
+	if(!body || rbuf[0] != 0x7C) {
 		sc_debug(card->ctx, SC_LOG_DEBUG_VERBOSE, "Could not find outer tag 0x7C in response");
 		r =  SC_ERROR_INVALID_DATA;
 		goto err;
@@ -1914,7 +1912,7 @@ static int piv_general_external_authenticate(sc_card_t *card,
 	/* Remove the encompassing outer TLV of 0x7C and get the data */
 	body = sc_asn1_find_tag(card->ctx, rbuf,
 		r, 0x7C, &body_len);
-	if (!body) {
+	if (!body || rbuf[0] != 0x7C) {
 		sc_debug(card->ctx, SC_LOG_DEBUG_VERBOSE, "Invalid Challenge Data response of NULL\n");
 		r =  SC_ERROR_INVALID_DATA;
 		goto err;
@@ -2079,7 +2077,7 @@ piv_get_serial_nr_from_CHUI(sc_card_t* card, sc_serial_number_t* serial)
 	r = SC_ERROR_INTERNAL;
 	if (rbuflen != 0) {
 		body = sc_asn1_find_tag(card->ctx, rbuf, rbuflen, 0x53, &bodylen); /* Pass the outer wrapper asn1 */
-		if (body != NULL && bodylen != 0) {
+		if (body != NULL && bodylen != 0 && rbuf[0] == 0x53) {
 			fascn = sc_asn1_find_tag(card->ctx, body, bodylen, 0x30, &fascnlen); /* Find the FASC-N data */
 			guid = sc_asn1_find_tag(card->ctx, body, bodylen, 0x34, &guidlen);
 
@@ -2311,10 +2309,10 @@ static int piv_validate_general_authentication(sc_card_t *card,
 	piv_private_data_t * priv = PIV_DATA(card);
 	int r, tmplen, tmplen2;
 	u8 *p;
-	const u8 *tag;
+	const unsigned char *p2;
 	size_t taglen;
-	const u8 *body;
 	size_t bodylen;
+	unsigned int cla, tag;
 	unsigned int real_alg_id, op_tag;
 
 	u8 sbuf[4096]; /* needs work. for 3072 keys, needs 384+10 or so */
@@ -2367,20 +2365,28 @@ static int piv_validate_general_authentication(sc_card_t *card,
 
 	r = piv_general_io(card, 0x87, real_alg_id, priv->key_ref,
 			sbuf, p - sbuf, rbuf, sizeof rbuf);
+	if (r < 0)
+		goto err;
 
-	if (r >= 0) {
-		body = sc_asn1_find_tag(card->ctx, rbuf, r, 0x7c, &bodylen);
-		if (body) {
-			tag = sc_asn1_find_tag(card->ctx, body,  bodylen, 0x82, &taglen);
-			if (tag) {
-				memcpy(out, tag, taglen);
-				r = taglen;
-			} else
-				r = SC_ERROR_INVALID_DATA;
-		} else
-			r = SC_ERROR_INVALID_DATA;
+	p2 = rbuf;
+	r = sc_asn1_read_tag(&p2, r, &cla, &tag, &bodylen);
+	if (p2 == NULL || r < 0 || bodylen == 0 || (cla|tag) != 0x7C) {
+		LOG_TEST_GOTO_ERR(card->ctx, SC_ERROR_INVALID_DATA, "Can't find 0x7C");
+        }
+
+	r = sc_asn1_read_tag(&p2, bodylen, &cla, &tag, &taglen);
+	if (p2 == NULL || r < 0 || taglen == 0 || (cla|tag) != 0x82) {
+		LOG_TEST_GOTO_ERR(card->ctx, SC_ERROR_INVALID_DATA, "Can't find 0x82");
 	}
 
+	if (taglen > outlen) {
+		LOG_TEST_GOTO_ERR(card->ctx, SC_ERROR_INVALID_DATA, "data read longer then buffer");
+	}
+
+	memcpy(out, p2, taglen);
+	r = taglen;
+
+err:
 	LOG_FUNC_RETURN(card->ctx, r);
 }
 
@@ -2394,19 +2400,19 @@ piv_compute_signature(sc_card_t *card, const u8 * data, size_t datalen,
 	int i;
 	size_t nLen;
 	u8 rbuf[128]; /* For EC conversions  384 will fit */
-	const u8 * body;
-	size_t bodylen;
-	const u8 * tag;
-	size_t taglen;
+	const unsigned char *pseq, *pint, *ptemp, *pend;
+	unsigned int cla, tag;
+	size_t seqlen;
+	size_t intlen;
+	size_t templen;
 
 	SC_FUNC_CALLED(card->ctx, SC_LOG_DEBUG_VERBOSE);
 
 	/* The PIV returns a DER SEQUENCE{INTEGER, INTEGER}
-	 * Which may have leading 00 to force positive
-	 * TODO: -DEE should check if PKCS15 want the same
-	 * But PKCS11 just wants 2* filed_length in bytes
+	 * Which may have leading 00 to force a positive integer
+	 * But PKCS11 just wants 2* field_length in bytes
 	 * So we have to strip out the integers
-	 * if present and pad on left if too short.
+	 * and pad on left if too short.
 	 */
 
 	if (priv->alg_id == 0x11 || priv->alg_id == 0x14 ) {
@@ -2424,32 +2430,34 @@ piv_compute_signature(sc_card_t *card, const u8 * data, size_t datalen,
 		if (r < 0)
 			goto err;
 
-		body = sc_asn1_find_tag(card->ctx, rbuf, r, 0x30, &bodylen);
-
-		for (i = 0; i<2; i++) {
-			if (body) {
-				tag = sc_asn1_find_tag(card->ctx, body,  bodylen, 0x02, &taglen);
-				if (tag) {
-					bodylen -= taglen - (tag - body);
-					body = tag + taglen;
-
-					if (taglen > nLen) { /* drop leading 00 if present */
-						if (*tag != 0x00) {
-							r = SC_ERROR_INVALID_DATA;
-							goto err;
-						}
-						tag++;
-						taglen--;
-					}
-					memcpy(out + nLen*i + nLen - taglen , tag, taglen);
-				} else {
+		pseq = rbuf;
+		r = sc_asn1_read_tag(&pseq, r, &cla, &tag, &seqlen);
+		if (pseq == NULL || r < 0 || seqlen == 0 || (cla|tag) != 0x30)
+			LOG_TEST_GOTO_ERR(card->ctx, SC_ERROR_INVALID_DATA, "Can't find 0x30");
+
+		pint = pseq;
+		pend = pseq + seqlen;
+		for (i = 0; i < 2; i++) {
+			r = sc_asn1_read_tag(&pint, (pend - pint), &cla, &tag, &intlen);
+			if (pint == NULL || r < 0 || intlen == 0 || (cla|tag) != 0x02)
+				LOG_TEST_GOTO_ERR(card->ctx, SC_ERROR_INVALID_DATA, "Can't find 0x02");
+			if (intlen > nLen + 1)
+				LOG_TEST_GOTO_ERR(card->ctx, SC_ERROR_INVALID_DATA,"Signature too long");
+
+			ptemp = pint;
+			templen = intlen;
+			if (intlen > nLen) { /* drop leading 00 if present */
+				if (*ptemp != 0x00) {
+					LOG_TEST_GOTO_ERR(card->ctx, SC_ERROR_INVALID_DATA,"Signature too long");
 					r = SC_ERROR_INVALID_DATA;
 					goto err;
 				}
-			} else  {
-				r = SC_ERROR_INVALID_DATA;
-				goto err;
+				ptemp++;
+				templen--;
 			}
+			memcpy(out + nLen*i + nLen - templen , ptemp, templen);
+			pint += intlen; /* next integer */
+			
 		}
 		r = 2 * nLen;
 	} else { /* RSA is all set */
