Fix certificate checker callback lifetime.
OpenSSL's SSL_CTX_set_verify() function allows us to set a callback
called after certificate validation but doesn't provide a way to pass
private data to this callback. CL:183832 was passing the pointer to the
CertificateChecker instance using a global pointer, nevertheless the
lifetime of this pointer was wrong since libcurl can trigger this
callback asynchronously when the SSL certificates are downloaded.
This patch converts the CertificateChecker into a singleton class and
uses the same trick previously used to pass the ServerToCheck value
using different callbacks.
Bug: 25818567
Test: Run an update on edison-userdebug; FEATURES=test emerge-link update_engine
Change-Id: I84cdb2f8c5ac86d1463634e73e867f213f7a2f5a
diff --git a/common/certificate_checker.cc b/common/certificate_checker.cc
index b8d8c94..86df950 100644
--- a/common/certificate_checker.cc
+++ b/common/certificate_checker.cc
@@ -18,12 +18,10 @@
#include <string>
-#include <base/lazy_instance.h>
#include <base/logging.h>
#include <base/strings/string_number_conversions.h>
#include <base/strings/string_util.h>
#include <base/strings/stringprintf.h>
-#include <base/threading/thread_local.h>
#include <curl/curl.h>
#include <openssl/evp.h>
#include <openssl/ssl.h>
@@ -36,13 +34,6 @@
namespace chromeos_update_engine {
-namespace {
-// A lazily created thread local storage for passing the current certificate
-// checker to the openssl callback.
-base::LazyInstance<base::ThreadLocalPointer<CertificateChecker>>::Leaky
- lazy_tls_ptr;
-} // namespace
-
bool OpenSSLWrapper::GetCertificateDigest(X509_STORE_CTX* x509_ctx,
int* out_depth,
unsigned int* out_digest_length,
@@ -63,53 +54,92 @@
return success;
}
+// static
+CertificateChecker* CertificateChecker::cert_checker_singleton_ = nullptr;
+
CertificateChecker::CertificateChecker(PrefsInterface* prefs,
- OpenSSLWrapper* openssl_wrapper,
- ServerToCheck server_to_check)
- : prefs_(prefs),
- openssl_wrapper_(openssl_wrapper),
- server_to_check_(server_to_check) {
+ OpenSSLWrapper* openssl_wrapper)
+ : prefs_(prefs), openssl_wrapper_(openssl_wrapper) {
+}
+
+CertificateChecker::~CertificateChecker() {
+ if (cert_checker_singleton_ == this)
+ cert_checker_singleton_ = nullptr;
+}
+
+void CertificateChecker::Init() {
+ CHECK(cert_checker_singleton_ == nullptr);
+ cert_checker_singleton_ = this;
}
// static
CURLcode CertificateChecker::ProcessSSLContext(CURL* curl_handle,
SSL_CTX* ssl_ctx,
void* ptr) {
- CertificateChecker* cert_checker = reinterpret_cast<CertificateChecker*>(ptr);
+ ServerToCheck* server_to_check = reinterpret_cast<ServerToCheck*>(ptr);
+
+ if (!cert_checker_singleton_) {
+ DLOG(WARNING) << "No CertificateChecker singleton initialized.";
+ return CURLE_FAILED_INIT;
+ }
// From here we set the SSL_CTX to another callback, from the openssl library,
// which will be called after each server certificate is validated. However,
// since openssl does not allow us to pass our own data pointer to the
- // callback, the certificate check will have to be done statically. To pass
- // the pointer to this instance, we use a thread-safe pointer in lazy_tls_ptr
- // during the calls and clear them after it.
- CHECK(cert_checker != nullptr);
- lazy_tls_ptr.Pointer()->Set(cert_checker);
- SSL_CTX_set_verify(ssl_ctx, SSL_VERIFY_PEER, VerifySSLCallback);
- // Sanity check: We should not re-enter this method during certificate
- // checking.
- CHECK(lazy_tls_ptr.Pointer()->Get() == cert_checker);
- lazy_tls_ptr.Pointer()->Set(nullptr);
+ // callback, the certificate check will have to be done statically. Since we
+ // need to know which update server we are using in order to check the
+ // certificate, we hardcode Chrome OS's two known update servers here, and
+ // define a different static callback for each. Since this code should only
+ // run in official builds, this should not be a problem. However, if an update
+ // server different from the ones listed here is used, the check will not
+ // take place.
+ int (*verify_callback)(int, X509_STORE_CTX*);
+ switch (*server_to_check) {
+ case ServerToCheck::kDownload:
+ verify_callback = &CertificateChecker::VerifySSLCallbackDownload;
+ break;
+ case ServerToCheck::kUpdate:
+ verify_callback = &CertificateChecker::VerifySSLCallbackUpdate;
+ break;
+ case ServerToCheck::kNone:
+ verify_callback = nullptr;
+ break;
+ }
+ SSL_CTX_set_verify(ssl_ctx, SSL_VERIFY_PEER, verify_callback);
return CURLE_OK;
}
// static
+int CertificateChecker::VerifySSLCallbackDownload(int preverify_ok,
+ X509_STORE_CTX* x509_ctx) {
+ return VerifySSLCallback(preverify_ok, x509_ctx, ServerToCheck::kDownload);
+}
+
+// static
+int CertificateChecker::VerifySSLCallbackUpdate(int preverify_ok,
+ X509_STORE_CTX* x509_ctx) {
+ return VerifySSLCallback(preverify_ok, x509_ctx, ServerToCheck::kUpdate);
+}
+
+// static
int CertificateChecker::VerifySSLCallback(int preverify_ok,
- X509_STORE_CTX* x509_ctx) {
- CertificateChecker* cert_checker = lazy_tls_ptr.Pointer()->Get();
- CHECK(cert_checker != nullptr);
- return cert_checker->CheckCertificateChange(preverify_ok, x509_ctx) ? 1 : 0;
+ X509_STORE_CTX* x509_ctx,
+ ServerToCheck server_to_check) {
+ CHECK(cert_checker_singleton_ != nullptr);
+ return cert_checker_singleton_->CheckCertificateChange(
+ preverify_ok, x509_ctx, server_to_check) ? 1 : 0;
}
bool CertificateChecker::CheckCertificateChange(int preverify_ok,
- X509_STORE_CTX* x509_ctx) {
+ X509_STORE_CTX* x509_ctx,
+ ServerToCheck server_to_check) {
TEST_AND_RETURN_FALSE(prefs_ != nullptr);
// If pre-verification failed, we are not interested in the current
// certificate. We store a report to UMA and just propagate the fail result.
if (!preverify_ok) {
- NotifyCertificateChecked(CertificateCheckResult::kFailed);
+ NotifyCertificateChecked(server_to_check, CertificateCheckResult::kFailed);
return false;
}
@@ -123,7 +153,7 @@
digest)) {
LOG(WARNING) << "Failed to generate digest of X509 certificate "
<< "from update server.";
- NotifyCertificateChecked(CertificateCheckResult::kValid);
+ NotifyCertificateChecked(server_to_check, CertificateCheckResult::kValid);
return true;
}
@@ -133,7 +163,7 @@
string storage_key =
base::StringPrintf("%s-%d-%d", kPrefsUpdateServerCertificate,
- static_cast<int>(server_to_check_), depth);
+ static_cast<int>(server_to_check), depth);
string stored_digest;
// If there's no stored certificate, we just store the current one and return.
if (!prefs_->GetString(storage_key, &stored_digest)) {
@@ -141,7 +171,7 @@
LOG(WARNING) << "Failed to store server certificate on storage key "
<< storage_key;
}
- NotifyCertificateChecked(CertificateCheckResult::kValid);
+ NotifyCertificateChecked(server_to_check, CertificateCheckResult::kValid);
return true;
}
@@ -152,19 +182,23 @@
LOG(WARNING) << "Failed to store server certificate on storage key "
<< storage_key;
}
- NotifyCertificateChecked(CertificateCheckResult::kValidChanged);
+ LOG(INFO) << "Certificate changed from " << stored_digest << " to "
+ << digest_string << ".";
+ NotifyCertificateChecked(server_to_check,
+ CertificateCheckResult::kValidChanged);
return true;
}
- NotifyCertificateChecked(CertificateCheckResult::kValid);
+ NotifyCertificateChecked(server_to_check, CertificateCheckResult::kValid);
// Since we don't perform actual SSL verification, we return success.
return true;
}
void CertificateChecker::NotifyCertificateChecked(
+ ServerToCheck server_to_check,
CertificateCheckResult result) {
if (observer_)
- observer_->CertificateChecked(server_to_check_, result);
+ observer_->CertificateChecked(server_to_check, result);
}
} // namespace chromeos_update_engine
diff --git a/common/certificate_checker.h b/common/certificate_checker.h
index bc675be..c785192 100644
--- a/common/certificate_checker.h
+++ b/common/certificate_checker.h
@@ -78,6 +78,9 @@
enum class ServerToCheck {
kUpdate = 0,
kDownload,
+
+ // Ignore this server.
+ kNone,
};
// Responsible for checking whether update server certificates change, and
@@ -96,10 +99,8 @@
CertificateCheckResult result) = 0;
};
- CertificateChecker(PrefsInterface* prefs,
- OpenSSLWrapper* openssl_wrapper,
- ServerToCheck server_to_check);
- virtual ~CertificateChecker() = default;
+ CertificateChecker(PrefsInterface* prefs, OpenSSLWrapper* openssl_wrapper);
+ ~CertificateChecker();
// This callback is called by libcurl just before the initialization of an
// SSL connection after having processed all other SSL related options. Used
@@ -109,6 +110,11 @@
SSL_CTX* ssl_ctx,
void* cert_checker);
+ // Initialize this class as the singleton instance. Only one instance can be
+ // initialized at a time and it should be initialized before other methods
+ // can be used.
+ void Init();
+
// Set the certificate observer to the passed instance. To remove the
// observer, pass a nullptr. The |observer| instance must be valid while this
// CertificateChecker verifies certificates.
@@ -120,24 +126,37 @@
FRIEND_TEST(CertificateCheckerTest, ChangedCertificate);
FRIEND_TEST(CertificateCheckerTest, FailedCertificate);
- // This callback is called by openssl after initial SSL verification. It is
- // used to perform any additional security verification on the connection,
- // but we use them here to get hold of the server certificate, in order to
- // determine if it has changed since the last connection. Since openssl forces
- // us to do this statically, we pass a pointer to this instance via a
- // thread-safe global pointer. The assigned callback is then called once per
+ // These callbacks are asynchronously called by openssl after initial SSL
+ // verification. They are used to perform any additional security verification
+ // on the connection, but we use them here to get hold of the server
+ // certificate, in order to determine if it has changed since the last
+ // connection. Since openssl forces us to do this statically, we define two
+ // different callbacks for the two different official update servers, and only
+ // assign the correspondent one. The assigned callback is then called once per
// each certificate on the server and returns 1 for success and 0 for failure.
- static int VerifySSLCallback(int preverify_ok, X509_STORE_CTX* x509_ctx);
+ static int VerifySSLCallbackDownload(int preverify_ok,
+ X509_STORE_CTX* x509_ctx);
+ static int VerifySSLCallbackUpdate(int preverify_ok,
+ X509_STORE_CTX* x509_ctx);
+ static int VerifySSLCallback(int preverify_ok,
+ X509_STORE_CTX* x509_ctx,
+ ServerToCheck server_to_check);
// Checks if server certificate stored in |x509_ctx| has changed since last
- // connection to that same server, specified by the |server_to_check_| member.
+ // connection to that same server, specified by |server_to_check|.
// This is called by the callbacks defined above. The result of the
// certificate check is passed to the observer, if any. Returns true on
// success and false otherwise.
- bool CheckCertificateChange(int preverify_ok, X509_STORE_CTX* x509_ctx);
+ bool CheckCertificateChange(int preverify_ok,
+ X509_STORE_CTX* x509_ctx,
+ ServerToCheck server_to_check);
// Notifies the observer, if any, of a certificate checking.
- void NotifyCertificateChecked(CertificateCheckResult result);
+ void NotifyCertificateChecked(ServerToCheck server_to_check,
+ CertificateCheckResult result);
+
+ // The CertificateChecker singleton instance.
+ static CertificateChecker* cert_checker_singleton_;
// Prefs instance used to store the certificates seen in the past.
PrefsInterface* prefs_;
@@ -145,9 +164,6 @@
// The wrapper for openssl operations.
OpenSSLWrapper* openssl_wrapper_;
- // The server to check from this certificate checker instance.
- ServerToCheck server_to_check_;
-
// The observer called whenever a certificate is checked, if not null.
Observer* observer_{nullptr};
diff --git a/common/certificate_checker_unittest.cc b/common/certificate_checker_unittest.cc
index 15811c0..c30acc5 100644
--- a/common/certificate_checker_unittest.cc
+++ b/common/certificate_checker_unittest.cc
@@ -50,6 +50,7 @@
cert_key_prefix_.c_str(),
static_cast<int>(server_to_check_),
depth_);
+ cert_checker.Init();
cert_checker.SetObserver(&observer_);
}
@@ -70,7 +71,7 @@
string cert_key_;
testing::StrictMock<MockCertificateCheckObserver> observer_;
- CertificateChecker cert_checker{&prefs_, &openssl_wrapper_, server_to_check_};
+ CertificateChecker cert_checker{&prefs_, &openssl_wrapper_};
};
// check certificate change, new
@@ -86,7 +87,8 @@
EXPECT_CALL(observer_,
CertificateChecked(server_to_check_,
CertificateCheckResult::kValid));
- ASSERT_TRUE(cert_checker.CheckCertificateChange(1, nullptr));
+ ASSERT_TRUE(
+ cert_checker.CheckCertificateChange(1, nullptr, server_to_check_));
}
// check certificate change, unchanged
@@ -103,7 +105,8 @@
EXPECT_CALL(observer_,
CertificateChecked(server_to_check_,
CertificateCheckResult::kValid));
- ASSERT_TRUE(cert_checker.CheckCertificateChange(1, nullptr));
+ ASSERT_TRUE(
+ cert_checker.CheckCertificateChange(1, nullptr, server_to_check_));
}
// check certificate change, changed
@@ -120,7 +123,8 @@
CertificateChecked(server_to_check_,
CertificateCheckResult::kValidChanged));
EXPECT_CALL(prefs_, SetString(cert_key_, digest_hex_)).WillOnce(Return(true));
- ASSERT_TRUE(cert_checker.CheckCertificateChange(1, nullptr));
+ ASSERT_TRUE(
+ cert_checker.CheckCertificateChange(1, nullptr, server_to_check_));
}
// check certificate change, failed
@@ -129,7 +133,8 @@
CertificateCheckResult::kFailed));
EXPECT_CALL(prefs_, GetString(_, _)).Times(0);
EXPECT_CALL(openssl_wrapper_, GetCertificateDigest(_, _, _, _)).Times(0);
- ASSERT_FALSE(cert_checker.CheckCertificateChange(0, nullptr));
+ ASSERT_FALSE(
+ cert_checker.CheckCertificateChange(0, nullptr, server_to_check_));
}
} // namespace chromeos_update_engine
diff --git a/common/libcurl_http_fetcher.cc b/common/libcurl_http_fetcher.cc
index 51643a4..13784fa 100644
--- a/common/libcurl_http_fetcher.cc
+++ b/common/libcurl_http_fetcher.cc
@@ -44,13 +44,9 @@
const int kNoNetworkRetrySeconds = 10;
} // namespace
-LibcurlHttpFetcher::LibcurlHttpFetcher(
- ProxyResolver* proxy_resolver,
- HardwareInterface* hardware,
- std::unique_ptr<CertificateChecker> certificate_checker)
- : HttpFetcher(proxy_resolver),
- hardware_(hardware),
- certificate_checker_(std::move(certificate_checker)) {
+LibcurlHttpFetcher::LibcurlHttpFetcher(ProxyResolver* proxy_resolver,
+ HardwareInterface* hardware)
+ : HttpFetcher(proxy_resolver), hardware_(hardware) {
// Dev users want a longer timeout (180 seconds) because they may
// be waiting on the dev server to build an image.
if (!hardware_->IsOfficialBuild())
@@ -237,10 +233,10 @@
CURLE_OK);
CHECK_EQ(curl_easy_setopt(curl_handle_, CURLOPT_SSL_CIPHER_LIST, "HIGH:!ADH"),
CURLE_OK);
- if (certificate_checker_ != nullptr) {
- CHECK_EQ(curl_easy_setopt(curl_handle_, CURLOPT_SSL_CTX_DATA,
- certificate_checker_.get()),
- CURLE_OK);
+ if (server_to_check_ != ServerToCheck::kNone) {
+ CHECK_EQ(
+ curl_easy_setopt(curl_handle_, CURLOPT_SSL_CTX_DATA, &server_to_check_),
+ CURLE_OK);
CHECK_EQ(curl_easy_setopt(curl_handle_, CURLOPT_SSL_CTX_FUNCTION,
CertificateChecker::ProcessSSLContext),
CURLE_OK);
diff --git a/common/libcurl_http_fetcher.h b/common/libcurl_http_fetcher.h
index df0a7be..900c973 100644
--- a/common/libcurl_http_fetcher.h
+++ b/common/libcurl_http_fetcher.h
@@ -40,11 +40,7 @@
class LibcurlHttpFetcher : public HttpFetcher {
public:
LibcurlHttpFetcher(ProxyResolver* proxy_resolver,
- HardwareInterface* hardware,
- std::unique_ptr<CertificateChecker> certificate_checker);
- LibcurlHttpFetcher(ProxyResolver* proxy_resolver,
- HardwareInterface* hardware)
- : LibcurlHttpFetcher(proxy_resolver, hardware, nullptr) {}
+ HardwareInterface* hardware);
// Cleans up all internal state. Does not notify delegate
~LibcurlHttpFetcher() override;
@@ -85,6 +81,10 @@
no_network_max_retries_ = retries;
}
+ void set_server_to_check(ServerToCheck server_to_check) {
+ server_to_check_ = server_to_check;
+ }
+
size_t GetBytesDownloaded() override {
return static_cast<size_t>(bytes_downloaded_);
}
@@ -231,9 +231,10 @@
// if we get a terminate request, queue it until we can handle it.
bool terminate_requested_{false};
- // The CertificateChecker used to check this connection's certificate. If no
- // certificate check needs to be performed, this should be empty.
- std::unique_ptr<CertificateChecker> certificate_checker_;
+ // The ServerToCheck used when checking this connection's certificate. If no
+ // certificate check needs to be performed, this should be set to
+ // ServerToCheck::kNone.
+ ServerToCheck server_to_check_{ServerToCheck::kNone};
int low_speed_limit_bps_{kDownloadLowSpeedLimitBps};
int low_speed_time_seconds_{kDownloadLowSpeedTimeSeconds};
