From bed07b187506ded20ef39dcbed56dc323ae44ff4 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Mon, 17 Oct 2022 12:28:07 +0100 Subject: [PATCH] Consolidate sequence counter incrementing code The sequence counter was incremented in numerous different ways in numerous different locations. We introduce a single function to do this inside the record layer. Reviewed-by: Richard Levitte Reviewed-by: Tomas Mraz Reviewed-by: Hugo Landau (Merged from https://github.com/openssl/openssl/pull/19424) --- crypto/err/openssl.txt | 1 + include/openssl/sslerr.h | 1 + ssl/record/methods/dtls_meth.c | 6 ++++-- ssl/record/methods/recmethod_local.h | 2 ++ ssl/record/methods/ssl3_meth.c | 4 +++- ssl/record/methods/tls13_meth.c | 10 ++-------- ssl/record/methods/tls1_meth.c | 26 +++++++++++--------------- ssl/record/methods/tls_common.c | 22 +++++++++++++++++++--- ssl/record/rec_layer_s3.c | 11 ----------- ssl/record/record_local.h | 1 - ssl/ssl_err.c | 2 ++ 11 files changed, 45 insertions(+), 41 deletions(-) diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt index 653b775330..9cf7cb881b 100644 --- a/crypto/err/openssl.txt +++ b/crypto/err/openssl.txt @@ -1490,6 +1490,7 @@ SSL_R_REQUIRED_COMPRESSION_ALGORITHM_MISSING:342:\ required compression algorithm missing SSL_R_SCSV_RECEIVED_WHEN_RENEGOTIATING:345:scsv received when renegotiating SSL_R_SCT_VERIFICATION_FAILED:208:sct verification failed +SSL_R_SEQUENCE_CTR_WRAPPED:326:sequence ctr wrapped SSL_R_SERVERHELLO_TLSEXT:275:serverhello tlsext SSL_R_SESSION_ID_CONTEXT_UNINITIALIZED:277:session id context uninitialized SSL_R_SHUTDOWN_WHILE_IN_INIT:407:shutdown while in init diff --git a/include/openssl/sslerr.h b/include/openssl/sslerr.h index 98bddbb857..080aa24150 100644 --- a/include/openssl/sslerr.h +++ b/include/openssl/sslerr.h @@ -239,6 +239,7 @@ # define SSL_R_REQUIRED_COMPRESSION_ALGORITHM_MISSING 342 # define SSL_R_SCSV_RECEIVED_WHEN_RENEGOTIATING 345 # define SSL_R_SCT_VERIFICATION_FAILED 208 +# define SSL_R_SEQUENCE_CTR_WRAPPED 326 # define SSL_R_SERVERHELLO_TLSEXT 275 # define SSL_R_SESSION_ID_CONTEXT_UNINITIALIZED 277 # define SSL_R_SHUTDOWN_WHILE_IN_INIT 407 diff --git a/ssl/record/methods/dtls_meth.c b/ssl/record/methods/dtls_meth.c index d219d7d02e..d810ed7a28 100644 --- a/ssl/record/methods/dtls_meth.c +++ b/ssl/record/methods/dtls_meth.c @@ -810,8 +810,10 @@ int dtls_write_records(OSSL_RECORD_LAYER *rl, OSSL_RECORD_TEMPLATE *templates, goto err; } - /* TODO(RECLAYER): FIXME */ - ssl3_record_sequence_update(rl->sequence); + if (!tls_increment_sequence_ctr(rl)) { + /* RLAYERfatal() already called */ + goto err; + } /* now let's set up wb */ SSL3_BUFFER_set_left(wb, SSL3_RECORD_get_length(&wr)); diff --git a/ssl/record/methods/recmethod_local.h b/ssl/record/methods/recmethod_local.h index 2ee6c2e753..e1267500cf 100644 --- a/ssl/record/methods/recmethod_local.h +++ b/ssl/record/methods/recmethod_local.h @@ -344,6 +344,8 @@ __owur int ssl3_cbc_digest_record(const EVP_MD *md, const unsigned char *mac_secret, size_t mac_secret_length, char is_sslv3); +int tls_increment_sequence_ctr(OSSL_RECORD_LAYER *rl); + int tls_default_read_n(OSSL_RECORD_LAYER *rl, size_t n, size_t max, int extend, int clearold, size_t *readbytes); int tls_get_more_records(OSSL_RECORD_LAYER *rl); diff --git a/ssl/record/methods/ssl3_meth.c b/ssl/record/methods/ssl3_meth.c index 90cf5542c3..18827de9a0 100644 --- a/ssl/record/methods/ssl3_meth.c +++ b/ssl/record/methods/ssl3_meth.c @@ -296,7 +296,9 @@ static int ssl3_mac(OSSL_RECORD_LAYER *rl, SSL3_RECORD *rec, unsigned char *md, EVP_MD_CTX_free(md_ctx); } - ssl3_record_sequence_update(seq); + if (!tls_increment_sequence_ctr(rl)) + return 0; + return 1; } diff --git a/ssl/record/methods/tls13_meth.c b/ssl/record/methods/tls13_meth.c index ad22f11bf1..5cd40a4fe2 100644 --- a/ssl/record/methods/tls13_meth.c +++ b/ssl/record/methods/tls13_meth.c @@ -122,14 +122,8 @@ static int tls13_cipher(OSSL_RECORD_LAYER *rl, SSL3_RECORD *recs, size_t n_recs, for (loop = 0; loop < SEQ_NUM_SIZE; loop++) iv[offset + loop] = staticiv[offset + loop] ^ seq[loop]; - /* Increment the sequence counter */ - for (loop = SEQ_NUM_SIZE; loop > 0; loop--) { - ++seq[loop - 1]; - if (seq[loop - 1] != 0) - break; - } - if (loop == 0) { - /* Sequence has wrapped */ + if (!tls_increment_sequence_ctr(rl)) { + /* RLAYERfatal already called */ return 0; } diff --git a/ssl/record/methods/tls1_meth.c b/ssl/record/methods/tls1_meth.c index 166ee548eb..6917fd897b 100644 --- a/ssl/record/methods/tls1_meth.c +++ b/ssl/record/methods/tls1_meth.c @@ -162,7 +162,7 @@ static int tls1_cipher(OSSL_RECORD_LAYER *rl, SSL3_RECORD *recs, size_t n_recs, EVP_CIPHER_CTX *ds; size_t reclen[SSL_MAX_PIPELINES]; unsigned char buf[SSL_MAX_PIPELINES][EVP_AEAD_TLS1_AAD_LEN]; - int i, pad = 0, tmpr, provided; + int pad = 0, tmpr, provided; size_t bs, ctr, padnum, loop; unsigned char padval; const EVP_CIPHER *enc; @@ -247,10 +247,9 @@ static int tls1_cipher(OSSL_RECORD_LAYER *rl, SSL3_RECORD *recs, size_t n_recs, memcpy(buf[ctr], dtlsseq, 8); } else { memcpy(buf[ctr], seq, 8); - for (i = 7; i >= 0; i--) { /* increment */ - ++seq[i]; - if (seq[i] != 0) - break; + if (!tls_increment_sequence_ctr(rl)) { + /* RLAYERfatal already called */ + return 0; } } @@ -324,7 +323,6 @@ static int tls1_cipher(OSSL_RECORD_LAYER *rl, SSL3_RECORD *recs, size_t n_recs, } if (!rl->isdtls && rl->tlstree) { - unsigned char *seq; int decrement_seq = 0; /* @@ -335,8 +333,9 @@ static int tls1_cipher(OSSL_RECORD_LAYER *rl, SSL3_RECORD *recs, size_t n_recs, if (sending && !rl->use_etm) decrement_seq = 1; - seq = rl->sequence; - if (EVP_CIPHER_CTX_ctrl(ds, EVP_CTRL_TLSTREE, decrement_seq, seq) <= 0) { + if (EVP_CIPHER_CTX_ctrl(ds, EVP_CTRL_TLSTREE, decrement_seq, + rl->sequence) <= 0) { + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR); return 0; } @@ -455,7 +454,6 @@ static int tls1_mac(OSSL_RECORD_LAYER *rl, SSL3_RECORD *rec, unsigned char *md, unsigned char *seq = rl->sequence; EVP_MD_CTX *hash; size_t md_size; - int i; EVP_MD_CTX *hmac = NULL, *mac_ctx; unsigned char header[13]; int t; @@ -526,13 +524,11 @@ static int tls1_mac(OSSL_RECORD_LAYER *rl, SSL3_RECORD *rec, unsigned char *md, BIO_dump_indent(trc_out, rec->data, rec->length, 4); } OSSL_TRACE_END(TLS); - if (!rl->isdtls) { - for (i = 7; i >= 0; i--) { - ++seq[i]; - if (seq[i] != 0) - break; - } + if (!rl->isdtls && !tls_increment_sequence_ctr(rl)) { + /* RLAYERfatal already called */ + goto end; } + OSSL_TRACE_BEGIN(TLS) { BIO_printf(trc_out, "md:\n"); BIO_dump_indent(trc_out, md, md_size, 4); diff --git a/ssl/record/methods/tls_common.c b/ssl/record/methods/tls_common.c index fe2620a8ea..40016c121d 100644 --- a/ssl/record/methods/tls_common.c +++ b/ssl/record/methods/tls_common.c @@ -247,6 +247,24 @@ static int tls_release_read_buffer(OSSL_RECORD_LAYER *rl) return 1; } +int tls_increment_sequence_ctr(OSSL_RECORD_LAYER *rl) +{ + int i; + + /* Increment the sequence counter */ + for (i = SEQ_NUM_SIZE; i > 0; i--) { + ++(rl->sequence[i - 1]); + if (rl->sequence[i - 1] != 0) + break; + } + if (i == 0) { + /* Sequence has wrapped */ + RLAYERfatal(rl, SSL_AD_INTERNAL_ERROR, SSL_R_SEQUENCE_CTR_WRAPPED); + return 0; + } + return 1; +} + /* * Return values are as per SSL_read() */ @@ -753,7 +771,7 @@ int tls_get_more_records(OSSL_RECORD_LAYER *rl) * If in encrypt-then-mac mode calculate mac from encrypted record. All * the details below are public so no timing details can leak. */ - if (rl->use_etm && rl->md_ctx) { + if (rl->use_etm && rl->md_ctx != NULL) { unsigned char *mac; for (j = 0; j < num_recs; j++) { @@ -838,8 +856,6 @@ int tls_get_more_records(OSSL_RECORD_LAYER *rl) if (rl->enc_ctx != NULL && !rl->use_etm && EVP_MD_CTX_get0_md(rl->md_ctx) != NULL) { - /* rl->md_ctx != NULL => mac_size != -1 */ - for (j = 0; j < num_recs; j++) { SSL_MAC_BUF *thismb = &macbufs[j]; diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index 04f130bc2e..5c0168aa43 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -999,17 +999,6 @@ int ssl3_read_bytes(SSL *ssl, int type, int *recvd_type, unsigned char *buf, } } -void ssl3_record_sequence_update(unsigned char *seq) -{ - int i; - - for (i = 7; i >= 0; i--) { - ++seq[i]; - if (seq[i] != 0) - break; - } -} - /* * Returns true if the current rrec was sent in SSLv2 backwards compatible * format and false otherwise. diff --git a/ssl/record/record_local.h b/ssl/record/record_local.h index c49afbd2c6..04bf2ef6a0 100644 --- a/ssl/record/record_local.h +++ b/ssl/record/record_local.h @@ -26,7 +26,6 @@ #define DTLS_RECORD_LAYER_get_r_epoch(rl) ((rl)->d->r_epoch) int dtls_buffer_record(SSL_CONNECTION *s, TLS_RECORD *rec); -void ssl3_record_sequence_update(unsigned char *seq); /* Macros/functions provided by the SSL3_BUFFER component */ diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index 1a4f441a9f..7345a3f5e2 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -372,6 +372,8 @@ static const ERR_STRING_DATA SSL_str_reasons[] = { "scsv received when renegotiating"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_SCT_VERIFICATION_FAILED), "sct verification failed"}, + {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_SEQUENCE_CTR_WRAPPED), + "sequence ctr wrapped"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_SERVERHELLO_TLSEXT), "serverhello tlsext"}, {ERR_PACK(ERR_LIB_SSL, 0, SSL_R_SESSION_ID_CONTEXT_UNINITIALIZED), "session id context uninitialized"}, -- 2.39.2