Bug 1483129 - TLS 1.3 RFC version, r=ekr

This retains the ability to negotiate draft versions of DTLS 1.3, but uses the
final RFC version for TLS 1.3.

This also refactors the handling of the downgrade sentinel.  As we've discovered
- to our dismay - some MitM boxes forward handshake messages when they
shouldn't.  This could result in triggering the downgrade sentinel.  I've done
two things here:

- The server always sets the sentinel.  It reduces the assumed version if it
  only supports a draft version though on the basis that the client might
  expect the full version.

- The client has a new option SSL_ENABLE_HELLO_DOWNGRADE_CHECK which is disabled
  by default.  The client will reject a handshake that appears to be a downgrade
  only when this is explicitly enabled.  The client will allow an apparent
  downgrade to TLS 1.2 if it is running a draft version of TLS 1.3.

The allowance for a draft version is now only effective for DTLS 1.3.

Tests for version downgrade have been updated and enabled.  These were rotten in
a few ways, but nothing dramatic.

8 files changed, 149 insertions, 120 deletions

diff --git a/lib/ssl/ssl.h b/lib/ssl/ssl.h
index ecc4f9506..edc011447 100644
--- a/lib/ssl/ssl.h
+++ b/lib/ssl/ssl.h
@@ -282,6 +282,15 @@ SSL_IMPORT PRFileDesc *DTLS_ImportFD(PRFileDesc *model, PRFileDesc *fd);
  */
 #define SSL_ENABLE_DTLS_SHORT_HEADER 36
 
+/*
+ * Enables the processing of the downgrade sentinel that can be added to the
+ * ServerHello.random by a server that supports Section 4.1.3 of TLS 1.3
+ * [RFC8446].  This sentinel will always be generated by a server that
+ * negotiates a version lower than its maximum, this only controls whether a
+ * client will treat receipt of a value that indicates a downgrade as an error.
+ */
+#define SSL_ENABLE_HELLO_DOWNGRADE_CHECK 37
+
 #ifdef SSL_DEPRECATED_FUNCTION
 /* Old deprecated function names */
 SSL_IMPORT SECStatus SSL_Enable(PRFileDesc *fd, int option, PRIntn on);
diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
index 96127e782..8d8fcfa1e 100644
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -6414,9 +6414,6 @@ ssl3_HandleServerHello(sslSocket *ss, PRUint8 *b, PRUint32 length)
     SSL3AlertDescription desc = illegal_parameter;
     const PRUint8 *savedMsg = b;
     const PRUint32 savedLength = length;
-#ifndef TLS_1_3_DRAFT_VERSION
-    SSL3ProtocolVersion downgradeCheckVersion;
-#endif
 
     SSL_TRC(3, ("%d: SSL3[%d]: handle server_hello handshake",
                 SSL_GETPID(), ss->fd));
@@ -6553,39 +6550,43 @@ ssl3_HandleServerHello(sslSocket *ss, PRUint8 *b, PRUint32 length)
         goto alert_loser;
     }
 
