[openssl] OpenSSL_1_1_1-stable update
kaduk at mit.edu
kaduk at mit.edu
Fri Mar 13 23:31:33 UTC 2020
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 <bkaduk at 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 <tmraz at fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/10943)
(cherry picked from commit 3cd14e5e65011660ad8e3603cf871c8366b565fd)
commit 44bad9cbf7daa5ff7dd201e0c61e684b2e2eb971
Author: Benjamin Kaduk <bkaduk at 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 <tmraz at fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/10943)
(cherry picked from commit 2e3ec2e1578977fca830a47fd7f521e290540e6d)
commit 910c8ffaf83a498667c10a28580dc18cbfd643c5
Author: Benjamin Kaduk <bkaduk at 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 <tmraz at fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/10943)
(cherry picked from commit d74014c4b8740f28a54b562f799ad1e754b517b9)
commit a666af9f9df20c466ff5b5554610b5460cf3a362
Author: Benjamin Kaduk <bkaduk at 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 <tmraz at fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/10943)
(cherry picked from commit 1866a0d380fc361d9be2ca0509de0f2281505db5)
commit cf900cbc5c32bfd31a1d3d68a2bd94368a35aafe
Author: Benjamin Kaduk <bkaduk at 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 <tmraz at fedoraproject.org>
(Merged from https://github.com/openssl/openssl/pull/10943)
(cherry picked from commit fe41c06e69613b1a4814b3e3cdbf460f2678ec99)
commit d3133cc77cd0b052b6792d3e1edb9e5a202c6695
Author: Benjamin Kaduk <bkaduk at 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 <tmraz at 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);
More information about the openssl-commits
mailing list