Bug 1307772 - Require that the server use the draft version, r=ekr
authorMartin Thomson <martin.thomson@gmail.com>
Thu, 06 Oct 2016 00:00:32 +1100
changeset 12685 2d463826e09ac3cd8dafc4c9510c1bd81b16b115
parent 12684 66d0a9ca356480db53561a3d3f75c1cb4a671b0b
child 12686 fdf4601f23d80f337d97a3658d09833ee4d742c2
push id1639
push usermartin.thomson@gmail.com
push dateFri, 07 Oct 2016 06:25:41 +0000
reviewersekr
bugs1307772
Bug 1307772 - Require that the server use the draft version, r=ekr
external_tests/ssl_gtest/ssl_agent_unittest.cc
external_tests/ssl_gtest/ssl_hrr_unittest.cc
lib/ssl/dtlscon.c
lib/ssl/ssl3con.c
lib/ssl/sslimpl.h
lib/ssl/sslproto.h
lib/ssl/tls13con.c
lib/ssl/tls13con.h
--- a/external_tests/ssl_gtest/ssl_agent_unittest.cc
+++ b/external_tests/ssl_gtest/ssl_agent_unittest.cc
@@ -3,49 +3,53 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "ssl.h"
 #include "sslerr.h"
 #include "sslproto.h"
 