-#ifndef TLS_1_3_DRAFT_VERSION
-    /* Check the ServerHello.random per
-     * [draft-ietf-tls-tls13-11 Section 6.3.1.1].
-     *
-     * TLS 1.3 clients receiving a TLS 1.2 or below ServerHello MUST check
-     * that the top eight octets are not equal to either of these values.
-     * TLS 1.2 clients SHOULD also perform this check if the ServerHello
-     * indicates TLS 1.1 or below.  If a match is found the client MUST
-     * abort the handshake with a fatal "illegal_parameter" alert.
-     *
-     * Disable this test during the TLS 1.3 draft version period.
-     */
-    downgradeCheckVersion = ss->ssl3.downgradeCheckVersion ? ss->ssl3.downgradeCheckVersion
-                                                           : ss->vrange.max;
-
-    if (downgradeCheckVersion >= SSL_LIBRARY_VERSION_TLS_1_2 &&
-        downgradeCheckVersion > ss->version) {
-        /* Both sections use the same sentinel region. */
-        PRUint8 *downgrade_sentinel =
-            ss->ssl3.hs.server_random +
-            SSL3_RANDOM_LENGTH - sizeof(tls13_downgrade_random);
-        if (!PORT_Memcmp(downgrade_sentinel,
-                         tls13_downgrade_random,
-                         sizeof(tls13_downgrade_random)) ||
-            !PORT_Memcmp(downgrade_sentinel,
-                         tls12_downgrade_random,
-                         sizeof(tls12_downgrade_random))) {
-            desc = illegal_parameter;
-            errCode = SSL_ERROR_RX_MALFORMED_SERVER_HELLO;
-            goto alert_loser;
+    /* Disable this check for DTLS while we're on draft versions. */
+    if (ss->opt.enableHelloDowngradeCheck
+#ifdef DTLS_1_3_DRAFT_VERSION
+        && !IS_DTLS(ss)
+#endif
+            ) {
+        /* Check the ServerHello.random per [RFC 8446 Section 4.1.3].
+         *
+         * TLS 1.3 clients receiving a ServerHello indicating TLS 1.2 or below
+         * MUST check that the last 8 bytes are not equal to either of these
+         * values.  TLS 1.2 clients SHOULD also check that the last 8 bytes are
+         * not equal to the second value if the ServerHello indicates TLS 1.1 or
+         * below.  If a match is found, the client MUST abort the handshake with
+         * an "illegal_parameter" alert.
+         */
+        SSL3ProtocolVersion checkVersion =
+            ss->ssl3.downgradeCheckVersion ? ss->ssl3.downgradeCheckVersion
+                                           : ss->vrange.max;
+
+        if (checkVersion >= SSL_LIBRARY_VERSION_TLS_1_2 &&
+            checkVersion > ss->version) {
+            /* Both sections use the same sentinel region. */
+            PRUint8 *downgrade_sentinel =
+                ss->ssl3.hs.server_random +
+                SSL3_RANDOM_LENGTH - sizeof(tls13_downgrade_random);
+            if (!PORT_Memcmp(downgrade_sentinel,
+                             tls13_downgrade_random,
+                             sizeof(tls13_downgrade_random)) ||
+                !PORT_Memcmp(downgrade_sentinel,
+                             tls12_downgrade_random,
+                             sizeof(tls12_downgrade_random))) {
+                desc = illegal_parameter;
+                errCode = SSL_ERROR_RX_MALFORMED_SERVER_HELLO;
+                goto alert_loser;
+            }
         }
     }
-#endif
 
     /* Finally, now all the version-related checks have passed. */
     ss->ssl3.hs.preliminaryInfo |= ssl_preinfo_version;
@@ -8098,6 +8099,62 @@ ssl3_SelectServerCert(sslSocket *ss)
     return SECFailure;
 }
 
+static SECStatus
+ssl_GenerateServerRandom(sslSocket *ss)
+{
+    SECStatus rv = ssl3_GetNewRandom(ss->ssl3.hs.server_random);
+    if (rv != SECSuccess) {
+        return SECFailure;
+    }
+
+    if (ss->version == ss->vrange.max) {
+        return SECSuccess;
+    }
+#ifdef DTLS_1_3_DRAFT_VERSION
+    if (IS_DTLS(ss)) {
+        return SECSuccess;
+    }
+#endif
+
+    /*
+     * [RFC 8446 Section 4.1.3].
+     *
+     * TLS 1.3 servers which negotiate TLS 1.2 or below in response to a
+     * ClientHello MUST set the last 8 bytes of their Random value specially in
+     * their ServerHello.
+     *
+     * If negotiating TLS 1.2, TLS 1.3 servers MUST set the last 8 bytes of
+     * their Random value to the bytes:
+     *
+     *   44 4F 57 4E 47 52 44 01
+     *
+     * If negotiating TLS 1.1 or below, TLS 1.3 servers MUST, and TLS 1.2
+     * servers SHOULD, set the last 8 bytes of their ServerHello.Random value to
+     * the bytes:
+     *
+     *   44 4F 57 4E 47 52 44 00
+     */
+    PRUint8 *downgradeSentinel =
+        ss->ssl3.hs.server_random +
+        SSL3_RANDOM_LENGTH - sizeof(tls13_downgrade_random);
+
+    switch (ss->vrange.max) {
+        case SSL_LIBRARY_VERSION_TLS_1_3:
+            PORT_Memcpy(downgradeSentinel,
+                        tls13_downgrade_random, sizeof(tls13_downgrade_random));
+            break;
+        case SSL_LIBRARY_VERSION_TLS_1_2:
+            PORT_Memcpy(downgradeSentinel,
+                        tls12_downgrade_random, sizeof(tls12_downgrade_random));
+            break;
+        default:
+            /* Do not change random. */
+            break;
+    }
+
+    return SECSuccess;
+}
+
 /* Called from ssl3_HandleHandshakeMessage() when it has deciphered a complete
  * ssl3 Client Hello message.
  * Caller must hold Handshake and RecvBuf locks.
@@ -8292,56 +8349,6 @@ ssl3_HandleClientHello(sslSocket *ss, PRUint8 *b, PRUint32 length)
         }
     }
 
-    /* Generate the Server Random now so it is available
-     * when we process the ClientKeyShare in TLS 1.3 */
-    rv = ssl3_GetNewRandom(ss->ssl3.hs.server_random);
-    if (rv != SECSuccess) {
-        errCode = SSL_ERROR_GENERATE_RANDOM_FAILURE;
-        goto loser;
-    }
-
-#ifndef TLS_1_3_DRAFT_VERSION
-    /*
-     * [draft-ietf-tls-tls13-11 Section 6.3.1.1].
-     * TLS 1.3 server implementations which respond to a ClientHello with a
-     * client_version indicating TLS 1.2 or below MUST set the last eight
-     * bytes of their Random value to the bytes:
-     *
-     * 44 4F 57 4E 47 52 44 01
-     *
-     * TLS 1.2 server implementations which respond to a ClientHello with a
-     * client_version indicating TLS 1.1 or below SHOULD set the last eight
-     * bytes of their Random value to the bytes:
-     *
-     * 44 4F 57 4E 47 52 44 00
-     *
-     * TODO(ekr@rtfm.com): Note this change was not added in the SSLv2
-     * compat processing code since that will most likely be removed before
-     * we ship the final version of TLS 1.3. Bug 1306672.
-     */
-    if (ss->vrange.max > ss->version) {
-        PRUint8 *downgrade_sentinel =
-            ss->ssl3.hs.server_random +
-            SSL3_RANDOM_LENGTH - sizeof(tls13_downgrade_random);
-
-        switch (ss->vrange.max) {
-            case SSL_LIBRARY_VERSION_TLS_1_3:
-                PORT_Memcpy(downgrade_sentinel,
-                            tls13_downgrade_random,
-                            sizeof(tls13_downgrade_random));
-                break;
-            case SSL_LIBRARY_VERSION_TLS_1_2:
-                PORT_Memcpy(downgrade_sentinel,
-                            tls12_downgrade_random,
-                            sizeof(tls12_downgrade_random));
-                break;
-            default:
-                /* Do not change random. */
-                break;
-        }
-    }
-#endif
-
     /* If there is a cookie, then this is a second ClientHello (TLS 1.3). */
     if (ssl3_FindExtension(ss, ssl_tls13_cookie_xtn)) {
         ss->ssl3.hs.helloRetry = PR_TRUE;
@@ -9088,6 +9095,7 @@ ssl_ConstructServerHello(sslSocket *ss, PRBool helloRetry,
     SECStatus rv;
     SSL3ProtocolVersion version;
     sslSessionID *sid = ss->sec.ci.sid;
+    const PRUint8 *random;
 
     version = PR_MIN(ss->version, SSL_LIBRARY_VERSION_TLS_1_2);
     if (IS_DTLS(ss)) {
@@ -9097,9 +9105,17 @@ ssl_ConstructServerHello(sslSocket *ss, PRBool helloRetry,
     if (rv != SECSuccess) {
         return SECFailure;
     }
-    /* Random already generated in ssl3_HandleClientHello */
-    rv = sslBuffer_Append(messageBuf, helloRetry ? ssl_hello_retry_random : ss->ssl3.hs.server_random,
-                          SSL3_RANDOM_LENGTH);
+
+    if (helloRetry) {
+        random = ssl_hello_retry_random;
+    } else {
+        rv = ssl_GenerateServerRandom(ss);
+        if (rv != SECSuccess) {
+            return SECFailure;
+        }
+        random = ss->ssl3.hs.server_random;
+    }
+    rv = sslBuffer_Append(messageBuf, random, SSL3_RANDOM_LENGTH);
     if (rv != SECSuccess) {
         return SECFailure;
     }
diff --git a/lib/ssl/ssl3prot.h b/lib/ssl/ssl3prot.h
index f46b9cfe5..bfaa10d3f 100644
--- a/lib/ssl/ssl3prot.h
+++ b/lib/ssl/ssl3prot.h
@@ -13,10 +13,8 @@
 typedef PRUint16 SSL3ProtocolVersion;
 /* version numbers are defined in sslproto.h */
 
-/* The TLS 1.3 draft version. Used to avoid negotiating
- * between incompatible pre-standard TLS 1.3 drafts.
- * TODO(ekr@rtfm.com): Remove when TLS 1.3 is published. */
-#define TLS_1_3_DRAFT_VERSION 28
+/* DTLS 1.3 is still a draft. */
+#define DTLS_1_3_DRAFT_VERSION 28
 
 typedef PRUint16 ssl3CipherSuite;
 /* The cipher suites are defined in sslproto.h */
diff --git a/lib/ssl/sslimpl.h b/lib/ssl/sslimpl.h
index 33765a1e3..4230c6c47 100644
--- a/lib/ssl/sslimpl.h
+++ b/lib/ssl/sslimpl.h
@@ -266,6 +266,7 @@ typedef struct sslOptionsStr {
     unsigned int enable0RttData : 1;
     unsigned int enableTls13CompatMode : 1;
     unsigned int enableDtlsShortHeader : 1;
+    unsigned int enableHelloDowngradeCheck : 1;
 } sslOptions;
 
 typedef enum { sslHandshakingUndetermined = 0,
diff --git a/lib/ssl/sslsock.c b/lib/ssl/sslsock.c
index 33595ffae..1f599b929 100644
--- a/lib/ssl/sslsock.c
+++ b/lib/ssl/sslsock.c
@@ -82,7 +82,8 @@ static sslOptions ssl_defaults = {
     .requireDHENamedGroups = PR_FALSE,
     .enable0RttData = PR_FALSE,
     .enableTls13CompatMode = PR_FALSE,
-    .enableDtlsShortHeader = PR_FALSE
+    .enableDtlsShortHeader = PR_FALSE,
+    .enableHelloDowngradeCheck = PR_FALSE
 };
 
 /*
@@ -821,6 +822,10 @@ SSL_OptionSet(PRFileDesc *fd, PRInt32 which, PRIntn val)
             ss->opt.enableDtlsShortHeader = val;
             break;
 
+        case SSL_ENABLE_HELLO_DOWNGRADE_CHECK:
+            ss->opt.enableHelloDowngradeCheck = val;
+            break;
+
         default:
             PORT_SetError(SEC_ERROR_INVALID_ARGS);
             rv = SECFailure;
@@ -963,6 +968,9 @@ SSL_OptionGet(PRFileDesc *fd, PRInt32 which, PRIntn *pVal)
         case SSL_ENABLE_DTLS_SHORT_HEADER:
             val = ss->opt.enableDtlsShortHeader;
             break;
+        case SSL_ENABLE_HELLO_DOWNGRADE_CHECK:
+            val = ss->opt.enableHelloDowngradeCheck;
+            break;
         default:
             PORT_SetError(SEC_ERROR_INVALID_ARGS);
             rv = SECFailure;
@@ -1089,6 +1097,9 @@ SSL_OptionGetDefault(PRInt32 which, PRIntn *pVal)
         case SSL_ENABLE_DTLS_SHORT_HEADER:
             val = ssl_defaults.enableDtlsShortHeader;
             break;
+        case SSL_ENABLE_HELLO_DOWNGRADE_CHECK:
+            val = ssl_defaults.enableHelloDowngradeCheck;
+            break;
         default:
             PORT_SetError(SEC_ERROR_INVALID_ARGS);
             rv = SECFailure;
@@ -1284,6 +1295,10 @@ SSL_OptionSetDefault(PRInt32 which, PRIntn val)
             ssl_defaults.enableDtlsShortHeader = val;
             break;
 
+        case SSL_ENABLE_HELLO_DOWNGRADE_CHECK:
+            ssl_defaults.enableHelloDowngradeCheck = val;
+            break;
+
         default:
             PORT_SetError(SEC_ERROR_INVALID_ARGS);
             return SECFailure;
diff --git a/lib/ssl/tls13con.c b/lib/ssl/tls13con.c
index 3a10ef9d6..1194c0d23 100644
--- a/lib/ssl/tls13con.c
+++ b/lib/ssl/tls13con.c
@@ -5273,11 +5273,12 @@ tls13_HandleEarlyApplicationData(sslSocket *ss, sslBuffer *origBuf)
 }
 
 PRUint16
-tls13_EncodeDraftVersion(SSL3ProtocolVersion version)
+tls13_EncodeDraftVersion(SSL3ProtocolVersion version, SSLProtocolVariant variant)
 {
-#ifdef TLS_1_3_DRAFT_VERSION
-    if (version == SSL_LIBRARY_VERSION_TLS_1_3) {
-        return 0x7f00 | TLS_1_3_DRAFT_VERSION;
+#ifdef DTLS_1_3_DRAFT_VERSION
+    if (version == SSL_LIBRARY_VERSION_TLS_1_3 &&
+        variant == ssl_variant_datagram) {
+        return 0x7f00 | DTLS_1_3_DRAFT_VERSION;
     }
 #endif
     return (PRUint16)version;
@@ -5287,7 +5288,6 @@ SECStatus
 tls13_ClientReadSupportedVersion(sslSocket *ss)
 {
     PRUint32 temp;
-    SSL3ProtocolVersion v;
     TLSExtension *versionExtension;
     SECItem it;
     SECStatus rv;
@@ -5309,29 +5309,15 @@ tls13_ClientReadSupportedVersion(sslSocket *ss)
         FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_SERVER_HELLO, illegal_parameter);
         return SECFailure;
     }
-    v = (SSL3ProtocolVersion)temp;
 
-    /* You cannot negotiate < TLS 1.3 with supported_versions. */
-    if (v < SSL_LIBRARY_VERSION_TLS_1_3) {
+    if (temp != tls13_EncodeDraftVersion(SSL_LIBRARY_VERSION_TLS_1_3,
+                                         ss->protocolVariant)) {
+        /* You cannot negotiate < TLS 1.3 with supported_versions. */
         FATAL_ERROR(ss, SSL_ERROR_RX_MALFORMED_SERVER_HELLO, illegal_parameter);
         return SECFailure;
     }
 
-#ifdef TLS_1_3_DRAFT_VERSION
-    if (temp == SSL_LIBRARY_VERSION_TLS_1_3) {
-        FATAL_ERROR(ss, SSL_ERROR_UNSUPPORTED_VERSION, protocol_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
-
-    ss->version = v;
+    ss->version = SSL_LIBRARY_VERSION_TLS_1_3;
     return SECSuccess;
 }
 
@@ -5355,7 +5341,7 @@ tls13_NegotiateVersion(sslSocket *ss, const TLSExtension *supportedVersions)
         return SECFailure;
     }
     for (version = ss->vrange.max; version >= ss->vrange.min; --version) {
-        PRUint16 wire = tls13_EncodeDraftVersion(version);
+        PRUint16 wire = tls13_EncodeDraftVersion(version, ss->protocolVariant);
         unsigned long offset;
 
         for (offset = 0; offset < versions.len; offset += 2) {
diff --git a/lib/ssl/tls13con.h b/lib/ssl/tls13con.h
index 03a28b9f7..87428c317 100644
--- a/lib/ssl/tls13con.h
+++ b/lib/ssl/tls13con.h
@@ -101,7 +101,8 @@ SECStatus tls13_ProtectRecord(sslSocket *ss,
 PRInt32 tls13_Read0RttData(sslSocket *ss, void *buf, PRInt32 len);
 SECStatus tls13_HandleEarlyApplicationData(sslSocket *ss, sslBuffer *origBuf);
 PRBool tls13_ClientAllow0Rtt(const sslSocket *ss, const sslSessionID *sid);
-PRUint16 tls13_EncodeDraftVersion(SSL3ProtocolVersion version);
+PRUint16 tls13_EncodeDraftVersion(SSL3ProtocolVersion version,
+                                  SSLProtocolVariant variant);
 SECStatus tls13_ClientReadSupportedVersion(sslSocket *ss);
 SECStatus tls13_NegotiateVersion(sslSocket *ss,
                                  const TLSExtension *supported_versions);
diff --git a/lib/ssl/tls13exthandle.c b/lib/ssl/tls13exthandle.c
index 1ab8a8e59..a4b2967e5 100644
--- a/lib/ssl/tls13exthandle.c
+++ b/lib/ssl/tls13exthandle.c
@@ -751,7 +751,9 @@ tls13_ClientSendSupportedVersionsXtn(const sslSocket *ss, TLSExtensionData *xtnD
     }
 
     for (version = ss->vrange.max; version >= ss->vrange.min; --version) {
-        rv = sslBuffer_AppendNumber(buf, tls13_EncodeDraftVersion(version), 2);
+        PRUint16 wire = tls13_EncodeDraftVersion(version,
+                                                 ss->protocolVariant);
+        rv = sslBuffer_AppendNumber(buf, wire, 2);
         if (rv != SECSuccess) {
             return SECFailure;
         }
@@ -779,8 +781,9 @@ tls13_ServerSendSupportedVersionsXtn(const sslSocket *ss, TLSExtensionData *xtnD
     SSL_TRC(3, ("%d: TLS13[%d]: server send supported_versions extension",
                 SSL_GETPID(), ss->fd));
 
-    rv = sslBuffer_AppendNumber(
-        buf, tls13_EncodeDraftVersion(SSL_LIBRARY_VERSION_TLS_1_3), 2);
+    PRUint16 ver = tls13_EncodeDraftVersion(SSL_LIBRARY_VERSION_TLS_1_3,
+                                            ss->protocolVariant);
+    rv = sslBuffer_AppendNumber(buf, ver, 2);
     if (rv != SECSuccess) {
         return SECFailure;
     }