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>

4 files changed, 61 insertions, 10 deletions

diff --git a/test/recipes/70-test_sslcertstatus.t b/test/recipes/70-test_sslcertstatus.t
index 9a0c5f8eac..303de5e566 100755
--- 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 4582c5c720..c253f748e0 100755
--- 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 8a361fd803..4e9c85f82f 100755
--- 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 82cb85676d..022b3a8d6c 100644
--- 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 {