[openssl-commits] [openssl] master update
Matt Caswell
matt at openssl.org
Wed Aug 8 09:41:50 UTC 2018
The branch master has been updated
via f460e8396f8cb1be1bbd6a8a22d7e24b80d8a607 (commit)
via de9e884b2f43c59834c2b1c3cfde35fa2c797f2b (commit)
via 7426cd343d99d3d82e3fb06c8df18e5cc6bcec75 (commit)
from b4f001eb1a9e0bd0fda8f3c7dfbccb6422ad8c47 (commit)
- Log -----------------------------------------------------------------
commit f460e8396f8cb1be1bbd6a8a22d7e24b80d8a607
Author: Matt Caswell <matt at openssl.org>
Date: Tue Aug 7 16:22:31 2018 +0100
Add a test for unencrypted alert
Test that a server can handle an unecrypted alert when normally the next
message is encrypted.
Reviewed-by: Rich Salz <rsalz at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6887)
commit de9e884b2f43c59834c2b1c3cfde35fa2c797f2b
Author: Matt Caswell <matt at openssl.org>
Date: Tue Aug 7 12:40:08 2018 +0100
Tolerate encrypted or plaintext alerts
At certain points in the handshake we could receive either a plaintext or
an encrypted alert from the client. We should tolerate both where
appropriate.
Reviewed-by: Rich Salz <rsalz at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6887)
commit 7426cd343d99d3d82e3fb06c8df18e5cc6bcec75
Author: Matt Caswell <matt at openssl.org>
Date: Tue Aug 7 10:25:54 2018 +0100
Ensure that we write out alerts correctly after early_data
If we sent early_data and then received back an HRR, the enc_write_ctx
was stale resulting in errors if an alert needed to be sent.
Thanks to Quarkslab for reporting this.
In any case it makes little sense to encrypt alerts using the
client_early_traffic_secret, so we add special handling for alerts sent
after early_data. All such alerts are sent in plaintext.
Reviewed-by: Rich Salz <rsalz at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6887)
-----------------------------------------------------------------------
Summary of changes:
ssl/record/rec_layer_s3.c | 17 +++++---
ssl/record/ssl3_record.c | 9 +++-
ssl/record/ssl3_record_tls13.c | 8 +++-
ssl/s3_enc.c | 4 +-
ssl/statem/statem.c | 3 +-
ssl/statem/statem.h | 19 +++++++-
ssl/statem/statem_lib.c | 6 +++
ssl/statem/statem_srvr.c | 20 ++++++---
ssl/t1_enc.c | 4 +-
ssl/tls13_enc.c | 11 +++--
...0-test_sslskewith0p.t => 70-test_tls13alerts.t} | 38 ++++++----------
util/perl/TLSProxy/Alert.pm | 51 ++++++++++++++++++++++
util/perl/TLSProxy/Message.pm | 15 +++++++
util/perl/TLSProxy/Record.pm | 4 +-
14 files changed, 159 insertions(+), 50 deletions(-)
copy test/recipes/{70-test_sslskewith0p.t => 70-test_tls13alerts.t} (55%)
create mode 100644 util/perl/TLSProxy/Alert.pm
diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c
index 1628ac8..d208695 100644
--- a/ssl/record/rec_layer_s3.c
+++ b/ssl/record/rec_layer_s3.c
@@ -816,8 +816,8 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf,
/* Clear our SSL3_RECORD structures */
memset(wr, 0, sizeof(wr));
for (j = 0; j < numpipes; j++) {
- unsigned int version = SSL_TREAT_AS_TLS13(s) ? TLS1_2_VERSION
- : s->version;
+ unsigned int version = (s->version == TLS1_3_VERSION) ? TLS1_2_VERSION
+ : s->version;
unsigned char *compressdata = NULL;
size_t maxcomplen;
unsigned int rectype;
@@ -829,7 +829,10 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf,
* In TLSv1.3, once encrypting, we always use application data for the
* record type
*/
- if (SSL_TREAT_AS_TLS13(s) && s->enc_write_ctx != NULL)
+ if (SSL_TREAT_AS_TLS13(s)
+ && s->enc_write_ctx != NULL
+ && (s->statem.enc_write_state != ENC_WRITE_STATE_WRITE_PLAIN_ALERTS
+ || type != SSL3_RT_ALERT))
rectype = SSL3_RT_APPLICATION_DATA;
else
rectype = type;
@@ -892,7 +895,10 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf,
SSL3_RECORD_reset_input(&wr[j]);
}
- if (SSL_TREAT_AS_TLS13(s) && s->enc_write_ctx != NULL) {
+ if (SSL_TREAT_AS_TLS13(s)
+ && s->enc_write_ctx != NULL
+ && (s->statem.enc_write_state != ENC_WRITE_STATE_WRITE_PLAIN_ALERTS
+ || type != SSL3_RT_ALERT)) {
size_t rlen, max_send_fragment;
if (!WPACKET_put_bytes_u8(thispkt, type)) {
@@ -981,8 +987,7 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf,
SSL3_RECORD_set_length(thiswr, len);
}
- if (s->early_data_state == SSL_EARLY_DATA_WRITING
- || s->early_data_state == SSL_EARLY_DATA_WRITE_RETRY) {
+ if (s->statem.enc_write_state == ENC_WRITE_STATE_WRITE_PLAIN_ALERTS) {
/*
* We haven't actually negotiated the version yet, but we're trying to
* send early data - so we need to use the tls13enc function.
diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c
index ad478bf..a616bf0 100644
--- a/ssl/record/ssl3_record.c
+++ b/ssl/record/ssl3_record.c
@@ -342,7 +342,10 @@ int ssl3_get_record(SSL *s)
if (SSL_IS_TLS13(s) && s->enc_read_ctx != NULL) {
if (thisrr->type != SSL3_RT_APPLICATION_DATA
&& (thisrr->type != SSL3_RT_CHANGE_CIPHER_SPEC
- || !SSL_IS_FIRST_HANDSHAKE(s))) {
+ || !SSL_IS_FIRST_HANDSHAKE(s))
+ && (thisrr->type != SSL3_RT_ALERT
+ || s->statem.enc_read_state
+ != ENC_READ_STATE_ALLOW_PLAIN_ALERTS)) {
SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE,
SSL_F_SSL3_GET_RECORD, SSL_R_BAD_RECORD_TYPE);
return -1;
@@ -692,7 +695,9 @@ int ssl3_get_record(SSL *s)
}
}
- if (SSL_IS_TLS13(s) && s->enc_read_ctx != NULL) {
+ if (SSL_IS_TLS13(s)
+ && s->enc_read_ctx != NULL
+ && thisrr->type != SSL3_RT_ALERT) {
size_t end;
if (thisrr->length == 0
diff --git a/ssl/record/ssl3_record_tls13.c b/ssl/record/ssl3_record_tls13.c
index 8822ca2..a11ed48 100644
--- a/ssl/record/ssl3_record_tls13.c
+++ b/ssl/record/ssl3_record_tls13.c
@@ -52,7 +52,13 @@ int tls13_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending)
seq = RECORD_LAYER_get_read_sequence(&s->rlayer);
}
- if (ctx == NULL) {
+ /*
+ * If we're sending an alert and ctx != NULL then we must be forcing
+ * plaintext alerts. If we're reading and ctx != NULL then we allow
+ * plaintext alerts at certain points in the handshake. If we've got this
+ * far then we have already validated that a plaintext alert is ok here.
+ */
+ if (ctx == NULL || rec->type == SSL3_RT_ALERT) {
memmove(rec->data, rec->input, rec->length);
rec->input = rec->data;
return 1;
diff --git a/ssl/s3_enc.c b/ssl/s3_enc.c
index 6d78aa1..5f40381 100644
--- a/ssl/s3_enc.c
+++ b/ssl/s3_enc.c
@@ -155,7 +155,7 @@ int ssl3_change_cipher_state(SSL *s, int which)
RECORD_LAYER_reset_read_sequence(&s->rlayer);
mac_secret = &(s->s3->read_mac_secret[0]);
} else {
- s->statem.invalid_enc_write_ctx = 1;
+ s->statem.enc_write_state = ENC_WRITE_STATE_INVALID;
if (s->enc_write_ctx != NULL) {
reuse_dd = 1;
} else if ((s->enc_write_ctx = EVP_CIPHER_CTX_new()) == NULL) {
@@ -238,7 +238,7 @@ int ssl3_change_cipher_state(SSL *s, int which)
goto err;
}
- s->statem.invalid_enc_write_ctx = 0;
+ s->statem.enc_write_state = ENC_WRITE_STATE_VALID;
OPENSSL_cleanse(exp_key, sizeof(exp_key));
OPENSSL_cleanse(exp_iv, sizeof(exp_iv));
return 1;
diff --git a/ssl/statem/statem.c b/ssl/statem/statem.c
index 7f1017d..d75f9ea 100644
--- a/ssl/statem/statem.c
+++ b/ssl/statem/statem.c
@@ -123,7 +123,8 @@ void ossl_statem_fatal(SSL *s, int al, int func, int reason, const char *file,
s->statem.in_init = 1;
s->statem.state = MSG_FLOW_ERROR;
ERR_put_error(ERR_LIB_SSL, func, reason, file, line);
- if (al != SSL_AD_NO_ALERT && !s->statem.invalid_enc_write_ctx)
+ if (al != SSL_AD_NO_ALERT
+ && s->statem.enc_write_state != ENC_WRITE_STATE_INVALID)
ssl3_send_alert(s, SSL3_AL_FATAL, al);
}
diff --git a/ssl/statem/statem.h b/ssl/statem/statem.h
index 95dd881..144d930 100644
--- a/ssl/statem/statem.h
+++ b/ssl/statem/statem.h
@@ -71,6 +71,22 @@ typedef enum {
WRITE_STATE_POST_WORK
} WRITE_STATE;
+typedef enum {
+ /* The enc_write_ctx can be used normally */
+ ENC_WRITE_STATE_VALID,
+ /* The enc_write_ctx cannot be used */
+ ENC_WRITE_STATE_INVALID,
+ /* Write alerts in plaintext, but otherwise use the enc_write_ctx */
+ ENC_WRITE_STATE_WRITE_PLAIN_ALERTS
+} ENC_WRITE_STATES;
+
+typedef enum {
+ /* The enc_read_ctx can be used normally */
+ ENC_READ_STATE_VALID,
+ /* We may receive encrypted or plaintext alerts */
+ ENC_READ_STATE_ALLOW_PLAIN_ALERTS
+} ENC_READ_STATES;
+
/*****************************************************************************
* *
* This structure should be considered "opaque" to anything outside of the *
@@ -100,7 +116,8 @@ struct ossl_statem_st {
/* Should we skip the CertificateVerify message? */
unsigned int no_cert_verify;
int use_timer;
- int invalid_enc_write_ctx;
+ ENC_WRITE_STATES enc_write_state;
+ ENC_READ_STATES enc_read_state;
};
typedef struct ossl_statem_st OSSL_STATEM;
diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index caed61a..8a7d178 100644
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -747,6 +747,12 @@ MSG_PROCESS_RETURN tls_process_finished(SSL *s, PACKET *pkt)
/* This is a real handshake so make sure we clean it up at the end */
if (s->server) {
+ /*
+ * To get this far we must have read encrypted data from the client. We
+ * no longer tolerate unencrypted alerts. This value is ignored if less
+ * than TLSv1.3
+ */
+ s->statem.enc_read_state = ENC_READ_STATE_VALID;
if (s->post_handshake_auth != SSL_PHA_REQUESTED)
s->statem.cleanuphand = 1;
if (SSL_IS_TLS13(s) && !tls13_save_handshake_digest_for_pha(s)) {
diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index eb9070e..db5aafe 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -848,12 +848,7 @@ WORK_STATE ossl_statem_server_post_work(SSL *s, WORK_STATE wst)
return WORK_MORE_A;
break;
}
- /*
- * TODO(TLS1.3): This actually causes a problem. We don't yet know
- * whether the next record we are going to receive is an unencrypted
- * alert, or an encrypted handshake message. We're going to need
- * something clever in the record layer for this.
- */
+
if (SSL_IS_TLS13(s)) {
if (!s->method->ssl3_enc->setup_key_block(s)
|| !s->method->ssl3_enc->change_cipher_state(s,
@@ -868,6 +863,12 @@ WORK_STATE ossl_statem_server_post_work(SSL *s, WORK_STATE wst)
/* SSLfatal() already called */
return WORK_ERROR;
}
+ /*
+ * We don't yet know whether the next record we are going to receive
+ * is an unencrypted alert, an encrypted alert, or an encrypted
+ * handshake message. We temporarily tolerate unencrypted alerts.
+ */
+ s->statem.enc_read_state = ENC_READ_STATE_ALLOW_PLAIN_ALERTS;
break;
}
@@ -3523,6 +3524,13 @@ MSG_PROCESS_RETURN tls_process_client_certificate(SSL *s, PACKET *pkt)
size_t chainidx;
SSL_SESSION *new_sess = NULL;
+ /*
+ * To get this far we must have read encrypted data from the client. We no
+ * longer tolerate unencrypted alerts. This value is ignored if less than
+ * TLSv1.3
+ */
+ s->statem.enc_read_state = ENC_READ_STATE_VALID;
+
if ((sk = sk_X509_new_null()) == NULL) {
SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PROCESS_CLIENT_CERTIFICATE,
ERR_R_MALLOC_FAILURE);
diff --git a/ssl/t1_enc.c b/ssl/t1_enc.c
index 23d3efb..2db913f 100644
--- a/ssl/t1_enc.c
+++ b/ssl/t1_enc.c
@@ -154,7 +154,7 @@ int tls1_change_cipher_state(SSL *s, int which)
mac_secret = &(s->s3->read_mac_secret[0]);
mac_secret_size = &(s->s3->read_mac_secret_size);
} else {
- s->statem.invalid_enc_write_ctx = 1;
+ s->statem.enc_write_state = ENC_WRITE_STATE_INVALID;
if (s->ext.use_etm)
s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC_WRITE;
else
@@ -316,7 +316,7 @@ int tls1_change_cipher_state(SSL *s, int which)
ERR_R_INTERNAL_ERROR);
goto err;
}
- s->statem.invalid_enc_write_ctx = 0;
+ s->statem.enc_write_state = ENC_WRITE_STATE_VALID;
#ifdef SSL_DEBUG
printf("which = %04X\nkey=", which);
diff --git a/ssl/tls13_enc.c b/ssl/tls13_enc.c
index 48990fd..22db2f8 100644
--- a/ssl/tls13_enc.c
+++ b/ssl/tls13_enc.c
@@ -425,7 +425,7 @@ int tls13_change_cipher_state(SSL *s, int which)
RECORD_LAYER_reset_read_sequence(&s->rlayer);
} else {
- s->statem.invalid_enc_write_ctx = 1;
+ s->statem.enc_write_state = ENC_WRITE_STATE_INVALID;
if (s->enc_write_ctx != NULL) {
EVP_CIPHER_CTX_reset(s->enc_write_ctx);
} else {
@@ -648,7 +648,10 @@ int tls13_change_cipher_state(SSL *s, int which)
goto err;
}
- s->statem.invalid_enc_write_ctx = 0;
+ if (!s->server && label == client_early_traffic)
+ s->statem.enc_write_state = ENC_WRITE_STATE_WRITE_PLAIN_ALERTS;
+ else
+ s->statem.enc_write_state = ENC_WRITE_STATE_VALID;
ret = 1;
err:
OPENSSL_cleanse(secret, sizeof(secret));
@@ -671,7 +674,7 @@ int tls13_update_key(SSL *s, int sending)
insecret = s->client_app_traffic_secret;
if (sending) {
- s->statem.invalid_enc_write_ctx = 1;
+ s->statem.enc_write_state = ENC_WRITE_STATE_INVALID;
iv = s->write_iv;
ciph_ctx = s->enc_write_ctx;
RECORD_LAYER_reset_write_sequence(&s->rlayer);
@@ -692,7 +695,7 @@ int tls13_update_key(SSL *s, int sending)
memcpy(insecret, secret, hashlen);
- s->statem.invalid_enc_write_ctx = 0;
+ s->statem.enc_write_state = ENC_WRITE_STATE_VALID;
ret = 1;
err:
OPENSSL_cleanse(secret, sizeof(secret));
diff --git a/test/recipes/70-test_sslskewith0p.t b/test/recipes/70-test_tls13alerts.t
similarity index 55%
copy from test/recipes/70-test_sslskewith0p.t
copy to test/recipes/70-test_tls13alerts.t
index 53a8b51..7111d40 100644
--- a/test/recipes/70-test_sslskewith0p.t
+++ b/test/recipes/70-test_tls13alerts.t
@@ -1,5 +1,5 @@
#! /usr/bin/env perl
-# Copyright 2015-2018 The OpenSSL Project Authors. All Rights Reserved.
+# Copyright 2018 The OpenSSL Project Authors. All Rights Reserved.
#
# Licensed under the OpenSSL license (the "License"). You may not use
# this file except in compliance with the License. You can obtain a copy
@@ -11,7 +11,7 @@ use OpenSSL::Test qw/:DEFAULT cmdstr srctop_file bldtop_dir/;
use OpenSSL::Test::Utils;
use TLSProxy::Proxy;
-my $test_name = "test_sslskewith0p";
+my $test_name = "test_tls13alerts";
setup($test_name);
plan skip_all => "TLSProxy isn't usable on $^O"
@@ -20,47 +20,37 @@ plan skip_all => "TLSProxy isn't usable on $^O"
plan skip_all => "$test_name needs the dynamic engine feature enabled"
if disabled("engine") || disabled("dynamic-engine");
-plan skip_all => "dh is not supported by this OpenSSL build"
- if disabled("dh");
-
plan skip_all => "$test_name needs the sock feature enabled"
if disabled("sock");
-plan skip_all => "$test_name needs TLS enabled"
- if alldisabled(available_protocols("tls"));
+plan skip_all => "$test_name needs TLS1.3 enabled"
+ if disabled("tls1_3");
$ENV{OPENSSL_ia32cap} = '~0x200000200000000';
+
my $proxy = TLSProxy::Proxy->new(
- \&ske_0_p_filter,
+ undef,
cmdstr(app(["openssl"]), display => 1),
srctop_file("apps", "server.pem"),
(!$ENV{HARNESS_ACTIVE} || $ENV{HARNESS_VERBOSE})
);
-#We must use an anon DHE cipher for this test
-$proxy->cipherc('ADH-AES128-SHA:@SECLEVEL=0');
-$proxy->ciphers('ADH-AES128-SHA:@SECLEVEL=0');
-
-$proxy->clientflags("-no_tls1_3");
+#Test 1: We test that a server can handle an unencrypted alert when normally the
+# next message is encrypted
+$proxy->filter(\&alert_filter);
$proxy->start() or plan skip_all => "Unable to start up Proxy for tests";
plan tests => 1;
-ok(TLSProxy::Message->fail, "ServerKeyExchange with 0 p");
+my $alert = TLSProxy::Message->alert();
+ok(TLSProxy::Message->fail() && !$alert->server() && !$alert->encrypted(), "Client sends an unecrypted alert");
-sub ske_0_p_filter
+sub alert_filter
{
my $proxy = shift;
- # We're only interested in the SKE - always in flight 1
if ($proxy->flight != 1) {
return;
}
- foreach my $message (@{$proxy->message_list}) {
- if ($message->mt == TLSProxy::Message::MT_SERVER_KEY_EXCHANGE) {
- #Set p to a value of 0
- $message->p(pack('C', 0));
-
- $message->repack();
- }
- }
+ ${$proxy->message_list}[1]->session_id_len(1);
+ ${$proxy->message_list}[1]->repack();
}
diff --git a/util/perl/TLSProxy/Alert.pm b/util/perl/TLSProxy/Alert.pm
new file mode 100644
index 0000000..e66883d
--- /dev/null
+++ b/util/perl/TLSProxy/Alert.pm
@@ -0,0 +1,51 @@
+# Copyright 2018 The OpenSSL Project Authors. All Rights Reserved.
+#
+# Licensed under the OpenSSL license (the "License"). You may not use
+# this file except in compliance with the License. You can obtain a copy
+# in the file LICENSE in the source distribution or at
+# https://www.openssl.org/source/license.html
+
+use strict;
+
+package TLSProxy::Alert;
+
+sub new
+{
+ my $class = shift;
+ my ($server,
+ $encrypted,
+ $level,
+ $description) = @_;
+
+ my $self = {
+ server => $server,
+ encrypted => $encrypted,
+ level => $level,
+ description => $description
+ };
+
+ return bless $self, $class;
+}
+
+#Read only accessors
+sub server
+{
+ my $self = shift;
+ return $self->{server};
+}
+sub encrypted
+{
+ my $self = shift;
+ return $self->{encrypted};
+}
+sub level
+{
+ my $self = shift;
+ return $self->{level};
+}
+sub description
+{
+ my $self = shift;
+ return $self->{description};
+}
+1;
diff --git a/util/perl/TLSProxy/Message.pm b/util/perl/TLSProxy/Message.pm
index 56570f9..44952ad 100644
--- a/util/perl/TLSProxy/Message.pm
+++ b/util/perl/TLSProxy/Message.pm
@@ -9,6 +9,8 @@ use strict;
package TLSProxy::Message;
+use TLSProxy::Alert;
+
use constant TLS_MESSAGE_HEADER_LENGTH => 4;
#Message types
@@ -140,6 +142,7 @@ my @message_rec_list = ();
my @message_frag_lens = ();
my $ciphersuite = 0;
my $successondata = 0;
+my $alert;
sub clear
{
@@ -152,6 +155,7 @@ sub clear
$successondata = 0;
@message_rec_list = ();
@message_frag_lens = ();
+ $alert = undef;
}
#Class method to extract messages from a record
@@ -281,6 +285,11 @@ sub get_messages
if ($alertlev == AL_LEVEL_FATAL || $alertdesc == AL_DESC_CLOSE_NOTIFY) {
$end = 1;
}
+ $alert = TLSProxy::Alert->new(
+ $server,
+ $record->encrypted,
+ $alertlev,
+ $alertdesc);
}
return @messages;
@@ -388,6 +397,12 @@ sub fail
my $class = shift;
return !$success && $end;
}
+
+sub alert
+{
+ return $alert;
+}
+
sub new
{
my $class = shift;
diff --git a/util/perl/TLSProxy/Record.pm b/util/perl/TLSProxy/Record.pm
index 9de51b3..8db50d0 100644
--- a/util/perl/TLSProxy/Record.pm
+++ b/util/perl/TLSProxy/Record.pm
@@ -97,7 +97,9 @@ sub get_records
$data # decrypt_data
);
- if ($content_type != RT_CCS) {
+ if ($content_type != RT_CCS
+ && (!TLSProxy::Proxy->is_tls13()
+ || $content_type != RT_ALERT)) {
if (($server && $server_encrypting)
|| (!$server && $client_encrypting)) {
if (!TLSProxy::Proxy->is_tls13() && $etm) {
More information about the openssl-commits
mailing list