Ensure key/cert pointer validity before accessing the DER

In many cases the getDER() function is called on a shared_ptr to a key or
certficiate without checking the pointer validity which may lead to segfaults.
Add proper checks before calling the getDER() function.

Change-Id: Ifb209737f14a13f6e7946e21c9d7c1cf5791973e

8 files changed, 102 insertions, 71 deletions

diff --git a/src/manager/client-async/client-manager-async-impl.cpp b/src/manager/client-async/client-manager-async-impl.cpp
index 96454cbd..3837771e 100644
--- a/src/manager/client-async/client-manager-async-impl.cpp
+++ b/src/manager/client-async/client-manager-async-impl.cpp
@@ -42,7 +42,7 @@ void ManagerAsync::Impl::saveKey(const ObserverPtr &observer,
 {
 	observerCheck(observer);
 
-	if (alias.empty() || !key) {
+	if (alias.empty() || !key || key->empty()) {
 		observer->ReceivedError(CKM_API_ERROR_INPUT_PARAM);
 		return;
 	}
@@ -62,7 +62,7 @@ void ManagerAsync::Impl::saveCertificate(const ObserverPtr &observer,
 {
 	observerCheck(observer);
 
-	if (alias.empty() || !cert) {
+	if (alias.empty() || !cert || cert->empty()) {
 		observer->ReceivedError(CKM_API_ERROR_INPUT_PARAM);
 		return;
 	}
diff --git a/src/manager/client-async/client-manager-async-impl.h b/src/manager/client-async/client-manager-async-impl.h
index c0cfaab5..65f4970a 100644
--- a/src/manager/client-async/client-manager-async-impl.h
+++ b/src/manager/client-async/client-manager-async-impl.h
@@ -135,19 +135,11 @@ public:
 		const T &trusted,
 		bool useSystemTrustedCertificates)
 	{
-		observerCheck(observer);
+		if (!certificate || certificate->empty())
+			ThrowMsg(Exc::InputParam, "Empty certificate");
 
-		if (!certificate) {
-			observer->ReceivedError(CKM_API_ERROR_INPUT_PARAM);
-			return;
-		}
-
-		try_catch_async([&]() {
-			sendToStorage(observer, static_cast<int>(command), m_counter,
-						  certificate->getDER(), untrusted, trusted, useSystemTrustedCertificates);
-		}, [&observer](int error) {
-			observer->ReceivedError(error);
-		});
+		sendToStorage(observer, static_cast<int>(command), m_counter,
+					  certificate->getDER(), untrusted, trusted, useSystemTrustedCertificates);
 	}
 
 	void crypt(
@@ -158,6 +150,8 @@ public:
 		const RawBuffer &input,
 		bool encryption);
 
+	static void observerCheck(const ManagerAsync::ObserverPtr &observer);
+
 private:
 	template <typename... Args>
 	void sendToStorage(const ManagerAsync::ObserverPtr &observer,
@@ -172,8 +166,6 @@ private:
 										   m_counter));
 	}
 
-	void observerCheck(const ManagerAsync::ObserverPtr &observer);
-
 	typedef std::unique_ptr<ConnectionThread> ConnectionThreadPtr;
 
 	ConnectionThreadPtr &thread()
diff --git a/src/manager/client-async/client-manager-async.cpp b/src/manager/client-async/client-manager-async.cpp
index 7ef0696a..471c8483 100644
--- a/src/manager/client-async/client-manager-async.cpp
+++ b/src/manager/client-async/client-manager-async.cpp
@@ -21,6 +21,7 @@
 
 #include <ckm/ckm-manager-async.h>
 #include <client-manager-async-impl.h>
+#include <exception.h>
 
 namespace CKM {
 
@@ -29,8 +30,11 @@ RawBufferVector toRawBufferVector(const CertificateShPtrVector &certificates)
 {
 	RawBufferVector rawBufferVector;
 
-	for (auto &e : certificates)
+	for (auto &e : certificates) {
+		if (!e || e->empty())
+			ThrowMsg(Exc::InputParam, "Empty certificate");
 		rawBufferVector.push_back(e->getDER());
+	}
 
 	return rawBufferVector;
 }
@@ -205,12 +209,18 @@ void ManagerAsync::getCertificateChain(const ObserverPtr &observer,
 									   const CertificateShPtrVector &trustedCertificates,
 									   bool useSystemTrustedCertificates)
 {
-	m_impl->getCertChain(observer,
-						 LogicCommand::GET_CHAIN_CERT,
-						 certificate,
-						 toRawBufferVector(untrustedCertificates),
-						 toRawBufferVector(trustedCertificates),
-						 useSystemTrustedCertificates);
+	Impl::observerCheck(observer);
+
+	try_catch_async([&]() {
+		m_impl->getCertChain(observer,
+							 LogicCommand::GET_CHAIN_CERT,
+							 certificate,
+							 toRawBufferVector(untrustedCertificates),
+							 toRawBufferVector(trustedCertificates),
+							 useSystemTrustedCertificates);
+	}, [&observer](int error) {
+		observer->ReceivedError(error);
+	});
 }
 
 void ManagerAsync::getCertificateChain(const ObserverPtr &observer,
@@ -219,12 +229,18 @@ void ManagerAsync::getCertificateChain(const ObserverPtr &observer,
 									   const AliasVector &trustedCertificates,
 									   bool useSystemTrustedCertificates)
 {
-	m_impl->getCertChain(observer,
-						 LogicCommand::GET_CHAIN_ALIAS,
-						 certificate,
-						 toLabelNameVector(untrustedCertificates),
-						 toLabelNameVector(trustedCertificates),
-						 useSystemTrustedCertificates);
+	Impl::observerCheck(observer);
+
+	try_catch_async([&]() {
+		m_impl->getCertChain(observer,
+							 LogicCommand::GET_CHAIN_ALIAS,
+							 certificate,
+							 toLabelNameVector(untrustedCertificates),
+							 toLabelNameVector(trustedCertificates),
+							 useSystemTrustedCertificates);
+	}, [&observer](int error) {
+		observer->ReceivedError(error);
+	});
 }
 
 void ManagerAsync::createSignature(const ObserverPtr &observer,
diff --git a/src/manager/client-capi/ckmc-manager.cpp b/src/manager/client-capi/ckmc-manager.cpp
index 2aa3c48b..37dd14fc 100644
--- a/src/manager/client-capi/ckmc-manager.cpp
+++ b/src/manager/client-capi/ckmc-manager.cpp
@@ -45,22 +45,34 @@ inline CKM::Policy _toCkmPolicy(const ckmc_policy_s &policy)
 	return CKM::Policy(_tostring(policy.password), policy.extractable);
 }
 
-inline CKM::KeyShPtr _toCkmKey(const ckmc_key_s *key)
+CKM::KeyShPtr _toCkmKey(const ckmc_key_s *key)
 {
-	return (key == nullptr) ?
-		   CKM::KeyShPtr() :
-		   CKM::Key::create(
+	if (key == nullptr)
+		return CKM::KeyShPtr();
+
+	auto ckmKey = CKM::Key::create(
 			   CKM::RawBuffer(key->raw_key, key->raw_key + key->key_size),
 			   _tostring(key->password));
+
+	if (!ckmKey || ckmKey->empty())
+		ThrowMsg(CKM::Exc::InvalidFormat, "Key parsing failed");
+
+	return ckmKey;
 }
 
-inline CKM::CertificateShPtr _toCkmCertificate(const ckmc_cert_s *cert)
+CKM::CertificateShPtr _toCkmCertificate(const ckmc_cert_s *cert)
 {
-	return (cert == nullptr) ?
-		   CKM::CertificateShPtr() :
-		   CKM::Certificate::create(
+	if (cert == nullptr)
+		return CKM::CertificateShPtr();
+
+	auto ckmCert = CKM::Certificate::create(
 			   CKM::RawBuffer(cert->raw_cert, cert->raw_cert + cert->cert_size),
 			   static_cast<CKM::DataFormat>(static_cast<int>(cert->data_format)));
+
+	if (!ckmCert || ckmCert->empty())
+		ThrowMsg(CKM::Exc::InvalidFormat, "Certificate parsing failed");
+
+	return ckmCert;
 }
 
 CKM::CertificateShPtrVector _toCkmCertificateVector(const ckmc_cert_list_s
@@ -101,6 +113,9 @@ ckmc_cert_list_s *_toNewCkmCertList(const CKM::CertificateShPtrVector
 	ckmc_cert_list_s *plist = nullptr;
 
 	for (const auto &e : certVector) {
+		if (!e || e->empty())
+			ThrowMsg(CKM::Exc::BadResponse, "Empty certificate received from server");
+
 		auto rawBuffer = e->getDER();
 		ckmc_cert_s *pcert = nullptr;
 		int ret = ckmc_cert_new(rawBuffer.data(), rawBuffer.size(), CKMC_FORM_DER,
@@ -223,6 +238,9 @@ int ckmc_get_key(const char *alias, const char *password, ckmc_key_s **key)
 	if ((ret = mgr->getKey(alias, _tostring(password), ckmKey)) != CKM_API_SUCCESS)
 		return to_ckmc_error(ret);
 
+	if (!ckmKey || ckmKey->empty())
+		return CKMC_ERROR_BAD_RESPONSE;
+
 	auto buffer = ckmKey->getDER();
 	return ckmc_key_new(
 			   buffer.data(),
@@ -287,13 +305,9 @@ int ckmc_save_cert(const char *alias, const ckmc_cert_s cert,
 	if (alias == nullptr || cert.raw_cert == nullptr || cert.cert_size == 0)
 		return CKMC_ERROR_INVALID_PARAMETER;
 
-	auto ckmCert = _toCkmCertificate(&cert);
-
-	if (!ckmCert)
-		return CKMC_ERROR_INVALID_FORMAT;
-
 	auto mgr = CKM::Manager::create();
-	return to_ckmc_error(mgr->saveCertificate(CKM::Alias(alias), ckmCert,
+	return to_ckmc_error(mgr->saveCertificate(CKM::Alias(alias),
+						 _toCkmCertificate(&cert),
 						 _toCkmPolicy(policy)));
 
 	EXCEPTION_GUARD_END
@@ -324,6 +338,9 @@ int ckmc_get_cert(const char *alias, const char *password, ckmc_cert_s **cert)
 								   ckmCert)) != CKM_API_SUCCESS)
 		return to_ckmc_error(ret);
 
+	if (!ckmCert || ckmCert->empty())
+		return CKMC_ERROR_BAD_RESPONSE;
+
 	auto buffer = ckmCert->getDER();
 	return ckmc_cert_new(buffer.data(), buffer.size(), CKMC_FORM_DER, cert);
 
@@ -424,6 +441,9 @@ int ckmc_get_pkcs12(const char *alias, const char *key_password,
 	auto pkcsKey = pkcs->getKey();
 
 	if (pkcsKey) {
+		if (pkcsKey->empty())
+			return CKMC_ERROR_BAD_RESPONSE;
+
 		ckmc_key_s *private_key = nullptr;
 		auto buffer = pkcsKey->getDER();
 		ckmc_key_type_e keyType = static_cast<ckmc_key_type_e>(pkcsKey->getType());
@@ -439,6 +459,9 @@ int ckmc_get_pkcs12(const char *alias, const char *key_password,
 	auto pkcsCert = pkcs->getCertificate();
 
 	if (pkcsCert) {
+		if (pkcsCert->empty())
+			return CKMC_ERROR_BAD_RESPONSE;
+
 		ckmc_cert_s *cert = nullptr;
 		CKM::RawBuffer buffer = pkcsCert->getDER();
 		ret = ckmc_cert_new(buffer.data(), buffer.size(), CKMC_FORM_DER, &cert);
@@ -716,15 +739,10 @@ int ckmc_get_cert_chain(const ckmc_cert_s *cert,
 			cert_chain_list == nullptr)
 		return CKMC_ERROR_INVALID_PARAMETER;
 
-	auto ckmCert = _toCkmCertificate(cert);
-
-	if (!ckmCert)
-		return CKMC_ERROR_INVALID_FORMAT;
-
 	CKM::CertificateShPtrVector ckmCertChain;
 	auto mgr = CKM::Manager::create();
 	int ret = mgr->getCertificateChain(
-				  ckmCert,
+				  _toCkmCertificate(cert),
 				  _toCkmCertificateVector(untrustedcerts),
 				  EMPTY_CERT_VECTOR,
 				  true,
@@ -750,14 +768,10 @@ int ckmc_get_cert_chain_with_alias(const ckmc_cert_s *cert,
 			cert_chain_list == nullptr)
 		return CKMC_ERROR_INVALID_PARAMETER;
 
-	auto ckmCert = _toCkmCertificate(cert);
-
-	if (!ckmCert)
-		return CKMC_ERROR_INVALID_FORMAT;
-
 	CKM::CertificateShPtrVector ckmCertChain;
 	auto mgr = CKM::Manager::create();
-	int ret = mgr->getCertificateChain(ckmCert, _toCkmAliasVector(untrustedcerts),
+	int ret = mgr->getCertificateChain(_toCkmCertificate(cert),
+									   _toCkmAliasVector(untrustedcerts),
 									   EMPTY_ALIAS_VECTOR, true, ckmCertChain);
 
 	if (ret != CKM_API_SUCCESS)
@@ -783,15 +797,10 @@ int ckmc_get_cert_chain_with_trustedcert(const ckmc_cert_s *cert,
 			ppcert_chain_list == nullptr)
 		return CKMC_ERROR_INVALID_PARAMETER;
 
-	auto ckmCert = _toCkmCertificate(cert);
-
-	if (!ckmCert)
-		return CKMC_ERROR_INVALID_PARAMETER;
-
 	CKM::CertificateShPtrVector ckmCertChain;
 	auto mgr = CKM::Manager::create();
 	int ret = mgr->getCertificateChain(
-				  ckmCert,
+				  _toCkmCertificate(cert),
 				  _toCkmCertificateVector(untrustedcerts),
 				  _toCkmCertificateVector(trustedcerts),
 				  sys_certs,
diff --git a/src/manager/client/client-common.cpp b/src/manager/client/client-common.cpp
index d0c7a2f5..9ca3df9e 100644
--- a/src/manager/client/client-common.cpp
+++ b/src/manager/client/client-common.cpp
@@ -39,6 +39,7 @@
 #include <ckm/ckm-error.h>
 #include <ckmc/ckmc-type.h>
 #include <client-common.h>
+#include <ckmc-type-converter.h>
 
 namespace {
 
@@ -341,6 +342,8 @@ int try_catch_enclosure(const std::function<int()> &func)
 {
 	try {
 		return func();
+	} catch (const Exc::Exception &e) {
+		return to_ckmc_error(e.error());
 	} catch (const std::bad_alloc &e) {
 		LogError("memory allocation exception: " << e.what());
 		return CKMC_ERROR_OUT_OF_MEMORY;
diff --git a/src/manager/client/client-manager-impl.cpp b/src/manager/client/client-manager-impl.cpp
index f1b68bb1..fa4f5a9f 100644
--- a/src/manager/client/client-manager-impl.cpp
+++ b/src/manager/client/client-manager-impl.cpp
@@ -143,7 +143,7 @@ int Manager::Impl::saveBinaryData(
 int Manager::Impl::saveKey(const Alias &alias, const KeyShPtr &key,
 						   const Policy &policy)
 {
-	if (key.get() == NULL)
+	if (key.get() == NULL || key->empty())
 		return CKM_API_ERROR_INPUT_PARAM;
 
 	try {
@@ -159,7 +159,7 @@ int Manager::Impl::saveCertificate(
 	const CertificateShPtr &cert,
 	const Policy &policy)
 {
-	if (cert.get() == NULL)
+	if (cert.get() == NULL || cert->empty())
 		return CKM_API_ERROR_INPUT_PARAM;
 
 	return saveBinaryData(alias, DataType::CERTIFICATE, cert->getDER(), policy);
@@ -626,11 +626,17 @@ int Manager::Impl::getCertificateChain(
 	if (!certificate || certificate->empty())
 		return CKM_API_ERROR_INPUT_PARAM;
 
-	for (auto &e : untrustedCertificates)
+	for (auto &e : untrustedCertificates) {
+		if (!e || e->empty())
+			return CKM_API_ERROR_INPUT_PARAM;
 		untrustedVector.push_back(e->getDER());
+	}
 
-	for (auto &e : trustedCertificates)
+	for (auto &e : trustedCertificates) {
+		if (!e || e->empty())
+			return CKM_API_ERROR_INPUT_PARAM;
 		trustedVector.push_back(e->getDER());
+	}
 
 	return getCertChain(
 			   m_storageConnection,
diff --git a/src/manager/common/exception.h b/src/manager/common/exception.h
index 874fbd0f..22188300 100644
--- a/src/manager/common/exception.h
+++ b/src/manager/common/exception.h
@@ -124,6 +124,10 @@ using InputParam =
 	DefineException<CKM_API_ERROR_INPUT_PARAM, true, PrintDebug>;
 using AuthenticationFailed =
 	DefineException<CKM_API_ERROR_AUTHENTICATION_FAILED, true, PrintDebug>;
+using InvalidFormat =
+	DefineException<CKM_API_ERROR_INVALID_FORMAT, true, PrintDebug>;
+using BadResponse =
+	DefineException<CKM_API_ERROR_BAD_RESPONSE, true, PrintDebug>;
 
 
 struct TransactionFailed : public DatabaseFailed {
diff --git a/src/manager/common/protocols.cpp b/src/manager/common/protocols.cpp
index a42d75d2..8cf65725 100644
--- a/src/manager/common/protocols.cpp
+++ b/src/manager/common/protocols.cpp
@@ -108,14 +108,15 @@ PKCS12Serializable::PKCS12Serializable(IStream &stream)
 	for (size_t i = 0; i < numCA; i++) {
 		RawBuffer CAcertData;
 		Deserialization::Deserialize(stream, CAcertData);
-		m_ca.emplace_back(CKM::Certificate::create(CAcertData, DataFormat::FORM_DER));
-
-		if (m_pkey)
+		auto ca = CKM::Certificate::create(CAcertData, DataFormat::FORM_DER);
+		if (ca) {
 			LogDebug("ca certificate from pkcs deserialized success. cert size: " <<
-					 CAcertData.size() << " and DER size: " << CKM::Certificate::create(CAcertData,
-							 DataFormat::FORM_DER)->getDER().size());
-		else
+					 CAcertData.size() << " and DER size: " << ca->getDER().size());
+
+			m_ca.emplace_back(std::move(ca));
+		} else {
 			LogError("ca certificate from pkcs deserialized fail");
+		}
 	}
 }