nss: Fix GetCertType returning SERVER_CERT for explicitly distrusted CA certs.

nss: Fix GetCertType returning SERVER_CERT for explicitly distrusted CA certs.

(Based on wtc's patch from crbug.com/96654.)

BUG=96654
TEST=X509CertificateModelTest.GetTypeCA

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129662

[mattm, 2012-03-28 02:53:39]

Is setting the cert trust on a temporary cert ok? It seems to work for the
test..

[wtc, 2012-03-28 18:10:13]

On 2012/03/28 02:53:39, mattm wrote:
> Is setting the cert trust on a temporary cert ok? It seems to work for the
> test..

Yes, that should be OK.  I believe we do that to our test
root CA cert, which is also a temporary cert.

[wtc, 2012-03-28 19:36:05]

Patch set 1 LGTM.

Mozilla upstream still has this bug:
http://mxr.mozilla.org/mozilla-central/ident?i=getCertType

The Mozilla upstream bug report is
https://bugzilla.mozilla.org/show_bug.cgi?id=193960

Comment 7 and comment 8 of that bug has a patch that
contains the change in this CL:
https://bugzilla.mozilla.org/show_bug.cgi?id=193960#c7

Comment 17 said that patch was checked in:
https://bugzilla.mozilla.org/show_bug.cgi?id=193960#c17

Indeed it was:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show...

But the change was partially reverted in the next revision:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show...

because of another bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=178692

So it seems that we will be bringing that bug back with
this CL.  In my opinion it is still worthwhile.

Perhaps the best fix is to make trust.HasPeer() work
properly?

Note: this link has the "cvs blame" info for the getCertType
function:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/...

https://chromiumcodereview.appspot.com/9875010/diff/6001/chrome/chrome_tests....
File chrome/chrome_tests.gypi (right):

https://chromiumcodereview.appspot.com/9875010/diff/6001/chrome/chrome_tests....
chrome/chrome_tests.gypi:2065: 'common/net/x509_certificate_model_unittest.cc',

Nit: should this file be listed after the files in the
common/net/gaia directory?

https://chromiumcodereview.appspot.com/9875010/diff/6001/chrome/common/net/x5...
File chrome/common/net/x509_certificate_model_unittest.cc (right):

https://chromiumcodereview.appspot.com/9875010/diff/6001/chrome/common/net/x5...
chrome/common/net/x509_certificate_model_unittest.cc:55: net::CertDatabase
cert_db_;

Nit: cert_db_ => cert_db

This is a local variable.

Now I see why you asked that question.  It does look strange
to involve the net::CertDatabase class when we set the trust
on a temporary certificate.

It implies net::CertDatabase::SetCertTrust can be a static
method, at least for the NSS-based implementation.

https://chromiumcodereview.appspot.com/9875010/diff/6001/chrome/third_party/m...
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp (right):

https://chromiumcodereview.appspot.com/9875010/diff/6001/chrome/third_party/m...
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:1062: return
net::CA_CERT;

I suggest combining these two:
  if (trust.HasAnyCA() || CERT_IsCACert(cert, NULL))
    return net::CA_CERT;

[mattm, 2012-03-29 00:24:45]

I added a test case for the server certs too.

I wanted to add one for the X509 v1 cert issue but couldn't find a suitable cert
(example site given in that bug isn't up anymore).

I uploaded two patchsets, patchset 3 is basically the same with the nit fixes
and another test added.
patchset 2 adds a HasTrustedPeer check to GetCertType between the two CA checks.

Let me know which you prefer.

> So it seems that we will be bringing that bug back with
> this CL.  In my opinion it is still worthwhile.
>
> Perhaps the best fix is to make trust.HasPeer() work
> properly?

Not sure how trust.HasPeer can be fixed..  I guess we could use HasTrustedPeer
instead like in patchset 2.  But what we really want is something that can tell
without any trust bits set, right? (Like
https://bugzilla.mozilla.org/show_bug.cgi?id=236887)

https://chromiumcodereview.appspot.com/9875010/diff/6001/chrome/chrome_tests....
File chrome/chrome_tests.gypi (right):

https://chromiumcodereview.appspot.com/9875010/diff/6001/chrome/chrome_tests....
chrome/chrome_tests.gypi:2065: 'common/net/x509_certificate_model_unittest.cc',
On 2012/03/28 19:36:05, wtc wrote:
> 
> Nit: should this file be listed after the files in the
> common/net/gaia directory?

Done.

https://chromiumcodereview.appspot.com/9875010/diff/6001/chrome/common/net/x5...
File chrome/common/net/x509_certificate_model_unittest.cc (right):

https://chromiumcodereview.appspot.com/9875010/diff/6001/chrome/common/net/x5...
chrome/common/net/x509_certificate_model_unittest.cc:55: net::CertDatabase
cert_db_;
On 2012/03/28 19:36:05, wtc wrote:
> 
> Nit: cert_db_ => cert_db
> 
> This is a local variable.

done

https://chromiumcodereview.appspot.com/9875010/diff/6001/chrome/third_party/m...
File chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp (right):

https://chromiumcodereview.appspot.com/9875010/diff/6001/chrome/third_party/m...
chrome/third_party/mozilla_security_manager/nsNSSCertHelper.cpp:1062: return
net::CA_CERT;
On 2012/03/28 19:36:05, wtc wrote:
> 
> I suggest combining these two:
>   if (trust.HasAnyCA() || CERT_IsCACert(cert, NULL))
>     return net::CA_CERT;

Done.

[wtc, 2012-03-29 18:39:37]

Patch set 3 LGTM.

I didn't realize that the old Mozilla bug
https://bugzilla.mozilla.org/show_bug.cgi?id=178692
involved an X.509 v1 certificate.  Today only some old
root CA certificates should still be X.509 v1.  New self-signed
server certificates should be X.509 v3.  So it is fine to
break v1 self-signed server certificates.

I also found that the trust records for distrusted CA certs
and distrusted server certs are indistinguishable -- both
have only the CERTDB_TERMINAL_RECORD bit set.  So in this
case we must distinguish them by certificate contents.

I think this check, as a standalone check,

    if (trust.HasPeer(PR_TRUE, PR_FALSE, PR_FALSE))
      return net::SERVER_CERT;

can be improved to distinguish between trusted CA certs
and trusted server certs.  You can do that in a separate
CL.  Since we perform this check only after testing
trust.HasAnyCA() || CERT_IsCACert(cert, NULL), it's
probably not worth fixing.

[Ryan Sleevi, 2012-03-29 19:26:49]

Hey matt, just some quick drive-by nits.

https://chromiumcodereview.appspot.com/9875010/diff/12001/chrome/common/net/x...
File chrome/common/net/x509_certificate_model_unittest.cc (right):

https://chromiumcodereview.appspot.com/9875010/diff/12001/chrome/common/net/x...
chrome/common/net/x509_certificate_model_unittest.cc:33: }
use net/base/cert_test_util.h to get this path.

https://chromiumcodereview.appspot.com/9875010/diff/12001/chrome/common/net/x...
chrome/common/net/x509_certificate_model_unittest.cc:42:
net::X509Certificate::FORMAT_AUTO);
use net/base/cert_test_util.h to load these files (same for line 42)

https://chromiumcodereview.appspot.com/9875010/diff/12001/chrome/common/net/x...
chrome/common/net/x509_certificate_model_unittest.cc:46: // Remove this when
OpenSSL build implements the necessary functions.
Is there a bug filed explaining what needs to be implemented? (same for line 80)

[Ryan Sleevi, 2012-03-29 19:29:34]

On 2012/03/29 18:39:37, wtc wrote:
> Patch set 3 LGTM.
> 
> I didn't realize that the old Mozilla bug
> https://bugzilla.mozilla.org/show_bug.cgi?id=178692
> involved an X.509 v1 certificate.  Today only some old
> root CA certificates should still be X.509 v1.  New self-signed
> server certificates should be X.509 v3.  So it is fine to
> break v1 self-signed server certificates.

Is this true that we're breaking such certs from being trusted?

It's already enough of a gripe on several other devices (namely, ICS G:N), that
require v3 certificates, and there are a number of large sites (such as
Dreamhost) that still issue v1 certs for internal servers. It would be a sad
affair (for me) to have this break, just my $.02.

[mattm, 2012-03-29 19:44:27]

On 2012/03/29 19:29:34, Ryan Sleevi wrote:
> On 2012/03/29 18:39:37, wtc wrote:
> > Patch set 3 LGTM.
> > 
> > I didn't realize that the old Mozilla bug
> > https://bugzilla.mozilla.org/show_bug.cgi?id=178692
> > involved an X.509 v1 certificate.  Today only some old
> > root CA certificates should still be X.509 v1.  New self-signed
> > server certificates should be X.509 v3.  So it is fine to
> > break v1 self-signed server certificates.
> 
> Is this true that we're breaking such certs from being trusted?
> 
> It's already enough of a gripe on several other devices (namely, ICS G:N),
that
> require v3 certificates, and there are a number of large sites (such as
> Dreamhost) that still issue v1 certs for internal servers. It would be a sad
> affair (for me) to have this break, just my $.02.

They would show up in the Authorities cert tab instead, which means we could
trust them as CAs but not just as a server. (Unless you use certutil, of
course.)

[mattm, 2012-03-29 19:47:05]

On 2012/03/29 19:26:49, Ryan Sleevi wrote:
> Hey matt, just some quick drive-by nits.
> 
>
https://chromiumcodereview.appspot.com/9875010/diff/12001/chrome/common/net/x...
> File chrome/common/net/x509_certificate_model_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9875010/diff/12001/chrome/common/net/x...
> chrome/common/net/x509_certificate_model_unittest.cc:33: }
> use net/base/cert_test_util.h to get this path.
> 
>
https://chromiumcodereview.appspot.com/9875010/diff/12001/chrome/common/net/x...
> chrome/common/net/x509_certificate_model_unittest.cc:42:
> net::X509Certificate::FORMAT_AUTO);
> use net/base/cert_test_util.h to load these files (same for line 42)

