[openssl-commits] [openssl] master update
Matt Caswell
matt at openssl.org
Tue Mar 7 16:47:40 UTC 2017
The branch master has been updated
via 75e314f2d573d4f984ff6a371be7a4966bf5f4c5 (commit)
via 774c909bc9bba503bd3657c5d89a9b689c4da2f3 (commit)
via 524420d8459fa07a8e4900bc9dfb558b215edbbd (commit)
via b8c49611bc26c8f9a980b814496a3069cd524b79 (commit)
from c1f84df2484d9d6745f5aaf1eb708f0bcd3faf28 (commit)
- Log -----------------------------------------------------------------
commit 75e314f2d573d4f984ff6a371be7a4966bf5f4c5
Author: Matt Caswell <matt at openssl.org>
Date: Tue Mar 7 16:21:38 2017 +0000
Fix the number of tests to skip if TLSv1.3 is disabled
Reviewed-by: Rich Salz <rsalz at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/2875)
commit 774c909bc9bba503bd3657c5d89a9b689c4da2f3
Author: Matt Caswell <matt at openssl.org>
Date: Tue Mar 7 12:03:10 2017 +0000
Add a test for records not on the record boundary
Test that we check that key change messages appear on a record boundary.
Reviewed-by: Rich Salz <rsalz at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/2875)
commit 524420d8459fa07a8e4900bc9dfb558b215edbbd
Author: Matt Caswell <matt at openssl.org>
Date: Tue Mar 7 10:21:58 2017 +0000
Check TLSv1.3 ServerHello, Finished and KeyUpdates are on record boundary
In TLSv1.3 the above messages signal a key change. The spec requires that
the end of these messages must align with a record boundary. We can detect
this by checking for decrypted but as yet unread record data sitting in
OpenSSL buffers at the point where we process the messages.
Reviewed-by: Rich Salz <rsalz at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/2875)
commit b8c49611bc26c8f9a980b814496a3069cd524b79
Author: Matt Caswell <matt at openssl.org>
Date: Fri Mar 3 12:41:39 2017 +0000
Provide a function to test whether we have unread records pending
Also updates SSL_has_pending() to use it. This actually fixes a bug in
SSL_has_pending() which is supposed to return 1 if we have any processed
or unprocessed data sitting in OpenSSL buffers. However it failed to return
1 if we had processed non-application data pending.
Reviewed-by: Rich Salz <rsalz at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/2875)
-----------------------------------------------------------------------
Summary of changes:
include/openssl/ssl.h | 1 +
ssl/record/rec_layer_s3.c | 13 ++++
ssl/record/record.h | 1 +
ssl/ssl_err.c | 1 +
ssl/ssl_lib.c | 2 +-
ssl/statem/statem_clnt.c | 10 ++++
ssl/statem/statem_lib.c | 20 +++++++
test/recipes/70-test_sslrecords.t | 123 ++++++++++++++++++++++++++++++++++----
8 files changed, 159 insertions(+), 12 deletions(-)
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index c569407..9fbf3d1 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -2575,6 +2575,7 @@ int ERR_load_SSL_strings(void);
# define SSL_R_MISSING_SRP_PARAM 358
# define SSL_R_MISSING_TMP_DH_KEY 171
# define SSL_R_MISSING_TMP_ECDH_KEY 311
+# define SSL_R_NOT_ON_RECORD_BOUNDARY 182
# define SSL_R_NO_CERTIFICATES_RETURNED 176
# define SSL_R_NO_CERTIFICATE_ASSIGNED 177
# define SSL_R_NO_CERTIFICATE_SET 179
diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c
index 2cdc62d..a14d372 100644
--- a/ssl/record/rec_layer_s3.c
+++ b/ssl/record/rec_layer_s3.c
@@ -77,11 +77,24 @@ void RECORD_LAYER_release(RECORD_LAYER *rl)
SSL3_RECORD_release(rl->rrec, SSL_MAX_PIPELINES);
}
+/* Checks if we have unprocessed read ahead data pending */
int RECORD_LAYER_read_pending(const RECORD_LAYER *rl)
{
return SSL3_BUFFER_get_left(&rl->rbuf) != 0;
}
+/* Checks if we have decrypted unread record data pending */
+int RECORD_LAYER_processed_read_pending(const RECORD_LAYER *rl)
+{
+ size_t curr_rec = 0, num_recs = RECORD_LAYER_get_numrpipes(rl);
+ const SSL3_RECORD *rr = rl->rrec;
+
+ while (curr_rec < num_recs && SSL3_RECORD_is_read(&rr[curr_rec]))
+ curr_rec++;
+
+ return curr_rec < num_recs;
+}
+
int RECORD_LAYER_write_pending(const RECORD_LAYER *rl)
{
return (rl->numwpipes > 0)
diff --git a/ssl/record/record.h b/ssl/record/record.h
index 2c9b9dd..6880f77 100644
--- a/ssl/record/record.h
+++ b/ssl/record/record.h
@@ -207,6 +207,7 @@ void RECORD_LAYER_init(RECORD_LAYER *rl, SSL *s);
void RECORD_LAYER_clear(RECORD_LAYER *rl);
void RECORD_LAYER_release(RECORD_LAYER *rl);
int RECORD_LAYER_read_pending(const RECORD_LAYER *rl);
+int RECORD_LAYER_processed_read_pending(const RECORD_LAYER *rl);
int RECORD_LAYER_write_pending(const RECORD_LAYER *rl);
void RECORD_LAYER_reset_read_sequence(RECORD_LAYER *rl);
void RECORD_LAYER_reset_write_sequence(RECORD_LAYER *rl);
diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c
index ee1ca62..23987e6 100644
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -625,6 +625,7 @@ static ERR_STRING_DATA SSL_str_reasons[] = {
{ERR_REASON(SSL_R_MISSING_SRP_PARAM), "can't find SRP server param"},
{ERR_REASON(SSL_R_MISSING_TMP_DH_KEY), "missing tmp dh key"},
{ERR_REASON(SSL_R_MISSING_TMP_ECDH_KEY), "missing tmp ecdh key"},
+ {ERR_REASON(SSL_R_NOT_ON_RECORD_BOUNDARY), "not on record boundary"},
{ERR_REASON(SSL_R_NO_CERTIFICATES_RETURNED), "no certificates returned"},
{ERR_REASON(SSL_R_NO_CERTIFICATE_ASSIGNED), "no certificate assigned"},
{ERR_REASON(SSL_R_NO_CERTIFICATE_SET), "no certificate set"},
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index f0e8639..581941e 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -1323,7 +1323,7 @@ int SSL_has_pending(const SSL *s)
* data. That data may not result in any application data, or we may fail
* to parse the records for some reason.
*/
- if (SSL_pending(s))
+ if (RECORD_LAYER_processed_read_pending(&s->rlayer))
return 1;
return RECORD_LAYER_read_pending(&s->rlayer);
diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index b11cd19..9f4a719 100644
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -1237,6 +1237,16 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt)
goto f_err;
}
+ /*
+ * In TLSv1.3 a ServerHello message signals a key change so the end of the
+ * message must be on a record boundary.
+ */
+ if (SSL_IS_TLS13(s) && RECORD_LAYER_processed_read_pending(&s->rlayer)) {
+ al = SSL_AD_UNEXPECTED_MESSAGE;
+ SSLerr(SSL_F_TLS_PROCESS_SERVER_HELLO, SSL_R_NOT_ON_RECORD_BOUNDARY);
+ goto f_err;
+ }
+
/* load the server hello data */
/* load the server random */
if (!PACKET_copy_bytes(pkt, s->s3->server_random, SSL3_RANDOM_SIZE)) {
diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index 32bcad4..36c96e5 100644
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -539,6 +539,16 @@ MSG_PROCESS_RETURN tls_process_key_update(SSL *s, PACKET *pkt)
goto err;
}
+ /*
+ * A KeyUpdate message signals a key change so the end of the message must
+ * be on a record boundary.
+ */
+ if (RECORD_LAYER_processed_read_pending(&s->rlayer)) {
+ al = SSL_AD_UNEXPECTED_MESSAGE;
+ SSLerr(SSL_F_TLS_PROCESS_KEY_UPDATE, SSL_R_NOT_ON_RECORD_BOUNDARY);
+ goto err;
+ }
+
if (!PACKET_get_1(pkt, &updatetype)
|| PACKET_remaining(pkt) != 0
|| (updatetype != SSL_KEY_UPDATE_NOT_REQUESTED
@@ -676,6 +686,16 @@ MSG_PROCESS_RETURN tls_process_finished(SSL *s, PACKET *pkt)
if (s->server)
s->statem.cleanuphand = 1;
+ /*
+ * In TLSv1.3 a Finished message signals a key change so the end of the
+ * message must be on a record boundary.
+ */
+ if (SSL_IS_TLS13(s) && RECORD_LAYER_processed_read_pending(&s->rlayer)) {
+ al = SSL_AD_UNEXPECTED_MESSAGE;
+ SSLerr(SSL_F_TLS_PROCESS_FINISHED, SSL_R_NOT_ON_RECORD_BOUNDARY);
+ goto f_err;
+ }
+
/* If this occurs, we have missed a message */
if (!SSL_IS_TLS13(s) && !s->s3->change_cipher_spec) {
al = SSL_AD_UNEXPECTED_MESSAGE;
diff --git a/test/recipes/70-test_sslrecords.t b/test/recipes/70-test_sslrecords.t
index 2ffa2cd..ee49cb3 100644
--- a/test/recipes/70-test_sslrecords.t
+++ b/test/recipes/70-test_sslrecords.t
@@ -34,19 +34,14 @@ my $proxy = TLSProxy::Proxy->new(
(!$ENV{HARNESS_ACTIVE} || $ENV{HARNESS_VERBOSE})
);
+my $boundary_test_type;
+
#Test 1: Injecting out of context empty records should fail
my $content_type = TLSProxy::Record::RT_APPLICATION_DATA;
my $inject_recs_num = 1;
$proxy->serverflags("-tls1_2");
$proxy->start() or plan skip_all => "Unable to start up Proxy for tests";
-my $num_tests = 11;
-if (!disabled("tls1_1")) {
- $num_tests++;
-}
-if (!disabled("tls1_3")) {
- $num_tests += 3;
-}
-plan tests => $num_tests;
+plan tests => 18;
ok(TLSProxy::Message->fail(), "Out of context empty records test");
#Test 2: Injecting in context empty records should succeed
@@ -133,8 +128,10 @@ $proxy->filter(\&add_unknown_record_type);
$proxy->start();
ok(TLSProxy::Message->fail(), "Unrecognised record type in TLS1.2");
-#Test 11: Sending an unrecognised record type in TLS1.1 should fail
-if (!disabled("tls1_1")) {
+SKIP: {
+ skip "TLSv1.1 disabled", 1 if disabled("tls1_1");
+
+ #Test 11: Sending an unrecognised record type in TLS1.1 should fail
$proxy->clear();
$proxy->clientflags("-tls1_1");
$proxy->start();
@@ -149,7 +146,9 @@ $proxy->start();
ok(TLSProxy::Message->fail(), "Changed record version in TLS1.2");
#TLS1.3 specific tests
-if (!disabled("tls1_3")) {
+SKIP: {
+ skip "TLSv1.3 disabled", 6 if disabled("tls1_3");
+
#Test 13: Sending a different record version in TLS1.3 should succeed
$proxy->clear();
$proxy->filter(\&change_version);
@@ -168,6 +167,36 @@ if (!disabled("tls1_3")) {
$proxy->filter(\&change_outer_record_type);
$proxy->start();
ok(TLSProxy::Message->fail(), "Wrong outer record type in TLS1.3");
+
+ use constant {
+ DATA_AFTER_SERVER_HELLO => 0,
+ DATA_AFTER_FINISHED => 1,
+ DATA_AFTER_KEY_UPDATE => 2
+ };
+
+ #Test 16: Sending a ServerHello which doesn't end on a record boundary
+ # should fail
+ $proxy->clear();
+ $boundary_test_type = DATA_AFTER_SERVER_HELLO;
+ $proxy->filter(\¬_on_record_boundary);
+ $proxy->start();
+ ok(TLSProxy::Message->fail(), "Record not on bounday in TLS1.3 (ServerHello)");
+
+ #Test 17: Sending a Finished which doesn't end on a record boundary
+ # should fail
+ $proxy->clear();
+ $boundary_test_type = DATA_AFTER_FINISHED;
+ $proxy->filter(\¬_on_record_boundary);
+ $proxy->start();
+ ok(TLSProxy::Message->fail(), "Record not on bounday in TLS1.3 (Finished)");
+
+ #Test 18: Sending a KeyUpdate which doesn't end on a record boundary
+ # should fail
+ $proxy->clear();
+ $boundary_test_type = DATA_AFTER_KEY_UPDATE;
+ $proxy->filter(\¬_on_record_boundary);
+ $proxy->start();
+ ok(TLSProxy::Message->fail(), "Record not on bounday in TLS1.3 (KeyUpdate)");
}
@@ -459,3 +488,75 @@ sub change_outer_record_type
$i++;
${$proxy->record_list}[$i]->outer_content_type(TLSProxy::Record::RT_HANDSHAKE);
}
+
+sub not_on_record_boundary
+{
+ my $proxy = shift;
+ my $data;
+
+ #Find server's first flight
+ if ($proxy->flight != 1) {
+ return;
+ }
+
+ if ($boundary_test_type == DATA_AFTER_SERVER_HELLO) {
+ #Merge the ServerHello and EncryptedExtensions records into one
+ my $i;
+ for ($i = 0; ${$proxy->record_list}[$i]->flight() < 1; $i++) {
+ next;
+ }
+ $data = ${$proxy->record_list}[$i]->data();
+ $data .= ${$proxy->record_list}[$i + 1]->decrypt_data();
+ ${$proxy->record_list}[$i]->data($data);
+ ${$proxy->record_list}[$i]->len(length $data);
+
+ #Delete the old EncryptedExtensions record
+ splice @{$proxy->record_list}, $i + 1, 1;
+ } elsif ($boundary_test_type == DATA_AFTER_FINISHED) {
+ $data = ${$proxy->record_list}[-1]->decrypt_data;
+
+ #Add a KeyUpdate message onto the end of the Finished record
+ my $keyupdate = pack "C5",
+ 0x18, # KeyUpdate
+ 0x00, 0x00, 0x01, # Message length
+ 0x00; # Update not requested
+
+ $data .= $keyupdate;
+
+ #Add content type and tag
+ $data .= pack("C", TLSProxy::Record::RT_HANDSHAKE).("\0"x16);
+
+ #Update the record
+ ${$proxy->record_list}[-1]->data($data);
+ ${$proxy->record_list}[-1]->len(length $data);
+ } else {
+ #KeyUpdates must end on a record boundary
+
+ my $record = TLSProxy::Record->new(
+ 1,
+ TLSProxy::Record::RT_APPLICATION_DATA,
+ TLSProxy::Record::VERS_TLS_1_0,
+ 0,
+ 0,
+ 0,
+ 0,
+ "",
+ ""
+ );
+
+ #Add two KeyUpdate messages into a single record
+ my $keyupdate = pack "C5",
+ 0x18, # KeyUpdate
+ 0x00, 0x00, 0x01, # Message length
+ 0x00; # Update not requested
+
+ $data = $keyupdate.$keyupdate;
+
+ #Add content type and tag
+ $data .= pack("C", TLSProxy::Record::RT_HANDSHAKE).("\0"x16);
+
+ $record->data($data);
+ $record->len(length $data);
+ push @{$proxy->record_list}, $record;
+ }
+}
More information about the openssl-commits
mailing list