[tor-commits] [stegotorus/master] Switch back to OpenSSL as cryptography library.

zwol at torproject.org zwol at torproject.org
Fri Jul 20 23:17:07 UTC 2012


commit 069607870199763ab539fc7d458646a6c3ee5f4b
Author: Zack Weinberg <zackw at panix.com>
Date:   Tue Mar 27 21:02:20 2012 -0700

    Switch back to OpenSSL as cryptography library.
---
 README.Linda               |    4 +-
 configure.ac               |    9 +-
 src/audit-globals.sh       |    2 +
 src/crypt.cc               |  598 +++++++++++++++++++++++++++-----------------
 src/crypt.h                |    1 +
 src/rng.cc                 |  260 +++++++++----------
 src/rng.h                  |    2 +-
 src/test/unittest.cc       |    6 +-
 src/test/unittest_crypt.cc |    6 +-
 src/util.h                 |    1 +
 10 files changed, 515 insertions(+), 374 deletions(-)

diff --git a/README.Linda b/README.Linda
index 83e0dad..3cb4d2e 100644
--- a/README.Linda
+++ b/README.Linda
@@ -22,9 +22,9 @@ automake (GNU automake) 1.11.1
 
 $> sudo port install pkgconfig
 
- d) libcrypto++, libevent-2, 
+ d) openssl >= 1.0.1, libevent-2
 
-$> sudo port install libcryptopp
+$> sudo port install openssl
 $> sudo port install libevent
 
  e) tor
diff --git a/configure.ac b/configure.ac
index d491479..9002078 100644
--- a/configure.ac
+++ b/configure.ac
@@ -43,16 +43,17 @@ AM_CONDITIONAL([INTEGRATION_TESTS], [test "$PYOS" = "posix"])
 
 ### Libraries ###
 
-# libcrypto++ 5.6.0 is the earliest version with GCM support.
-PKG_CHECK_MODULES([libcryptopp], [libcrypto++ >= 5.6.0])
+# Presently no need for libssl, only libcrypto.
+# We require version 1.0.1 for GCM support.
+PKG_CHECK_MODULES([libcrypto], [libcrypto >= 1.0.1])
 # libevent 2.0 radically changed the API
 PKG_CHECK_MODULES([libevent], [libevent >= 2.0])
 # there's no good reason not to require the latest zlib, which is
 # from 2009
 PKG_CHECK_MODULES([libz], [zlib >= 1.2.3.4])
 
-LIBS="$libevent_LIBS $libcryptopp_LIBS $libz_LIBS"
-lib_CPPFLAGS="$libevent_CFLAGS $libcryptopp_CFLAGS $libz_CFLAGS"
+LIBS="$libevent_LIBS $libcrypto_LIBS $libz_LIBS"
+lib_CPPFLAGS="$libevent_CFLAGS $libcrypto_CFLAGS $libz_CFLAGS"
 AC_SUBST(lib_CPPFLAGS)
 
 # ntohl and a bunch of related functions require a special library on Windows.
diff --git a/src/audit-globals.sh b/src/audit-globals.sh
index e1de07d..ec08b21 100644
--- a/src/audit-globals.sh
+++ b/src/audit-globals.sh
@@ -40,6 +40,8 @@ sed '
   /^util log_dest$/d
   /^util log_min_sev$/d
   /^util the_evdns_base$/d
+  /^crypt log_crypto()::initialized$/d
+  /^crypt init_crypto()::initialized$/d
 
   # These are grandfathered; they need to be removed.
   /^steg\/embed embed_init$/d
diff --git a/src/crypt.cc b/src/crypt.cc
index f0f9c4f..d964708 100644
--- a/src/crypt.cc
+++ b/src/crypt.cc
@@ -5,61 +5,137 @@
 #include "util.h"
 #include "crypt.h"
 
-// temporary, I hope, kludge
-#ifdef __APPLE_CC__
-#define CRYPTOPP_DISABLE_X86ASM
-#endif
-
-#include <cryptopp/aes.h>
-#include <cryptopp/hmac.h>
-#include <cryptopp/gcm.h>
-#include <cryptopp/sha.h>
-#include <cryptopp/secblock.h>
-
-// work around a bug in crypto++ 5.6.0's pwdbased.h
-#if CRYPTOPP_VERSION < 561
-namespace CryptoPP {
-class Integer {
-public:
-  Integer& operator++();
-  Integer& operator+(Integer const&);
-  void Encode(...);
-};
+#include <openssl/engine.h>
+#include <openssl/err.h>
+#include <openssl/evp.h>
+#include <openssl/hmac.h>
+
+static 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.
+  }
 }
-#endif
 
-#include <cryptopp/pwdbased.h>
+static void
+log_crypto()
+{
+  static bool initialized = false;
+  if (!initialized) {
+    initialized = true;
+    ERR_load_crypto_strings();
+    atexit(ERR_free_strings);
+  }
+  unsigned long err;
+  while ((err = ERR_get_error()) != 0)
+    log_warn("%s: %s: %s",
+             ERR_lib_error_string(err),
+             ERR_func_error_string(err),
+             ERR_reason_error_string(err));
+}
 
-// 32 bytes -> no extra memory allocation even for big keys, hopefully
-typedef CryptoPP::SecBlockWithHint<byte, 32> KeyBlock;
+static void ATTR_NORETURN
+log_crypto_abort(const char *msg)
+{
+  log_crypto();
+  log_abort("libcrypto error in %s", msg);
+}
 
-/* Note: this file wraps a C++ library into a C-style program and must
-   insulate that program from C++ semantics it is not prepared to handle;
-   most importantly, all exceptions must be converted to error codes. */
+static int
+log_crypto_warn(const char *msg)
+{
+  log_crypto();
+  log_warn("libcrypto error in %s", msg);
+  return -1;
+}
 
