Add length check when traverse certchain in libspdm_x509_verify_cert_chain

[rw8896]

libspdm_x509_verify_cert_chain() assumes the input certchain is "One or more ASN.1 DER-encoded X.509 certificates" but it didn't traverse the whole certchain with the code below.

libspdm/os_stub/cryptlib_mbedtls/pk/x509.c

Lines 711 to 716 in ddfd7a1

	ret = mbedtls_asn1_get_tag(
	&tmp_ptr, cert_chain + cert_chain_length, &asn1_len,
	MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE);
	if (ret != 0) {
	break;
	}

Should it add more check to make sure there is no more data left unchecked? E.g.

if (ret != 0) {
if (current_cert < cert_chain + cert_chain_length)
verify_flag = false;
break;
}

[steven-bellock]

This is a case where the length field in the SPDM Certificate Chain Struct says the size of the certificate chain is X, but when the certificate chain is actually parsed the value is Y, where Y < X? And if that happens then that is considered an error? If we can't come up with any good reason why that should be allowed, then yeah, seems like an OK check.

[rw8896]

This is a case where the length field in the SPDM Certificate Chain Struct says the size of the certificate chain is X, but when the certificate chain is actually parsed the value is Y, where Y < X?

Yes, or maybe there are gaps between certificates in the certificate chain, this function will stop before the gap and could also return true without traversing all the certificates down.

And if that happens then that is considered an error?

Yes, I think it should be an error.

[jyao1]

Agree. If cert chain length is X, but only size Y (where Y < X) is parsed, then it should be treated as an error.

[rw8896]

And before calling libspdm_x509_verify_cert(), it should also check if current cert exceeds the end of certchain
if (current_cert + libspdm_x509_verify_cert > cert_chain + cert_chain_length)

libspdm/os_stub/cryptlib_mbedtls/pk/x509.c

Lines 718 to 727 in ddfd7a1

	current_cert_len = asn1_len + (tmp_ptr - current_cert);
	if (libspdm_x509_verify_cert(current_cert, current_cert_len,
	preceding_cert,
	preceding_cert_len) == false) {
	verify_flag = false;
	break;
	} else {
	verify_flag = true;
	}

[steven-bellock]

@rw8896 are you interested in adding this check?

[rw8896]

Sure, will do.