From: Matt Caswell Date: Wed, 31 Aug 2022 16:37:48 +0000 (+0100) Subject: Convert the write record layer to supply proper return values X-Git-Tag: openssl-3.2.0-alpha1~2026 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=320145d5b3a11492427fe1cab9ca4de52402c72d;p=thirdparty%2Fopenssl.git Convert the write record layer to supply proper return values This also means we can convert SSLfatal calls to RLAYERfatal Reviewed-by: Hugo Landau Reviewed-by: Richard Levitte (Merged from https://github.com/openssl/openssl/pull/19198) --- diff --git a/ssl/record/methods/tls_common.c b/ssl/record/methods/tls_common.c index 809a9f8fb4..5fb5f4ebbc 100644 --- a/ssl/record/methods/tls_common.c +++ b/ssl/record/methods/tls_common.c @@ -107,7 +107,7 @@ static int tls_setup_write_buffer(OSSL_RECORD_LAYER *rl, size_t numwpipes, rl->numwpipes = numwpipes; if (len == 0) { - if (SSL_CONNECTION_IS_DTLS(s)) + if (rl->isdtls) headerlen = DTLS1_RT_HEADER_LENGTH + 1; else headerlen = SSL3_RT_HEADER_LENGTH; @@ -145,7 +145,7 @@ static int tls_setup_write_buffer(OSSL_RECORD_LAYER *rl, size_t numwpipes, * buffers. We assume we're so doomed that we won't even be able * to send an alert. */ - SSLfatal(s, SSL_AD_NO_ALERT, ERR_R_MALLOC_FAILURE); + RLAYERfatal(rl, SSL_AD_NO_ALERT, ERR_R_MALLOC_FAILURE); return 0; } } else { @@ -1438,7 +1438,7 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, /* Check we don't have pending data waiting to write */ if (!ossl_assert(rl->nextwbuf >= rl->numwpipes || SSL3_BUFFER_get_left(&rl->wbuf[rl->nextwbuf]) == 0)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); goto err; } @@ -1452,7 +1452,7 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, } else { mac_size = EVP_MD_CTX_get_size(s->write_hash); if (mac_size < 0) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); goto err; } } @@ -1472,14 +1472,14 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, * smaller. It is wasteful to allocate a full sized buffer here */ if (!tls_setup_write_buffer(rl, numtempl + prefix, 0)) { - /* SSLfatal() already called */ - return -1; + /* RLAYERfatal() already called */ + goto err; } } using_ktls = BIO_get_ktls_send(rl->bio); if (!ossl_assert(!using_ktls || !prefix)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); goto err; } @@ -1509,7 +1509,7 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, if (!WPACKET_init_static_len(&pkt[0], SSL3_BUFFER_get_buf(wb), SSL3_BUFFER_get_len(wb), 0) || !WPACKET_allocate_bytes(&pkt[0], align, NULL)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); goto err; } wpinited = 1; @@ -1541,7 +1541,7 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, if (!WPACKET_init_static_len(thispkt, SSL3_BUFFER_get_buf(wb), SSL3_BUFFER_get_len(wb), 0) || !WPACKET_allocate_bytes(thispkt, align, NULL)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); goto err; } wpinited++; @@ -1556,7 +1556,7 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, if (mode == EVP_CIPH_CBC_MODE) { eivlen = EVP_CIPHER_CTX_get_iv_length(s->enc_write_ctx); if (eivlen < 0) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_R_LIBRARY_BUG); + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, SSL_R_LIBRARY_BUG); goto err; } if (eivlen <= 1) @@ -1615,7 +1615,7 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, || (maxcomplen > 0 && !WPACKET_reserve_bytes(thispkt, maxcomplen, &compressdata)))) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); goto err; } @@ -1638,7 +1638,7 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, if (s->compress != NULL) { if (!ssl3_do_compress(s, thiswr) || !WPACKET_allocate_bytes(thispkt, thiswr->length, NULL)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_R_COMPRESSION_FAILURE); + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, SSL_R_COMPRESSION_FAILURE); goto err; } } else { @@ -1646,7 +1646,7 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, SSL3_RECORD_reset_data(&wr[j]); } else { if (!WPACKET_memcpy(thispkt, thiswr->input, thiswr->length)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); goto err; } SSL3_RECORD_reset_input(&wr[j]); @@ -1661,7 +1661,7 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, size_t rlen, max_send_fragment; if (!WPACKET_put_bytes_u8(thispkt, thistempl->type)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); goto err; } SSL3_RECORD_add_length(thiswr, 1); @@ -1695,8 +1695,8 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, if (padding > max_padding) padding = max_padding; if (!WPACKET_memset(thispkt, 0, padding)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, - ERR_R_INTERNAL_ERROR); + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, + ERR_R_INTERNAL_ERROR); goto err; } SSL3_RECORD_add_length(thiswr, padding); @@ -1715,7 +1715,7 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, if (!WPACKET_allocate_bytes(thispkt, mac_size, &mac) || !ssl->method->ssl3_enc->mac(s, thiswr, mac, 1)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); goto err; } } @@ -1729,13 +1729,13 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, if (!WPACKET_reserve_bytes(thispkt, SSL_RT_MAX_CIPHER_BLOCK_SIZE, NULL) - /* - * We also need next the amount of bytes written to this - * sub-packet - */ - || !WPACKET_get_length(thispkt, &len)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); - goto err; + /* + * We also need next the amount of bytes written to this + * sub-packet + */ + || !WPACKET_get_length(thispkt, &len)) { + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + goto err; } /* Get a pointer to the start of this record excluding header */ @@ -1752,26 +1752,23 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, * send early data - so we need to use the tls13enc function. */ if (tls13_enc(s, wr, numtempl, 1, NULL, mac_size) < 1) { - if (!ossl_statem_in_error(s)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); - } + if (!ossl_statem_in_error(s)) + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); goto err; } } else { if (!using_ktls) { if (prefix) { if (ssl->method->ssl3_enc->enc(s, wr, 1, 1, NULL, mac_size) < 1) { - if (!ossl_statem_in_error(s)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); - } + if (!ossl_statem_in_error(s)) + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); goto err; } } if (ssl->method->ssl3_enc->enc(s, wr + prefix, numtempl, 1, NULL, mac_size) < 1) { - if (!ossl_statem_in_error(s)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); - } + if (!ossl_statem_in_error(s)) + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); goto err; } } @@ -1796,7 +1793,7 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, && !WPACKET_allocate_bytes(thispkt, thiswr->length - origlen, NULL))) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); goto err; } if (rl->use_etm && mac_size != 0) { @@ -1804,7 +1801,7 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, if (!WPACKET_allocate_bytes(thispkt, mac_size, &mac) || !ssl->method->ssl3_enc->mac(s, thiswr, mac, 1)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); goto err; } SSL3_RECORD_add_length(thiswr, mac_size); @@ -1812,7 +1809,7 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, if (!WPACKET_get_length(thispkt, &len) || !WPACKET_close(thispkt)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); goto err; } @@ -1831,7 +1828,7 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, } if (!WPACKET_finish(thispkt)) { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); goto err; } @@ -1856,30 +1853,23 @@ int tls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, err: for (j = 0; j < wpinited; j++) WPACKET_cleanup(&pkt[j]); - return -1; + return OSSL_RECORD_RETURN_FATAL; } -/* if SSL3_BUFFER_get_left() != 0, we need to call this - * - * Return values are as per SSL_write() - */ int tls_retry_write_records(OSSL_RECORD_LAYER *rl) { - int i; + int i, ret; SSL3_BUFFER *thiswb; size_t tmpwrit = 0; - SSL_CONNECTION *s = rl->cbarg; if (rl->nextwbuf >= rl->numwpipes) - return 1; + return OSSL_RECORD_RETURN_SUCCESS; for (;;) { thiswb = &rl->wbuf[rl->nextwbuf]; clear_sys_error(); if (rl->bio != NULL) { - s->rwstate = SSL_WRITING; - /* * To prevent coalescing of control and data messages, * such as in buffer_write, we flush the BIO @@ -1887,18 +1877,34 @@ int tls_retry_write_records(OSSL_RECORD_LAYER *rl) if (BIO_get_ktls_send(rl->bio) && thiswb->type != SSL3_RT_APPLICATION_DATA) { i = BIO_flush(rl->bio); - if (i <= 0) - return i; + if (i <= 0) { + if (BIO_should_retry(rl->bio)) + ret = OSSL_RECORD_RETURN_RETRY; + else + ret = OSSL_RECORD_RETURN_FATAL; + return ret; + } BIO_set_ktls_ctrl_msg(rl->bio, thiswb->type); } i = BIO_write(rl->bio, (char *) &(SSL3_BUFFER_get_buf(thiswb) [SSL3_BUFFER_get_offset(thiswb)]), (unsigned int)SSL3_BUFFER_get_left(thiswb)); - if (i >= 0) + if (i >= 0) { tmpwrit = i; + if (i == 0 && BIO_should_retry(rl->bio)) + ret = OSSL_RECORD_RETURN_RETRY; + else + ret = OSSL_RECORD_RETURN_SUCCESS; + } else { + if (BIO_should_retry(rl->bio)) + ret = OSSL_RECORD_RETURN_RETRY; + else + ret = OSSL_RECORD_RETURN_FATAL; + } } else { - SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_R_BIO_NOT_SET); + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, SSL_R_BIO_NOT_SET); + ret = OSSL_RECORD_RETURN_FATAL; i = -1; } @@ -1914,12 +1920,11 @@ int tls_retry_write_records(OSSL_RECORD_LAYER *rl) SSL3_BUFFER_add_offset(thiswb, tmpwrit); if (++(rl->nextwbuf) < rl->numwpipes) continue; - s->rwstate = SSL_NOTHING; if (rl->nextwbuf == rl->numwpipes && (rl->mode & SSL_MODE_RELEASE_BUFFERS) != 0) tls_release_write_buffer(rl); - return 1; + return OSSL_RECORD_RETURN_SUCCESS; } else if (i <= 0) { if (rl->isdtls) { /* @@ -1932,7 +1937,7 @@ int tls_retry_write_records(OSSL_RECORD_LAYER *rl) tls_release_write_buffer(rl); } - return i; + return ret; } SSL3_BUFFER_add_offset(thiswb, tmpwrit); SSL3_BUFFER_sub_left(thiswb, tmpwrit); diff --git a/ssl/record/rec_layer_d1.c b/ssl/record/rec_layer_d1.c index cad2ddda2d..7baeb42849 100644 --- a/ssl/record/rec_layer_d1.c +++ b/ssl/record/rec_layer_d1.c @@ -270,7 +270,7 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, do { rr = &sc->rlayer.tlsrecs[sc->rlayer.num_recs]; - ret = HANDLE_RLAYER_RETURN(sc, + ret = HANDLE_RLAYER_READ_RETURN(sc, sc->rlayer.rrlmethod->read_record(sc->rlayer.rrl, &rr->rechandle, &rr->version, &rr->type, diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index 9152ea86aa..0c70995312 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -266,7 +266,8 @@ int ssl3_write_bytes(SSL *ssl, int type, const void *buf_, size_t len, return i; } else if (i > 0) { /* Retry needed */ - i = s->rlayer.wrlmethod->retry_write_records(s->rlayer.wrl); + i = HANDLE_RLAYER_WRITE_RETURN(s, + s->rlayer.wrlmethod->retry_write_records(s->rlayer.wrl)); if (i <= 0) return i; tot += s->rlayer.wpend_tot; @@ -539,7 +540,8 @@ int ssl3_write_bytes(SSL *ssl, int type, const void *buf_, size_t len, s->rlayer.wpend_tot = n; } - i = s->rlayer.wrlmethod->write_records(s->rlayer.wrl, tmpls, numpipes); + i = HANDLE_RLAYER_WRITE_RETURN(s, + s->rlayer.wrlmethod->write_records(s->rlayer.wrl, tmpls, numpipes)); if (i <= 0) { /* SSLfatal() already called if appropriate */ s->rlayer.wnum = tot; @@ -559,18 +561,28 @@ int ssl3_write_bytes(SSL *ssl, int type, const void *buf_, size_t len, } } -int ossl_tls_handle_rlayer_return(SSL_CONNECTION *s, int ret, char *file, - int line) +int ossl_tls_handle_rlayer_return(SSL_CONNECTION *s, int writing, int ret, + char *file, int line) { SSL *ssl = SSL_CONNECTION_GET_SSL(s); if (ret == OSSL_RECORD_RETURN_RETRY) { - s->rwstate = SSL_READING; + s->rwstate = writing ? SSL_WRITING : SSL_READING; ret = -1; } else { s->rwstate = SSL_NOTHING; if (ret == OSSL_RECORD_RETURN_EOF) { - if (s->options & SSL_OP_IGNORE_UNEXPECTED_EOF) { + if (writing) { + /* + * This shouldn't happen with a writing operation. We treat it + * as fatal. + */ + ERR_new(); + ERR_set_debug(file, line, 0); + ossl_statem_fatal(s, SSL_AD_INTERNAL_ERROR, + ERR_R_INTERNAL_ERROR, NULL); + ret = OSSL_RECORD_RETURN_FATAL; + } else if ((s->options & SSL_OP_IGNORE_UNEXPECTED_EOF) != 0) { SSL_set_shutdown(ssl, SSL_RECEIVED_SHUTDOWN); s->s3.warn_alert = SSL_AD_CLOSE_NOTIFY; } else { @@ -725,7 +737,7 @@ int ssl3_read_bytes(SSL *ssl, int type, int *recvd_type, unsigned char *buf, do { rr = &s->rlayer.tlsrecs[s->rlayer.num_recs]; - ret = HANDLE_RLAYER_RETURN(s, + ret = HANDLE_RLAYER_READ_RETURN(s, s->rlayer.rrlmethod->read_record(s->rlayer.rrl, &rr->rechandle, &rr->version, &rr->type, diff --git a/ssl/record/record.h b/ssl/record/record.h index 03151e58d6..76b3314cb9 100644 --- a/ssl/record/record.h +++ b/ssl/record/record.h @@ -252,11 +252,14 @@ int do_dtls1_write(SSL_CONNECTION *s, int type, const unsigned char *buf, void dtls1_reset_seq_numbers(SSL_CONNECTION *s, int rw); void ssl_release_record(SSL_CONNECTION *s, TLS_RECORD *rr); -# define HANDLE_RLAYER_RETURN(s, ret) \ - ossl_tls_handle_rlayer_return(s, ret, OPENSSL_FILE, OPENSSL_LINE) +# define HANDLE_RLAYER_READ_RETURN(s, ret) \ + ossl_tls_handle_rlayer_return(s, 0, ret, OPENSSL_FILE, OPENSSL_LINE) -int ossl_tls_handle_rlayer_return(SSL_CONNECTION *s, int ret, char *file, - int line); +# define HANDLE_RLAYER_WRITE_RETURN(s, ret) \ + ossl_tls_handle_rlayer_return(s, 1, ret, OPENSSL_FILE, OPENSSL_LINE) + +int ossl_tls_handle_rlayer_return(SSL_CONNECTION *s, int writing, int ret, + char *file, int line); int ssl_set_new_record_layer(SSL_CONNECTION *s, int version, int direction, int level, unsigned char *key, size_t keylen, diff --git a/ssl/s3_msg.c b/ssl/s3_msg.c index 64e23f3a9b..c656cd7670 100644 --- a/ssl/s3_msg.c +++ b/ssl/s3_msg.c @@ -108,7 +108,8 @@ int ssl3_dispatch_alert(SSL *s) if (RECORD_LAYER_write_pending(&sc->rlayer)) return -1; - i = sc->rlayer.wrlmethod->write_records(sc->rlayer.wrl, &templ, 1); + i = HANDLE_RLAYER_WRITE_RETURN(sc, + sc->rlayer.wrlmethod->write_records(sc->rlayer.wrl, &templ, 1)); if (i <= 0) { sc->s3.alert_dispatch = 1; } else {