From 359affdead3af497f1673204c5c34061d28dfa7b Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Tue, 17 May 2022 16:16:40 +0100 Subject: [PATCH] Add support for moving data from one epoch to the next Sometimes data read by a record layer in one epoch is actually intended for the next epoch. For example in a TLS with read_ahead, the read_ahead data could contain a KeyUpdate message followed by application data encrypted with new keys. Therefore we implement a mechanism for passing this data across the epochs. Reviewed-by: Hugo Landau Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/18132) --- ssl/record/methods/ktls_meth.c | 9 +-- ssl/record/methods/recmethod_local.h | 20 +++++- ssl/record/methods/tls_common.c | 93 +++++++++++++++++++++------- ssl/record/rec_layer_s3.c | 22 +++++-- ssl/record/recordmethod.h | 7 ++- ssl/ssl_lib.c | 6 ++ ssl/ssl_local.h | 3 +- 7 files changed, 123 insertions(+), 37 deletions(-) diff --git a/ssl/record/methods/ktls_meth.c b/ssl/record/methods/ktls_meth.c index 3a9c836077..59f52e4488 100644 --- a/ssl/record/methods/ktls_meth.c +++ b/ssl/record/methods/ktls_meth.c @@ -481,8 +481,8 @@ ktls_new_record_layer(OSSL_LIB_CTX *libctx, const char *propq, int vers, const EVP_CIPHER *ciph, size_t taglen, /* TODO(RECLAYER): This probably should not be an int */ int mactype, - const EVP_MD *md, const SSL_COMP *comp, BIO *transport, - BIO_ADDR *local, BIO_ADDR *peer, + const EVP_MD *md, const SSL_COMP *comp, BIO *prev, + BIO *transport, BIO *next, BIO_ADDR *local, BIO_ADDR *peer, const OSSL_PARAM *settings, const OSSL_PARAM *options, OSSL_RECORD_LAYER **retrl, /* TODO(RECLAYER): Remove me */ @@ -492,8 +492,9 @@ ktls_new_record_layer(OSSL_LIB_CTX *libctx, const char *propq, int vers, ret = tls_int_new_record_layer(libctx, propq, vers, role, direction, level, key, keylen, iv, ivlen, mackey, mackeylen, - ciph, taglen, mactype, md, comp, transport, - local, peer, settings, options, retrl, s); + ciph, taglen, mactype, md, comp, prev, + transport, next, local, peer, settings, + options, retrl, s); if (ret != OSSL_RECORD_RETURN_SUCCESS) return ret; diff --git a/ssl/record/methods/recmethod_local.h b/ssl/record/methods/recmethod_local.h index ef124c8237..8cad4b3979 100644 --- a/ssl/record/methods/recmethod_local.h +++ b/ssl/record/methods/recmethod_local.h @@ -68,7 +68,22 @@ struct ossl_record_layer_st int version; int role; int direction; + + /* + * A BIO containing any data read in the previous epoch that was destined + * for this epoch + */ + BIO *prev; + + /* The transport BIO */ BIO *bio; + + /* + * A BIO where we will send any data read by us that is destined for the + * next epoch. + */ + BIO *next; + /* Types match the equivalent structures in the SSL object */ uint64_t options; /* @@ -190,13 +205,14 @@ tls_int_new_record_layer(OSSL_LIB_CTX *libctx, const char *propq, int vers, const EVP_CIPHER *ciph, size_t taglen, /* TODO(RECLAYER): This probably should not be an int */ int mactype, - const EVP_MD *md, const SSL_COMP *comp, BIO *transport, + const EVP_MD *md, const SSL_COMP *comp, BIO *prev, + BIO *transport, BIO *next, BIO_ADDR *local, BIO_ADDR *peer, const OSSL_PARAM *settings, const OSSL_PARAM *options, OSSL_RECORD_LAYER **retrl, /* TODO(RECLAYER): Remove me */ SSL_CONNECTION *s); -void tls_free(OSSL_RECORD_LAYER *rl); +int tls_free(OSSL_RECORD_LAYER *rl); int tls_reset(OSSL_RECORD_LAYER *rl); int tls_unprocessed_read_pending(OSSL_RECORD_LAYER *rl); int tls_processed_read_pending(OSSL_RECORD_LAYER *rl); diff --git a/ssl/record/methods/tls_common.c b/ssl/record/methods/tls_common.c index fa6551dffb..2b26829e4b 100644 --- a/ssl/record/methods/tls_common.c +++ b/ssl/record/methods/tls_common.c @@ -19,6 +19,8 @@ # define SSL_AD_NO_ALERT -1 +static void tls_int_free(OSSL_RECORD_LAYER *rl); + void ossl_rlayer_fatal(OSSL_RECORD_LAYER *rl, int al, int reason, const char *fmt, ...) { @@ -284,6 +286,7 @@ int tls_default_read_n(OSSL_RECORD_LAYER *rl, size_t n, size_t max, int extend, while (left < n) { size_t bioread = 0; int ret; + BIO *bio = rl->prev != NULL ? rl->prev : rl->bio; /* * Now we have len+left bytes at the front of s->s3.rbuf.buf and @@ -292,14 +295,23 @@ int tls_default_read_n(OSSL_RECORD_LAYER *rl, size_t n, size_t max, int extend, */ clear_sys_error(); - if (rl->bio != NULL) { - ret = BIO_read(rl->bio, pkt + len + left, max - left); + if (bio != NULL) { + ret = BIO_read(bio, pkt + len + left, max - left); if (ret > 0) { bioread = ret; ret = OSSL_RECORD_RETURN_SUCCESS; - } else if (BIO_should_retry(rl->bio)) { + } else if (BIO_should_retry(bio)) { + if (rl->prev != NULL) { + /* + * We were reading from the previous epoch. Now there is no + * more data, so swap to the actual transport BIO + */ + BIO_free(rl->prev); + rl->prev = NULL; + continue; + } ret = OSSL_RECORD_RETURN_RETRY; - } else if (BIO_eof(rl->bio)) { + } else if (BIO_eof(bio)) { ret = OSSL_RECORD_RETURN_EOF; } else { ret = OSSL_RECORD_RETURN_FATAL; @@ -989,10 +1001,10 @@ tls_int_new_record_layer(OSSL_LIB_CTX *libctx, const char *propq, int vers, const EVP_CIPHER *ciph, size_t taglen, /* TODO(RECLAYER): This probably should not be an int */ int mactype, - const EVP_MD *md, const SSL_COMP *comp, BIO *transport, - BIO_ADDR *local, BIO_ADDR *peer, - const OSSL_PARAM *settings, const OSSL_PARAM *options, - OSSL_RECORD_LAYER **retrl, + const EVP_MD *md, const SSL_COMP *comp, BIO *prev, + BIO *transport, BIO *next, BIO_ADDR *local, + BIO_ADDR *peer, const OSSL_PARAM *settings, + const OSSL_PARAM *options, OSSL_RECORD_LAYER **retrl, /* TODO(RECLAYER): Remove me */ SSL_CONNECTION *s) { @@ -1006,11 +1018,6 @@ tls_int_new_record_layer(OSSL_LIB_CTX *libctx, const char *propq, int vers, return OSSL_RECORD_RETURN_FATAL; } - if (transport != NULL && !BIO_up_ref(transport)) { - RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_MALLOC_FAILURE); - goto err; - } - /* * TODO(RECLAYER): Need to handle the case where the params are updated * after the record layer has been created. @@ -1058,10 +1065,18 @@ tls_int_new_record_layer(OSSL_LIB_CTX *libctx, const char *propq, int vers, if (!tls_set1_bio(rl, transport)) goto err; + if (prev != NULL && !BIO_up_ref(prev)) + goto err; + rl->prev = prev; + + if (next != NULL && !BIO_up_ref(next)) + goto err; + rl->next = next; + *retrl = rl; return OSSL_RECORD_RETURN_SUCCESS; err: - OPENSSL_free(rl); + tls_int_free(rl); return OSSL_RECORD_RETURN_FATAL; } @@ -1073,8 +1088,8 @@ tls_new_record_layer(OSSL_LIB_CTX *libctx, const char *propq, int vers, const EVP_CIPHER *ciph, size_t taglen, /* TODO(RECLAYER): This probably should not be an int */ int mactype, - const EVP_MD *md, const SSL_COMP *comp, BIO *transport, - BIO_ADDR *local, BIO_ADDR *peer, + const EVP_MD *md, const SSL_COMP *comp, BIO *prev, + BIO *transport, BIO *next, BIO_ADDR *local, BIO_ADDR *peer, const OSSL_PARAM *settings, const OSSL_PARAM *options, OSSL_RECORD_LAYER **retrl, /* TODO(RECLAYER): Remove me */ @@ -1084,8 +1099,9 @@ tls_new_record_layer(OSSL_LIB_CTX *libctx, const char *propq, int vers, ret = tls_int_new_record_layer(libctx, propq, vers, role, direction, level, key, keylen, iv, ivlen, mackey, mackeylen, - ciph, taglen, mactype, md, comp, transport, - local, peer, settings, options, retrl, s); + ciph, taglen, mactype, md, comp, prev, + transport, next, local, peer, settings, + options, retrl, s); if (ret != OSSL_RECORD_RETURN_SUCCESS) return ret; @@ -1147,8 +1163,8 @@ dtls_new_record_layer(OSSL_LIB_CTX *libctx, const char *propq, int vers, const EVP_CIPHER *ciph, size_t taglen, /* TODO(RECLAYER): This probably should not be an int */ int mactype, - const EVP_MD *md, const SSL_COMP *comp, BIO *transport, - BIO_ADDR *local, BIO_ADDR *peer, + const EVP_MD *md, const SSL_COMP *comp, BIO *prev, + BIO *transport, BIO *next, BIO_ADDR *local, BIO_ADDR *peer, const OSSL_PARAM *settings, const OSSL_PARAM *options, OSSL_RECORD_LAYER **retrl, /* TODO(RECLAYER): Remove me */ @@ -1159,8 +1175,9 @@ dtls_new_record_layer(OSSL_LIB_CTX *libctx, const char *propq, int vers, ret = tls_int_new_record_layer(libctx, propq, vers, role, direction, level, key, keylen, iv, ivlen, mackey, mackeylen, - ciph, taglen, mactype, md, comp, transport, - local, peer, settings, options, retrl, s); + ciph, taglen, mactype, md, comp, prev, + transport, next, local, peer, settings, + options, retrl, s); if (ret != OSSL_RECORD_RETURN_SUCCESS) return ret; @@ -1171,13 +1188,43 @@ dtls_new_record_layer(OSSL_LIB_CTX *libctx, const char *propq, int vers, return OSSL_RECORD_RETURN_SUCCESS; } -void tls_free(OSSL_RECORD_LAYER *rl) +static void tls_int_free(OSSL_RECORD_LAYER *rl) { /* TODO(RECLAYER): Cleanse sensitive fields */ + BIO_free(rl->prev); BIO_free(rl->bio); + BIO_free(rl->next); + SSL3_BUFFER_release(&rl->rbuf); + + EVP_CIPHER_CTX_free(rl->enc_read_ctx); + EVP_MD_CTX_free(rl->read_hash); + COMP_CTX_free(rl->expand); + OPENSSL_free(rl); } +int tls_free(OSSL_RECORD_LAYER *rl) +{ + SSL3_BUFFER *rbuf; + size_t left, written; + int ret = 1; + + rbuf = &rl->rbuf; + + left = SSL3_BUFFER_get_left(rbuf); + if (left > 0) { + /* + * This record layer is closing but we still have data left in our + * buffer. It must be destined for the next epoch - so push it there. + */ + ret = BIO_write_ex(rl->next, rbuf->buf + rbuf->offset, left, &written); + } + tls_int_free(rl); + + return ret; +} + + int tls_reset(OSSL_RECORD_LAYER *rl) { memset(rl, 0, sizeof(*rl)); diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index c5f4591391..8ea42d19b9 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -1806,8 +1806,10 @@ int ssl_set_new_record_layer(SSL_CONNECTION *s, int version, meth = ssl_select_next_record_layer(s, level); - if (s->rrlmethod != NULL) - s->rrlmethod->free(s->rrl); + if (s->rrlmethod != NULL && !s->rrlmethod->free(s->rrl)) { + ERR_raise(ERR_LIB_SSL, ERR_R_INTERNAL_ERROR); + goto err; + } if (meth != NULL) s->rrlmethod = meth; @@ -1834,14 +1836,24 @@ int ssl_set_new_record_layer(SSL_CONNECTION *s, int version, for (;;) { int rlret; + BIO *prev = s->rrlnext; + + s->rrlnext = BIO_new(BIO_s_mem()); + + if (s->rrlnext == NULL) { + BIO_free(prev); + SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); + goto err; + } rlret = s->rrlmethod->new_record_layer(sctx->libctx, sctx->propq, version, s->server, direction, level, key, keylen, iv, ivlen, mackey, mackeylen, ciph, taglen, - mactype, md, comp, s->rbio, - NULL, NULL, NULL, options, - &s->rrl, s); + mactype, md, comp, prev, s->rbio, + s->rrlnext, NULL, NULL, NULL, + options, &s->rrl, s); + BIO_free(prev); switch (rlret) { case OSSL_RECORD_RETURN_FATAL: SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_R_RECORD_LAYER_FAILURE); diff --git a/ssl/record/recordmethod.h b/ssl/record/recordmethod.h index 8f025cc785..b86de6518a 100644 --- a/ssl/record/recordmethod.h +++ b/ssl/record/recordmethod.h @@ -158,14 +158,17 @@ struct ossl_record_method_st { int mactype, const EVP_MD *md, const SSL_COMP *comp, - BIO *transport, BIO_ADDR *local, + BIO *prev, + BIO *transport, + BIO *next, + BIO_ADDR *local, BIO_ADDR *peer, const OSSL_PARAM *settings, const OSSL_PARAM *options, OSSL_RECORD_LAYER **ret, /* TODO(RECLAYER): Remove me */ SSL_CONNECTION *s); - void (*free)(OSSL_RECORD_LAYER *rl); + int (*free)(OSSL_RECORD_LAYER *rl); int (*reset)(OSSL_RECORD_LAYER *rl); /* Is this needed? */ diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 9905a188b9..402ae157b5 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -656,6 +656,8 @@ int ossl_ssl_connection_reset(SSL *s) } RECORD_LAYER_clear(&sc->rlayer); + BIO_free(sc->rrlnext); + sc->rrlnext = NULL; /* * TODO(RECLAYER): The record method should probably initialy come from the @@ -1351,6 +1353,10 @@ void ossl_ssl_connection_free(SSL *ssl) if (s == NULL) return; + if (s->rrlmethod != NULL) + s->rrlmethod->free(s->rrl); /* Ignore return value */ + BIO_free(s->rrlnext); + X509_VERIFY_PARAM_free(s->param); dane_final(&s->dane); diff --git a/ssl/ssl_local.h b/ssl/ssl_local.h index 90fb9516ab..b4920a1c12 100644 --- a/ssl/ssl_local.h +++ b/ssl/ssl_local.h @@ -1771,7 +1771,8 @@ struct ssl_connection_st { const OSSL_RECORD_METHOD *rrlmethod; /* The read direction record layer */ OSSL_RECORD_LAYER *rrl; - + /* BIO to store data destined for the next record layer epoch */ + BIO *rrlnext; /* Default password callback. */ pem_password_cb *default_passwd_callback; -- 2.34.1