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 <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/20404)

10 files changed, 28 insertions, 23 deletions

diff --git a/include/internal/recordmethod.h b/include/internal/recordmethod.h
index fda3549590..30d2208568 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 2a7cc03e55..62f8425d09 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 beac10e9eb..e6310908d8 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 998c1efdda..32865fdbf1 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 2520288dd4..6a2d128807 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 220ff3fcf1..c567b471ea 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 8113d78da0..a277f09e18 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 89b29cb9a9..415e4906b3 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 c042830cb1..2e26a3f3df 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 27d95c73df..8089a8c0dc 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;