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

4 files changed, 30 insertions, 7 deletions

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;
    }
 
 }