Fix off by one when decoding TLS-CBC ciphertexts

6 files changed, 196 insertions, 28 deletions

diff --git a/src/cli/timing_tests.cpp b/src/cli/timing_tests.cpp
index 31d588011..5c0c504df 100644
--- a/src/cli/timing_tests.cpp
+++ b/src/cli/timing_tests.cpp
@@ -200,7 +200,9 @@ class Lucky13_Timing_Test final : public Timing_Test
       Lucky13_Timing_Test(const std::string& mac_name, size_t mac_keylen)
          : m_mac_algo(mac_name)
          , m_mac_keylen(mac_keylen)
-         , m_dec("AES-128", 16, m_mac_algo, m_mac_keylen, true, false) {}
+         , m_dec(Botan::BlockCipher::create_or_throw("AES-128"),
+                 Botan::MessageAuthenticationCode::create_or_throw("HMAC(" + m_mac_algo + ")"),
+                 16, m_mac_keylen, true, false) {}
 
       std::vector<uint8_t> prepare_input(std::string input) override;
       ticks measure_critical_function(std::vector<uint8_t> input) override;
diff --git a/src/lib/tls/tls_cbc/tls_cbc.cpp b/src/lib/tls/tls_cbc/tls_cbc.cpp
index ca80a3d3c..a745a548b 100644
--- a/src/lib/tls/tls_cbc/tls_cbc.cpp
+++ b/src/lib/tls/tls_cbc/tls_cbc.cpp
@@ -24,26 +24,25 @@ namespace TLS {
 * TLS_CBC_HMAC_AEAD_Mode Constructor
 */
 TLS_CBC_HMAC_AEAD_Mode::TLS_CBC_HMAC_AEAD_Mode(Cipher_Dir dir,
-                                               const std::string& cipher_name,
+                                               std::unique_ptr<BlockCipher> cipher,
+                                               std::unique_ptr<MessageAuthenticationCode> mac,
                                                size_t cipher_keylen,
-                                               const std::string& mac_name,
                                                size_t mac_keylen,
                                                bool use_explicit_iv,
                                                bool use_encrypt_then_mac) :
-   m_cipher_name(cipher_name),
-   m_mac_name(mac_name),
+   m_cipher_name(cipher->name()),
+   m_mac_name(mac->name()),
    m_cipher_keylen(cipher_keylen),
    m_mac_keylen(mac_keylen),
    m_use_encrypt_then_mac(use_encrypt_then_mac)
    {
-   m_mac = MessageAuthenticationCode::create_or_throw("HMAC(" + m_mac_name + ")");
-   std::unique_ptr<BlockCipher> cipher = BlockCipher::create_or_throw(m_cipher_name);
-
-   m_tag_size = m_mac->output_length();
+   m_tag_size = mac->output_length();
    m_block_size = cipher->block_size();
 
    m_iv_size = use_explicit_iv ? m_block_size : 0;
 
+   m_mac = std::move(mac);
+
    if(dir == ENCRYPTION)
       m_cbc.reset(new CBC_Encryption(cipher.release(), new Null_Padding));
    else
@@ -419,7 +418,7 @@ void TLS_CBC_HMAC_AEAD_Decryption::finish(secure_vector<uint8_t>& buffer, size_t
       (sending empty records, instead of 1/(n-1) splitting)
       */
 
-      const uint16_t size_ok_mask = CT::is_lte<uint16_t>(static_cast<uint16_t>(tag_size() + pad_size), static_cast<uint16_t>(record_len + 1));
+      const uint16_t size_ok_mask = CT::is_lte<uint16_t>(static_cast<uint16_t>(tag_size() + pad_size), static_cast<uint16_t>(record_len));
       pad_size &= size_ok_mask;
 
       CT::unpoison(record_contents, record_len);
diff --git a/src/lib/tls/tls_cbc/tls_cbc.h b/src/lib/tls/tls_cbc/tls_cbc.h
index 012b9e51f..c8a808156 100644
--- a/src/lib/tls/tls_cbc/tls_cbc.h
+++ b/src/lib/tls/tls_cbc/tls_cbc.h
@@ -46,9 +46,9 @@ class BOTAN_TEST_API TLS_CBC_HMAC_AEAD_Mode : public AEAD_Mode
 
  protected:
       TLS_CBC_HMAC_AEAD_Mode(Cipher_Dir direction,
-                             const std::string& cipher_name,
+                             std::unique_ptr<BlockCipher> cipher,
+                             std::unique_ptr<MessageAuthenticationCode> mac,
                              size_t cipher_keylen,
-                             const std::string& mac_name,
                              size_t mac_keylen,
                              bool use_explicit_iv,
                              bool use_encrypt_then_mac);
@@ -104,16 +104,17 @@ class BOTAN_TEST_API TLS_CBC_HMAC_AEAD_Encryption final : public TLS_CBC_HMAC_AE
    public:
       /**
       */
-      TLS_CBC_HMAC_AEAD_Encryption(const std::string& cipher_algo,
-                                   const size_t cipher_keylen,
-                                   const std::string& mac_algo,
-                                   const size_t mac_keylen,
-                                   bool use_explicit_iv,
-                                   bool use_encrypt_then_mac) :
+      TLS_CBC_HMAC_AEAD_Encryption(
+                             std::unique_ptr<BlockCipher> cipher,
+                             std::unique_ptr<MessageAuthenticationCode> mac,
+                             const size_t cipher_keylen,
+                             const size_t mac_keylen,
+                             bool use_explicit_iv,
+                             bool use_encrypt_then_mac) :
          TLS_CBC_HMAC_AEAD_Mode(ENCRYPTION,
-                                cipher_algo,
+                                std::move(cipher),
+                                std::move(mac),
                                 cipher_keylen,
-                                mac_algo,
                                 mac_keylen,
                                 use_explicit_iv,
                                 use_encrypt_then_mac)
@@ -138,16 +139,16 @@ class BOTAN_TEST_API TLS_CBC_HMAC_AEAD_Decryption final : public TLS_CBC_HMAC_AE
    public:
       /**
       */
-      TLS_CBC_HMAC_AEAD_Decryption(const std::string& cipher_algo,
+      TLS_CBC_HMAC_AEAD_Decryption(std::unique_ptr<BlockCipher> cipher,
+                                   std::unique_ptr<MessageAuthenticationCode> mac,
                                    const size_t cipher_keylen,
-                                   const std::string& mac_algo,
                                    const size_t mac_keylen,
                                    bool use_explicit_iv,
                                    bool use_encrypt_then_mac) :
          TLS_CBC_HMAC_AEAD_Mode(DECRYPTION,
-                                cipher_algo,
+                                std::move(cipher),
+                                std::move(mac),
                                 cipher_keylen,
-                                mac_algo,
                                 mac_keylen,
                                 use_explicit_iv,
                                 use_encrypt_then_mac)
diff --git a/src/lib/tls/tls_record.cpp b/src/lib/tls/tls_record.cpp
index ded3831d0..1f564a689 100644
--- a/src/lib/tls/tls_record.cpp
+++ b/src/lib/tls/tls_record.cpp
@@ -57,12 +57,15 @@ Connection_Cipher_State::Connection_Cipher_State(Protocol_Version version,
       {
 #if defined(BOTAN_HAS_TLS_CBC)
       // legacy CBC+HMAC mode
+      auto mac = MessageAuthenticationCode::create_or_throw("HMAC(" + suite.mac_algo() + ")");
+      auto cipher = BlockCipher::create_or_throw(suite.cipher_algo());
+
       if(our_side)
          {
          m_aead.reset(new TLS_CBC_HMAC_AEAD_Encryption(
-                         suite.cipher_algo(),
+                         std::move(cipher),
+                         std::move(mac),
                          suite.cipher_keylen(),
-                         suite.mac_algo(),
                          suite.mac_keylen(),
                          version.supports_explicit_cbc_ivs(),
                          uses_encrypt_then_mac));
@@ -70,9 +73,9 @@ Connection_Cipher_State::Connection_Cipher_State(Protocol_Version version,
       else
          {
          m_aead.reset(new TLS_CBC_HMAC_AEAD_Decryption(
-                         suite.cipher_algo(),
+                         std::move(cipher),
+                         std::move(mac),
                          suite.cipher_keylen(),
-                         suite.mac_algo(),
                          suite.mac_keylen(),
                          version.supports_explicit_cbc_ivs(),
                          uses_encrypt_then_mac));
diff --git a/src/tests/data/tls_cbc.vec b/src/tests/data/tls_cbc.vec
new file mode 100644
index 000000000..7df3a13de
--- /dev/null
+++ b/src/tests/data/tls_cbc.vec
@@ -0,0 +1,50 @@
+
+MACsize = 16
+Blocksize = 16
+Record = 000000000000000000000000000000000F0F0F0F0F0F0F0F0F0F0F0F0F0F0F0F
+Valid = 1
+
+MACsize = 20
+Blocksize = 16
+Record = 00000000000000000000000000000000000000000B0B0B0B0B0B0B0B0B0B0B0B
+Valid = 1
+
+MACsize = 32
+Blocksize = 16
+Record = 00000000000000000000000000000000000000000000000000000000000000000F0F0F0F0F0F0F0F0F0F0F0F0F0F0F0F
+Valid = 1
+
+MACsize = 16
+Blocksize = 16
+Record = 000001000000000000000000000000000F0F0E0F0F0F0F0F0F0F0F0F0F0F0F0F
+Valid = 0
+
+MACsize = 16
+Blocksize = 16
+Record = 01000000000000000000000000000000010E0E0E0E0E0E0E0E0E0E0E0E0E0E0E
+Valid = 1
+
+MACsize = 16
+Blocksize = 16
+Record = 0F0F0F0F0F0F0F0F0F0F0F0F0F0F0F0F
+Valid = 0
+
+MACsize = 20
+Blocksize = 16
+Record = 000000000000000000000000000000000000000C0C0C0C0C0C0C0C0C0C0C0C0C
+Valid = 0
+
+MACsize = 20
+Blocksize = 16
+Record = 0000000000000000000000000000001010101010101010101010101010101000
+Valid = 0
+
+MACsize = 20
+Blocksize = 16
+Record = 0000000000000000000000000000111111111111111111111111111111110000
+Valid = 0
+
+MACsize = 20
+Blocksize = 16
+Record = 0000000000000000000000000012121212121212121212121212121212000000
+Valid = 0
diff --git a/src/tests/test_tls.cpp b/src/tests/test_tls.cpp
index 6b8fa2867..ece5ef249 100644
--- a/src/tests/test_tls.cpp
+++ b/src/tests/test_tls.cpp
@@ -44,6 +44,119 @@ class TLS_CBC_Padding_Tests final : public Text_Based_Test
 
 BOTAN_REGISTER_TEST("tls_cbc_padding", TLS_CBC_Padding_Tests);
 
+class TLS_CBC_Tests final : public Text_Based_Test
+   {
+   public:
+
+      class ZeroMac : public Botan::MessageAuthenticationCode
+         {
+         public:
+            ZeroMac(size_t mac_len) : m_mac_len(mac_len) {}
+
+            void clear() override {}
+
+            std::string name() const override { return "ZeroMac"; }
+            size_t output_length() const override { return m_mac_len; }
+
+            void add_data(const uint8_t[], size_t) override {}
+
+            void final_result(uint8_t out[])
+               {
+               std::memset(out, 0, m_mac_len);
+               }
+
+            Botan::Key_Length_Specification key_spec() const override
+               {
+               return Botan::Key_Length_Specification(0, 0, 1);
+               }
+
+            virtual MessageAuthenticationCode* clone() const override { return new ZeroMac(m_mac_len); }
+
+         private:
+            void key_schedule(const uint8_t[], size_t) override {}
+
+            size_t m_mac_len;
+         };
+
+      class Noop_Block_Cipher : public Botan::BlockCipher
+         {
+         public:
+            Noop_Block_Cipher(size_t bs) : m_bs(bs) {}
+
+            void encrypt_n(const uint8_t in[], uint8_t out[], size_t blocks) const override
+               {
+               std::memmove(out, in, blocks * m_bs);
+               }
+
+            void decrypt_n(const uint8_t in[], uint8_t out[], size_t blocks) const override
+               {
+               std::memmove(out, in, blocks * m_bs);
+               }
+
+            size_t block_size() const override { return m_bs; }
+            void clear() override { }
+            std::string name() const override { return "noop"; }
+
+            Botan::Key_Length_Specification key_spec() const override
+               {
+               return Botan::Key_Length_Specification(0, 0, 1);
+               }
+
+            virtual BlockCipher* clone() const override { return new Noop_Block_Cipher(m_bs); }
+         private:
+            void key_schedule(const uint8_t[], size_t) override {}
+
+            size_t m_bs;
+         };
+
+      TLS_CBC_Tests() : Text_Based_Test("tls_cbc.vec", "Blocksize,MACsize,Record,Valid") {}
+
+      Test::Result run_one_test(const std::string&, const VarMap& vars) override
+         {
+         Test::Result result("TLS CBC");
+
+         const size_t block_size = get_req_sz(vars, "Blocksize");
+         const size_t mac_len = get_req_sz(vars, "MACsize");
+         const std::vector<uint8_t> record = get_req_bin(vars, "Record");
+         const bool is_valid = get_req_sz(vars, "Valid") == 1;
+
+         // todo test permutations
+         bool explicit_iv = true;
+         bool encrypt_then_mac = false;
+
+         Botan::TLS::TLS_CBC_HMAC_AEAD_Decryption tls_cbc(
+            std::unique_ptr<Botan::BlockCipher>(new Noop_Block_Cipher(block_size)),
+            std::unique_ptr<Botan::MessageAuthenticationCode>(new ZeroMac(mac_len)),
+            0, 0, explicit_iv, encrypt_then_mac);
+
+         tls_cbc.set_key(std::vector<uint8_t>(0));
+         std::vector<uint8_t> ad(13);
+         tls_cbc.set_associated_data(ad.data(), ad.size());
+
+         Botan::secure_vector<uint8_t> vec(record.begin(), record.end());
+
+         try
+            {
+            tls_cbc.finish(vec, 0);
+            if(is_valid)
+               result.test_success("Accepted valid TLS-CBC ciphertext");
+            else
+               result.test_failure("Accepted invalid TLS-CBC ciphertext");
+            }
+         catch(std::exception& e)
+            {
+            if(is_valid)
+               result.test_failure("Rejected valid TLS-CBC ciphertext");
+            else
+               result.test_success("Accepted invalid TLS-CBC ciphertext");
+            }
+
+         return result;
+         }
+   };
+
+BOTAN_REGISTER_TEST("tls_cbc", TLS_CBC_Tests);
+
 #endif
 
 class Test_TLS_Alert_Strings : public Test