Less buffer movement in the record reader

3 files changed, 83 insertions, 73 deletions

diff --git a/src/tls/rec_read.cpp b/src/tls/rec_read.cpp
index f6cc1057d..080d6a1f8 100644
--- a/src/tls/rec_read.cpp
+++ b/src/tls/rec_read.cpp
@@ -1,6 +1,6 @@
 /*
 * TLS Record Reading
-* (C) 2004-2010 Jack Lloyd
+* (C) 2004-2012 Jack Lloyd
 *
 * Released under the terms of the Botan license
 */
@@ -9,6 +9,7 @@
 #include <botan/lookup.h>
 #include <botan/loadstor.h>
 #include <botan/internal/tls_session_key.h>
+#include <botan/internal/assert.h>
 
 namespace Botan {
 
@@ -17,16 +18,16 @@ namespace Botan {
 */
 void Record_Reader::reset()
    {
-   cipher.reset();
+   m_cipher.reset();
 
-   delete mac;
-   mac = 0;
+   delete m_mac;
+   m_mac = 0;
 
-   mac_size = 0;
-   block_size = 0;
-   iv_size = 0;
-   major = minor = 0;
-   seq_no = 0;
+   m_mac_size = 0;
+   m_block_size = 0;
+   m_iv_size = 0;
+   m_major = m_minor = 0;
+   m_seq_no = 0;
    }
 
 /*
@@ -37,8 +38,8 @@ void Record_Reader::set_version(Version_Code version)
    if(version != SSL_V3 && version != TLS_V10 && version != TLS_V11)
       throw Invalid_Argument("Record_Reader: Invalid protocol version");
 
-   major = (version >> 8) & 0xFF;
-   minor = (version & 0xFF);
+   m_major = (version >> 8) & 0xFF;
+   m_minor = (version & 0xFF);
    }
 
 /*
@@ -47,7 +48,7 @@ void Record_Reader::set_version(Version_Code version)
 Version_Code Record_Reader::get_version() const
    {
    return static_cast<Version_Code>(
-      (static_cast<u16bit>(major) << 8) | minor);
+      (static_cast<u16bit>(m_major) << 8) | m_minor);
    }
 
 /*
@@ -57,10 +58,10 @@ void Record_Reader::activate(const TLS_Cipher_Suite& suite,
                              const SessionKeys& keys,
                              Connection_Side side)
    {
-   cipher.reset();
-   delete mac;
-   mac = 0;
-   seq_no = 0;
+   m_cipher.reset();
+   delete m_mac;
+   m_mac = 0;
+   m_seq_no = 0;
 
    SymmetricKey mac_key, cipher_key;
    InitializationVector iv;
@@ -83,22 +84,22 @@ void Record_Reader::activate(const TLS_Cipher_Suite& suite,
 
    if(have_block_cipher(cipher_algo))
       {
-      cipher.append(get_cipher(
+      m_cipher.append(get_cipher(
                        cipher_algo + "/CBC/NoPadding",
                        cipher_key, iv, DECRYPTION)
          );
-      block_size = block_size_of(cipher_algo);
+      m_block_size = block_size_of(cipher_algo);
 
-      if(major > 3 || (major == 3 && minor >= 2))
-         iv_size = block_size;
+      if(m_major > 3 || (m_major == 3 && m_minor >= 2))
+         m_iv_size = m_block_size;
       else
-         iv_size = 0;
+         m_iv_size = 0;
       }
    else if(have_stream_cipher(cipher_algo))
       {
-      cipher.append(get_cipher(cipher_algo, cipher_key, DECRYPTION));
-      block_size = 0;
-      iv_size = 0;
+      m_cipher.append(get_cipher(cipher_algo, cipher_key, DECRYPTION));
+      m_block_size = 0;
+      m_iv_size = 0;
       }
    else
       throw Invalid_Argument("Record_Reader: Unknown cipher " + cipher_algo);
@@ -107,13 +108,13 @@ void Record_Reader::activate(const TLS_Cipher_Suite& suite,
       {
       Algorithm_Factory& af = global_state().algorithm_factory();
 
-      if(major == 3 && minor == 0)
-         mac = af.make_mac("SSL3-MAC(" + mac_algo + ")");
+      if(m_major == 3 && m_minor == 0)
+         m_mac = af.make_mac("SSL3-MAC(" + mac_algo + ")");
       else
-         mac = af.make_mac("HMAC(" + mac_algo + ")");
+         m_mac = af.make_mac("HMAC(" + mac_algo + ")");
 
-      mac->set_key(mac_key);
-      mac_size = mac->output_length();
+      m_mac->set_key(mac_key);
+      m_mac_size = m_mac->output_length();
       }
    else
       throw Invalid_Argument("Record_Reader: Unknown hash " + mac_algo);
@@ -121,18 +122,18 @@ void Record_Reader::activate(const TLS_Cipher_Suite& suite,
 
 void Record_Reader::add_input(const byte input[], size_t input_size)
    {
-   input_queue.write(input, input_size);
+   m_input_queue.write(input, input_size);
    }
 
 /*
 * Retrieve the next record
 */
 size_t Record_Reader::get_record(byte& msg_type,
-                                 MemoryRegion<byte>& output)
+                                 MemoryVector<byte>& output)
    {
    byte header[5] = { 0 };
 
-   const size_t have_in_queue = input_queue.size();
+   const size_t have_in_queue = m_input_queue.size();
 
    if(have_in_queue < sizeof(header))
       return (sizeof(header) - have_in_queue);
@@ -140,7 +141,7 @@ size_t Record_Reader::get_record(byte& msg_type,
    /*
    * We peek first to make sure we have the full record
    */
-   input_queue.peek(header, sizeof(header));
+   m_input_queue.peek(header, sizeof(header));
 
    // SSLv2-format client hello?
    if(header[0] & 0x80 && header[2] == 1 && header[3] == 3)
@@ -153,7 +154,7 @@ size_t Record_Reader::get_record(byte& msg_type,
       msg_type = HANDSHAKE;
       output.resize(record_len + 4);
 
-      input_queue.read(&output[2], record_len + 2);
+      m_input_queue.read(&output[2], record_len + 2);
       output[0] = CLIENT_HELLO_SSLV2;
       output[1] = 0;
       output[2] = header[0] & 0x7F;
@@ -174,21 +175,25 @@ size_t Record_Reader::get_record(byte& msg_type,
    const u16bit version    = make_u16bit(header[1], header[2]);
    const u16bit record_len = make_u16bit(header[3], header[4]);
 
-   if(major && (header[1] != major || header[2] != minor))
+   if(m_major && (header[1] != m_major || header[2] != m_minor))
       throw TLS_Exception(PROTOCOL_VERSION,
                           "Record_Reader: Got unexpected version");
 
+   if(record_len > MAX_CIPHERTEXT_SIZE)
+      throw TLS_Exception(RECORD_OVERFLOW,
+                          "Got message that exceeds maximum size");
+
    // If insufficient data, return without doing anything
    if(have_in_queue < (sizeof(header) + record_len))
       return (sizeof(header) + record_len - have_in_queue);
 
-   SecureVector<byte> buffer(record_len);
+   m_readbuf.resize(record_len);
 
-   input_queue.read(header, sizeof(header)); // pull off the header
-   input_queue.read(&buffer[0], buffer.size());
+   m_input_queue.read(header, sizeof(header)); // pull off the header
+   m_input_queue.read(&m_readbuf[0], m_readbuf.size());
 
-   // We are handshaking, no crypto to do so return as-is
-   if(mac_size == 0)
+   // Null mac means no encryption either, only valid during handshake
+   if(m_mac_size == 0)
       {
       if(header[0] != CHANGE_CIPHER_SPEC &&
          header[0] != ALERT &&
@@ -198,20 +203,22 @@ size_t Record_Reader::get_record(byte& msg_type,
          }
 
       msg_type = header[0];
-      output = buffer;
+      std::swap(output, m_readbuf); // move semantics
       return 0; // got a full record
       }
 
    // Otherwise, decrypt, check MAC, return plaintext
 
-   cipher.process_msg(buffer);
-   SecureVector<byte> plaintext = cipher.read_all(Pipe::LAST_MESSAGE);
+   // FIXME: process in-place
+   m_cipher.process_msg(m_readbuf);
+   size_t got_back = m_cipher.read(&m_readbuf[0], m_readbuf.size(), Pipe::LAST_MESSAGE);
+   BOTAN_ASSERT_EQUAL(got_back, m_readbuf.size(), "Cipher didn't decrypt full amount");
 
    size_t pad_size = 0;
 
-   if(block_size)
+   if(m_block_size)
       {
-      byte pad_value = plaintext[plaintext.size()-1];
+      byte pad_value = m_readbuf[m_readbuf.size()-1];
       pad_size = pad_value + 1;
 
       /*
@@ -224,7 +231,7 @@ size_t Record_Reader::get_record(byte& msg_type,
       */
       if(version == SSL_V3)
          {
-         if(pad_value > block_size)
+         if(pad_value > m_block_size)
             pad_size = 0;
          }
       else
@@ -232,7 +239,7 @@ size_t Record_Reader::get_record(byte& msg_type,
          bool padding_good = true;
 
          for(size_t i = 0; i != pad_size; ++i)
-            if(plaintext[plaintext.size()-i-1] != pad_value)
+            if(m_readbuf[m_readbuf.size()-i-1] != pad_value)
                padding_good = false;
 
          if(!padding_good)
@@ -240,36 +247,38 @@ size_t Record_Reader::get_record(byte& msg_type,
          }
       }
 
-   if(plaintext.size() < mac_size + pad_size + iv_size)
+   if(m_readbuf.size() < m_mac_size + pad_size + m_iv_size)
       throw Decoding_Error("Record_Reader: Record truncated");
 
-   const size_t mac_offset = plaintext.size() - (mac_size + pad_size);
-   SecureVector<byte> received_mac(&plaintext[mac_offset],
-                                   mac_size);
-
-   const u16bit plain_length = plaintext.size() - (mac_size + pad_size + iv_size);
+   const u16bit plain_length = m_readbuf.size() - (m_mac_size + pad_size + m_iv_size);
 
-   mac->update_be(seq_no);
-   mac->update(header[0]); // msg_type
+   m_mac->update_be(m_seq_no);
+   m_mac->update(header[0]); // msg_type
 
    if(version != SSL_V3)
       for(size_t i = 0; i != 2; ++i)
-         mac->update(get_byte(i, version));
+         m_mac->update(get_byte(i, version));
+
+   m_mac->update_be(plain_length);
+   m_mac->update(&m_readbuf[m_iv_size], plain_length);
+
+   ++m_seq_no;
 
-   mac->update_be(plain_length);
-   mac->update(&plaintext[iv_size], plain_length);
+   MemoryVector<byte> computed_mac = m_mac->final();
 
-   ++seq_no;
+   const size_t mac_offset = m_readbuf.size() - (m_mac_size + pad_size);
 
-   SecureVector<byte> computed_mac = mac->final();
+   if(computed_mac.size() != m_mac_size)
+      throw TLS_Exception(INTERNAL_ERROR,
+                          "MAC produced value of unexpected size");
 
-   if(received_mac != computed_mac)
+   if(!same_mem(&m_readbuf[mac_offset], &computed_mac[0], m_mac_size))
       throw TLS_Exception(BAD_RECORD_MAC, "Record_Reader: MAC failure");
 
    msg_type = header[0];
 
    output.resize(plain_length);
-   copy_mem(&output[0], &plaintext[iv_size], plain_length);
+   copy_mem(&output[0], &m_readbuf[m_iv_size], plain_length);
    return 0;
    }
 
diff --git a/src/tls/rec_wri.cpp b/src/tls/rec_wri.cpp
index 199eb0234..d9f86492d 100644
--- a/src/tls/rec_wri.cpp
+++ b/src/tls/rec_wri.cpp
@@ -271,7 +271,7 @@ void Record_Writer::send_record(byte type, const byte input[], size_t length)
       // FIXME: this could be done in-place without copying
       m_cipher.process_msg(&m_writebuf[5], buf_size);
       size_t got_back = m_cipher.read(&m_writebuf[5], buf_size, Pipe::LAST_MESSAGE);
-      BOTAN_ASSERT_EQUAL(got_back, buf_size, "CBC didn't encrypt full blocks");
+      BOTAN_ASSERT_EQUAL(got_back, buf_size, "Cipher didn't encrypt full amount");
 
       m_output_fn(&m_writebuf[0], m_writebuf.size());
 
diff --git a/src/tls/tls_record.h b/src/tls/tls_record.h
index 69ca1b9bc..052fd43d8 100644
--- a/src/tls/tls_record.h
+++ b/src/tls/tls_record.h
@@ -88,7 +88,7 @@ class BOTAN_DLL Record_Reader
       * @return Number of bytes still needed (minimum), or 0 if success
       */
       size_t get_record(byte& msg_type,
-                        MemoryRegion<byte>& buffer);
+                        MemoryVector<byte>& buffer);
 
       SecureVector<byte> get_record(byte& msg_type);
 
@@ -102,19 +102,20 @@ class BOTAN_DLL Record_Reader
 
       void reset();
 
-      bool currently_empty() const { return input_queue.size() == 0; }
+      bool currently_empty() const { return m_input_queue.size() == 0; }
 
-      Record_Reader() { mac = 0; reset(); }
+      Record_Reader() { m_mac = 0; reset(); }
 
-      ~Record_Reader() { delete mac; }
+      ~Record_Reader() { delete m_mac; }
    private:
-      SecureQueue input_queue;
+      MemoryVector<byte> m_readbuf;
+      SecureQueue m_input_queue;
 
-      Pipe cipher;
-      MessageAuthenticationCode* mac;
-      size_t block_size, mac_size, iv_size;
-      u64bit seq_no;
-      byte major, minor;
+      Pipe m_cipher;
+      MessageAuthenticationCode* m_mac;
+      size_t m_block_size, m_mac_size, m_iv_size, m_max_fragment;
+      u64bit m_seq_no;
+      byte m_major, m_minor;
    };
 
 }