Build a chain in ClientCertStoreNSS to send intermediates to the server.

net/ssl/client_cert_store_nss.cc

Index: net/ssl/client_cert_store_nss.cc
diff --git a/net/ssl/client_cert_store_nss.cc b/net/ssl/client_cert_store_nss.cc
index 169351710b52677359a9566c46f848d77f24f6cd..d375872987ec7295d68f69ba01490cf395f232a9 100644
--- a/net/ssl/client_cert_store_nss.cc
+++ b/net/ssl/client_cert_store_nss.cc
@@ -15,6 +15,7 @@
 #include "base/strings/string_piece.h"
 #include "base/threading/worker_pool.h"
 #include "crypto/nss_crypto_module_delegate.h"
+#include "net/cert/scoped_nss_types.h"
 #include "net/cert/x509_util.h"
 #include "net/ssl/ssl_cert_request_info.h"
 
@@ -94,16 +95,48 @@ void ClientCertStoreNSS::FilterCertsOnWorkerThread(
     }
 
     // Check if the certificate issuer is allowed by the server.
-    if (request.cert_authorities.empty() ||
-        (!query_nssdb && cert->IsIssuedByEncoded(request.cert_authorities)) ||
-        (query_nssdb &&
-         NSS_CmpCertChainWCANames(handle, &ca_names) == SECSuccess)) {
-      DVLOG(2) << "matched cert: " << base::StringPiece(handle->nickname);
+    if (!request.cert_authorities.empty()) {
+      if ((!query_nssdb &&
+           !cert->IsIssuedByEncoded(request.cert_authorities)) ||
+          (query_nssdb &&
+           NSS_CmpCertChainWCANames(handle, &ca_names) != SECSuccess)) {
+        DVLOG(2) << "skipped non-matching cert: "
+                 << base::StringPiece(handle->nickname);
+        continue;
+      }
+    }
+
+    DVLOG(2) << "matched cert: " << base::StringPiece(handle->nickname);
+
+    if (!query_nssdb) {

[davidben, 2015/12/14 22:26:32]

This is only false in tests. Which... you could argue this means we're barely
testing anything and I would agree. Didn't see a clear way to avoid that, so I
left things as they were here.

[Ryan Sleevi, 2015/12/16 01:46:54]

Wait, it's false for all tests? There shouldn't be any reason we can't enable
this for tests at this point.

       filtered_certs->push_back(cert);
-    } else {
-      DVLOG(2) << "skipped non-matching cert: "
+      continue;
+    }
+
+    // Build a certificate chain from |handle| to include any
+    // intermediates. Some deployments expect the client to supply intermediates
+    // out of the local store. https://crbug.com/548631

[Ryan Sleevi, 2015/12/16 01:46:53]

// Attempt to construct a complete certificate chain from |handle|,
// as some servers expect the client to send intermediates.
//
// Consider a chain of EE -> A -> B, where -> indicates "issued by".
// The server may indicate that "B" is an acceptable certificate
// authority, and expect the client to send both EE and A, so that it
// can construct the path back to B. See https://crbug.com/548631 for
// an example.
//
// Note that there is an edge case, in the event of cross-certificates;
// namely, consider the following chains:
// EE -> A -> B
// EE -> A' -> C
// Where A and A' have the same subject and public key, but different
// issuers. Unfortunately, NSS does not offer an API to indicate which
// trust anchor(s) are preferred in chain building, thus it's possible
// for the client to send EE and A, when the server needs EE and A'.
//
// Ideally, servers would be properly configured such that they can
// always handle clients sending only EE, and that should be the first
// response if any other edge cases emerge. However, this best-effort
// case is enough to resolve some incompatabilities.

+    ScopedCERTCertificateList chain(CERT_CertChainFromCert(
+        handle, certUsageSSLClient, PR_FALSE /* includeRoot */));
+    if (!chain) {
+      DVLOG(2) << "could not build chain: "
                << base::StringPiece(handle->nickname);
+      continue;

[Ryan Sleevi, 2015/12/16 01:46:53]

Wow, NSS is dumb; it shouldn't choke if it can't build a chain, it should just
send the leaf.

+    }
+    std::vector<base::StringPiece> der_chain;
+    for (int i = 0; i < chain->len; i++) {
+      der_chain.push_back(
+          base::StringPiece(reinterpret_cast<const char*>(chain->certs[i].data),
+                            chain->certs[i].len));
+    }
+    scoped_refptr<X509Certificate> full_cert =
+        X509Certificate::CreateFromDERCertChain(der_chain);
+    if (!full_cert.get()) {
+      DVLOG(2) << "could not decode chain: "
+               << base::StringPiece(handle->nickname);
+      continue;
     }
+    filtered_certs->push_back(full_cert);
   }
   DVLOG(2) << "num_raw:" << num_raw
            << " num_filtered:" << filtered_certs->size();