+// This is an internal header, used to get TLS_1_3_DRAFT_VERSION.
+#include "ssl3prot.h"
+
 #include <memory>
 
 #include "databuffer.h"
 #include "tls_agent.h"
 #include "tls_connect.h"
 #include "tls_filter.h"
 #include "tls_parser.h"
 
 namespace nss_test {
 
+static const uint8_t kD13 = TLS_1_3_DRAFT_VERSION;
 // This is a 1-RTT ClientHello with ECDHE.
 const static uint8_t kCannedTls13ClientHello[] = {
     0x01, 0x00, 0x00, 0xcf, 0x03, 0x03, 0x6c, 0xb3, 0x46, 0x81, 0xc8, 0x1a,
     0xf9, 0xd2, 0x05, 0x97, 0x48, 0x7c, 0xa8, 0x31, 0x03, 0x1c, 0x06, 0xa8,
     0x62, 0xb1, 0x90, 0xd6, 0x21, 0x44, 0x7f, 0xc1, 0x9b, 0x87, 0x3e, 0xad,
     0x91, 0x85, 0x00, 0x00, 0x06, 0x13, 0x01, 0x13, 0x03, 0x13, 0x02, 0x01,
     0x00, 0x00, 0xa0, 0x00, 0x00, 0x00, 0x0b, 0x00, 0x09, 0x00, 0x00, 0x06,
     0x73, 0x65, 0x72, 0x76, 0x65, 0x72, 0xff, 0x01, 0x00, 0x01, 0x00, 0x00,
     0x0a, 0x00, 0x12, 0x00, 0x10, 0x00, 0x17, 0x00, 0x18, 0x00, 0x19, 0x01,
     0x00, 0x01, 0x01, 0x01, 0x02, 0x01, 0x03, 0x01, 0x04, 0x00, 0x28, 0x00,
     0x47, 0x00, 0x45, 0x00, 0x17, 0x00, 0x41, 0x04, 0x86, 0x4a, 0xb9, 0xdc,
     0x6a, 0x38, 0xa7, 0xce, 0xe7, 0xc2, 0x4f, 0xa6, 0x28, 0xb9, 0xdc, 0x65,
     0xbf, 0x73, 0x47, 0x3c, 0x9c, 0x65, 0x8c, 0x47, 0x6d, 0x57, 0x22, 0x8a,
     0xc2, 0xb3, 0xc6, 0x80, 0x72, 0x86, 0x08, 0x86, 0x8f, 0x52, 0xc5, 0xcb,
     0xbf, 0x2a, 0xb5, 0x59, 0x64, 0xcc, 0x0c, 0x49, 0x95, 0x36, 0xe4, 0xd9,
     0x2f, 0xd4, 0x24, 0x66, 0x71, 0x6f, 0x5d, 0x70, 0xe2, 0xa0, 0xea, 0x26,
-    0x00, 0x2b, 0x00, 0x03, 0x02, 0x7f, 0x10, 0x00, 0x0d, 0x00, 0x20, 0x00,
+    0x00, 0x2b, 0x00, 0x03, 0x02, 0x7f, kD13, 0x00, 0x0d, 0x00, 0x20, 0x00,
     0x1e, 0x04, 0x03, 0x05, 0x03, 0x06, 0x03, 0x02, 0x03, 0x08, 0x04, 0x08,
     0x05, 0x08, 0x06, 0x04, 0x01, 0x05, 0x01, 0x06, 0x01, 0x02, 0x01, 0x04,
     0x02, 0x05, 0x02, 0x06, 0x02, 0x02, 0x02};
 
 const static uint8_t kCannedTls13ServerHello[] = {
-    0x03, 0x04, 0x21, 0x12, 0xa7, 0xa7, 0x0d, 0x85, 0x8b, 0xb8, 0x0c, 0xbb,
+    0x7f, kD13, 0x21, 0x12, 0xa7, 0xa7, 0x0d, 0x85, 0x8b, 0xb8, 0x0c, 0xbb,
     0xdc, 0xa6, 0xfd, 0x97, 0xfe, 0x31, 0x26, 0x49, 0x2d, 0xa8, 0x6c, 0x7b,
     0x65, 0x30, 0x71, 0x00, 0x31, 0x03, 0x2b, 0x94, 0xe2, 0x16, 0x13, 0x01,
     0x00, 0x4d, 0x00, 0x0d, 0x00, 0x00, 0x00, 0x28, 0x00, 0x45, 0x00, 0x17,
     0x00, 0x41, 0x04, 0x10, 0x97, 0x3d, 0x7a, 0xcf, 0xa2, 0x34, 0xe3, 0x69,
     0xc4, 0xdd, 0x1e, 0xf2, 0xd6, 0xc0, 0x9a, 0x3e, 0xf5, 0x41, 0xf3, 0x03,
     0x23, 0x94, 0xd2, 0x31, 0x85, 0xb7, 0xae, 0x5d, 0xfa, 0xc6, 0x9a, 0xd0,
     0xa5, 0x44, 0xa3, 0x3a, 0xe0, 0xbb, 0x61, 0xaa, 0x0a, 0x6f, 0xe8, 0xaf,
     0xdf, 0x86, 0xd8, 0x48, 0x36, 0x9c, 0x19, 0x70, 0x55, 0x84, 0xb0, 0x1c,
@@ -66,104 +70,85 @@ TEST_P(TlsAgentTest, EarlyCertificateVer
                  SSL_ERROR_RX_UNEXPECTED_CERT_VERIFY);
 }
 
 TEST_P(TlsAgentTestClient, CannedHello) {
   DataBuffer buffer;
   EnsureInit();
   agent_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_3,
                           SSL_LIBRARY_VERSION_TLS_1_3);
-  DataBuffer server_hello_inner(kCannedTls13ServerHello,
-                                sizeof(kCannedTls13ServerHello));
-  uint16_t wire_version =
-      mode_ == STREAM ? SSL_LIBRARY_VERSION_TLS_1_3
-                      : TlsVersionToDtlsVersion(SSL_LIBRARY_VERSION_TLS_1_3);
-  server_hello_inner.Write(0, wire_version, 2);
   DataBuffer server_hello;
-  MakeHandshakeMessage(kTlsHandshakeServerHello, server_hello_inner.data(),
-                       server_hello_inner.len(), &server_hello);
+  MakeHandshakeMessage(kTlsHandshakeServerHello, kCannedTls13ServerHello,
+                       sizeof(kCannedTls13ServerHello), &server_hello);
   MakeRecord(kTlsHandshakeType, SSL_LIBRARY_VERSION_TLS_1_3,
              server_hello.data(), server_hello.len(), &buffer);
   ProcessMessage(buffer, TlsAgent::STATE_CONNECTING);
 }
 
 TEST_P(TlsAgentTestClient, EncryptedExtensionsInClear) {
-  DataBuffer buffer;
-  DataBuffer server_hello_inner(kCannedTls13ServerHello,
-                                sizeof(kCannedTls13ServerHello));
-  server_hello_inner.Write(
-      0, mode_ == STREAM ? SSL_LIBRARY_VERSION_TLS_1_3
-                         : TlsVersionToDtlsVersion(SSL_LIBRARY_VERSION_TLS_1_3),
-      2);
   DataBuffer server_hello;
-  MakeHandshakeMessage(kTlsHandshakeServerHello, server_hello_inner.data(),
-                       server_hello_inner.len(), &server_hello);
+  MakeHandshakeMessage(kTlsHandshakeServerHello, kCannedTls13ServerHello,
+                       sizeof(kCannedTls13ServerHello), &server_hello);
   DataBuffer encrypted_extensions;
   MakeHandshakeMessage(kTlsHandshakeEncryptedExtensions, nullptr, 0,
                        &encrypted_extensions, 1);
   server_hello.Append(encrypted_extensions);
+  DataBuffer buffer;
   MakeRecord(kTlsHandshakeType, SSL_LIBRARY_VERSION_TLS_1_3,
              server_hello.data(), server_hello.len(), &buffer);
   EnsureInit();
   agent_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_3,
                           SSL_LIBRARY_VERSION_TLS_1_3);
   ProcessMessage(buffer, TlsAgent::STATE_ERROR,
                  SSL_ERROR_RX_UNEXPECTED_HANDSHAKE);
 }
 
 TEST_F(TlsAgentStreamTestClient, EncryptedExtensionsInClearTwoPieces) {
-  DataBuffer buffer;
-  DataBuffer buffer2;
-  DataBuffer server_hello_inner(kCannedTls13ServerHello,
-                                sizeof(kCannedTls13ServerHello));
-  server_hello_inner.Write(0, SSL_LIBRARY_VERSION_TLS_1_3, 2);
   DataBuffer server_hello;
-  MakeHandshakeMessage(kTlsHandshakeServerHello, server_hello_inner.data(),
-                       server_hello_inner.len(), &server_hello);
+  MakeHandshakeMessage(kTlsHandshakeServerHello, kCannedTls13ServerHello,
+                       sizeof(kCannedTls13ServerHello), &server_hello);
   DataBuffer encrypted_extensions;
   MakeHandshakeMessage(kTlsHandshakeEncryptedExtensions, nullptr, 0,
                        &encrypted_extensions, 1);
   server_hello.Append(encrypted_extensions);
+  DataBuffer buffer;
   MakeRecord(kTlsHandshakeType, SSL_LIBRARY_VERSION_TLS_1_3,
              server_hello.data(), 20, &buffer);
 
+  DataBuffer buffer2;
   MakeRecord(kTlsHandshakeType, SSL_LIBRARY_VERSION_TLS_1_3,
              server_hello.data() + 20, server_hello.len() - 20, &buffer2);
 
   EnsureInit();
   agent_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_3,
                           SSL_LIBRARY_VERSION_TLS_1_3);
   ProcessMessage(buffer, TlsAgent::STATE_CONNECTING);
   ProcessMessage(buffer2, TlsAgent::STATE_ERROR,
                  SSL_ERROR_RX_UNEXPECTED_HANDSHAKE);
 }
 
 TEST_F(TlsAgentDgramTestClient, EncryptedExtensionsInClearTwoPieces) {
-  DataBuffer buffer;
-  DataBuffer buffer2;
-  DataBuffer server_hello_inner(kCannedTls13ServerHello,
-                                sizeof(kCannedTls13ServerHello));
-  server_hello_inner.Write(
-      0, TlsVersionToDtlsVersion(SSL_LIBRARY_VERSION_TLS_1_3), 2);
   DataBuffer server_hello_frag1;
   MakeHandshakeMessageFragment(
-      kTlsHandshakeServerHello, server_hello_inner.data(),
-      server_hello_inner.len(), &server_hello_frag1, 0, 0, 20);
+      kTlsHandshakeServerHello, kCannedTls13ServerHello,
+      sizeof(kCannedTls13ServerHello), &server_hello_frag1, 0, 0, 20);
   DataBuffer server_hello_frag2;
