From d3b574fee1c4ad887a219fadb1674349ae0ce4b7 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Fri, 4 Jan 2019 16:55:15 +0000 Subject: [PATCH] Add a test for correct handling of the cryptopro bug extension This was complicated by the fact that we were using this extension for our duplicate extension handling tests. In order to add tests for cryptopro bug the duplicate extension handling tests needed to change first. Reviewed-by: Paul Dale (Merged from https://github.com/openssl/openssl/pull/7984) (cherry picked from commit 9effc496ad8a9b0ec737c69cc0fddf610a045ea4) --- test/recipes/70-test_sslextension.t | 32 ++++++++++++++++++++--- util/perl/TLSProxy/Certificate.pm | 5 ---- util/perl/TLSProxy/ClientHello.pm | 7 ++--- util/perl/TLSProxy/EncryptedExtensions.pm | 5 ---- util/perl/TLSProxy/Message.pm | 16 ++++++++---- util/perl/TLSProxy/ServerHello.pm | 2 +- 6 files changed, 42 insertions(+), 25 deletions(-) diff --git a/test/recipes/70-test_sslextension.t b/test/recipes/70-test_sslextension.t index 20e1933f00..525de06f2a 100644 --- a/test/recipes/70-test_sslextension.t +++ b/test/recipes/70-test_sslextension.t @@ -88,9 +88,11 @@ sub inject_duplicate_extension 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, ""); + # Add a duplicate extension. We use cryptopro_bug since we never + # normally write that one, and it is allowed as unsolicited in the + # ServerHello + $message->set_extension(TLSProxy::Message::EXT_CRYPTOPRO_BUG_EXTENSION, ""); + $message->dupext(TLSProxy::Message::EXT_CRYPTOPRO_BUG_EXTENSION); $message->repack(); } } @@ -173,9 +175,23 @@ sub inject_unsolicited_extension $sent_unsolisited_extension = 1; } +sub inject_cryptopro_extension +{ + my $proxy = shift; + + # We're only interested in the initial ClientHello + if ($proxy->flight != 0) { + return; + } + + my $message = ${$proxy->message_list}[0]; + $message->set_extension(TLSProxy::Message::EXT_CRYPTOPRO_BUG_EXTENSION, ""); + $message->repack(); +} + # Test 1-2: Sending a duplicate extension should fail. $proxy->start() or plan skip_all => "Unable to start up Proxy for tests"; -plan tests => 7; +plan tests => 8; ok($fatal_alert, "Duplicate ClientHello extension"); $fatal_alert = 0; @@ -234,3 +250,11 @@ SKIP: { $proxy->start(); ok($fatal_alert, "Unsolicited server name extension (TLSv1.3)"); } + +#Test 8: Send the cryptopro extension in a ClientHello. Normally this is an +# unsolicited extension only ever seen in the ServerHello. We should +# ignore it in a ClientHello +$proxy->clear(); +$proxy->filter(\&inject_cryptopro_extension); +$proxy->start(); +ok(TLSProxy::Message->success(), "Cryptopro extension in ClientHello"); diff --git a/util/perl/TLSProxy/Certificate.pm b/util/perl/TLSProxy/Certificate.pm index d3bf7f2180..a415897352 100644 --- a/util/perl/TLSProxy/Certificate.pm +++ b/util/perl/TLSProxy/Certificate.pm @@ -138,11 +138,6 @@ 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('C', length($self->context())); $data .= $self->context; diff --git a/util/perl/TLSProxy/ClientHello.pm b/util/perl/TLSProxy/ClientHello.pm index 2ae9d6f55d..ec4846966c 100644 --- a/util/perl/TLSProxy/ClientHello.pm +++ b/util/perl/TLSProxy/ClientHello.pm @@ -124,11 +124,6 @@ sub extension_contents $extension .= pack("n", $key); $extension .= pack("n", length($extdata)); $extension .= $extdata; - if ($key == TLSProxy::Message::EXT_DUPLICATE_EXTENSION) { - $extension .= pack("n", $key); - $extension .= pack("n", length($extdata)); - $extension .= $extdata; - } return $extension; } @@ -151,6 +146,8 @@ sub set_message_contents foreach my $key (keys %{$self->extension_data}) { next if ($key == TLSProxy::Message::EXT_PSK); $extensions .= $self->extension_contents($key); + #Add extension twice if we are duplicating that extension + $extensions .= $self->extension_contents($key) if ($key == $self->dupext); } #PSK extension always goes last... if (defined ${$self->extension_data}{TLSProxy::Message::EXT_PSK}) { diff --git a/util/perl/TLSProxy/EncryptedExtensions.pm b/util/perl/TLSProxy/EncryptedExtensions.pm index 81242e29ff..cd529eed8e 100644 --- a/util/perl/TLSProxy/EncryptedExtensions.pm +++ b/util/perl/TLSProxy/EncryptedExtensions.pm @@ -81,11 +81,6 @@ 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)); diff --git a/util/perl/TLSProxy/Message.pm b/util/perl/TLSProxy/Message.pm index 16ed012066..ee507f9e21 100644 --- a/util/perl/TLSProxy/Message.pm +++ b/util/perl/TLSProxy/Message.pm @@ -86,10 +86,7 @@ use constant { EXT_SIG_ALGS_CERT => 50, EXT_RENEGOTIATE => 65281, EXT_NPN => 13172, - # This extension is an unofficial extension only ever written by OpenSSL - # (i.e. not read), and even then only when enabled. We use it to test - # handling of duplicate extensions. - EXT_DUPLICATE_EXTENSION => 0xfde8, + EXT_CRYPTOPRO_BUG_EXTENSION => 0xfde8, EXT_UNKNOWN => 0xfffe, #Unknown extension that should appear last EXT_FORCE_LAST => 0xffff @@ -420,7 +417,8 @@ sub new records => $records, mt => $mt, startoffset => $startoffset, - message_frag_lens => $message_frag_lens + message_frag_lens => $message_frag_lens, + dupext => -1 }; return bless $self, $class; @@ -575,6 +573,14 @@ sub encoded_length my $self = shift; return TLS_MESSAGE_HEADER_LENGTH + length($self->data); } +sub dupext +{ + my $self = shift; + if (@_) { + $self->{dupext} = shift; + } + return $self->{dupext}; +} sub successondata { my $class = shift; diff --git a/util/perl/TLSProxy/ServerHello.pm b/util/perl/TLSProxy/ServerHello.pm index 84f2faab05..c95bbeeaeb 100644 --- a/util/perl/TLSProxy/ServerHello.pm +++ b/util/perl/TLSProxy/ServerHello.pm @@ -154,7 +154,7 @@ sub set_message_contents $extensions .= pack("n", $key); $extensions .= pack("n", length($extdata)); $extensions .= $extdata; - if ($key == TLSProxy::Message::EXT_DUPLICATE_EXTENSION) { + if ($key == $self->dupext) { $extensions .= pack("n", $key); $extensions .= pack("n", length($extdata)); $extensions .= $extdata; -- GitLab