TLS: reject duplicate extensions

Adapted from BoringSSL. Added a test.

The extension parsing code is already attempting to already handle this for
some individual extensions, but it is doing so inconsistently. Duplicate
efforts in individual extension parsing will be cleaned up in a follow-up.

Reviewed-by: Stephen Henson <steve@openssl.org>

diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 36d17dd224f21bc6effb4ad66bc6551f759af3af..9709103d5fb984dda3003a7e0cd77460b1ceb2d6 100644 (file)
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -2123,6 +2123,7 @@ void ERR_load_SSL_strings(void);
 # define SSL_F_STATE_MACHINE                              353
 # define SSL_F_TLS12_CHECK_PEER_SIGALG                    333
 # define SSL_F_TLS1_CHANGE_CIPHER_STATE                   209
+# define SSL_F_TLS1_CHECK_DUPLICATE_EXTENSIONS            341
 # define SSL_F_TLS1_CHECK_SERVERHELLO_TLSEXT              274
 # define SSL_F_TLS1_EXPORT_KEYING_MATERIAL                314
 # define SSL_F_TLS1_GET_CURVELIST                         338

diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c
index e6b9bbdb9cc813c7e9cfe447cdd8d441088aaece..46f483febe29b1270efcb248761fd4253c9f3581 100644 (file)
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -273,6 +273,8 @@ static ERR_STRING_DATA SSL_str_functs[] = {
     {ERR_FUNC(SSL_F_STATE_MACHINE), "state_machine"},
     {ERR_FUNC(SSL_F_TLS12_CHECK_PEER_SIGALG), "tls12_check_peer_sigalg"},
     {ERR_FUNC(SSL_F_TLS1_CHANGE_CIPHER_STATE), "tls1_change_cipher_state"},
+    {ERR_FUNC(SSL_F_TLS1_CHECK_DUPLICATE_EXTENSIONS),
+     "tls1_check_duplicate_extensions"},
     {ERR_FUNC(SSL_F_TLS1_CHECK_SERVERHELLO_TLSEXT),
      "TLS1_CHECK_SERVERHELLO_TLSEXT"},
     {ERR_FUNC(SSL_F_TLS1_EXPORT_KEYING_MATERIAL),

diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 7a2047dcca1e5c7d676fc7b9238f2ff5fda79d57..db5f0f6b442a86b535a79276fccf491a8df8e410 100644 (file)
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -109,6 +109,7 @@
  */
 
 #include <stdio.h>
+#include <stdlib.h>
 #include <openssl/objects.h>
 #include <openssl/evp.h>
 #include <openssl/hmac.h>
@@ -124,7 +125,7 @@ static int tls_decrypt_ticket(SSL *s, const unsigned char *tick, int ticklen,
                               const unsigned char *sess_id, int sesslen,
                               SSL_SESSION **psess);
 static int ssl_check_clienthello_tlsext_early(SSL *s);
-int ssl_check_serverhello_tlsext(SSL *s);
+static int ssl_check_serverhello_tlsext(SSL *s);
 
 SSL3_ENC_METHOD const TLSv1_enc_data = {
     tls1_enc,
@@ -1037,6 +1038,79 @@ static int tls_use_ticket(SSL *s)
     return ssl_security(s, SSL_SECOP_TICKET, 0, 0, NULL);
 }
 
+static int compare_uint(const void *p1, const void *p2) {
+    unsigned int u1 = *((const unsigned int *)p1);
+    unsigned int u2 = *((const unsigned int *)p2);
+    if (u1 < u2)
+        return -1;
+    else if (u1 > u2)
+        return 1;
+    else
+        return 0;
+}
+
+/*
+ * Per http://tools.ietf.org/html/rfc5246#section-7.4.1.4, there may not be
+ * more than one extension of the same type in a ClientHello or ServerHello.
+ * This function does an initial scan over the extensions block to filter those
+ * out. It returns 1 if all extensions are unique, and 0 if the extensions
+ * contain duplicates, could not be successfully parsed, or an internal error
+ * occurred.
+ */
+static int tls1_check_duplicate_extensions(const PACKET *packet) {
+    PACKET extensions = *packet;
+    size_t num_extensions = 0, i = 0;
+    unsigned int *extension_types = NULL;
+    int ret = 0;
+
+    /* First pass: count the extensions. */
+    while (PACKET_remaining(&extensions) > 0) {
+        unsigned int type;
+        PACKET extension;
+        if (!PACKET_get_net_2(&extensions, &type) ||
+            !PACKET_get_length_prefixed_2(&extensions, &extension)) {
+            goto done;
+        }
+        num_extensions++;
+    }
+
+    if (num_extensions <= 1)
+        return 1;
+
+    extension_types = OPENSSL_malloc(sizeof(unsigned int) * num_extensions);
+    if (extension_types == NULL) {
+        SSLerr(SSL_F_TLS1_CHECK_DUPLICATE_EXTENSIONS, ERR_R_MALLOC_FAILURE);
+        goto done;
+    }
+
+    /* Second pass: gather the extension types. */
+    extensions = *packet;
+    for (i = 0; i < num_extensions; i++) {
+        PACKET extension;
+        if (!PACKET_get_net_2(&extensions, &extension_types[i]) ||
+            !PACKET_get_length_prefixed_2(&extensions, &extension)) {
+            /* This should not happen. */
+            SSLerr(SSL_F_TLS1_CHECK_DUPLICATE_EXTENSIONS, ERR_R_INTERNAL_ERROR);
+            goto done;
+        }
+    }
+
+    if (PACKET_remaining(&extensions) != 0) {
+        SSLerr(SSL_F_TLS1_CHECK_DUPLICATE_EXTENSIONS, ERR_R_INTERNAL_ERROR);
+        goto done;
+    }
+    /* Sort the extensions and make sure there are no duplicates. */
+    qsort(extension_types, num_extensions, sizeof(unsigned int), compare_uint);
+    for (i = 1; i < num_extensions; i++) {
+        if (extension_types[i - 1] == extension_types[i])
+            goto done;
+    }
+    ret = 1;
+ done:
+    OPENSSL_free(extension_types);
+    return ret;
+}
+
 unsigned char *ssl_add_clienthello_tlsext(SSL *s, unsigned char *buf,
                                           unsigned char *limit, int *al)
 {
@@ -1837,6 +1911,9 @@ static int ssl_scan_clienthello_tlsext(SSL *s, PACKET *pkt, int *al)
     if (PACKET_remaining(pkt) != len)
         goto err;
 
+    if (!tls1_check_duplicate_extensions(pkt))
+        goto err;
+
     while (PACKET_get_net_2(pkt, &type) && PACKET_get_net_2(pkt, &size)) {
         PACKET subpkt;
 
@@ -2301,6 +2378,11 @@ static int ssl_scan_serverhello_tlsext(SSL *s, PACKET *pkt, int *al)
         return 0;
     }
 
+    if (!tls1_check_duplicate_extensions(pkt)) {
+        *al = SSL_AD_DECODE_ERROR;
+        return 0;
+    }
+
     while (PACKET_get_net_2(pkt, &type) && PACKET_get_net_2(pkt, &size)) {
         const unsigned char *data;
         PACKET spkt;

diff --git a/test/recipes/70-test_sslcertstatus.t b/test/recipes/70-test_sslcertstatus.t
index 9a0c5f8eac406535b350c0f808eba6e6c861e299..303de5e56635cfec038868c7f711c24766723862 100755 (executable)
--- a/test/recipes/70-test_sslcertstatus.t
+++ b/test/recipes/70-test_sslcertstatus.t
@@ -99,7 +99,7 @@ sub certstatus_filter
         if ($message->mt == TLSProxy::Message::MT_SERVER_HELLO) {
             #Add the status_request to the ServerHello even though we are not
             #going to send a CertificateStatus message
-            $message->set_extension(TLSProxy::ClientHello::EXT_STATUS_REQUEST,
+            $message->set_extension(TLSProxy::Message::EXT_STATUS_REQUEST,
                                     "");
 
             $message->repack();

diff --git a/test/recipes/70-test_sslextension.t b/test/recipes/70-test_sslextension.t
index 4582c5c72087aa13815812192b9336f90906a5c7..c253f748e0d84dc4d50877336ec19a53f83fc2c6 100755 (executable)
--- a/test/recipes/70-test_sslextension.t
+++ b/test/recipes/70-test_sslextension.t
@@ -78,9 +78,9 @@ my $proxy = TLSProxy::Proxy->new(
     (!$ENV{HARNESS_ACTIVE} || $ENV{HARNESS_VERBOSE})
 );
 
-plan tests => 1;
+plan tests => 3;
 
-#Test 1: Sending a zero length extension block should pass
+# Test 1: Sending a zero length extension block should pass
 $proxy->start();
 ok(TLSProxy::Message->success, "Zero extension length test");
 
@@ -95,13 +95,64 @@ sub extension_filter
 
     foreach my $message (@{$proxy->message_list}) {
         if ($message->mt == TLSProxy::Message::MT_CLIENT_HELLO) {
-            #Remove all extensions and set the extension len to zero
+            # Remove all extensions and set the extension len to zero
             $message->extension_data({});
             $message->extensions_len(0);
-            #Extensions have been removed so make sure we don't try to use them
+            # Extensions have been removed so make sure we don't try to use them
             $message->process_extensions();
 
             $message->repack();
         }
     }
 }
+
+# Test 2-3: Sending a duplicate extension should fail.
+sub inject_duplicate_extension
+{
+  my ($proxy, $message_type) = @_;
+
+    foreach my $message (@{$proxy->message_list}) {
+        if ($message->mt == $message_type) {
+          my %extensions = %{$message->extension_data};
+            # Add a duplicate (unknown) extension.
+            $message->set_extension(TLSProxy::Message::EXT_DUPLICATE_EXTENSION, "");
+            $message->set_extension(TLSProxy::Message::EXT_DUPLICATE_EXTENSION, "");
+            $message->repack();
+        }
+    }
+}
+
+sub inject_duplicate_extension_clienthello
+{
+    my $proxy = shift;
+
+    # We're only interested in the initial ClientHello
+    if ($proxy->flight != 0) {
+        return;
+    }
+
+    inject_duplicate_extension($proxy, TLSProxy::Message::MT_CLIENT_HELLO);
+}
+
+sub inject_duplicate_extension_serverhello
+{
+    my $proxy = shift;
+
+    # We're only interested in the initial ServerHello
+    if ($proxy->flight != 1) {
+        return;
+    }
+
+    inject_duplicate_extension($proxy, TLSProxy::Message::MT_SERVER_HELLO);
+}
+
+$proxy->clear();
+$proxy->filter(\&inject_duplicate_extension_clienthello);
+$proxy->start();
+ok(TLSProxy::Message->fail(), "Duplicate ClientHello extension");
+
+$proxy->clear();
+$proxy->filter(\&inject_duplicate_extension_serverhello);
+$proxy->start();
+ok(TLSProxy::Message->fail(), "Duplicate ServerHello extension");
+

diff --git a/test/recipes/70-test_sslsessiontick.t b/test/recipes/70-test_sslsessiontick.t
index 8a361fd80387e4f119ccc6ae7c0edb00b3196a23..4e9c85f82fc47cc5b43a388a86f99b2cc71cc950 100755 (executable)
--- a/test/recipes/70-test_sslsessiontick.t
+++ b/test/recipes/70-test_sslsessiontick.t
@@ -198,7 +198,7 @@ sub inject_empty_ticket_filter {
     foreach my $message (@{$proxy->message_list}) {
         push @new_message_list, $message;
         if ($message->mt == TLSProxy::Message::MT_SERVER_HELLO) {
-            $message->set_extension(TLSProxy::ClientHello::EXT_SESSION_TICKET, "");
+            $message->set_extension(TLSProxy::Message::EXT_SESSION_TICKET, "");
             $message->repack();
             # Tack NewSessionTicket onto the ServerHello record.
             # This only works if the ServerHello is exactly one record.
@@ -226,7 +226,7 @@ sub checkmessages($$$$$$)
                #Get the extensions data
                my %extensions = %{$message->extension_data};
                if (defined
-                    $extensions{TLSProxy::ClientHello::EXT_SESSION_TICKET}) {
+                    $extensions{TLSProxy::Message::EXT_SESSION_TICKET}) {
                    if ($message->mt == TLSProxy::Message::MT_CLIENT_HELLO) {
                        $chellotickext = 1;
                    } else {

diff --git a/test/recipes/70-test_tlsextms.t b/test/recipes/70-test_tlsextms.t
index 82cb85676d18151a71f7c6529fc08b1d3ffdac48..022b3a8d6c5f643b8192ef801ac2aa2123761257 100644 (file)
--- a/test/recipes/70-test_tlsextms.t
+++ b/test/recipes/70-test_tlsextms.t
@@ -215,11 +215,11 @@ sub extms_filter
 
     foreach my $message (@{$proxy->message_list}) {
         if ($crmextms && $message->mt == TLSProxy::Message::MT_CLIENT_HELLO) {
-            $message->delete_extension(TLSProxy::ClientHello::EXT_EXTENDED_MASTER_SECRET);
+            $message->delete_extension(TLSProxy::Message::EXT_EXTENDED_MASTER_SECRET);
             $message->repack();
         }
         if ($srmextms && $message->mt == TLSProxy::Message::MT_SERVER_HELLO) {
-            $message->delete_extension(TLSProxy::ClientHello::EXT_EXTENDED_MASTER_SECRET);
+            $message->delete_extension(TLSProxy::Message::EXT_EXTENDED_MASTER_SECRET);
             $message->repack();
         }
     }
@@ -237,7 +237,7 @@ sub checkmessages($$$$$)
         #Get the extensions data
         my %extensions = %{$message->extension_data};
         if (defined
-            $extensions{TLSProxy::ClientHello::EXT_EXTENDED_MASTER_SECRET}) {
+            $extensions{TLSProxy::Message::EXT_EXTENDED_MASTER_SECRET}) {
             if ($message->mt == TLSProxy::Message::MT_CLIENT_HELLO) {
                 $cextms = 1;
             } else {

diff --git a/util/TLSProxy/ClientHello.pm b/util/TLSProxy/ClientHello.pm
index 72661129ae0f6dcc23d01f02d8320f7000e9b84f..383062842c8da49b1b50631144af097bf50b61a7 100644 (file)
--- a/util/TLSProxy/ClientHello.pm
+++ b/util/TLSProxy/ClientHello.pm
@@ -57,13 +57,6 @@ package TLSProxy::ClientHello;
 
 use parent 'TLSProxy::Message';
 
-use constant {
-    EXT_STATUS_REQUEST => 5,
-    EXT_ENCRYPT_THEN_MAC => 22,
-    EXT_EXTENDED_MASTER_SECRET => 23,
-    EXT_SESSION_TICKET => 35
-};
-
 sub new
 {
     my $class = shift;
@@ -90,7 +83,7 @@ sub new
     $self->{comp_meth_len} = 0;
     $self->{comp_meths} = [];
     $self->{extensions_len} = 0;
-    $self->{extensions_data} = "";
+    $self->{extension_data} = "";
 
     return $self;
 }
@@ -161,7 +154,7 @@ sub process_extensions
     #Clear any state from a previous run
     TLSProxy::Record->etm(0);
 
-    if (exists $extensions{&EXT_ENCRYPT_THEN_MAC}) {
+    if (exists $extensions{TLSProxy::Message::EXT_ENCRYPT_THEN_MAC}) {
         TLSProxy::Record->etm(1);
     }
 }
@@ -187,6 +180,11 @@ sub set_message_contents
         $extensions .= pack("n", $key);
         $extensions .= pack("n", length($extdata));
         $extensions .= $extdata;
+        if ($key == TLSProxy::Message::EXT_DUPLICATE_EXTENSION) {
+          $extensions .= pack("n", $key);
+          $extensions .= pack("n", length($extdata));
+          $extensions .= $extdata;
+        }
     }
 
     $data .= pack('n', length($extensions));
@@ -276,6 +274,11 @@ sub extension_data
     }
     return $self->{extension_data};
 }
+sub set_extension
+{
+    my ($self, $ext_type, $ext_data) = @_;
+    $self->{extension_data}{$ext_type} = $ext_data;
+}
 sub delete_extension
 {
     my ($self, $ext_type) = @_;

diff --git a/util/TLSProxy/Message.pm b/util/TLSProxy/Message.pm
index ddd0a6d3a840e4059be9fef94f0076da551e8b9a..bbb0ad70619a14038db824e001c51714c3ba293f 100644 (file)
--- a/util/TLSProxy/Message.pm
+++ b/util/TLSProxy/Message.pm
@@ -101,6 +101,16 @@ my %message_type = (
     MT_NEXT_PROTO, "NextProto"
 );
 
+use constant {
+    EXT_STATUS_REQUEST => 5,
+    EXT_ENCRYPT_THEN_MAC => 22,
+    EXT_EXTENDED_MASTER_SECRET => 23,
+    EXT_SESSION_TICKET => 35,
+    # This extension does not exist and isn't recognised by OpenSSL.
+    # We use it to test handling of duplicate extensions.
+    EXT_DUPLICATE_EXTENSION => 1234
+};
+
 my $payload = "";
 my $messlen = -1;
 my $mt;

diff --git a/util/TLSProxy/ServerHello.pm b/util/TLSProxy/ServerHello.pm
index 487538ab1286fcfe631b19f46bf663cb8b4159bf..7cf753508d1dda62473ddb8a2a0e9309fb8ff87a 100644 (file)
--- a/util/TLSProxy/ServerHello.pm
+++ b/util/TLSProxy/ServerHello.pm
@@ -80,7 +80,7 @@ sub new
     $self->{session} = "";
     $self->{ciphersuite} = 0;
     $self->{comp_meth} = 0;
-    $self->{extensions_data} = "";
+    $self->{extension_data} = "";
 
     return $self;
 }
@@ -161,6 +161,11 @@ sub set_message_contents
         $extensions .= pack("n", $key);
         $extensions .= pack("n", length($extdata));
         $extensions .= $extdata;
+        if ($key == TLSProxy::Message::EXT_DUPLICATE_EXTENSION) {
+          $extensions .= pack("n", $key);
+          $extensions .= pack("n", length($extdata));
+          $extensions .= $extdata;
+        }
     }
 
     $data .= pack('n', length($extensions));