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

6 files changed, 38 insertions, 8 deletions

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<uint8_t>& 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<uint8_t>& 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<uint8_t>& 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<uint8_t>& 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<uint8_t>& 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<BlockCipher>(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<uint8_t>& 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<uint8_t> 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<uint8_t>& 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;
    }
 
 }