nagai/fune
3
0
Fork 0
forked from mirrors/gecko-dev
Code Pull requests Activity

Bug 1487228 - (2/2) avoid holding CERTCertList instances long-term in nsNSSCertList r=jcj

Browse source 
Each instance of CERTCertList creates a PLArena with a chunk size of 2048 bytes,
but only needs space for 3 pointers per certificate in the list. The majority of
the time Gecko uses CERTCertList, we'll store ~3 certificates (although in some
cases we do store a few hundred, such as in tests or the certificate manager).
This is fairly inefficient. This patch starts the process of avoiding using
CERTCertList in Gecko by converting nsNSSCertList (i.e. nsIX509CertList) (as
well as nsNSSCertListEnumerator) to use a more efficient data structure to hold
references to certificates long-term. Future follow-up patches could (and
should) update certificate verification APIs in PSM to avoid CERTCertList as
well.

Depends on D5096

Differential Revision: https://phabricator.services.mozilla.com/D5097

--HG--
extra : moz-landing-system : lando
This commit is contained in:
Dana Keeler 2018-09-12 18:14:03 +00:00
parent 8f21632c33
commit fae63f9b28
3 changed files with 100 additions and 96 deletions
repo.diff.show_diff_stats Download patch file Download diff file Expand all files Collapse all files

144
security/manager/ssl/nsNSSCertificate.cpp
View file

