From 760fa362f334617bd96884b30a61c961cbf5b700 Mon Sep 17 00:00:00 2001
From: Adrien Beraud <adrien.beraud@savoirfairelinux.com>
Date: Wed, 29 Mar 2017 03:18:34 +0200
Subject: [PATCH] trust store: fix bad pointer access
gnutls_x509_trust_list_add_cas makes the trust list take ownership
of the certificate. The certificate is then destroyed by
gnutls_x509_trust_list_remove_cas.
At this point using existing shared_ptr<Certificate> pointing to this
structure will cause use of freed memory (bad pointer access).
To fix this we use new dht::crypto::Certificate method getCopy(),
that returns a raw gnutls certificate structure copy, to provide
to the trust list or any other APIs taking ownership of the object.
Change-Id: I6152ce0de5e463df01cfda0b18ef5c40a1739c2a
Tuleap: #1548
Reviewed-by: Guillaume Roguez <guillaume.roguez@savoirfairelinux.com>
---
src/security/certstore.cpp | 18 ++++++++++--------
src/security/tlsvalidator.cpp | 2 +-
2 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/src/security/certstore.cpp b/src/security/certstore.cpp
index 877b0177d9..e82d5cb002 100644
--- a/src/security/certstore.cpp
+++ b/src/security/certstore.cpp
@@ -329,7 +329,7 @@ CertificateStore::getTrustedCertificates() const
std::vector<gnutls_x509_crt_t> crts;
crts.reserve(trustedCerts_.size());
for (auto& crt : trustedCerts_)
- crts.emplace_back(crt->cert);
+ crts.emplace_back(crt->getCopy());
return crts;
}
@@ -404,14 +404,12 @@ statusToStr(TrustStatus s)
TrustStore::TrustStore()
{
- //gnutls_x509_trust_list_init(&trust_, 0);
gnutls_x509_trust_list_init(&allowed_, 0);
}
TrustStore::~TrustStore()
{
- //gnutls_x509_trust_list_deinit(trust_, false);
- gnutls_x509_trust_list_deinit(allowed_, false);
+ gnutls_x509_trust_list_deinit(allowed_, true);
}
TrustStore&
@@ -421,7 +419,7 @@ TrustStore::operator=(TrustStore&& o)
certStatus_ = std::move(o.certStatus_);
revokedList_ = std::move(o.revokedList_);
if (allowed_)
- gnutls_x509_trust_list_deinit(allowed_, false);
+ gnutls_x509_trust_list_deinit(allowed_, true);
allowed_ = std::move(o.allowed_);
o.allowed_ = nullptr;
return *this;
@@ -638,7 +636,7 @@ getRevocationList(const crypto::Certificate& crt)
const auto& crls = crt.getRevocationLists();
crls_ret.reserve(crls.size());
for (const auto& crl : crls)
- crls_ret.emplace_back(crl->get());
+ crls_ret.emplace_back(crl->getCopy());
return crls_ret;
}
@@ -663,10 +661,14 @@ TrustStore::setStoreCertStatus(const crypto::Certificate& crt, TrustStore::Permi
return;
if (status == PermissionStatus::ALLOWED) {
- gnutls_x509_trust_list_add_cas(allowed_, &crt.cert, 1, 0);
+ auto crt_copy = crt.getCopy();
+ gnutls_x509_trust_list_add_cas(allowed_, &crt_copy, 1, GNUTLS_TL_NO_DUPLICATES);
auto crls = getRevocationList(crt);
if (not crls.empty())
- if (gnutls_x509_trust_list_add_crls(allowed_, crls.data(), crls.size(), GNUTLS_TL_VERIFY_CRL, 0) == 0)
+ if (gnutls_x509_trust_list_add_crls(
+ allowed_,
+ crls.data(), crls.size(),
+ GNUTLS_TL_VERIFY_CRL | GNUTLS_TL_NO_DUPLICATES, 0) == 0)
RING_WARN("No CRLs where added");
}
else
diff --git a/src/security/tlsvalidator.cpp b/src/security/tlsvalidator.cpp
index 7a39ba8f2b..9af465942d 100644
--- a/src/security/tlsvalidator.cpp
+++ b/src/security/tlsvalidator.cpp
@@ -473,7 +473,7 @@ unsigned int TlsValidator::compareToCa()
GNUTLS_PROFILE_TO_VFLAGS(GNUTLS_PROFILE_MEDIUM),
&caValidationOutput_, nullptr);
- gnutls_x509_trust_list_deinit(trust, false);
+ gnutls_x509_trust_list_deinit(trust, true);
if (err) {
RING_WARN("gnutls_x509_trust_list_verify_crt2 failed: %s", gnutls_strerror(err));
--
GitLab