net: add ability to distinguish user-added root CAs.

net: add ability to distinguish user-added root CAs.

We have several places where a need to distinguish `real' root CAs from
user-added root CAs will be useful:
  1) Monoscope wants to inspect correctly signed, but unknown certificates, but
     doesn't want to deal with proxy MITM certificates.
  2) HSTS is likely to add a method for pinning to a certificate, but we don't
     want to break every proxy MITM with it.

This change adds several lists of known, `real' roots. These lists present an
ongoing maintainance issue. However, in the event that the lists are incomplete
in the future, we fail open. This is because roots not in these lists are
treated as user-added and user-added roots have more authority than `real'
roots.

In some sense, this is a problem because it might be a security issue that new
roots are given too much authority. On the other hand, we're not breaking
things when we're behind on updating the lists so the maintainance issue isn't
too pressing.

BUG=none
TEST=none

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

[agl, 2011-04-05 13:18:38]

Note: the try server are failing on this CL because they can't add nist.der (a
binary file). If you like this CL then I'll commit in two stages: first a CL to
add nist.der, then the real commit once the tryservers are happy.

[wtc, 2011-04-06 04:28:38]

Most of my comments are style nits.  Two are marked with
"BUG".  One suggestion on reversing the sense of
is_probably_mitm_cert so that it can default to false.

http://codereview.chromium.org/6793041/diff/2001/net/base/cert_verify_result.h
File net/base/cert_verify_result.h (right):

http://codereview.chromium.org/6793041/diff/2001/net/base/cert_verify_result....
net/base/cert_verify_result.h:24: is_probably_mitm_cert = true;
Why does is_probably_mitm_cert default to true?  I guess
we have to presume a cert is probably a MITM cert.

You can define this member as is_issued_by_known_root.
Then it can default to false.  (The IsRootedAtKnownRoot
functions in x509_certificate_{mac,win}.cc can be renamed
IsIssuedByKnownRoot.)

http://codereview.chromium.org/6793041/diff/2001/net/base/cert_verify_result....
net/base/cert_verify_result.h:40: // case that this certificate was generated by
a MITM proxy who's root has
Typo: who's => whose

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:11: #include <stdlib.h>
Nit: the C system header <stdlib.h> should be listed before
C++ system headers.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:534: static int CompareSHA1Hashes(const void* a,
const void* b) {
Move this file scope static function to the unnamed namespace
above, on lines 23-119.  Add a comment to note that this is a
comparison function for bsearch.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:541: unsigned bytelen) {
The name of the third parameter should match its name in
the declaration (array_byte_len).

This function should be listed after VerifyNameMatch, to
match the declaration order in the header file.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate.h#...
net/base/x509_certificate.h:329: unsigned array_byte_len);
This method should be private.

You may be able to use the SHA1Fingerprint type, at least
for the first argument.  The static method CalculateFingerprint
should be useful.

Declare array_byte_len as size_t.  (Only NSS uses unsigned int
for lengths, but we're not dealing with NSS here.)

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_ma...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:914: verify_result->is_probably_mitm_cert =
!IsRootedAtKnownRoot(os_cert_handle());
BUG (inefficiency): we already have the cert chain in
|completed_chain|.  See line 789 above.  So we should pass
completed_chain instead of cert_handle_ to IsRootedAtKnownRoot.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:1226: static uint8
kKnownRootCertSHA1Hashes[][20] = {
BUG: this array is not sorted.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:1557: static bool
IsRootedAtKnownRoot(SecCertificateRef cert_ref) {
Move this static function into the unnamed namespace above
at lines 34-497.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:1575: base::SHA1HashBytes(der_bytes.Data,
der_bytes.Length, hash);
You can use the CalculateFingerprint static method.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_ns...
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:208: static bool IsKnownRoot(CERTCertificate*
root) {
Remove 'static', because this is in the unnamed namespace.

It's better to name this function IsBuiltinRoot.  ("Built-in"
is what NSS calls these roots.)

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:784: // We don't need the trust anchor for the
first PKIXVerifyCert call.
Delete this comment because it's no longer true.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_un...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_un...
net/base/x509_certificate_unittest.cc:484: TEST(X509CertificateTest,
TestProbablyMITMCert) {
Please document when this certificate will expire.  (See
examples on lines 401 and 451.)  This lets people know it's
fine to disable this test when it starts to fail.

Alternatively, allow the cert->Verify() call to fail with the
ERR_CERT_DATE_VALID error (CERT_STATUS_DATE_VALID).  You
are only interested in the value of verify_result.is_probably_mitm_cert
anyway.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_wi...
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_wi...
net/base/x509_certificate_win.cc:1653: static bool
IsRootedAtKnownRoot(PCCERT_CHAIN_CONTEXT chain_context) {
Move this static function to the unnamed namespace above.

You can use the CalculateFingerprint static method.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_wi...
net/base/x509_certificate_win.cc:1659: PCCERT_CONTEXT cert =
element[num_elements-1]->pCertContext;
Nit: add spaces around '-'.

[agl, 2011-04-06 19:02:02]

(I'm still working the patch past the try bots.)

http://codereview.chromium.org/6793041/diff/2001/net/base/cert_verify_result.h
File net/base/cert_verify_result.h (right):

http://codereview.chromium.org/6793041/diff/2001/net/base/cert_verify_result....
net/base/cert_verify_result.h:24: is_probably_mitm_cert = true;
On 2011/04/06 04:28:38, wtc wrote:
> Why does is_probably_mitm_cert default to true?  I guess
> we have to presume a cert is probably a MITM cert.
> 
> You can define this member as is_issued_by_known_root.
> Then it can default to false.  (The IsRootedAtKnownRoot
> functions in x509_certificate_{mac,win}.cc can be renamed
> IsIssuedByKnownRoot.)

Done.

http://codereview.chromium.org/6793041/diff/2001/net/base/cert_verify_result....
net/base/cert_verify_result.h:40: // case that this certificate was generated by
a MITM proxy who's root has
On 2011/04/06 04:28:38, wtc wrote:
> Typo: who's => whose

Done.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate.cc
File net/base/x509_certificate.cc (right):

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:11: #include <stdlib.h>
On 2011/04/06 04:28:38, wtc wrote:
> Nit: the C system header <stdlib.h> should be listed before
> C++ system headers.

Done.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:534: static int CompareSHA1Hashes(const void* a,
const void* b) {
On 2011/04/06 04:28:38, wtc wrote:
> Move this file scope static function to the unnamed namespace
> above, on lines 23-119.  Add a comment to note that this is a
> comparison function for bsearch.

Done.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate.cc...
net/base/x509_certificate.cc:541: unsigned bytelen) {
On 2011/04/06 04:28:38, wtc wrote:
> The name of the third parameter should match its name in
> the declaration (array_byte_len).

Done.

> This function should be listed after VerifyNameMatch, to
> match the declaration order in the header file.

Have moved after IsBlacklisted() in the .h file in order to make it private.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate.h#...
net/base/x509_certificate.h:329: unsigned array_byte_len);
On 2011/04/06 04:28:38, wtc wrote:
> This method should be private.

Done, although this means that the IsIssuedByKnownRoot have to be members of
X509Certificate too.

> 
> You may be able to use the SHA1Fingerprint type, at least
> for the first argument.  The static method CalculateFingerprint
> should be useful.
> 
> Declare array_byte_len as size_t.  (Only NSS uses unsigned int
> for lengths, but we're not dealing with NSS here.)

Done.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_ma...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:914: verify_result->is_probably_mitm_cert =
!IsRootedAtKnownRoot(os_cert_handle());
On 2011/04/06 04:28:38, wtc wrote:
> BUG (inefficiency): we already have the cert chain in
> |completed_chain|.  See line 789 above.  So we should pass
> completed_chain instead of cert_handle_ to IsRootedAtKnownRoot.

Done.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:1226: static uint8
kKnownRootCertSHA1Hashes[][20] = {
On 2011/04/06 04:28:38, wtc wrote:
> BUG: this array is not sorted.

Hah! Good point, thanks!

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:1557: static bool
IsRootedAtKnownRoot(SecCertificateRef cert_ref) {
On 2011/04/06 04:28:38, wtc wrote:
> Move this static function into the unnamed namespace above
> at lines 34-497.

It's no longer static because IsSHA1HashInSortedArray is now private. Also, it
has to be after the array of hashes. So I've moved the array into a header file.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:1575: base::SHA1HashBytes(der_bytes.Data,
der_bytes.Length, hash);
On 2011/04/06 04:28:38, wtc wrote:
> You can use the CalculateFingerprint static method.

Done.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_ns...
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:208: static bool IsKnownRoot(CERTCertificate*
root) {
On 2011/04/06 04:28:38, wtc wrote:
> Remove 'static', because this is in the unnamed namespace.
> 
> It's better to name this function IsBuiltinRoot.  ("Built-in"
> is what NSS calls these roots.)

Done.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:784: // We don't need the trust anchor for the
first PKIXVerifyCert call.
On 2011/04/06 04:28:38, wtc wrote:
> Delete this comment because it's no longer true.

Done.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_un...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_un...
net/base/x509_certificate_unittest.cc:484: TEST(X509CertificateTest,
TestProbablyMITMCert) {
On 2011/04/06 04:28:38, wtc wrote:
> Please document when this certificate will expire.  (See
> examples on lines 401 and 451.)  This lets people know it's
> fine to disable this test when it starts to fail.
> 
> Alternatively, allow the cert->Verify() call to fail with the
> ERR_CERT_DATE_VALID error (CERT_STATUS_DATE_VALID).  You
> are only interested in the value of verify_result.is_probably_mitm_cert
> anyway.

Done.

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_wi...
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_wi...
net/base/x509_certificate_win.cc:1653: static bool
IsRootedAtKnownRoot(PCCERT_CHAIN_CONTEXT chain_context) {
On 2011/04/06 04:28:38, wtc wrote:
> Move this static function to the unnamed namespace above.
> 
> You can use the CalculateFingerprint static method.

(Now a private function, same as the mac case.)

http://codereview.chromium.org/6793041/diff/2001/net/base/x509_certificate_wi...
net/base/x509_certificate_win.cc:1659: PCCERT_CONTEXT cert =
element[num_elements-1]->pCertContext;
On 2011/04/06 04:28:38, wtc wrote:
> Nit: add spaces around '-'.

Done.

[wtc, 2011-04-07 05:01:54]

LGTM.  Note the two comments marked with "BUG" below.

http://codereview.chromium.org/6793041/diff/6004/net/base/cert_verify_result.h
File net/base/cert_verify_result.h (right):

http://codereview.chromium.org/6793041/diff/6004/net/base/cert_verify_result....
net/base/cert_verify_result.h:37: // is_issued_by_known_root is true if
recognise the root CA as a standard
Nit: add "we" before "recognise"?

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate.h#...
net/base/x509_certificate.h:350: bool IsIssuedByKnownRoot(CFArrayRef chain);
IsIssuedByKnownRoot can be a *static* method.

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_ma...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:528: return
X509Certificate::IsSHA1HashInSortedArray(
Can we omit X509Certificate:: in this method?

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:927: 
It seems better to set verify_result->is_issued_by_known_root
here, as you did before, or at line 899-900 above, when we
know the certificate is good.  I believe |completed_chain|
is still in scope.

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_ns...
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:208: bool IsBuiltinRoot(CERTCertificate* root)
{
Please change this back to IsKnownRoot for consistency.
Sorry...

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:210: return true;
BUG: this should still return false.

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_un...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_un...
net/base/x509_certificate_unittest.cc:494:
EXPECT_TRUE(verify_result.is_issued_by_known_root);
BUG (FUTURE): cert->Verify() sets
verify_result.is_issued_by_known_root only if the certificate
is good.  So when this certificate expires,
verify_result.is_issued_by_known_root will be false.

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_wi...
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_wi...
net/base/x509_certificate_win.cc:22: #include
"net/base/x509_certificate_win_known_hashes.h"
Typo: hashes => roots

Nit: it may be better to name these headers
x509_certificate_known_roots_win.h and
x509_certificate_known_roots_mac.h, so that
they match the _win.* and _mac.* patterns that
our GYP files can exclude on other platforms.

[agl, 2011-04-07 15:02:48]

http://codereview.chromium.org/6793041/diff/6004/net/base/cert_verify_result.h
File net/base/cert_verify_result.h (right):

http://codereview.chromium.org/6793041/diff/6004/net/base/cert_verify_result....
net/base/cert_verify_result.h:37: // is_issued_by_known_root is true if
recognise the root CA as a standard
On 2011/04/07 05:01:54, wtc wrote:
> Nit: add "we" before "recognise"?

Done.

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate.h
File net/base/x509_certificate.h (right):

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate.h#...
net/base/x509_certificate.h:350: bool IsIssuedByKnownRoot(CFArrayRef chain);
On 2011/04/07 05:01:54, wtc wrote:
> IsIssuedByKnownRoot can be a *static* method.

Done.

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_ma...
File net/base/x509_certificate_mac.cc (right):

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_ma...
net/base/x509_certificate_mac.cc:528: return
X509Certificate::IsSHA1HashInSortedArray(
On 2011/04/07 05:01:54, wtc wrote:
> Can we omit X509Certificate:: in this method?

Done.

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_ns...
File net/base/x509_certificate_nss.cc (right):

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:208: bool IsBuiltinRoot(CERTCertificate* root)
{
On 2011/04/07 05:01:54, wtc wrote:
> Please change this back to IsKnownRoot for consistency.
> Sorry...

Done.

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_ns...
net/base/x509_certificate_nss.cc:210: return true;
On 2011/04/07 05:01:54, wtc wrote:
> BUG: this should still return false.

I'm not sure that it should. In the event of an error, I was going to default to
saying that it was known. However, these errors should almost be CHECKs, so
hopefully the matter doesn't arise.

I've changed this to `false' and made it consistent with the same functions for
Mac and Windows.

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_un...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_un...
net/base/x509_certificate_unittest.cc:494:
EXPECT_TRUE(verify_result.is_issued_by_known_root);
On 2011/04/07 05:01:54, wtc wrote:
> BUG (FUTURE): cert->Verify() sets
> verify_result.is_issued_by_known_root only if the certificate
> is good.  So when this certificate expires,
> verify_result.is_issued_by_known_root will be false.

