Remove some unnecessary function pointers from OSSL_RECORD_METHOD

We had some temporary function pointers in OSSL_RECORD_METHOD which were
only necessary during the process of refactoring the read record layer.
These are no longer required so can be removed.

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/18132)
This commit is contained in:
Matt Caswell 2022-07-19 15:49:51 +01:00
parent 3a7a539ec5
commit 81c9ebd909
14 changed files with 73 additions and 226 deletions

View File

@ -561,6 +561,7 @@ extern "C" {
#define OSSL_LIBSSL_RECORD_LAYER_PARAM_OPTIONS "options"
#define OSSL_LIBSSL_RECORD_LAYER_PARAM_MODE "mode"
#define OSSL_LIBSSL_RECORD_LAYER_PARAM_READ_AHEAD "read_ahead"
#define OSSL_LIBSSL_RECORD_LAYER_READ_BUFFER_LEN "read_buffer_len"
#define OSSL_LIBSSL_RECORD_LAYER_PARAM_USE_ETM "use_etm"
#define OSSL_LIBSSL_RECORD_LAYER_PARAM_STREAM_MAC "stream_mac"
#define OSSL_LIBSSL_RECORD_LAYER_PARAM_TLSTREE "tlstree"

View File

@ -444,8 +444,8 @@ int DTLSv1_listen(SSL *ssl, BIO_ADDR *client)
unsigned char cookie[DTLS1_COOKIE_LENGTH];
unsigned char seq[SEQ_NUM_SIZE];
const unsigned char *data;
unsigned char *buf, *wbuf;
size_t fragoff, fraglen, msglen, reclen, align = 0;
unsigned char *buf = NULL, *wbuf;
size_t fragoff, fraglen, msglen;
unsigned int rectype, versmajor, msgseq, msgtype, clientvers, cookielen;
BIO *rbio, *wbio;
BIO_ADDR *tmpclient = NULL;
@ -490,21 +490,12 @@ int DTLSv1_listen(SSL *ssl, BIO_ADDR *client)
/* ERR_raise() already called */
return -1;
}
buf = s->rrlmethod->get0_rbuf(s->rrl)->buf;
buf = OPENSSL_malloc(DTLS1_RT_HEADER_LENGTH + SSL3_RT_MAX_PLAIN_LENGTH);
if (buf == NULL) {
ERR_raise(ERR_LIB_SSL, ERR_R_MALLOC_FAILURE);
return -1;
}
wbuf = RECORD_LAYER_get_wbuf(&s->rlayer)[0].buf;
#if defined(SSL3_ALIGN_PAYLOAD)
# if SSL3_ALIGN_PAYLOAD != 0
/*
* Using SSL3_RT_HEADER_LENGTH here instead of DTLS1_RT_HEADER_LENGTH for
* consistency with read_n. In practice it should make no difference
* for sensible values of SSL3_ALIGN_PAYLOAD because the difference between
* SSL3_RT_HEADER_LENGTH and DTLS1_RT_HEADER_LENGTH is exactly 8
*/
align = (size_t)buf + SSL3_RT_HEADER_LENGTH;
align = SSL3_ALIGN_PAYLOAD - 1 - ((align - 1) % SSL3_ALIGN_PAYLOAD);
# endif
#endif
buf += align;
do {
/* Get a packet */
@ -517,12 +508,14 @@ int DTLSv1_listen(SSL *ssl, BIO_ADDR *client)
/* Non-blocking IO */
goto end;
}
return -1;
ret = -1;
goto end;
}
if (!PACKET_buf_init(&pkt, buf, n)) {
ERR_raise(ERR_LIB_SSL, ERR_R_INTERNAL_ERROR);
return -1;
ret = -1;
goto end;
}
/*
@ -572,7 +565,6 @@ int DTLSv1_listen(SSL *ssl, BIO_ADDR *client)
ERR_raise(ERR_LIB_SSL, SSL_R_LENGTH_MISMATCH);
goto end;
}
reclen = PACKET_remaining(&msgpkt);
/*
* We allow data remaining at the end of the packet because there could
* be a second record (but we ignore it)
@ -666,7 +658,8 @@ int DTLSv1_listen(SSL *ssl, BIO_ADDR *client)
if (ssl->ctx->app_verify_cookie_cb == NULL) {
ERR_raise(ERR_LIB_SSL, SSL_R_NO_VERIFY_COOKIE_CALLBACK);
/* This is fatal */
return -1;
ret = -1;
goto end;
}
if (ssl->ctx->app_verify_cookie_cb(ssl, PACKET_data(&cookiepkt),
(unsigned int)PACKET_remaining(&cookiepkt)) == 0) {
@ -698,7 +691,8 @@ int DTLSv1_listen(SSL *ssl, BIO_ADDR *client)
cookielen > 255) {
ERR_raise(ERR_LIB_SSL, SSL_R_COOKIE_GEN_CALLBACK_FAILURE);
/* This is fatal */
return -1;
ret = -1;
goto end;
}
/*
@ -762,7 +756,8 @@ int DTLSv1_listen(SSL *ssl, BIO_ADDR *client)
ERR_raise(ERR_LIB_SSL, ERR_R_INTERNAL_ERROR);
WPACKET_cleanup(&wpkt);
/* This is fatal */
return -1;
ret = -1;
goto end;
}
/*
@ -805,7 +800,8 @@ int DTLSv1_listen(SSL *ssl, BIO_ADDR *client)
*/
goto end;
}
return -1;
ret = -1;
goto end;
}
if (BIO_flush(wbio) <= 0) {
@ -816,7 +812,8 @@ int DTLSv1_listen(SSL *ssl, BIO_ADDR *client)
*/
goto end;
}
return -1;
ret = -1;
goto end;
}
}
} while (next != LISTEN_SUCCESS);
@ -847,14 +844,32 @@ int DTLSv1_listen(SSL *ssl, BIO_ADDR *client)
if (BIO_dgram_get_peer(rbio, client) <= 0)
BIO_ADDR_clear(client);
/* Buffer the record in the processed_rcds queue */
/* TODO(RECLAYER): This is nasty and reaches inside the record layer. FIXME */
if (!dtls_buffer_listen_record(s->rrl, reclen, seq, align))
return -1;
/* Buffer the record for use by the record layer */
if (BIO_write(s->rrlnext, buf, n) != n) {
ERR_raise(ERR_LIB_SSL, ERR_R_INTERNAL_ERROR);
ret = -1;
goto end;
}
/*
* Reset the record layer - but this time we can use the record we just
* buffered in s->rrlnext
*/
if (!ssl_set_new_record_layer(s,
DTLS_ANY_VERSION,
OSSL_RECORD_DIRECTION_READ,
OSSL_RECORD_PROTECTION_LEVEL_NONE,
NULL, 0, NULL, 0, NULL, 0, NULL, 0,
NID_undef, NULL, NULL)) {
/* SSLfatal already called */
ret = -1;
goto end;
}
ret = 1;
end:
BIO_ADDR_free(tmpclient);
OPENSSL_free(buf);
return ret;
}
#endif

