From: Matt Caswell Date: Thu, 23 Feb 2023 17:02:54 +0000 (+0000) Subject: Make the data field for get_record() const X-Git-Tag: openssl-3.2.0-alpha1~1016 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2eb91b0ec325924ae4b7dc596617a6fff71d7ae6;p=thirdparty%2Fopenssl.git Make the data field for get_record() const Improves consistency with the QUIC rstream implementation - and improves the abstraction between the TLS implementation and the abstract record layer. We should not expect that the TLS implementation should be able to change the underlying buffer. Future record layers may not expect that. Reviewed-by: Tomas Mraz Reviewed-by: Paul Dale (Merged from https://github.com/openssl/openssl/pull/20404) --- diff --git a/include/internal/recordmethod.h b/include/internal/recordmethod.h index fda3549590c..30d22085689 100644 --- a/include/internal/recordmethod.h +++ b/include/internal/recordmethod.h @@ -231,7 +231,7 @@ struct ossl_record_method_st { * multiple records in one go and buffer them. */ int (*read_record)(OSSL_RECORD_LAYER *rl, void **rechandle, int *rversion, - int *type, unsigned char **data, size_t *datalen, + 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 diff --git a/ssl/quic/quic_tls.c b/ssl/quic/quic_tls.c index 2a7cc03e553..62f8425d093 100644 --- a/ssl/quic/quic_tls.c +++ b/ssl/quic/quic_tls.c @@ -332,7 +332,7 @@ static int quic_retry_write_records(OSSL_RECORD_LAYER *rl) } static int quic_read_record(OSSL_RECORD_LAYER *rl, void **rechandle, - int *rversion, int *type, unsigned char **data, + int *rversion, int *type, const unsigned char **data, size_t *datalen, uint16_t *epoch, unsigned char *seq_num) { @@ -341,11 +341,7 @@ static int quic_read_record(OSSL_RECORD_LAYER *rl, void **rechandle, BIO_clear_retry_flags(rl->dummybio); - /* - * TODO(QUIC): Cast data to const, which should be safe....can read_record - * be modified to pass this as const to start with? - */ - if (!rl->qtls->args.crypto_recv_rcd_cb((const unsigned char **)data, datalen, + if (!rl->qtls->args.crypto_recv_rcd_cb(data, datalen, rl->qtls->args.crypto_recv_rcd_cb_arg)) { QUIC_TLS_FATAL(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); return OSSL_RECORD_RETURN_FATAL; diff --git a/ssl/record/methods/recmethod_local.h b/ssl/record/methods/recmethod_local.h index beac10e9eb5..e6310908d83 100644 --- a/ssl/record/methods/recmethod_local.h +++ b/ssl/record/methods/recmethod_local.h @@ -459,7 +459,7 @@ int tls_retry_write_records(OSSL_RECORD_LAYER *rl); int tls_get_alert_code(OSSL_RECORD_LAYER *rl); int tls_set1_bio(OSSL_RECORD_LAYER *rl, BIO *bio); int tls_read_record(OSSL_RECORD_LAYER *rl, void **rechandle, int *rversion, - int *type, unsigned char **data, size_t *datalen, + 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_default_set_protocol_version(OSSL_RECORD_LAYER *rl, int version); diff --git a/ssl/record/methods/tls_common.c b/ssl/record/methods/tls_common.c index 998c1efddac..32865fdbf15 100644 --- a/ssl/record/methods/tls_common.c +++ b/ssl/record/methods/tls_common.c @@ -1088,7 +1088,7 @@ int tls13_common_post_process_record(OSSL_RECORD_LAYER *rl, TLS_RL_RECORD *rec) } int tls_read_record(OSSL_RECORD_LAYER *rl, void **rechandle, int *rversion, - int *type, unsigned char **data, size_t *datalen, + int *type, const unsigned char **data, size_t *datalen, uint16_t *epoch, unsigned char *seq_num) { TLS_RL_RECORD *rec; diff --git a/ssl/record/rec_layer_d1.c b/ssl/record/rec_layer_d1.c index 2520288dd4c..6a2d1288071 100644 --- a/ssl/record/rec_layer_d1.c +++ b/ssl/record/rec_layer_d1.c @@ -58,9 +58,10 @@ void DTLS_RECORD_LAYER_clear(RECORD_LAYER *rl) while ((item = pqueue_pop(d->buffered_app_data.q)) != NULL) { rec = (TLS_RECORD *)item->data; + if (rl->s->options & SSL_OP_CLEANSE_PLAINTEXT) - OPENSSL_cleanse(rec->data, rec->length); - OPENSSL_free(rec->data); + OPENSSL_cleanse(rec->allocdata, rec->length); + OPENSSL_free(rec->allocdata); OPENSSL_free(item->data); pitem_free(item); } @@ -99,7 +100,7 @@ static int dtls_buffer_record(SSL_CONNECTION *s, TLS_RECORD *rec) * now. Copying data isn't good - but this should be infrequent so we * accept it here. */ - rdata->data = OPENSSL_memdup(rec->data, rec->length); + rdata->data = rdata->allocdata = OPENSSL_memdup(rec->data, rec->length); if (rdata->data == NULL) { OPENSSL_free(rdata); pitem_free(item); @@ -126,7 +127,7 @@ static int dtls_buffer_record(SSL_CONNECTION *s, TLS_RECORD *rec) if (pqueue_insert(queue->q, item) == NULL) { /* Must be a duplicate so ignore it */ - OPENSSL_free(rdata->data); + OPENSSL_free(rdata->allocdata); OPENSSL_free(rdata); pitem_free(item); } @@ -349,8 +350,9 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, if (rr->length == 0) ssl_release_record(sc, rr); } else { + /* TODO(RECLAYER): Casting away the const is wrong! FIX ME */ if (sc->options & SSL_OP_CLEANSE_PLAINTEXT) - OPENSSL_cleanse(&(rr->data[rr->off]), n); + OPENSSL_cleanse((unsigned char *)&(rr->data[rr->off]), n); rr->length -= n; rr->off += n; if (rr->length == 0) @@ -380,7 +382,7 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf, if (rr->type == SSL3_RT_ALERT) { unsigned int alert_level, alert_descr; - unsigned char *alert_bytes = rr->data + rr->off; + const unsigned char *alert_bytes = rr->data + rr->off; PACKET alert; if (!PACKET_buf_init(&alert, alert_bytes, rr->length) diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index 220ff3fcf1a..c567b471eae 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -503,7 +503,8 @@ void ssl_release_record(SSL_CONNECTION *s, TLS_RECORD *rr) s->rlayer.rrlmethod->release_record(s->rlayer.rrl, rr->rechandle); } else { /* We allocated the buffers for this record (only happens with DTLS) */ - OPENSSL_free(rr->data); + OPENSSL_free(rr->allocdata); + rr->allocdata = NULL; } s->rlayer.curr_rec++; } @@ -720,8 +721,9 @@ int ssl3_read_bytes(SSL *ssl, int type, int *recvd_type, unsigned char *buf, if (rr->length == 0) ssl_release_record(s, rr); } else { + /* TODO(RECLAYER) Casting away the const here is wrong! FIX ME */ if (s->options & SSL_OP_CLEANSE_PLAINTEXT) - OPENSSL_cleanse(&(rr->data[rr->off]), n); + OPENSSL_cleanse((unsigned char *)&(rr->data[rr->off]), n); rr->length -= n; rr->off += n; if (rr->length == 0) @@ -784,8 +786,7 @@ int ssl3_read_bytes(SSL *ssl, int type, int *recvd_type, unsigned char *buf, if (rr->type == SSL3_RT_ALERT) { unsigned int alert_level, alert_descr; - unsigned char *alert_bytes = rr->data - + rr->off; + const unsigned char *alert_bytes = rr->data + rr->off; PACKET alert; if (!PACKET_buf_init(&alert, alert_bytes, rr->length) diff --git a/ssl/record/record.h b/ssl/record/record.h index 8113d78da05..a277f09e184 100644 --- a/ssl/record/record.h +++ b/ssl/record/record.h @@ -24,7 +24,12 @@ typedef struct tls_record_st { int version; int type; /* The data buffer containing bytes from the record */ - unsigned char *data; + const unsigned char *data; + /* + * Buffer that we allocated to store data. If non NULL always the same as + * data (but non-const) + */ + unsigned char *allocdata; /* Number of remaining to be read in the data buffer */ size_t length; /* Offset into the data buffer where to start reading */ diff --git a/ssl/ssl_local.h b/ssl/ssl_local.h index 89b29cb9a9c..415e4906b3e 100644 --- a/ssl/ssl_local.h +++ b/ssl/ssl_local.h @@ -2665,7 +2665,7 @@ __owur int dtls1_get_queue_priority(unsigned short seq, int is_ccs); int dtls1_retransmit_buffered_messages(SSL_CONNECTION *s); void dtls1_clear_received_buffer(SSL_CONNECTION *s); void dtls1_clear_sent_buffer(SSL_CONNECTION *s); -void dtls1_get_message_header(unsigned char *data, +void dtls1_get_message_header(const unsigned char *data, struct hm_header_st *msg_hdr); __owur OSSL_TIME dtls1_default_timeout(void); __owur OSSL_TIME *dtls1_get_timeout(SSL_CONNECTION *s, OSSL_TIME *timeleft); diff --git a/ssl/statem/statem_dtls.c b/ssl/statem/statem_dtls.c index c042830cb18..2e26a3f3df3 100644 --- a/ssl/statem/statem_dtls.c +++ b/ssl/statem/statem_dtls.c @@ -1301,7 +1301,8 @@ static unsigned char *dtls1_write_message_header(SSL_CONNECTION *s, return p; } -void dtls1_get_message_header(unsigned char *data, struct hm_header_st *msg_hdr) +void dtls1_get_message_header(const unsigned char *data, struct + hm_header_st *msg_hdr) { memset(msg_hdr, 0, sizeof(*msg_hdr)); msg_hdr->type = *(data++); diff --git a/test/sslapitest.c b/test/sslapitest.c index 27d95c73df1..8089a8c0dc0 100644 --- a/test/sslapitest.c +++ b/test/sslapitest.c @@ -1708,7 +1708,7 @@ static int execute_cleanse_plaintext(const SSL_METHOD *smeth, SSL_CTX *cctx = NULL, *sctx = NULL; SSL *clientssl = NULL, *serverssl = NULL; int testresult = 0; - void *zbuf; + const unsigned char *zbuf; SSL_CONNECTION *serversc; TLS_RECORD *rr;