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 <m...@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.edlin...@hotmail.de> Reviewed-by: Kurt Roeckx <k...@roeckx.be> (Merged from https://github.com/openssl/openssl/pull/6340) commit ba70904949d2f9eec160043bf9a97182b33a2b82 Author: Matt Caswell <m...@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.edlin...@hotmail.de> Reviewed-by: Kurt Roeckx <k...@roeckx.be> (Merged from https://github.com/openssl/openssl/pull/6340) commit c748834ff7af7949519d2820a79ec35e809b5a71 Author: Matt Caswell <m...@openssl.org> Date: Wed May 23 12:11:15 2018 +0100 Add a bi-directional shutdown test Reviewed-by: Bernd Edlinger <bernd.edlin...@hotmail.de> Reviewed-by: Kurt Roeckx <k...@roeckx.be> (Merged from https://github.com/openssl/openssl/pull/6340) commit 93f528f36eb9423c31b2d75669cea85da97f9633 Author: Matt Caswell <m...@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.edlin...@hotmail.de> Reviewed-by: Kurt Roeckx <k...@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); _____ openssl-commits mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-commits