Detect NULL characters in certificate Subject common...

Detect NULL characters in certificate Subject common
names.

R=hawk
BUG=24190
TEST=the X509CertificateTest.PaypalNullCertParsing test

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

[wtc, 2009-10-07 23:23:18]

Adam, please review.  I will also ask someone familiar
with CryptoAPI to review this CL.

[hawk, 2009-10-07 23:51:59]

http://codereview.chromium.org/261016/diff/1/2
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/261016/diff/1/2#newcode216
Line 216: default:
This will allow rarely-used string encodings through (not that there's many
certs out there with CNs encoded as BMPStrings!). How about a 16-bit case that
handles CERT_RDN_UTF8_STRING and CERT_RDN_BMP_STRING, a 32 bit case for
CERT_RDN_UNIVERSAL_STRING and a default case for 8 bit strings, which will also
pick up VisibleStrings and IA5Strings?

http://codereview.chromium.org/261016/diff/1/2#newcode543
Line 543: verify_result->cert_status |= CERT_STATUS_INVALID;
Small tangent: on the Mac we're returning CERT_STATUS_COMMON_NAME_INVALID. I'm
planning on changing to CERT_STATUS_INVALID, since the NULL character needs a
stronger warning (i.e., it's likely an attack rather than a configuration
mistake).

[wtc, 2009-10-08 00:00:16]

hawk: thanks for your review!

http://codereview.chromium.org/261016/diff/1/2
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/261016/diff/1/2#newcode216
Line 216: default:
The reason I only have cases for CERT_RDN_PRINTABLE_STRING,
CERT_RDN_T61_STRING, and CERT_RDN_UTF8_STRING is that I
have actually tested my code with these kinds of strings,
so I know whether they are 8-bit or 16-bit characters.  (I
didn't spend too much time reading MSDN docs.)

I will look into your suggested change.

http://codereview.chromium.org/261016/diff/1/2#newcode543
Line 543: verify_result->cert_status |= CERT_STATUS_INVALID;
Agreed.  The reason I chose CERT_STATUS_INVALID is that our
SSL interstial page for CERT_STATUS_INVALID does not have a
"Proceed anyway" button, so users can't click through.

[abarth-chromium, 2009-10-08 01:44:57]

I'm not familiar with the crypto API, but this looks great if it works.  Thanks
wtc.

[wtc, 2009-10-08 01:50:32]

Adam, thanks for taking a look.  I'll let hawk do the code review.

[wtc, 2009-10-08 18:38:59]

hawk: How does Patch Set 2 look?

http://codereview.chromium.org/261016/diff/1/2
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/261016/diff/1/2#newcode216
Line 216: default:
I added cases for CERT_RDN_BMP_STRING and
CERT_RDN_UNIVERSAL_STRING, but I'm not using the default
case for 8-bit strings as you suggested.  This function
must not report false positives, so I am not checking
string types that shouldn't be used in a common name,
such as VisibleStrings and IA5Strings.

I can be talked into adding IA5Strings to the 8-bit case
since they're used in the sujectAltName extension.

[hawk, 2009-10-08 19:02:35]

LGTM with one small change.

If you haven't see it, Peter Gutmann's X.509 Style Guide is a must-read on
certificate foibles:

http://www.cs.auckland.ac.nz/~pgut001/pubs/x509guide.txt

It will make you laugh, it will make you cry.

http://codereview.chromium.org/261016/diff/1/2
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/261016/diff/1/2#newcode216
Line 216: default:
On 2009/10/08 18:38:59, wtc wrote:
> I can be talked into adding IA5Strings to the 8-bit case
> since they're used in the sujectAltName extension.

You should add IA5String, and VisibleString as well -- CAs are notorious for
ignoring specs (after all, that's how we got into this embedded-NULL mess in the
first place!)

[wtc, 2009-10-08 20:35:17]

hawk: I made the change you suggested and committed the CL.
Thanks for the link to the X.509 Style Guide.  I knew about it but
have never read it.