Test+fix handling "wrong" downgrade signals

This accounts for cases that can only occur when een non-compliant server sends
the wrong downgrade signal. (TLS1.1 signal when negotiating TLS1.2 or TLS1.2
signal when negotiating TLS1.0/TLS1.1). According to the TLS1.3 RFC these
cases should be rejected:

RFC8446, 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.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/27518)

diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index a23d83eacec20dddb4198742a0a3d43bd1494a5a..4bcefe79a52ef44ae70113929b1d43144111e604 100644 (file)
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -2365,23 +2365,24 @@ int ssl_choose_client_version(SSL_CONNECTION *s, int version,
         real_max = ver_max;
 
     /* Check for downgrades */
-    if (s->version == TLS1_2_VERSION && real_max > s->version) {
-        if (memcmp(tls12downgrade,
+    /* TODO(DTLSv1.3): Update this code for DTLSv1.3 */
+    if (!SSL_CONNECTION_IS_DTLS(s) && real_max > s->version) {
+        /* Signal applies to all versions */
+        if (memcmp(tls11downgrade,
                    s->s3.server_random + SSL3_RANDOM_SIZE
-                                        - sizeof(tls12downgrade),
-                   sizeof(tls12downgrade)) == 0) {
+                   - sizeof(tls11downgrade),
+                   sizeof(tls11downgrade)) == 0) {
             s->version = origv;
             SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER,
                      SSL_R_INAPPROPRIATE_FALLBACK);
             return 0;
         }
-    } else if (!SSL_CONNECTION_IS_DTLS(s)
-               && s->version < TLS1_2_VERSION
-               && real_max > s->version) {
-        if (memcmp(tls11downgrade,
-                   s->s3.server_random + SSL3_RANDOM_SIZE
-                                        - sizeof(tls11downgrade),
-                   sizeof(tls11downgrade)) == 0) {
+        /* Only when accepting TLS1.3 */
+        if (real_max == TLS1_3_VERSION
+            && memcmp(tls12downgrade,
+                      s->s3.server_random + SSL3_RANDOM_SIZE
+                      - sizeof(tls12downgrade),
+                      sizeof(tls12downgrade)) == 0) {
             s->version = origv;
             SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER,
                      SSL_R_INAPPROPRIATE_FALLBACK);

diff --git a/test/recipes/70-test_tls13downgrade.t b/test/recipes/70-test_tls13downgrade.t
index 9a5925c8e60faf95c5878b5acf9b42eaa1286262..601187c51905af88c312d60216ae3b77854e9c04 100644 (file)
--- a/test/recipes/70-test_tls13downgrade.t
+++ b/test/recipes/70-test_tls13downgrade.t
@@ -38,16 +38,25 @@ use constant {
     DOWNGRADE_TO_TLS_1_2 => 0,
     DOWNGRADE_TO_TLS_1_1 => 1,
     FALLBACK_FROM_TLS_1_3 => 2,
+    DOWNGRADE_TO_TLS_1_2_WITH_TLS_1_1_SIGNAL => 3,
+    DOWNGRADE_TO_TLS_1_1_WITH_TLS_1_2_SIGNAL => 4,
 };
 
 #Test 1: Downgrade from TLSv1.3 to TLSv1.2
 $proxy->filter(\&downgrade_filter);
 my $testtype = DOWNGRADE_TO_TLS_1_2;
 $proxy->start() or plan skip_all => "Unable to start up Proxy for tests";
-plan tests => 6;
+plan tests => 8;
 ok(is_illegal_parameter_client_alert(), "Downgrade TLSv1.3 to TLSv1.2");
 
-#Test 2: Client falls back from TLSv1.3 (server does not support the fallback
+#Test 2: Downgrade from TLSv1.3 to TLSv1.2 (server sends TLSv1.1 signal)
+$proxy->clear();
+$testtype = DOWNGRADE_TO_TLS_1_2_WITH_TLS_1_1_SIGNAL;
+$proxy->start();
+ok(is_illegal_parameter_client_alert(),
+   "Downgrade from TLSv1.3 to TLSv1.2 (server sends TLSv1.1 signal)");
+
+#Test 3: Client falls back from TLSv1.3 (server does not support the fallback
 #        SCSV)
 $proxy->clear();
 $testtype = FALLBACK_FROM_TLS_1_3;
@@ -56,13 +65,13 @@ $proxy->start();
 ok(is_illegal_parameter_client_alert(), "Fallback from TLSv1.3");
 
 SKIP: {
-    skip "TLSv1.1 disabled", 4 if disabled("tls1_1");
+    skip "TLSv1.1 disabled", 5 if disabled("tls1_1");
 
     my $client_flags = "-min_protocol TLSv1.1 -cipher DEFAULT:\@SECLEVEL=0";
     my $server_flags = "-min_protocol TLSv1.1";
     my $ciphers = "AES128-SHA:\@SECLEVEL=0";
 
-    #Test 3: Downgrade from TLSv1.3 to TLSv1.1
+    #Test 4: Downgrade from TLSv1.3 to TLSv1.1
     $proxy->clear();
     $testtype = DOWNGRADE_TO_TLS_1_1;
     $proxy->clientflags($client_flags);
@@ -71,7 +80,17 @@ SKIP: {
     $proxy->start();
     ok(is_illegal_parameter_client_alert(), "Downgrade TLSv1.3 to TLSv1.1");
 
-    #Test 4: Downgrade from TLSv1.2 to TLSv1.1
+    #Test 5: Downgrade from TLSv1.3 to TLSv1.1 (server sends TLSv1.2 signal)
+    $proxy->clear();
+    $testtype = DOWNGRADE_TO_TLS_1_1_WITH_TLS_1_2_SIGNAL;
+    $proxy->clientflags($client_flags);
+    $proxy->serverflags($server_flags);
+    $proxy->ciphers($ciphers);
+    $proxy->start();
+    ok(is_illegal_parameter_client_alert(),
+       "Downgrade TLSv1.3 to TLSv1.1 (server sends TLSv1.2 signal)");
+
+    #Test 6: Downgrade from TLSv1.2 to TLSv1.1
     $proxy->clear();
     $testtype = DOWNGRADE_TO_TLS_1_1;
     $proxy->clientflags($client_flags." -max_protocol TLSv1.2");
@@ -80,7 +99,7 @@ SKIP: {
     $proxy->start();
     ok(is_illegal_parameter_client_alert(), "Downgrade TLSv1.2 to TLSv1.1");
 
-    #Test 5: A client side protocol "hole" should not be detected as a downgrade
+    #Test 7: A client side protocol "hole" should not be detected as a downgrade
     $proxy->clear();
     $proxy->filter(undef);
     $proxy->clientflags($client_flags." -no_tls1_2");
@@ -89,7 +108,7 @@ SKIP: {
     $proxy->start();
     ok(TLSProxy::Message->success(), "TLSv1.2 client-side protocol hole");
 
-    #Test 6: A server side protocol "hole" should not be detected as a downgrade
+    #Test 8: A server side protocol "hole" should not be detected as a downgrade
     $proxy->clear();
     $proxy->filter(undef);
     $proxy->clientflags($client_flags);
@@ -115,33 +134,54 @@ sub downgrade_filter
 {
     my $proxy = shift;
 
-    # We're only interested in the initial ClientHello
-    if ($proxy->flight != 0) {
+    # We're only interested in the initial ClientHello and ServerHello
+    if ($proxy->flight > 1) {
         return;
     }
 
-    my $message = ${$proxy->message_list}[0];
-
-    my $ext;
-    if ($testtype == FALLBACK_FROM_TLS_1_3) {
-        #The default ciphersuite we use for TLSv1.2 without any SCSV
-        my @ciphersuites = (TLSProxy::Message::CIPHER_RSA_WITH_AES_128_CBC_SHA);
-        $message->ciphersuite_len(2 * scalar @ciphersuites);
-        $message->ciphersuites(\@ciphersuites);
-    } else {
-        if ($testtype == DOWNGRADE_TO_TLS_1_2) {
-            $ext = pack "C3",
-                0x02, # Length
-                0x03, 0x03; #TLSv1.2
-        } else {
-            $ext = pack "C3",
-                0x02, # Length
-                0x03, 0x02; #TLSv1.1
+    my $message = ${$proxy->message_list}[$proxy->flight];
+
+    # ServerHello
+    if ($proxy->flight == 1 && defined($message)) {
+        # Update the last byte of the downgrade signal
+        if ($testtype == DOWNGRADE_TO_TLS_1_2_WITH_TLS_1_1_SIGNAL) {
+            $message->random(substr($message->random, 0, 31) . "\0");
+            $message->repack();
+        } elsif ($testtype == DOWNGRADE_TO_TLS_1_1_WITH_TLS_1_2_SIGNAL) {
+            $message->random(substr($message->random, 0, 31) . "\1");
+            $message->repack();
         }
 
-        $message->set_extension(TLSProxy::Message::EXT_SUPPORTED_VERSIONS, $ext);
+        return;
     }
 
-    $message->repack();
+    # ClientHello
+    if ($proxy->flight == 0) {
+        my $ext;
+        if ($testtype == FALLBACK_FROM_TLS_1_3) {
+            #The default ciphersuite we use for TLSv1.2 without any SCSV
+            my @ciphersuites = (TLSProxy::Message::CIPHER_RSA_WITH_AES_128_CBC_SHA);
+            $message->ciphersuite_len(2 * scalar @ciphersuites);
+            $message->ciphersuites(\@ciphersuites);
+        }
+        else {
+            if ($testtype == DOWNGRADE_TO_TLS_1_2
+                || $testtype == DOWNGRADE_TO_TLS_1_2_WITH_TLS_1_1_SIGNAL) {
+                $ext = pack "C3",
+                    0x02,       # Length
+                    0x03, 0x03; #TLSv1.2
+            }
+            else {
+                $ext = pack "C3",
+                    0x02,       # Length
+                    0x03, 0x02; #TLSv1.1
+            }
+
+            $message->set_extension(TLSProxy::Message::EXT_SUPPORTED_VERSIONS,
+                                    $ext);
+        }
+
+        $message->repack();
+    }
 }