I've moved the code which sets |is_issued_by_known_root| down in each case so
that it's only set in the event of a valid certificate. Now, expiry is a problem
here but I can't find a way around it :( The test will blow up in the future.

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_wi...
File net/base/x509_certificate_win.cc (right):

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_wi...
net/base/x509_certificate_win.cc:22: #include
"net/base/x509_certificate_win_known_hashes.h"
On 2011/04/07 05:01:54, wtc wrote:
> Typo: hashes => roots
> 
> Nit: it may be better to name these headers
> x509_certificate_known_roots_win.h and
> x509_certificate_known_roots_mac.h, so that
> they match the _win.* and _mac.* patterns that
> our GYP files can exclude on other platforms.

Done.

[wtc, 2011-04-08 00:39:17]

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_un...
File net/base/x509_certificate_unittest.cc (right):

http://codereview.chromium.org/6793041/diff/6004/net/base/x509_certificate_un...
net/base/x509_certificate_unittest.cc:494:
EXPECT_TRUE(verify_result.is_issued_by_known_root);
On 2011/04/07 15:02:49, agl wrote:
> Now, expiry is a problem
> here but I can't find a way around it :( The test will blow up in the future.

Yes.  The X509CertificateTest unit tests that use real
certificates all have this problem.  We should at least
document when the certificate will expire so that build
sheriffs will know they can disable the test safely when
the test blows up.