Address CFB carryover bug

Test data generated by 1.10 so hopefully no further issues here.

GH #1200

3 files changed, 115 insertions, 47 deletions

diff --git a/src/lib/modes/cfb/cfb.cpp b/src/lib/modes/cfb/cfb.cpp
index 56d234090..d22f1a35b 100644
--- a/src/lib/modes/cfb/cfb.cpp
+++ b/src/lib/modes/cfb/cfb.cpp
@@ -1,6 +1,6 @@
 /*
 * CFB Mode
-* (C) 1999-2007,2013 Jack Lloyd
+* (C) 1999-2007,2013,2017 Jack Lloyd
 * (C) 2016 Daniel Neus, Rohde & Schwarz Cybersecurity
 *
 * Botan is released under the Simplified BSD License (see license.txt)
@@ -28,8 +28,8 @@ void CFB_Mode::clear()
 
 void CFB_Mode::reset()
    {
-   m_shift_register.clear();
-   m_keystream_buf.clear();
+   m_state.clear();
+   m_keystream.clear();
    }
 
 std::string CFB_Mode::name() const
@@ -82,44 +82,46 @@ void CFB_Mode::start_msg(const uint8_t nonce[], size_t nonce_len)
 
    if(nonce_len == 0)
       {
-      if(m_shift_register.empty())
+      if(m_state.empty())
          {
          throw Invalid_State("CFB requires a non-empty initial nonce");
          }
+      // No reason to encrypt state->keystream_buf, because no change
       }
    else
       {
-      m_shift_register.assign(nonce, nonce + nonce_len);
+      m_state.assign(nonce, nonce + nonce_len);
+      m_keystream.resize(m_state.size());
+      cipher().encrypt(m_state, m_keystream);
+      m_keystream_pos = 0;
       }
-
-   m_keystream_buf.resize(m_shift_register.size());
-   cipher().encrypt(m_shift_register, m_keystream_buf);
    }
 
 size_t CFB_Encryption::process(uint8_t buf[], size_t sz)
    {
    const size_t BS = cipher().block_size();
 
-   secure_vector<uint8_t>& state = shift_register();
    const size_t shift = feedback();
-   size_t left = sz;
+   const size_t carryover = BS - shift;
 
-   while(left)
+   for(size_t i = 0; i != sz; ++i)
       {
-      const size_t took = std::min(shift, left);
-      xor_buf(buf, &keystream_buf()[0], took);
+      buf[i] = (m_keystream[m_keystream_pos] ^= buf[i]);
+
+      m_keystream_pos++;
 
-      // Assumes feedback-sized block except for last input
-      if (BS - shift > 0)
+      if(m_keystream_pos == shift)
          {
-         copy_mem(state.data(), &state[shift], BS - shift);
+         if(carryover > 0)
+            {
+            copy_mem(m_state.data(), &m_state[shift], carryover);
+            }
+         copy_mem(&m_state[carryover], m_keystream.data(), shift);
+         cipher().encrypt(m_state, m_keystream);
+         m_keystream_pos = 0;
          }
-      copy_mem(&state[BS-shift], buf, took);
-      cipher().encrypt(state, keystream_buf());
-
-      buf += took;
-      left -= took;
       }
+
    return sz;
    }
 
@@ -131,31 +133,29 @@ void CFB_Encryption::finish(secure_vector<uint8_t>& buffer, size_t offset)
 size_t CFB_Decryption::process(uint8_t buf[], size_t sz)
    {
    const size_t BS = cipher().block_size();
-
-   secure_vector<uint8_t>& state = shift_register();
    const size_t shift = feedback();
-   size_t left = sz;
+   const size_t carryover = BS - shift;
 
-   while(left)
+   for(size_t i = 0; i != sz; ++i)
       {
-      const size_t took = std::min(shift, left);
+      uint8_t k = m_keystream[m_keystream_pos];
+      m_keystream[m_keystream_pos] = buf[i];
+      buf[i] ^= k;
 
-      // first update shift register with ciphertext
-      if (BS - shift > 0)
+      m_keystream_pos++;
+
+      if(m_keystream_pos == shift)
          {
-         copy_mem(state.data(), &state[shift], BS - shift);
+         if(carryover > 0)
+            {
+            copy_mem(m_state.data(), &m_state[shift], carryover);
+            }
+         copy_mem(&m_state[carryover], m_keystream.data(), shift);
+         cipher().encrypt(m_state, m_keystream);
+         m_keystream_pos = 0;
          }
-      copy_mem(&state[BS-shift], buf, took);
-
-      // then decrypt
-      xor_buf(buf, &keystream_buf()[0], took);
-
-      // then update keystream
-      cipher().encrypt(state, keystream_buf());
-
-      buf += took;
-      left -= took;
       }
+
    return sz;
    }
 
diff --git a/src/lib/modes/cfb/cfb.h b/src/lib/modes/cfb/cfb.h
index a128539a4..ce85d2c2e 100644
--- a/src/lib/modes/cfb/cfb.h
+++ b/src/lib/modes/cfb/cfb.h
@@ -40,22 +40,19 @@ class BOTAN_DLL CFB_Mode : public Cipher_Mode
    protected:
       CFB_Mode(BlockCipher* cipher, size_t feedback_bits);
 
-      const BlockCipher& cipher() const { return *m_cipher; }
-
       size_t feedback() const { return m_feedback_bytes; }
+      const BlockCipher& cipher() const { return *m_cipher; }
 
-      secure_vector<uint8_t>& shift_register() { return m_shift_register; }
-
-      secure_vector<uint8_t>& keystream_buf() { return m_keystream_buf; }
+      secure_vector<uint8_t> m_state;
+      secure_vector<uint8_t> m_keystream;
+      size_t m_keystream_pos = 0;
 
    private:
       void start_msg(const uint8_t nonce[], size_t nonce_len) override;
       void key_schedule(const uint8_t key[], size_t length) override;
 
       std::unique_ptr<BlockCipher> m_cipher;
-      secure_vector<uint8_t> m_shift_register;
-      secure_vector<uint8_t> m_keystream_buf;
-      size_t m_feedback_bytes;
+      const size_t m_feedback_bytes;
    };
 
 /**
diff --git a/src/tests/test_filters.cpp b/src/tests/test_filters.cpp
index 6199f88e2..af00ae4fc 100644
--- a/src/tests/test_filters.cpp
+++ b/src/tests/test_filters.cpp
@@ -43,6 +43,7 @@ class Filter_Tests : public Test
          results.push_back(test_pipe_mac());
          results.push_back(test_pipe_stream());
          results.push_back(test_pipe_cbc());
+         results.push_back(test_pipe_cfb());
          results.push_back(test_pipe_compress());
          results.push_back(test_pipe_codec());
          results.push_back(test_fork());
@@ -274,6 +275,76 @@ class Filter_Tests : public Test
          return result;
          }
 
+      Test::Result test_pipe_cfb()
+         {
+         Test::Result result("Pipe CFB");
+
+#if defined(BOTAN_HAS_BLOWFISH) && defined(BOTAN_HAS_MODE_CFB)
+
+         // Generated with Botan 1.10
+
+         const Botan::InitializationVector iv("AABBCCDDEEFF0123");
+         const Botan::SymmetricKey key("AABBCCDDEEFF0123");
+
+         const uint8_t msg_bits[] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 };
+
+         const std::string cfb_expected[] = {
+            "A4",
+            "BEA4",
+            "06AD98",
+            "E4AFC5AC",
+            "A9B531559C",
+            "38B60DA66445",
+            "194F5E93199839",
+            "093B6381D2E5D806",
+            "B44FA624226EECF027",
+            "80B8DC3332A835AC11A8",
+            "2C0E910A1E5C38344CC5BB",
+            "3CB6180AE2E189342F681023",
+            "DE0F4B10C7D9CADDB5A9078199",
+            "FAE18B0ED873F234CCD6E1555B2D",
+            "7195FFE735B0A95065BA244C77A11F",
+         };
+
+         Botan::Keyed_Filter* cfb_enc =
+            new Botan::Cipher_Mode_Filter(Botan::get_cipher_mode("Blowfish/CFB", Botan::ENCRYPTION));
+         cfb_enc->set_key(key);
+         cfb_enc->set_iv(iv);
+         Botan::Pipe enc_pipe(cfb_enc, new Botan::Hex_Encoder);
+
+         Botan::Keyed_Filter* cfb_dec =
+            new Botan::Cipher_Mode_Filter(Botan::get_cipher_mode("Blowfish/CFB", Botan::DECRYPTION));
+         cfb_dec->set_key(key);
+         cfb_dec->set_iv(iv);
+         Botan::Pipe dec_pipe(new Botan::Hex_Decoder, cfb_dec, new Botan::Hex_Encoder);
+
+         for(size_t i = 1; i != sizeof(msg_bits); ++i)
+            {
+            enc_pipe.start_msg();
+            enc_pipe.write(msg_bits, i);
+            enc_pipe.end_msg();
+
+            dec_pipe.process_msg(cfb_expected[i-1]);
+            }
+
+         result.test_eq("enc pipe msg count", enc_pipe.message_count(), sizeof(msg_bits) - 1);
+         result.test_eq("dec pipe msg count", dec_pipe.message_count(), sizeof(msg_bits) - 1);
+
+         for(size_t i = 0; i != enc_pipe.message_count(); ++i)
+            {
+            result.test_eq("encrypt", enc_pipe.read_all_as_string(i), cfb_expected[i]);
+            }
+
+         for(size_t i = 0; i != enc_pipe.message_count(); ++i)
+            {
+            result.test_eq("decrypt", dec_pipe.read_all_as_string(i),
+                           Botan::hex_encode(msg_bits, i+1));
+            }
+#endif
+
+         return result;
+         }
+
       Test::Result test_pipe_cbc()
          {
          Test::Result result("Pipe CBC");