[openssl-commits] [openssl] master update
Matt Caswell
matt at openssl.org
Wed Jun 27 09:08:00 UTC 2018
The branch master has been updated
via 358ffa05cd3a088822c7d06256bc87516d918798 (commit)
via ba70904949d2f9eec160043bf9a97182b33a2b82 (commit)
via c748834ff7af7949519d2820a79ec35e809b5a71 (commit)
via 93f528f36eb9423c31b2d75669cea85da97f9633 (commit)
from c7504aeb640a88949dfe3146f7e0f275f517464c (commit)
- Log -----------------------------------------------------------------
commit 358ffa05cd3a088822c7d06256bc87516d918798
Author: Matt Caswell <matt at openssl.org>
Date: Mon Jun 25 14:51:11 2018 +0100
Return a fatal error if application data is encountered during shutdown
Currently if you encounter application data while waiting for a
close_notify from the peer, and you have called SSL_shutdown() then
you will get a -1 return (fatal error) and SSL_ERROR_SYSCALL from
SSL_get_error(). This isn't accurate (it should be SSL_ERROR_SSL) and
isn't persistent (you can call SSL_shutdown() again and it might then work).
We change this into a proper fatal error that is persistent.
Reviewed-by: Bernd Edlinger <bernd.edlinger at hotmail.de>
Reviewed-by: Kurt Roeckx <kurt at roeckx.be>
(Merged from https://github.com/openssl/openssl/pull/6340)
commit ba70904949d2f9eec160043bf9a97182b33a2b82
Author: Matt Caswell <matt at openssl.org>
Date: Thu Jun 21 13:30:38 2018 +0100
Return SSL_ERROR_WANT_READ if SSL_shutdown() encounters handshake data
In the case where we are shutdown for writing and awaiting a close_notify
back from a subsequent SSL_shutdown() call we skip over handshake data
that is received. This should not be treated as an error - instead it
should be signalled with SSL_ERROR_WANT_READ.
Reviewed-by: Bernd Edlinger <bernd.edlinger at hotmail.de>
Reviewed-by: Kurt Roeckx <kurt at roeckx.be>
(Merged from https://github.com/openssl/openssl/pull/6340)
commit c748834ff7af7949519d2820a79ec35e809b5a71
Author: Matt Caswell <matt at openssl.org>
Date: Wed May 23 12:11:15 2018 +0100
Add a bi-directional shutdown test
Reviewed-by: Bernd Edlinger <bernd.edlinger at hotmail.de>
Reviewed-by: Kurt Roeckx <kurt at roeckx.be>
(Merged from https://github.com/openssl/openssl/pull/6340)
commit 93f528f36eb9423c31b2d75669cea85da97f9633
Author: Matt Caswell <matt at openssl.org>
Date: Wed May 23 12:00:10 2018 +0100
Auto retry if we ditch records during shutdown
If we've sent a close_notify and we're waiting for one back we drop
incoming records until we see the close_notify we're looking for. If
SSL_MODE_AUTO_RETRY is on, then we should immediately try and read the
next record.
Fixes #6262
Reviewed-by: Bernd Edlinger <bernd.edlinger at hotmail.de>
Reviewed-by: Kurt Roeckx <kurt at roeckx.be>
(Merged from https://github.com/openssl/openssl/pull/6340)
-----------------------------------------------------------------------
Summary of changes:
crypto/err/openssl.txt | 2 +
include/openssl/sslerr.h | 1 +
ssl/record/rec_layer_s3.c | 100 +++++++++++++++++++++--------------
ssl/ssl_err.c | 2 +
test/sslapitest.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++
test/ssltestlib.c | 26 ++++++++--
test/ssltestlib.h | 1 +
7 files changed, 220 insertions(+), 42 deletions(-)
diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt
index e65a806..ee68388 100644
--- a/crypto/err/openssl.txt
+++ b/crypto/err/openssl.txt
@@ -2544,6 +2544,8 @@ SM2_R_INVALID_ENCODING:104:invalid encoding
SM2_R_INVALID_FIELD:105:invalid field
SM2_R_NO_PARAMETERS_SET:109:no parameters set
SM2_R_USER_ID_TOO_LARGE:106:user id too large
+SSL_R_APPLICATION_DATA_AFTER_CLOSE_NOTIFY:291:\
+ application data after close notify
SSL_R_APP_DATA_IN_HANDSHAKE:100:app data in handshake
SSL_R_ATTEMPT_TO_REUSE_SESSION_IN_DIFFERENT_CONTEXT:272:\
attempt to reuse session in different context
diff --git a/include/openssl/sslerr.h b/include/openssl/sslerr.h
index b2c6c1e..9eba6d8 100644
--- a/include/openssl/sslerr.h
+++ b/include/openssl/sslerr.h
@@ -449,6 +449,7 @@ int ERR_load_SSL_strings(void);
/*
* SSL reason codes.
*/
+# define SSL_R_APPLICATION_DATA_AFTER_CLOSE_NOTIFY 291
# define SSL_R_APP_DATA_IN_HANDSHAKE 100
# define SSL_R_ATTEMPT_TO_REUSE_SESSION_IN_DIFFERENT_CONTEXT 272
# define SSL_R_AT_LEAST_TLS_1_0_NEEDED_IN_FIPS_MODE 143
diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c
index 8d5b53f..1628ac8 100644
--- a/ssl/record/rec_layer_s3.c
+++ b/ssl/record/rec_layer_s3.c
@@ -1457,40 +1457,6 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
return -1;
}
- /*
- * In case of record types for which we have 'fragment' storage, fill
- * that so that we can process the data at a fixed place.
- */
- {
- size_t dest_maxlen = 0;
- unsigned char *dest = NULL;
- size_t *dest_len = NULL;
-
- if (SSL3_RECORD_get_type(rr) == SSL3_RT_HANDSHAKE) {
- dest_maxlen = sizeof(s->rlayer.handshake_fragment);
- dest = s->rlayer.handshake_fragment;
- dest_len = &s->rlayer.handshake_fragment_len;
- }
-
- if (dest_maxlen > 0) {
- n = dest_maxlen - *dest_len; /* available space in 'dest' */
- if (SSL3_RECORD_get_length(rr) < n)
- n = SSL3_RECORD_get_length(rr); /* available bytes */
-
- /* now move 'n' bytes: */
- memcpy(dest + *dest_len,
- SSL3_RECORD_get_data(rr) + SSL3_RECORD_get_off(rr), n);
- SSL3_RECORD_add_off(rr, n);
- SSL3_RECORD_sub_length(rr, n);
- *dest_len += n;
- if (SSL3_RECORD_get_length(rr) == 0)
- SSL3_RECORD_set_read(rr);
-
- if (*dest_len < dest_maxlen)
- goto start; /* fragment was too small */
- }
- }
-
/*-
* s->rlayer.handshake_fragment_len == 4 iff rr->type == SSL3_RT_HANDSHAKE;
* (Possibly rr is 'empty' now, i.e. rr->length may be 0.)
@@ -1583,12 +1549,70 @@ int ssl3_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
return -1;
}
- if (s->shutdown & SSL_SENT_SHUTDOWN) { /* but we have not received a
- * shutdown */
- s->rwstate = SSL_NOTHING;
+ /*
+ * 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);
- return 0;
+
+ if (SSL3_RECORD_get_type(rr) == SSL3_RT_HANDSHAKE) {
+ BIO *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);
+ } else {
+ /*
+ * The peer is continuing to send application data, but we have
+ * already sent close_notify. If this was expected we should have
+ * been called via SSL_read() and this would have been handled
+ * above.
+ * No alert sent because we already sent close_notify
+ */
+ SSLfatal(s, SSL_AD_NO_ALERT, SSL_F_SSL3_READ_BYTES,
+ SSL_R_APPLICATION_DATA_AFTER_CLOSE_NOTIFY);
+ }
+ return -1;
+ }
+
+ /*
+ * For handshake data we have 'fragment' storage, so fill that so that we
+ * can process the header at a fixed place. This is done after the
+ * "SHUTDOWN" code above to avoid filling the fragment storage with data
+ * that we're just going to discard.
+ */
+ if (SSL3_RECORD_get_type(rr) == SSL3_RT_HANDSHAKE) {
+ size_t dest_maxlen = sizeof(s->rlayer.handshake_fragment);
+ unsigned char *dest = s->rlayer.handshake_fragment;
+ size_t *dest_len = &s->rlayer.handshake_fragment_len;
+
+ n = dest_maxlen - *dest_len; /* available space in 'dest' */
+ if (SSL3_RECORD_get_length(rr) < n)
+ n = SSL3_RECORD_get_length(rr); /* available bytes */
+
+ /* now move 'n' bytes: */
+ memcpy(dest + *dest_len,
+ SSL3_RECORD_get_data(rr) + SSL3_RECORD_get_off(rr), n);
+ SSL3_RECORD_add_off(rr, n);
+ SSL3_RECORD_sub_length(rr, n);
+ *dest_len += n;
+ if (SSL3_RECORD_get_length(rr) == 0)
+ SSL3_RECORD_set_read(rr);
+
+ if (*dest_len < dest_maxlen)
+ goto start; /* fragment was too small */
}
if (SSL3_RECORD_get_type(rr) == SSL3_RT_CHANGE_CIPHER_SPEC) {
diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c
index 03c5bf2..9ce643a 100644
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -726,6 +726,8 @@ static const ERR_STRING_DATA SSL_str_functs[] = {
};
static const ERR_STRING_DATA SSL_str_reasons[] = {
+ {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_APPLICATION_DATA_AFTER_CLOSE_NOTIFY),
+ "application data after close notify"},
{ERR_PACK(ERR_LIB_SSL, 0, SSL_R_APP_DATA_IN_HANDSHAKE),
"app data in handshake"},
{ERR_PACK(ERR_LIB_SSL, 0, SSL_R_ATTEMPT_TO_REUSE_SESSION_IN_DIFFERENT_CONTEXT),
diff --git a/test/sslapitest.c b/test/sslapitest.c
index 61619a3..baf0881 100644
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -4972,6 +4972,135 @@ static int test_ticket_callbacks(int tst)
return testresult;
}
+/*
+ * Test bi-directional shutdown.
+ * Test 0: TLSv1.2
+ * 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
+ * doesn't read it
+ */
+static int test_shutdown(int tst)
+{
+ SSL_CTX *cctx = NULL, *sctx = NULL;
+ SSL *clientssl = NULL, *serverssl = NULL;
+ int testresult = 0;
+ char msg[] = "A test message";
+ char buf[80];
+ size_t written, readbytes;
+
+#ifdef OPENSSL_NO_TLS1_2
+ if (tst == 0)
+ return 1;
+#endif
+#ifdef OPENSSL_NO_TLS1_3
+ if (tst != 0)
+ return 1;
+#endif
+
+ if (!TEST_true(create_ssl_ctx_pair(TLS_server_method(),
+ TLS_client_method(),
+ TLS1_VERSION,
+ (tst <= 1) ? TLS1_2_VERSION
+ : TLS1_3_VERSION,
+ &sctx, &cctx, cert, privkey))
+ || !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)))
+ goto end;
+ } else if (!TEST_true(create_ssl_connection(serverssl, clientssl,
+ SSL_ERROR_NONE))) {
+ goto end;
+ }
+
+ if (!TEST_int_eq(SSL_shutdown(clientssl), 0))
+ goto end;
+
+ if (tst >= 4) {
+ /*
+ * Reading on the server after the client has sent close_notify should
+ * fail and provide SSL_ERROR_ZERO_RETURN
+ */
+ if (!TEST_false(SSL_read_ex(serverssl, buf, sizeof(buf), &readbytes))
+ || !TEST_int_eq(SSL_get_error(serverssl, 0),
+ SSL_ERROR_ZERO_RETURN)
+ || !TEST_int_eq(SSL_get_shutdown(serverssl),
+ SSL_RECEIVED_SHUTDOWN)
+ /*
+ * 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))
+ goto end;
+ if (tst == 4) {
+ /* Should still be able to read data from server */
+ if (!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;
+ }
+ }
+
+ /* Writing on the client after sending close_notify shouldn't be possible */
+ if (!TEST_false(SSL_write_ex(clientssl, msg, sizeof(msg), &written)))
+ goto end;
+
+ if (tst < 4) {
+ /*
+ * For these tests the client has sent close_notify but it has not yet
+ * been received by the server. The server has not sent close_notify
+ * yet.
+ */
+ if (!TEST_int_eq(SSL_shutdown(serverssl), 0)
+ /*
+ * Writing on the server after sending close_notify shouldn't
+ * be possible.
+ */
+ || !TEST_false(SSL_write_ex(serverssl, msg, sizeof(msg), &written))
+ || !TEST_int_eq(SSL_shutdown(clientssl), 1)
+ || !TEST_int_eq(SSL_shutdown(serverssl), 1))
+ goto end;
+ } else if (tst == 4) {
+ /*
+ * 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))
+ goto end;
+ } else {
+ /*
+ * tst == 5
+ *
+ * The client has sent close_notify and is expecting a close_notify
+ * back, but instead there is application data first. The shutdown
+ * should fail with a fatal error.
+ */
+ if (!TEST_int_eq(SSL_shutdown(clientssl), -1)
+ || !TEST_int_eq(SSL_get_error(clientssl, -1), SSL_ERROR_SSL))
+ goto end;
+ }
+
+ testresult = 1;
+
+ end:
+ SSL_free(serverssl);
+ SSL_free(clientssl);
+ SSL_CTX_free(sctx);
+ SSL_CTX_free(cctx);
+
+ return testresult;
+}
+
int setup_tests(void)
{
if (!TEST_ptr(cert = test_get_argument(0))
@@ -5069,6 +5198,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);
return 1;
}
diff --git a/test/ssltestlib.c b/test/ssltestlib.c
index 2ef4b5d..a055d3b 100644
--- a/test/ssltestlib.c
+++ b/test/ssltestlib.c
@@ -680,12 +680,14 @@ int create_ssl_objects(SSL_CTX *serverctx, SSL_CTX *clientctx, SSL **sssl,
return 0;
}
-int create_ssl_connection(SSL *serverssl, SSL *clientssl, int want)
+/*
+ * Create an SSL connection, but does not ready any post-handshake
+ * NewSessionTicket messages.
+ */
+int create_bare_ssl_connection(SSL *serverssl, SSL *clientssl, int want)
{
- int retc = -1, rets = -1, err, abortctr = 0, i;
+ int retc = -1, rets = -1, err, abortctr = 0;
int clienterr = 0, servererr = 0;
- unsigned char buf;
- size_t readbytes;
int isdtls = SSL_is_dtls(serverssl);
do {
@@ -738,6 +740,22 @@ int create_ssl_connection(SSL *serverssl, SSL *clientssl, int want)
}
} while (retc <=0 || rets <= 0);
+ return 1;
+}
+
+/*
+ * Create an SSL connection including any post handshake NewSessionTicket
+ * messages.
+ */
+int create_ssl_connection(SSL *serverssl, SSL *clientssl, int want)
+{
+ int i;
+ unsigned char buf;
+ size_t readbytes;
+
+ if (!create_bare_ssl_connection(serverssl, clientssl, want))
+ return 0;
+
/*
* We attempt to read some data on the client side which we expect to fail.
* This will ensure we have received the NewSessionTicket in TLSv1.3 where
diff --git a/test/ssltestlib.h b/test/ssltestlib.h
index c96dff5..31e3037 100644
--- a/test/ssltestlib.h
+++ b/test/ssltestlib.h
@@ -18,6 +18,7 @@ int create_ssl_ctx_pair(const SSL_METHOD *sm, const SSL_METHOD *cm,
char *privkeyfile);
int create_ssl_objects(SSL_CTX *serverctx, SSL_CTX *clientctx, SSL **sssl,
SSL **cssl, BIO *s_to_c_fbio, BIO *c_to_s_fbio);
+int create_bare_ssl_connection(SSL *serverssl, SSL *clientssl, int want);
int create_ssl_connection(SSL *serverssl, SSL *clientssl, int want);
void shutdown_ssl_connection(SSL *serverssl, SSL *clientssl);
More information about the openssl-commits
mailing list