A fix for bug 192. First, when renegotiating in the client, attempt to

renegotiate using our currently negotiated version instead of our
preferred version. It turns out that neither OpenSSL nor GnuTLS like
clients changing the version between negotiations, both send a
protocol_version alert. So we probably want to avoid doing that.

On the server side, handle a client sending inconsistent versions as
best we can. If the client attmepts to renegotiate a session using a
later version, return a server hello with their original version (this
is what OpenSSL does). If they attempt to renegotiate using an earlier
version, send a fatal alert and close the connection, since this seems
like a dubious thing to do.

Also, fix the situation where we as a TLS v1.0 server (because of
configuration) are talking to a TLS v1.2 client. We would still use
their signature_algorithms extension and send a SHA-256 (or whatever)
signature!

11 files changed, 99 insertions, 72 deletions

diff --git a/src/tls/c_hello.cpp b/src/tls/c_hello.cpp
index 93b7e217c..919ed93f4 100644
--- a/src/tls/c_hello.cpp
+++ b/src/tls/c_hello.cpp
@@ -63,13 +63,14 @@ std::vector<byte> Hello_Request::serialize() const
 */
 Client_Hello::Client_Hello(Record_Writer& writer,
                            Handshake_Hash& hash,
+                           Protocol_Version version,
                            const Policy& policy,
                            RandomNumberGenerator& rng,
                            const std::vector<byte>& reneg_info,
                            bool next_protocol,
                            const std::string& hostname,
                            const std::string& srp_identifier) :
-   m_version(policy.pref_version()),
+   m_version(version),
    m_random(make_hello_random(rng)),
    m_suites(ciphersuite_list(policy, (srp_identifier != ""))),
    m_comp_methods(policy.compression()),
@@ -238,19 +239,6 @@ void Client_Hello::deserialize_sslv2(const std::vector<byte>& buf)
 
    m_secure_renegotiation =
       value_exists(m_suites, static_cast<u16bit>(TLS_EMPTY_RENEGOTIATION_INFO_SCSV));
-
-   if(m_version >= Protocol_Version::TLS_V12)
-      {
-      m_supported_algos.push_back(std::make_pair("SHA-1", "RSA"));
-      m_supported_algos.push_back(std::make_pair("SHA-1", "DSA"));
-      m_supported_algos.push_back(std::make_pair("SHA-1", "ECDSA"));
-      }
-   else
-      {
-      m_supported_algos.push_back(std::make_pair("TLS.Digest.0", "RSA"));
-      m_supported_algos.push_back(std::make_pair("SHA-1", "DSA"));
-      m_supported_algos.push_back(std::make_pair("SHA-1", "ECDSA"));
-      }
    }
 
 /*
@@ -319,33 +307,6 @@ void Client_Hello::deserialize(const std::vector<byte>& buf)
       m_supported_algos = sigs->supported_signature_algorthms();
       }
 
-   if(m_supported_algos.empty())
-      {
-      if(m_version >= Protocol_Version::TLS_V12)
-         {
-         /*
-         The rule for when a TLS 1.2 client not sending the extension
-         is strange; in theory, the server is supposed to act as if
-         the client had sent only SHA-1 using whatever signature
-         algorithm we end up negotiating. Right here, we don't know
-         what we'll end up negotiating (depends on policy), but we do
-         know that we'll only negotiate something the client sent, so
-         we can safely say it supports everything here and know that
-         we'll filter it out later.
-         */
-         m_supported_algos.push_back(std::make_pair("SHA-1", "RSA"));
-         m_supported_algos.push_back(std::make_pair("SHA-1", "DSA"));
-         m_supported_algos.push_back(std::make_pair("SHA-1", "ECDSA"));
-         }
-      else
-         {
-         // For versions before TLS 1.2, insert fake values for the old defaults
-         m_supported_algos.push_back(std::make_pair("TLS.Digest.0", "RSA"));
-         m_supported_algos.push_back(std::make_pair("SHA-1", "DSA"));
-         m_supported_algos.push_back(std::make_pair("SHA-1", "ECDSA"));
-         }
-      }
-
    if(Maximum_Fragment_Length* frag = extensions.get<Maximum_Fragment_Length>())
       {
       m_fragment_size = frag->fragment_size();
diff --git a/src/tls/c_kex.cpp b/src/tls/c_kex.cpp
index e687ff98a..2981cbaed 100644
--- a/src/tls/c_kex.cpp
+++ b/src/tls/c_kex.cpp
@@ -295,7 +295,7 @@ Client_Key_Exchange::Client_Key_Exchange(const std::vector<byte>& contents,
          }
       catch(...)
          {
-         // Randomize the hide timing channel
+         // Randomize to hide timing channel
          pre_master = rng.random_vec(48);
          pre_master[0] = client_version.major_version();
          pre_master[1] = client_version.minor_version();
diff --git a/src/tls/cert_req.cpp b/src/tls/cert_req.cpp
index 6ec5339bb..31f4fb1e1 100644
--- a/src/tls/cert_req.cpp
+++ b/src/tls/cert_req.cpp
@@ -114,13 +114,6 @@ Certificate_Req::Certificate_Req(const std::vector<byte>& buf,
          m_supported_algos.push_back(std::make_pair(hash, sig));
          }
       }
-   else
-      {
-      // The hardcoded settings from previous protocol versions
-      m_supported_algos.push_back(std::make_pair("TLS.Digest.0", "RSA"));
-      m_supported_algos.push_back(std::make_pair("SHA-1", "DSA"));
-      m_supported_algos.push_back(std::make_pair("SHA-1", "ECDSA"));
-      }
 
    const u16bit purported_size = reader.get_u16bit();
 
diff --git a/src/tls/rec_read.cpp b/src/tls/rec_read.cpp
index fd57496c8..2529b1679 100644
--- a/src/tls/rec_read.cpp
+++ b/src/tls/rec_read.cpp
@@ -62,6 +62,11 @@ void Record_Reader::set_version(Protocol_Version version)
    m_version = version;
    }
 
