From 9c14752f8872401de413fb46a96146b0d6bf926e Mon Sep 17 00:00:00 2001 From: Alex Klyubin Date: Tue, 8 Apr 2014 16:02:24 -0700 Subject: tls_psk_hint Fix TLS-PSK identity hint implementation issues. PSK identity hint can be stored in SSL_CTX and in SSL/SSL_SESSION, similar to other TLS parameters, with the value in SSL/SSL_SESSION taking precedence over the one in SSL_CTX. The value in SSL_CTX is shared (used as the default) between all SSL instances associated with that SSL_CTX, whereas the value in SSL/SSL_SESSION is confined to that particular TLS/SSL connection/session. The existing implementation of TLS-PSK does not correctly distinguish between PSK identity hint in SSL_CTX and in SSL/SSL_SESSION. This change fixes these issues: 1. SSL_use_psk_identity_hint does nothing and returns "success" when the SSL object does not have an associated SSL_SESSION. 2. On the client, the hint in SSL_CTX (which is shared between multiple SSL instances) is overwritten with the hint received from server or reset to NULL if no hint was received. 3. On the client, psk_client_callback is invoked with the hint from SSL_CTX rather than from current SSL/SSL_SESSION (i.e., the one received from the server). Issue #2 above masks this issue. 4. On the server, the hint in SSL/SSL_SESSION is ignored and the hint from SSL_CTX is sent to the client. 5. On the server, the hint in SSL/SSL_SESSION is reset to the one in SSL_CTX after the ClientKeyExchange message step. This change fixes the issues by: * Adding storage for the hint in the SSL object. The idea being that the hint in the associated SSL_SESSION takes precedence. * Reading the hint during the handshake only from the associated SSL_SESSION object. * Initializing the hint in SSL object with the one from the SSL_CTX object. * Initializing the hint in SSL_SESSION object with the one from the SSL object. * Making SSL_use_psk_identity_hint and SSL_get_psk_identity_hint set/get the hint to/from SSL_SESSION associated with the provided SSL object, or, if no SSL_SESSION is available, set/get the hint to/from the provided SSL object. * Removing code which resets the hint during handshake. --- ssl/d1_clnt.c | 13 +------------ ssl/d1_srvr.c | 10 +++++----- ssl/s3_clnt.c | 37 +++++++++++++------------------------ ssl/s3_srvr.c | 44 ++++++++++++++++---------------------------- ssl/ssl.h | 4 ++++ ssl/ssl_lib.c | 56 +++++++++++++++++++++++++++++++++++++++++++++----------- ssl/ssl_sess.c | 12 ++++++++++++ 7 files changed, 96 insertions(+), 80 deletions(-) diff --git a/ssl/d1_clnt.c b/ssl/d1_clnt.c index f857946..b017139 100644 --- a/ssl/d1_clnt.c +++ b/ssl/d1_clnt.c @@ -1434,7 +1434,7 @@ int dtls1_send_client_key_exchange(SSL *s) goto err; } - psk_len = s->psk_client_callback(s, s->ctx->psk_identity_hint, + psk_len = s->psk_client_callback(s, s->session->psk_identity_hint, identity, PSK_MAX_IDENTITY_LEN, psk_or_pre_ms, sizeof(psk_or_pre_ms)); if (psk_len > PSK_MAX_PSK_LEN) @@ -1459,17 +1459,6 @@ int dtls1_send_client_key_exchange(SSL *s) t+=psk_len; s2n(psk_len, t); - if (s->session->psk_identity_hint != NULL) - OPENSSL_free(s->session->psk_identity_hint); - s->session->psk_identity_hint = BUF_strdup(s->ctx->psk_identity_hint); - if (s->ctx->psk_identity_hint != NULL && - s->session->psk_identity_hint == NULL) - { - SSLerr(SSL_F_DTLS1_SEND_CLIENT_KEY_EXCHANGE, - ERR_R_MALLOC_FAILURE); - goto psk_err; - } - if (s->session->psk_identity != NULL) OPENSSL_free(s->session->psk_identity); s->session->psk_identity = BUF_strdup(identity); diff --git a/ssl/d1_srvr.c b/ssl/d1_srvr.c index 1384ab0..c181db6 100644 --- a/ssl/d1_srvr.c +++ b/ssl/d1_srvr.c @@ -471,7 +471,7 @@ int dtls1_accept(SSL *s) /* PSK: send ServerKeyExchange if PSK identity * hint if provided */ #ifndef OPENSSL_NO_PSK - || ((alg_k & SSL_kPSK) && s->ctx->psk_identity_hint) + || ((alg_k & SSL_kPSK) && s->session->psk_identity_hint) #endif || (alg_k & (SSL_kEDH|SSL_kDHr|SSL_kDHd)) || (alg_k & SSL_kEECDH) @@ -1288,7 +1288,7 @@ int dtls1_send_server_key_exchange(SSL *s) if (type & SSL_kPSK) { /* reserve size for record length and PSK identity hint*/ - n+=2+strlen(s->ctx->psk_identity_hint); + n+=2+strlen(s->session->psk_identity_hint); } else #endif /* !OPENSSL_NO_PSK */ @@ -1365,9 +1365,9 @@ int dtls1_send_server_key_exchange(SSL *s) if (type & SSL_kPSK) { /* copy PSK identity hint */ - s2n(strlen(s->ctx->psk_identity_hint), p); - strncpy((char *)p, s->ctx->psk_identity_hint, strlen(s->ctx->psk_identity_hint)); - p+=strlen(s->ctx->psk_identity_hint); + s2n(strlen(s->session->psk_identity_hint), p); + strncpy((char *)p, s->session->psk_identity_hint, strlen(s->session->psk_identity_hint)); + p+=strlen(s->session->psk_identity_hint); } #endif diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c index 12c3fe8..17367a2 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c @@ -1374,9 +1374,11 @@ int ssl3_get_key_exchange(SSL *s) if (s->s3->tmp.new_cipher->algorithm_auth & SSL_aPSK) { s->session->sess_cert=ssl_sess_cert_new(); - if (s->ctx->psk_identity_hint) - OPENSSL_free(s->ctx->psk_identity_hint); - s->ctx->psk_identity_hint = NULL; + if (s->session->psk_identity_hint) + { + OPENSSL_free(s->session->psk_identity_hint); + s->session->psk_identity_hint = NULL; + } } #endif s->s3->tmp.reuse_message=1; @@ -1426,7 +1428,11 @@ int ssl3_get_key_exchange(SSL *s) al=SSL_AD_HANDSHAKE_FAILURE; n2s(p,i); param_len=i+2; - s->ctx->psk_identity_hint = NULL; + if (s->session->psk_identity_hint) + { + OPENSSL_free(s->session->psk_identity_hint); + s->session->psk_identity_hint = NULL; + } if (i != 0) { /* Store PSK identity hint for later use, hint is used @@ -1452,10 +1458,8 @@ int ssl3_get_key_exchange(SSL *s) * NULL-terminated string. */ memcpy(tmp_id_hint, p, i); memset(tmp_id_hint+i, 0, PSK_MAX_IDENTITY_LEN+1-i); - if (s->ctx->psk_identity_hint != NULL) - OPENSSL_free(s->ctx->psk_identity_hint); - s->ctx->psk_identity_hint = BUF_strdup(tmp_id_hint); - if (s->ctx->psk_identity_hint == NULL) + s->session->psk_identity_hint = BUF_strdup(tmp_id_hint); + if (s->session->psk_identity_hint == NULL) { SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, ERR_R_MALLOC_FAILURE); goto f_err; @@ -2338,7 +2342,7 @@ int ssl3_send_client_key_exchange(SSL *s) goto err; } - psk_len = s->psk_client_callback(s, s->ctx->psk_identity_hint, + psk_len = s->psk_client_callback(s, s->session->psk_identity_hint, identity, PSK_MAX_IDENTITY_LEN, psk, sizeof(psk)); if (psk_len > PSK_MAX_PSK_LEN) { @@ -2374,21 +2378,6 @@ int ssl3_send_client_key_exchange(SSL *s) n += 2; } - if (s->session->psk_identity_hint != NULL) - OPENSSL_free(s->session->psk_identity_hint); - s->session->psk_identity_hint = NULL; - if (s->ctx->psk_identity_hint) - { - s->session->psk_identity_hint = BUF_strdup(s->ctx->psk_identity_hint); - if (s->ctx->psk_identity_hint != NULL && - s->session->psk_identity_hint == NULL) - { - SSLerr(SSL_F_SSL3_SEND_CLIENT_KEY_EXCHANGE, - ERR_R_MALLOC_FAILURE); - goto psk_err; - } - } - if (s->session->psk_identity != NULL) OPENSSL_free(s->session->psk_identity); s->session->psk_identity = BUF_strdup(identity); diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index d6f1a35..c360337 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -492,7 +492,7 @@ int ssl3_accept(SSL *s) * - the key exchange is kEECDH. */ #ifndef OPENSSL_NO_PSK - || ((alg_a & SSL_aPSK) && ((alg_k & SSL_kEECDH) || s->ctx->psk_identity_hint)) + || ((alg_a & SSL_aPSK) && ((alg_k & SSL_kEECDH) || s->session->psk_identity_hint)) #endif #ifndef OPENSSL_NO_SRP /* SRP: send ServerKeyExchange */ @@ -1702,6 +1702,10 @@ int ssl3_send_server_key_exchange(SSL *s) int curve_id = 0; BN_CTX *bn_ctx = NULL; #endif +#ifndef OPENSSL_NO_PSK + const char* psk_identity_hint; + size_t psk_identity_hint_len; +#endif EVP_PKEY *pkey; const EVP_MD *md = NULL; unsigned char *p,*d; @@ -1730,9 +1734,12 @@ int ssl3_send_server_key_exchange(SSL *s) if (alg_a & SSL_aPSK) { /* size for PSK identity hint */ - n+=2; - if (s->ctx->psk_identity_hint) - n+=strlen(s->ctx->psk_identity_hint); + psk_identity_hint = s->session->psk_identity_hint; + if (psk_identity_hint) + psk_identity_hint_len = strlen(psk_identity_hint); + else + psk_identity_hint_len = 0; + n+=2+psk_identity_hint_len; } #endif /* !OPENSSL_NO_PSK */ #ifndef OPENSSL_NO_RSA @@ -2025,20 +2032,12 @@ int ssl3_send_server_key_exchange(SSL *s) #ifndef OPENSSL_NO_PSK if (alg_a & SSL_aPSK) { - if (s->ctx->psk_identity_hint) - { - /* copy PSK identity hint */ - s2n(strlen(s->ctx->psk_identity_hint), p); - strncpy((char *)p, s->ctx->psk_identity_hint, strlen(s->ctx->psk_identity_hint)); - p+=strlen(s->ctx->psk_identity_hint); - } - else + /* copy PSK identity hint (if provided) */ + s2n(psk_identity_hint_len, p); + if (psk_identity_hint_len > 0) { - /* No identity hint is provided. */ - *p = 0; - p += 1; - *p = 0; - p += 1; + memcpy(p, psk_identity_hint, psk_identity_hint_len); + p+=psk_identity_hint_len; } } #endif /* OPENSSL_NO_PSK */ @@ -2393,17 +2392,6 @@ int ssl3_get_client_key_exchange(SSL *s) goto psk_err; } - if (s->session->psk_identity_hint != NULL) - OPENSSL_free(s->session->psk_identity_hint); - s->session->psk_identity_hint = BUF_strdup(s->ctx->psk_identity_hint); - if (s->ctx->psk_identity_hint != NULL && - s->session->psk_identity_hint == NULL) - { - SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, - ERR_R_MALLOC_FAILURE); - goto psk_err; - } - p += i; n -= (i + 2); psk_err = 0; diff --git a/ssl/ssl.h b/ssl/ssl.h index a7e1455..f044cd1 100644 --- a/ssl/ssl.h +++ b/ssl/ssl.h @@ -1441,6 +1441,10 @@ struct ssl_st #endif /* OPENSSL_NO_KRB5 */ #ifndef OPENSSL_NO_PSK + /* PSK identity hint is stored here only to enable setting a hint on an SSL object before an + * SSL_SESSION is associated with it. Once an SSL_SESSION is associated with this SSL object, + * the psk_identity_hint from the session takes precedence over this one. */ + char *psk_identity_hint; unsigned int (*psk_client_callback)(SSL *ssl, const char *hint, char *identity, unsigned int max_identity_len, unsigned char *psk, unsigned int max_psk_len); diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 3e49cab..cf24292 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -388,6 +388,13 @@ SSL *SSL_new(SSL_CTX *ctx) CRYPTO_new_ex_data(CRYPTO_EX_INDEX_SSL, s, &s->ex_data); #ifndef OPENSSL_NO_PSK + s->psk_identity_hint = NULL; + if (ctx->psk_identity_hint) + { + s->psk_identity_hint = BUF_strdup(ctx->psk_identity_hint); + if (s->psk_identity_hint == NULL) + goto err; + } s->psk_client_callback=ctx->psk_client_callback; s->psk_server_callback=ctx->psk_server_callback; #endif @@ -648,6 +655,11 @@ void SSL_free(SSL *s) OPENSSL_free(s->alpn_client_proto_list); #endif +#ifndef OPENSSL_NO_PSK + if (s->psk_identity_hint) + OPENSSL_free(s->psk_identity_hint); +#endif + if (s->client_CA != NULL) sk_X509_NAME_pop_free(s->client_CA,X509_NAME_free); @@ -3361,32 +3373,54 @@ int SSL_use_psk_identity_hint(SSL *s, const char *identity_hint) if (s == NULL) return 0; - if (s->session == NULL) - return 1; /* session not created yet, ignored */ - if (identity_hint != NULL && strlen(identity_hint) > PSK_MAX_IDENTITY_LEN) { SSLerr(SSL_F_SSL_USE_PSK_IDENTITY_HINT, SSL_R_DATA_LENGTH_TOO_LONG); return 0; } - if (s->session->psk_identity_hint != NULL) + + /* Clear hint in SSL and associated SSL_SESSION (if any). */ + if (s->psk_identity_hint != NULL) + { + OPENSSL_free(s->psk_identity_hint); + s->psk_identity_hint = NULL; + } + if (s->session != NULL && s->session->psk_identity_hint != NULL) + { OPENSSL_free(s->session->psk_identity_hint); + s->session->psk_identity_hint = NULL; + } + if (identity_hint != NULL) { - s->session->psk_identity_hint = BUF_strdup(identity_hint); - if (s->session->psk_identity_hint == NULL) - return 0; + /* The hint is stored in SSL and SSL_SESSION with the one in + * SSL_SESSION taking precedence. Thus, if SSL_SESSION is avaiable, + * we store the hint there, otherwise we store it in SSL. */ + if (s->session != NULL) + { + s->session->psk_identity_hint = BUF_strdup(identity_hint); + if (s->session->psk_identity_hint == NULL) + return 0; + } + else + { + s->psk_identity_hint = BUF_strdup(identity_hint); + if (s->psk_identity_hint == NULL) + return 0; + } } - else - s->session->psk_identity_hint = NULL; return 1; } const char *SSL_get_psk_identity_hint(const SSL *s) { - if (s == NULL || s->session == NULL) + if (s == NULL) return NULL; - return(s->session->psk_identity_hint); + /* The hint is stored in SSL and SSL_SESSION with the one in SSL_SESSION + * taking precedence. */ + if (s->session != NULL) + return(s->session->psk_identity_hint); + return(s->psk_identity_hint); } const char *SSL_get_psk_identity(const SSL *s) diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c index 44268e7..cdd198c 100644 --- a/ssl/ssl_sess.c +++ b/ssl/ssl_sess.c @@ -437,6 +437,18 @@ int ssl_get_new_session(SSL *s, int session) } #endif #endif +#ifndef OPENSSL_NO_PSK + if (s->psk_identity_hint) + { + ss->psk_identity_hint = BUF_strdup(s->psk_identity_hint); + if (ss->psk_identity_hint == NULL) + { + SSLerr(SSL_F_SSL_GET_NEW_SESSION, ERR_R_MALLOC_FAILURE); + SSL_SESSION_free(ss); + return 0; + } + } +#endif } else { -- 2.0.0.526.g5318336