Rationalise RECORD_LAYER_clear() and clear_record_layer()

We had two functions which were very similarly named, that did almost the
same thing, but not quite. We bring the two together. Doing this also fixes
a possible bug where some data may not be correctly freed when the
RECORD_LAYER_clear() version was used.

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/23256)

diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c
index 4d057ec1846b77d291a2e37363c2120d1d729bd7..12a4ff8e98d4b846d6735bbb55ed0a3879f32c14 100644 (file)
--- a/ssl/record/rec_layer_s3.c
+++ b/ssl/record/rec_layer_s3.c
@@ -25,14 +25,29 @@ void RECORD_LAYER_init(RECORD_LAYER *rl, SSL_CONNECTION *s)
     rl->s = s;
 }
 
-void RECORD_LAYER_clear(RECORD_LAYER *rl)
+int RECORD_LAYER_clear(RECORD_LAYER *rl)
 {
+    int ret = 1;
+
+    /* Clear any buffered records we no longer need */
+    while (rl->curr_rec < rl->num_recs)
+        ret &= ssl_release_record(rl->s,
+                                  &(rl->tlsrecs[rl->curr_rec++]),
+                                  0);
+
+
     rl->wnum = 0;
     memset(rl->handshake_fragment, 0, sizeof(rl->handshake_fragment));
     rl->handshake_fragment_len = 0;
     rl->wpend_tot = 0;
     rl->wpend_type = 0;
     rl->wpend_buf = NULL;
+    rl->alert_count = 0;
+    rl->num_recs = 0;
+    rl->curr_rec = 0;
+
+    BIO_free(rl->rrlnext);
+    rl->rrlnext = NULL;
 
     if (rl->rrlmethod != NULL)
         rl->rrlmethod->free(rl->rrl); /* Ignore return value */
@@ -47,6 +62,35 @@ void RECORD_LAYER_clear(RECORD_LAYER *rl)
 
     if (rl->d)
         DTLS_RECORD_LAYER_clear(rl);
+
+    return ret;
+}
+
+int RECORD_LAYER_reset(RECORD_LAYER *rl)
+{
+    int ret;
+
+    ret = RECORD_LAYER_clear(rl);
+
+    /* We try and reset both record layers even if one fails */
+    ret &= ssl_set_new_record_layer(rl->s,
+                                    SSL_CONNECTION_IS_DTLS(rl->s)
+                                        ? DTLS_ANY_VERSION : TLS_ANY_VERSION,
+                                    OSSL_RECORD_DIRECTION_READ,
+                                    OSSL_RECORD_PROTECTION_LEVEL_NONE, NULL, 0,
+                                    NULL, 0, NULL, 0, NULL,  0, NULL, 0,
+                                    NID_undef, NULL, NULL, NULL);
+
+    ret &= ssl_set_new_record_layer(rl->s,
+                                    SSL_CONNECTION_IS_DTLS(rl->s)
+                                        ? DTLS_ANY_VERSION : TLS_ANY_VERSION,
+                                    OSSL_RECORD_DIRECTION_WRITE,
+                                    OSSL_RECORD_PROTECTION_LEVEL_NONE, NULL, 0,
+                                    NULL, 0, NULL, 0, NULL,  0, NULL, 0,
+                                    NID_undef, NULL, NULL, NULL);
+
+    /* SSLfatal already called in the event of failure */
+    return ret;
 }
 
 /* Checks if we have unprocessed read ahead data pending */

