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)

diff --git a/include/internal/recordmethod.h b/include/internal/recordmethod.h
index fda3549590c6752b205e89e7adda5e6679988f57..30d22085689ddb79ed8ee32c0071450d0a9cba95 100644 (file)
--- 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 2a7cc03e553a429278056871d743e91988344e47..62f8425d0930b24b55aba113140950eeb9c339a4 100644 (file)
--- 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 beac10e9eb537e0cf34320c41213a6828fe1e112..e6310908d83235d7d585709f23d10c35748d9fee 100644 (file)
--- 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 998c1efddacf64b736d57ac7bdf9874542226641..32865fdbf15b42c7ee27171cb3b5d1c2c7a200a3 100644 (file)
--- 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 2520288dd4c2aaa07ca2911154611f4be1eba648..6a2d1288071fa3d371736da76eb3430efbfd9817 100644 (file)
--- 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 220ff3fcf1a02459fbcc91f72b30272257c3c430..c567b471eaefcd8cbc9171056aaa8091c21741da 100644 (file)
--- 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 8113d78da05692a52b18dfb382e61e9622b22314..a277f09e184ec716b7769a9b7265c3c13b836df1 100644 (file)
--- 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 89b29cb9a9c6e7e18b9932860e233537acfec7c6..415e4906b3e30c43dff3fe331ff81d01965664dd 100644 (file)
--- 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 c042830cb18d5e9a2c031faf7894da59e718b0fe..2e26a3f3df3d4cb01f452ea6271fff0ebb205a75 100644 (file)
--- 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 27d95c73df199d742360dc7fc1526d399c961d64..8089a8c0dc048ee1263492f408b457cfd5f67903 100644 (file)
--- 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;