commit 8a0c099d09284c6717b599c6d70c4cc1913ecc55 Author: Zack Weinberg zackw@cmu.edu Date: Tue Jun 5 13:49:59 2012 -0700

Prepare for having more than one file that touches OpenSSL.

* Expose init_crypto, log_crypto_abort, and log_crypto_warn. * Use an explicit teardown function for OpenSSL state instead of relying on atexit().

Portability/reliability fixes en passant.

* Clarify situation with malloc/realloc in configure.ac. * Actually check whether we need -lm for floor(). * Use CRYPTO_set_mem_functions to send OpenSSL-internal memory allocations through xmalloc/xrealloc. --- configure.ac | 11 +++++--- src/audit-globals.sh | 4 +- src/crypt.cc | 72 +++++++++++++++++++++++++++++-------------------- src/crypt.h | 27 ++++++++++++++++++ src/main.cc | 3 ++ src/test/unittest.cc | 4 +++ 6 files changed, 86 insertions(+), 35 deletions(-)

diff --git a/configure.ac b/configure.ac index ab56311..889cfae 100644 --- a/configure.ac +++ b/configure.ac @@ -16,14 +16,12 @@ dnl with a rationale for each: dnl dnl Defined by C89, therefore unnecessary to check for nowadays: dnl -dnl AC_CHECK_FUNCS([atexit floor memset strcasecmp strchr strerror -dnl strrchr strstr strtoul]) +dnl AC_CHECK_FUNCS([memset strcasecmp strchr strerror strrchr strstr strtoul]) dnl AC_CHECK_HEADERS([limits.h stddef.h stdlib.h string.h]) dnl AC_CHECK_TYPES([ptrdiff_t]) dnl AC_TYPE_SIZE_T dnl -dnl We don't make use of the unspecified-behavior corner cases that -dnl these pin down: +dnl Dealt with by our 'xmalloc' and 'xrealloc' wrappers: dnl dnl AC_FUNC_MALLOC dnl AC_FUNC_REALLOC @@ -136,6 +134,11 @@ AC_SUBST(lib_CPPFLAGS) AX_LIB_WINSOCK2 LIBS="$LIBS $ws32_LIBS"

+# We might need to explicitly link -lm for floor(). +AC_SEARCH_LIBS([floor], [m], [], [ + AC_MSG_ERROR([unable to find 'floor']) +]) + ### System features ###

AC_CHECK_HEADERS([execinfo.h paths.h],,,[/**/]) diff --git a/src/audit-globals.sh b/src/audit-globals.sh index 13565aa..025549f 100644 --- a/src/audit-globals.sh +++ b/src/audit-globals.sh @@ -35,8 +35,8 @@ sed ' /^connections last_ckt_serial$/d /^connections last_conn_serial$/d /^connections shutting_down$/d - /^crypt init_crypto()::initialized$/d - /^crypt log_crypto()::initialized$/d + /^crypt crypto_initialized$/d + /^crypt crypto_errs_initialized$/d /^main allow_kq$/d /^main handle_signal_cb(int, short, void*)::got_sigint$/d /^main registration_helper$/d diff --git a/src/crypt.cc b/src/crypt.cc index 053f7e6..d4af5e0 100644 --- a/src/crypt.cc +++ b/src/crypt.cc @@ -11,30 +11,46 @@ #include <openssl/evp.h> #include <openssl/hmac.h>

-static void +static bool crypto_initialized = false; +static bool crypto_errs_initialized = false; + +#define REQUIRE_INIT_CRYPTO() \ + log_assert(crypto_initialized) + +void init_crypto() { - static bool initialized = false; - if (!initialized) { - initialized = true; - ENGINE_load_builtin_engines(); - ENGINE_register_all_complete(); - atexit(ENGINE_cleanup); - - // we don't need to call OpenSSL_add_all_algorithms or EVP_cleanup, - // since we never look up ciphers by textual name. - } + log_assert(!crypto_initialized); + + crypto_initialized = true; + CRYPTO_set_mem_functions(xmalloc, xrealloc, free); + ENGINE_load_builtin_engines(); + ENGINE_register_all_complete(); + + // we don't need to call OpenSSL_add_all_algorithms, since we never + // look up ciphers by textual name. +} + +void +free_crypto() +{ + // we don't need to call EVP_cleanup, since we never called + // OpenSSL_add_all_algorithms. + + if (crypto_initialized) + ENGINE_cleanup(); + if (crypto_errs_initialized) + ERR_free_strings(); }

