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

3 files changed, 168 insertions, 119 deletions

diff --git a/src/tests/data/modes/cbc.vec b/src/tests/data/modes/cbc.vec
index 966d23108..05d30ab7e 100644
--- a/src/tests/data/modes/cbc.vec
+++ b/src/tests/data/modes/cbc.vec
@@ -1205,6 +1205,11 @@ Nonce = 000102030405060708090A0B0C0D0E0F
 In = 6BC1BEE22E409F96E93D7E117393172AAE2D8A571E03AC9C9EB76FAC45AF8E5130C81C46A35CE411E5FBC1191A0A52EFF69F2445DF4F9B17AD2B417BE66C3710
 Out = 7649ABAC8119B246CEE98E9B12E9197D5086CB9B507219EE95DB113A917678B273BED6B8E3C1743B7116E69E222295163FF1CAA1681FAC09120ECA307586E1A7
 
+Key = 2B7E151628AED2A6ABF7158809CF4F3C
+Nonce = 00000000000000000000000000000000
+In = 6BC1BEE22E409F96E93D7E117393172AAE2D8A571E03AC9C9EB76FAC45AF8E5130C81C46A35CE411E5FBC1191A0A52EFF69F2445DF4F9B17AD2B417BE66C3710
+Out = 3AD77BB40D7A3660A89ECAF32466EF97B148C17F309EE692287AE57CF12ADD49C93D11BFAF08C5DC4D90B37B4DEE002BA7356E1207BB406639E5E5CEB9A9ED93
+
 [AES-192/CBC/NoPadding]
 Key = 8E73B0F7DA0E6452C810F32B809079E562F8EAD2522C6B7B
 Nonce = 000102030405060708090A0B0C0D0E0F
@@ -1217,13 +1222,6 @@ Nonce = 000102030405060708090A0B0C0D0E0F
 In = 6BC1BEE22E409F96E93D7E117393172AAE2D8A571E03AC9C9EB76FAC45AF8E5130C81C46A35CE411E5FBC1191A0A52EFF69F2445DF4F9B17AD2B417BE66C3710
 Out = F58C4C04D6E5F1BA779EABFB5F7BFBD69CFC4E967EDB808D679F777BC6702C7D39F23369A9D9BACFA530E26304231461B2EB05E2C39BE9FCDA6C19078C6A9D1B
 
-# test empty nonce, must be equivalent to zero
-[AES-128/CBC/NoPadding]
-Key = 2B7E151628AED2A6ABF7158809CF4F3C
-Nonce =
-In = 6BC1BEE22E409F96E93D7E117393172AAE2D8A571E03AC9C9EB76FAC45AF8E5130C81C46A35CE411E5FBC1191A0A52EFF69F2445DF4F9B17AD2B417BE66C3710
-Out = 3AD77BB40D7A3660A89ECAF32466EF97B148C17F309EE692287AE57CF12ADD49C93D11BFAF08C5DC4D90B37B4DEE002BA7356E1207BB406639E5E5CEB9A9ED93
-
 # RFC 3962: Advanced Encryption Standard (AES) Encryption for Kerberos 5
 [AES-128/CBC/CTS]
 Key = 636869636b656e207465726979616b69