diff --git a/ssl/record/record.h b/ssl/record/record.h
index ae5afc7b4a832aab3f4b9d07affc2183176a965b..6c8545d7069581de85be7f71320f5aa393479314 100644 (file)
--- a/ssl/record/record.h
+++ b/ssl/record/record.h
@@ -139,7 +139,8 @@ typedef struct record_layer_st {
 #define RECORD_LAYER_get_read_ahead(rl)         ((rl)->read_ahead)
 
 void RECORD_LAYER_init(RECORD_LAYER *rl, SSL_CONNECTION *s);
-void RECORD_LAYER_clear(RECORD_LAYER *rl);
+int RECORD_LAYER_clear(RECORD_LAYER *rl);
+int RECORD_LAYER_reset(RECORD_LAYER *rl);
 int RECORD_LAYER_read_pending(const RECORD_LAYER *rl);
 int RECORD_LAYER_processed_read_pending(const RECORD_LAYER *rl);
 int RECORD_LAYER_write_pending(const RECORD_LAYER *rl);

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 8a834c3527a7fef13a4027cba047814f39a9ed07..818d5d11a1e97a35c75c9d00a043d5f767feb814 100644 (file)
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -558,50 +558,6 @@ static int ssl_check_allowed_versions(int min_version, int max_version)
 void OPENSSL_VPROC_FUNC(void) {}
 #endif
 
-static int clear_record_layer(SSL_CONNECTION *s)
-{
-    int ret = 1;
-
-    /* Clear any buffered records we no longer need */
-    while (s->rlayer.curr_rec < s->rlayer.num_recs)
-        ret &= ssl_release_record(s,
-                                  &(s->rlayer.tlsrecs[s->rlayer.curr_rec ++]),
-                                  0);
-
-    BIO_free(s->rlayer.rrlnext);
-    s->rlayer.rrlnext = NULL;
-
-    /* Reset various fields */
-    s->rlayer.wnum = 0;
-    s->rlayer.handshake_fragment_len = 0;
-    s->rlayer.wpend_tot = 0;
-    s->rlayer.wpend_type = 0;
-    s->rlayer.wpend_buf = NULL;
-    s->rlayer.alert_count = 0;
-    s->rlayer.num_recs = 0;
-    s->rlayer.curr_rec = 0;
-
-    /* We try and reset both record layers even if one fails */
-    ret &= ssl_set_new_record_layer(s,
-                                    SSL_CONNECTION_IS_DTLS(s) ? DTLS_ANY_VERSION
-                                                              : TLS_ANY_VERSION,
-                                    OSSL_RECORD_DIRECTION_READ,
-                                    OSSL_RECORD_PROTECTION_LEVEL_NONE, NULL, 0,
-                                    NULL, 0, NULL, 0, NULL,  0, NULL, 0,
-                                    NID_undef, NULL, NULL, NULL);
-
-    ret &= ssl_set_new_record_layer(s,
-                                    SSL_CONNECTION_IS_DTLS(s) ? DTLS_ANY_VERSION
-                                                              : TLS_ANY_VERSION,
-                                    OSSL_RECORD_DIRECTION_WRITE,
-                                    OSSL_RECORD_PROTECTION_LEVEL_NONE, NULL, 0,
-                                    NULL, 0, NULL, 0, NULL,  0, NULL, 0,
-                                    NID_undef, NULL, NULL, NULL);
-
-    /* SSLfatal already called in the event of failure */
-    return ret;
-}
-
 int SSL_clear(SSL *s)
 {
     if (s->method == NULL) {
@@ -687,11 +643,7 @@ int ossl_ssl_connection_reset(SSL *s)
             return 0;
     }
 
-    RECORD_LAYER_clear(&sc->rlayer);
-    BIO_free(sc->rlayer.rrlnext);
-    sc->rlayer.rrlnext = NULL;
-
-    if (!clear_record_layer(sc))
+    if (!RECORD_LAYER_reset(&sc->rlayer))
         return 0;
 
     return 1;
@@ -1455,6 +1407,7 @@ void ossl_ssl_connection_free(SSL *ssl)
     /* Ignore return value */
     ssl_free_wbio_buffer(s);
 
+    /* Ignore return value */
     RECORD_LAYER_clear(&s->rlayer);
 
     BUF_MEM_free(s->init_buf);
@@ -4795,7 +4748,7 @@ void SSL_set_accept_state(SSL *s)
     ossl_statem_clear(sc);
     sc->handshake_func = s->method->ssl_accept;
     /* Ignore return value. Its a void public API function */
-    clear_record_layer(sc);
+    RECORD_LAYER_reset(&sc->rlayer);
 }
 
 void SSL_set_connect_state(SSL *s)
@@ -4814,7 +4767,7 @@ void SSL_set_connect_state(SSL *s)
     ossl_statem_clear(sc);
     sc->handshake_func = s->method->ssl_connect;
     /* Ignore return value. Its a void public API function */
-    clear_record_layer(sc);
+    RECORD_LAYER_reset(&sc->rlayer);
 }
 
 int ssl_undefined_function(SSL *s)