static void log_crypto() { - static bool initialized = false; - if (!initialized) { - initialized = true; + if (!crypto_errs_initialized) { + crypto_errs_initialized = true; ERR_load_crypto_strings(); - atexit(ERR_free_strings); } + unsigned long err; while ((err = ERR_get_error()) != 0) log_warn("%s: %s: %s", @@ -43,14 +59,14 @@ log_crypto() ERR_reason_error_string(err)); }

-static void ATTR_NORETURN +void ATTR_NORETURN log_crypto_abort(const char *msg) { log_crypto(); log_abort("libcrypto error in %s", msg); }

-static int +int log_crypto_warn(const char *msg) { log_crypto(); @@ -145,7 +161,7 @@ namespace { ecb_encryptor * ecb_encryptor::create(const uint8_t *key, size_t keylen) { - init_crypto(); + REQUIRE_INIT_CRYPTO();

ecb_encryptor_impl *enc = new ecb_encryptor_impl; if (!EVP_EncryptInit_ex(&enc->ctx, aes_ecb_by_size(keylen), 0, key, 0)) @@ -159,7 +175,7 @@ ecb_encryptor::create(const uint8_t *key, size_t keylen) ecb_encryptor * ecb_encryptor::create(key_generator *gen, size_t keylen) { - init_crypto(); + REQUIRE_INIT_CRYPTO();

MemBlock key(keylen); size_t got = gen->generate(key, keylen); @@ -177,7 +193,7 @@ ecb_encryptor::create(key_generator *gen, size_t keylen) ecb_decryptor * ecb_decryptor::create(const uint8_t *key, size_t keylen) { - init_crypto(); + REQUIRE_INIT_CRYPTO();

ecb_decryptor_impl *dec = new ecb_decryptor_impl; if (!EVP_DecryptInit_ex(&dec->ctx, aes_ecb_by_size(keylen), 0, key, 0)) @@ -191,7 +207,7 @@ ecb_decryptor::create(const uint8_t *key, size_t keylen) ecb_decryptor * ecb_decryptor::create(key_generator *gen, size_t keylen) { - init_crypto(); + REQUIRE_INIT_CRYPTO();

MemBlock key(keylen); size_t got = gen->generate(key, keylen); @@ -272,7 +288,7 @@ namespace { gcm_encryptor * gcm_encryptor::create(const uint8_t *key, size_t keylen) { - init_crypto(); + REQUIRE_INIT_CRYPTO();

gcm_encryptor_impl *enc = new gcm_encryptor_impl; if (!EVP_EncryptInit_ex(&enc->ctx, aes_gcm_by_size(keylen), 0, key, 0)) @@ -284,7 +300,7 @@ gcm_encryptor::create(const uint8_t *key, size_t keylen) gcm_encryptor * gcm_encryptor::create(key_generator *gen, size_t keylen) { - init_crypto(); + REQUIRE_INIT_CRYPTO();

MemBlock key(keylen); size_t got = gen->generate(key, keylen); @@ -300,7 +316,7 @@ gcm_encryptor::create(key_generator *gen, size_t keylen) gcm_decryptor * gcm_decryptor::create(const uint8_t *key, size_t keylen) { - init_crypto(); + REQUIRE_INIT_CRYPTO();

gcm_decryptor_impl *dec = new gcm_decryptor_impl; if (!EVP_DecryptInit_ex(&dec->ctx, aes_gcm_by_size(keylen), 0, key, 0)) @@ -312,7 +328,7 @@ gcm_decryptor::create(const uint8_t *key, size_t keylen) gcm_decryptor * gcm_decryptor::create(key_generator *gen, size_t keylen) { - init_crypto(); + REQUIRE_INIT_CRYPTO();

MemBlock key(keylen); size_t got = gen->generate(key, keylen); @@ -430,6 +446,7 @@ key_generator::from_random_secret(const uint8_t *key, size_t klen, const uint8_t *ctxt, size_t clen) { log_assert(klen <= INT_MAX && slen < INT_MAX && clen < INT_MAX); + REQUIRE_INIT_CRYPTO();

MemBlock prk(SHA256_LEN);

@@ -438,8 +455,6 @@ key_generator::from_random_secret(const uint8_t *key, size_t klen, slen = SHA256_LEN; }

- init_crypto(); - if (HMAC(EVP_sha256(), salt, slen, key, klen, prk, 0) == 0) log_crypto_abort("key_generator::from_random_secret");

@@ -460,6 +475,7 @@ key_generator::from_passphrase(const uint8_t *phra, size_t plen, // salt, then put the result through HKDF-Extract with the salt.

log_assert(plen <= INT_MAX && slen < INT_MAX); + REQUIRE_INIT_CRYPTO();

MemBlock prk(SHA256_LEN);

@@ -468,8 +484,6 @@ key_generator::from_passphrase(const uint8_t *phra, size_t plen, slen = SHA256_LEN; }

- init_crypto(); - if (!PKCS5_PBKDF2_HMAC((const char *)phra, plen, salt, slen, 10000, EVP_sha256(), SHA256_LEN, prk)) log_crypto_abort("key_generator::from_passphrase"); diff --git a/src/crypt.h b/src/crypt.h index d7e5fbf..4da3309 100644 --- a/src/crypt.h +++ b/src/crypt.h @@ -9,6 +9,33 @@ const size_t AES_BLOCK_LEN = 16; const size_t GCM_TAG_LEN = 16; const size_t SHA256_LEN = 32;

+/** + * Initialize cryptography library. Must be called before anything that + * uses any of the APIs below. + */ +void init_crypto(); + +/** + * Tear down cryptography library. + */ +void free_crypto(); + +/** + * Report a cryptography failure. + * @msg should describe the operation that failed. + * Always returns -1; this allows you to write + * if (some operation failed) + * return log_crypto_warn("some operation"); + */ +int log_crypto_warn(const char *msg); + +/** + * Report a cryptography failure which is a fatal error. + * @msg should describe the operation that failed. + * Does not return. + */ +void ATTR_NORETURN log_crypto_abort(const char *msg); + struct key_generator;

struct ecb_encryptor diff --git a/src/main.cc b/src/main.cc index 9cc1300..dd69e68 100644 --- a/src/main.cc +++ b/src/main.cc @@ -381,6 +381,8 @@ main(int, const char *const *argv)

/* Configurations have been established; proceed with initialization. */

+ init_crypto(); + /* Ugly method to fix a Windows problem: http://archives.seul.org/libevent/users/Oct-2010/msg00049.html */ #ifdef _WIN32 @@ -499,6 +501,7 @@ main(int, const char *const *argv) free(stdin_eof); event_base_free(the_event_base); event_config_free(evcfg); + free_crypto(); log_close();

return 0; diff --git a/src/test/unittest.cc b/src/test/unittest.cc index cba4906..958c82c 100644 --- a/src/test/unittest.cc +++ b/src/test/unittest.cc @@ -108,6 +108,8 @@ main(int argc, const char **argv) log_set_method(LOG_METHOD_NULL, 0); }

+ init_crypto(); + /* Ugly method to fix a Windows problem: http://archives.seul.org/libevent/users/Oct-2010/msg00049.html */ #ifdef _WIN32 @@ -118,7 +120,9 @@ main(int argc, const char **argv) #endif

rv = tinytest_main(argc, argv, unittest_groups); + conn_start_shutdown(1); + free_crypto();

return rv; }