diff options
author | Matt Caswell <matt@openssl.org> | 2023-02-27 09:19:16 +0000 |
---|---|---|
committer | Pauli <pauli@openssl.org> | 2023-04-12 11:02:01 +1000 |
commit | 7a4e109ebe5af83bad6447889e43ac2612375070 (patch) | |
tree | 31dcd6ed8004872d115a0a05c1a20574b8724dbe | |
parent | 2eb91b0ec325924ae4b7dc596617a6fff71d7ae6 (diff) |
Allow partially releasing a record for TLS
This enables the cleansing of plaintext to occur in the record layer and
avoids the need to cast away const above the record layer.
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/20404)
-rw-r--r-- | include/internal/recordmethod.h | 12 | ||||
-rw-r--r-- | ssl/quic/quic_tls.c | 27 | ||||
-rw-r--r-- | ssl/record/methods/recmethod_local.h | 2 | ||||
-rw-r--r-- | ssl/record/methods/tls_common.c | 21 | ||||
-rw-r--r-- | ssl/record/rec_layer_d1.c | 41 | ||||
-rw-r--r-- | ssl/record/rec_layer_s3.c | 71 | ||||
-rw-r--r-- | ssl/record/record.h | 2 |
7 files changed, 114 insertions, 62 deletions
diff --git a/include/internal/recordmethod.h b/include/internal/recordmethod.h index 30d2208568..7327e30482 100644 --- a/include/internal/recordmethod.h +++ b/include/internal/recordmethod.h @@ -225,7 +225,8 @@ struct ossl_record_method_st { * filled in with the epoch and sequence number from the record. * An opaque record layer handle for the record is returned in |*rechandle| * which is used in a subsequent call to |release_record|. The buffer must - * remain available until release_record is called. + * remain available until all the bytes from record are released via one or + * more release_record calls. * * Internally the the OSSL_RECORD_METHOD the implementation may read/process * multiple records in one go and buffer them. @@ -234,11 +235,12 @@ struct ossl_record_method_st { int *type, const unsigned char **data, size_t *datalen, uint16_t *epoch, unsigned char *seq_num); /* - * Release a buffer associated with a record previously read with - * read_record. Records are guaranteed to be released in the order that they - * are read. + * Release length bytes from a buffer associated with a record previously + * read with read_record. Once all the bytes from a record are released, the + * whole record and its associated buffer is released. Records are + * guaranteed to be released in the order that they are read. */ - int (*release_record)(OSSL_RECORD_LAYER *rl, void *rechandle); + int (*release_record)(OSSL_RECORD_LAYER *rl, void *rechandle, size_t length); /* * In the event that a fatal error is returned from the functions above then diff --git a/ssl/quic/quic_tls.c b/ssl/quic/quic_tls.c index 62f8425d09..2dedc760cb 100644 --- a/ssl/quic/quic_tls.c +++ b/ssl/quic/quic_tls.c @@ -58,9 +58,12 @@ struct ossl_record_layer_st { */ int alert; - /* Amount of crypto stream data we have received but not yet released */ + /* Amount of crypto stream data we read in the last call to quic_read_record */ size_t recread; + /* Amount of crypto stream data read but not yet released */ + size_t recunreleased; + /* Callbacks */ OSSL_FUNC_rlayer_msg_callback_fn *msg_callback; void *cbarg; @@ -336,7 +339,7 @@ static int quic_read_record(OSSL_RECORD_LAYER *rl, void **rechandle, size_t *datalen, uint16_t *epoch, unsigned char *seq_num) { - if (rl->recread != 0) + if (rl->recread != 0 || rl->recunreleased != 0) return OSSL_RECORD_RETURN_FATAL; BIO_clear_retry_flags(rl->dummybio); @@ -355,7 +358,7 @@ static int quic_read_record(OSSL_RECORD_LAYER *rl, void **rechandle, *rechandle = rl; *rversion = TLS1_3_VERSION; *type = SSL3_RT_HANDSHAKE; - rl->recread = *datalen; + rl->recread = rl->recunreleased = *datalen; /* epoch/seq_num are not relevant for TLS */ if (rl->msg_callback != NULL) { @@ -386,22 +389,30 @@ static int quic_read_record(OSSL_RECORD_LAYER *rl, void **rechandle, return OSSL_RECORD_RETURN_SUCCESS; } -static int quic_release_record(OSSL_RECORD_LAYER *rl, void *rechandle) +static int quic_release_record(OSSL_RECORD_LAYER *rl, void *rechandle, + size_t length) { - if (!ossl_assert(rl->recread > 0) || !ossl_assert(rl == rechandle)) { + if (!ossl_assert(rl->recread > 0) + || !ossl_assert(rl->recunreleased <= rl->recread) + || !ossl_assert(rl == rechandle) + || !ossl_assert(length <= rl->recunreleased)) { QUIC_TLS_FATAL(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); - return 0; + return OSSL_RECORD_RETURN_FATAL; } + rl->recunreleased -= length; + + if (rl->recunreleased > 0) + return OSSL_RECORD_RETURN_SUCCESS; + if (!rl->qtls->args.crypto_release_rcd_cb(rl->recread, rl->qtls->args.crypto_release_rcd_cb_arg)) { QUIC_TLS_FATAL(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); return OSSL_RECORD_RETURN_FATAL; } - rl->recread = 0; - return 1; + return OSSL_RECORD_RETURN_SUCCESS; } static int quic_get_alert_code(OSSL_RECORD_LAYER *rl) diff --git a/ssl/record/methods/recmethod_local.h b/ssl/record/methods/recmethod_local.h index e6310908d8..9454ca56b8 100644 --- a/ssl/record/methods/recmethod_local.h +++ b/ssl/record/methods/recmethod_local.h @@ -461,7 +461,7 @@ int tls_set1_bio(OSSL_RECORD_LAYER *rl, BIO *bio); int tls_read_record(OSSL_RECORD_LAYER *rl, void **rechandle, int *rversion, int *type, const unsigned char **data, size_t *datalen, uint16_t *epoch, unsigned char *seq_num); -int tls_release_record(OSSL_RECORD_LAYER *rl, void *rechandle); +int tls_release_record(OSSL_RECORD_LAYER *rl, void *rechandle, size_t length); int tls_default_set_protocol_version(OSSL_RECORD_LAYER *rl, int version); int tls_set_protocol_version(OSSL_RECORD_LAYER *rl, int version); void tls_set_plain_alerts(OSSL_RECORD_LAYER *rl, int allow); diff --git a/ssl/record/methods/tls_common.c b/ssl/record/methods/tls_common.c index 32865fdbf1..a93bd91daf 100644 --- a/ssl/record/methods/tls_common.c +++ b/ssl/record/methods/tls_common.c @@ -1132,15 +1132,32 @@ int tls_read_record(OSSL_RECORD_LAYER *rl, void **rechandle, int *rversion, return OSSL_RECORD_RETURN_SUCCESS; } -int tls_release_record(OSSL_RECORD_LAYER *rl, void *rechandle) +int tls_release_record(OSSL_RECORD_LAYER *rl, void *rechandle, size_t length) { + TLS_RL_RECORD *rec = &rl->rrec[rl->num_released]; + if (!ossl_assert(rl->num_released < rl->curr_rec) - || !ossl_assert(rechandle == &rl->rrec[rl->num_released])) { + || !ossl_assert(rechandle == rec)) { /* Should not happen */ RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, SSL_R_INVALID_RECORD); return OSSL_RECORD_RETURN_FATAL; } + if (rec->length < length) { + /* Should not happen */ + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + return OSSL_RECORD_RETURN_FATAL; + } + + if ((rl->options & SSL_OP_CLEANSE_PLAINTEXT) != 0) + OPENSSL_cleanse(rec->data + rec->off, length); + + rec->off += length; + rec->length -= length; + + if (rec->length > 0) + return OSSL_RECORD_RETURN_SUCCESS; + rl->num_released++; if (rl->curr_rec == rl->num_released diff --git a/ssl/record/rec_layer_d1.c b/ssl/record/rec_layer_d1.c index 6a2d128807..fed57b65cd 100644 --- a/ssl/record/rec_layer_d1.c +++ b/ssl/record/rec_layer_d1.c @@ -293,7 +293,8 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, /* SSLfatal() already called */ return -1; } - ssl_release_record(sc, rr); + if (!ssl_release_record(sc, rr, 0)) + return -1; goto start; } @@ -302,7 +303,8 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, * 'peek' mode) */ if (sc->shutdown & SSL_RECEIVED_SHUTDOWN) { - ssl_release_record(sc, rr); + if (!ssl_release_record(sc, rr, 0)) + return -1; sc->rwstate = SSL_NOTHING; return 0; } @@ -335,8 +337,8 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, * SSL_read() with a zero length buffer will eventually cause * SSL_pending() to report data as being available. */ - if (rr->length == 0) - ssl_release_record(sc, rr); + if (rr->length == 0 && !ssl_release_record(sc, rr, 0)) + return -1; return 0; } @@ -347,16 +349,11 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, memcpy(buf, &(rr->data[rr->off]), n); if (peek) { - if (rr->length == 0) - ssl_release_record(sc, rr); + if (rr->length == 0 && !ssl_release_record(sc, rr, 0)) + return -1; } else { - /* TODO(RECLAYER): Casting away the const is wrong! FIX ME */ - if (sc->options & SSL_OP_CLEANSE_PLAINTEXT) - OPENSSL_cleanse((unsigned char *)&(rr->data[rr->off]), n); - rr->length -= n; - rr->off += n; - if (rr->length == 0) - ssl_release_record(sc, rr); + if (!ssl_release_record(sc, rr, n)) + return -1; } #ifndef OPENSSL_NO_SCTP /* @@ -409,7 +406,8 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, if (alert_level == SSL3_AL_WARNING) { sc->s3.warn_alert = alert_descr; - ssl_release_record(sc, rr); + if (!ssl_release_record(sc, rr, 0)) + return -1; sc->rlayer.alert_count++; if (sc->rlayer.alert_count == MAX_WARN_ALERT_COUNT) { @@ -444,7 +442,8 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, SSL_AD_REASON_OFFSET + alert_descr, "SSL alert number %d", alert_descr); sc->shutdown |= SSL_RECEIVED_SHUTDOWN; - ssl_release_record(sc, rr); + if (!ssl_release_record(sc, rr, 0)) + return -1; SSL_CTX_remove_session(sc->session_ctx, sc->session); return 0; } else { @@ -458,7 +457,8 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, if (sc->shutdown & SSL_SENT_SHUTDOWN) { /* but we have not received a * shutdown */ sc->rwstate = SSL_NOTHING; - ssl_release_record(sc, rr); + if (!ssl_release_record(sc, rr, 0)) + return -1; return 0; } @@ -467,7 +467,8 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, * We can't process a CCS now, because previous handshake messages * are still missing, so just drop it. */ - ssl_release_record(sc, rr); + if (!ssl_release_record(sc, rr, 0)) + return -1; goto start; } @@ -483,7 +484,8 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, */ if (rr->epoch != sc->rlayer.d->r_epoch || rr->length < DTLS1_HM_HEADER_LENGTH) { - ssl_release_record(sc, rr); + if (!ssl_release_record(sc, rr, 0)) + return -1; goto start; } @@ -504,7 +506,8 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, if (ossl_statem_in_error(sc)) return -1; } - ssl_release_record(sc, rr); + if (!ssl_release_record(sc, rr, 0)) + return -1; if (!(sc->mode & SSL_MODE_AUTO_RETRY)) { if (!sc->rlayer.rrlmethod->unprocessed_read_pending(sc->rlayer.rrl)) { /* no read-ahead left? */ diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index c567b471ea..e59588472f 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -10,6 +10,7 @@ #include <stdio.h> #include <limits.h> #include <errno.h> +#include <assert.h> #include "../ssl_local.h" #include <openssl/evp.h> #include <openssl/buffer.h> @@ -496,17 +497,35 @@ int ossl_tls_handle_rlayer_return(SSL_CONNECTION *s, int writing, int ret, return ret; } -void ssl_release_record(SSL_CONNECTION *s, TLS_RECORD *rr) +int ssl_release_record(SSL_CONNECTION *s, TLS_RECORD *rr, size_t length) { + assert(rr->length >= length); if (rr->rechandle != NULL) { + if (length == 0) + length = rr->length; /* The record layer allocated the buffers for this record */ - s->rlayer.rrlmethod->release_record(s->rlayer.rrl, rr->rechandle); - } else { + if (HANDLE_RLAYER_READ_RETURN(s, + s->rlayer.rrlmethod->release_record(s->rlayer.rrl, + rr->rechandle, + length)) <= 0) { + /* RLAYER_fatal already called */ + return 0; + } + + if (length == rr->length) + s->rlayer.curr_rec++; + } else if (length == 0 || length == rr->length) { /* We allocated the buffers for this record (only happens with DTLS) */ OPENSSL_free(rr->allocdata); rr->allocdata = NULL; } - s->rlayer.curr_rec++; + rr->length -= length; + if (rr->length > 0) + rr->off += length; + else + rr->off = 0; + + return 1; } /*- @@ -700,8 +719,8 @@ int ssl3_read_bytes(SSL *ssl, int type, int *recvd_type, unsigned char *buf, * SSL_read() with a zero length buffer will eventually cause * SSL_pending() to report data as being available. */ - if (rr->length == 0) - ssl_release_record(s, rr); + if (rr->length == 0 && !ssl_release_record(s, rr, 0)) + return -1; return 0; } @@ -718,16 +737,11 @@ int ssl3_read_bytes(SSL *ssl, int type, int *recvd_type, unsigned char *buf, buf += n; if (peek) { /* Mark any zero length record as consumed CVE-2016-6305 */ - if (rr->length == 0) - ssl_release_record(s, rr); + if (rr->length == 0 && !ssl_release_record(s, rr, 0)) + return -1; } else { - /* TODO(RECLAYER) Casting away the const here is wrong! FIX ME */ - if (s->options & SSL_OP_CLEANSE_PLAINTEXT) - OPENSSL_cleanse((unsigned char *)&(rr->data[rr->off]), n); - rr->length -= n; - rr->off += n; - if (rr->length == 0) - ssl_release_record(s, rr); + if (!ssl_release_record(s, rr, n)) + return -1; } if (rr->length == 0 || (peek && n == rr->length)) { @@ -814,7 +828,8 @@ int ssl3_read_bytes(SSL *ssl, int type, int *recvd_type, unsigned char *buf, if ((!is_tls13 && alert_level == SSL3_AL_WARNING) || (is_tls13 && alert_descr == SSL_AD_USER_CANCELLED)) { s->s3.warn_alert = alert_descr; - ssl_release_record(s, rr); + if (!ssl_release_record(s, rr, 0)) + return -1; s->rlayer.alert_count++; if (s->rlayer.alert_count == MAX_WARN_ALERT_COUNT) { @@ -841,7 +856,8 @@ int ssl3_read_bytes(SSL *ssl, int type, int *recvd_type, unsigned char *buf, SSL_AD_REASON_OFFSET + alert_descr, "SSL alert number %d", alert_descr); s->shutdown |= SSL_RECEIVED_SHUTDOWN; - ssl_release_record(s, rr); + if (!ssl_release_record(s, rr, 0)) + return -1; SSL_CTX_remove_session(s->session_ctx, s->session); return 0; } else if (alert_descr == SSL_AD_NO_RENEGOTIATION) { @@ -876,7 +892,8 @@ int ssl3_read_bytes(SSL *ssl, int type, int *recvd_type, unsigned char *buf, * sent close_notify. */ if (!SSL_CONNECTION_IS_TLS13(s)) { - ssl_release_record(s, rr); + if (!ssl_release_record(s, rr, 0)) + return -1; if ((s->mode & SSL_MODE_AUTO_RETRY) != 0) goto start; @@ -895,7 +912,8 @@ int ssl3_read_bytes(SSL *ssl, int type, int *recvd_type, unsigned char *buf, * above. * No alert sent because we already sent close_notify */ - ssl_release_record(s, rr); + if (!ssl_release_record(s, rr, 0)) + return -1; SSLfatal(s, SSL_AD_NO_ALERT, SSL_R_APPLICATION_DATA_AFTER_CLOSE_NOTIFY); return -1; @@ -918,12 +936,12 @@ int ssl3_read_bytes(SSL *ssl, int type, int *recvd_type, unsigned char *buf, n = rr->length; /* available bytes */ /* now move 'n' bytes: */ - memcpy(dest + *dest_len, rr->data + rr->off, n); - rr->off += n; - rr->length -= n; - *dest_len += n; - if (rr->length == 0) - ssl_release_record(s, rr); + if (n > 0) { + memcpy(dest + *dest_len, rr->data + rr->off, n); + *dest_len += n; + if (!ssl_release_record(s, rr, n)) + return -1; + } if (*dest_len < dest_maxlen) goto start; /* fragment was too small */ @@ -1027,7 +1045,8 @@ int ssl3_read_bytes(SSL *ssl, int type, int *recvd_type, unsigned char *buf, /* SSLfatal() already called */ return -1; } - ssl_release_record(s, rr); + if (!ssl_release_record(s, rr, 0)) + return -1; goto start; } else { SSLfatal(s, SSL_AD_UNEXPECTED_MESSAGE, SSL_R_UNEXPECTED_RECORD); diff --git a/ssl/record/record.h b/ssl/record/record.h index a277f09e18..7dcbbb36e9 100644 --- a/ssl/record/record.h +++ b/ssl/record/record.h @@ -165,7 +165,7 @@ __owur int dtls1_write_bytes(SSL_CONNECTION *s, int type, const void *buf, int do_dtls1_write(SSL_CONNECTION *s, int type, const unsigned char *buf, size_t len, size_t *written); void dtls1_increment_epoch(SSL_CONNECTION *s, int rw); -void ssl_release_record(SSL_CONNECTION *s, TLS_RECORD *rr); +int ssl_release_record(SSL_CONNECTION *s, TLS_RECORD *rr, size_t length); # define HANDLE_RLAYER_READ_RETURN(s, ret) \ ossl_tls_handle_rlayer_return(s, 0, ret, OPENSSL_FILE, OPENSSL_LINE) |