Fix client certificate authentication on Mac and Linux introduced in r178732

net/ssl/client_cert_store_impl_nss.cc

Index: net/ssl/client_cert_store_impl_nss.cc
diff --git a/net/ssl/client_cert_store_impl_nss.cc b/net/ssl/client_cert_store_impl_nss.cc
index 255f7700461010b24f969ceddd5e286dd2f2545b..716156e6820a889e61ba8e09dadd24718eab81de 100644
--- a/net/ssl/client_cert_store_impl_nss.cc
+++ b/net/ssl/client_cert_store_impl_nss.cc
@@ -16,11 +16,33 @@ namespace {
 
 bool GetClientCertsImpl(CERTCertList* cert_list,
                         const SSLCertRequestInfo& request,
+                        bool query_nssdb,
                         CertificateList* selected_certs) {
   DCHECK(cert_list);
   DCHECK(selected_certs);
 
   selected_certs->clear();
+
+  // Create a "fake" CERTDistNames structure. No public API exists to create
+  // one from a list of issuers.
+  CERTDistNames ca_names;
+  ca_names.arena = NULL;
+  ca_names.nnames = 0;
+  ca_names.names = NULL;
+  ca_names.head = NULL;
+
+  std::vector<SECItem> ca_names_items(request.cert_authorities.size());
+  for (size_t i = 0; i < request.cert_authorities.size(); ++i) {
+    const std::string& authority = request.cert_authorities[i];
+    ca_names_items[i].type = siDERNameBuffer;

[wtc, 2013/04/16 18:50:32]

This can be the default siBuffer, which seems to be the |type|
value NSS itself uses:
http://mxr.mozilla.org/nss/source/lib/certhigh/certhigh.c#541

I believe NSS_CmpCertChainWCANames doesn't care about the
|type| value, so it is best to not use siDERNameBuffer,
which is only used in one file:
http://mxr.mozilla.org/nss/ident?i=siDERNameBuffer

+    ca_names_items[i].data =
+        reinterpret_cast<unsigned char*>(const_cast<char*>(authority.data()));
+    ca_names_items[i].len = static_cast<unsigned int>(authority.size());
+  }
+  ca_names.nnames = static_cast<int>(ca_names_items.size());
+  if (!ca_names_items.empty())
+    ca_names.names = &ca_names_items[0];
+
   for (CERTCertListNode* node = CERT_LIST_HEAD(cert_list);
        !CERT_LIST_END(node, cert_list);
        node = CERT_LIST_NEXT(node)) {
@@ -34,11 +56,12 @@ bool GetClientCertsImpl(CERTCertList* cert_list,
         node->cert, X509Certificate::OSCertHandles());
 
     // Check if the certificate issuer is allowed by the server.
-    if (!request.cert_authorities.empty() &&
-        !cert->IsIssuedByEncoded(request.cert_authorities)) {
-      continue;
+    if (request.cert_authorities.empty() ||
+        cert->IsIssuedByEncoded(request.cert_authorities) ||
+        (query_nssdb &&
+         NSS_CmpCertChainWCANames(node->cert, &ca_names) == SECSuccess)) {

[wtc, 2013/04/16 18:50:32]

It seems that we only need to call one of cert->IsIssuedByEncoded()
and NSS_CmpCertChainWCANames(), depending on the value of
query_nssdb.

It also seems that cert->IsIssuedByEncoded() should call
NSS_CmpCertChainWCANames(). We used to call NSS_CmpCertChainWCANames()
but that call is gone. Perhaps that's the source of this bug?

[Ryan Sleevi, 2013/04/16 19:00:26]

Yes, that is the (real) root of the bug, but the issue here is as it relates to
testing.

IsIssuedByEncoded() was introduced so that we could have consistent
testing/filtering, even if we couldn't do 'actual' client cert discovery and
testing. See the ClientCertStoreImpl unittests.

IsIssuedByEncoded() only checks that |cert| or its
|intermediate_ca_certificates_| contain the issuer name - it does not attempt to
perform any chain/path discovery.

ClientCertStoreImpl is where we do the actual *discovery* portion of enumerating
chains. As discussed with ppi@ and digit@, the plan was to (eventually) move
this into an asynchronous operation, so that we could further optimize the time
spent here (eg: useful for proxies that require SSL client auth, or sites with
policy-configured certs) by doing things like caching.

GetClientCerts() is the "public" API, while "SelectClientCerts" is the testing
API (and in a follow-up, I'll rename it to be SelectClientCertsForTesting so we
get the nice presubmit warning).

For testing, we DON'T want to query any DBs - we want something hermetically
reproducible. That's why NSS_CmpCertWCANames() is only called on query_nssdb.

The bug came because the Windows code for querying the cert store was properly
ported, but the Linux & Mac implementations  weren't.

That said, I will switch it to only use IsIssuedByEncoded() when !query_nssdb,
since that's an easy optimization to make.

+      selected_certs->push_back(cert);
     }
-    selected_certs->push_back(cert);
   }
 
   std::sort(selected_certs->begin(), selected_certs->end(),
@@ -57,7 +80,7 @@ bool ClientCertStoreImpl::GetClientCerts(const SSLCertRequestInfo& request,
   if (!client_certs)
     return true;
 
-  bool rv = GetClientCertsImpl(client_certs, request, selected_certs);
+  bool rv = GetClientCertsImpl(client_certs, request, true, selected_certs);
   CERT_DestroyCertList(client_certs);
   return rv;
 }
@@ -73,7 +96,7 @@ bool ClientCertStoreImpl::SelectClientCerts(const CertificateList& input_certs,
         cert_list, CERT_DupCertificate(input_certs[i]->os_cert_handle()));
   }
 
-  bool rv = GetClientCertsImpl(cert_list, request, selected_certs);
+  bool rv = GetClientCertsImpl(cert_list, request, false, selected_certs);
   CERT_DestroyCertList(cert_list);
   return rv;
 }