[openssl-commits] [openssl] master update
kaduk at mit.edu
kaduk at mit.edu
Fri Jul 20 13:35:05 UTC 2018
The branch master has been updated
via c5d1fb78fd0fdbe1f1e61211bd56192a0f95bc91 (commit)
via 1c4aa31d79821dee9be98e915159d52cc30d8403 (commit)
via 4cc968df403ed9321d0df722aba33323ae575ce0 (commit)
from f20aa69e33a7b418e052cf210374e2267cb93a5c (commit)
- Log -----------------------------------------------------------------
commit c5d1fb78fd0fdbe1f1e61211bd56192a0f95bc91
Author: Benjamin Kaduk <bkaduk at akamai.com>
Date: Wed May 30 11:12:22 2018 -0500
Add TODO comment for a nonsensical public API
The API used to set what SNI value to send in the ClientHello
can also be used on server SSL objects, with undocumented and
un-useful behavior. Unfortunately, when generic SSL_METHODs
are used, s->server is still set, prior to the start of the
handshake, so we cannot prevent this nonsensical usage at the
present time. Leave a note to revisit this when ABI-breaking
changes are permitted.
Reviewed-by: Matt Caswell <matt at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6378)
commit 1c4aa31d79821dee9be98e915159d52cc30d8403
Author: Benjamin Kaduk <bkaduk at akamai.com>
Date: Wed May 30 09:49:29 2018 -0500
Normalize SNI hostname handling for SSL and SSL_SESSION
In particular, adhere to the rule that we must not modify any
property of an SSL_SESSION object once it is (or might be) in
a session cache. Such modifications are thread-unsafe and have
been observed to cause crashes at runtime.
To effect this change, standardize on the property that
SSL_SESSION->ext.hostname is set only when that SNI value
has been negotiated by both parties for use with that session.
For session resumption this is trivially the case, so only new
handshakes are affected.
On the client, the new semantics are that the SSL->ext.hostname is
for storing the value configured by the caller, and this value is
used when constructing the ClientHello. On the server, SSL->ext.hostname
is used to hold the value received from the client. Only if the
SNI negotiation is successful will the hostname be stored into the
session object; the server can do this after it sends the ServerHello,
and the client after it has received and processed the ServerHello.
This obviates the need to remove the hostname from the session object
in case of failed negotiation (a change that was introduced in commit
9fb6cb810b769abbd60f11ef6e936a4e4456b19d in order to allow TLS 1.3
early data when SNI was present in the ClientHello but not the session
being resumed), which was modifying cached sessions in certain cases.
(In TLS 1.3 we always produce a new SSL_SESSION object for new
connections, even in the case of resumption, so no TLS 1.3 handshakes
were affected.)
Reviewed-by: Matt Caswell <matt at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6378)
commit 4cc968df403ed9321d0df722aba33323ae575ce0
Author: Benjamin Kaduk <bkaduk at akamai.com>
Date: Wed May 30 09:28:03 2018 -0500
const-ify some input SSL * arguments
These tiny functions only read from the input SSL, and we are
about to use them from functions that only have a const SSL* available,
so propagate const a bit further.
Reviewed-by: Matt Caswell <matt at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/6378)
-----------------------------------------------------------------------
Summary of changes:
doc/man3/SSL_in_init.pod | 6 +++---
include/openssl/ssl.h | 6 +++---
ssl/s3_lib.c | 9 +++++++++
ssl/ssl_lib.c | 10 +++++++++-
ssl/ssl_sess.c | 9 ---------
ssl/statem/extensions.c | 25 ++++++++++++++++++++++---
ssl/statem/extensions_srvr.c | 19 ++++++++++++-------
ssl/statem/statem.c | 6 +++---
8 files changed, 61 insertions(+), 29 deletions(-)
diff --git a/doc/man3/SSL_in_init.pod b/doc/man3/SSL_in_init.pod
index 37ebff6..d8467a9 100644
--- a/doc/man3/SSL_in_init.pod
+++ b/doc/man3/SSL_in_init.pod
@@ -14,9 +14,9 @@ SSL_get_state
#include <openssl/ssl.h>
- int SSL_in_init(SSL *s);
- int SSL_in_before(SSL *s);
- int SSL_is_init_finished(SSL *s);
+ int SSL_in_init(const SSL *s);
+ int SSL_in_before(const SSL *s);
+ int SSL_is_init_finished(const SSL *s);
int SSL_in_connect_init(SSL *s);
int SSL_in_accept_init(SSL *s);
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 2376828..155d651 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -1058,9 +1058,9 @@ typedef enum {
/* Is the SSL_connection established? */
# define SSL_in_connect_init(a) (SSL_in_init(a) && !SSL_is_server(a))
# define SSL_in_accept_init(a) (SSL_in_init(a) && SSL_is_server(a))
-int SSL_in_init(SSL *s);
-int SSL_in_before(SSL *s);
-int SSL_is_init_finished(SSL *s);
+int SSL_in_init(const SSL *s);
+int SSL_in_before(const SSL *s);
+int SSL_is_init_finished(const SSL *s);
/*
* The following 3 states are kept in ssl->rlayer.rstate when reads fail, you
diff --git a/ssl/s3_lib.c b/ssl/s3_lib.c
index 354769b..c170eed 100644
--- a/ssl/s3_lib.c
+++ b/ssl/s3_lib.c
@@ -3466,6 +3466,15 @@ long ssl3_ctrl(SSL *s, int cmd, long larg, void *parg)
break;
#endif /* !OPENSSL_NO_EC */
case SSL_CTRL_SET_TLSEXT_HOSTNAME:
+ /*
+ * TODO(OpenSSL1.2)
+ * This API is only used for a client to set what SNI it will request
+ * from the server, but we currently allow it to be used on servers
+ * as well, which is a programming error. Currently we just clear
+ * the field in SSL_do_handshake() for server SSLs, but when we can
+ * make ABI-breaking changes, we may want to make use of this API
+ * an error on server SSLs.
+ */
if (larg == TLSEXT_NAMETYPE_host_name) {
size_t len;
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 38391fd..10a7694 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -2612,7 +2612,15 @@ const char *SSL_get_servername(const SSL *s, const int type)
if (type != TLSEXT_NAMETYPE_host_name)
return NULL;
- return s->session && !s->ext.hostname ?
+ /*
+ * TODO(OpenSSL1.2) clean up this compat mess. This API is
+ * currently a mix of "what did I configure" and "what did the
+ * peer send" and "what was actually negotiated"; we should have
+ * a clear distinction amongst those three.
+ */
+ if (SSL_in_init(s))
+ return s->ext.hostname;
+ return (s->session != NULL && s->ext.hostname == NULL) ?
s->session->ext.hostname : s->ext.hostname;
}
diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c
index 628b9f0..d4a4808 100644
--- a/ssl/ssl_sess.c
+++ b/ssl/ssl_sess.c
@@ -421,15 +421,6 @@ int ssl_get_new_session(SSL *s, int session)
return 0;
}
- if (s->ext.hostname) {
- ss->ext.hostname = OPENSSL_strdup(s->ext.hostname);
- if (ss->ext.hostname == NULL) {
- SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_SSL_GET_NEW_SESSION,
- ERR_R_INTERNAL_ERROR);
- SSL_SESSION_free(ss);
- return 0;
- }
- }
} else {
ss->session_id_length = 0;
}
diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index 5309b12..85945ac 100644
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -929,9 +929,28 @@ static int final_server_name(SSL *s, unsigned int context, int sent)
ret = s->session_ctx->ext.servername_cb(s, &altmp,
s->session_ctx->ext.servername_arg);
- if (!sent) {
- OPENSSL_free(s->session->ext.hostname);
- s->session->ext.hostname = NULL;
+ /*
+ * For servers, propagate the SNI hostname from the temporary
+ * storage in the SSL to the persistent SSL_SESSION, now that we
+ * know we accepted it.
+ * Clients make this copy when parsing the server's response to
+ * the extension, which is when they find out that the negotiation
+ * was successful.
+ */
+ if (s->server) {
+ if (!sent) {
+ /* Nothing from the client this handshake; cleanup stale value */
+ OPENSSL_free(s->ext.hostname);
+ s->ext.hostname = NULL;
+ } else if (ret == SSL_TLSEXT_ERR_OK && (!s->hit || SSL_IS_TLS13(s))) {
+ /* Only store the hostname in the session if we accepted it. */
+ OPENSSL_free(s->session->ext.hostname);
+ s->session->ext.hostname = OPENSSL_strdup(s->ext.hostname);
+ if (s->session->ext.hostname == NULL && s->ext.hostname != NULL) {
+ SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_FINAL_SERVER_NAME,
+ ERR_R_INTERNAL_ERROR);
+ }
+ }
}
/*
diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c
index f5ab5bb..00c0ec9 100644
--- a/ssl/statem/extensions_srvr.c
+++ b/ssl/statem/extensions_srvr.c
@@ -127,7 +127,7 @@ int tls_parse_ctos_server_name(SSL *s, PACKET *pkt, unsigned int context,
return 0;
}
- if (!s->hit) {
+ if (!s->hit || SSL_IS_TLS13(s)) {
if (PACKET_remaining(&hostname) > TLSEXT_MAXLEN_host_name) {
SSLfatal(s, SSL_AD_UNRECOGNIZED_NAME,
SSL_F_TLS_PARSE_CTOS_SERVER_NAME,
@@ -142,21 +142,26 @@ int tls_parse_ctos_server_name(SSL *s, PACKET *pkt, unsigned int context,
return 0;
}
- OPENSSL_free(s->session->ext.hostname);
- s->session->ext.hostname = NULL;
- if (!PACKET_strndup(&hostname, &s->session->ext.hostname)) {
+ /*
+ * Store the requested SNI in the SSL as temporary storage.
+ * If we accept it, it will get stored in the SSL_SESSION as well.
+ */
+ OPENSSL_free(s->ext.hostname);
+ s->ext.hostname = NULL;
+ if (!PACKET_strndup(&hostname, &s->ext.hostname)) {
SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_PARSE_CTOS_SERVER_NAME,
ERR_R_INTERNAL_ERROR);
return 0;
}
s->servername_done = 1;
- } else {
+ }
+ if (s->hit) {
/*
* TODO(openssl-team): if the SNI doesn't match, we MUST
* fall back to a full handshake.
*/
- s->servername_done = s->session->ext.hostname
+ s->servername_done = (s->session->ext.hostname != NULL)
&& PACKET_equal(&hostname, s->session->ext.hostname,
strlen(s->session->ext.hostname));
@@ -1325,7 +1330,7 @@ EXT_RETURN tls_construct_stoc_server_name(SSL *s, WPACKET *pkt,
size_t chainidx)
{
if (s->hit || s->servername_done != 1
- || s->session->ext.hostname == NULL)
+ || s->ext.hostname == NULL)
return EXT_RETURN_NOT_SENT;
if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_server_name)
diff --git a/ssl/statem/statem.c b/ssl/statem/statem.c
index cf6472c..7f1017d 100644
--- a/ssl/statem/statem.c
+++ b/ssl/statem/statem.c
@@ -68,17 +68,17 @@ OSSL_HANDSHAKE_STATE SSL_get_state(const SSL *ssl)
return ssl->statem.hand_state;
}
-int SSL_in_init(SSL *s)
+int SSL_in_init(const SSL *s)
{
return s->statem.in_init;
}
-int SSL_is_init_finished(SSL *s)
+int SSL_is_init_finished(const SSL *s)
{
return !(s->statem.in_init) && (s->statem.hand_state == TLS_ST_OK);
}
-int SSL_in_before(SSL *s)
+int SSL_in_before(const SSL *s)
{
/*
* Historically being "in before" meant before anything had happened. In the
More information about the openssl-commits
mailing list