Don't assume the server key exchange consists of a series of BigInts.

That happens to be true for DH and export RSA key exchanges but isn't
true for ECDH or SRP. (It's almost true for SRP, but if the salt had a
leading zero byte it would be lost in the conversion).

3 files changed, 25 insertions, 31 deletions

diff --git a/src/tls/c_kex.cpp b/src/tls/c_kex.cpp
index 8bf923041..78c60c1cc 100644
--- a/src/tls/c_kex.cpp
+++ b/src/tls/c_kex.cpp
@@ -50,19 +50,23 @@ Client_Key_Exchange::Client_Key_Exchange(Record_Writer& writer,
 
    if(state->server_kex)
       {
-      const std::vector<BigInt>& params = state->server_kex->params();
+      TLS_Data_Reader reader(state->server_kex->params());
 
       if(state->suite.kex_algo() == "DH")
          {
-         if(params.size() != 3)
+         BigInt p = BigInt::decode(reader.get_range<byte>(2, 1, 65535));
+         BigInt g = BigInt::decode(reader.get_range<byte>(2, 1, 65535));
+         BigInt Y = BigInt::decode(reader.get_range<byte>(2, 1, 65535));
+
+         if(reader.remaining_bytes())
             throw Decoding_Error("Bad params size for DH key exchange");
 
-         DL_Group group(params[0], params[1]);
+         DL_Group group(p, g);
 
          if(!group.verify_group(rng, true))
             throw Internal_Error("DH group failed validation, possible attack");
 
-         DH_PublicKey counterparty_key(group, params[2]);
+         DH_PublicKey counterparty_key(group, Y);
 
          // FIXME Check that public key is residue?
 
diff --git a/src/tls/s_kex.cpp b/src/tls/s_kex.cpp
index 0a7ae9b14..4425285f0 100644
--- a/src/tls/s_kex.cpp
+++ b/src/tls/s_kex.cpp
@@ -8,9 +8,9 @@
 #include <botan/internal/tls_messages.h>
 #include <botan/internal/tls_reader.h>
 #include <botan/internal/tls_extensions.h>
+#include <botan/loadstor.h>
 #include <botan/pubkey.h>
 #include <botan/dh.h>
-#include <botan/loadstor.h>
 #include <memory>
 
 namespace Botan {
@@ -25,13 +25,11 @@ Server_Key_Exchange::Server_Key_Exchange(Record_Writer& writer,
                                          RandomNumberGenerator& rng,
                                          const Private_Key* private_key)
    {
-   const DH_PublicKey* dh_pub = dynamic_cast<const DH_PublicKey*>(state->kex_priv);
-
-   if(dh_pub)
+   if(const DH_PublicKey* dh_pub = dynamic_cast<const DH_PublicKey*>(state->kex_priv))
       {
-      m_params.push_back(dh_pub->get_domain().get_p());
-      m_params.push_back(dh_pub->get_domain().get_g());
-      m_params.push_back(BigInt::decode(dh_pub->public_value()));
+      append_tls_length_value(m_params, BigInt::encode(dh_pub->get_domain().get_p()), 2);
+      append_tls_length_value(m_params, BigInt::encode(dh_pub->get_domain().get_g()), 2);
+      append_tls_length_value(m_params, dh_pub->public_value(), 2);
       }
    else
       throw Invalid_Argument("Unknown key type " + state->kex_priv->algo_name() +
@@ -44,7 +42,7 @@ Server_Key_Exchange::Server_Key_Exchange(Record_Writer& writer,
 
    signer.update(state->client_hello->random());
    signer.update(state->server_hello->random());
-   signer.update(serialize_params());
+   signer.update(params());
    m_signature = signer.signature(rng);
 
    send(writer, state->hash);
@@ -55,7 +53,7 @@ Server_Key_Exchange::Server_Key_Exchange(Record_Writer& writer,
 */
 MemoryVector<byte> Server_Key_Exchange::serialize() const
    {
-   MemoryVector<byte> buf = serialize_params();
+   MemoryVector<byte> buf = params();
 
    // This should be an explicit version check
    if(m_hash_algo != "" && m_sig_algo != "")
@@ -69,19 +67,6 @@ MemoryVector<byte> Server_Key_Exchange::serialize() const
    }
 
 /**
-* Serialize the ServerParams structure
-*/
-MemoryVector<byte> Server_Key_Exchange::serialize_params() const
-   {
-   MemoryVector<byte> buf;
-
-   for(size_t i = 0; i != m_params.size(); ++i)
-      append_tls_length_value(buf, BigInt::encode(m_params[i]), 2);
-
-   return buf;
-   }
-
-/**
 * Deserialize a Server Key Exchange message
 */
 Server_Key_Exchange::Server_Key_Exchange(const MemoryRegion<byte>& buf,
@@ -94,6 +79,12 @@ Server_Key_Exchange::Server_Key_Exchange(const MemoryRegion<byte>& buf,
 
    TLS_Data_Reader reader(buf);
 
+   /*
+   * We really are just serializing things back to what they were
+   * before, but unfortunately to know where the signature is we need
+   * to be able to parse the whole thing anyway.
+   */
+
    if(kex_algo == "DH")
       {
       // 3 bigints, DH p, g, Y
@@ -101,7 +92,7 @@ Server_Key_Exchange::Server_Key_Exchange(const MemoryRegion<byte>& buf,
       for(size_t i = 0; i != 3; ++i)
          {
          BigInt v = BigInt::decode(reader.get_range<byte>(2, 1, 65535));
-         m_params.push_back(v);
+         append_tls_length_value(m_params, BigInt::encode(v), 2);
          }
       }
    else
@@ -134,7 +125,7 @@ bool Server_Key_Exchange::verify(const X509_Certificate& cert,
 
    verifier.update(state->client_hello->random());
    verifier.update(state->server_hello->random());
-   verifier.update(serialize_params());
+   verifier.update(params());
 
    return verifier.check_signature(m_signature);
    }
diff --git a/src/tls/tls_messages.h b/src/tls/tls_messages.h
index 72d9a1c60..2b2e02baa 100644
--- a/src/tls/tls_messages.h
+++ b/src/tls/tls_messages.h
@@ -359,7 +359,7 @@ class Server_Key_Exchange : public Handshake_Message
    public:
       Handshake_Type type() const { return SERVER_KEX; }
 
-      const std::vector<BigInt>& params() const { return m_params; }
+      const MemoryVector<byte>& params() const { return m_params; }
 
       bool verify(const X509_Certificate& cert,
                   Handshake_State* state) const;
@@ -375,9 +375,8 @@ class Server_Key_Exchange : public Handshake_Message
                           Protocol_Version version);
    private:
       MemoryVector<byte> serialize() const;
-      MemoryVector<byte> serialize_params() const;
 
-      std::vector<BigInt> m_params;
+      MemoryVector<byte> m_params;
 
       std::string m_sig_algo; // sig algo used to create signature
       std::string m_hash_algo; // hash used to create signature