Comment 7 for bug 925657

Revision history for this message
Jamie Strandboge (jdstrand) wrote :

Thanks for the changes. I reviewed the git commits and this is exactly what you want to be doing. I do have some comments:
crypto_cert_subject_common_name():
* Need to verify subject_name != NULL
* Need to verify index != -1
* Need to verify entry != NULL
* Need to verify entry_data != NULL
* Need to verify length is not < 0
* Need to verify common_name does not have embedded '\0' characters be verifying the length of common_name with ASN1_STRING_length

Similar checks need to be done in crypto_cert_subject_alt_name() (ie, check return codes for *everything*).

From man ASN1_STRING_data:
"In general it cannot be assumed that the data returned by ASN1_STRING_data() is null terminated or does not contain embedded nulls."

As such you should:
a) make sure that the strings you are returning in crypto_cert_subject_common_name() and crypto_cert_subject_alt_name() are NULL terminated, otherwise the strcmp()s you do elsewhere on these can cause you trouble.
b) leverage the fact that differences between ASN1_STRING_length and a (properly terminated) strlen means that you have embedded NULLs.

Why are embedded NULLs bad? Consider if an attacker used this for the common name: CN=rdp.foo.com\0mitm.attacker.org\0