[openssl-commits] [openssl] master update
Matt Caswell
matt at openssl.org
Mon Jun 27 13:54:55 UTC 2016
The branch master has been updated
via c3fd55d4a6ed1025c471603b67fbbbce606a5171 (commit)
via 63916e9a234c1e6bbf82cc21b7d2e39cdecb30d0 (commit)
from 6f4a6a5cd472d56937a8e9d6665e7c9cc6b1b2e2 (commit)
- Log -----------------------------------------------------------------
commit c3fd55d4a6ed1025c471603b67fbbbce606a5171
Author: Matt Caswell <matt at openssl.org>
Date: Tue Jun 21 16:33:52 2016 +0100
Add a test for fragmented alerts
The previous commit fixed a problem where fragmented alerts would cause an
infinite loop. This commit adds a test for these fragmented alerts.
Reviewed-by: Andy Polyakov <appro at openssl.org>
commit 63916e9a234c1e6bbf82cc21b7d2e39cdecb30d0
Author: Matt Caswell <matt at openssl.org>
Date: Tue Jun 21 15:25:53 2016 +0100
Ensure read records are marked as read
In some situations (such as when we receive a fragment of an alert)
we try to get the next packet but did not mark the current one as read,
meaning that we got the same record back again - leading to an infinite
loop.
Found using the BoringSSL test suite.
Reviewed-by: Andy Polyakov <appro at openssl.org>
-----------------------------------------------------------------------
Summary of changes:
ssl/record/rec_layer_s3.c | 9 +++++-
test/recipes/70-test_sslrecords.t | 62 ++++++++++++++++++++++++++++++++++++++-
util/TLSProxy/Message.pm | 3 +-
3 files changed, 71 insertions(+), 3 deletions(-)
diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c
index bce82a7..fa20b35 100644
--- a/ssl/record/rec_layer_s3.c
+++ b/ssl/record/rec_layer_s3.c
@@ -1232,8 +1232,10 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
SSL3_RECORD_add_length(rr, -1);
}
- if (*dest_len < dest_maxlen)
+ if (*dest_len < dest_maxlen) {
+ SSL3_RECORD_set_read(rr);
goto start; /* fragment was too small */
+ }
}
}
@@ -1316,6 +1318,7 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
(s->session != NULL) && (s->session->cipher != NULL) &&
!(s->ctx->options & SSL_OP_ALLOW_UNSAFE_LEGACY_RENEGOTIATION)) {
SSL3_RECORD_set_length(rr, 0);
+ SSL3_RECORD_set_read(rr);
ssl3_send_alert(s, SSL3_AL_WARNING, SSL_AD_NO_RENEGOTIATION);
goto start;
}
@@ -1342,6 +1345,7 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
if (alert_level == SSL3_AL_WARNING) {
s->s3->warn_alert = alert_descr;
+ SSL3_RECORD_set_read(rr);
if (alert_descr == SSL_AD_CLOSE_NOTIFY) {
s->shutdown |= SSL_RECEIVED_SHUTDOWN;
return (0);
@@ -1372,6 +1376,7 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
BIO_snprintf(tmp, sizeof tmp, "%d", alert_descr);
ERR_add_error_data(2, "SSL alert number ", tmp);
s->shutdown |= SSL_RECEIVED_SHUTDOWN;
+ SSL3_RECORD_set_read(rr);
SSL_CTX_remove_session(s->session_ctx, s->session);
return (0);
} else {
@@ -1387,6 +1392,7 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
* shutdown */
s->rwstate = SSL_NOTHING;
SSL3_RECORD_set_length(rr, 0);
+ SSL3_RECORD_set_read(rr);
return (0);
}
@@ -1443,6 +1449,7 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
*/
if (s->version >= TLS1_VERSION && s->version <= TLS1_1_VERSION) {
SSL3_RECORD_set_length(rr, 0);
+ SSL3_RECORD_set_read(rr);
goto start;
}
al = SSL_AD_UNEXPECTED_MESSAGE;
diff --git a/test/recipes/70-test_sslrecords.t b/test/recipes/70-test_sslrecords.t
index b0e3739..94aabdc 100644
--- a/test/recipes/70-test_sslrecords.t
+++ b/test/recipes/70-test_sslrecords.t
@@ -38,7 +38,7 @@ my $proxy = TLSProxy::Proxy->new(
my $content_type = TLSProxy::Record::RT_APPLICATION_DATA;
my $inject_recs_num = 1;
$proxy->start() or plan skip_all => "Unable to start up Proxy for tests";
-plan tests => 3;
+plan tests => 4;
ok(TLSProxy::Message->fail(), "Out of context empty records test");
#Test 2: Injecting in context empty records should succeed
@@ -54,6 +54,14 @@ $inject_recs_num = 33;
$proxy->start();
ok(TLSProxy::Message->fail(), "Too many in context empty records test");
+#Test 4: Injecting a fragmented fatal alert should fail. We actually expect no
+# alerts to be sent from either side because *we* injected the fatal
+# alert, i.e. this will look like a disorderly close
+$proxy->clear();
+$proxy->filter(\&add_frag_alert_filter);
+$proxy->start();
+ok(!TLSProxy::Message->end(), "Fragmented alert records test");
+
sub add_empty_recs_filter
{
my $proxy = shift;
@@ -78,3 +86,55 @@ sub add_empty_recs_filter
push @{$proxy->record_list}, $record;
}
}
+
+sub add_frag_alert_filter
+{
+ my $proxy = shift;
+ my $byte;
+
+ # We're only interested in the initial ClientHello
+ if ($proxy->flight != 0) {
+ return;
+ }
+
+ # Add a zero length fragment first
+ #my $record = TLSProxy::Record->new(
+ # 0,
+ # TLSProxy::Record::RT_ALERT,
+ # TLSProxy::Record::VERS_TLS_1_2,
+ # 0,
+ # 0,
+ # 0,
+ # "",
+ # ""
+ #);
+ #push @{$proxy->record_list}, $record;
+
+ # Now add the alert level (Fatal) as a seperate record
+ $byte = pack('C', TLSProxy::Message::AL_LEVEL_FATAL);
+ my $record = TLSProxy::Record->new(
+ 0,
+ TLSProxy::Record::RT_ALERT,
+ TLSProxy::Record::VERS_TLS_1_2,
+ 1,
+ 1,
+ 1,
+ $byte,
+ $byte
+ );
+ push @{$proxy->record_list}, $record;
+
+ # And finally the description (Unexpected message) in a third record
+ $byte = pack('C', TLSProxy::Message::AL_DESC_UNEXPECTED_MESSAGE);
+ $record = TLSProxy::Record->new(
+ 0,
+ TLSProxy::Record::RT_ALERT,
+ TLSProxy::Record::VERS_TLS_1_2,
+ 1,
+ 1,
+ 1,
+ $byte,
+ $byte
+ );
+ push @{$proxy->record_list}, $record;
+}
diff --git a/util/TLSProxy/Message.pm b/util/TLSProxy/Message.pm
index 85d5d6b..b8db22f 100644
--- a/util/TLSProxy/Message.pm
+++ b/util/TLSProxy/Message.pm
@@ -36,7 +36,8 @@ use constant {
#Alert descriptions
use constant {
- AL_DESC_CLOSE_NOTIFY => 0
+ AL_DESC_CLOSE_NOTIFY => 0,
+ AL_DESC_UNEXPECTED_MESSAGE => 10
};
my %message_type = (
More information about the openssl-commits
mailing list