[openssl] master update
kaduk at mit.edu
kaduk at mit.edu
Fri Mar 13 22:02:38 UTC 2020
The branch master has been updated
via 3cd14e5e65011660ad8e3603cf871c8366b565fd (commit)
via 2e3ec2e1578977fca830a47fd7f521e290540e6d (commit)
via d74014c4b8740f28a54b562f799ad1e754b517b9 (commit)
via 1866a0d380fc361d9be2ca0509de0f2281505db5 (commit)
via fe41c06e69613b1a4814b3e3cdbf460f2678ec99 (commit)
via 06f876837a8ec76b28c42953731a156c0c3700e2 (commit)
from c08dea30d4d127412097b39d9974ba6090041a7c (commit)
- Log -----------------------------------------------------------------
commit 3cd14e5e65011660ad8e3603cf871c8366b565fd
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)
commit 2e3ec2e1578977fca830a47fd7f521e290540e6d
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)
commit d74014c4b8740f28a54b562f799ad1e754b517b9
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)
commit 1866a0d380fc361d9be2ca0509de0f2281505db5
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)
commit fe41c06e69613b1a4814b3e3cdbf460f2678ec99
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)
commit 06f876837a8ec76b28c42953731a156c0c3700e2
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)
-----------------------------------------------------------------------
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 c921207698..4073891de0 100644
--- a/crypto/err/openssl.txt
+++ b/crypto/err/openssl.txt
@@ -1310,6 +1310,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 0bacb2e0cb..e7fa5a1e16 100644
--- a/doc/man3/BIO_get_ex_new_index.pod
+++ b/doc/man3/BIO_get_ex_new_index.pod
@@ -53,7 +53,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
@@ -74,7 +74,7 @@ there are no backward compatibility concerns.
=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 98fbfb57bf..3804e5080f 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 25e304ed10..8ccdf3dc6b 100644
--- a/include/openssl/sslerr.h
+++ b/include/openssl/sslerr.h
@@ -94,6 +94,7 @@ int ERR_load_SSL_strings(void);
# define SSL_F_OSSL_STATEM_SERVER_CONSTRUCT_MESSAGE 0
# define SSL_F_OSSL_STATEM_SERVER_POST_PROCESS_MESSAGE 0
# define SSL_F_OSSL_STATEM_SERVER_POST_WORK 0
+# define SSL_F_OSSL_STATEM_SERVER_PRE_WORK 0
# define SSL_F_OSSL_STATEM_SERVER_PROCESS_MESSAGE 0
# define SSL_F_OSSL_STATEM_SERVER_READ_TRANSITION 0
# define SSL_F_OSSL_STATEM_SERVER_WRITE_TRANSITION 0
diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c
index 51f8a0f63d..ffdf3a90fb 100644
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -4642,7 +4642,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 c5956ea37c..812dabe860 100644
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -860,9 +860,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 ab032ae956..1cc106876c 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -744,7 +744,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;
@@ -948,9 +956,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 94719b23ac..642f676a45 100644
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -651,6 +651,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,
@@ -7214,6 +7329,7 @@ int setup_tests(void)
#ifndef OPENSSL_NO_TLS1_2
ADD_TEST(test_client_hello_cb);
ADD_TEST(test_no_ems);
+ 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