Fix imported server certs being distrusted in NSS 3.13.

net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp

Index: net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp
diff --git a/net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp b/net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp
index 0cf430d793195af800d8e5746bbdea134f892e0e..2db0f633d906a7cf2175ee4afe131d5a6446ce4b 100644
--- a/net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp
+++ b/net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp
@@ -39,6 +39,7 @@
 #include "net/third_party/mozilla_security_manager/nsNSSCertificateDB.h"
 
 #include <cert.h>
+#include <certdb.h>
 #include <pk11pub.h>
 #include <secerr.h>
 
@@ -47,7 +48,14 @@
 #include "crypto/scoped_nss_types.h"
 #include "net/base/net_errors.h"
 #include "net/base/x509_certificate.h"
-#include "net/third_party/mozilla_security_manager/nsNSSCertTrust.h"
+
+#if !defined(CERTDB_TERMINAL_RECORD)
+/* NSS 3.13 renames CERTDB_VALID_PEER to CERTDB_TERMINAL_RECORD
+ * and marks CERTDB_VALID_PEER as deprecated.
+ * If we're using an older version, rename it ourselves.
+ */
+#define CERTDB_TERMINAL_RECORD CERTDB_VALID_PEER
+#endif
 
 namespace mozilla_security_manager {
 
@@ -56,6 +64,9 @@ bool ImportCACerts(const net::CertificateList& certificates,
                    net::X509Certificate* root,
                    net::CertDatabase::TrustBits trustBits,
                    net::CertDatabase::ImportCertFailureList* not_imported) {
+  if (certificates.empty() || !root)
+    return false;
+
   crypto::ScopedPK11Slot slot(crypto::GetPublicNSSKeySlot());
   if (!slot.get()) {
     LOG(ERROR) << "Couldn't get internal key slot!";
@@ -158,7 +169,11 @@ bool ImportCACerts(const net::CertificateList& certificates,
 
 // Based on nsNSSCertificateDB::ImportServerCertificate.
 bool ImportServerCert(const net::CertificateList& certificates,
+                      net::CertDatabase::TrustBits trustBits,
                       net::CertDatabase::ImportCertFailureList* not_imported) {
+  if (certificates.empty())
+    return false;
+
   crypto::ScopedPK11Slot slot(crypto::GetPublicNSSKeySlot());
   if (!slot.get()) {
     LOG(ERROR) << "Couldn't get internal key slot!";
@@ -184,9 +199,7 @@ bool ImportServerCert(const net::CertificateList& certificates,
     }
   }
 
-  // Set as valid peer, but without any extra trust.
-  SetCertTrust(certificates[0].get(), net::SERVER_CERT,
-               net::CertDatabase::UNTRUSTED);
+  SetCertTrust(certificates[0].get(), net::SERVER_CERT, trustBits);
   // TODO(mattm): Report SetCertTrust result?  Putting in not_imported
   // wouldn't quite match up since it was imported...
 
@@ -200,25 +213,50 @@ SetCertTrust(const net::X509Certificate* cert,
              net::CertType type,
              net::CertDatabase::TrustBits trustBits)
 {
+  // DCHECK that caller isn't setting trust and distrust for the same use.

[wtc, 2012/05/22 00:28:39]

Perhaps the function should return false if the caller sets
trust and distrust for the same use?

Since |ssl_flags| looks similar to the |sslFlags| member of
CERTCertTrust, I suggest that we name these variables
ssl_bits or ssl_trust_bits or even kSSLTrustBits. etc.

[mattm, 2012/05/26 03:41:35]

done.
done.

+  const unsigned ssl_flags = net::CertDatabase::TRUSTED_SSL |
+      net::CertDatabase::DISTRUSTED_SSL;
+  DCHECK_NE(trustBits & ssl_flags, ssl_flags);
+  const unsigned email_flags = net::CertDatabase::TRUSTED_EMAIL |
+      net::CertDatabase::DISTRUSTED_EMAIL;
+  DCHECK_NE(trustBits & email_flags, email_flags);
+  const unsigned obj_sign_flags = net::CertDatabase::TRUSTED_OBJ_SIGN |
+      net::CertDatabase::DISTRUSTED_OBJ_SIGN;
+  DCHECK_NE(trustBits & obj_sign_flags, obj_sign_flags);
+
   SECStatus srv;
-  nsNSSCertTrust trust;
   CERTCertificate *nsscert = cert->os_cert_handle();
   if (type == net::CA_CERT) {
-    // always start with untrusted and move up
-    trust.SetValidCA();
-    trust.AddCATrust(trustBits & net::CertDatabase::TRUSTED_SSL,
-                     trustBits & net::CertDatabase::TRUSTED_EMAIL,
-                     trustBits & net::CertDatabase::TRUSTED_OBJ_SIGN);
-    srv = CERT_ChangeCertTrust(CERT_GetDefaultCertDB(),
-                               nsscert,
-                               trust.GetTrust());
+    CERTCertTrust trust = {CERTDB_VALID_CA, CERTDB_VALID_CA, CERTDB_VALID_CA};
+
+    if (trustBits & net::CertDatabase::DISTRUSTED_SSL)
+      trust.sslFlags |= CERTDB_TERMINAL_RECORD;
+    else if (trustBits & net::CertDatabase::TRUSTED_SSL)
+      trust.sslFlags |= CERTDB_TRUSTED_CA | CERTDB_TRUSTED_CLIENT_CA;

[wtc, 2012/05/22 00:28:39]

The CERTDB_TERMINAL_RECORD bit also needs to be set for explicit
trust.

[mattm, 2012/05/26 03:41:35]

I added some tests and it does not seem to be necessary.  I did notice a
different problem though,, where distrusting an intermediate cert would not have
any effect if the root cert was trusted.  Seems to be because we were using
CERTDB_VALID_CA|CERTDB_TERMINAL_RECORD.  I changed it to use just
CERTDB_TERMINAL_RECORD in that case, which appears to work.

[wtc, 2012/05/30 00:19:24]

We should get to the bottom of this.  I suspect that
the new problem you noticed is a NSS libpkix bug.  We
can verify this by editing net/base/cert_verify_proc_nss.cc
and changing CertVerifyProcNSS::VerifyInternal to call
CERT_VerifyCert instead of PKIXVerifyCert.  Please see
if your tests report different reports.

If you are not sure how to make this change, please ask me.

I will also start an email thread with the NSS developer
Bob Relyea about the correct NSS trust flags to use.

[mattm, 2012/05/30 22:40:58]

Just tested this.

With PKIXVerifyCert:

CERTDB_VALID_CA|CERTDB_TERMINAL_RECORD verifies successfully
CERTDB_TERMINAL_RECORD gives SEC_ERROR_UNTRUSTED_CERT

With CERT_VerifyCert:

CERTDB_VALID_CA|CERTDB_TERMINAL_RECORD gives SEC_ERROR_UNTRUSTED_ISSUER
CERTDB_TERMINAL_RECORD gives SEC_ERROR_UNTRUSTED_ISSUER 

+
+    if (trustBits & net::CertDatabase::DISTRUSTED_EMAIL)
+      trust.emailFlags |= CERTDB_TERMINAL_RECORD;
+    else if (trustBits & net::CertDatabase::TRUSTED_EMAIL)
+      trust.emailFlags |= CERTDB_TRUSTED_CA | CERTDB_TRUSTED_CLIENT_CA;
+
+    if (trustBits & net::CertDatabase::DISTRUSTED_OBJ_SIGN)
+      trust.objectSigningFlags |= CERTDB_TERMINAL_RECORD;
+    else if (trustBits & net::CertDatabase::TRUSTED_OBJ_SIGN)
+      trust.objectSigningFlags |= CERTDB_TRUSTED_CA | CERTDB_TRUSTED_CLIENT_CA;
+
+    srv = CERT_ChangeCertTrust(CERT_GetDefaultCertDB(), nsscert, &trust);
   } else if (type == net::SERVER_CERT) {
-    // always start with untrusted and move up
-    trust.SetValidPeer();
-    trust.AddPeerTrust(trustBits & net::CertDatabase::TRUSTED_SSL, 0, 0);
-    srv = CERT_ChangeCertTrust(CERT_GetDefaultCertDB(),
-                               nsscert,
-                               trust.GetTrust());
+    CERTCertTrust trust = {0};
+    // We only modify the sslFlags, so copy the other flags.
+    CERT_GetCertTrust(nsscert, &trust);
+    trust.sslFlags = 0;
+
+    if (trustBits & net::CertDatabase::DISTRUSTED_SSL)
+      trust.sslFlags |= CERTDB_TERMINAL_RECORD;
+    else if (trustBits & net::CertDatabase::TRUSTED_SSL)
+      trust.sslFlags |= CERTDB_TRUSTED | CERTDB_TERMINAL_RECORD;
+
+    srv = CERT_ChangeCertTrust(CERT_GetDefaultCertDB(), nsscert, &trust);
   } else {
     // ignore user and email/unknown certs
     return true;