From d50177fdf379a828c5daccf0385ab84cafeabc80 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 16 May 2017 17:53:03 +0200 Subject: [PATCH] Fix FALLBACK_SCSV parsing Fixed a bug in ssl_srv.c when parsing TLS_FALLBACK_SCSV in the ciphersuite list that caused it to miss it sometimes. Reported by Hugo Leisink as issue #810. Fix initially by @andreasag01; this commit isolates the bug fix and adds a non-regression test. --- ChangeLog | 6 +++ library/ssl_srv.c | 2 +- tests/scripts/tcp_client.pl | 86 +++++++++++++++++++++++++++++++++++++ tests/ssl-opt.sh | 33 ++++++++++++++ 4 files changed, 126 insertions(+), 1 deletion(-) create mode 100755 tests/scripts/tcp_client.pl diff --git a/ChangeLog b/ChangeLog index 13de8672c..594253a45 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,11 @@ mbed TLS ChangeLog (Sorted per branch, date) += mbed TLS 2.x.x branch released xxxx-xx-xx + +Security + * Fixed offset in FALLBACK_SCSV parsing that caused TLS server to fail to + detect it sometimes. Reported by Hugo Leisink. #810 + = mbed TLS 2.4.2 branch released 2017-03-08 Security diff --git a/library/ssl_srv.c b/library/ssl_srv.c index fc0d2d7b4..8a264aaca 100644 --- a/library/ssl_srv.c +++ b/library/ssl_srv.c @@ -1700,7 +1700,7 @@ read_record_header: #endif #if defined(MBEDTLS_SSL_FALLBACK_SCSV) - for( i = 0, p = buf + 41 + sess_len; i < ciph_len; i += 2, p += 2 ) + for( i = 0, p = buf + ciph_offset + 2; i < ciph_len; i += 2, p += 2 ) { if( p[0] == (unsigned char)( ( MBEDTLS_SSL_FALLBACK_SCSV_VALUE >> 8 ) & 0xff ) && p[1] == (unsigned char)( ( MBEDTLS_SSL_FALLBACK_SCSV_VALUE ) & 0xff ) ) diff --git a/tests/scripts/tcp_client.pl b/tests/scripts/tcp_client.pl new file mode 100755 index 000000000..11cbf1b1b --- /dev/null +++ b/tests/scripts/tcp_client.pl @@ -0,0 +1,86 @@ +#!/usr/bin/env perl + +# A simple TCP client that sends some data and expects a response. +# Usage: tcp_client.pl HOSTNAME PORT DATA1 RESPONSE1 +# DATA: hex-encoded data to send to the server +# RESPONSE: regexp that must match the server's response + +use warnings; +use strict; +use IO::Socket::INET; + +# Pack hex digits into a binary string, ignoring whitespace. +sub parse_hex { + my ($hex) = @_; + $hex =~ s/\s+//g; + return pack('H*', $hex); +} + +## Open a TCP connection to the specified host and port. +sub open_connection { + my ($host, $port) = @_; + my $socket = IO::Socket::INET->new(PeerAddr => $host, + PeerPort => $port, + Proto => 'tcp', + Timeout => 1); + die "Cannot connect to $host:$port: $!" unless $socket; + return $socket; +} + +## Close the TCP connection. +sub close_connection { + my ($connection) = @_; + $connection->shutdown(2); + # Ignore shutdown failures (at least for now) + return 1; +} + +## Write the given data, expressed as hexadecimal +sub write_data { + my ($connection, $hexdata) = @_; + my $data = parse_hex($hexdata); + my $total_sent = 0; + while ($total_sent < length($data)) { + my $sent = $connection->send($data, 0); + if (!defined $sent) { + die "Unable to send data: $!"; + } + $total_sent += $sent; + } + return 1; +} + +## Read a response and check it against an expected prefix +sub read_response { + my ($connection, $expected_hex) = @_; + my $expected_data = parse_hex($expected_hex); + my $start_offset = 0; + while ($start_offset < length($expected_data)) { + my $actual_data; + my $ok = $connection->recv($actual_data, length($expected_data)); + if (!defined $ok) { + die "Unable to receive data: $!"; + } + if (($actual_data ^ substr($expected_data, $start_offset)) =~ /[^\000]/) { + printf STDERR ("Received \\x%02x instead of \\x%02x at offset %d\n", + ord(substr($actual_data, $-[0], 1)), + ord(substr($expected_data, $start_offset + $-[0], 1)), + $start_offset + $-[0]); + return 0; + } + $start_offset += length($actual_data); + } + return 1; +} + +if (@ARGV != 4) { + print STDERR "Usage: $0 HOSTNAME PORT DATA1 RESPONSE1\n"; + exit(3); +} +my ($host, $port, $data1, $response1) = @ARGV; +my $connection = open_connection($host, $port); +write_data($connection, $data1); +if (!read_response($connection, $response1)) { + exit(1); +} +close_connection($connection); diff --git a/tests/ssl-opt.sh b/tests/ssl-opt.sh index 41fbc3d29..c6649a3ea 100755 --- a/tests/ssl-opt.sh +++ b/tests/ssl-opt.sh @@ -28,11 +28,13 @@ set -u : ${OPENSSL_CMD:=openssl} # OPENSSL would conflict with the build system : ${GNUTLS_CLI:=gnutls-cli} : ${GNUTLS_SERV:=gnutls-serv} +: ${PERL:=perl} O_SRV="$OPENSSL_CMD s_server -www -cert data_files/server5.crt -key data_files/server5.key" O_CLI="echo 'GET / HTTP/1.0' | $OPENSSL_CMD s_client" G_SRV="$GNUTLS_SERV --x509certfile data_files/server5.crt --x509keyfile data_files/server5.key" G_CLI="echo 'GET / HTTP/1.0' | $GNUTLS_CLI --x509cafile data_files/test-ca_cat12.crt" +TCP_CLIENT="$PERL scripts/tcp_client.pl" TESTS=0 FAILS=0 @@ -965,6 +967,37 @@ run_test "Fallback SCSV: enabled, max version, openssl client" \ -s "received FALLBACK_SCSV" \ -S "inapropriate fallback" +## ClientHello generated with +## "openssl s_client -CAfile tests/data_files/test-ca.crt -tls1_1 -connect localhost:4433 -cipher ..." +## then manually twiddling the ciphersuite list. +## The ClientHello content is spelled out below as a hex string as +## "prefix ciphersuite1 ciphersuite2 ciphersuite3 ciphersuite4 suffix". +## The expected response is an inappropriate_fallback alert. +requires_openssl_with_fallback_scsv +run_test "Fallback SCSV: beginning of list" \ + "$P_SRV debug_level=2" \ + "$TCP_CLIENT localhost $SRV_PORT '160301003e0100003a03022aafb94308dc22ca1086c65acc00e414384d76b61ecab37df1633b1ae1034dbe000008 5600 0031 0032 0033 0100000900230000000f000101' '15030200020256'" \ + 0 \ + -s "received FALLBACK_SCSV" \ + -s "inapropriate fallback" + +requires_openssl_with_fallback_scsv +run_test "Fallback SCSV: end of list" \ + "$P_SRV debug_level=2" \ + "$TCP_CLIENT localhost $SRV_PORT '160301003e0100003a03022aafb94308dc22ca1086c65acc00e414384d76b61ecab37df1633b1ae1034dbe000008 0031 0032 0033 5600 0100000900230000000f000101' '15030200020256'" \ + 0 \ + -s "received FALLBACK_SCSV" \ + -s "inapropriate fallback" + +## Here the expected response is a valid ServerHello prefix, up to the random. +requires_openssl_with_fallback_scsv +run_test "Fallback SCSV: not in list" \ + "$P_SRV debug_level=2" \ + "$TCP_CLIENT localhost $SRV_PORT '160301003e0100003a03022aafb94308dc22ca1086c65acc00e414384d76b61ecab37df1633b1ae1034dbe000008 0056 0031 0032 0033 0100000900230000000f000101' '16030200300200002c0302'" \ + 0 \ + -S "received FALLBACK_SCSV" \ + -S "inapropriate fallback" + # Tests for CBC 1/n-1 record splitting run_test "CBC Record splitting: TLS 1.2, no splitting" \