From bdc32a98b97c97145054edfa217569351ff41baa Mon Sep 17 00:00:00 2001 From: Jack Lloyd Date: Sat, 29 Sep 2018 06:39:03 -0400 Subject: Refactor mode tests, and correct bugs found Several problems in CBC found by adding tests - If you set a key, then set a nonce, then set a new key, you could encrypt without setting a new nonce. - It was possible to call CBC finish without setting a nonce, which would crash. - If you had an CBC decryption object, set a key, set a nonce, then reset message state, it should throw because no nonce is set. Instead it would carry on using an all-zero nonce. Disable CommonCrypto with PKCS7 padding as it seems to have some problem that I cannot figure out from the build logs. This work sponsored by Ribose Inc --- src/lib/ffi/ffi_cipher.cpp | 1 + src/lib/modes/cbc/cbc.cpp | 8 +++++++- src/lib/prov/commoncrypto/commoncrypto_block.cpp | 2 +- src/lib/prov/commoncrypto/commoncrypto_mode.cpp | 12 ++++++++++-- src/lib/prov/commoncrypto/commoncrypto_utils.cpp | 10 ++++++---- src/lib/prov/openssl/openssl_mode.cpp | 13 +++++++++++++ 6 files changed, 38 insertions(+), 8 deletions(-) (limited to 'src/lib') diff --git a/src/lib/ffi/ffi_cipher.cpp b/src/lib/ffi/ffi_cipher.cpp index ec6893405..7b672d407 100644 --- a/src/lib/ffi/ffi_cipher.cpp +++ b/src/lib/ffi/ffi_cipher.cpp @@ -167,6 +167,7 @@ int botan_cipher_update(botan_cipher_t cipher_obj, while(input_size >= ud && output_size >= ud) { + // FIXME we can use process here and avoid the copy copy_mem(mbuf.data(), input, ud); cipher.update(mbuf); diff --git a/src/lib/modes/cbc/cbc.cpp b/src/lib/modes/cbc/cbc.cpp index 76b78e4f6..c01fc4328 100644 --- a/src/lib/modes/cbc/cbc.cpp +++ b/src/lib/modes/cbc/cbc.cpp @@ -2,6 +2,7 @@ * CBC Mode * (C) 1999-2007,2013,2017 Jack Lloyd * (C) 2016 Daniel Neus, Rohde & Schwarz Cybersecurity +* (C) 2018 Ribose Inc * * Botan is released under the Simplified BSD License (see license.txt) */ @@ -65,6 +66,7 @@ bool CBC_Mode::valid_nonce_length(size_t n) const void CBC_Mode::key_schedule(const uint8_t key[], size_t length) { m_cipher->set_key(key, length); + m_state.clear(); } void CBC_Mode::start_msg(const uint8_t nonce[], size_t nonce_len) @@ -124,6 +126,7 @@ size_t CBC_Encryption::process(uint8_t buf[], size_t sz) void CBC_Encryption::finish(secure_vector& buffer, size_t offset) { + BOTAN_STATE_CHECK(state().empty() == false); BOTAN_ASSERT(buffer.size() >= offset, "Offset is sane"); const size_t BS = block_size(); @@ -155,6 +158,7 @@ size_t CTS_Encryption::output_length(size_t input_length) const void CTS_Encryption::finish(secure_vector& buffer, size_t offset) { + BOTAN_STATE_CHECK(state().empty() == false); BOTAN_ASSERT(buffer.size() >= offset, "Offset is sane"); uint8_t* buf = buffer.data() + offset; const size_t sz = buffer.size() - offset; @@ -237,6 +241,7 @@ size_t CBC_Decryption::process(uint8_t buf[], size_t sz) void CBC_Decryption::finish(secure_vector& buffer, size_t offset) { + BOTAN_STATE_CHECK(state().empty() == false); BOTAN_ASSERT(buffer.size() >= offset, "Offset is sane"); const size_t sz = buffer.size() - offset; @@ -257,7 +262,7 @@ void CBC_Decryption::finish(secure_vector& buffer, size_t offset) void CBC_Decryption::reset() { - zeroise(state()); + CBC_Mode::reset(); zeroise(m_tempbuf); } @@ -273,6 +278,7 @@ size_t CTS_Decryption::minimum_final_size() const void CTS_Decryption::finish(secure_vector& buffer, size_t offset) { + BOTAN_STATE_CHECK(state().empty() == false); BOTAN_ASSERT(buffer.size() >= offset, "Offset is sane"); const size_t sz = buffer.size() - offset; uint8_t* buf = buffer.data() + offset; diff --git a/src/lib/prov/commoncrypto/commoncrypto_block.cpp b/src/lib/prov/commoncrypto/commoncrypto_block.cpp index e912ed57e..b3680506b 100644 --- a/src/lib/prov/commoncrypto/commoncrypto_block.cpp +++ b/src/lib/prov/commoncrypto/commoncrypto_block.cpp @@ -143,7 +143,7 @@ make_commoncrypto_block_cipher(const std::string& name) CommonCryptor_Opts opts = commoncrypto_opts_from_algo(name); return std::unique_ptr(new CommonCrypto_BlockCipher(name, opts)); } - catch(CommonCrypto_Error e) + catch(CommonCrypto_Error& e) { return nullptr; } diff --git a/src/lib/prov/commoncrypto/commoncrypto_mode.cpp b/src/lib/prov/commoncrypto/commoncrypto_mode.cpp index baae11679..a3a27637a 100644 --- a/src/lib/prov/commoncrypto/commoncrypto_mode.cpp +++ b/src/lib/prov/commoncrypto/commoncrypto_mode.cpp @@ -120,6 +120,7 @@ void CommonCrypto_Cipher_Mode::finish(secure_vector& buffer, size_t offset) { verify_key_set(m_key_set); + BOTAN_STATE_CHECK(m_nonce_set); BOTAN_ASSERT(buffer.size() >= offset, "Offset ok"); uint8_t* buf = buffer.data() + offset; @@ -153,7 +154,10 @@ size_t CommonCrypto_Cipher_Mode::update_granularity() const size_t CommonCrypto_Cipher_Mode::minimum_final_size() const { - return 0; + if(m_direction == ENCRYPTION) + return 0; + else + return m_opts.block_size; } size_t CommonCrypto_Cipher_Mode::default_nonce_length() const @@ -196,6 +200,9 @@ void CommonCrypto_Cipher_Mode::reset() { return; } + + m_nonce_set = false; + CCCryptorStatus status = CCCryptorReset(m_cipher, nullptr); if(status != kCCSuccess) { @@ -220,6 +227,7 @@ void CommonCrypto_Cipher_Mode::key_schedule(const uint8_t key[], size_t length) } m_key_set = true; + m_nonce_set = false; } } @@ -232,7 +240,7 @@ make_commoncrypto_cipher_mode(const std::string& name, Cipher_Dir direction) CommonCryptor_Opts opts = commoncrypto_opts_from_algo(name); return new CommonCrypto_Cipher_Mode(name, direction, opts); } - catch(CommonCrypto_Error e) + catch(CommonCrypto_Error& e) { return nullptr; } diff --git a/src/lib/prov/commoncrypto/commoncrypto_utils.cpp b/src/lib/prov/commoncrypto/commoncrypto_utils.cpp index 62736b88c..9ec9c30ee 100644 --- a/src/lib/prov/commoncrypto/commoncrypto_utils.cpp +++ b/src/lib/prov/commoncrypto/commoncrypto_utils.cpp @@ -133,14 +133,16 @@ CommonCryptor_Opts commoncrypto_opts_from_algo(const std::string& algo) throw CommonCrypto_Error("Unsupported cipher mode!"); } - if(cipher_mode_padding.empty() || cipher_mode_padding == "PKCS7") + if(cipher_mode_padding == "NoPadding") { - opts.padding = ccPKCS7Padding; + opts.padding = ccNoPadding; } - else if(cipher_mode_padding == "NoPadding") + /* + else if(cipher_mode_padding.empty() || cipher_mode_padding == "PKCS7") { - opts.padding = ccNoPadding; + opts.padding = ccPKCS7Padding; } + */ else { throw CommonCrypto_Error("Unsupported cipher mode padding!"); diff --git a/src/lib/prov/openssl/openssl_mode.cpp b/src/lib/prov/openssl/openssl_mode.cpp index ce98e0c62..5636d008f 100644 --- a/src/lib/prov/openssl/openssl_mode.cpp +++ b/src/lib/prov/openssl/openssl_mode.cpp @@ -86,11 +86,20 @@ void OpenSSL_Cipher_Mode::start_msg(const uint8_t nonce[], size_t nonce_len) if(!valid_nonce_length(nonce_len)) throw Invalid_IV_Length(name(), nonce_len); + if(nonce_len) { if(!EVP_CipherInit_ex(m_cipher, nullptr, nullptr, nullptr, nonce, -1)) throw OpenSSL_Error("EVP_CipherInit_ex nonce"); } + else if(m_nonce_set == false) + { + const std::vector zeros(m_block_size); + if(!EVP_CipherInit_ex(m_cipher, nullptr, nullptr, nullptr, zeros.data(), -1)) + throw OpenSSL_Error("EVP_CipherInit_ex nonce"); + } + // otherwise existing CBC state left unchanged + m_nonce_set = true; } @@ -116,6 +125,7 @@ void OpenSSL_Cipher_Mode::finish(secure_vector& buffer, size_t offset) { verify_key_set(m_key_set); + BOTAN_STATE_CHECK(m_nonce_set); BOTAN_ASSERT(buffer.size() >= offset, "Offset ok"); uint8_t* buf = buffer.data() + offset; @@ -163,6 +173,7 @@ size_t OpenSSL_Cipher_Mode::output_length(size_t input_length) const void OpenSSL_Cipher_Mode::clear() { m_key_set = false; + m_nonce_set = false; const EVP_CIPHER* algo = EVP_CIPHER_CTX_cipher(m_cipher); @@ -180,6 +191,7 @@ void OpenSSL_Cipher_Mode::reset() { if(!EVP_CipherInit_ex(m_cipher, nullptr, nullptr, nullptr, nullptr, -1)) throw OpenSSL_Error("EVP_CipherInit_ex clear"); + m_nonce_set = false; } Key_Length_Specification OpenSSL_Cipher_Mode::key_spec() const @@ -194,6 +206,7 @@ void OpenSSL_Cipher_Mode::key_schedule(const uint8_t key[], size_t length) if(!EVP_CipherInit_ex(m_cipher, nullptr, nullptr, key, nullptr, -1)) throw OpenSSL_Error("EVP_CipherInit_ex key"); m_key_set = true; + m_nonce_set = false; } } -- cgit v1.2.3