-#define CATCH_ALL_EXCEPTIONS(rv)                                \
-  catch (std::exception& e) {                                   \
-    log_warn("%s: %s", __func__, e.what());                     \
-  } catch (...) {                                               \
-    log_warn("%s: exception of abnormal type", __func__);       \
-  }                                                             \
-  return rv /* deliberate absence of semicolon */
+static const EVP_CIPHER *
+aes_ecb_by_size(size_t keylen)
+{
+  switch (keylen) {
+  case 16: return EVP_aes_128_ecb();
+  case 24: return EVP_aes_192_ecb();
+  case 32: return EVP_aes_256_ecb();
+  default:
+    log_abort("AES only supports 16, 24, or 32-byte keys");
+  }
+}
 
-// Crypto++ doesn't let us set a key without also setting an IV,
-// even though we will always override the IV later.
-static const uint8_t dummy_iv[16] = {};
+static const EVP_CIPHER *
+aes_gcm_by_size(size_t keylen)
+{
+  switch (keylen) {
+  case 16: return EVP_aes_128_gcm();
+  case 24: return EVP_aes_192_gcm();
+  case 32: return EVP_aes_256_gcm();
+  default:
+    log_abort("AES only supports 16, 24, or 32-byte keys");
+  }
+}
 
 namespace {
+
+  // loosely based on crypto++'s SecByteBlock
+  class MemBlock {
+  public:
+    explicit MemBlock(size_t l) : data(new uint8_t[l]), len(l)
+    { memset(data, 0, l); }
+
+    MemBlock(const uint8_t *d, size_t l) : data(new uint8_t[l]), len(l)
+    { if (d) memcpy(data, d, l); else memset(data, 0, l); }
+
+    ~MemBlock()
+    { memset(data, 0, len); delete [] data; }
+
+    operator const void*() const
+    { return data; }
+    operator void*()
+    { return data; }
+
+    operator const uint8_t*() const
+    { return data; }
+    operator uint8_t*()
+    { return data; }
+
+    const uint8_t *begin() const
+    { return data; }
+    uint8_t *begin()
+    { return data; }
+
+    const uint8_t *end() const
+    { return data+len; }
+    uint8_t *end()
+    { return data+len; }
+
+    size_t size() const { return len; }
+
+  private:
+    MemBlock(MemBlock const&) DELETE_METHOD;
+
+    uint8_t *data;
+    size_t len;
+  };
+
   struct ecb_encryptor_impl : ecb_encryptor
   {
-    CryptoPP::ECB_Mode<CryptoPP::AES>::Encryption ctx;
+    EVP_CIPHER_CTX ctx;
+    ecb_encryptor_impl() { EVP_CIPHER_CTX_init(&ctx); }
     virtual ~ecb_encryptor_impl();
     virtual void encrypt(uint8_t *out, const uint8_t *in);
   };
 
   struct ecb_decryptor_impl : ecb_decryptor
   {
-    CryptoPP::ECB_Mode<CryptoPP::AES>::Decryption ctx;
+    EVP_CIPHER_CTX ctx;
+    ecb_decryptor_impl() { EVP_CIPHER_CTX_init(&ctx); }
     virtual ~ecb_decryptor_impl();
     virtual void decrypt(uint8_t *out, const uint8_t *in);
   };
@@ -68,82 +144,98 @@ namespace {
 ecb_encryptor *
 ecb_encryptor::create(const uint8_t *key, size_t keylen)
 {
-  try {
-    ecb_encryptor_impl *enc = new ecb_encryptor_impl;
-    enc->ctx.SetKey(key, keylen);
-    return enc;
-  }
-  CATCH_ALL_EXCEPTIONS(0);
+  init_crypto();
+
+  ecb_encryptor_impl *enc = new ecb_encryptor_impl;
+  if (!EVP_EncryptInit_ex(&enc->ctx, aes_ecb_by_size(keylen), 0, key, 0))
+    log_crypto_abort("ecb_encryptor::create");
+  if (!EVP_CIPHER_CTX_set_padding(&enc->ctx, 0))
+    log_crypto_abort("ecb_encryptor::disable padding");
+
+  return enc;
 }
 
 ecb_encryptor *
 ecb_encryptor::create(key_generator *gen, size_t keylen)
 {
-  try {
-    KeyBlock key(keylen);
-    size_t got = gen->generate(key, keylen);
-    log_assert(got == keylen);
-
-    ecb_encryptor_impl *enc = new ecb_encryptor_impl;
-    enc->ctx.SetKey(key, keylen);
-    return enc;
-  }
-  CATCH_ALL_EXCEPTIONS(0);
+  init_crypto();
+
+  MemBlock key(keylen);
+  size_t got = gen->generate(key, keylen);
+  log_assert(got == keylen);
+
+  ecb_encryptor_impl *enc = new ecb_encryptor_impl;
+  if (!EVP_EncryptInit_ex(&enc->ctx, aes_ecb_by_size(keylen), 0, key, 0))
+    log_crypto_abort("ecb_encryptor::create");
+  if (!EVP_CIPHER_CTX_set_padding(&enc->ctx, 0))
+    log_crypto_abort("ecb_encryptor::disable padding");
+
+  return enc;
 }
 
 ecb_decryptor *
 ecb_decryptor::create(const uint8_t *key, size_t keylen)
 {
-  try {
-    ecb_decryptor_impl *dec = new ecb_decryptor_impl;
-    dec->ctx.SetKey(key, keylen);
-    return dec;
-  }
-  CATCH_ALL_EXCEPTIONS(0);
+  init_crypto();
+
+  ecb_decryptor_impl *dec = new ecb_decryptor_impl;
+  if (!EVP_DecryptInit_ex(&dec->ctx, aes_ecb_by_size(keylen), 0, key, 0))
+    log_crypto_abort("ecb_decryptor::create");
+  if (!EVP_CIPHER_CTX_set_padding(&dec->ctx, 0))
+    log_crypto_abort("ecb_decryptor::disable padding");
+
+  return dec;
 }
 
 ecb_decryptor *
 ecb_decryptor::create(key_generator *gen, size_t keylen)
 {
-  try {
-    KeyBlock key(keylen);
-    size_t got = gen->generate(key, keylen);
-    log_assert(got == keylen);
-
-    ecb_decryptor_impl *dec = new ecb_decryptor_impl;
-    dec->ctx.SetKey(key, keylen);
-    return dec;
-  }
-  CATCH_ALL_EXCEPTIONS(0);
+  init_crypto();
+
+  MemBlock key(keylen);
+  size_t got = gen->generate(key, keylen);
+  log_assert(got == keylen);
+
+  ecb_decryptor_impl *dec = new ecb_decryptor_impl;
+  if (!EVP_DecryptInit_ex(&dec->ctx, aes_ecb_by_size(keylen), 0, key, 0))
+    log_crypto_abort("ecb_decryptor::create");
+  if (!EVP_CIPHER_CTX_set_padding(&dec->ctx, 0))
+    log_crypto_abort("ecb_decryptor::disable padding");
+
+  return dec;
 }
 
 ecb_encryptor::~ecb_encryptor() {}
-ecb_encryptor_impl::~ecb_encryptor_impl() {}
+ecb_encryptor_impl::~ecb_encryptor_impl()
+{ EVP_CIPHER_CTX_cleanup(&ctx); }
+
 ecb_decryptor::~ecb_decryptor() {}
-ecb_decryptor_impl::~ecb_decryptor_impl() {}
+ecb_decryptor_impl::~ecb_decryptor_impl()
+{ EVP_CIPHER_CTX_cleanup(&ctx); }
 
 void
 ecb_encryptor_impl::encrypt(uint8_t *out, const uint8_t *in)
 {
-  try {
-    this->ctx.ProcessData(out, in, AES_BLOCK_LEN);
-  }
-  CATCH_ALL_EXCEPTIONS();
+  int olen;
+  if (!EVP_EncryptUpdate(&ctx, out, &olen, in, AES_BLOCK_LEN) ||
+      size_t(olen) != AES_BLOCK_LEN)
+    log_crypto_abort("ecb_encryptor::encrypt");
 }
 
 void
 ecb_decryptor_impl::decrypt(uint8_t *out, const uint8_t *in)
 {
-  try {
-    this->ctx.ProcessData(out, in, AES_BLOCK_LEN);
-  }
-  CATCH_ALL_EXCEPTIONS();
+  int olen;
+  if (!EVP_DecryptUpdate(&ctx, out, &olen, in, AES_BLOCK_LEN) ||
+      size_t(olen) != AES_BLOCK_LEN)
+    log_crypto_abort("ecb_decryptor::decrypt");
 }
 
 namespace {
   struct gcm_encryptor_impl : gcm_encryptor
   {
-    CryptoPP::GCM<CryptoPP::AES>::Encryption ctx;
+    EVP_CIPHER_CTX ctx;
+    gcm_encryptor_impl() { EVP_CIPHER_CTX_init(&ctx); }
     virtual ~gcm_encryptor_impl();
     virtual void encrypt(uint8_t *out, const uint8_t *in, size_t inlen,
                          const uint8_t *nonce, size_t nlen);
@@ -151,158 +243,202 @@ namespace {
 
   struct gcm_decryptor_impl : gcm_decryptor
   {
-    CryptoPP::GCM<CryptoPP::AES>::Decryption ctx;
+    EVP_CIPHER_CTX ctx;
+    gcm_decryptor_impl() { EVP_CIPHER_CTX_init(&ctx); }
     virtual ~gcm_decryptor_impl();
     virtual int decrypt(uint8_t *out, const uint8_t *in, size_t inlen,
                         const uint8_t *nonce, size_t nlen);
   };
 }
 
+// It *appears* (from inspecting the guts of libcrypto, *not* from the
+// documentation) that - at least for AES-GCM - it is legitimate to
+// call EVP_EncryptInit once with a key but no IV, and then once per
+// block with an IV but no key.  If this doesn't turn out to work,
+// there are also some (completely undocumented, feh) EVP_CTRL_* ops
+// that might help, but in the worst case we may have to save the key
+// at creation time, and delay the EVP_EncryptInit call to when we
+// know both key and IV, i.e. at block-encryption time.  This would be
+// unfortunate since it would entail doing AES key expansion once per
+// block instead of once per key.
+
+// It also *appears* (again, from inspecting the guts of libcrypto)
+// that we do not have to worry about EVP_EncryptFinal trying to do
+// PKCS padding when GCM is in use.  If this is wrong we will have to
+// find some way to make it not happen, which might entail ditching
+// EVP.  Feh, I say, feh.
+
 gcm_encryptor *
 gcm_encryptor::create(const uint8_t *key, size_t keylen)
 {
-  try {
-    gcm_encryptor_impl *enc = new gcm_encryptor_impl;
-    // sadly, these are not checkable at compile time
-    log_assert(enc->ctx.DigestSize() == GCM_TAG_LEN);
-    log_assert(!enc->ctx.NeedsPrespecifiedDataLengths());
-    enc->ctx.SetKeyWithIV(key, keylen, dummy_iv, sizeof dummy_iv);
-    return enc;
-  }
-  CATCH_ALL_EXCEPTIONS(0);
+  init_crypto();
+
+  gcm_encryptor_impl *enc = new gcm_encryptor_impl;
+  if (!EVP_EncryptInit_ex(&enc->ctx, aes_gcm_by_size(keylen), 0, key, 0))
+    log_crypto_abort("gcm_encryptor::create");
+
+  return enc;
 }
 
 gcm_encryptor *
 gcm_encryptor::create(key_generator *gen, size_t keylen)
 {
-  try {
-    KeyBlock key(keylen);
-    size_t got = gen->generate(key, keylen);
-    log_assert(got == keylen);
-
-    gcm_encryptor_impl *enc = new gcm_encryptor_impl;
-    // sadly, these are not checkable at compile time
-    log_assert(enc->ctx.DigestSize() == GCM_TAG_LEN);
-    log_assert(!enc->ctx.NeedsPrespecifiedDataLengths());
-    enc->ctx.SetKeyWithIV(key, keylen, dummy_iv, sizeof dummy_iv);
-    return enc;
-  }
-  CATCH_ALL_EXCEPTIONS(0);
+  init_crypto();
+
+  MemBlock key(keylen);
+  size_t got = gen->generate(key, keylen);
+  log_assert(got == keylen);
+
+  gcm_encryptor_impl *enc = new gcm_encryptor_impl;
+  if (!EVP_EncryptInit_ex(&enc->ctx, aes_gcm_by_size(keylen), 0, key, 0))
+    log_crypto_abort("gcm_encryptor::create");
+
+  return enc;
 }
 
 gcm_decryptor *
 gcm_decryptor::create(const uint8_t *key, size_t keylen)
 {
-  try {
-    gcm_decryptor_impl *dec = new gcm_decryptor_impl;
-    // sadly, these are not checkable at compile time
-    log_assert(dec->ctx.DigestSize() == GCM_TAG_LEN);
-    log_assert(!dec->ctx.NeedsPrespecifiedDataLengths());
-    dec->ctx.SetKeyWithIV(key, keylen, dummy_iv, sizeof dummy_iv);
-    return dec;
-  }
-  CATCH_ALL_EXCEPTIONS(0);
+  init_crypto();
+
+  gcm_decryptor_impl *dec = new gcm_decryptor_impl;
+  if (!EVP_DecryptInit_ex(&dec->ctx, aes_gcm_by_size(keylen), 0, key, 0))
+    log_crypto_abort("gcm_decryptor::create");
+
+  return dec;
 }
 
 gcm_decryptor *
 gcm_decryptor::create(key_generator *gen, size_t keylen)
 {
-  try {
-    KeyBlock key(keylen);
-    size_t got = gen->generate(key, keylen);
-    log_assert(got == keylen);
-
-    gcm_decryptor_impl *dec = new gcm_decryptor_impl;
-    // sadly, these are not checkable at compile time
-    log_assert(dec->ctx.DigestSize() == GCM_TAG_LEN);
-    log_assert(!dec->ctx.NeedsPrespecifiedDataLengths());
-    dec->ctx.SetKeyWithIV(key, keylen, dummy_iv, sizeof dummy_iv);
-    return dec;
-  }
-  CATCH_ALL_EXCEPTIONS(0);
+  init_crypto();
+
+  MemBlock key(keylen);
+  size_t got = gen->generate(key, keylen);
+  log_assert(got == keylen);
+
+  gcm_decryptor_impl *dec = new gcm_decryptor_impl;
+  if (!EVP_DecryptInit_ex(&dec->ctx, aes_gcm_by_size(keylen), 0, key, 0))
+    log_crypto_abort("gcm_decryptor::create");
+
+  return dec;
 }
 
 gcm_encryptor::~gcm_encryptor() {}
-gcm_encryptor_impl::~gcm_encryptor_impl() {}
+gcm_encryptor_impl::~gcm_encryptor_impl()
+{ EVP_CIPHER_CTX_cleanup(&ctx); }
 gcm_decryptor::~gcm_decryptor() {}
-gcm_decryptor_impl::~gcm_decryptor_impl() {}
+gcm_decryptor_impl::~gcm_decryptor_impl()
+{ EVP_CIPHER_CTX_cleanup(&ctx); }
 
 void
 gcm_encryptor_impl::encrypt(uint8_t *out, const uint8_t *in, size_t inlen,
                             const uint8_t *nonce, size_t nlen)
 {
-  try {
-    this->ctx.EncryptAndAuthenticate(out, out + inlen, 16,
-                                     nonce, nlen, 0, 0, in, inlen);
-  }
-  CATCH_ALL_EXCEPTIONS();
+  log_assert(inlen <= size_t(INT_MAX));
+
+  if (nlen != size_t(EVP_CIPHER_CTX_iv_length(&ctx)))
+    if (!EVP_CIPHER_CTX_ctrl(&ctx, EVP_CTRL_GCM_SET_IVLEN, nlen, 0))
+      log_crypto_abort("gcm_encryptor::reset nonce length");
+
+  if (!EVP_EncryptInit_ex(&ctx, 0, 0, 0, nonce))
+    log_crypto_abort("gcm_encryptor::set nonce");
+
+  int olen;
+  if (!EVP_EncryptUpdate(&ctx, 0, &olen, (const uint8_t *)"", 0) || olen != 0)
+    log_crypto_abort("gcm_encryptor::set null AAD");
+
+  if (!EVP_EncryptUpdate(&ctx, out, &olen, in, inlen) || size_t(olen) != inlen)
+    log_crypto_abort("gcm_encryptor::encrypt");
+
+  if (!EVP_EncryptFinal_ex(&ctx, out + inlen, &olen) || olen != 0)
+    log_crypto_abort("gcm_encryptor::finalize");
+
+  if (!EVP_CIPHER_CTX_ctrl(&ctx, EVP_CTRL_GCM_GET_TAG, 16, out + inlen))
+    log_crypto_abort("gcm_encryptor::write tag");
 }
 
 int
 gcm_decryptor_impl::decrypt(uint8_t *out, const uint8_t *in, size_t inlen,
                             const uint8_t *nonce, size_t nlen)
 {
-  try {
-    return this->ctx.DecryptAndVerify(out,
-                                      in + inlen - 16, 16,
-                                      nonce, nlen, 0, 0, in, inlen - 16)
-      ? 0 : -1; // caller will log decryption failure
-  }
-  CATCH_ALL_EXCEPTIONS(-1);
-}
+  log_assert(inlen <= size_t(INT_MAX));
+
+  if (nlen != size_t(EVP_CIPHER_CTX_iv_length(&ctx)))
+    if (!EVP_CIPHER_CTX_ctrl(&ctx, EVP_CTRL_GCM_SET_IVLEN, nlen, 0))
+      log_crypto_abort("gcm_decryptor::reset nonce length");
+
+  if (!EVP_DecryptInit_ex(&ctx, 0, 0, 0, nonce))
+    return log_crypto_warn("gcm_decryptor::set nonce");
 
-typedef CryptoPP::HMAC<CryptoPP::SHA256> HMAC_SHA256;
-typedef CryptoPP::PKCS5_PBKDF2_HMAC<CryptoPP::SHA256> PBKDF2_SHA256;
-const size_t SHA256_LEN = CryptoPP::SHA256::DIGESTSIZE;
+  if (!EVP_CIPHER_CTX_ctrl(&ctx, EVP_CTRL_GCM_SET_TAG, 16,
+                           (void *)(in + inlen - 16)))
+    return log_crypto_warn("gcm_decryptor::set tag");
+
+  int olen;
+  if (!EVP_DecryptUpdate(&ctx, 0, &olen, (const uint8_t *)"", 0) || olen != 0)
+    return log_crypto_warn("gcm_decryptor::set null AAD");
+
+  inlen -= 16;
+  if (!EVP_DecryptUpdate(&ctx, out, &olen, in, inlen) || size_t(olen) != inlen)
+    return log_crypto_warn("gcm_encryptor::decrypt");
+
+  if (!EVP_DecryptFinal_ex(&ctx, out + inlen, &olen) || olen != 0)
+    return log_crypto_warn("gcm_decryptor::check tag");
+
+  return 0;
+}
 
 namespace {
   struct key_generator_impl : key_generator
   {
-    HMAC_SHA256 expander;
-    CryptoPP::SecByteBlock prevT;
-    CryptoPP::SecByteBlock info;
+    HMAC_CTX expander;
+    MemBlock prevT;
+    MemBlock info;
+
     uint8_t counter;
     uint8_t leftover;
-    bool dead : 1;
+    bool dead;
 
     virtual ~key_generator_impl();
     virtual size_t generate(uint8_t *buf, size_t len);
 
-    key_generator_impl(const uint8_t *prk,
-                       const uint8_t *info, size_t ilen)
-      : expander(prk, SHA256_LEN),
-        prevT(0),
+    key_generator_impl(const uint8_t *prk, const uint8_t *info, size_t ilen)
+      : prevT(SHA256_LEN),
         info(info, ilen),
         counter(1),
         leftover(0),
         dead(false)
-    {}
+    {
+      HMAC_CTX_init(&expander);
+      if (!HMAC_Init_ex(&expander, prk, SHA256_LEN, EVP_sha256(), 0))
+        log_crypto_abort("key_generator_impl::construction");
+    }
   };
 }
 
+static const uint8_t nosalt[SHA256_LEN] = {};
+
 key_generator *
 key_generator::from_random_secret(const uint8_t *key,  size_t klen,
                                   const uint8_t *salt, size_t slen,
                                   const uint8_t *ctxt, size_t clen)
 {
-  try {
-    HMAC_SHA256 extractor;
-    static const uint8_t nosalt[SHA256_LEN] = {};
-    uint8_t prk[SHA256_LEN];
-
-    if (slen == 0) {
-      salt = nosalt;
-      slen = SHA256_LEN;
-    }
+  log_assert(klen <= INT_MAX && slen < INT_MAX && clen < INT_MAX);
 
-    extractor.SetKey(salt, slen);
-    extractor.CalculateDigest(prk, key, klen);
+  MemBlock prk(SHA256_LEN);
 
-    key_generator *r = new key_generator_impl(prk, ctxt, clen);
-    memset(prk, 0, SHA256_LEN);
-    return r;
+  if (slen == 0) {
+    salt = nosalt;
+    slen = SHA256_LEN;
   }
-  CATCH_ALL_EXCEPTIONS(0);
+
+  init_crypto();
+
+  if (HMAC(EVP_sha256(), salt, slen, key, klen, prk, 0) == 0)
+    log_crypto_abort("key_generator::from_random_secret");
+
+  return new key_generator_impl(prk, ctxt, clen);
 }
 
 key_generator *
@@ -310,30 +446,30 @@ key_generator::from_passphrase(const uint8_t *phra, size_t plen,
                                const uint8_t *salt, size_t slen,
                                const uint8_t *ctxt, size_t clen)
 {
-  try {
-    PBKDF2_SHA256 extractor;
-    static const uint8_t nosalt[SHA256_LEN] = {};
-    uint8_t prk[SHA256_LEN];
-
-    if (slen == 0) {
-      salt = nosalt;
-      slen = SHA256_LEN;
-    }
+  // The PBKDF2-HMAC<hash> construction, ignoring the iteration
+  // process, is very similar to the HKDF-Extract<hash> construction;
+  // the biggest difference is that you key the HMAC with the
+  // passphrase rather than the salt.  I *think* it is appropriate
+  // to just feed its output directly to the HKDF-Expand phase; an
+  // alternative would be to run PBKDF2 on the passphrase without a
+  // salt, then put the result through HKDF-Extract with the salt.
+
+  log_assert(plen <= INT_MAX && slen < INT_MAX);
+
+  MemBlock prk(SHA256_LEN);
 
-    // The PBKDF2-HMAC<hash> construction, ignoring the iteration
-    // process, is very similar to the HKDF-Extract<hash> construction;
-    // the biggest difference is that you key the HMAC with the
-    // passphrase rather than the salt.  I *think* it is appropriate
-    // to just feed its output directly to the HKDF-Expand phase; an
-    // alternative would be to run PBKDF2 on the passphrase without a
-    // salt, then put the result through HKDF-Extract with the salt.
-    extractor.DeriveKey(prk, SHA256_LEN, 0, phra, plen, salt, slen, 1000);
-
-    key_generator *r = new key_generator_impl(prk, ctxt, clen);
-    memset(prk, 0, SHA256_LEN);
-    return r;
+  if (slen == 0) {
+    salt = nosalt;
+    slen = SHA256_LEN;
   }
-  CATCH_ALL_EXCEPTIONS(0);
+
+  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");
+
+  return new key_generator_impl(prk, ctxt, clen);
 }
 
 size_t
@@ -343,45 +479,53 @@ key_generator_impl::generate(uint8_t *buf, size_t len)
     memset(buf, 0, len);
     return 0;
   }
-  try {
-    size_t n = 0;
-    if (leftover >= len) {
-      memcpy(buf, prevT.end() - leftover, len);
-      leftover -= len;
-      return len;
-    } else if (leftover) {
-      memcpy(buf, prevT.end() - leftover, leftover);
-      n += leftover;
-      leftover = 0;
+
+  size_t n = 0;
+  if (leftover >= len) {
+    memcpy(buf, prevT.end() - leftover, len);
+    leftover -= len;
+    return len;
+  } else if (leftover) {
+    memcpy(buf, prevT.end() - leftover, leftover);
+    n += leftover;
+    leftover = 0;
+  }
+  while (n < len) {
+    // compute the next block
+    if (!HMAC_Update(&expander, info, info.size()))
+      log_crypto_abort("generate::apply info");
+    if (!HMAC_Update(&expander, &counter, 1))
+      log_crypto_abort("generate::apply counter");
+    if (!HMAC_Final(&expander, prevT, 0))
+      log_crypto_abort("generate::extract");
+
+    if (n + SHA256_LEN < len) {
+      memcpy(buf + n, prevT, SHA256_LEN);
+      n += SHA256_LEN;
+    } else {
+      leftover = SHA256_LEN - (len - n);
+      memcpy(buf + n, prevT, len - n);
+      n = len;
     }
-    while (n < len) {
-      // generate the next block
-      expander.Update(prevT, prevT.size());
-      expander.Update(info, info.size());
-      expander.Update(&counter, 1);
-      counter++;
-      prevT.New(SHA256_LEN);
-      expander.Final(prevT);
-
-      if (n + SHA256_LEN < len) {
-        memcpy(buf + n, prevT, SHA256_LEN);
-        n += SHA256_LEN;
-      } else {
-        leftover = SHA256_LEN - (len - n);
-        memcpy(buf + n, prevT, len - n);
-        n = len;
-      }
-      if (counter == 0) {
-        if (n < len)
-          memset(buf + n, 0, len - n);
-        dead = true;
-        break;
-      }
+
+    // prepare to compute the next+1 block
+    counter++;
+    if (counter == 0) {
+      if (n < len)
+        memset(buf + n, 0, len - n);
+      dead = true;
+      break;
     }
-    return n;
+
+    if (!HMAC_Init_ex(&expander, 0, 0, 0, 0))
+      log_crypto_abort("generate::reset hmac");
+    if (!HMAC_Update(&expander, prevT, prevT.size()))
+      log_crypto_abort("generate::feedback");
   }
-  CATCH_ALL_EXCEPTIONS((memset(buf, 0, len), 0));
+
+  return n;
 }
 
 key_generator::~key_generator() {}
-key_generator_impl::~key_generator_impl() {}
+key_generator_impl::~key_generator_impl()
+{ HMAC_CTX_cleanup(&expander); }
diff --git a/src/crypt.h b/src/crypt.h
index 4136a35..b3bbcfa 100644
--- a/src/crypt.h
+++ b/src/crypt.h
@@ -7,6 +7,7 @@
 
 const size_t AES_BLOCK_LEN = 16;
 const size_t GCM_TAG_LEN   = 16;
+const size_t SHA256_LEN    = 32;
 
 struct key_generator;
 
diff --git a/src/rng.cc b/src/rng.cc
index 9b7ff57..86a55c1 100644
--- a/src/rng.cc
+++ b/src/rng.cc
@@ -5,112 +5,105 @@
 #include "util.h"
 #include "rng.h"
 
-#include <limits>
 #include <cmath>
-#include <cryptopp/osrng.h>
+#include <algorithm>
 
-/* Note: this file wraps a C++ library into a C-style program and must
-   insulate that program from C++ semantics it is not prepared to handle;
-   most importantly, all exceptions must be converted to error codes. */
+#include <openssl/rand.h>
 
-#define CATCH_ALL_EXCEPTIONS(rv)                                \
-  catch (std::exception& e) {                                   \
-    log_warn("%s: %s", __func__, e.what());                     \
-  } catch (...) {                                               \
-    log_warn("%s: exception of abnormal type", __func__);       \
-  }                                                             \
-  return rv /* deliberate absence of semicolon */
-
-static CryptoPP::AutoSeededRandomPool *rng;
-
-static void
-rng_teardown()
-{
-  delete rng;
-  rng = 0;
-}
-
-static void
-rng_init()
-{
-  if (!rng) {
-    rng = new CryptoPP::AutoSeededRandomPool;
-    if (!rng)
-      throw std::bad_alloc();
-    atexit(rng_teardown);
-  }
-}
+/* OpenSSL's rng is global, automatically seeds itself, and does not
+   appear to need to be torn down explicitly. */
 
 /**
-   Fills 'buf' with 'buflen' random bytes and returns 0 on success.
-   Returns -1 on failure.
-*/
-int
+ * Fills 'buf' with 'buflen' random bytes.  Cannot fail.
+ */
+void
 rng_bytes(uint8_t *buf, size_t buflen)
 {
-  try {
-    rng_init();
-    rng->GenerateBlock(buf, buflen);
-    return 0;
-  }
-  CATCH_ALL_EXCEPTIONS(-1);
+  log_assert(buflen < INT_MAX);
+  int rv = RAND_bytes(buf, (int)buflen);
+  log_assert(rv);
 }
 
-
-/** Return a pseudorandom integer, chosen uniformly from the values
- * between 0 and <b>max</b>-1 inclusive.  <b>max</b> must be between 1 and
- * INT_MAX+1, inclusive. */
+/**
+ * Return a pseudorandom integer, chosen uniformly from the values
+ * in the range [0, max). 'max' must be in the range [1, INT_MAX).
+ */
 int
 rng_int(unsigned int max)
 {
-  log_assert(max <= ((unsigned int)INT_MAX)+1);
-  log_assert(max > 0); /* don't div by 0 */
-
-  try {
-    rng_init();
-    return rng->GenerateWord32(0, max-1);
+  log_assert(max > 0 && max <= ((unsigned int)INT_MAX)+1);
+
+  /* rng_bytes will only give us a whole number of bytes, so to get a
+     uniformly random number in [0, max) we need to rejection-sample.
+     To minimize the number of rejections, we do the following: Find
+     the least k ("nbits") such that 2**k >= max, and the least b
+     ("nbytes") such that b*CHAR_BIT >= k.  Generate b random bytes,
+     and rearrange them into an integer.  Mask off all but the least k
+     bits.  Accept the result if it is less than max.  This way, the
+     probability of accepting each candidate result is always greater
+     than 0.5.  */
+
+  unsigned int nbytes, nbits, mask, rv;
+  unsigned char buf[sizeof(int)];
+
+#if __GNUC__ >= 4
+  nbits = CHAR_BIT*sizeof(int) - __builtin_clz(max);
+#else
+#error "Need fallback for __builtin_clz"
+#endif
+  nbytes = (nbits / CHAR_BIT) + 1;
+  mask = (1U << nbits) - 1;
+
+  for (;;) {
+    rng_bytes(buf, nbytes);
+
+    rv = 0;
+    for (unsigned int i = 0; i < nbytes; i++)
+      rv = (rv << CHAR_BIT) | buf[i];
+
+    rv &= mask;
+
+    if (rv < max)
+      return rv;
   }
-  CATCH_ALL_EXCEPTIONS(-1);
 }
 
-/** Return a pseudorandom integer, chosen uniformly from the values
+/**
+ * Return a pseudorandom integer, chosen uniformly from the values
  * between 'min' and 'max-1', inclusive.  'max' must be between
- * 'min+1' and 'INT_MAX+1', inclusive. */
+ * 'min+1' and 'INT_MAX+1', inclusive.
+ */
 int
 rng_range(unsigned int min, unsigned int max)
 {
   log_assert(max <= ((unsigned int)INT_MAX)+1);
   log_assert(max > min);
 
-  try {
-    rng_init();
-    return rng->GenerateWord32(min, max-1);
-  }
-  CATCH_ALL_EXCEPTIONS(-1);
+  return min + rng_int(max - min);
 }
 
-/** Internal use only (can be externalized if someone has a good use
- *  for it): generate a random double-precision floating-point number
- *  in the range (0.0, 1.0] (note that this is _not_ the usual convention,
- *  but it saves a call to nextafter() in the sole current user).
- *
- *  For what we use this for, it is important that we can, at least
- *  potentially, generate _every_ representable real number in the
- *  desired interval, with genuine uniformity.  The usual tactic of
- *  generating a random integer and dividing does not do this, because
- *  the rational numbers produced by random()/MAX are evenly spaced on
- *  the real line, but floating point numbers close to zero are *not*.
+/**
+ * Internal use only (can be externalized if someone has a good use
+ * for it): generate a random double-precision floating-point number
+ * in the range (0.0, 1.0] (note that this is _not_ the usual convention,
+ * but it saves a call to nextafter() in the sole current user).
  *
- *  For the same reason, the trick for avoiding division suggested
- *  e.g. by "Common Lisp, the Language", generating a random number in
- *  [1.0, 2.0) by overwriting the mantissa of a 1.0 and then
- *  subtracting 1.0, does not help -- you can do the first step
- *  precisely because the representable binary floating point numbers
- *  between 1.0 and 2.0 *are* evenly spaced on the real line.
+ * For what we use this for, it is important that we can, at least
+ * potentially, generate _every_ representable real number in the
+ * desired interval, with genuine uniformity.  The usual tactic of
+ * generating a random integer and dividing does not do this, because
+ * the rational numbers produced by random()/MAX are evenly spaced on
+ * the real line, but floating point numbers close to zero are *not*.
  *
- *  The more complicated, but correct, algorithm here was developed by
- *  Allen B. Downey: http://allendowney.com/research/rand/
+ * For the same reason, the trick for avoiding division suggested
+ * e.g. by "Common Lisp, the Language", generating a random number in
+ * [1.0, 2.0) by overwriting the mantissa of a 1.0 and then
+ * subtracting 1.0, does not help -- you can do the first step
+ * precisely because the representable binary floating point numbers
+ * between 1.0 and 2.0 *are* evenly spaced on the real line.
  *
+ * The more complicated, but correct, algorithm here was developed by
+ * Allen B. Downey: http://allendowney.com/research/rand/
  */
 static double
 rng_double()
@@ -122,7 +115,7 @@ rng_double()
     bool get()
     {
       if (n == 0) {
-        bits = rng->GenerateByte();
+        rng_bytes((uint8_t *)&bits, 1);
         n = CHAR_BIT;
       }
       bool rv = bits & 1;
@@ -135,66 +128,64 @@ rng_double()
     unsigned int n;
   };
 
+  static_assert(sizeof(double) == sizeof(uint64_t),
+                "this code works only with 64-bit, IEEE double");
   union ieee754_double {
     double d;
     uint64_t i;
   };
 
-  try {
-    rng_init();
-
-    /* Because of how the Crypto++ RNG works, it is convenient to
-       generate the mantissa first, contra Downey, and use the
-       leftover bits to seed the bit-generator that we use for the
-       exponent; this does not change the algorithm fundamentally,
-       because only the final adjustment step depends on both. */
-
-    uint64_t mantissa = rng->GenerateWord32();
-    uint32_t b = rng->GenerateWord32();
-
-    mantissa |= uint64_t(b & 0x000FFFFF) << 32;
-
-    /* This is the core of Downey's algorithm: 50% of the time we
-       should generate the highest exponent of a number in (0,1) (note
-       that _neither_ endpoint is included right now).  25% of the
-       time, we should generate the second highest exponent, 12.5% of
-       the time, we should generate the third highest, and so on.  In
-       other words, we should start with the highest exponent, flip a
-       coin, and keep subtracting 1 until either we hit zero or the
-       coin comes up heads.
-
-       If anyone knows how to do this in _constant_ time, instead of
-       variable time bounded by a constant, please tell me.
-    */
-
-    rngbit bits((b & 0xFFF00000) >> 20, 12);
-    uint32_t exponent = 0x3FE; /* 1111111110 = 2^{-1} */
-    do {
-      if (bits.get()) break;
-    } while (--exponent);
-
-    /* Finally a slight adjustment: if the mantissa is zero, then
-       half the time we should increment the exponent by one.
-       Do this unconditionally if the exponent is also zero
-       (so we never generate 0.0). */
-    if (mantissa == 0 && (exponent == 0 || bits.get()))
-      exponent++;
-
-    /* Assemble and return the number. */
-    union ieee754_double n;
-    n.i = (uint64_t(exponent) << 52) | mantissa;
-    return n.d;
-  }
-  CATCH_ALL_EXCEPTIONS(std::numeric_limits<double>::quiet_NaN());
+  /* It is convenient to generate the mantissa first, contra Downey,
+     and use the leftover bits to seed the bit-generator that we use
+     for the exponent; this does not change the algorithm
+     fundamentally, because only the final adjustment step depends
+     on both. */
+
+  uint64_t mantissa = 0;
+  rng_bytes((uint8_t *)&mantissa, sizeof(mantissa));
+
+  rngbit bits(uint32_t(mantissa >> 52), 12);
+  mantissa &= UINT64_C(0x000FFFFFFFFFFFFF);
+
+  /* This is the core of Downey's algorithm: 50% of the time we
+     should generate the highest exponent of a number in (0,1) (note
+     that _neither_ endpoint is included right now).  25% of the
+     time, we should generate the second highest exponent, 12.5% of
+     the time, we should generate the third highest, and so on.  In
+     other words, we should start with the highest exponent, flip a
+     coin, and keep subtracting 1 until either we hit zero or the
+     coin comes up heads.
+
+     If anyone knows how to do this in _constant_ time, instead of
+     variable time bounded by a constant, please tell me.
+  */
+
+  uint32_t exponent = 0x3FE; /* 1111111110 = 2^{-1} */
+  do {
+    if (bits.get()) break;
+  } while (--exponent);
+
+  /* Finally a slight adjustment: if the mantissa is zero, then
+     half the time we should increment the exponent by one.
+     Do this unconditionally if the exponent is also zero
+     (so we never generate 0.0). */
+  if (mantissa == 0 && (exponent == 0 || bits.get()))
+    exponent++;
+
+  /* Assemble and return the number. */
+  union ieee754_double n;
+  n.i = (uint64_t(exponent) << 52) | mantissa;
+  return n.d;
 }
 
-/** Return a random integer in the range [0, hi),
- *  from a truncated geometric distribution whose expected value
- *  (prior to truncation) is 'xv'.
- *  (The rate parameter 'lambda' that's usually used to characterize
- *  the geometric/exponential distribution is equal to 1/xv.)
- *  'hi' must be no more than INT_MAX+1, as for 'rng_range'.
- *  'xv' must be greater than 0 and less than 'hi'.
+/**
+ * Return a random integer in the range [0, hi),
+ * from a truncated geometric distribution whose expected value
+ * (prior to truncation) is 'xv'.
+ * (The rate parameter 'lambda' that's usually used to characterize
+ * the geometric/exponential distribution is equal to 1/xv.)
+ * 'hi' must be no more than INT_MAX+1, as for 'rng_range'.
+ * 'xv' must be greater than 0 and less than 'hi'.
  */
 int
 rng_range_geom(unsigned int hi, unsigned int xv)
@@ -202,7 +193,6 @@ rng_range_geom(unsigned int hi, unsigned int xv)
   using std::exp;
   using std::log;
   using std::floor;
-  using std::isnan;
   using std::min;
   using std::max;
 
@@ -210,8 +200,6 @@ rng_range_geom(unsigned int hi, unsigned int xv)
   log_assert(0 < xv && xv < hi);
 
   double U = rng_double();
-  if (isnan(U))
-    return -1;
 
   /* The exponential distribution with expected value
          xe = 1/log(1 + 1/xv)
diff --git a/src/rng.h b/src/rng.h
index 02b0947..5cc0a5a 100644
--- a/src/rng.h
+++ b/src/rng.h
@@ -6,7 +6,7 @@
 #define RNG_H
 
 /** Set b to contain n random bytes. */
-int rng_bytes(uint8_t *b, size_t n);
+void rng_bytes(uint8_t *b, size_t n);
 
 /** Return a random integer in the range [0, max).
  * 'max' must be between 1 and INT_MAX+1, inclusive.
diff --git a/src/test/unittest.cc b/src/test/unittest.cc
index 31d237a..d09c2a6 100644
--- a/src/test/unittest.cc
+++ b/src/test/unittest.cc
@@ -100,8 +100,12 @@ main(int argc, const char **argv)
   char *logminsev;
 
   logminsev = getenv("TT_LOG");
-  if (logminsev)
+  if (logminsev) {
+    log_set_method(LOG_METHOD_STDERR, 0);
     log_set_min_severity(logminsev);
+  } else {
+    log_set_method(LOG_METHOD_NULL, 0);
+  }
 
   /* Ugly method to fix a Windows problem:
      http://archives.seul.org/libevent/users/Oct-2010/msg00049.html */
diff --git a/src/test/unittest_crypt.cc b/src/test/unittest_crypt.cc
index 2e50623..0053646 100644
--- a/src/test/unittest_crypt.cc
+++ b/src/test/unittest_crypt.cc
@@ -968,10 +968,10 @@ test_crypt_rng(void *)
      An entropy test wouldn't really help either.
      I guess I'll just copy Tor's unit test methodology here :3 */
 
-  uint8_t data1[100],data2[100];
+  uint8_t data1[100], data2[100];
 
-  tt_int_op(0, ==, rng_bytes(data1, 100));
-  tt_int_op(0, ==, rng_bytes(data2, 100));
+  rng_bytes(data1, 100);
+  rng_bytes(data2, 100);
 
   tt_mem_op(data1, !=, data2, 100);
 
diff --git a/src/util.h b/src/util.h
index 392d15a..32750c1 100644
--- a/src/util.h
+++ b/src/util.h
@@ -8,6 +8,7 @@
 #include "config.h"
 
 #define __STDC_LIMIT_MACROS
+#define __STDC_CONSTANT_MACROS
 #include <limits.h>
 #include <stdarg.h> /* va_list */
 #include <stddef.h> /* size_t, ptrdiff_t, offsetof, NULL */





More information about the tor-commits mailing list