From 8157d44b624da08142f3f9f6edc37fb5542c2573 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Wed, 28 Sep 2016 11:13:48 +0100 Subject: [PATCH] Convert ServerHello construction to WPACKET Reviewed-by: Rich Salz --- ssl/d1_srtp.c | 24 ----- ssl/ssl_locl.h | 8 +- ssl/statem/statem_srvr.c | 84 +++++++--------- ssl/t1_lib.c | 201 +++++++++++++++++---------------------- ssl/t1_reneg.c | 36 +++---- 5 files changed, 134 insertions(+), 219 deletions(-) diff --git a/ssl/d1_srtp.c b/ssl/d1_srtp.c index b5e5ef3f51..bcefb9ec3a 100644 --- a/ssl/d1_srtp.c +++ b/ssl/d1_srtp.c @@ -203,30 +203,6 @@ int ssl_parse_clienthello_use_srtp_ext(SSL *s, PACKET *pkt, int *al) return 0; } -int ssl_add_serverhello_use_srtp_ext(SSL *s, unsigned char *p, int *len, - int maxlen) -{ - if (p) { - if (maxlen < 5) { - SSLerr(SSL_F_SSL_ADD_SERVERHELLO_USE_SRTP_EXT, - SSL_R_SRTP_PROTECTION_PROFILE_LIST_TOO_LONG); - return 1; - } - - if (s->srtp_profile == 0) { - SSLerr(SSL_F_SSL_ADD_SERVERHELLO_USE_SRTP_EXT, - SSL_R_USE_SRTP_NOT_NEGOTIATED); - return 1; - } - s2n(2, p); - s2n(s->srtp_profile->id, p); - *p++ = 0; - } - *len = 5; - - return 0; -} - int ssl_parse_serverhello_use_srtp_ext(SSL *s, PACKET *pkt, int *al) { unsigned int id, ct, mki; diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index 630fea892e..1eddf9d885 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -2017,8 +2017,7 @@ __owur int tls1_shared_list(SSL *s, const unsigned char *l1, size_t l1len, const unsigned char *l2, size_t l2len, int nmatch); __owur int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al); -__owur unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf, - unsigned char *limit, int *al); +__owur int ssl_add_serverhello_tlsext(SSL *s, WPACKET *pkt, int *al); __owur int ssl_parse_clienthello_tlsext(SSL *s, PACKET *pkt); void ssl_set_default_md(SSL *s); __owur int tls1_set_server_sigalgs(SSL *s); @@ -2066,8 +2065,7 @@ __owur int ssl_security_cert_chain(SSL *s, STACK_OF(X509) *sk, X509 *ex, __owur EVP_MD_CTX *ssl_replace_hash(EVP_MD_CTX **hash, const EVP_MD *md); void ssl_clear_hash_ctx(EVP_MD_CTX **hash); -__owur int ssl_add_serverhello_renegotiate_ext(SSL *s, unsigned char *p, - int *len, int maxlen); +__owur int ssl_add_serverhello_renegotiate_ext(SSL *s, WPACKET *pkt); __owur int ssl_parse_serverhello_renegotiate_ext(SSL *s, PACKET *pkt, int *al); __owur int ssl_parse_clienthello_renegotiate_ext(SSL *s, PACKET *pkt, int *al); __owur long ssl_get_algorithm2(SSL *s); @@ -2084,8 +2082,6 @@ void ssl_set_client_disabled(SSL *s); __owur int ssl_cipher_disabled(SSL *s, const SSL_CIPHER *c, int op); __owur int ssl_parse_clienthello_use_srtp_ext(SSL *s, PACKET *pkt, int *al); -__owur int ssl_add_serverhello_use_srtp_ext(SSL *s, unsigned char *p, int *len, - int maxlen); __owur int ssl_parse_serverhello_use_srtp_ext(SSL *s, PACKET *pkt, int *al); __owur int ssl_handshake_hash(SSL *s, unsigned char *out, int outlen); diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index 8a2791ad77..190d198e3b 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -1499,26 +1499,23 @@ WORK_STATE tls_post_process_client_hello(SSL *s, WORK_STATE wst) int tls_construct_server_hello(SSL *s) { - unsigned char *buf; - unsigned char *p, *d; - int i, sl; - int al = 0; - unsigned long l; - - buf = (unsigned char *)s->init_buf->data; - - /* Do the message type and length last */ - d = p = ssl_handshake_start(s); - - *(p++) = s->version >> 8; - *(p++) = s->version & 0xff; + int sl; + int al = SSL_AD_INTERNAL_ERROR; + int compm; + size_t len; + WPACKET pkt; - /* - * Random stuff. Filling of the server_random takes place in - * tls_process_client_hello() - */ - memcpy(p, s->s3->server_random, SSL3_RANDOM_SIZE); - p += SSL3_RANDOM_SIZE; + if (!WPACKET_init(&pkt, s->init_buf) + || !ssl_set_handshake_header2(s, &pkt, SSL3_MT_SERVER_HELLO) + || !WPACKET_put_bytes_u16(&pkt, s->version) + /* + * Random stuff. Filling of the server_random takes place in + * tls_process_client_hello() + */ + || !WPACKET_memcpy(&pkt, s->s3->server_random, SSL3_RANDOM_SIZE)) { + SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_HELLO, ERR_R_INTERNAL_ERROR); + goto err; + } /*- * There are several cases for the session ID to send @@ -1544,50 +1541,35 @@ int tls_construct_server_hello(SSL *s) sl = s->session->session_id_length; if (sl > (int)sizeof(s->session->session_id)) { SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_HELLO, ERR_R_INTERNAL_ERROR); - ossl_statem_set_error(s); - return 0; + goto err; } - *(p++) = sl; - memcpy(p, s->session->session_id, sl); - p += sl; - /* put the cipher */ - i = ssl3_put_cipher_by_char_old(s->s3->tmp.new_cipher, p); - p += i; - - /* put the compression method */ + /* set up the compression method */ #ifdef OPENSSL_NO_COMP - *(p++) = 0; + compm = 0; #else if (s->s3->tmp.new_compression == NULL) - *(p++) = 0; + compm = 0; else - *(p++) = s->s3->tmp.new_compression->id; + compm = s->s3->tmp.new_compression->id; #endif - if (ssl_prepare_serverhello_tlsext(s) <= 0) { - SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_HELLO, SSL_R_SERVERHELLO_TLSEXT); - ossl_statem_set_error(s); - return 0; - } - if ((p = - ssl_add_serverhello_tlsext(s, p, buf + SSL3_RT_MAX_PLAIN_LENGTH, - &al)) == NULL) { - ssl3_send_alert(s, SSL3_AL_FATAL, al); - SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_HELLO, ERR_R_INTERNAL_ERROR); - ossl_statem_set_error(s); - return 0; - } - - /* do the header */ - l = (p - d); - if (!ssl_set_handshake_header(s, SSL3_MT_SERVER_HELLO, l)) { + if (!WPACKET_sub_memcpy_u8(&pkt, s->session->session_id, sl) + || !s->method->put_cipher_by_char(s->s3->tmp.new_cipher, &pkt, &len) + || !WPACKET_put_bytes_u8(&pkt, compm) + || !ssl_prepare_serverhello_tlsext(s) + || !ssl_add_serverhello_tlsext(s, &pkt, &al) + || !ssl_close_construct_packet(s, &pkt)) { SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_HELLO, ERR_R_INTERNAL_ERROR); - ossl_statem_set_error(s); - return 0; + goto err; } return 1; + err: + WPACKET_cleanup(&pkt); + ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR); + ossl_statem_set_error(s); + return 0; } int tls_construct_server_done(SSL *s) diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 40932fa985..0a73eed434 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -1392,12 +1392,8 @@ int ssl_add_clienthello_tlsext(SSL *s, WPACKET *pkt, int *al) return 1; } -unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf, - unsigned char *limit, int *al) +int ssl_add_serverhello_tlsext(SSL *s, WPACKET *pkt, int *al) { - int extdatalen = 0; - unsigned char *orig = buf; - unsigned char *ret = buf; #ifndef OPENSSL_NO_NEXTPROTONEG int next_proto_neg_seen; #endif @@ -1408,30 +1404,16 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf, using_ecc = using_ecc && (s->session->tlsext_ecpointformatlist != NULL); #endif - ret += 2; - if (ret >= limit) - return NULL; /* this really never occurs, but ... */ - - if (s->s3->send_connection_binding) { - int el; - - if (!ssl_add_serverhello_renegotiate_ext(s, 0, &el, 0)) { - SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); - return NULL; - } - - if ((limit - ret - 4 - el) < 0) - return NULL; - - s2n(TLSEXT_TYPE_renegotiate, ret); - s2n(el, ret); - - if (!ssl_add_serverhello_renegotiate_ext(s, ret, &el, el)) { - SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); - return NULL; - } + if (!WPACKET_start_sub_packet_u16(pkt) + || !WPACKET_set_flags(pkt, WPACKET_FLAGS_ABANDON_ON_ZERO_LENGTH)) { + SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); + return 0; + } - ret += el; + if (s->s3->send_connection_binding && + !ssl_add_serverhello_renegotiate_ext(s, pkt)) { + SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); + return 0; } /* Only add RI for SSLv3 */ @@ -1439,12 +1421,12 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf, goto done; if (!s->hit && s->servername_done == 1 - && s->session->tlsext_hostname != NULL) { - if ((long)(limit - ret - 4) < 0) - return NULL; - - s2n(TLSEXT_TYPE_server_name, ret); - s2n(0, ret); + && s->session->tlsext_hostname != NULL) { + if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_server_name) + || !WPACKET_put_bytes_u16(pkt, 0)) { + SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); + return 0; + } } #ifndef OPENSSL_NO_EC if (using_ecc) { @@ -1453,25 +1435,15 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf, /* * Add TLS extension ECPointFormats to the ServerHello message */ - long lenmax; - tls1_get_formatlist(s, &plist, &plistlen); - if ((lenmax = limit - ret - 5) < 0) - return NULL; - if (plistlen > (size_t)lenmax) - return NULL; - if (plistlen > 255) { + if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_ec_point_formats) + || !WPACKET_start_sub_packet_u16(pkt) + || !WPACKET_sub_memcpy_u8(pkt, plist, plistlen) + || !WPACKET_close(pkt)) { SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); - return NULL; + return 0; } - - s2n(TLSEXT_TYPE_ec_point_formats, ret); - s2n(plistlen + 1, ret); - *(ret++) = (unsigned char)plistlen; - memcpy(ret, plist, plistlen); - ret += plistlen; - } /* * Currently the server should not respond with a SupportedCurves @@ -1480,10 +1452,11 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf, #endif /* OPENSSL_NO_EC */ if (s->tlsext_ticket_expected && tls_use_ticket(s)) { - if ((long)(limit - ret - 4) < 0) - return NULL; - s2n(TLSEXT_TYPE_session_ticket, ret); - s2n(0, ret); + if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_session_ticket) + || !WPACKET_put_bytes_u16(pkt, 0)) { + SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); + return 0; + } } else { /* * if we don't add the above TLSEXT, we can't add a session ticket @@ -1493,31 +1466,23 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf, } if (s->tlsext_status_expected) { - if ((long)(limit - ret - 4) < 0) - return NULL; - s2n(TLSEXT_TYPE_status_request, ret); - s2n(0, ret); + if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_status_request) + || !WPACKET_put_bytes_u16(pkt, 0)) { + SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); + return 0; + } } #ifndef OPENSSL_NO_SRTP if (SSL_IS_DTLS(s) && s->srtp_profile) { - int el; - - /* Returns 0 on success!! */ - if (ssl_add_serverhello_use_srtp_ext(s, 0, &el, 0)) { - SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); - return NULL; - } - if ((limit - ret - 4 - el) < 0) - return NULL; - - s2n(TLSEXT_TYPE_use_srtp, ret); - s2n(el, ret); - - if (ssl_add_serverhello_use_srtp_ext(s, ret, &el, el)) { + if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_use_srtp) + || !WPACKET_start_sub_packet_u16(pkt) + || !WPACKET_put_bytes_u16(pkt, 2) + || !WPACKET_put_bytes_u16(pkt, s->srtp_profile->id) + || !WPACKET_put_bytes_u8(pkt, 0) + || !WPACKET_close(pkt)) { SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); - return NULL; + return 0; } - ret += el; } #endif @@ -1532,28 +1497,31 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf, 0x2a, 0x85, 0x03, 0x02, 0x02, 0x16, 0x30, 0x08, 0x06, 0x06, 0x2a, 0x85, 0x03, 0x02, 0x02, 0x17 }; - if (limit - ret < 36) - return NULL; - memcpy(ret, cryptopro_ext, 36); - ret += 36; - + if (!WPACKET_memcpy(pkt, cryptopro_ext, sizeof(cryptopro_ext))) { + SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); + return 0; + } } #ifndef OPENSSL_NO_HEARTBEATS /* Add Heartbeat extension if we've received one */ if (SSL_IS_DTLS(s) && (s->tlsext_heartbeat & SSL_DTLSEXT_HB_ENABLED)) { - if ((limit - ret - 4 - 1) < 0) - return NULL; - s2n(TLSEXT_TYPE_heartbeat, ret); - s2n(1, ret); /*- * Set mode: * 1: peer may send requests * 2: peer not allowed to send requests */ if (s->tlsext_heartbeat & SSL_DTLSEXT_HB_DONT_RECV_REQUESTS) - *(ret++) = SSL_DTLSEXT_HB_DONT_SEND_REQUESTS; + mode = SSL_DTLSEXT_HB_DONT_SEND_REQUESTS; else - *(ret++) = SSL_DTLSEXT_HB_ENABLED; + mode = SSL_DTLSEXT_HB_ENABLED; + + if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_heartbeat) + || !WPACKET_start_sub_packet_u16(pkt) + || !WPACKET_put_bytes_u8(pkt, mode) + || !WPACKET_close(pkt)) { + SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); + return 0; + } } #endif @@ -1570,18 +1538,20 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf, s-> ctx->next_protos_advertised_cb_arg); if (r == SSL_TLSEXT_ERR_OK) { - if ((long)(limit - ret - 4 - npalen) < 0) - return NULL; - s2n(TLSEXT_TYPE_next_proto_neg, ret); - s2n(npalen, ret); - memcpy(ret, npa, npalen); - ret += npalen; + if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_next_proto_neg) + || !WPACKET_sub_memcpy_u16(pkt, npa, npalen)) { + SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); + return 0; + } s->s3->next_proto_neg_seen = 1; } } #endif - if (!custom_ext_add_old(s, 1, &ret, limit, al)) - return NULL; + if (!custom_ext_add(s, 1, pkt, al)) { + SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); + return 0; + } + if (s->s3->flags & TLS1_FLAGS_ENCRYPT_THEN_MAC) { /* * Don't use encrypt_then_mac if AEAD or RC4 might want to disable @@ -1593,36 +1563,41 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf, || s->s3->tmp.new_cipher->algorithm_enc == SSL_eGOST2814789CNT12) s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC; else { - s2n(TLSEXT_TYPE_encrypt_then_mac, ret); - s2n(0, ret); + if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_encrypt_then_mac) + || !WPACKET_put_bytes_u16(pkt, 0)) { + SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); + return 0; + } } } if (s->s3->flags & TLS1_FLAGS_RECEIVED_EXTMS) { - s2n(TLSEXT_TYPE_extended_master_secret, ret); - s2n(0, ret); + if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_extended_master_secret) + || !WPACKET_put_bytes_u16(pkt, 0)) { + SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); + return 0; + } } if (s->s3->alpn_selected != NULL) { - const unsigned char *selected = s->s3->alpn_selected; - unsigned int len = s->s3->alpn_selected_len; - - if ((long)(limit - ret - 4 - 2 - 1 - len) < 0) - return NULL; - s2n(TLSEXT_TYPE_application_layer_protocol_negotiation, ret); - s2n(3 + len, ret); - s2n(1 + len, ret); - *ret++ = len; - memcpy(ret, selected, len); - ret += len; + if (!WPACKET_put_bytes_u16(pkt, + TLSEXT_TYPE_application_layer_protocol_negotiation) + || !WPACKET_start_sub_packet_u16(pkt) + || !WPACKET_start_sub_packet_u16(pkt) + || !WPACKET_sub_memcpy_u8(pkt, s->s3->alpn_selected, + s->s3->alpn_selected_len) + || !WPACKET_close(pkt) + || !WPACKET_close(pkt)) { + SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); + return 0; + } } done: - - if ((extdatalen = ret - orig - 2) == 0) - return orig; - - s2n(extdatalen, orig); - return ret; + if (!WPACKET_close(pkt)) { + SSLerr(SSL_F_SSL_ADD_SERVERHELLO_TLSEXT, ERR_R_INTERNAL_ERROR); + return 0; + } + return 1; } /* diff --git a/ssl/t1_reneg.c b/ssl/t1_reneg.c index f5136e26cd..f3e01bb833 100644 --- a/ssl/t1_reneg.c +++ b/ssl/t1_reneg.c @@ -50,32 +50,18 @@ int ssl_parse_clienthello_renegotiate_ext(SSL *s, PACKET *pkt, int *al) } /* Add the server's renegotiation binding */ -int ssl_add_serverhello_renegotiate_ext(SSL *s, unsigned char *p, int *len, - int maxlen) +int ssl_add_serverhello_renegotiate_ext(SSL *s, WPACKET *pkt) { - if (p) { - if ((s->s3->previous_client_finished_len + - s->s3->previous_server_finished_len + 1) > maxlen) { - SSLerr(SSL_F_SSL_ADD_SERVERHELLO_RENEGOTIATE_EXT, - SSL_R_RENEGOTIATE_EXT_TOO_LONG); - return 0; - } - - /* Length byte */ - *p = s->s3->previous_client_finished_len + - s->s3->previous_server_finished_len; - p++; - - memcpy(p, s->s3->previous_client_finished, - s->s3->previous_client_finished_len); - p += s->s3->previous_client_finished_len; - - memcpy(p, s->s3->previous_server_finished, - s->s3->previous_server_finished_len); - } - - *len = s->s3->previous_client_finished_len - + s->s3->previous_server_finished_len + 1; + if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_renegotiate) + || !WPACKET_start_sub_packet_u16(pkt) + || !WPACKET_start_sub_packet_u8(pkt) + || !WPACKET_memcpy(pkt, s->s3->previous_client_finished, + s->s3->previous_client_finished_len) + || !WPACKET_memcpy(pkt, s->s3->previous_server_finished, + s->s3->previous_server_finished_len) + || !WPACKET_close(pkt) + || !WPACKET_close(pkt)) + return 0; return 1; } -- GitLab