The branch OpenSSL_1_1_1-stable has been updated via 2f0dab7e59cc50c89b6d54962b81cf96c30fe725 (commit) via 44bad9cbf7daa5ff7dd201e0c61e684b2e2eb971 (commit) via 910c8ffaf83a498667c10a28580dc18cbfd643c5 (commit) via a666af9f9df20c466ff5b5554610b5460cf3a362 (commit) via cf900cbc5c32bfd31a1d3d68a2bd94368a35aafe (commit) via d3133cc77cd0b052b6792d3e1edb9e5a202c6695 (commit) from 9011225188e0779833617516bdd76ab122fe2509 (commit)
- Log ----------------------------------------------------------------- commit 2f0dab7e59cc50c89b6d54962b81cf96c30fe725 Author: Benjamin Kaduk <bka...@akamai.com> Date: Fri Mar 6 13:19:45 2020 -0800 Add test that changes ciphers on CCS The TLS (pre-1.3) ChangeCipherState message is usually used to indicate the switch from the unencrypted to encrypted part of the handshake. However, it can also be used in cases where there is an existing session (such as during resumption handshakes) or when changing from one cipher to a different one (such as during renegotiation when the cipher list offered by the client has changed). This test serves to exercise such situations, allowing us to detect whether session objects are being modified in cases when they must remain immutable for thread-safety purposes. Reviewed-by: Tomas Mraz <tm...@fedoraproject.org> (Merged from https://github.com/openssl/openssl/pull/10943) (cherry picked from commit 3cd14e5e65011660ad8e3603cf871c8366b565fd) commit 44bad9cbf7daa5ff7dd201e0c61e684b2e2eb971 Author: Benjamin Kaduk <bka...@akamai.com> Date: Fri Jan 24 13:44:27 2020 -0800 Code to thread-safety in ChangeCipherState The server-side ChangeCipherState processing stores the new cipher in the SSL_SESSION object, so that the new state can be used if this session gets resumed. However, writing to the session is only thread-safe for initial handshakes, as at other times the session object may be in a shared cache and in use by another thread at the same time. Reflect this invariant in the code by only writing to s->session->cipher when it is currently NULL (we do not cache sessions with no cipher). The code prior to this change would never actually change the (non-NULL) cipher value in a session object, since our server enforces that (pre-TLS-1.3) resumptions use the exact same cipher as the initial connection, and non-abbreviated renegotiations have produced a new session object before we get to this point. Regardless, include logic to detect such a condition and abort the handshake if it occurs, to avoid any risk of inadvertently using the wrong cipher on a connection. Reviewed-by: Tomas Mraz <tm...@fedoraproject.org> (Merged from https://github.com/openssl/openssl/pull/10943) (cherry picked from commit 2e3ec2e1578977fca830a47fd7f521e290540e6d) commit 910c8ffaf83a498667c10a28580dc18cbfd643c5 Author: Benjamin Kaduk <bka...@akamai.com> Date: Fri Jan 24 13:25:53 2020 -0800 Don't write to the session when computing TLS 1.3 keys TLS 1.3 maintains a separate keys chedule in the SSL object, but was writing to the 'master_key_length' field in the SSL_SESSION when generating the per-SSL master_secret. (The generate_master_secret SSL3_ENC_METHOD function needs an output variable for the master secret length, but the TLS 1.3 implementation just uses the output size of the handshake hash function to get the lengths, so the only natural-looking thing to use as the output length was the field in the session. This would potentially involve writing to a SSL_SESSION object that was in the cache (i.e., resumed) and shared with other threads, though. The thread-safety impact should be minimal, since TLS 1.3 requires the hash from the original handshake to be associated with the resumption PSK and used for the subsequent connection. This means that (in the resumption case) the value being written would be the same value that was previously there, so the only risk would be on architectures that can produce torn writes/reads for aligned size_t values. Since the value is essentially ignored anyway, just provide the address of a local dummy variable to generate_master_secret() instead. Reviewed-by: Tomas Mraz <tm...@fedoraproject.org> (Merged from https://github.com/openssl/openssl/pull/10943) (cherry picked from commit d74014c4b8740f28a54b562f799ad1e754b517b9) commit a666af9f9df20c466ff5b5554610b5460cf3a362 Author: Benjamin Kaduk <bka...@akamai.com> Date: Fri Jan 24 13:25:02 2020 -0800 Fix whitespace nit in ssl_generate_master_secret() Use a space after a comma. Reviewed-by: Tomas Mraz <tm...@fedoraproject.org> (Merged from https://github.com/openssl/openssl/pull/10943) (cherry picked from commit 1866a0d380fc361d9be2ca0509de0f2281505db5) commit cf900cbc5c32bfd31a1d3d68a2bd94368a35aafe Author: Benjamin Kaduk <bka...@akamai.com> Date: Fri Jan 17 11:15:59 2020 -0800 doc: fix spelling of TYPE_get_ex_new_index The generated macros are TYPE_get_ex_new_index() (to match CRYPTO_get_ex_new_index()), not TYPE_get_new_ex_index(), even though the latter spelling seems more natural. Reviewed-by: Tomas Mraz <tm...@fedoraproject.org> (Merged from https://github.com/openssl/openssl/pull/10943) (cherry picked from commit fe41c06e69613b1a4814b3e3cdbf460f2678ec99) commit d3133cc77cd0b052b6792d3e1edb9e5a202c6695 Author: Benjamin Kaduk <bka...@akamai.com> Date: Thu Jan 16 14:37:44 2020 -0800 Additional updates to SSL_CTX_sess_set_get_cb.pod Generally modernize the language. Refer to TLS instead of SSL/TLS, and try to have more consistent usage of commas and that/which. Reword some descriptions to avoid implying that a list of potential reasons for behavior is an exhaustive list. Clarify how get_session_cb() is only called on servers (i.e., in general, and that it's given the session ID proposed by the client). Clarify the semantics of the get_cb()'s "copy" argument. The behavior seems to have changed in commit 8876bc054802b043a3ec95554b6c5873291770be, though the behavior prior to that commit was not to leave the reference-count unchanged if *copy was not written to -- instead, libssl seemed to assume that the callback already had incremented the reference count. Reviewed-by: Tomas Mraz <tm...@fedoraproject.org> (Merged from https://github.com/openssl/openssl/pull/10943) (cherry picked from commit 06f876837a8ec76b28c42953731a156c0c3700e2) ----------------------------------------------------------------------- Summary of changes: crypto/err/openssl.txt | 1 + doc/man3/BIO_get_ex_new_index.pod | 4 +- doc/man3/SSL_CTX_sess_set_get_cb.pod | 39 ++++++------ include/openssl/sslerr.h | 1 + ssl/s3_lib.c | 2 +- ssl/statem/statem_lib.c | 4 +- ssl/statem/statem_srvr.c | 14 ++++- test/sslapitest.c | 116 +++++++++++++++++++++++++++++++++++ 8 files changed, 157 insertions(+), 24 deletions(-) diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt index 10444a17f9..f5324c6819 100644 --- a/crypto/err/openssl.txt +++ b/crypto/err/openssl.txt @@ -1180,6 +1180,7 @@ SSL_F_OSSL_STATEM_SERVER_CONSTRUCT_MESSAGE:431:* SSL_F_OSSL_STATEM_SERVER_POST_PROCESS_MESSAGE:601:\ ossl_statem_server_post_process_message SSL_F_OSSL_STATEM_SERVER_POST_WORK:602:ossl_statem_server_post_work +SSL_F_OSSL_STATEM_SERVER_PRE_WORK:640: SSL_F_OSSL_STATEM_SERVER_PROCESS_MESSAGE:603:ossl_statem_server_process_message SSL_F_OSSL_STATEM_SERVER_READ_TRANSITION:418:ossl_statem_server_read_transition SSL_F_OSSL_STATEM_SERVER_WRITE_TRANSITION:604:\ diff --git a/doc/man3/BIO_get_ex_new_index.pod b/doc/man3/BIO_get_ex_new_index.pod index e61228f1ca..b063ccc5e4 100644 --- a/doc/man3/BIO_get_ex_new_index.pod +++ b/doc/man3/BIO_get_ex_new_index.pod @@ -39,7 +39,7 @@ L<CRYPTO_get_ex_new_index(3)>. These functions handle application-specific data for OpenSSL data structures. -TYPE_get_new_ex_index() is a macro that calls CRYPTO_get_ex_new_index() +TYPE_get_ex_new_index() is a macro that calls CRYPTO_get_ex_new_index() with the correct B<index> value. TYPE_set_ex_data() is a function that calls CRYPTO_set_ex_data() with @@ -50,7 +50,7 @@ an offset into the opaque exdata part of the TYPE object. =head1 RETURN VALUES -TYPE_get_new_ex_index() returns a new index on success or -1 on error. +TYPE_get_ex_new_index() returns a new index on success or -1 on error. TYPE_set_ex_data() returns 1 on success or 0 on error. diff --git a/doc/man3/SSL_CTX_sess_set_get_cb.pod b/doc/man3/SSL_CTX_sess_set_get_cb.pod index 11eda7e141..1b0f8a341b 100644 --- a/doc/man3/SSL_CTX_sess_set_get_cb.pod +++ b/doc/man3/SSL_CTX_sess_set_get_cb.pod @@ -28,19 +28,19 @@ SSL_CTX_sess_set_new_cb, SSL_CTX_sess_set_remove_cb, SSL_CTX_sess_set_get_cb, SS =head1 DESCRIPTION -SSL_CTX_sess_set_new_cb() sets the callback function, which is automatically +SSL_CTX_sess_set_new_cb() sets the callback function that is called whenever a new session was negotiated. -SSL_CTX_sess_set_remove_cb() sets the callback function, which is -automatically called whenever a session is removed by the SSL engine, -because it is considered faulty or the session has become obsolete because -of exceeding the timeout value. +SSL_CTX_sess_set_remove_cb() sets the callback function that is +called whenever a session is removed by the SSL engine. For example, +this can occur because a session is considered faulty or has become obsolete +because of exceeding the timeout value. -SSL_CTX_sess_set_get_cb() sets the callback function which is called, -whenever a SSL/TLS client proposed to resume a session but the session +SSL_CTX_sess_set_get_cb() sets the callback function that is called +whenever a TLS client proposed to resume a session but the session could not be found in the internal session cache (see L<SSL_CTX_set_session_cache_mode(3)>). -(SSL/TLS server only.) +(TLS server only.) SSL_CTX_sess_get_new_cb(), SSL_CTX_sess_get_remove_cb(), and SSL_CTX_sess_get_get_cb() retrieve the function pointers set by the @@ -56,7 +56,8 @@ L<d2i_SSL_SESSION(3)> interface. The new_session_cb() is called whenever a new session has been negotiated and session caching is enabled (see L<SSL_CTX_set_session_cache_mode(3)>). The -new_session_cb() is passed the B<ssl> connection and the ssl session B<sess>. +new_session_cb() is passed the B<ssl> connection and the nascent +ssl session B<sess>. Since sessions are reference-counted objects, the reference count on the session is incremented before the callback, on behalf of the application. If the callback returns B<0>, the session will be immediately removed from the @@ -78,21 +79,23 @@ In TLSv1.3 it is recommended that each SSL_SESSION object is only used for resumption once. One way of enforcing that is for applications to call L<SSL_CTX_remove_session(3)> after a session has been used. -The remove_session_cb() is called, whenever the SSL engine removes a session -from the internal cache. This happens when the session is removed because +The remove_session_cb() is called whenever the SSL engine removes a session +from the internal cache. This can happen when the session is removed because it is expired or when a connection was not shutdown cleanly. It also happens for all sessions in the internal session cache when L<SSL_CTX_free(3)> is called. The remove_session_cb() is passed the B<ctx> and the ssl session B<sess>. It does not provide any feedback. -The get_session_cb() is only called on SSL/TLS servers with the session id -proposed by the client. The get_session_cb() is always called, also when +The get_session_cb() is only called on SSL/TLS servers, and is given +the session id +proposed by the client. The get_session_cb() is always called, even when session caching was disabled. The get_session_cb() is passed the -B<ssl> connection, the session id of length B<length> at the memory location -B<data>. With the parameter B<copy> the callback can require the -SSL engine to increment the reference count of the SSL_SESSION object, -Normally the reference count is not incremented and therefore the -session must not be explicitly freed with +B<ssl> connection and the session id of length B<length> at the memory location +B<data>. By setting the parameter B<copy> to B<1>, the callback can require the +SSL engine to increment the reference count of the SSL_SESSION object; +setting B<copy> to B<0> causes the reference count to remain unchanged. +If the get_session_cb() does not write to B<copy>, the reference count +is incremented and the session must be explicitly freed with L<SSL_SESSION_free(3)>. =head1 RETURN VALUES diff --git a/include/openssl/sslerr.h b/include/openssl/sslerr.h index b6cac4f5f5..0ef684f3c1 100644 --- a/include/openssl/sslerr.h +++ b/include/openssl/sslerr.h @@ -88,6 +88,7 @@ int ERR_load_SSL_strings(void); # define SSL_F_OSSL_STATEM_SERVER_CONSTRUCT_MESSAGE 431 # define SSL_F_OSSL_STATEM_SERVER_POST_PROCESS_MESSAGE 601 # define SSL_F_OSSL_STATEM_SERVER_POST_WORK 602 +# define SSL_F_OSSL_STATEM_SERVER_PRE_WORK 640 # define SSL_F_OSSL_STATEM_SERVER_PROCESS_MESSAGE 603 # define SSL_F_OSSL_STATEM_SERVER_READ_TRANSITION 418 # define SSL_F_OSSL_STATEM_SERVER_WRITE_TRANSITION 604 diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c index 0cb3e8ccd3..dc44fa5c4b 100644 --- a/ssl/s3_lib.c +++ b/ssl/s3_lib.c @@ -4639,7 +4639,7 @@ int ssl_generate_master_secret(SSL *s, unsigned char *pms, size_t pmslen, OPENSSL_clear_free(s->s3->tmp.psk, psklen); s->s3->tmp.psk = NULL; if (!s->method->ssl3_enc->generate_master_secret(s, - s->session->master_key,pskpms, pskpmslen, + s->session->master_key, pskpms, pskpmslen, &s->session->master_key_length)) { OPENSSL_clear_free(pskpms, pskpmslen); /* SSLfatal() already called */ diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index 3ca4ce79a2..5882ecb2c3 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -844,9 +844,11 @@ MSG_PROCESS_RETURN tls_process_finished(SSL *s, PACKET *pkt) return MSG_PROCESS_ERROR; } } else { + /* TLS 1.3 gets the secret size from the handshake md */ + size_t dummy; if (!s->method->ssl3_enc->generate_master_secret(s, s->master_secret, s->handshake_secret, 0, - &s->session->master_key_length)) { + &dummy)) { /* SSLfatal() already called */ return MSG_PROCESS_ERROR; } diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index f08c09b33e..75619d9bca 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -743,7 +743,15 @@ WORK_STATE ossl_statem_server_pre_work(SSL *s, WORK_STATE wst) case TLS_ST_SW_CHANGE: if (SSL_IS_TLS13(s)) break; - s->session->cipher = s->s3->tmp.new_cipher; + /* Writes to s->session are only safe for initial handshakes */ + if (s->session->cipher == NULL) { + s->session->cipher = s->s3->tmp.new_cipher; + } else if (s->session->cipher != s->s3->tmp.new_cipher) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, + SSL_F_OSSL_STATEM_SERVER_PRE_WORK, + ERR_R_INTERNAL_ERROR); + return WORK_ERROR; + } if (!s->method->ssl3_enc->setup_key_block(s)) { /* SSLfatal() already called */ return WORK_ERROR; @@ -947,9 +955,11 @@ WORK_STATE ossl_statem_server_post_work(SSL *s, WORK_STATE wst) } #endif if (SSL_IS_TLS13(s)) { + /* TLS 1.3 gets the secret size from the handshake md */ + size_t dummy; if (!s->method->ssl3_enc->generate_master_secret(s, s->master_secret, s->handshake_secret, 0, - &s->session->master_key_length) + &dummy) || !s->method->ssl3_enc->change_cipher_state(s, SSL3_CC_APPLICATION | SSL3_CHANGE_CIPHER_SERVER_WRITE)) /* SSLfatal() already called */ diff --git a/test/sslapitest.c b/test/sslapitest.c index 94a3d5f5fd..f109563325 100644 --- a/test/sslapitest.c +++ b/test/sslapitest.c @@ -592,6 +592,121 @@ end: } #endif +/* + * Very focused test to exercise a single case in the server-side state + * machine, when the ChangeCipherState message needs to actually change + * from one cipher to a different cipher (i.e., not changing from null + * encryption to reall encryption). + */ +static int test_ccs_change_cipher(void) +{ + SSL_CTX *cctx = NULL, *sctx = NULL; + SSL *clientssl = NULL, *serverssl = NULL; + SSL_SESSION *sess = NULL, *sesspre, *sesspost; + int testresult = 0; + int i; + unsigned char buf; + size_t readbytes; + + /* + * Create a conection so we can resume and potentially (but not) use + * a different cipher in the second connection. + */ + if (!TEST_true(create_ssl_ctx_pair(TLS_server_method(), + TLS_client_method(), + TLS1_VERSION, TLS1_2_VERSION, + &sctx, &cctx, cert, privkey)) + || !TEST_true(SSL_CTX_set_options(sctx, SSL_OP_NO_TICKET)) + || !TEST_true(create_ssl_objects(sctx, cctx, &serverssl, &clientssl, + NULL, NULL)) + || !TEST_true(SSL_set_cipher_list(clientssl, "AES128-GCM-SHA256")) + || !TEST_true(create_ssl_connection(serverssl, clientssl, + SSL_ERROR_NONE)) + || !TEST_ptr(sesspre = SSL_get0_session(serverssl)) + || !TEST_ptr(sess = SSL_get1_session(clientssl))) + goto end; + + shutdown_ssl_connection(serverssl, clientssl); + serverssl = clientssl = NULL; + + /* Resume, preferring a different cipher. Our server will force the + * same cipher to be used as the initial handshake. */ + if (!TEST_true(create_ssl_objects(sctx, cctx, &serverssl, &clientssl, + NULL, NULL)) + || !TEST_true(SSL_set_session(clientssl, sess)) + || !TEST_true(SSL_set_cipher_list(clientssl, "AES256-GCM-SHA384:AES128-GCM-SHA256")) + || !TEST_true(create_ssl_connection(serverssl, clientssl, + SSL_ERROR_NONE)) + || !TEST_true(SSL_session_reused(clientssl)) + || !TEST_true(SSL_session_reused(serverssl)) + || !TEST_ptr(sesspost = SSL_get0_session(serverssl)) + || !TEST_ptr_eq(sesspre, sesspost) + || !TEST_int_eq(TLS1_CK_RSA_WITH_AES_128_GCM_SHA256, + SSL_CIPHER_get_id(SSL_get_current_cipher(clientssl)))) + goto end; + shutdown_ssl_connection(serverssl, clientssl); + serverssl = clientssl = NULL; + + /* + * Now create a fresh connection and try to renegotiate a different + * cipher on it. + */ + if (!TEST_true(create_ssl_ctx_pair(TLS_server_method(), + TLS_client_method(), + TLS1_VERSION, TLS1_2_VERSION, + &sctx, &cctx, cert, privkey)) + || !TEST_true(create_ssl_objects(sctx, cctx, &serverssl, &clientssl, + NULL, NULL)) + || !TEST_true(SSL_set_cipher_list(clientssl, "AES128-GCM-SHA256")) + || !TEST_true(create_ssl_connection(serverssl, clientssl, + SSL_ERROR_NONE)) + || !TEST_ptr(sesspre = SSL_get0_session(serverssl)) + || !TEST_true(SSL_set_cipher_list(clientssl, "AES256-GCM-SHA384")) + || !TEST_true(SSL_renegotiate(clientssl)) + || !TEST_true(SSL_renegotiate_pending(clientssl))) + goto end; + /* Actually drive the renegotiation. */ + for (i = 0; i < 3; i++) { + if (SSL_read_ex(clientssl, &buf, sizeof(buf), &readbytes) > 0) { + if (!TEST_ulong_eq(readbytes, 0)) + goto end; + } else if (!TEST_int_eq(SSL_get_error(clientssl, 0), + SSL_ERROR_WANT_READ)) { + goto end; + } + if (SSL_read_ex(serverssl, &buf, sizeof(buf), &readbytes) > 0) { + if (!TEST_ulong_eq(readbytes, 0)) + goto end; + } else if (!TEST_int_eq(SSL_get_error(serverssl, 0), + SSL_ERROR_WANT_READ)) { + goto end; + } + } + /* sesspre and sesspost should be different since the cipher changed. */ + if (!TEST_false(SSL_renegotiate_pending(clientssl)) + || !TEST_false(SSL_session_reused(clientssl)) + || !TEST_false(SSL_session_reused(serverssl)) + || !TEST_ptr(sesspost = SSL_get0_session(serverssl)) + || !TEST_ptr_ne(sesspre, sesspost) + || !TEST_int_eq(TLS1_CK_RSA_WITH_AES_256_GCM_SHA384, + SSL_CIPHER_get_id(SSL_get_current_cipher(clientssl)))) + goto end; + + shutdown_ssl_connection(serverssl, clientssl); + serverssl = clientssl = NULL; + + testresult = 1; + +end: + SSL_free(serverssl); + SSL_free(clientssl); + SSL_CTX_free(sctx); + SSL_CTX_free(cctx); + SSL_SESSION_free(sess); + + return testresult; +} + static int execute_test_large_message(const SSL_METHOD *smeth, const SSL_METHOD *cmeth, int min_version, int max_version, @@ -6423,6 +6538,7 @@ int setup_tests(void) #endif #ifndef OPENSSL_NO_TLS1_2 ADD_TEST(test_client_hello_cb); + ADD_TEST(test_ccs_change_cipher); #endif #ifndef OPENSSL_NO_TLS1_3 ADD_ALL_TESTS(test_early_data_read_write, 3);