summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--include/openssl/ssl.h1
-rw-r--r--ssl/ssl_err.c2
-rw-r--r--ssl/t1_lib.c84
-rwxr-xr-xtest/recipes/70-test_sslcertstatus.t2
-rwxr-xr-xtest/recipes/70-test_sslextension.t59
-rwxr-xr-xtest/recipes/70-test_sslsessiontick.t4
-rw-r--r--test/recipes/70-test_tlsextms.t6
-rw-r--r--util/TLSProxy/ClientHello.pm21
-rw-r--r--util/TLSProxy/Message.pm10
-rw-r--r--util/TLSProxy/ServerHello.pm7
10 files changed, 175 insertions, 21 deletions
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 36d17dd224..9709103d5f 100644
--- 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 e6b9bbdb9c..46f483febe 100644
--- 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 7a2047dcca..db5f0f6b44 100644
--- 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 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 {
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));
View Lorry Controller Status