Already committed, so will do this in a separate CL.


https://chromiumcodereview.appspot.com/9875010/diff/12001/chrome/common/net/x...
> chrome/common/net/x509_certificate_model_unittest.cc:46: // Remove this when
> OpenSSL build implements the necessary functions.
> Is there a bug filed explaining what needs to be implemented? (same for line
80)

Not sure if there is a bug, but there is a bunch of "TODO(...): implement me"
comments in chrome/common/net/x509_certificate_model_openssl.cc.

[wtc, 2012-03-30 00:53:10]

On 2012/03/29 19:29:34, Ryan Sleevi wrote:
>
> Is this true that we're breaking such certs from being trusted?

Such certs can still be trusted by being chained up to a trusted
root CA cert.

What won't work is to explicitly trust such certs as server certs,
without building a chain.

> It's already enough of a gripe on several other devices (namely, ICS G:N),
that
> require v3 certificates, and there are a number of large sites (such as
> Dreamhost) that still issue v1 certs for internal servers.

Unless those v1 certs are self-signed server certs, they won't
be affected by this CL.

The problem we need to solve is to determine whether a v1 cert is
a CA cert or an SSL server cert.  Since a v1 cert has no extensions,
CERT_IsCACert() assumes a v1 self-signed cert is a root cert.
I guess if the Subject common name is a DNS name, we can guess the
v1 self-signed cert is a server cert.