+Protocol_Version Record_Reader::get_version() const
+   {
+   return m_version;
+   }
+
 /*
 * Set the keys for reading
 */
diff --git a/src/tls/tls_channel.cpp b/src/tls/tls_channel.cpp
index b86066574..e7313b709 100644
--- a/src/tls/tls_channel.cpp
+++ b/src/tls/tls_channel.cpp
@@ -50,6 +50,9 @@ size_t Channel::received_data(const byte buf[], size_t buf_size)
                                                 consumed,
                                                 rec_type, record);
 
+         BOTAN_ASSERT(consumed <= buf_size,
+                      "Record reader consumed sane amount");
+
          buf += consumed;
          buf_size -= consumed;
 
diff --git a/src/tls/tls_client.cpp b/src/tls/tls_client.cpp
index d19249d68..ec3d9953f 100644
--- a/src/tls/tls_client.cpp
+++ b/src/tls/tls_client.cpp
@@ -71,6 +71,7 @@ Client::Client(std::function<void (const byte[], size_t)> output_fn,
       state->client_hello = new Client_Hello(
          writer,
          state->hash,
+         policy.pref_version(),
          policy,
          rng,
          secure_renegotiation.for_client_hello(),
@@ -117,6 +118,7 @@ void Client::renegotiate(bool force_full_renegotiation)
       state->client_hello = new Client_Hello(
          writer,
          state->hash,
+         reader.get_version(),
          policy,
          rng,
          secure_renegotiation.for_client_hello());
diff --git a/src/tls/tls_handshake_state.cpp b/src/tls/tls_handshake_state.cpp
index 7f289c205..8b06facc3 100644
--- a/src/tls/tls_handshake_state.cpp
+++ b/src/tls/tls_handshake_state.cpp
@@ -188,35 +188,61 @@ KDF* Handshake_State::protocol_specific_prf()
    throw Internal_Error("Unknown version code " + version().to_string());
    }
 
-std::pair<std::string, Signature_Format>
-Handshake_State::choose_sig_format(const Private_Key* key,
-                                   std::string& hash_algo_out,
-                                   std::string& sig_algo_out,
-                                   bool for_client_auth)
+namespace {
+
+std::string choose_hash(const std::string& sig_algo,
+                        Protocol_Version negotiated_version,
+                        bool for_client_auth,
+                        Client_Hello* client_hello,
+                        Certificate_Req* cert_req)
    {
-   const std::string sig_algo = key->algo_name();
+   if(negotiated_version < Protocol_Version::TLS_V12)
+      {
+      if(for_client_auth && negotiated_version == Protocol_Version::SSL_V3)
+         return "Raw";
+
+      if(sig_algo == "RSA")
+         return "TLS.Digest.0";
 
-   const std::vector<std::pair<std::string, std::string> > supported_algos =
-      (for_client_auth) ? cert_req->supported_algos() : client_hello->supported_algos();
+      if(sig_algo == "DSA")
+         return "SHA-1";
+
+      if(sig_algo == "ECDSA")
+         return "SHA-1";
+
+      throw Internal_Error("Unknown TLS signature algo " + sig_algo);
+      }
 
-   std::string hash_algo;
+   const auto supported_algos = for_client_auth ?
+      cert_req->supported_algos() :
+      client_hello->supported_algos();
 
-   for(size_t i = 0; i != supported_algos.size(); ++i)
+   for(auto algo : supported_algos)
       {
-      if(supported_algos[i].second == sig_algo)
-         {
-         hash_algo = supported_algos[i].first;
-         break;
-         }
+      if(algo.second == sig_algo)
+         return algo.first;
       }
 
-   if(for_client_auth && this->version() == Protocol_Version::SSL_V3)
-      hash_algo = "Raw";
+   // TLS v1.2 default hash if the counterparty sent nothing
+   return "SHA-1";
+   }
+
+}
 
-   if(hash_algo == "" && this->version() == Protocol_Version::TLS_V12)
-      hash_algo = "SHA-1"; // TLS 1.2 but no compatible hashes set (?)
+std::pair<std::string, Signature_Format>
+Handshake_State::choose_sig_format(const Private_Key* key,
+                                   std::string& hash_algo_out,
+                                   std::string& sig_algo_out,
+                                   bool for_client_auth)
+   {
+   const std::string sig_algo = key->algo_name();
 
-   BOTAN_ASSERT(hash_algo != "", "Couldn't figure out hash to use");
+   const std::string hash_algo =
+      choose_hash(sig_algo,
+                  this->version(),
+                  for_client_auth,
+                  client_hello,
+                  cert_req);
 
    if(this->version() >= Protocol_Version::TLS_V12)
       {
diff --git a/src/tls/tls_messages.h b/src/tls/tls_messages.h
index 31c35d901..0760b1815 100644
--- a/src/tls/tls_messages.h
+++ b/src/tls/tls_messages.h
@@ -115,6 +115,7 @@ class Client_Hello : public Handshake_Message
 
       Client_Hello(Record_Writer& writer,
                    Handshake_Hash& hash,
+                   Protocol_Version version,
                    const Policy& policy,
                    RandomNumberGenerator& rng,
                    const std::vector<byte>& reneg_info,
diff --git a/src/tls/tls_record.h b/src/tls/tls_record.h
index 72d5e276d..9b1f7e0f7 100644
--- a/src/tls/tls_record.h
+++ b/src/tls/tls_record.h
@@ -101,6 +101,8 @@ class BOTAN_DLL Record_Reader
 
       void set_version(Protocol_Version version);
 
+      Protocol_Version get_version() const;
+
       void reset();
 
       void set_maximum_fragment_size(size_t max_fragment);
diff --git a/src/tls/tls_server.cpp b/src/tls/tls_server.cpp
index bb3dfe5ff..204affb16 100644
--- a/src/tls/tls_server.cpp
+++ b/src/tls/tls_server.cpp
@@ -282,10 +282,42 @@ void Server::process_handshake_msg(Handshake_Type type,
          throw TLS_Exception(Alert::PROTOCOL_VERSION,
                              "Client version is unacceptable by policy");
 
-      if(client_version <= policy.pref_version())
-         state->set_version(client_version);
-      else
+      if(client_version > policy.pref_version())
+         {
          state->set_version(policy.pref_version());
+         }
+      else
+         {
+         Protocol_Version prev_version = reader.get_version();
+
+         if(prev_version.valid() && client_version != prev_version)
+            {
+            /*
+            * If this is a renegotation, and the client has offered a
+            * later version than what it initially negotiated,
+            * negotiate the old version. This matches OpenSSL's
+            * behavior. If the client is offering a version earlier
+            * than what it initially negotiated, reject as a probable
+            * attack.
+            */
+            if(client_version > prev_version)
+               {
+               state->set_version(prev_version);
+               }
+            else
+               {
+               throw TLS_Exception(Alert::PROTOCOL_VERSION,
+                                   "Client negotiated " +
+                                   prev_version.to_string() +
+                                   " then renegotiated with " +
+                                   client_version.to_string());
+               }
+            }
+         else
+            {
+            state->set_version(client_version);
+            }
+         }
 
       if(!policy.allow_insecure_renegotiation() &&
          !(secure_renegotiation.initial_handshake() || secure_renegotiation.supported()))
diff --git a/src/tls/tls_version.h b/src/tls/tls_version.h
index aa689b300..38013445f 100644
--- a/src/tls/tls_version.h
+++ b/src/tls/tls_version.h
@@ -33,6 +33,8 @@ class BOTAN_DLL Protocol_Version
       Protocol_Version(byte major, byte minor) :
          m_version((static_cast<u16bit>(major) << 8) | minor) {}
 
+      bool valid() const { return (m_version != 0); }
+
       /**
       * Get the major version of the protocol version
       */