Bug 1307772 - Require that the server use the draft version, r=ekr

--HG--
extra : source : d0ed57c57e9875c9ff43d521a6774a38b3bd7e88
extra : amend_source : 42cf245ff6285d4d54bbe8c4ce1d200dc279a07f

external_tests/ssl_gtest/ssl_agent_unittest.cc

@@ -8,6 +8,9 @@
 #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"
@@ -18,6 +21,7 @@
 
 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,
@@ -34,13 +38,13 @@ const static uint8_t kCannedTls13ClientHello[] = {
     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,
@@ -71,35 +75,23 @@ TEST_P(TlsAgentTestClient, CannedHello) {
   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();
@@ -110,21 +102,18 @@ TEST_P(TlsAgentTestClient, EncryptedExtensionsInClear) {
 }
 
 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);
 
@@ -137,28 +126,24 @@ TEST_F(TlsAgentStreamTestClient, EncryptedExtensionsInClearTwoPieces) {
 }
 
 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);
 

external_tests/ssl_gtest/ssl_hrr_unittest.cc

@@ -9,6 +9,9 @@
 #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"
@@ -196,8 +199,7 @@ class HelloRetryRequestAgentTest : public TlsAgentTestClient {
     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);

lib/ssl/dtlscon.c

@@ -999,7 +999,7 @@ 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",
@@ -1013,17 +1013,24 @@ dtls_HandleHelloVerifyRequest(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
         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. */

lib/ssl/ssl3con.c

@@ -1054,6 +1054,52 @@ ssl3_NegotiateVersion(sslSocket *ss, SSL3ProtocolVersion peerVersion,
     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)
 {
@@ -6468,7 +6514,6 @@ ssl3_HandleServerHello(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
     SECItem sidBytes = { siBuffer, NULL, 0 };
     PRBool isTLS = PR_FALSE;
     SSL3AlertDescription desc = illegal_parameter;
-    SSL3ProtocolVersion version;
 #ifndef TLS_1_3_DRAFT_VERSION
     SSL3ProtocolVersion downgradeCheckVersion;
 #endif
@@ -6499,49 +6544,23 @@ ssl3_HandleServerHello(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
         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;

lib/ssl/sslimpl.h

@@ -1716,6 +1716,9 @@ extern SECStatus ssl3_HandleHandshakeMessage(sslSocket *ss, SSL3Opaque *b,
 
 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);

lib/ssl/sslproto.h

@@ -31,7 +31,7 @@
 /* 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

lib/ssl/tls13con.c

@@ -1614,7 +1614,7 @@ 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));
@@ -1644,17 +1644,18 @@ tls13_HandleHelloRetryRequest(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
         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. */
+    rv = ssl_ClientReadVersion(ss, &b, &length, &version);
+    if (rv != SECSuccess) {
+        return SECFailure; /* alert already sent */
     }
-    version = tls13_DecodeDraftVersion((PRUint16)tmp);
     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 */
@@ -4202,23 +4203,14 @@ tls13_HandleEarlyApplicationData(sslSocket *ss, sslBuffer *origBuf)
 }
 
 PRUint16
-tls13_EncodeDraftVersion(PRUint16 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;
-#endif
-}
-
-PRUint16
-tls13_DecodeDraftVersion(PRUint16 version)
+tls13_EncodeDraftVersion(SSL3ProtocolVersion 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;
+    if (version == SSL_LIBRARY_VERSION_TLS_1_3) {
+        return 0x7f00 | TLS_1_3_DRAFT_VERSION;
+    }
 #endif
+    return (PRUint16)version;
 }
 
 /* Pick the highest version we support that is also advertised. */