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>

3 files changed, 28 insertions, 10 deletions

diff --git a/util/TLSProxy/ClientHello.pm b/util/TLSProxy/ClientHello.pm
index 72661129ae..383062842c 100644
--- 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 ddd0a6d3a8..bbb0ad7061 100644
--- 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 487538ab12..7cf753508d 100644
--- 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));