@ -53,6 +53,26 @@ using namespace mozilla::psm;
extern LazyLogModule gPIPNSSLog;
class nsNSSCertListEnumerator : public nsSimpleEnumerator
{
public:
NS_DECL_NSISIMPLEENUMERATOR
const nsID& DefaultInterface() override { return NS_GET_IID(nsIX509Cert); }
explicit nsNSSCertListEnumerator(
const std::vector<UniqueCERTCertificate>& certs);
nsNSSCertListEnumerator(const nsNSSCertListEnumerator&) = delete;
void operator=(const nsNSSCertListEnumerator&) = delete;
private:
virtual ~nsNSSCertListEnumerator() = default;
std::vector<UniqueCERTCertificate> mCerts;
std::vector<UniqueCERTCertificate>::const_iterator mPosition;
};
// This is being stored in an uint32_t that can otherwise
// only take values from nsIX509Cert's list of cert types.
// As nsIX509Cert is frozen, we choose a value not contained
@ -837,18 +857,31 @@ NS_IMPL_ISUPPORTS_CI(nsNSSCertList,
nsNSSCertList::nsNSSCertList(UniqueCERTCertList certList)
{
if (certList) {
mCertList = std::move(certList);
} else {
mCertList = UniqueCERTCertList(CERT_NewCertList());
// Commonly we'll store only 3 certificates. If this is a verified certificate
// chain, it may be as many as 8 certificates. If this is a list of all known
// certificates, it may be a few hundred. We'll optimize for the common case
// (i.e. a verified certificate chain).
mCerts.reserve(8);
if (certList.get()) {
for (CERTCertListNode* node = CERT_LIST_HEAD(certList.get());
!CERT_LIST_END(node, certList.get());
node = CERT_LIST_NEXT(node)) {
UniqueCERTCertificate cert(CERT_DupCertificate(node->cert));
mCerts.push_back(std::move(cert));
}
}
}
nsNSSCertList::nsNSSCertList()
{
mCertList = UniqueCERTCertList(CERT_NewCertList());
// Commonly we'll store only 3 certificates. If this is a verified certificate
// chain, it may be as many as 8 certificates. If this is a list of all known
// certificates, it may be a few hundred. We'll optimize for the common case
// (i.e. a verified certificate chain).
mCerts.reserve(8);
}
// This is the implementation of nsIX509CertList.getCertList().
nsNSSCertList*
nsNSSCertList::GetCertList()
{
@ -864,15 +897,7 @@ nsNSSCertList::AddCert(nsIX509Cert* aCert)
NS_ERROR("Somehow got nullptr for mCertificate in nsNSSCertificate.");
return NS_ERROR_FAILURE;
}
if (!mCertList) {
NS_ERROR("Somehow got nullptr for mCertList in nsNSSCertList.");
return NS_ERROR_FAILURE;
}
if (CERT_AddCertToListTail(mCertList.get(), cert.get()) != SECSuccess) {
return NS_ERROR_OUT_OF_MEMORY;
}
Unused << cert.release(); // Ownership transferred to the cert list.
mCerts.push_back(std::move(cert));
return NS_OK;
}
@ -908,11 +933,6 @@ nsNSSCertList::DupCertList(const UniqueCERTCertList& certList)
NS_IMETHODIMP
nsNSSCertList::AsPKCS7Blob(/*out*/ nsACString& result)
{
MOZ_ASSERT(mCertList);
if (!mCertList) {
return NS_ERROR_FAILURE;
}
UniqueNSSCMSMessage cmsg(NSS_CMSMessage_Create(nullptr));
if (!cmsg) {
MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
@ -988,27 +1008,12 @@ nsNSSCertList::AsPKCS7Blob(/*out*/ nsACString& result)
NS_IMETHODIMP
nsNSSCertList::Write(nsIObjectOutputStream* aStream)
{
NS_ENSURE_STATE(mCertList);
nsresult rv = NS_OK;
// First, enumerate the certs to get the length of the list
uint32_t certListLen = 0;
CERTCertListNode* node = nullptr;
for (node = CERT_LIST_HEAD(mCertList);
!CERT_LIST_END(node, mCertList);
node = CERT_LIST_NEXT(node), ++certListLen) {
}
// Write the length of the list
rv = aStream->Write32(certListLen);
nsresult rv = aStream->Write32(mCerts.size());
// Repeat the loop, and serialize each certificate
node = nullptr;
for (node = CERT_LIST_HEAD(mCertList);
!CERT_LIST_END(node, mCertList);
node = CERT_LIST_NEXT(node))
{
nsCOMPtr<nsIX509Cert> cert = nsNSSCertificate::Create(node->cert);
// Serialize each certificate
for (const auto& certRef : mCerts) {
nsCOMPtr<nsIX509Cert> cert = nsNSSCertificate::Create(certRef.get());
if (!cert) {
rv = NS_ERROR_OUT_OF_MEMORY;
break;
@ -1027,16 +1032,13 @@ nsNSSCertList::Write(nsIObjectOutputStream* aStream)
NS_IMETHODIMP
nsNSSCertList::Read(nsIObjectInputStream* aStream)
{
NS_ENSURE_STATE(mCertList);
nsresult rv = NS_OK;
uint32_t certListLen;
rv = aStream->Read32(&certListLen);
nsresult rv = aStream->Read32(&certListLen);
if (NS_FAILED(rv)) {
return rv;
}
for(uint32_t i = 0; i < certListLen; ++i) {
for (uint32_t i = 0; i < certListLen; ++i) {
nsCOMPtr<nsISupports> certSupports;
rv = aStream->ReadObject(true, getter_AddRefs(certSupports));
if (NS_FAILED(rv)) {
@ -1056,13 +1058,7 @@ nsNSSCertList::Read(nsIObjectInputStream* aStream)
NS_IMETHODIMP
nsNSSCertList::GetEnumerator(nsISimpleEnumerator** _retval)
{
if (!mCertList) {
return NS_ERROR_FAILURE;
}
nsCOMPtr<nsISimpleEnumerator> enumerator =
new nsNSSCertListEnumerator(mCertList);
nsCOMPtr<nsISimpleEnumerator> enumerator(new nsNSSCertListEnumerator(mCerts));
enumerator.forget(_retval);
return NS_OK;
}
@ -1219,16 +1215,14 @@ nsNSSCertList::GetRootCertificate(/* out */ nsCOMPtr<nsIX509Cert>& aRoot)
if (aRoot) {
return NS_ERROR_UNEXPECTED;
}
CERTCertListNode* rootNode = CERT_LIST_TAIL(mCertList);
if (!rootNode) {
return NS_ERROR_UNEXPECTED;
}
if (CERT_LIST_END(rootNode, mCertList)) {
// Empty list, leave aRoot empty
// If the list is empty, leave aRoot empty.
if (mCerts.size() < 1) {
return NS_OK;
}
// Duplicates the certificate
aRoot = nsNSSCertificate::Create(rootNode->cert);
const UniqueCERTCertificate& cert = mCerts.back();
// This increases the refcount on the underlying CERTCertificate, which aRoot
// will own.
aRoot = nsNSSCertificate::Create(cert.get());
if (!aRoot) {
return NS_ERROR_OUT_OF_MEMORY;
}
@ -1236,39 +1230,41 @@ nsNSSCertList::GetRootCertificate(/* out */ nsCOMPtr<nsIX509Cert>& aRoot)
}
nsNSSCertListEnumerator::nsNSSCertListEnumerator(
const UniqueCERTCertList& certList)
const std::vector<UniqueCERTCertificate>& certs)
{
MOZ_ASSERT(certList);
mCertList = nsNSSCertList::DupCertList(certList);
mCerts.reserve(certs.size());
// Unfortunately we can't just clone the vector because we have to ensure the
// reference counts on the CERTCertificates are accurate.
for (const auto& certRef : certs) {
UniqueCERTCertificate cert(CERT_DupCertificate(certRef.get()));
mCerts.push_back(std::move(cert));
}
mPosition = mCerts.cbegin();
}
NS_IMETHODIMP
nsNSSCertListEnumerator::HasMoreElements(bool* _retval)
{
NS_ENSURE_TRUE(mCertList, NS_ERROR_FAILURE);
*_retval = !CERT_LIST_EMPTY(mCertList);
*_retval = mPosition != mCerts.cend();
return NS_OK;
}
NS_IMETHODIMP
nsNSSCertListEnumerator::GetNext(nsISupports** _retval)
{
NS_ENSURE_TRUE(mCertList, NS_ERROR_FAILURE);
CERTCertListNode* node = CERT_LIST_HEAD(mCertList);
if (CERT_LIST_END(node, mCertList)) {
return NS_ERROR_FAILURE;
*_retval = nullptr;
if (mPosition == mCerts.cend()) {
return NS_ERROR_UNEXPECTED;
}
nsCOMPtr<nsIX509Cert> nssCert = nsNSSCertificate::Create(node->cert);
const UniqueCERTCertificate& certRef = *mPosition;
// nsNSSCertificate::Create calls nsNSSCertificate::nsNSSCertificate, which
// increases the reference count on the underlying CERTCertificate itself.
nsCOMPtr<nsIX509Cert> nssCert = nsNSSCertificate::Create(certRef.get());
if (!nssCert) {
return NS_ERROR_OUT_OF_MEMORY;
}
nssCert.forget(_retval);
CERT_RemoveCertListNode(node);
mPosition++;
return NS_OK;
}

26
security/manager/ssl/nsNSSCertificate.h
View file

@ -81,7 +81,12 @@ public:
NS_DECL_NSIX509CERTLIST
NS_DECL_NSISERIALIZABLE
// certList is adopted
// The only way to call this is with std::move(some cert list) (because the
// copy constructor should be deleted for UniqueCERTCertList), so we
// effectively take ownership of it. What actually happens is we iterate
// through the list getting our own owned reference to each certificate in the
// list, and then the UniqueCERTCertList is dropped as it goes out of scope
// (thus releasing its own reference to each certificate).
explicit nsNSSCertList(mozilla::UniqueCERTCertList certList);
nsNSSCertList();
@ -117,29 +122,12 @@ public:
private:
virtual ~nsNSSCertList() {}
mozilla::UniqueCERTCertList mCertList;
std::vector<mozilla::UniqueCERTCertificate> mCerts;
nsNSSCertList(const nsNSSCertList&) = delete;
void operator=(const nsNSSCertList&) = delete;
};
class nsNSSCertListEnumerator : public nsSimpleEnumerator
{
public:
NS_DECL_NSISIMPLEENUMERATOR
const nsID& DefaultInterface() override { return NS_GET_IID(nsIX509Cert); }
explicit nsNSSCertListEnumerator(const mozilla::UniqueCERTCertList& certList);
private:
virtual ~nsNSSCertListEnumerator() {}
mozilla::UniqueCERTCertList mCertList;
nsNSSCertListEnumerator(const nsNSSCertListEnumerator&) = delete;
void operator=(const nsNSSCertListEnumerator&) = delete;
};
#define NS_X509CERT_CID { /* 660a3226-915c-4ffb-bb20-8985a632df05 */ \
0x660a3226, \
0x915c, \

26
security/manager/ssl/tests/gtest/DeserializeCertTest.cpp
View file

@ -6,6 +6,7 @@
#include "gtest/gtest.h"
#include "nsCOMPtr.h"
#include "nsISimpleEnumerator.h"
#include "nsITransportSecurityInfo.h"
#include "nsIX509Cert.h"
#include "nsIX509CertList.h"
@ -29,7 +30,8 @@
// in service workers. See bug 1248628.
void
deserializeAndVerify(const nsCString &serializedSecInfo,
bool hasFailedCertChain)
bool hasFailedCertChain,
size_t failedCertChainLength = 0)
{
nsCOMPtr<nsISupports> secInfo;
nsresult rv = NS_DeserializeObject(serializedSecInfo, getter_AddRefs(secInfo));
@ -48,8 +50,26 @@ deserializeAndVerify(const nsCString &serializedSecInfo,
rv = securityInfo->GetFailedCertChain(getter_AddRefs(failedChain));
ASSERT_EQ(NS_OK, rv);
if (hasFailedCertChain) {
if (hasFailedCertChain) {
ASSERT_TRUE(failedChain);
nsCOMPtr<nsISimpleEnumerator> enumerator;
rv = failedChain->GetEnumerator(getter_AddRefs(enumerator));
ASSERT_EQ(NS_OK, rv);
bool hasMore;
rv = enumerator->HasMoreElements(&hasMore);
ASSERT_EQ(NS_OK, rv);
size_t actualFailedCertChainLength = 0;
while (hasMore) {
nsCOMPtr<nsISupports> supports;
rv = enumerator->GetNext(getter_AddRefs(supports));
ASSERT_EQ(NS_OK, rv);
nsCOMPtr<nsIX509Cert> cert(do_QueryInterface(supports));
actualFailedCertChainLength++;
ASSERT_TRUE(cert);
rv = enumerator->HasMoreElements(&hasMore);
ASSERT_EQ(NS_OK, rv);
}
ASSERT_EQ(failedCertChainLength, actualFailedCertChainLength);
} else {
ASSERT_FALSE(failedChain);
}
@ -231,6 +251,6 @@ TEST(psm_DeserializeCert, preSSLStatusConsolidationFailedCertChain)
"uN2I5mM3NvMnP2Or19O1Bk//iGD6AyJfiZFcii+FsDrJhbzw6lakEV7O/EnD0kk2l7I0VMtg1xZBbEw7P6+V9zz5cAzaaq7EB0mC"
"E+jJckSzSETBN+7lyVD8gwmHYxxZfPnUM/yvPbMU9L3xWD/z6HHwO6r+9m7BT+2pHjBC");
deserializeAndVerify(base64Serialization, true);
deserializeAndVerify(base64Serialization, true, 2);
}