commit 3ae30293e91c5b77dd29306045468682d4e3656e Author: Zack Weinberg <zackw@panix.com> Date: Sat Mar 24 21:16:16 2012 -0700 Crypto tweaks post-new chopper. * It is now possible to create an encryption or decryption object directly from a key generator. * The dangerous, no longer required decrypt_unchecked() method has been removed. --- src/crypt.cc | 86 +++++++++++++++++++++++++++++++++++-------- src/crypt.h | 30 +++++++++++---- src/protocol/chop.cc | 33 ++++------------- src/test/unittest_crypt.cc | 5 --- 4 files changed, 100 insertions(+), 54 deletions(-) diff --git a/src/crypt.cc b/src/crypt.cc index e81e8d4..f0f9c4f 100644 --- a/src/crypt.cc +++ b/src/crypt.cc @@ -14,6 +14,7 @@ #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 @@ -29,6 +30,9 @@ public: #include <cryptopp/pwdbased.h> +// 32 bytes -> no extra memory allocation even for big keys, hopefully +typedef CryptoPP::SecBlockWithHint<byte, 32> KeyBlock; + /* 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. */ @@ -72,6 +76,21 @@ ecb_encryptor::create(const uint8_t *key, size_t keylen) CATCH_ALL_EXCEPTIONS(0); } +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); +} + ecb_decryptor * ecb_decryptor::create(const uint8_t *key, size_t keylen) { @@ -83,6 +102,21 @@ ecb_decryptor::create(const uint8_t *key, size_t keylen) CATCH_ALL_EXCEPTIONS(0); } +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); +} + ecb_encryptor::~ecb_encryptor() {} ecb_encryptor_impl::~ecb_encryptor_impl() {} ecb_decryptor::~ecb_decryptor() {} @@ -121,9 +155,6 @@ namespace { virtual ~gcm_decryptor_impl(); virtual int decrypt(uint8_t *out, const uint8_t *in, size_t inlen, const uint8_t *nonce, size_t nlen); - virtual void decrypt_unchecked(uint8_t *out, - const uint8_t *in, size_t inlen, - const uint8_t *nonce, size_t nlen); }; } @@ -141,6 +172,24 @@ gcm_encryptor::create(const uint8_t *key, size_t keylen) CATCH_ALL_EXCEPTIONS(0); } +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); +} + gcm_decryptor * gcm_decryptor::create(const uint8_t *key, size_t keylen) { @@ -155,6 +204,24 @@ gcm_decryptor::create(const uint8_t *key, size_t keylen) CATCH_ALL_EXCEPTIONS(0); } +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); +} + gcm_encryptor::~gcm_encryptor() {} gcm_encryptor_impl::~gcm_encryptor_impl() {} gcm_decryptor::~gcm_decryptor() {} @@ -184,19 +251,6 @@ gcm_decryptor_impl::decrypt(uint8_t *out, const uint8_t *in, size_t inlen, CATCH_ALL_EXCEPTIONS(-1); } -void -gcm_decryptor_impl::decrypt_unchecked(uint8_t *out, - const uint8_t *in, size_t inlen, - const uint8_t *nonce, size_t nlen) -{ - try { - // there is no convenience function for this - this->ctx.Resynchronize(nonce, nlen); - this->ctx.ProcessData(out, in, inlen); - } - CATCH_ALL_EXCEPTIONS(); -} - typedef CryptoPP::HMAC<CryptoPP::SHA256> HMAC_SHA256; typedef CryptoPP::PKCS5_PBKDF2_HMAC<CryptoPP::SHA256> PBKDF2_SHA256; const size_t SHA256_LEN = CryptoPP::SHA256::DIGESTSIZE; diff --git a/src/crypt.h b/src/crypt.h index c0084e9..4136a35 100644 --- a/src/crypt.h +++ b/src/crypt.h @@ -8,6 +8,8 @@ const size_t AES_BLOCK_LEN = 16; const size_t GCM_TAG_LEN = 16; +struct key_generator; + struct ecb_encryptor { ecb_encryptor() {} @@ -17,6 +19,11 @@ struct ecb_encryptor as the symmetric key. 'keylen' must be 16, 24, or 32 bytes. */ static ecb_encryptor *create(const uint8_t *key, size_t keylen); + /** Return a new AES/ECB encryption state, generating a key of + length 'keylen' from the key generator 'gen'. 'keylen' must be + 16, 24, or 32 bytes. */ + static ecb_encryptor *create(key_generator *gen, size_t keylen); + /** Encrypt exactly AES_BLOCK_LEN bytes of data in the buffer 'in' and write the result to 'out'. */ virtual void encrypt(uint8_t *out, const uint8_t *in) = 0; @@ -35,6 +42,11 @@ struct ecb_decryptor as the symmetric key. 'keylen' must be 16, 24, or 32 bytes. */ static ecb_decryptor *create(const uint8_t *key, size_t keylen); + /** Return a new AES/ECB decryption state, generating a key of + length 'keylen' from the key generator 'gen'. 'keylen' must be + 16, 24, or 32 bytes. */ + static ecb_decryptor *create(key_generator *gen, size_t keylen); + /** Decrypt exactly AES_BLOCK_LEN bytes of data in the buffer 'in' and write the result to 'out'. */ virtual void decrypt(uint8_t *out, const uint8_t *in) = 0; @@ -54,6 +66,11 @@ struct gcm_encryptor as the symmetric key. 'keylen' must be 16, 24, or 32 bytes. */ static gcm_encryptor *create(const uint8_t *key, size_t keylen); + /** Return a new AES/GCM encryption state, generating a key of + length 'keylen' from the key generator 'gen'. 'keylen' must be + 16, 24, or 32 bytes. */ + static gcm_encryptor *create(key_generator *gen, size_t keylen); + /** Encrypt 'inlen' bytes of data in the buffer 'in', writing the result plus an authentication tag to the buffer 'out', whose length must be at least 'inlen'+16 bytes. Use 'nonce' @@ -76,6 +93,11 @@ struct gcm_decryptor as the symmetric key. 'keylen' must be 16, 24, or 32 bytes. */ static gcm_decryptor *create(const uint8_t *key, size_t keylen); + /** Return a new AES/GCM decryption state, generating a key of + length 'keylen' from the key generator 'gen'. 'keylen' must be + 16, 24, or 32 bytes. */ + static gcm_decryptor *create(key_generator *gen, size_t keylen); + /** Decrypt 'inlen' bytes of data in the buffer 'in'; the last 16 bytes of this buffer are assumed to be the authentication tag. Write the result to the buffer 'out', whose length must be at @@ -85,14 +107,6 @@ struct gcm_decryptor virtual int decrypt(uint8_t *out, const uint8_t *in, size_t inlen, const uint8_t *nonce, size_t nlen) = 0; - /** Decrypt 'inlen' bytes of data in the buffer 'in' WITHOUT - CHECKING THE AUTHENTICATION TAG. Arguments same as decrypt(). - This should be used only to decode just enough of an incoming - block to know how long it's going to be and therefore where the - tag begins. */ - virtual void decrypt_unchecked(uint8_t *out, const uint8_t *in, size_t inlen, - const uint8_t *nonce, size_t nlen) = 0; - private: gcm_decryptor(const gcm_decryptor&) DELETE_METHOD; gcm_decryptor& operator=(const gcm_decryptor&) DELETE_METHOD; diff --git a/src/protocol/chop.cc b/src/protocol/chop.cc index e81214a..44e79a3 100644 --- a/src/protocol/chop.cc +++ b/src/protocol/chop.cc @@ -512,32 +512,17 @@ chop_config_t::circuit_create(size_t) key_generator::from_passphrase((const uint8_t *)passphrase, sizeof(passphrase) - 1, 0, 0, 0, 0); - uint8_t kbuf[16]; if (mode == LSN_SIMPLE_SERVER) { - log_assert(kgen->generate(kbuf, 16) == 16); - ckt->send_crypt = gcm_encryptor::create(kbuf, 16); - - log_assert(kgen->generate(kbuf, 16) == 16); - ckt->send_hdr_crypt = ecb_encryptor::create(kbuf, 16); - - log_assert(kgen->generate(kbuf, 16) == 16); - ckt->recv_crypt = gcm_decryptor::create(kbuf, 16); - - log_assert(kgen->generate(kbuf, 16) == 16); - ckt->recv_hdr_crypt = ecb_decryptor::create(kbuf, 16); + ckt->send_crypt = gcm_encryptor::create(kgen, 16); + ckt->send_hdr_crypt = ecb_encryptor::create(kgen, 16); + ckt->recv_crypt = gcm_decryptor::create(kgen, 16); + ckt->recv_hdr_crypt = ecb_decryptor::create(kgen, 16); } else { - log_assert(kgen->generate(kbuf, 16) == 16); - ckt->recv_crypt = gcm_decryptor::create(kbuf, 16); - - log_assert(kgen->generate(kbuf, 16) == 16); - ckt->recv_hdr_crypt = ecb_decryptor::create(kbuf, 16); - - log_assert(kgen->generate(kbuf, 16) == 16); - ckt->send_crypt = gcm_encryptor::create(kbuf, 16); - - log_assert(kgen->generate(kbuf, 16) == 16); - ckt->send_hdr_crypt = ecb_encryptor::create(kbuf, 16); + ckt->recv_crypt = gcm_decryptor::create(kgen, 16); + ckt->recv_hdr_crypt = ecb_decryptor::create(kgen, 16); + ckt->send_crypt = gcm_encryptor::create(kgen, 16); + ckt->send_hdr_crypt = ecb_encryptor::create(kgen, 16); std::pair<chop_circuit_table::iterator, bool> out; do { @@ -551,7 +536,6 @@ chop_config_t::circuit_create(size_t) out.first->second = ckt; } - memset(kbuf, 0, 16); delete kgen; return ckt; } @@ -1027,7 +1011,6 @@ chop_circuit_t::check_for_eof() conn_send_eof(conn); } } - // If we're the client we have to keep trying to talk as long as we // haven't both sent and received a FIN, or we might deadlock. diff --git a/src/test/unittest_crypt.cc b/src/test/unittest_crypt.cc index b903c11..2e50623 100644 --- a/src/test/unittest_crypt.cc +++ b/src/test/unittest_crypt.cc @@ -762,11 +762,6 @@ test_crypt_aesgcm_good_dec(void *) tt_int_op(rv, ==, 0); tt_mem_op(obuf, ==, testvecs[i].pt, testvecs[i].len); - c->decrypt_unchecked(obuf, - (const uint8_t *)testvecs[i].ct, testvecs[i].len, - (const uint8_t *)testvecs[i].iv, 16); - tt_mem_op(obuf, ==, testvecs[i].pt, testvecs[i].len); - delete c; } c = 0;