On OS X, when parsing certificates, properly handle non-ASCII/UTF-8 string types.

On OS X, when parsing certificates, properly handle non-ASCII/UTF-8 string types.

Specifically, ensure that certificates whose subjects or issuers contain values
encoded as BMPStrings, T61Strings, or UniversalStrings can be properly decoded
and normalized to UTF-8.

BUG=108033
TEST=net_unittests:X509TypesTest.*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=115361

[Ryan Sleevi, 2011-12-21 00:04:20]

wtc: Hopefully an easy review.

Consider that SecCertificateGetSubject and SecCertificateGetIssuer are both
deprecated in 10.7, it didn't seem any worse to switch the CSSM structure
parsing with the existing raw BER parsing used for client auth.

[Ryan Sleevi, 2011-12-21 00:10:18]

http://codereview.chromium.org/9006041/diff/1/net/base/x509_cert_types_mac.cc
File net/base/x509_cert_types_mac.cc (left):

http://codereview.chromium.org/9006041/diff/1/net/base/x509_cert_types_mac.cc...
net/base/x509_cert_types_mac.cc:57: struct KeyValuePair {
This structure effectively mirrored the existing CSSM_X509_TYPE_VALUE_PAIR
structure. The only change between the two is the numeric values for the types.

My initial approach had been to harmonize the two types so that both Parse() and
ParseDistinguishedName() could use CSSM_X509_TYPE_VALUE_PAIR, but that ended up
being unnecessary with CSSMOID_X509V1SubjectNameStd offering the raw DER.

So the only justification for this change that remains is "one less data type",
which is a very, very trivial goal. Still, I believe it improves
readability/ease of use to use a common & well-documented datatype (plus readers
of this code can easily cross-reference w/ Apple code to see how it is used)

[wtc, 2011-12-21 01:10:07]

LGTM!

http://codereview.chromium.org/9006041/diff/1/net/base/x509_certificate_mac.cc
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/9006041/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:261: void GetDistinguishedName(const
CSSMCachedCertificate& cached_cert,

Nit: GetDistinguishedName => GetCertDistinguishedName ?

http://codereview.chromium.org/9006041/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:266: if (status || !distinguished_name.field())

Can distinguished_name.field() be NULL when cached_cert.GetField() succeeds?

[Ryan Sleevi, 2011-12-21 02:14:56]

http://codereview.chromium.org/9006041/diff/1/net/base/x509_certificate_mac.cc
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/9006041/diff/1/net/base/x509_certificate_mac.c...
net/base/x509_certificate_mac.cc:266: if (status || !distinguished_name.field())
On 2011/12/21 01:10:08, wtc wrote:
> 
> Can distinguished_name.field() be NULL when cached_cert.GetField() succeeds?

Judging by the Apple code, it is seen as a possibility.

See the method Certificate::isSelfSigned() in
http://www.opensource.apple.com/source/libsecurity_keychain/libsecurity_keych...

	issuer  = copyFirstFieldValue(CSSMOID_X509V1IssuerNameStd);
	subject = copyFirstFieldValue(CSSMOID_X509V1SubjectNameStd);
	if((issuer == NULL) || (subject == NULL)) {
		ortn = paramErr;
	}

Note that if there are any error returns (status != noErr), it will throw an
exception, so the NULL check seems clearly meant to cover a situation that does
not throw.

In practice, it shouldn't be possible (getFieldSubjectStd of 
http://www.opensource.apple.com/source/libsecurity_apple_x509_cl/libsecurity_...
always returns true as long as you're retrieving the first field value), but I'm
not sure if it's reliable to code against this implementation detail.

[commit-bot: I haz the power, 2011-12-21 02:25:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/9006041/8002

[commit-bot: I haz the power, 2011-12-21 10:37:49]

Try job failure for 9006041-8002 (retry) on mac_rel for step "ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2011-12-21 15:27:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/9006041/8002

[commit-bot: I haz the power, 2011-12-21 17:57:26]

Change committed as 115361