commit c280078cf094a51f1106dcdc3adfcb4f64ee7225
author Demi Marie Obenour <demiobenour@gmail.com> Tue May 30 13:49:29 2023 -0400
committer Sandrine Bailleux <sandrine.bailleux@arm.com> Thu Jun 15 15:31:00 2023 +0200
tree 9ccf256b7d0b94afb1b0741b6614b8fd3fd5e577
parent 300fa8865081d5d19f3fdb2406a99a80ad593507

fix(auth): allow hashes of different lengths

Trusted Board Boot supports multiple hash algorithms, including SHA-256,
SHA-384, and SHA-512.  These algorithms produce hashes of different
lengths, so the resulting DER-encoded hash objects are also of different
lengths.  However, the common Trusted Board Boot code only stores the
contents of the object, not its length.  Before commit
f47547b35462571636a76b737602e827ae43bc24, this was harmless: ASN.1
objects are self-delimiting, and any excess padding was ignored.
f47547b35462571636a76b737602e827ae43bc24 changed the code to reject
excess padding.  However, this breaks using a shorter hash in a build
that supports longer hashes: the shorter hash will have padding after
it, and verify_hash() will reject it.  This was found by an Arm
customer: TF-A v2.9 refused to boot, even though TF-A v2.6 (which did
not have f47547b35462571636a76b737602e827ae43bc24) worked just fine.

Storing the length of the hash turns out to be quite difficult.
However, it turns out that hashes verified by verify_hash() always come
from the ROTPK or an X.509 certificate extension.  Furthermore, _all_
X.509 certificate extensions used by Trusted Board Boot are ASN.1
DER encoded, so it is possible to reject padding in get_ext().  Padding
after the ROTPK is harmless, and it is better to ignore that padding
than to refuse to boot the system.

Change-Id: I28a19d7783e6036b65e86426d78c8e5b2ed6f542
Fixes: f47547b35462571636a76b737602e827ae43bc24 ("fix(auth): reject invalid padding in digests")
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>

drivers/auth/mbedtls/mbedtls_crypto.c

diff --git a/drivers/auth/mbedtls/mbedtls_crypto.c b/drivers/auth/mbedtls/mbedtls_crypto.c
index 6d6364f..230cec9 100644
--- a/drivers/auth/mbedtls/mbedtls_crypto.c
+++ b/drivers/auth/mbedtls/mbedtls_crypto.c
@@ -172,17 +172,20 @@
 	int rc;
 
 	/*
-	 * Digest info should be an MBEDTLS_ASN1_SEQUENCE
-	 * and consume all bytes.
+	 * Digest info should be an MBEDTLS_ASN1_SEQUENCE, but padding after
+	 * it is allowed.  This is necessary to support multiple hash
+	 * algorithms.
 	 */
 	p = (unsigned char *)digest_info_ptr;
 	end = p + digest_info_len;
 	rc = mbedtls_asn1_get_tag(&p, end, &len, MBEDTLS_ASN1_CONSTRUCTED |
 				  MBEDTLS_ASN1_SEQUENCE);
-	if (rc != 0 || ((size_t)(end - p) != len)) {
+	if (rc != 0) {
 		return CRYPTO_ERR_HASH;
 	}
 
+	end = p + len;
+
 	/* Get the hash algorithm */
 	rc = mbedtls_asn1_get_alg(&p, end, &hash_oid, &params);
 	if (rc != 0) {

drivers/auth/mbedtls/mbedtls_x509_parser.c

diff --git a/drivers/auth/mbedtls/mbedtls_x509_parser.c b/drivers/auth/mbedtls/mbedtls_x509_parser.c
index fb5b036..8bde5bb 100644
--- a/drivers/auth/mbedtls/mbedtls_x509_parser.c
+++ b/drivers/auth/mbedtls/mbedtls_x509_parser.c
@@ -135,8 +135,38 @@
 		if ((oid != NULL) &&
 		    ((size_t)oid_len == strlen(oid_str)) &&
 		    (strcmp(oid, oid_str) == 0)) {
+			/* Extension must be ASN.1 DER */
+			if (len < 2) {
+				/* too short */
+				return IMG_PARSER_ERR_FORMAT;
+			}
+
+			if ((p[0] & 0x1F) == 0x1F) {
+				/* multi-byte ASN.1 DER tag, not allowed */
+				return IMG_PARSER_ERR_FORMAT;
+			}
+
+			if ((p[0] & 0xDF) == 0) {
+				/* UNIVERSAL 0 tag, not allowed */
+				return IMG_PARSER_ERR_FORMAT;
+			}
+
 			*ext = (void *)p;
 			*ext_len = (unsigned int)len;
+
+			/* Advance past the tag byte */
+			p++;
+
+			if (mbedtls_asn1_get_len(&p, end_ext_data, &len)) {
+				/* not valid DER */
+				return IMG_PARSER_ERR_FORMAT;
+			}
+
+			if (p + len != end_ext_data) {
+				/* junk after ASN.1 object */
+				return IMG_PARSER_ERR_FORMAT;
+			}
+
 			return IMG_PARSER_OK;
 		}