diff --git a/src/tests/test_aead.cpp b/src/tests/test_aead.cpp
index cf89a58bd..003f7c886 100644
--- a/src/tests/test_aead.cpp
+++ b/src/tests/test_aead.cpp
@@ -26,6 +26,8 @@ class AEAD_Tests final : public Text_Based_Test
                             const std::vector<uint8_t>& input, const std::vector<uint8_t>& expected,
                             const std::vector<uint8_t>& ad, const std::string& algo)
          {
+         const bool is_siv = algo.find("/SIV") != std::string::npos;
+
          Test::Result result(algo);
 
          std::unique_ptr<Botan::AEAD_Mode> enc(Botan::AEAD_Mode::create(algo, Botan::ENCRYPTION));
@@ -35,8 +37,16 @@ class AEAD_Tests final : public Text_Based_Test
          result.confirm("AEAD name is not empty", !enc->name().empty());
          result.confirm("AEAD default nonce size is accepted", enc->valid_nonce_length(enc->default_nonce_length()));
 
+         Botan::secure_vector<uint8_t> garbage = Test::rng().random_vec(enc->update_granularity());
+
+         if(is_siv == false)
+            {
+            result.test_throws("Unkeyed object throws for encrypt",
+                               [&]() { enc->update(garbage); });
+            }
+
          result.test_throws("Unkeyed object throws for encrypt",
-                            [&]() { Botan::secure_vector<uint8_t> buf; enc->finish(buf); });
+                            [&]() { enc->finish(garbage); });
 
          if(enc->associated_data_requires_key())
             {
@@ -47,9 +57,7 @@ class AEAD_Tests final : public Text_Based_Test
          // Ensure that test resets AD and message state
          enc->set_key(key);
 
-         Botan::secure_vector<uint8_t> garbage = Test::rng().random_vec(enc->update_granularity());
-
-         if(algo.find("/SIV") == std::string::npos)
+         if(is_siv == false)
             {
             result.test_throws("Cannot process data until nonce is set (enc)",
                                [&]() { enc->update(garbage); });
@@ -166,14 +174,24 @@ class AEAD_Tests final : public Text_Based_Test
                             const std::vector<uint8_t>& input, const std::vector<uint8_t>& expected,
                             const std::vector<uint8_t>& ad, const std::string& algo)
          {
+         const bool is_siv = algo.find("/SIV") != std::string::npos;
+
          Test::Result result(algo);
 
          std::unique_ptr<Botan::AEAD_Mode> dec(Botan::AEAD_Mode::create(algo, Botan::DECRYPTION));
 
          result.test_eq("AEAD decrypt output_length is correct", dec->output_length(input.size()), expected.size());
 
+         Botan::secure_vector<uint8_t> garbage = Test::rng().random_vec(dec->update_granularity());
+
+         if(is_siv == false)
+            {
+            result.test_throws("Unkeyed object throws for decrypt",
+                               [&]() { dec->update(garbage); });
+            }
+
          result.test_throws("Unkeyed object throws for decrypt",
-                            [&]() { Botan::secure_vector<uint8_t> buf; dec->finish(buf); });
+                            [&]() { dec->finish(garbage); });
 
          if(dec->associated_data_requires_key())
             {
@@ -186,9 +204,7 @@ class AEAD_Tests final : public Text_Based_Test
          dec->set_key(key);
          dec->set_ad(mutate_vec(ad));
 
-         Botan::secure_vector<uint8_t> garbage = Test::rng().random_vec(dec->update_granularity());
-
-         if(algo.find("/SIV") == std::string::npos)
+         if(is_siv == false)
             {
             result.test_throws("Cannot process data until nonce is set (dec)",
                                [&]() { dec->update(garbage); });
diff --git a/src/tests/test_modes.cpp b/src/tests/test_modes.cpp
index dbfa3d2bf..bb564ee9d 100644
--- a/src/tests/test_modes.cpp
+++ b/src/tests/test_modes.cpp
@@ -1,6 +1,7 @@
 /*
 * (C) 2014,2015,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)
 */
@@ -52,152 +53,186 @@ class Cipher_Mode_Tests final : public Text_Based_Test
 
             if(!enc || !dec)
                {
+               if(enc)
+                  result.test_failure("Provider " + provider_ask + " has encrypt but not decrypt");
+               if(dec)
+                  result.test_failure("Provider " + provider_ask + " has decrypt but not encrypt");
                result.note_missing(algo);
                return result;
                }
 
-            result.test_is_nonempty("provider", enc->provider());
-            result.test_eq("name", enc->name(), algo);
+            result.test_eq("enc and dec granularity is the same",
+                           enc->update_granularity(), dec->update_granularity());
 
-            result.test_eq("mode not authenticated", enc->authenticated(), false);
+            test_mode(result, algo, provider_ask, "encryption", *enc, key, nonce, input, expected);
+            test_mode(result, algo, provider_ask, "decryption", *dec, key, nonce, expected, input);
+            }
+
+         return result;
+         }
+
+   private:
+      void test_mode(Test::Result& result,
+                     const std::string& algo,
+                     const std::string& provider,
+                     const std::string& direction,
+                     Botan::Cipher_Mode& mode,
+                     const std::vector<uint8_t>& key,
+                     const std::vector<uint8_t>& nonce,
+                     const std::vector<uint8_t>& input,
+                     const std::vector<uint8_t>& expected)
+         {
+         const bool is_cbc = (algo.find("/CBC") != std::string::npos);
+         const bool is_ctr = (algo.find("CTR") != std::string::npos);
+
+         result.test_eq("name", mode.name(), algo);
 
-            result.test_throws("Unkeyed object throws for encrypt",
-                               [&]() { Botan::secure_vector<uint8_t> bad(16); enc->finish(bad); });
-            result.test_throws("Unkeyed object throws for decrypt",
-                               [&]() { Botan::secure_vector<uint8_t> bad(16); dec->finish(bad); });
+         // Some modes report base even if got from another provider
+         if(mode.provider() != "base")
+            {
+            result.test_eq("provider", mode.provider(), provider);
+            }
 
-            if(algo.find("/CTR") == std::string::npos)
+         result.test_eq("mode not authenticated", mode.authenticated(), false);
+
+         const size_t update_granularity = mode.update_granularity();
+         const size_t min_final_bytes = mode.minimum_final_size();
+
+         // FFI currently requires this, so assure it is true for all modes
+         result.test_gte("buffer sizes ok", update_granularity, min_final_bytes);
+
+         result.test_throws("Unkeyed object throws", [&]() {
+            Botan::secure_vector<uint8_t> bad(update_granularity);
+            mode.finish(bad);
+            });
+
+         if(is_cbc)
+            {
+            // can't test equal due to CBC padding
+
+            if(direction == "encryption")
                {
-               // can't test equal due to CBC padding
-               result.test_lte("output_length", enc->output_length(input.size()), expected.size());
-               result.test_gte("output_length", dec->output_length(expected.size()), input.size());
+               result.test_lte("output_length", mode.output_length(input.size()), expected.size());
                }
             else
                {
-               // assume all other modes are not expanding (currently true)
-               result.test_eq("output_length", enc->output_length(input.size()), expected.size());
-               result.test_eq("output_length", dec->output_length(expected.size()), input.size());
+               result.test_gte("output_length", mode.output_length(input.size()), expected.size());
                }
+            }
+         else
+            {
+            // assume all other modes are not expanding (currently true)
+            result.test_eq("output_length", mode.output_length(input.size()), expected.size());
+            }
 
-            // FFI currently requires this, so assure it is true for all modes
-            result.test_gte("enc buffer sizes ok", enc->update_granularity(), enc->minimum_final_size());
-            result.test_gte("dec buffer sizes ok", dec->update_granularity(), dec->minimum_final_size());
+         result.confirm("default nonce size is allowed",
+                        mode.valid_nonce_length(mode.default_nonce_length()));
 
-            result.confirm("default nonce size is allowed",
-                           enc->valid_nonce_length(enc->default_nonce_length()));
-            result.confirm("default nonce size is allowed",
-                           dec->valid_nonce_length(dec->default_nonce_length()));
+         // Test that disallowed nonce sizes result in an exception
+         const size_t large_nonce_size = 65000;
+         result.test_eq("Large nonce not allowed", mode.valid_nonce_length(large_nonce_size), false);
+         result.test_throws("Large nonce causes exception",
+                            [&mode,large_nonce_size]() { mode.start(nullptr, large_nonce_size); });
 
-            // Test that disallowed nonce sizes result in an exception
-            const size_t large_nonce_size = 65000;
-            result.test_eq("Large nonce not allowed", enc->valid_nonce_length(large_nonce_size), false);
-            result.test_throws("Large nonce causes exception",
-                               [&enc,large_nonce_size]() { enc->start(nullptr, large_nonce_size); });
+         Botan::secure_vector<uint8_t> garbage = Test::rng().random_vec(update_granularity);
 
-            // Test to make sure reset() resets what we need it to
-            enc->set_key(mutate_vec(key));
-            Botan::secure_vector<uint8_t> garbage = Test::rng().random_vec(enc->update_granularity());
+         // Test to make sure reset() resets what we need it to
+         result.test_throws("Cannot process data (update) until key is set",
+                            [&]() { mode.update(garbage); });
+         result.test_throws("Cannot process data (finish) until key is set",
+                            [&]() { mode.finish(garbage); });
 
-            if(algo.find("CTR") == std::string::npos)
-               {
-               result.test_throws("Cannot process data until nonce is set (enc)",
-                                  [&]() { enc->update(garbage); });
-               }
+         mode.set_key(mutate_vec(key));
 
-            enc->start(mutate_vec(nonce));
-            enc->update(garbage);
+         if(is_ctr == false)
+            {
+            result.test_throws("Cannot process data until nonce is set",
+                               [&]() { mode.update(garbage); });
+            }
 
-            enc->reset();
+         mode.start(mutate_vec(nonce));
+         mode.reset();
 
-            enc->set_key(key);
-            enc->start(nonce);
+         if(is_ctr == false)
+            {
+            result.test_throws("Cannot process data until nonce is set (after start/reset)",
+                               [&]() { mode.update(garbage); });
+            }
 
-            Botan::secure_vector<uint8_t> buf(input.begin(), input.end());
-            // TODO: should first update if possible
-            enc->finish(buf);
-            result.test_eq("encrypt", buf, expected);
+         mode.start(mutate_vec(nonce));
+         mode.update(garbage);
 
-            // additionally test process() if possible
-            size_t update_granularity = enc->update_granularity();
-            size_t input_length = input.size();
-            size_t min_final_bytes = enc->minimum_final_size();
-            if(input_length > (update_granularity + min_final_bytes))
-               {
-               // reset state first
-               enc->reset();
+         mode.reset();
 
-               enc->start(nonce);
-               buf.assign(input.begin(), input.end());
+         mode.set_key(key);
+         mode.start(nonce);
 
-               // we can process at max input_length
-               const size_t max_blocks_to_process = (input_length - min_final_bytes) / update_granularity;
-               const size_t bytes_to_process = max_blocks_to_process * update_granularity;
+         Botan::secure_vector<uint8_t> buf;
 
-               const size_t bytes_written = enc->process(buf.data(), bytes_to_process);
+         buf.assign(input.begin(), input.end());
+         mode.finish(buf);
+         result.test_eq(direction + " all-in-one", buf, expected);
 
-               result.test_eq("correct number of bytes processed", bytes_written, bytes_to_process);
+         // additionally test update() and process() if possible
+         if(input.size() >= update_granularity + min_final_bytes)
+            {
+            const size_t max_blocks_to_process = (input.size() - min_final_bytes) / update_granularity;
+            const size_t bytes_to_process = max_blocks_to_process * update_granularity;
 
-               enc->finish(buf, bytes_to_process);
-               result.test_eq("encrypt", buf, expected);
-               }
+            // test update, 1 block at a time
+            if(max_blocks_to_process > 1)
+               {
+               Botan::secure_vector<uint8_t> block(update_granularity);
+               buf.clear();
 
-            // decryption
-            buf.assign(expected.begin(), expected.end());
+               mode.start(nonce);
+               for(size_t i = 0; i != max_blocks_to_process; ++i)
+                  {
+                  block.assign(input.data() + i*update_granularity,
+                               input.data() + (i+1)*update_granularity);
 
-            // Test to make sure reset() resets what we need it to
-            dec->set_key(mutate_vec(key));
-            garbage = Test::rng().random_vec(dec->update_granularity());
+                  mode.update(block);
+                  buf += block;
+                  }
 
-            if(algo.find("CTR") == std::string::npos)
-               {
-               result.test_throws("Cannot process data until nonce is set (dec)",
-                                  [&]() { dec->update(garbage); });
-               }
+               Botan::secure_vector<uint8_t> last_bits(input.data() + bytes_to_process, input.data() + input.size());
+               mode.finish(last_bits);
+               buf += last_bits;
 
-            dec->start(mutate_vec(nonce));
-            dec->update(garbage);
+               result.test_eq(direction + " update-1", buf, expected);
+               }
 
-            dec->reset();
+            // test update with maximum length input
+            buf.assign(input.data(), input.data() + bytes_to_process);
+            Botan::secure_vector<uint8_t> last_bits(input.data() + bytes_to_process, input.data() + input.size());
 
-            dec->set_key(key);
-            dec->start(nonce);
-            dec->finish(buf);
-            result.test_eq("decrypt", buf, input);
+            mode.start(nonce);
+            mode.update(buf);
+            mode.finish(last_bits);
 
-            // additionally test process() if possible
-            update_granularity = dec->update_granularity();
-            input_length = expected.size();
-            min_final_bytes = dec->minimum_final_size();
-            if(input_length > (update_granularity + min_final_bytes))
-               {
-               // reset state first
-               dec->reset();
+            buf += last_bits;
 
-               dec->start(nonce);
-               buf.assign(expected.begin(), expected.end());
+            result.test_eq(direction + " update-all", buf, expected);
 
-               // we can process at max input_length
-               const size_t max_blocks_to_process = (input_length - min_final_bytes) / update_granularity;
-               const size_t bytes_to_process = max_blocks_to_process * update_granularity;
+            // test process with maximum length input
+            mode.start(nonce);
+            buf.assign(input.begin(), input.end());
 
-               const size_t bytes_written = dec->process(buf.data(), bytes_to_process);
+            const size_t bytes_written = mode.process(buf.data(), bytes_to_process);
 
-               result.test_eq("correct number of bytes processed", bytes_written, bytes_to_process);
+            result.test_eq("correct number of bytes processed", bytes_written, bytes_to_process);
 
-               dec->finish(buf, bytes_to_process);
-               result.test_eq("decrypt", buf, input);
-               }
+            mode.finish(buf, bytes_to_process);
+            result.test_eq(direction + " process", buf, expected);
+            }
 
-            enc->clear();
-            dec->clear();
+         mode.clear();
 
-            result.test_throws("Unkeyed object throws for encrypt after clear",
-                               [&]() { Botan::secure_vector<uint8_t> bad(16); enc->finish(bad); });
-            result.test_throws("Unkeyed object throws for decrypt after clear",
-                               [&]() { Botan::secure_vector<uint8_t> bad(16); dec->finish(bad); });
-            }
+         result.test_throws("Unkeyed object throws after clear", [&]() {
+            Botan::secure_vector<uint8_t> bad(update_granularity);
+            mode.finish(bad);
+            });
 
-         return result;
          }
    };