pkcs7: pkcs7_get_content_info_type should reset *p on error

[ Upstream commit d8e13853e506e00713d15fa5e23457ba21a16829 ]

Fuzzing revealed a crash where pkcs7_get_signed_data was accessing beyond
the bounds of the object, despite valid data being passed in to
mbedtls_pkcs7_parse_der.

Further investigation revealed that pkcs7_get_content_info_type will
reset *p to start if the second call to mbedtls_asn1_get_tag fails,
but not if the first call fails.

mbedtls_asn1_get_tag does indeed advance *p even in some failure
cases, so a reset is required.

Reset *p to start if the first call to mbedtls_asn1_get_tag fails.

Signed-off-by: Daniel Axtens <dja@axtens.net>
Reviewed-by: Nayna Jain <nayna@linux.ibm.com>
Tested-by: Nayna Jain <nayna@linux.ibm.com>
Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>

2 files changed, 35 insertions, 1 deletions

diff --git a/libstb/crypto/pkcs7/pkcs7.c b/libstb/crypto/pkcs7/pkcs7.c
index 4407e20..a523a9d 100644
--- a/libstb/crypto/pkcs7/pkcs7.c
+++ b/libstb/crypto/pkcs7/pkcs7.c
@@ -151,8 +151,10 @@ static int pkcs7_get_content_info_type( unsigned char **p, unsigned char *end,
 
     ret = mbedtls_asn1_get_tag( p, end, &len, MBEDTLS_ASN1_CONSTRUCTED
                                             | MBEDTLS_ASN1_SEQUENCE );
-    if( ret != 0 )
+    if( ret != 0 ) {
+	*p = start;
         return( MBEDTLS_ERR_PKCS7_INVALID_CONTENT_INFO + ret );
+    }
 
     ret = mbedtls_asn1_get_tag( p, end, &len, MBEDTLS_ASN1_OID );
     if( ret != 0 ) {
diff --git a/libstb/secvar/test/secvar-test-pkcs7.c b/libstb/secvar/test/secvar-test-pkcs7.c
new file mode 100644
index 0000000..d5e8870
--- /dev/null
+++ b/libstb/secvar/test/secvar-test-pkcs7.c
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: Apache-2.0 OR GPL-2.0-or-later
+/* Copyright 2021 IBM Corp. */
+
+#define MBEDTLS_PKCS7_C
+#include "secvar_common_test.c"
+#include "../../crypto/pkcs7/pkcs7.c"
+
+const char *secvar_test_name = "pkcs7";
+
+int run_test()
+{
+	const unsigned char underrun_p7s[] = {0x30, 0x48};
+	mbedtls_pkcs7 pkcs7;
+	unsigned char *data;
+	int rc;
+
+	mbedtls_pkcs7_init(&pkcs7);
+	/* The data must live in the heap, not the stack, for valgrind to
+	   catch the overread. */
+	data = malloc(sizeof(underrun_p7s));
+	memcpy(data, underrun_p7s, sizeof(underrun_p7s));
+	rc = mbedtls_pkcs7_parse_der(data, sizeof(underrun_p7s), &pkcs7);
+	free(data);
+	ASSERT(0 > rc);
+
+	return 0;
+}
+
+int main(void)
+{
+	return run_test();
+}