-  MakeHandshakeMessageFragment(kTlsHandshakeServerHello,
-                               server_hello_inner.data() + 20,
-                               server_hello_inner.len(), &server_hello_frag2, 0,
-                               20, server_hello_inner.len() - 20);
+  MakeHandshakeMessageFragment(
+      kTlsHandshakeServerHello, kCannedTls13ServerHello + 20,
+      sizeof(kCannedTls13ServerHello), &server_hello_frag2, 0, 20,
+      sizeof(kCannedTls13ServerHello) - 20);
   DataBuffer encrypted_extensions;
   MakeHandshakeMessage(kTlsHandshakeEncryptedExtensions, nullptr, 0,
                        &encrypted_extensions, 1);
   server_hello_frag2.Append(encrypted_extensions);
+  DataBuffer buffer;
   MakeRecord(kTlsHandshakeType, SSL_LIBRARY_VERSION_TLS_1_3,
              server_hello_frag1.data(), server_hello_frag1.len(), &buffer);
 
+  DataBuffer buffer2;
   MakeRecord(kTlsHandshakeType, SSL_LIBRARY_VERSION_TLS_1_3,
              server_hello_frag2.data(), server_hello_frag2.len(), &buffer2, 1);
 
   EnsureInit();
   agent_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_3,
                           SSL_LIBRARY_VERSION_TLS_1_3);
   ProcessMessage(buffer, TlsAgent::STATE_CONNECTING);
   ProcessMessage(buffer2, TlsAgent::STATE_ERROR,
--- a/external_tests/ssl_gtest/ssl_hrr_unittest.cc
+++ b/external_tests/ssl_gtest/ssl_hrr_unittest.cc
@@ -4,16 +4,19 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "secerr.h"
 #include "ssl.h"
 #include "sslerr.h"
 #include "sslproto.h"
 
+// This is internal, just to get TLS_1_3_DRAFT_VERSION.
+#include "ssl3prot.h"
+
 #include "gtest_utils.h"
 #include "scoped_ptrs.h"
 #include "tls_connect.h"
 #include "tls_filter.h"
 #include "tls_parser.h"
 
 namespace nss_test {
 
@@ -191,18 +194,17 @@ class HelloRetryRequestAgentTest : publi
     agent_->StartConnect();
   }
 
   void MakeCannedHrr(const uint8_t* body, size_t len, DataBuffer* hrr_record,
                      uint32_t seq_num = 0) const {
     DataBuffer hrr_data;
     hrr_data.Allocate(len + 4);
     size_t i = 0;
-    i = hrr_data.Write(i, static_cast<uint32_t>(SSL_LIBRARY_VERSION_TLS_1_3),
-                       2);
+    i = hrr_data.Write(i, 0x7f00 | TLS_1_3_DRAFT_VERSION, 2);
     i = hrr_data.Write(i, static_cast<uint32_t>(len), 2);
     if (len) {
       hrr_data.Write(i, body, len);
     }
     DataBuffer hrr;
     MakeHandshakeMessage(kTlsHandshakeHelloRetryRequest, hrr_data.data(),
                          hrr_data.len(), &hrr, seq_num);
     MakeRecord(kTlsHandshakeType, SSL_LIBRARY_VERSION_TLS_1_3, hrr.data(),
--- a/lib/ssl/dtlscon.c
+++ b/lib/ssl/dtlscon.c
@@ -994,41 +994,48 @@ dtls_SetMTU(sslSocket *ss, PRUint16 adve
  * DTLS hello_verify_request
  * Caller must hold Handshake and RecvBuf locks.
  */
 SECStatus
 dtls_HandleHelloVerifyRequest(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
 {
     int errCode = SSL_ERROR_RX_MALFORMED_HELLO_VERIFY_REQUEST;
     SECStatus rv;
-    PRInt32 temp;
+    SSL3ProtocolVersion temp;
     SSL3AlertDescription desc = illegal_parameter;
 
     SSL_TRC(3, ("%d: SSL3[%d]: handle hello_verify_request handshake",
                 SSL_GETPID(), ss->fd));
     PORT_Assert(ss->opt.noLocks || ssl_HaveRecvBufLock(ss));
     PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
 
     if (ss->ssl3.hs.ws != wait_server_hello) {
         errCode = SSL_ERROR_RX_UNEXPECTED_HELLO_VERIFY_REQUEST;
         desc = unexpected_message;
         goto alert_loser;
     }
 
-    /* The version */
-    temp = ssl3_ConsumeHandshakeNumber(ss, 2, &b, &length);
-    if (temp < 0) {
+    /* The version.
+     *
+     * RFC 4347 required that you verify that the server versions
+     * match (Section 4.2.1) in the HelloVerifyRequest and the
+     * ServerHello.
+     *
+     * RFC 6347 suggests (SHOULD) that servers always use 1.0 in
+     * HelloVerifyRequest and allows the versions not to match,
+     * especially when 1.2 is being negotiated.
+     *
+     * Therefore we do not do anything to enforce a match, just
+     * read and check that this value is sane.
+     */
+    rv = ssl_ClientReadVersion(ss, &b, &length, &temp);
+    if (rv != SECSuccess) {
         goto loser; /* alert has been sent */
     }
 
-    if (temp != SSL_LIBRARY_VERSION_DTLS_1_0_WIRE &&
-        temp != SSL_LIBRARY_VERSION_DTLS_1_2_WIRE) {
-        goto alert_loser;
-    }
-
     /* Read the cookie.
      * IMPORTANT: The value of ss->ssl3.hs.cookie is only valid while the
      * HelloVerifyRequest message remains valid. */
     rv = ssl3_ConsumeHandshakeVariable(ss, &ss->ssl3.hs.cookie, 1, &b, &length);
     if (rv != SECSuccess) {
         goto loser; /* alert has been sent */
     }
     if (ss->ssl3.hs.cookie.len > DTLS_COOKIE_BYTES) {
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -1049,16 +1049,62 @@ ssl3_NegotiateVersion(sslSocket *ss, SSL
     }
 
     ss->version = PR_MIN(peerVersion, ss->vrange.max);
     PORT_Assert(ssl3_VersionIsSupported(ss->protocolVariant, ss->version));
 
     return SECSuccess;
 }
 
+/* Used by the client when the server produces a version number.
+ * This reads, validates, and normalizes the value. */
+SECStatus
+ssl_ClientReadVersion(sslSocket *ss, SSL3Opaque **b, unsigned int *len,
+                      SSL3ProtocolVersion *version)
+{
+    SSL3ProtocolVersion v;
+    PRInt32 temp;
+
+    temp = ssl3_ConsumeHandshakeNumber(ss, 2, b, len);
+    if (temp < 0) {
+        return SECFailure; /* alert has been sent */
+    }
+
+#ifdef TLS_1_3_DRAFT_VERSION
+    if (temp == SSL_LIBRARY_VERSION_TLS_1_3) {
+        (void)SSL3_SendAlert(ss, alert_fatal, protocol_version);
+        PORT_SetError(SSL_ERROR_UNSUPPORTED_VERSION);
+        return SECFailure;
+    }
+    if (temp == tls13_EncodeDraftVersion(SSL_LIBRARY_VERSION_TLS_1_3)) {
+        v = SSL_LIBRARY_VERSION_TLS_1_3;
+    } else {
+        v = (SSL3ProtocolVersion)temp;
+    }
+#else
+    v = (SSL3ProtocolVersion)temp;
+#endif
+
+    if (IS_DTLS(ss)) {
+        /* If this fails, we get 0 back and the next check to fails. */
+        v = dtls_DTLSVersionToTLSVersion(v);
+    }
+
+    PORT_Assert(!SSL_ALL_VERSIONS_DISABLED(&ss->vrange));
+    if (ss->vrange.min > v || ss->vrange.max < v) {
+        (void)SSL3_SendAlert(ss, alert_fatal,
+                             (v > SSL_LIBRARY_VERSION_3_0) ? protocol_version
+                                                           : handshake_failure);
+        PORT_SetError(SSL_ERROR_UNSUPPORTED_VERSION);
+        return SECFailure;
+    }
+    *version = v;
+    return SECSuccess;
+}
+
 static SECStatus
 ssl3_GetNewRandom(SSL3Random *random)
 {
     SECStatus rv;
 
     rv = PK11_GenerateRandom(random->rand, SSL3_RANDOM_LENGTH);
     if (rv != SECSuccess) {
         ssl_MapLowLevelError(SSL_ERROR_GENERATE_RANDOM_FAILURE);
@@ -6463,17 +6509,16 @@ ssl3_HandleServerHello(sslSocket *ss, SS
     PRInt32 temp; /* allow for consume number failure */
     PRBool suite_found = PR_FALSE;
     int i;
     int errCode = SSL_ERROR_RX_MALFORMED_SERVER_HELLO;
     SECStatus rv;
     SECItem sidBytes = { siBuffer, NULL, 0 };
     PRBool isTLS = PR_FALSE;
     SSL3AlertDescription desc = illegal_parameter;
-    SSL3ProtocolVersion version;
 #ifndef TLS_1_3_DRAFT_VERSION
     SSL3ProtocolVersion downgradeCheckVersion;
 #endif
 
     SSL_TRC(3, ("%d: SSL3[%d]: handle server_hello handshake",
                 SSL_GETPID(), ss->fd));
     PORT_Assert(ss->opt.noLocks || ssl_HaveRecvBufLock(ss));
     PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
@@ -6494,59 +6539,33 @@ ssl3_HandleServerHello(sslSocket *ss, SS
         CERT_DestroyCertificate(ss->ssl3.clientCertificate);
         ss->ssl3.clientCertificate = NULL;
     }
     if (ss->ssl3.clientPrivateKey != NULL) {
         SECKEY_DestroyPrivateKey(ss->ssl3.clientPrivateKey);
         ss->ssl3.clientPrivateKey = NULL;
     }
 
-    temp = ssl3_ConsumeHandshakeNumber(ss, 2, &b, &length);
-    if (temp < 0) {
+    rv = ssl_ClientReadVersion(ss, &b, &length, &ss->version);
+    if (rv != SECSuccess) {
         goto loser; /* alert has been sent */
     }
-    version = tls13_DecodeDraftVersion((PRUint16)temp);
-
-    /* Try to translate DTLS versions. */
-    if (IS_DTLS(ss)) {
-        /* RFC 4347 required that you verify that the server versions
-         * match (Section 4.2.1) in the HelloVerifyRequest and the
-         * ServerHello.
-         *
-         * RFC 6347 suggests (SHOULD) that servers always use 1.0
-         * in HelloVerifyRequest and allows the versions not to match,
-         * especially when 1.2 is being negotiated.
-         *
-         * Therefore we do not check for matching here.
-         */
-        version = dtls_DTLSVersionToTLSVersion(version);
-        if (version == 0) { /* Insane version number */
-            goto alert_loser;
-        }
-    }
 
     /* We got a HelloRetryRequest, but the server didn't pick 1.3.  Scream. */
-    if (ss->ssl3.hs.helloRetry && version < SSL_LIBRARY_VERSION_TLS_1_3) {
+    if (ss->ssl3.hs.helloRetry && ss->version < SSL_LIBRARY_VERSION_TLS_1_3) {
         desc = illegal_parameter;
         errCode = SSL_ERROR_RX_MALFORMED_SERVER_HELLO;
         goto alert_loser;
     }
 
-    rv = ssl3_NegotiateVersion(ss, version, PR_FALSE);
-    if (rv != SECSuccess) {
-        desc = (version > SSL_LIBRARY_VERSION_3_0) ? protocol_version
-                                                   : handshake_failure;
-        errCode = SSL_ERROR_UNSUPPORTED_VERSION;
-        goto alert_loser;
-    }
     /* Check that the server negotiated the same version as it did
      * in the first handshake. This isn't really the best place for
      * us to be getting this version number, but it's what we have.
      * (1294697). */
-    if (ss->firstHsDone && (version != ss->ssl3.crSpec->version)) {
+    if (ss->firstHsDone && (ss->version != ss->ssl3.crSpec->version)) {
         desc = illegal_parameter;
         errCode = SSL_ERROR_UNSUPPORTED_VERSION;
         goto alert_loser;
     }
     ss->ssl3.hs.preliminaryInfo |= ssl_preinfo_version;
     isTLS = (ss->version > SSL_LIBRARY_VERSION_3_0);
 
     rv = ssl3_ConsumeHandshake(
--- a/lib/ssl/sslimpl.h
+++ b/lib/ssl/sslimpl.h
@@ -1711,16 +1711,19 @@ extern void ssl3_InitSocketPolicy(sslSoc
 
 extern SECStatus ssl3_RedoHandshake(sslSocket *ss, PRBool flushCache);
 extern SECStatus ssl3_HandleHandshakeMessage(sslSocket *ss, SSL3Opaque *b,
                                              PRUint32 length,
                                              PRBool endOfRecord);
 
 extern void ssl3_DestroySSL3Info(sslSocket *ss);
 
+extern SECStatus ssl_ClientReadVersion(sslSocket *ss, SSL3Opaque **b,
+                                       PRUint32 *length,
+                                       SSL3ProtocolVersion *version);
 extern SECStatus ssl3_NegotiateVersion(sslSocket *ss,
                                        SSL3ProtocolVersion peerVersion,
                                        PRBool allowLargerPeerVersion);
 
 extern SECStatus ssl_GetPeerInfo(sslSocket *ss);
 
 /* ECDH functions */
 extern SECStatus ssl3_SendECDHClientKeyExchange(sslSocket *ss,
--- a/lib/ssl/sslproto.h
+++ b/lib/ssl/sslproto.h
@@ -26,17 +26,17 @@
 #define SSL_LIBRARY_VERSION_DTLS_1_3            SSL_LIBRARY_VERSION_TLS_1_3
 
 /* deprecated old name */
 #define SSL_LIBRARY_VERSION_3_1_TLS SSL_LIBRARY_VERSION_TLS_1_0
 
 /* The DTLS versions used in the spec */
 #define SSL_LIBRARY_VERSION_DTLS_1_0_WIRE       ((~0x0100) & 0xffff)
 #define SSL_LIBRARY_VERSION_DTLS_1_2_WIRE       ((~0x0102) & 0xffff)
-#define SSL_LIBRARY_VERSION_DTLS_1_3_WIRE       0x0304
+#define SSL_LIBRARY_VERSION_DTLS_1_3_WIRE       SSL_LIBRARY_VERSION_DTLS_1_3
 
 /* Certificate types */
 #define SSL_CT_X509_CERTIFICATE                 0x01
 #if 0 /* XXX Not implemented yet */
 #define SSL_PKCS6_CERTIFICATE                   0x02
 #endif
 #define SSL_AT_MD5_WITH_RSA_ENCRYPTION          0x01
 
--- a/lib/ssl/tls13con.c
+++ b/lib/ssl/tls13con.c
@@ -1609,17 +1609,17 @@ tls13_SendCertificateRequest(sslSocket *
     return SECSuccess;
 }
 
 static SECStatus
 tls13_HandleHelloRetryRequest(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
 {
     SECStatus rv;
     PRInt32 tmp;
-    PRUint32 version;
+    SSL3ProtocolVersion version;
 
     SSL_TRC(3, ("%d: TLS13[%d]: handle hello retry request",
                 SSL_GETPID(), ss->fd));
 
     PORT_Assert(ss->opt.noLocks || ssl_HaveRecvBufLock(ss));
     PORT_Assert(ss->opt.noLocks || ssl_HaveSSL3HandshakeLock(ss));
 
     /* Client */
@@ -1639,27 +1639,28 @@ tls13_HandleHelloRetryRequest(sslSocket 
     if (ss->ssl3.hs.zeroRttState == ssl_0rtt_sent) {
         /* Oh well, back to the start. */
         tls13_SetNullCipherSpec(ss, &ss->ssl3.cwSpec);
         ss->ssl3.hs.zeroRttState = ssl_0rtt_ignored;
     } else {
         PORT_Assert(ss->ssl3.hs.zeroRttState == ssl_0rtt_none);
     }
 
-    tmp = ssl3_ConsumeHandshakeNumber(ss, 2, &b, &length);
-    if (tmp < 0) {
-        return SECFailure; /* error code already set */
-    }
-    version = tls13_DecodeDraftVersion((PRUint16)tmp);
+    /* Version. */
+    rv = ssl_ClientReadVersion(ss, &b, &length, &version);
+    if (rv != SECSuccess) {
+        return SECFailure; /* alert already sent */
+    }
     if (version > ss->vrange.max || version < SSL_LIBRARY_VERSION_TLS_1_3) {
         FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_HELLO_RETRY_REQUEST,
                     protocol_version);
         return SECFailure;
     }
 
+    /* Extensions. */
     tmp = ssl3_ConsumeHandshakeNumber(ss, 2, &b, &length);
     if (tmp < 0) {
         return SECFailure; /* error code already set */
     }
     /* Extensions must be non-empty and use the remainder of the message.
      * This means that a HelloRetryRequest cannot be a no-op: we must have an
      * extension, it must be one that we understand and recognize as being valid
      * for HelloRetryRequest, and all the extensions we permit cause us to
@@ -4197,33 +4198,24 @@ tls13_HandleEarlyApplicationData(sslSock
     PR_APPEND_LINK(&ed->link, &ss->ssl3.hs.bufferedEarlyData);
 
     origBuf->len = 0; /* So ssl3_GatherAppDataRecord will keep looping. */
 
     return SECSuccess;
 }
 
 PRUint16
-tls13_EncodeDraftVersion(PRUint16 version)
+tls13_EncodeDraftVersion(SSL3ProtocolVersion version)
 {
 #ifdef TLS_1_3_DRAFT_VERSION
-    return version == SSL_LIBRARY_VERSION_TLS_1_3 ? (0x7f00 | TLS_1_3_DRAFT_VERSION) : version;
-#else
-    return version;
+    if (version == SSL_LIBRARY_VERSION_TLS_1_3) {
+        return 0x7f00 | TLS_1_3_DRAFT_VERSION;
+    }
 #endif
-}
-
-PRUint16
-tls13_DecodeDraftVersion(PRUint16 version)
-{
-#ifdef TLS_1_3_DRAFT_VERSION
-    return version == (0x7f00 | TLS_1_3_DRAFT_VERSION) ? SSL_LIBRARY_VERSION_TLS_1_3 : version;
-#else
-    return version;
-#endif
+    return (PRUint16)version;
 }
 
 /* Pick the highest version we support that is also advertised. */
 SECStatus
 tls13_NegotiateVersion(sslSocket *ss, const TLSExtension *supported_versions)
 {
     PRUint16 version;
     /* Make a copy so we're nondestructive*/
--- a/lib/ssl/tls13con.h
+++ b/lib/ssl/tls13con.h
@@ -63,14 +63,14 @@ SECStatus tls13_ProtectRecord(sslSocket 
                               SSL3ContentType type,
                               const SSL3Opaque *pIn,
                               PRUint32 contentLen,
                               sslBuffer *wrBuf);
 PRInt32 tls13_Read0RttData(sslSocket *ss, void *buf, PRInt32 len);
 SECStatus tls13_HandleEndOfEarlyData(sslSocket *ss);
 SECStatus tls13_HandleEarlyApplicationData(sslSocket *ss, sslBuffer *origBuf);
 PRBool tls13_ClientAllow0Rtt(sslSocket *ss, const sslSessionID *sid);
-PRUint16 tls13_EncodeDraftVersion(PRUint16 version);
+PRUint16 tls13_EncodeDraftVersion(SSL3ProtocolVersion version);
 PRUint16 tls13_DecodeDraftVersion(PRUint16 version);
 SECStatus tls13_NegotiateVersion(sslSocket *ss,
                                  const TLSExtension *supported_versions);
 
 #endif /* __tls13con_h_ */