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/27535)

diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index 6f0eaa5d6c0d21f8a5f1a1aed2edfe5b9666402e..edc4e8122f669302095c4d2d7e6a09b7d52cf2ca 100644 (file)
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -1967,23 +1967,24 @@ int ssl_choose_client_version(SSL *s, int version, RAW_EXTENSION *extensions)
         real_max = ver_max;
 
     /* Check for downgrades */
-    if (s->version == TLS1_2_VERSION && real_max > s->version) {
-        if (memcmp(tls12downgrade,
+    if (!SSL_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_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 d2de096a54f44fcb01153dd5e59e92896a03fe0f..1a510c811958b6afd5f5b3ca40eeaf21ea1394a9 100644 (file)
--- a/test/recipes/70-test_tls13downgrade.t
+++ b/test/recipes/70-test_tls13downgrade.t
@@ -40,16 +40,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;
@@ -58,13 +67,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);
@@ -73,7 +82,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");
@@ -82,7 +101,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");
@@ -91,7 +110,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);
@@ -117,33 +136,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();
+    }
 }