From 380a522f689252e7f19e0c44ea49461ec7bd040f Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Fri, 19 May 2017 09:35:19 +0100 Subject: [PATCH] Replace instances of OPENSSL_assert() with soft asserts in libssl Reviewed-by: Tim Hudson (Merged from https://github.com/openssl/openssl/pull/3496) --- ssl/record/rec_layer_d1.c | 9 ++++--- ssl/record/record_locl.h | 2 +- ssl/record/ssl3_record.c | 52 ++++++++++++++++++++++++++++++--------- ssl/s3_cbc.c | 16 +++++++----- ssl/s3_enc.c | 5 +++- ssl/ssl_ciph.c | 13 +++++++--- ssl/ssl_init.c | 3 ++- ssl/ssl_lib.c | 6 +++-- ssl/ssl_locl.h | 2 +- ssl/statem/statem_dtls.c | 44 ++++++++++++++++++--------------- ssl/statem/statem_lib.c | 19 +++++++++----- 11 files changed, 114 insertions(+), 57 deletions(-) diff --git a/ssl/record/rec_layer_d1.c b/ssl/record/rec_layer_d1.c index 487b096b24..879a9b039c 100644 --- a/ssl/record/rec_layer_d1.c +++ b/ssl/record/rec_layer_d1.c @@ -734,7 +734,8 @@ int dtls1_write_bytes(SSL *s, int type, const void *buf, size_t len, { int i; - OPENSSL_assert(len <= SSL3_RT_MAX_PLAIN_LENGTH); + if (!ossl_assert(len <= SSL3_RT_MAX_PLAIN_LENGTH)) + return -1; s->rwstate = SSL_NOTHING; i = do_dtls1_write(s, type, buf, len, 0, written); return i; @@ -757,9 +758,9 @@ int do_dtls1_write(SSL *s, int type, const unsigned char *buf, * first check if there is a SSL3_BUFFER still being written out. This * will happen with non blocking IO */ - if (SSL3_BUFFER_get_left(wb) != 0) { - OPENSSL_assert(0); /* XDTLS: want to see if we ever get here */ - return ssl3_write_pending(s, type, buf, len, written); + if (!ossl_assert(SSL3_BUFFER_get_left(wb) == 0)) { + SSLerr(SSL_F_DO_DTLS1_WRITE, ERR_R_INTERNAL_ERROR); + return 0; } /* If we have an alert to send, lets send it */ diff --git a/ssl/record/record_locl.h b/ssl/record/record_locl.h index e249918ef4..2b55bece85 100644 --- a/ssl/record/record_locl.h +++ b/ssl/record/record_locl.h @@ -106,7 +106,7 @@ void SSL3_RECORD_set_seq_num(SSL3_RECORD *r, const unsigned char *seq_num); int ssl3_get_record(SSL *s); __owur int ssl3_do_compress(SSL *ssl, SSL3_RECORD *wr); __owur int ssl3_do_uncompress(SSL *ssl, SSL3_RECORD *rr); -void ssl3_cbc_copy_mac(unsigned char *out, +int ssl3_cbc_copy_mac(unsigned char *out, const SSL3_RECORD *rec, size_t md_size); __owur int ssl3_cbc_remove_padding(SSL3_RECORD *rec, size_t block_size, size_t mac_size); diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c index 8ebb1b92ee..bba0cc0f0b 100644 --- a/ssl/record/ssl3_record.c +++ b/ssl/record/ssl3_record.c @@ -515,7 +515,11 @@ int ssl3_get_record(SSL *s) unsigned char mac_tmp[EVP_MAX_MD_SIZE]; mac_size = EVP_MD_CTX_size(s->read_hash); - OPENSSL_assert(mac_size <= EVP_MAX_MD_SIZE); + if (!ossl_assert(mac_size <= EVP_MAX_MD_SIZE)) { + al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_SSL3_GET_RECORD, ERR_R_INTERNAL_ERROR); + goto f_err; + } for (j = 0; j < num_recs; j++) { thisrr = &rr[j]; @@ -542,7 +546,11 @@ int ssl3_get_record(SSL *s) * contents of the padding bytes. */ mac = mac_tmp; - ssl3_cbc_copy_mac(mac_tmp, thisrr, mac_size); + if (!ssl3_cbc_copy_mac(mac_tmp, thisrr, mac_size)) { + al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_SSL3_GET_RECORD, ERR_R_INTERNAL_ERROR); + goto f_err; + } thisrr->length -= mac_size; } else { /* @@ -859,7 +867,10 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending) if (sending) { if (EVP_MD_CTX_md(s->write_hash)) { int n = EVP_MD_CTX_size(s->write_hash); - OPENSSL_assert(n >= 0); + if (!ossl_assert(n >= 0)) { + SSLerr(SSL_F_TLS1_ENC, ERR_R_INTERNAL_ERROR); + return -1; + } } ds = s->enc_write_ctx; if (s->enc_write_ctx == NULL) @@ -892,7 +903,10 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int sending) } else { if (EVP_MD_CTX_md(s->read_hash)) { int n = EVP_MD_CTX_size(s->read_hash); - OPENSSL_assert(n >= 0); + if (!ossl_assert(n >= 0)) { + SSLerr(SSL_F_TLS1_ENC, ERR_R_INTERNAL_ERROR); + return -1; + } } ds = s->enc_read_ctx; if (s->enc_read_ctx == NULL) @@ -1179,7 +1193,8 @@ int tls1_mac(SSL *ssl, SSL3_RECORD *rec, unsigned char *md, int sending) } t = EVP_MD_CTX_size(hash); - OPENSSL_assert(t >= 0); + if (!ossl_assert(t >= 0)) + return 0; md_size = t; /* I should fix this up TLS TLS TLS TLS TLS XXXXXXXX */ @@ -1404,7 +1419,7 @@ int tls1_cbc_remove_padding(const SSL *s, */ #define CBC_MAC_ROTATE_IN_PLACE -void ssl3_cbc_copy_mac(unsigned char *out, +int ssl3_cbc_copy_mac(unsigned char *out, const SSL3_RECORD *rec, size_t md_size) { #if defined(CBC_MAC_ROTATE_IN_PLACE) @@ -1428,8 +1443,9 @@ void ssl3_cbc_copy_mac(unsigned char *out, size_t i, j; size_t rotate_offset; - OPENSSL_assert(rec->orig_len >= md_size); - OPENSSL_assert(md_size <= EVP_MAX_MD_SIZE); + if (!ossl_assert(rec->orig_len >= md_size + && md_size <= EVP_MAX_MD_SIZE)) + return 0; #if defined(CBC_MAC_ROTATE_IN_PLACE) rotated_mac = rotated_mac_buf + ((0 - (size_t)rotated_mac_buf) & 63); @@ -1474,6 +1490,8 @@ void ssl3_cbc_copy_mac(unsigned char *out, rotate_offset &= constant_time_lt_s(rotate_offset, md_size); } #endif + + return 1; } int dtls1_process_record(SSL *s, DTLS1_BITMAP *bitmap) @@ -1521,7 +1539,11 @@ int dtls1_process_record(SSL *s, DTLS1_BITMAP *bitmap) if (SSL_READ_ETM(s) && s->read_hash) { unsigned char *mac; mac_size = EVP_MD_CTX_size(s->read_hash); - OPENSSL_assert(mac_size <= EVP_MAX_MD_SIZE); + if (!ossl_assert(mac_size <= EVP_MAX_MD_SIZE)) { + al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_DTLS1_PROCESS_RECORD, ERR_R_INTERNAL_ERROR); + goto f_err; + } if (rr->orig_len < mac_size) { al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_DTLS1_PROCESS_RECORD, SSL_R_LENGTH_TOO_SHORT); @@ -1576,7 +1598,11 @@ int dtls1_process_record(SSL *s, DTLS1_BITMAP *bitmap) goto f_err; } mac_size = (size_t)imac_size; - OPENSSL_assert(mac_size <= EVP_MAX_MD_SIZE); + if (!ossl_assert(mac_size <= EVP_MAX_MD_SIZE)) { + al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_DTLS1_PROCESS_RECORD, ERR_R_INTERNAL_ERROR); + goto f_err; + } /* * orig_len is the length of the record before any padding was @@ -1601,7 +1627,11 @@ int dtls1_process_record(SSL *s, DTLS1_BITMAP *bitmap) * contents of the padding bytes. */ mac = mac_tmp; - ssl3_cbc_copy_mac(mac_tmp, rr, mac_size); + if (!ssl3_cbc_copy_mac(mac_tmp, rr, mac_size)) { + al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_DTLS1_PROCESS_RECORD, ERR_R_INTERNAL_ERROR); + goto f_err; + } rr->length -= mac_size; } else { /* diff --git a/ssl/s3_cbc.c b/ssl/s3_cbc.c index 186ab174ba..f8d7aed3e1 100644 --- a/ssl/s3_cbc.c +++ b/ssl/s3_cbc.c @@ -7,6 +7,7 @@ * https://www.openssl.org/source/license.html */ +#include #include "internal/constant_time_locl.h" #include "ssl_locl.h" @@ -165,7 +166,8 @@ int ssl3_cbc_digest_record(const EVP_MD_CTX *ctx, * This is a, hopefully redundant, check that allows us to forget about * many possible overflows later in this function. */ - OPENSSL_assert(data_plus_mac_plus_padding_size < 1024 * 1024); + if (!ossl_assert(data_plus_mac_plus_padding_size < 1024 * 1024)) + return 0; switch (EVP_MD_CTX_type(ctx)) { case NID_md5: @@ -227,15 +229,16 @@ int ssl3_cbc_digest_record(const EVP_MD_CTX *ctx, * ssl3_cbc_record_digest_supported should have been called first to * check that the hash function is supported. */ - OPENSSL_assert(0); + assert(0); if (md_out_size) *md_out_size = 0; return 0; } - OPENSSL_assert(md_length_size <= MAX_HASH_BIT_COUNT_BYTES); - OPENSSL_assert(md_block_size <= MAX_HASH_BLOCK_SIZE); - OPENSSL_assert(md_size <= EVP_MAX_MD_SIZE); + if (!ossl_assert(md_length_size <= MAX_HASH_BIT_COUNT_BYTES + && md_block_size <= MAX_HASH_BLOCK_SIZE + && md_size <= EVP_MAX_MD_SIZE)) + return 0; header_length = 13; if (is_sslv3) { @@ -331,7 +334,8 @@ int ssl3_cbc_digest_record(const EVP_MD_CTX *ctx, */ bits += 8 * md_block_size; memset(hmac_pad, 0, md_block_size); - OPENSSL_assert(mac_secret_length <= sizeof(hmac_pad)); + if (!ossl_assert(mac_secret_length <= sizeof(hmac_pad))) + return 0; memcpy(hmac_pad, mac_secret, mac_secret_length); for (i = 0; i < md_block_size; i++) hmac_pad[i] ^= 0x36; diff --git a/ssl/s3_enc.c b/ssl/s3_enc.c index 0d75567fc6..1cd28ee2b0 100644 --- a/ssl/s3_enc.c +++ b/ssl/s3_enc.c @@ -120,7 +120,10 @@ int ssl3_change_cipher_state(SSL *s, int which) c = s->s3->tmp.new_sym_enc; m = s->s3->tmp.new_hash; /* m == NULL will lead to a crash later */ - OPENSSL_assert(m); + if (!ossl_assert(m != NULL)) { + SSLerr(SSL_F_SSL3_CHANGE_CIPHER_STATE, ERR_R_INTERNAL_ERROR); + goto err2; + } #ifndef OPENSSL_NO_COMP if (s->s3->tmp.new_compression == NULL) comp = NULL; diff --git a/ssl/ssl_ciph.c b/ssl/ssl_ciph.c index be33ff3281..f05e86f0c5 100644 --- a/ssl/ssl_ciph.c +++ b/ssl/ssl_ciph.c @@ -371,7 +371,7 @@ static uint32_t disabled_mac_mask; static uint32_t disabled_mkey_mask; static uint32_t disabled_auth_mask; -void ssl_load_ciphers(void) +int ssl_load_ciphers(void) { size_t i; const ssl_cipher_table *t; @@ -396,13 +396,16 @@ void ssl_load_ciphers(void) disabled_mac_mask |= t->mask; } else { int tmpsize = EVP_MD_size(md); - OPENSSL_assert(tmpsize >= 0); + if (!ossl_assert(tmpsize >= 0)) + return 0; ssl_mac_secret_size[i] = tmpsize; } } /* Make sure we can access MD5 and SHA1 */ - OPENSSL_assert(ssl_digest_methods[SSL_MD_MD5_IDX] != NULL); - OPENSSL_assert(ssl_digest_methods[SSL_MD_SHA1_IDX] != NULL); + if (!ossl_assert(ssl_digest_methods[SSL_MD_MD5_IDX] != NULL)) + return 0; + if (!ossl_assert(ssl_digest_methods[SSL_MD_SHA1_IDX] != NULL)) + return 0; disabled_mkey_mask = 0; disabled_auth_mask = 0; @@ -460,6 +463,8 @@ void ssl_load_ciphers(void) if ((disabled_auth_mask & (SSL_aGOST01 | SSL_aGOST12)) == (SSL_aGOST01 | SSL_aGOST12)) disabled_mkey_mask |= SSL_kGOST; + + return 1; } #ifndef OPENSSL_NO_COMP diff --git a/ssl/ssl_init.c b/ssl/ssl_init.c index b286a98dec..1d9b799658 100644 --- a/ssl/ssl_init.c +++ b/ssl/ssl_init.c @@ -96,7 +96,8 @@ DEFINE_RUN_ONCE_STATIC(ossl_init_ssl_base) SSL_COMP_get_compression_methods(); #endif /* initialize cipher/digest methods table */ - ssl_load_ciphers(); + if (!ssl_load_ciphers()) + return 0; #ifdef OPENSSL_INIT_DEBUG fprintf(stderr, "OPENSSL_INIT: ossl_init_ssl_base: " diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index de63f84fc4..b318055eab 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -575,7 +575,8 @@ SSL *SSL_new(SSL_CTX *ctx) s->record_padding_arg = ctx->record_padding_arg; s->block_padding = ctx->block_padding; s->sid_ctx_length = ctx->sid_ctx_length; - OPENSSL_assert(s->sid_ctx_length <= sizeof s->sid_ctx); + if (!ossl_assert(s->sid_ctx_length <= sizeof s->sid_ctx)) + goto err; memcpy(&s->sid_ctx, &ctx->sid_ctx, sizeof(s->sid_ctx)); s->verify_callback = ctx->default_verify_callback; s->generate_session_id = ctx->generate_session_id; @@ -3609,7 +3610,8 @@ SSL_CTX *SSL_set_SSL_CTX(SSL *ssl, SSL_CTX *ctx) * Program invariant: |sid_ctx| has fixed size (SSL_MAX_SID_CTX_LENGTH), * so setter APIs must prevent invalid lengths from entering the system. */ - OPENSSL_assert(ssl->sid_ctx_length <= sizeof(ssl->sid_ctx)); + if (!ossl_assert(ssl->sid_ctx_length <= sizeof(ssl->sid_ctx))) + return NULL; /* * If the session ID context matches that of the parent SSL_CTX, diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index f113854925..b0932b0bc6 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -2136,7 +2136,7 @@ void ssl_set_masks(SSL *s); __owur STACK_OF(SSL_CIPHER) *ssl_get_ciphers_by_id(SSL *s); __owur int ssl_verify_alarm_type(long type); void ssl_sort_cipher_list(void); -void ssl_load_ciphers(void); +int ssl_load_ciphers(void); __owur int ssl_fill_hello_random(SSL *s, int server, unsigned char *field, size_t len, DOWNGRADE dgrd); __owur int ssl_generate_master_secret(SSL *s, unsigned char *pms, size_t pmslen, diff --git a/ssl/statem/statem_dtls.c b/ssl/statem/statem_dtls.c index 5cab35544f..83bd8d3322 100644 --- a/ssl/statem/statem_dtls.c +++ b/ssl/statem/statem_dtls.c @@ -32,7 +32,6 @@ #define RSMBLY_BITMASK_IS_COMPLETE(bitmask, msg_len, is_complete) { \ long ii; \ - OPENSSL_assert((msg_len) > 0); \ is_complete = 1; \ if (bitmask[(((msg_len) - 1) >> 3)] != bitmask_end_values[((msg_len) & 7)]) is_complete = 0; \ if (is_complete) for (ii = (((msg_len) - 1) >> 3) - 1; ii >= 0 ; ii--) \ @@ -122,9 +121,11 @@ int dtls1_do_write(SSL *s, int type) /* should have something reasonable now */ return -1; - if (s->init_off == 0 && type == SSL3_RT_HANDSHAKE) - OPENSSL_assert(s->init_num == - s->d1->w_msg_hdr.msg_len + DTLS1_HM_HEADER_LENGTH); + if (s->init_off == 0 && type == SSL3_RT_HANDSHAKE) { + if (!ossl_assert(s->init_num == + s->d1->w_msg_hdr.msg_len + DTLS1_HM_HEADER_LENGTH)) + return -1; + } if (s->write_hash) { if (s->enc_write_ctx @@ -254,7 +255,7 @@ int dtls1_do_write(SSL *s, int type) } else return -1; } else { - return (-1); + return -1; } } else { @@ -262,7 +263,8 @@ int dtls1_do_write(SSL *s, int type) * bad if this assert fails, only part of the handshake message * got sent. but why would this happen? */ - OPENSSL_assert(len == written); + if (!ossl_assert(len == written)) + return -1; if (type == SSL3_RT_HANDSHAKE && !s->d1->retransmitting) { /* @@ -578,6 +580,8 @@ dtls1_reassemble_fragment(SSL *s, const struct hm_header_st *msg_hdr) RSMBLY_BITMASK_MARK(frag->reassembly, (long)msg_hdr->frag_off, (long)(msg_hdr->frag_off + frag_len)); + if (!ossl_assert(msg_hdr->msg_len > 0)) + goto err; RSMBLY_BITMASK_IS_COMPLETE(frag->reassembly, (long)msg_hdr->msg_len, is_complete); @@ -600,7 +604,8 @@ dtls1_reassemble_fragment(SSL *s, const struct hm_header_st *msg_hdr) * would have returned it and control would never have reached this * branch. */ - OPENSSL_assert(item != NULL); + if (!ossl_assert(item != NULL)) + goto err; } return DTLS1_HM_FRAGMENT_RETRY; @@ -697,7 +702,8 @@ dtls1_process_out_of_seq_message(SSL *s, const struct hm_header_st *msg_hdr) * have been processed with |dtls1_reassemble_fragment|, above, or * the record will have been discarded. */ - OPENSSL_assert(item != NULL); + if (!ossl_assert(item != NULL)) + goto err; } return DTLS1_HM_FRAGMENT_RETRY; @@ -981,7 +987,8 @@ int dtls1_buffer_message(SSL *s, int is_ccs) * this function is called immediately after a message has been * serialized */ - OPENSSL_assert(s->init_off == 0); + if (!ossl_assert(s->init_off == 0)) + return 0; frag = dtls1_hm_fragment_new(s->init_num, 0); if (frag == NULL) @@ -991,13 +998,15 @@ int dtls1_buffer_message(SSL *s, int is_ccs) if (is_ccs) { /* For DTLS1_BAD_VER the header length is non-standard */ - OPENSSL_assert(s->d1->w_msg_hdr.msg_len + - ((s->version == - DTLS1_BAD_VER) ? 3 : DTLS1_CCS_HEADER_LENGTH) - == (unsigned int)s->init_num); + if (!ossl_assert(s->d1->w_msg_hdr.msg_len + + ((s->version == + DTLS1_BAD_VER) ? 3 : DTLS1_CCS_HEADER_LENGTH) + == (unsigned int)s->init_num)) + return 0; } else { - OPENSSL_assert(s->d1->w_msg_hdr.msg_len + - DTLS1_HM_HEADER_LENGTH == (unsigned int)s->init_num); + if (!ossl_assert(s->d1->w_msg_hdr.msg_len + + DTLS1_HM_HEADER_LENGTH == (unsigned int)s->init_num)) + return 0; } frag->msg_header.msg_len = s->d1->w_msg_hdr.msg_len; @@ -1045,11 +1054,6 @@ int dtls1_retransmit_message(SSL *s, unsigned short seq, int *found) unsigned char seq64be[8]; struct dtls1_retransmit_state saved_state; - /*- - OPENSSL_assert(s->init_num == 0); - OPENSSL_assert(s->init_off == 0); - */ - /* XDTLS: the requested message ought to be found, otherwise error */ memset(seq64be, 0, sizeof(seq64be)); seq64be[6] = (unsigned char)(seq >> 8); diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index e6f62937cb..7aedc41691 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -522,19 +522,23 @@ int tls_construct_finished(SSL *s, WPACKET *pkt) */ if (!SSL_IS_TLS13(s) && !ssl_log_secret(s, MASTER_SECRET_LABEL, s->session->master_key, - s->session->master_key_length)) - return 0; + s->session->master_key_length)) { + SSLerr(SSL_F_TLS_CONSTRUCT_FINISHED, ERR_R_INTERNAL_ERROR); + goto err; + } /* * Copy the finished so we can use it for renegotiation checks */ + if (!ossl_assert(finish_md_len <= EVP_MAX_MD_SIZE)) { + SSLerr(SSL_F_TLS_CONSTRUCT_FINISHED, ERR_R_INTERNAL_ERROR); + goto err; + } if (!s->server) { - OPENSSL_assert(finish_md_len <= EVP_MAX_MD_SIZE); memcpy(s->s3->previous_client_finished, s->s3->tmp.finish_md, finish_md_len); s->s3->previous_client_finished_len = finish_md_len; } else { - OPENSSL_assert(finish_md_len <= EVP_MAX_MD_SIZE); memcpy(s->s3->previous_server_finished, s->s3->tmp.finish_md, finish_md_len); s->s3->previous_server_finished_len = finish_md_len; @@ -765,13 +769,16 @@ MSG_PROCESS_RETURN tls_process_finished(SSL *s, PACKET *pkt) /* * Copy the finished so we can use it for renegotiation checks */ + if (!ossl_assert(md_len <= EVP_MAX_MD_SIZE)) { + al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_TLS_PROCESS_FINISHED, ERR_R_INTERNAL_ERROR); + goto f_err; + } if (s->server) { - OPENSSL_assert(md_len <= EVP_MAX_MD_SIZE); memcpy(s->s3->previous_client_finished, s->s3->tmp.peer_finish_md, md_len); s->s3->previous_client_finished_len = md_len; } else { - OPENSSL_assert(md_len <= EVP_MAX_MD_SIZE); memcpy(s->s3->previous_server_finished, s->s3->tmp.peer_finish_md, md_len); s->s3->previous_server_finished_len = md_len;