Fix client certificate authentication on Mac and Linux introduced in r178732

net/ssl/client_cert_store_impl_nss.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/ssl/client_cert_store_impl.h"
 
 #include <nss.h>
 #include <ssl.h>
 
 #include "base/logging.h"
 #include "net/cert/x509_util.h"
 
 namespace net {
 
 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)) {
     // Only offer unexpired certificates.
     if (CERT_CheckCertValidTimes(node->cert, PR_Now(), PR_TRUE) !=
         secCertTimeValid) {
       continue;
     }
 
     scoped_refptr<X509Certificate> cert = X509Certificate::CreateFromHandle(
         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(),
             x509_util::ClientCertSorter());
   return true;
 }
 
 }  // namespace
 
 bool ClientCertStoreImpl::GetClientCerts(const SSLCertRequestInfo& request,
                                          CertificateList* selected_certs) {
   CERTCertList* client_certs = CERT_FindUserCertsByUsage(
       CERT_GetDefaultCertDB(), certUsageSSLClient,
       PR_FALSE, PR_FALSE, NULL);
   // It is ok for a user not to have any client certs.
   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;
 }
 
 bool ClientCertStoreImpl::SelectClientCerts(const CertificateList& input_certs,
                                             const SSLCertRequestInfo& request,
                                             CertificateList* selected_certs) {
   CERTCertList* cert_list = CERT_NewCertList();
   if (!cert_list)
     return false;
   for (size_t i = 0; i < input_certs.size(); ++i) {
     CERT_AddCertToListTail(
         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;
 }
 
 }  // namespace net