Bug 1249939 - Improve TLS 1.3 draft extension handling, r=ekr

3 files changed, 73 insertions, 10 deletions

diff --git a/external_tests/ssl_gtest/ssl_extension_unittest.cc b/external_tests/ssl_gtest/ssl_extension_unittest.cc
index 59d05ba8b..a9e235e36 100644
--- a/external_tests/ssl_gtest/ssl_extension_unittest.cc
+++ b/external_tests/ssl_gtest/ssl_extension_unittest.cc
@@ -5,6 +5,7 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "ssl.h"
+#include "sslerr.h"
 #include "sslproto.h"
 
 #include <memory>
@@ -15,6 +16,21 @@
 
 namespace nss_test {
 
+class TlsExtensionDropper : public TlsExtensionFilter {
+ public:
+  TlsExtensionDropper(uint16_t extension)
+      : extension_(extension) {}
+  virtual PacketFilter::Action FilterExtension(
+      uint16_t extension_type, const DataBuffer&, DataBuffer*) {
+    if (extension_type == extension_) {
+      return DROP;
+    }
+    return KEEP;
+  }
+ private:
+    uint16_t extension_;
+};
+
 class TlsExtensionTruncator : public TlsExtensionFilter {
  public:
   TlsExtensionTruncator(uint16_t extension, size_t length)
@@ -603,6 +619,42 @@ TEST_P(TlsExtensionTest13, EmptyClientKeyShare) {
                        kTlsAlertHandshakeFailure);
 }
 
+TEST_P(TlsExtensionTest13, DropDraftVersion) {
+  EnsureTlsSetup();
+  client_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2,
+                           SSL_LIBRARY_VERSION_TLS_1_3);
+  server_->SetVersionRange(SSL_LIBRARY_VERSION_TLS_1_2,
+                           SSL_LIBRARY_VERSION_TLS_1_3);
+  client_->SetPacketFilter(
+      new TlsExtensionDropper(ssl_tls13_draft_version_xtn));
+  ConnectExpectFail();
+  // This will still fail (we can't just modify ClientHello without consequence)
+  // but the error is discovered later.
+  EXPECT_EQ(SSL_ERROR_DECRYPT_ERROR_ALERT, client_->error_code());
+  EXPECT_EQ(SSL_ERROR_BAD_HANDSHAKE_HASH_VALUE, server_->error_code());
+}
+
+TEST_P(TlsExtensionTest13, DropDraftVersionAndFail) {
+  EnsureTlsSetup();
+  // Since this is setup as TLS 1.3 only, expect the handshake to fail rather
+  // than just falling back to TLS 1.2.
+  client_->SetPacketFilter(
+      new TlsExtensionDropper(ssl_tls13_draft_version_xtn));
+  ConnectExpectFail();
+  EXPECT_EQ(SSL_ERROR_PROTOCOL_VERSION_ALERT, client_->error_code());
+  EXPECT_EQ(SSL_ERROR_UNSUPPORTED_VERSION, server_->error_code());
+}
+
+TEST_P(TlsExtensionTest13, ModifyDraftVersionAndFail) {
+  EnsureTlsSetup();
+  // As above, dropping back to 1.2 fails.
+  client_->SetPacketFilter(
+      new TlsExtensionDamager(ssl_tls13_draft_version_xtn, 1));
+  ConnectExpectFail();
+  EXPECT_EQ(SSL_ERROR_PROTOCOL_VERSION_ALERT, client_->error_code());
+  EXPECT_EQ(SSL_ERROR_UNSUPPORTED_VERSION, server_->error_code());
+}
+
 INSTANTIATE_TEST_CASE_P(ExtensionStream, TlsExtensionTestGeneric,
                         ::testing::Combine(
                           TlsConnectTestBase::kTlsModesStream,
diff --git a/lib/ssl/ssl3con.c b/lib/ssl/ssl3con.c
index 3654edbdd..2635ac18b 100644
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -8714,6 +8714,22 @@ ssl3_HandleClientHello(sslSocket *ss, SSL3Opaque *b, PRUint32 length)
         }
     }
 
+    /* TODO: For TLS 1.3 final, downgrade when the extension IS present. */
+    if (ss->version == SSL_LIBRARY_VERSION_TLS_1_3 &&
+        !ssl3_ExtensionNegotiated(ss, ssl_tls13_draft_version_xtn)) {
+        SSL_TRC(30, ("%d: SSL3[%d]: Unsupported version of TLS 1.3 "
+                     "advertised, expected %d",
+                     SSL_GETPID(), ss->fd, TLS_1_3_DRAFT_VERSION));
+        ss->version = SSL_LIBRARY_VERSION_TLS_1_2;
+        /* Maybe TLS 1.2 is disabled... */
+        if (ss->version < ss->vrange.min) {
+            desc = protocol_version;
+            level = alert_fatal;
+            errCode = SSL_ERROR_UNSUPPORTED_VERSION;
+            goto alert_loser;
+        }
+    }
+
     if (!ssl3_ExtensionNegotiated(ss, ssl_renegotiation_info_xtn)) {
         /* If we didn't receive an RI extension, look for the SCSV,
          * and if found, treat it just like an empty RI extension
diff --git a/lib/ssl/ssl3ext.c b/lib/ssl/ssl3ext.c
index 07d114ee4..27f15eeab 100644
--- a/lib/ssl/ssl3ext.c
+++ b/lib/ssl/ssl3ext.c
@@ -2789,19 +2789,14 @@ ssl3_ServerHandleDraftVersionXtn(sslSocket *ss, PRUint16 ex_type,
         return SECFailure;
     }
 
-    /*  Keep track of negotiated extensions. */
-    ss->xtnData.negotiated[ss->xtnData.numNegotiated++] = ex_type;
-
-    if (draft_version != TLS_1_3_DRAFT_VERSION) {
-        /*
-         * Incompatible/broken TLS 1.3 implementation. Fall back to TLS 1.2.
-         * TODO(ekr@rtfm.com): It's not entirely clear it's safe to roll back
-         * here. Need to double-check.
-         */
+    if (draft_version == TLS_1_3_DRAFT_VERSION) {
+        /* Mark this as negotiated only if the version matches.  The code in
+         * ssl3_HandleClientHello will then reduce the version if needed. */
+        ss->xtnData.negotiated[ss->xtnData.numNegotiated++] = ex_type;
+    } else {
         SSL_TRC(30, ("%d: SSL3[%d]: Incompatible version of TLS 1.3 (%d), "
                      "expected %d",
                      SSL_GETPID(), ss->fd, draft_version, TLS_1_3_DRAFT_VERSION));
-        ss->version = SSL_LIBRARY_VERSION_TLS_1_2;
     }
 
     return SECSuccess;