The server would incorrectly send a server key exchange message when a

pure RSA ciphersuite was negotiated.

Detection of version rollback attacks with pure RSA ciphersuites was
incorrect and would cause failures if the client supported a version
we didn't (eg GnuTLS with TLS 1.2 enabled).

Improve detection of SSLv2 client hellos. In particular, if a client
that only supports SSLv2 connects, we will detect this case and send a
protocol_version alert (which the SSLv2-only client will not
understand, but a packet analyzer probably will) plus an exception
with the message "Client claims to only support SSLv2, rejecting"
instead of the previous much less helpful "Unknown record type"
message.

Remove vestigial support for RSA export ciphersuite key exchange.

7 files changed, 85 insertions, 90 deletions

diff --git a/src/tls/c_kex.cpp b/src/tls/c_kex.cpp
index 48d76d1e6..22c0253c1 100644
--- a/src/tls/c_kex.cpp
+++ b/src/tls/c_kex.cpp
@@ -66,7 +66,7 @@ Client_Key_Exchange::Client_Key_Exchange(const MemoryRegion<byte>& contents,
    {
    include_length = true;
 
-   if(using_version == SSL_V3 && (suite.kex_type() == TLS_ALGO_KEYEXCH_RSA))
+   if(using_version == SSL_V3 && (suite.kex_type() == TLS_ALGO_KEYEXCH_NOKEX))
       include_length = false;
 
    deserialize(contents);
diff --git a/src/tls/rec_read.cpp b/src/tls/rec_read.cpp
index 59401e26c..c2c6309a6 100644
--- a/src/tls/rec_read.cpp
+++ b/src/tls/rec_read.cpp
@@ -14,14 +14,12 @@
 
 namespace Botan {
 
-Record_Reader::Record_Reader()
+Record_Reader::Record_Reader() :
+   m_readbuf(TLS_HEADER_SIZE + MAX_CIPHERTEXT_SIZE),
+   m_mac(0)
    {
-   m_mac = 0;
    reset();
    set_maximum_fragment_size(0);
-
-   // A single record is never larger than this
-   m_readbuf.resize(MAX_CIPHERTEXT_SIZE);
    }
 
 /*
@@ -170,41 +168,46 @@ size_t Record_Reader::add_input(const byte input_array[], size_t input_sz,
 
    consumed = 0;
 
-   const size_t HEADER_SIZE = 5;
-
-   if(m_readbuf_pos < HEADER_SIZE) // header incomplete?
+   if(m_readbuf_pos < TLS_HEADER_SIZE) // header incomplete?
       {
-      if(size_t needed = fill_buffer_to(input, input_sz, consumed, HEADER_SIZE))
+      if(size_t needed = fill_buffer_to(input, input_sz, consumed, TLS_HEADER_SIZE))
          return needed;
 
-      BOTAN_ASSERT_EQUAL(m_readbuf_pos, HEADER_SIZE,
-                         "Buffer error in SSL header");
+      BOTAN_ASSERT_EQUAL(m_readbuf_pos, TLS_HEADER_SIZE,
+                         "Have an entire header");
       }
 
-   // SSLv2-format client hello?
-   if(m_readbuf[0] & 0x80 && m_readbuf[2] == 1 && m_readbuf[3] >= 3)
+   // Possible SSLv2 format client hello
+   if(m_mac_size == 0 && (m_readbuf[0] & 0x80) && (m_readbuf[2] == 1))
       {
-      size_t record_len = make_u16bit(m_readbuf[0], m_readbuf[1]) & 0x7FFF;
+      if(m_readbuf[3] == 0 && m_readbuf[4] == 2)
+         throw TLS_Exception(PROTOCOL_VERSION,
+                             "Client claims to only support SSLv2, rejecting");
 
-      if(size_t needed = fill_buffer_to(input, input_sz, consumed, record_len + 2))
-         return needed;
+      if(m_readbuf[3] >= 3) // SSLv2 mapped TLS hello, then?
+         {
+         size_t record_len = make_u16bit(m_readbuf[0], m_readbuf[1]) & 0x7FFF;
 
-      BOTAN_ASSERT_EQUAL(m_readbuf_pos, (record_len + 2),
-                         "Buffer error in SSLv2 hello");
+         if(size_t needed = fill_buffer_to(input, input_sz, consumed, record_len + 2))
+            return needed;
 
-      msg_type = HANDSHAKE;
+         BOTAN_ASSERT_EQUAL(m_readbuf_pos, (record_len + 2),
+                            "Have the entire SSLv2 hello");
 
-      msg.resize(record_len + 4);
+         msg_type = HANDSHAKE;
 
-      // Fake v3-style handshake message wrapper
-      msg[0] = CLIENT_HELLO_SSLV2;
-      msg[1] = 0;
-      msg[2] = m_readbuf[0] & 0x7F;
-      msg[3] = m_readbuf[1];
+         msg.resize(record_len + 4);
 
-      copy_mem(&msg[4], &m_readbuf[2], m_readbuf_pos - 2);
-      m_readbuf_pos = 0;
-      return 0;
+         // Fake v3-style handshake message wrapper
+         msg[0] = CLIENT_HELLO_SSLV2;
+         msg[1] = 0;
+         msg[2] = m_readbuf[0] & 0x7F;
+         msg[3] = m_readbuf[1];
+
+         copy_mem(&msg[4], &m_readbuf[2], m_readbuf_pos - 2);
+         m_readbuf_pos = 0;
+         return 0;
+         }
       }
 
    if(m_readbuf[0] != CHANGE_CIPHER_SPEC &&
@@ -217,7 +220,7 @@ size_t Record_Reader::add_input(const byte input_array[], size_t input_sz,
       }
 
    const u16bit version    = make_u16bit(m_readbuf[1], m_readbuf[2]);
-   const u16bit record_len = make_u16bit(m_readbuf[3], m_readbuf[4]);
+   const size_t record_len = make_u16bit(m_readbuf[3], m_readbuf[4]);
 
    if(m_major && (m_readbuf[1] != m_major || m_readbuf[2] != m_minor))
       throw TLS_Exception(PROTOCOL_VERSION,
@@ -228,11 +231,12 @@ size_t Record_Reader::add_input(const byte input_array[], size_t input_sz,
                           "Got message that exceeds maximum size");
 
    if(size_t needed = fill_buffer_to(input, input_sz, consumed,
-                                     HEADER_SIZE + record_len))
+                                     TLS_HEADER_SIZE + record_len))
       return needed;
 
-   BOTAN_ASSERT_EQUAL(HEADER_SIZE + record_len, m_readbuf_pos,
-                      "Bad buffer handling in record body");
+   BOTAN_ASSERT_EQUAL(static_cast<size_t>(TLS_HEADER_SIZE) + record_len,
+                      m_readbuf_pos,
+                      "Have the full record");
 
    // Null mac means no encryption either, only valid during handshake
    if(m_mac_size == 0)
@@ -246,7 +250,7 @@ size_t Record_Reader::add_input(const byte input_array[], size_t input_sz,
 
       msg_type = m_readbuf[0];
       msg.resize(record_len);
-      copy_mem(&msg[0], &m_readbuf[HEADER_SIZE], record_len);
+      copy_mem(&msg[0], &m_readbuf[TLS_HEADER_SIZE], record_len);
 
       m_readbuf_pos = 0;
       return 0; // got a full record
@@ -255,18 +259,18 @@ size_t Record_Reader::add_input(const byte input_array[], size_t input_sz,
    // Otherwise, decrypt, check MAC, return plaintext
 
    // FIXME: avoid memory allocation by processing in place
-   m_cipher.process_msg(&m_readbuf[HEADER_SIZE], record_len);
-   size_t got_back = m_cipher.read(&m_readbuf[HEADER_SIZE], record_len, Pipe::LAST_MESSAGE);
-   BOTAN_ASSERT_EQUAL(got_back, record_len, "Cipher didn't decrypt full amount");
+   m_cipher.process_msg(&m_readbuf[TLS_HEADER_SIZE], record_len);
+   size_t got_back = m_cipher.read(&m_readbuf[TLS_HEADER_SIZE], record_len, Pipe::LAST_MESSAGE);
+   BOTAN_ASSERT_EQUAL(got_back, record_len, "Cipher encrypted full amount");
 
    BOTAN_ASSERT_EQUAL(m_cipher.remaining(Pipe::LAST_MESSAGE), 0,
-                      "Cipher produced extra output");
+                      "Cipher had no remaining inputs");
 
    size_t pad_size = 0;
 
    if(m_block_size)
       {
-      byte pad_value = m_readbuf[HEADER_SIZE + (record_len-1)];
+      byte pad_value = m_readbuf[TLS_HEADER_SIZE + (record_len-1)];
       pad_size = pad_value + 1;
 
       /*
@@ -287,7 +291,7 @@ size_t Record_Reader::add_input(const byte input_array[], size_t input_sz,
          bool padding_good = true;
 
          for(size_t i = 0; i != pad_size; ++i)
-            if(m_readbuf[HEADER_SIZE + (record_len-i-1)] != pad_value)
+            if(m_readbuf[TLS_HEADER_SIZE + (record_len-i-1)] != pad_value)
                padding_good = false;
 
          if(!padding_good)
@@ -311,7 +315,7 @@ size_t Record_Reader::add_input(const byte input_array[], size_t input_sz,
          m_mac->update(get_byte(i, version));
 
    m_mac->update_be(plain_length);
-   m_mac->update(&m_readbuf[HEADER_SIZE + m_iv_size], plain_length);
+   m_mac->update(&m_readbuf[TLS_HEADER_SIZE + m_iv_size], plain_length);
 
    ++m_seq_no;
 
@@ -323,13 +327,13 @@ size_t Record_Reader::add_input(const byte input_array[], size_t input_sz,
 
    const size_t mac_offset = record_len - (m_mac_size + pad_size);
 
-   if(!same_mem(&m_readbuf[HEADER_SIZE + mac_offset], &computed_mac[0], m_mac_size))
+   if(!same_mem(&m_readbuf[TLS_HEADER_SIZE + mac_offset], &computed_mac[0], m_mac_size))
       throw TLS_Exception(BAD_RECORD_MAC, "Record_Reader: MAC failure");
 
    msg_type = m_readbuf[0];
 
    msg.resize(plain_length);
-   copy_mem(&msg[0], &m_readbuf[HEADER_SIZE + m_iv_size], plain_length);
+   copy_mem(&msg[0], &m_readbuf[TLS_HEADER_SIZE + m_iv_size], plain_length);
    m_readbuf_pos = 0;
    return 0;
    }
diff --git a/src/tls/rec_wri.cpp b/src/tls/rec_wri.cpp
index e9097f813..fdcb98cc9 100644
--- a/src/tls/rec_wri.cpp
+++ b/src/tls/rec_wri.cpp
@@ -20,7 +20,7 @@ namespace Botan {
 * Record_Writer Constructor
 */
 Record_Writer::Record_Writer(std::tr1::function<void (const byte[], size_t)> out) :
-   m_output_fn(out)
+   m_output_fn(out), m_writebuf(TLS_HEADER_SIZE + MAX_CIPHERTEXT_SIZE)
    {
    m_mac = 0;
    reset();
@@ -190,7 +190,7 @@ void Record_Writer::send_record(byte type, const byte input[], size_t length)
 
    if(m_mac_size == 0)
       {
-      const byte header[5] = {
+      const byte header[TLS_HEADER_SIZE] = {
          type,
          m_major,
          m_minor,
@@ -198,7 +198,7 @@ void Record_Writer::send_record(byte type, const byte input[], size_t length)
          get_byte<u16bit>(1, length)
       };
 
-      m_output_fn(header, 5);
+      m_output_fn(header, TLS_HEADER_SIZE);
       m_output_fn(input, length);
       }
    else
@@ -225,7 +225,8 @@ void Record_Writer::send_record(byte type, const byte input[], size_t length)
          throw TLS_Exception(INTERNAL_ERROR,
                              "Record_Writer: Record is too big");
 
-      m_writebuf.resize(5 + buf_size);
+      BOTAN_ASSERT(m_writebuf.size() >= TLS_HEADER_SIZE + MAX_CIPHERTEXT_SIZE,
+                   "Write buffer is big enough");
 
       // TLS record header
       m_writebuf[0] = type;
@@ -234,7 +235,7 @@ void Record_Writer::send_record(byte type, const byte input[], size_t length)
       m_writebuf[3] = get_byte<u16bit>(0, buf_size);
       m_writebuf[4] = get_byte<u16bit>(1, buf_size);
 
-      byte* buf_write_ptr = &m_writebuf[5];
+      byte* buf_write_ptr = &m_writebuf[TLS_HEADER_SIZE];
 
       if(m_iv_size)
          {
@@ -262,11 +263,15 @@ 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, "Cipher didn't encrypt full amount");
+      m_cipher.process_msg(&m_writebuf[TLS_HEADER_SIZE], buf_size);
+      const size_t got_back = m_cipher.read(&m_writebuf[TLS_HEADER_SIZE], buf_size, Pipe::LAST_MESSAGE);
 
-      m_output_fn(&m_writebuf[0], m_writebuf.size());
+      BOTAN_ASSERT_EQUAL(got_back, buf_size, "Cipher encrypted full amount");
+
+      BOTAN_ASSERT_EQUAL(m_cipher.remaining(Pipe::LAST_MESSAGE), 0,
+                         "No data remains in pipe");
+
+      m_output_fn(&m_writebuf[0], TLS_HEADER_SIZE + buf_size);
 
       m_seq_no++;
       }
diff --git a/src/tls/tls_channel.cpp b/src/tls/tls_channel.cpp
index 6d554e425..a19836395 100644
--- a/src/tls/tls_channel.cpp
+++ b/src/tls/tls_channel.cpp
@@ -49,7 +49,8 @@ size_t TLS_Channel::received_data(const byte buf[], size_t buf_size)
          buf += consumed;
          buf_size -= consumed;
 
-         BOTAN_ASSERT_IMPLICATAION(needed, buf_size == 0);
+         BOTAN_ASSERT(buf_size == 0 || needed == 0,
+                      "Got a full record or consumed all input");
 
          if(buf_size == 0 && needed != 0)
             return needed; // need more data to complete record
diff --git a/src/tls/tls_client.cpp b/src/tls/tls_client.cpp
index 73806a1ba..7abcdf644 100644
--- a/src/tls/tls_client.cpp
+++ b/src/tls/tls_client.cpp
@@ -286,20 +286,12 @@ void TLS_Client::process_handshake_msg(Handshake_Type type,
 
       state->kex_pub = state->server_kex->key();
 
-      bool is_dh = false, is_rsa = false;
-
-      if(dynamic_cast<DH_PublicKey*>(state->kex_pub))
-         is_dh = true;
-      else if(dynamic_cast<RSA_PublicKey*>(state->kex_pub))
-         is_rsa = true;
-      else
+      if(dynamic_cast<DH_PublicKey*>(state->kex_pub) &&
+         state->suite.kex_type() != TLS_ALGO_KEYEXCH_DH)
+         {
          throw TLS_Exception(HANDSHAKE_FAILURE,
-                             "Unknown key type received in server kex");
-
-      if((is_dh && state->suite.kex_type() != TLS_ALGO_KEYEXCH_DH) ||
-         (is_rsa && state->suite.kex_type() != TLS_ALGO_KEYEXCH_RSA))
-         throw TLS_Exception(ILLEGAL_PARAMETER,
-                             "Certificate key type did not match ciphersuite");
+                             "Server sent DH key but negotiated something else");
+         }
 
       if(state->suite.sig_type() != TLS_ALGO_SIGNER_ANON)
          {
diff --git a/src/tls/tls_magic.h b/src/tls/tls_magic.h
index 712c36831..df49dfe05 100644
--- a/src/tls/tls_magic.h
+++ b/src/tls/tls_magic.h
@@ -14,6 +14,7 @@ namespace Botan {
 * Protocol Constants for SSL/TLS
 */
 enum Size_Limits {
+   TLS_HEADER_SIZE    = 5,
    MAX_PLAINTEXT_SIZE = 16*1024,
    MAX_COMPRESSED_SIZE = MAX_PLAINTEXT_SIZE + 1024,
    MAX_CIPHERTEXT_SIZE = MAX_COMPRESSED_SIZE + 1024
@@ -167,11 +168,9 @@ enum TLS_Ciphersuite_Algos {
 
    TLS_ALGO_KEYEXCH_MASK      = 0x00FF0000,
    TLS_ALGO_KEYEXCH_NOKEX     = 0x00010000, // exchange via key in server cert
-   TLS_ALGO_KEYEXCH_RSA       = 0x00020000,
-   TLS_ALGO_KEYEXCH_DH        = 0x00030000,
-   TLS_ALGO_KEYEXCH_ECDH      = 0x00040000,
-   TLS_ALGO_KEYEXCH_SRP       = 0x00050000,
-   TLS_ALGO_KEYEXCH_ANON      = 0x00060000,
+   TLS_ALGO_KEYEXCH_DH        = 0x00020000,
+   TLS_ALGO_KEYEXCH_ECDH      = 0x00030000,
+   TLS_ALGO_KEYEXCH_SRP       = 0x00040000,
 
    TLS_ALGO_MAC_MASK          = 0x0000FF00,
    TLS_ALGO_MAC_MD5           = 0x00000100,
diff --git a/src/tls/tls_server.cpp b/src/tls/tls_server.cpp
index e66936771..ccba16629 100644
--- a/src/tls/tls_server.cpp
+++ b/src/tls/tls_server.cpp
@@ -267,27 +267,21 @@ void TLS_Server::process_handshake_msg(Handshake_Type type,
                                                   server_certs);
             }
 
-         if(state->suite.kex_type() == TLS_ALGO_KEYEXCH_NOKEX)
+         if(state->suite.kex_type() != TLS_ALGO_KEYEXCH_NOKEX)
             {
-            state->kex_priv = PKCS8::copy_key(*private_key, rng);
-            }
-         else if(state->suite.kex_type() == TLS_ALGO_KEYEXCH_RSA)
-            {
-            // this seems, er, non-optimal...
-            state->kex_priv = new RSA_PrivateKey(rng, policy.rsa_export_keysize());
-            }
-         else if(state->suite.kex_type() == TLS_ALGO_KEYEXCH_DH)
-            {
-            state->kex_priv = new DH_PrivateKey(rng, policy.dh_group());
+            if(state->suite.kex_type() == TLS_ALGO_KEYEXCH_DH)
+               state->kex_priv = new DH_PrivateKey(rng, policy.dh_group());
+            else
+               throw Internal_Error("TLS_Server: Unknown ciphersuite kex type");
+
+            state->server_kex =
+               new Server_Key_Exchange(writer, state->hash, rng,
+                                       state->kex_priv, private_key,
+                                       state->client_hello->random(),
+                                       state->server_hello->random());
             }
          else
-            throw Internal_Error("TLS_Server: Unknown ciphersuite kex type");
-
-         state->server_kex =
-            new Server_Key_Exchange(writer, state->hash, rng,
-                                    state->kex_priv, private_key,
-                                    state->client_hello->random(),
-                                    state->server_hello->random());
+            state->kex_priv = PKCS8::copy_key(*private_key, rng);
 
          if(policy.require_client_auth())
             {
@@ -334,7 +328,7 @@ void TLS_Server::process_handshake_msg(Handshake_Type type,
 
       SecureVector<byte> pre_master =
          state->client_kex->pre_master_secret(rng, state->kex_priv,
-                                              state->server_hello->version());
+                                              state->client_hello->version());
 
       state->keys = SessionKeys(state->suite, state->version, pre_master,
                                 state->client_hello->random(),