[openssl-commits] [openssl] master update
Matt Caswell
matt at openssl.org
Fri Sep 7 10:15:42 UTC 2018
The branch master has been updated
via 57d7b988b498ed34e98d1957fbbded8342f2a952 (commit)
via 80eff008ec8767f844534d28a7c252cd23c08835 (commit)
via 1bf4cb0fe3b00e1c501a04ace4e3e3145314cb20 (commit)
from f922dac87d859cc7419207301533fe89582ac3ea (commit)
- Log -----------------------------------------------------------------
commit 57d7b988b498ed34e98d1957fbbded8342f2a952
Author: Matt Caswell <matt at openssl.org>
Date: Thu Sep 6 12:06:24 2018 +0100
Test that we can handle a PHA CertificateRequest after we sent close_notify
Even though we already sent close_notify the server may not have recieved
it yet and could issue a CertificateRequest to us. Since we've already
sent close_notify we can't send any reasonable response so we just ignore
it.
Reviewed-by: Tim Hudson <tjh at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/7114)
commit 80eff008ec8767f844534d28a7c252cd23c08835
Author: Kurt Roeckx <kurt at roeckx.be>
Date: Tue Sep 4 13:39:41 2018 +0100
Test that we can process a KeyUpdate received after we sent close_notify
Reviewed-by: Tim Hudson <tjh at openssl.org>
Reviewed-by: Matt Caswell <matt at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/7114)
commit 1bf4cb0fe3b00e1c501a04ace4e3e3145314cb20
Author: Matt Caswell <matt at openssl.org>
Date: Tue Sep 4 13:36:55 2018 +0100
Process KeyUpdate and NewSessionTicket messages after a close_notify
If we've sent a close_notify then we are restricted about what we can do
in response to handshake messages that we receive. However we can sensibly
process NewSessionTicket messages. We can also process a KeyUpdate message
as long as we also ignore any request for us to update our sending keys.
Reviewed-by: Tim Hudson <tjh at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/7114)
-----------------------------------------------------------------------
Summary of changes:
ssl/record/rec_layer_s3.c | 42 +++++++++++++++---------------
ssl/statem/statem_clnt.c | 27 ++++++++++++++++----
ssl/statem/statem_lib.c | 7 +++--
test/sslapitest.c | 65 +++++++++++++++++++++++++++++++++++------------
4 files changed, 98 insertions(+), 43 deletions(-)
diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c
index d208695..6d49571 100644
--- a/ssl/record/rec_layer_s3.c
+++ b/ssl/record/rec_layer_s3.c
@@ -1554,30 +1554,30 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
return -1;
}
- /*
- * If we've sent a close_notify but not yet received one back then ditch
- * anything we read.
- */
if ((s->shutdown & SSL_SENT_SHUTDOWN) != 0) {
- /*
- * In TLSv1.3 this could get problematic if we receive a KeyUpdate
- * message after we sent a close_notify because we're about to ditch it,
- * so we won't be able to read a close_notify sent afterwards! We don't
- * support that.
- */
- SSL3_RECORD_set_length(rr, 0);
- SSL3_RECORD_set_read(rr);
-
if (SSL3_RECORD_get_type(rr) == SSL3_RT_HANDSHAKE) {
BIO *rbio;
- if ((s->mode & SSL_MODE_AUTO_RETRY) != 0)
- goto start;
+ /*
+ * We ignore any handshake messages sent to us unless they are
+ * TLSv1.3 in which case we want to process them. For all other
+ * handshake messages we can't do anything reasonable with them
+ * because we are unable to write any response due to having already
+ * sent close_notify.
+ */
+ if (!SSL_IS_TLS13(s)) {
+ SSL3_RECORD_set_length(rr, 0);
+ SSL3_RECORD_set_read(rr);
- s->rwstate = SSL_READING;
- rbio = SSL_get_rbio(s);
- BIO_clear_retry_flags(rbio);
- BIO_set_retry_read(rbio);
+ if ((s->mode & SSL_MODE_AUTO_RETRY) != 0)
+ goto start;
+
+ s->rwstate = SSL_READING;
+ rbio = SSL_get_rbio(s);
+ BIO_clear_retry_flags(rbio);
+ BIO_set_retry_read(rbio);
+ return -1;
+ }
} else {
/*
* The peer is continuing to send application data, but we have
@@ -1586,10 +1586,12 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
* above.
* No alert sent because we already sent close_notify
*/
+ SSL3_RECORD_set_length(rr, 0);
+ SSL3_RECORD_set_read(rr);
SSLfatal(s, SSL_AD_NO_ALERT, SSL_F_SSL3_READ_BYTES,
SSL_R_APPLICATION_DATA_AFTER_CLOSE_NOTIFY);
+ return -1;
}
- return -1;
}
/*
diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 3dc29cc..8c658da 100644
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -423,11 +423,19 @@ static WRITE_TRAN ossl_statem_client13_write_transition(SSL *s)
st->hand_state = TLS_ST_CW_CERT;
return WRITE_TRAN_CONTINUE;
}
- /* Shouldn't happen - same as default case */
- SSLfatal(s, SSL_AD_INTERNAL_ERROR,
- SSL_F_OSSL_STATEM_CLIENT13_WRITE_TRANSITION,
- ERR_R_INTERNAL_ERROR);
- return WRITE_TRAN_ERROR;
+ /*
+ * We should only get here if we received a CertificateRequest after
+ * we already sent close_notify
+ */
+ if (!ossl_assert((s->shutdown & SSL_SENT_SHUTDOWN) != 0)) {
+ /* Shouldn't happen - same as default case */
+ SSLfatal(s, SSL_AD_INTERNAL_ERROR,
+ SSL_F_OSSL_STATEM_CLIENT13_WRITE_TRANSITION,
+ ERR_R_INTERNAL_ERROR);
+ return WRITE_TRAN_ERROR;
+ }
+ st->hand_state = TLS_ST_OK;
+ return WRITE_TRAN_CONTINUE;
case TLS_ST_CR_FINISHED:
if (s->early_data_state == SSL_EARLY_DATA_WRITE_RETRY
@@ -2446,6 +2454,15 @@ MSG_PROCESS_RETURN tls_process_certificate_request(SSL *s, PACKET *pkt)
PACKET reqctx, extensions;
RAW_EXTENSION *rawexts = NULL;
+ if ((s->shutdown & SSL_SENT_SHUTDOWN) != 0) {
+ /*
+ * We already sent close_notify. This can only happen in TLSv1.3
+ * post-handshake messages. We can't reasonably respond to this, so
+ * we just ignore it
+ */
+ return MSG_PROCESS_FINISHED_READING;
+ }
+
/* Free and zero certificate types: it is not present in TLS 1.3 */
OPENSSL_free(s->s3->tmp.ctype);
s->s3->tmp.ctype = NULL;
diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index 3961c14..adc8b98 100644
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -638,9 +638,12 @@ MSG_PROCESS_RETURN tls_process_key_update(SSL *s, PACKET *pkt)
/*
* If we get a request for us to update our sending keys too then, we need
* to additionally send a KeyUpdate message. However that message should
- * not also request an update (otherwise we get into an infinite loop).
+ * not also request an update (otherwise we get into an infinite loop). We
+ * ignore a request for us to update our sending keys too if we already
+ * sent close_notify.
*/
- if (updatetype == SSL_KEY_UPDATE_REQUESTED)
+ if (updatetype == SSL_KEY_UPDATE_REQUESTED
+ && (s->shutdown & SSL_SENT_SHUTDOWN) == 0)
s->key_update = SSL_KEY_UPDATE_NOT_REQUESTED;
if (!tls13_update_key(s, 0)) {
diff --git a/test/sslapitest.c b/test/sslapitest.c
index f9ba60a..bb51885 100644
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -5341,9 +5341,11 @@ static int test_ticket_callbacks(int tst)
* Test 1: TLSv1.2, server continues to read/write after client shutdown
* Test 2: TLSv1.3, no pending NewSessionTicket messages
* Test 3: TLSv1.3, pending NewSessionTicket messages
- * Test 4: TLSv1.3, server continues to read/write after client shutdown, client
- * reads it
- * Test 5: TLSv1.3, server continues to read/write after client shutdown, client
+ * Test 4: TLSv1.3, server continues to read/write after client shutdown, server
+ * sends key update, client reads it
+ * Test 5: TLSv1.3, server continues to read/write after client shutdown, server
+ * sends CertificateRequest, client reads and ignores it
+ * Test 6: TLSv1.3, server continues to read/write after client shutdown, client
* doesn't read it
*/
static int test_shutdown(int tst)
@@ -5354,6 +5356,7 @@ static int test_shutdown(int tst)
char msg[] = "A test message";
char buf[80];
size_t written, readbytes;
+ SSL_SESSION *sess;
#ifdef OPENSSL_NO_TLS1_2
if (tst <= 1)
@@ -5369,17 +5372,26 @@ static int test_shutdown(int tst)
TLS1_VERSION,
(tst <= 1) ? TLS1_2_VERSION
: TLS1_3_VERSION,
- &sctx, &cctx, cert, privkey))
- || !TEST_true(create_ssl_objects(sctx, cctx, &serverssl, &clientssl,
+ &sctx, &cctx, cert, privkey)))
+ goto end;
+
+ if (tst == 5)
+ SSL_CTX_set_post_handshake_auth(cctx, 1);
+
+ if (!TEST_true(create_ssl_objects(sctx, cctx, &serverssl, &clientssl,
NULL, NULL)))
goto end;
if (tst == 3) {
if (!TEST_true(create_bare_ssl_connection(serverssl, clientssl,
- SSL_ERROR_NONE)))
+ SSL_ERROR_NONE))
+ || !TEST_ptr_ne(sess = SSL_get_session(clientssl), NULL)
+ || !TEST_false(SSL_SESSION_is_resumable(sess)))
goto end;
} else if (!TEST_true(create_ssl_connection(serverssl, clientssl,
- SSL_ERROR_NONE))) {
+ SSL_ERROR_NONE))
+ || !TEST_ptr_ne(sess = SSL_get_session(clientssl), NULL)
+ || !TEST_true(SSL_SESSION_is_resumable(sess))) {
goto end;
}
@@ -5400,13 +5412,30 @@ static int test_shutdown(int tst)
* Even though we're shutdown on receive we should still be
* able to write.
*/
- || !TEST_true(SSL_write(serverssl, msg, sizeof(msg)))
- || !TEST_int_eq(SSL_shutdown(serverssl), 1))
+ || !TEST_true(SSL_write(serverssl, msg, sizeof(msg))))
goto end;
- if (tst == 4) {
- /* Should still be able to read data from server */
+ if (tst == 4
+ && !TEST_true(SSL_key_update(serverssl,
+ SSL_KEY_UPDATE_REQUESTED)))
+ goto end;
+ if (tst == 5) {
+ SSL_set_verify(serverssl, SSL_VERIFY_PEER, NULL);
+ if (!TEST_true(SSL_verify_client_post_handshake(serverssl)))
+ goto end;
+ }
+ if ((tst == 4 || tst == 5)
+ && !TEST_true(SSL_write(serverssl, msg, sizeof(msg))))
+ goto end;
+ if (!TEST_int_eq(SSL_shutdown(serverssl), 1))
+ goto end;
+ if (tst == 4 || tst == 5) {
+ /* Should still be able to read data from server */
if (!TEST_true(SSL_read_ex(clientssl, buf, sizeof(buf),
- &readbytes))
+ &readbytes))
+ || !TEST_size_t_eq(readbytes, sizeof(msg))
+ || !TEST_int_eq(memcmp(msg, buf, readbytes), 0)
+ || !TEST_true(SSL_read_ex(clientssl, buf, sizeof(buf),
+ &readbytes))
|| !TEST_size_t_eq(readbytes, sizeof(msg))
|| !TEST_int_eq(memcmp(msg, buf, readbytes), 0))
goto end;
@@ -5430,19 +5459,23 @@ static int test_shutdown(int tst)
*/
|| !TEST_false(SSL_write_ex(serverssl, msg, sizeof(msg), &written))
|| !TEST_int_eq(SSL_shutdown(clientssl), 1)
+ || !TEST_ptr_ne(sess = SSL_get_session(clientssl), NULL)
+ || !TEST_true(SSL_SESSION_is_resumable(sess))
|| !TEST_int_eq(SSL_shutdown(serverssl), 1))
goto end;
- } else if (tst == 4) {
+ } else if (tst == 4 || tst == 5) {
/*
* In this test the client has sent close_notify and it has been
* received by the server which has responded with a close_notify. The
* client needs to read the close_notify sent by the server.
*/
- if (!TEST_int_eq(SSL_shutdown(clientssl), 1))
+ if (!TEST_int_eq(SSL_shutdown(clientssl), 1)
+ || !TEST_ptr_ne(sess = SSL_get_session(clientssl), NULL)
+ || !TEST_true(SSL_SESSION_is_resumable(sess)))
goto end;
} else {
/*
- * tst == 5
+ * tst == 6
*
* The client has sent close_notify and is expecting a close_notify
* back, but instead there is application data first. The shutdown
@@ -5565,7 +5598,7 @@ int setup_tests(void)
ADD_ALL_TESTS(test_ssl_pending, 2);
ADD_ALL_TESTS(test_ssl_get_shared_ciphers, OSSL_NELEM(shared_ciphers_data));
ADD_ALL_TESTS(test_ticket_callbacks, 12);
- ADD_ALL_TESTS(test_shutdown, 6);
+ ADD_ALL_TESTS(test_shutdown, 7);
return 1;
}
More information about the openssl-commits
mailing list