If we're going to reject an insecure renegotiation, do it immediately,

even before parsing the client hello, since there is nothing the
client can say in the hello that will change our mind about it.

Call Policy::acceptable_protocol_version on the final negotiated
version, not what the client offered!

Clean up the server version choosing logic a bit.

Add Protocol_Version::best_known_match which returns the 'best'
version we know of matching a particular version.

3 files changed, 63 insertions, 41 deletions

diff --git a/src/tls/tls_server.cpp b/src/tls/tls_server.cpp
index 585a80584..f4ae71b2e 100644
--- a/src/tls/tls_server.cpp
+++ b/src/tls/tls_server.cpp
@@ -281,6 +281,15 @@ void Server::process_handshake_msg(Handshake_Type type,
 
    if(type == CLIENT_HELLO || type == CLIENT_HELLO_SSLV2)
       {
+      if(!m_policy.allow_insecure_renegotiation() &&
+         !(m_secure_renegotiation.initial_handshake() || m_secure_renegotiation.supported()))
+         {
+         delete m_state;
+         m_state = nullptr;
+         send_alert(Alert(Alert::NO_RENEGOTIATION));
+         return;
+         }
+
       m_state->client_hello = new Client_Hello(contents, type);
 
       if(m_state->client_hello->sni_hostname() != "")
@@ -288,59 +297,54 @@ void Server::process_handshake_msg(Handshake_Type type,
 
       Protocol_Version client_version = m_state->client_hello->version();
 
-      if(!m_policy.acceptable_protocol_version(m_state->version()))
-         {
-         throw TLS_Exception(Alert::PROTOCOL_VERSION,
-                             "Client version is unacceptable by policy");
-         }
+      const Protocol_Version prev_version = m_reader.get_version();
+      const bool is_renegotiation = prev_version.valid();
 
-      if(!client_version.known_version())
+      if((is_renegotiation && client_version == prev_version) ||
+         (!is_renegotiation && client_version.known_version()))
          {
          /*
-         This isn't a version we know. This implies this is a later
-         version than any version defined as of 2012. Offer them
-         the latest version we know.
+         Common cases: new client hello with some known version, or a
+         renegotiation using the same version as previously
+         negotiated.
          */
-         m_state->set_version(Protocol_Version::TLS_V12);
+
+         m_state->set_version(client_version);
          }
-      else
+      else if(is_renegotiation && (client_version != prev_version))
          {
-         Protocol_Version prev_version = m_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(prev_version > client_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(prev_version > client_version)
-               {
-               throw TLS_Exception(Alert::PROTOCOL_VERSION,
-                                   "Client negotiated " +
-                                   prev_version.to_string() +
-                                   " then renegotiated with " +
-                                   client_version.to_string());
-               }
-
-            m_state->set_version(prev_version);
+            throw TLS_Exception(Alert::PROTOCOL_VERSION,
+                                "Client negotiated " +
+                                prev_version.to_string() +
+                                " then renegotiated with " +
+                                client_version.to_string());
             }
          else
-            {
-            m_state->set_version(client_version);
-            }
+            m_state->set_version(prev_version);
+         }
+      else
+         {
+         /*
+         New negotiation using a version we don't know. Offer
+         them the best we currently know.
+         */
+         m_state->set_version(client_version.best_known_match());
          }
 
-      if(!m_policy.allow_insecure_renegotiation() &&
-         !(m_secure_renegotiation.initial_handshake() || m_secure_renegotiation.supported()))
+      if(!m_policy.acceptable_protocol_version(m_state->version()))
          {
-         delete m_state;
-         m_state = nullptr;
-         send_alert(Alert(Alert::NO_RENEGOTIATION));
-         return;
+         throw TLS_Exception(Alert::PROTOCOL_VERSION,
+                             "Client version is unacceptable by policy");
          }
 
       m_secure_renegotiation.update(m_state->client_hello);
diff --git a/src/tls/tls_version.cpp b/src/tls/tls_version.cpp
index 30e3855e7..f451da70e 100644
--- a/src/tls/tls_version.cpp
+++ b/src/tls/tls_version.cpp
@@ -46,6 +46,17 @@ bool Protocol_Version::operator>(const Protocol_Version& other) const
    return m_version > other.m_version;
    }
 
+Protocol_Version Protocol_Version::best_known_match() const
+   {
+   if(known_version())
+      return *this; // known version is its own best match
+
+   if(is_datagram_protocol())
+      return Protocol_Version::DTLS_V12;
+   else
+      return Protocol_Version::TLS_V12;
+   }
+
 bool Protocol_Version::known_version() const
    {
    // Don't include DTLS yet here as we can't actually process it
diff --git a/src/tls/tls_version.h b/src/tls/tls_version.h
index 4d1beb8cb..8112b2a11 100644
--- a/src/tls/tls_version.h
+++ b/src/tls/tls_version.h
@@ -72,6 +72,13 @@ class BOTAN_DLL Protocol_Version
       std::string to_string() const;
 
       /**
+      * If this version is known, return that. Otherwise return the
+      * best (most recent) version we know of.
+      * @return best matching protocol version
+      */
+      Protocol_Version best_known_match() const;
+
+      /**
       * @return true iff this is a DTLS version
       */
       bool is_datagram_protocol() const;