I wrote this patch as part of some work I'm probably not going to finish. However, I think it is still useful, because it is progress towards removing some of the implicit state assumed by CertVerifier and NSSCertDBTrustDomain, and I don't want it to bitrot (again), so here it is.

Comment on attachment 8484678 [details] [diff] [review] pass-cert-chain-to-VerifySSLServerCert.patch Review of attachment 8484678 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. r=me with comments and TODOs addressed. ::: security/certverifier/CertVerifier.cpp @@ +194,5 @@ > + ScopedCERTCertList certChain(CERT_NewCertList()); > + if (!certChain) { > + return SECFailure; > + } > + ScopedCERTCertificate certCopy(CERT_DupCertificate(cert)); Looks like if CERT_DupCertificate is passed a null pointer, it'll return NULL without setting an error code. We should either do that ourselves (and set some invalid args error code) or PR_ASSERT(cert) like we used to. ::: security/certverifier/CertVerifier.h @@ +25,5 @@ > static const Flags FLAG_MUST_BE_EV; > > // *evOidPolicy == SEC_OID_UNKNOWN means the cert is NOT EV > // Only one usage per verification is supported. > + SECStatus VerifyCert(const CERTCertList& certChain, It would be good to document that the head of the list is the peer cert. ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +550,5 @@ > MOZ_ASSERT(infoObject); > + > + nsCOMPtr<nsIX509CertList> certChain; > + if (NS_FAILED(infoObject->GetFailedCertChain(getter_AddRefs(certChain)))) { > + // TODO: set error code So, when are these todos going to happen? @@ +965,5 @@ > AccumulateSubjectCommonNameTelemetry(commonName.get(), > commonNameInSubjectAltNames); > } > > +// Ownership of peerCertChain may be transfered from peerCertChain Looks like it's called certList, below. ::: security/manager/ssl/src/nsNSSIOLayer.cpp @@ +383,5 @@ > const nsACString& hostname, > int32_t port, > bool* _retval) > { > + // TODO: assert that we're on the socket transport service thread. I'm pretty sure there's pre-existing code to do this in SSLServerCertVerification.cpp.

Pushed by dkeeler@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/63f999fa8c19 include the peer cert chain from the TLS handshake when verifying server certificates r=kjacobs