View File

@ -370,33 +370,6 @@ static int dtls_retrieve_rlayer_buffered_record(OSSL_RECORD_LAYER *rl,
return 0;
}
/* TODO(RECLAYER): FIXME. This is called directly from d1_lib.c. It should not be */
int dtls_buffer_listen_record(OSSL_RECORD_LAYER *rl, size_t len,
unsigned char *seq, size_t off)
{
SSL3_RECORD *rr;
rr = &rl->rrec[0];
memset(rr, 0, sizeof(SSL3_RECORD));
rr->length = len;
rr->type = SSL3_RT_HANDSHAKE;
memcpy(rr->seq_num, seq, sizeof(rr->seq_num));
rr->off = off;
rl->packet = rl->rbuf.buf;
rl->packet_length = DTLS1_RT_HEADER_LENGTH + len;
rr->data = rl->packet + DTLS1_RT_HEADER_LENGTH;
if (dtls_rlayer_buffer_record(rl, &(rl->processed_rcds),
SSL3_RECORD_get_seq_num(rr)) <= 0) {
/* SSLfatal() already called */
return 0;
}
return 1;
}
/*-
* Call this to get a new input record.
* It will return <= 0 if more data is needed, normally due to an error
@ -423,6 +396,13 @@ int dtls_get_more_records(OSSL_RECORD_LAYER *rl)
rr = rl->rrec;
if (rl->rbuf.buf == NULL) {
if (!rlayer_setup_read_buffer(rl)) {
/* RLAYERfatal() already called */
return OSSL_RECORD_RETURN_FATAL;
}
}
again:
/* if we're renegotiating, then there may be buffered records */
if (dtls_retrieve_rlayer_buffered_record(rl, &rl->processed_rcds)) {
@ -749,17 +729,5 @@ const OSSL_RECORD_METHOD ossl_dtls_record_method = {
NULL,
tls_set_first_handshake,
tls_set_max_pipelines,
dtls_set_in_init,
/*
* TODO(RECLAYER): Remove these. These function pointers are temporary hacks
* during the record layer refactoring. They need to be removed before the
* refactor is complete.
*/
tls_default_read_n,
tls_get0_rbuf,
tls_get0_packet,
tls_set0_packet,
tls_get_packet_length,
tls_reset_packet_length
dtls_set_in_init
};

View File

@ -545,17 +545,5 @@ const OSSL_RECORD_METHOD ossl_ktls_record_method = {
tls_set_plain_alerts,
tls_set_first_handshake,
tls_set_max_pipelines,
NULL,
/*
* TODO(RECLAYER): Remove these. These function pointers are temporary hacks
* during the record layer refactoring. They need to be removed before the
* refactor is complete.
*/
tls_default_read_n,
tls_get0_rbuf,
tls_get0_packet,
tls_set0_packet,
tls_get_packet_length,
tls_reset_packet_length
NULL
};

View File

@ -292,10 +292,4 @@ int tls_set_protocol_version(OSSL_RECORD_LAYER *rl, int version);
void tls_set_plain_alerts(OSSL_RECORD_LAYER *rl, int allow);
void tls_set_first_handshake(OSSL_RECORD_LAYER *rl, int first);
void tls_set_max_pipelines(OSSL_RECORD_LAYER *rl, size_t max_pipelines);
SSL3_BUFFER *tls_get0_rbuf(OSSL_RECORD_LAYER *rl);
unsigned char *tls_get0_packet(OSSL_RECORD_LAYER *rl);
void tls_set0_packet(OSSL_RECORD_LAYER *rl, unsigned char *packet,
size_t packetlen);
size_t tls_get_packet_length(OSSL_RECORD_LAYER *rl);
void tls_reset_packet_length(OSSL_RECORD_LAYER *rl);
int rlayer_setup_read_buffer(OSSL_RECORD_LAYER *rl);

View File

@ -149,11 +149,6 @@ static int rlayer_release_read_buffer(OSSL_RECORD_LAYER *rl)
return 1;
}
void tls_reset_packet_length(OSSL_RECORD_LAYER *rl)
{
rl->packet_length = 0;
}
/*
* Return values are as per SSL_read()
*/
@ -611,7 +606,7 @@ int tls_get_more_records(OSSL_RECORD_LAYER *rl)
num_recs++;
/* we have pulled in a full packet so zero things */
tls_reset_packet_length(rl);
rl->packet_length = 0;
rl->is_first_record = 0;
} while (num_recs < max_recs
&& thisrr->type == SSL3_RT_APPLICATION_DATA
@ -1055,6 +1050,12 @@ tls_int_new_record_layer(OSSL_LIB_CTX *libctx, const char *propq, int vers,
goto err;
}
p = OSSL_PARAM_locate_const(options, OSSL_LIBSSL_RECORD_LAYER_READ_BUFFER_LEN);
if (p != NULL && !OSSL_PARAM_get_size_t(p, &rl->rbuf.default_len)) {
RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, SSL_R_FAILED_TO_GET_PARAMETER);
goto err;
}
/* Loop through all the settings since they must all be understood */
for (p = settings; p->key != NULL; p++) {
if (strcmp(p->key, OSSL_LIBSSL_RECORD_LAYER_PARAM_USE_ETM) == 0) {
@ -1347,28 +1348,6 @@ void tls_set_max_pipelines(OSSL_RECORD_LAYER *rl, size_t max_pipelines)
rl->read_ahead = 1;
}
SSL3_BUFFER *tls_get0_rbuf(OSSL_RECORD_LAYER *rl)
{
return &rl->rbuf;
}
unsigned char *tls_get0_packet(OSSL_RECORD_LAYER *rl)
{
return rl->packet;
}
void tls_set0_packet(OSSL_RECORD_LAYER *rl, unsigned char *packet,
size_t packetlen)
{
rl->packet = packet;
rl->packet_length = packetlen;
}
size_t tls_get_packet_length(OSSL_RECORD_LAYER *rl)
{
return rl->packet_length;
}
const OSSL_RECORD_METHOD ossl_tls_record_method = {
tls_new_record_layer,
tls_free,
@ -1389,17 +1368,5 @@ const OSSL_RECORD_METHOD ossl_tls_record_method = {
tls_set_plain_alerts,
tls_set_first_handshake,
tls_set_max_pipelines,
NULL,
/*
* TODO(RECLAYER): Remove these. These function pointers are temporary hacks
* during the record layer refactoring. They need to be removed before the
* refactor is complete.
*/
tls_default_read_n,
tls_get0_rbuf,
tls_get0_packet,
tls_set0_packet,
tls_get_packet_length,
tls_reset_packet_length
NULL
};

View File

@ -233,14 +233,6 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
if (sc == NULL)
return -1;
if (!SSL3_BUFFER_is_initialised(sc->rrlmethod->get0_rbuf(sc->rrl))) {
/* Not initialized yet */
if (!ssl3_setup_buffers(sc)) {
/* SSLfatal() already called */
return -1;
}
}
if ((type && (type != SSL3_RT_APPLICATION_DATA) &&
(type != SSL3_RT_HANDSHAKE)) ||
(peek && (type != SSL3_RT_APPLICATION_DATA))) {
@ -542,7 +534,7 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
}
ssl_release_record(sc, rr);
if (!(sc->mode & SSL_MODE_AUTO_RETRY)) {
if (SSL3_BUFFER_get_left(sc->rrlmethod->get0_rbuf(sc->rrl)) == 0) {
if (!sc->rrlmethod->unprocessed_read_pending(sc->rrl)) {
/* no read-ahead left? */
BIO *bio;
@ -578,7 +570,7 @@ int dtls1_read_bytes(SSL *s, int type, int *recvd_type, unsigned char *buf,
return -1;
if (!(sc->mode & SSL_MODE_AUTO_RETRY)) {
if (SSL3_BUFFER_get_left(sc->rrlmethod->get0_rbuf(sc->rrl)) == 0) {
if (!sc->rrlmethod->unprocessed_read_pending(sc->rrl)) {
/* no read-ahead left? */
BIO *bio;
/*

View File

@ -68,8 +68,6 @@ void RECORD_LAYER_clear(RECORD_LAYER *rl)
void RECORD_LAYER_release(RECORD_LAYER *rl)
{
if (SSL3_BUFFER_is_initialised(&rl->rbuf))
ssl3_release_read_buffer(rl->s);
if (rl->numwpipes > 0)
ssl3_release_write_buffer(rl->s);
SSL3_RECORD_release(rl->rrec, SSL_MAX_PIPELINES);
@ -112,9 +110,6 @@ size_t ssl3_pending(const SSL *s)
if (sc == NULL)
return 0;
if (sc->rlayer.rstate == SSL_ST_READ_BODY)
return 0;
if (SSL_CONNECTION_IS_DTLS(sc)) {
TLS_RECORD *rdata;
pitem *item, *iter;
@ -132,6 +127,8 @@ size_t ssl3_pending(const SSL *s)
num += sc->rlayer.tlsrecs[i].length;
}
num += sc->rrlmethod->app_data_pending(sc->rrl);
return num;
}
@ -146,7 +143,7 @@ void SSL_set_default_read_buffer_len(SSL *s, size_t len)
if (sc == NULL)
return;
SSL3_BUFFER_set_default_len(sc->rrlmethod->get0_rbuf(sc->rrl), len);
sc->default_read_buf_len = len;
}
const char *SSL_rstate_string_long(const SSL *s)
@ -1797,7 +1794,7 @@ int ssl_set_new_record_layer(SSL_CONNECTION *s, int version,
int mactype, const EVP_MD *md,
const SSL_COMP *comp)
{
OSSL_PARAM options[4], *opts = options;
OSSL_PARAM options[5], *opts = options;
OSSL_PARAM settings[6], *set = settings;
const OSSL_RECORD_METHOD *origmeth = s->rrlmethod;
SSL_CTX *sctx = SSL_CONNECTION_GET_CTX(s);
@ -1827,6 +1824,8 @@ int ssl_set_new_record_layer(SSL_CONNECTION *s, int version,
&s->options);
*opts++ = OSSL_PARAM_construct_uint32(OSSL_LIBSSL_RECORD_LAYER_PARAM_MODE,
&s->mode);
*opts++ = OSSL_PARAM_construct_size_t(OSSL_LIBSSL_RECORD_LAYER_READ_BUFFER_LEN,
&s->default_read_buf_len);
*opts++ = OSSL_PARAM_construct_int(OSSL_LIBSSL_RECORD_LAYER_PARAM_READ_AHEAD,
&s->rlayer.read_ahead);
*opts = OSSL_PARAM_construct_end();

View File

@ -221,7 +221,6 @@ typedef struct ssl_mac_buf_st SSL_MAC_BUF;
#define RECORD_LAYER_set_read_ahead(rl, ra) ((rl)->read_ahead = (ra))
#define RECORD_LAYER_get_read_ahead(rl) ((rl)->read_ahead)
#define RECORD_LAYER_get_packet(rl) ((rl)->packet)
#define RECORD_LAYER_get_packet_length(rl) ((rl)->packet_length)
#define RECORD_LAYER_add_packet_length(rl, inc) ((rl)->packet_length += (inc))
#define DTLS_RECORD_LAYER_get_w_epoch(rl) ((rl)->d->w_epoch)
#define RECORD_LAYER_get_rbuf(rl) (&(rl)->rbuf)
@ -274,8 +273,6 @@ __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, int create_empty_fragment, size_t *written);
void dtls1_reset_seq_numbers(SSL_CONNECTION *s, int rw);
int dtls_buffer_listen_record(OSSL_RECORD_LAYER *rl, size_t len,
unsigned char *seq, size_t off);
void ssl_release_record(SSL_CONNECTION *s, TLS_RECORD *rr);
# define HANDLE_RLAYER_RETURN(s, ret) \

View File

@ -59,10 +59,8 @@ void ssl3_record_sequence_update(unsigned char *seq);
void SSL3_BUFFER_clear(SSL3_BUFFER *b);
void SSL3_BUFFER_set_data(SSL3_BUFFER *b, const unsigned char *d, size_t n);
void SSL3_BUFFER_release(SSL3_BUFFER *b);
__owur int ssl3_setup_read_buffer(SSL_CONNECTION *s);
__owur int ssl3_setup_write_buffer(SSL_CONNECTION *s, size_t numwpipes,
size_t len);
int ssl3_release_read_buffer(SSL_CONNECTION *s);
int ssl3_release_write_buffer(SSL_CONNECTION *s);
/* Macros/functions provided by the SSL3_RECORD component */

View File

@ -315,20 +315,6 @@ struct ossl_record_method_st {
* not. Default at creation of the record layer is "yes".
*/
void (*set_in_init)(OSSL_RECORD_LAYER *rl, int in_init);
/*
* TODO(RECLAYER): Remove these. These function pointers are temporary hacks
* during the record layer refactoring. They need to be removed before the
* refactor is complete.
*/
int (*read_n)(OSSL_RECORD_LAYER *rl, size_t n, size_t max, int extend,
int clearold, size_t *readbytes);
SSL3_BUFFER *(*get0_rbuf)(OSSL_RECORD_LAYER *rl);
unsigned char *(*get0_packet)(OSSL_RECORD_LAYER *rl);
void (*set0_packet)(OSSL_RECORD_LAYER *rl, unsigned char *packet,
size_t packetlen);
size_t (*get_packet_length)(OSSL_RECORD_LAYER *rl);
void (*reset_packet_length)(OSSL_RECORD_LAYER *rl);
};

View File

@ -34,50 +34,7 @@ void SSL3_BUFFER_release(SSL3_BUFFER *b)
b->buf = NULL;
}
int ssl3_setup_read_buffer(SSL_CONNECTION *s)
{
unsigned char *p;
size_t len, align = 0, headerlen;
SSL3_BUFFER *b;
b = s->rrlmethod->get0_rbuf(s->rrl);
if (SSL_CONNECTION_IS_DTLS(s))
headerlen = DTLS1_RT_HEADER_LENGTH;
else
headerlen = SSL3_RT_HEADER_LENGTH;
#if defined(SSL3_ALIGN_PAYLOAD) && SSL3_ALIGN_PAYLOAD!=0
align = (-SSL3_RT_HEADER_LENGTH) & (SSL3_ALIGN_PAYLOAD - 1);
#endif
if (b->buf == NULL) {
len = SSL3_RT_MAX_PLAIN_LENGTH
+ SSL3_RT_MAX_ENCRYPTED_OVERHEAD + headerlen + align;
#ifndef OPENSSL_NO_COMP
if (ssl_allow_compression(s))
len += SSL3_RT_MAX_COMPRESSED_OVERHEAD;
#endif
if (b->default_len > len)
len = b->default_len;
if ((p = OPENSSL_malloc(len)) == NULL) {
/*
* We've got a malloc failure, and we're still initialising buffers.
* We assume we're so doomed that we won't even be able to send an
* alert.
*/
SSLfatal(s, SSL_AD_NO_ALERT, ERR_R_MALLOC_FAILURE);
return 0;
}
b->buf = p;
b->len = len;
}
return 1;
}
int ssl3_setup_write_buffer(SSL_CONNECTION *s, size_t numwpipes,
size_t len)
int ssl3_setup_write_buffer(SSL_CONNECTION *s, size_t numwpipes, size_t len)
{
unsigned char *p;
size_t align = 0, headerlen;
@ -142,10 +99,6 @@ int ssl3_setup_write_buffer(SSL_CONNECTION *s, size_t numwpipes,
int ssl3_setup_buffers(SSL_CONNECTION *s)
{
if (!ssl3_setup_read_buffer(s)) {
/* SSLfatal() already called */
return 0;
}
if (!ssl3_setup_write_buffer(s, 1, 0)) {
/* SSLfatal() already called */
return 0;
@ -172,15 +125,3 @@ int ssl3_release_write_buffer(SSL_CONNECTION *s)
s->rlayer.numwpipes = 0;
return 1;
}
int ssl3_release_read_buffer(SSL_CONNECTION *s)
{
SSL3_BUFFER *b;
b = s->rrlmethod->get0_rbuf(s->rrl);
if (s->options & SSL_OP_CLEANSE_PLAINTEXT)
OPENSSL_cleanse(b->buf, b->len);
OPENSSL_free(b->buf);
b->buf = NULL;
return 1;
}

View File

@ -812,8 +812,7 @@ SSL *ossl_ssl_connection_new(SSL_CTX *ctx)
s->max_send_fragment = ctx->max_send_fragment;
s->split_send_fragment = ctx->split_send_fragment;
s->max_pipelines = ctx->max_pipelines;
if (ctx->default_read_buf_len > 0)
SSL_set_default_read_buffer_len(ssl, ctx->default_read_buf_len);
s->default_read_buf_len = ctx->default_read_buf_len;
s->ext.debug_cb = 0;
s->ext.debug_arg = NULL;

View File

@ -1773,6 +1773,8 @@ struct ssl_connection_st {
OSSL_RECORD_LAYER *rrl;
/* BIO to store data destined for the next record layer epoch */
BIO *rrlnext;
/* Default read buffer length to be passed to the record layer */
size_t default_read_buf_len;
/* Default password callback. */
pem_password_cb *default_passwd_callback;