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 <levitte@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/19424)

diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt
index 653b775330e05512997a1ffbf1b647375b55b61b..9cf7cb881bf8825934324ef32a01179ac52f3605 100644 (file)
--- 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 98bddbb857f0269d51d742ed459d9cff44b1f33b..080aa2415047b6cb85760f19df52cfa7614ff256 100644 (file)
--- 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 d219d7d02e0199bb37c73aece5fb577cd27b3a3b..d810ed7a28ff7ab14db6f30171b04b3fb1a62c50 100644 (file)
--- 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 2ee6c2e7531f9af2e7883e90ea204e7ec125ed90..e1267500cff369177d6ff52c577064f59845e160 100644 (file)
--- 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 90cf5542c3c1f5e6ffa9027e3c027a2f9d8f3d49..18827de9a08522b6086b1c02654ac8364f2a3086 100644 (file)
--- 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 ad22f11bf1d42d820c1ec177142cd5f8ba8f1d56..5cd40a4fe2fc27198eaaf294330b64d642162e0c 100644 (file)
--- 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 166ee548eb910c7e344cf686ab8584bafc734058..6917fd897b2262940026b873883bbc9e7ea43e0c 100644 (file)
--- 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 fe2620a8ea2a8acd0d8577e7cc97bf78f99cf6d9..40016c121ddb53ea41e1d84298affb784f73674a 100644 (file)
--- 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 04f130bc2eba140185c0c5205945f124bda295bd..5c0168aa4331aa47a0e6f155ba16bf3c2a99ae85 100644 (file)
--- 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 c49afbd2c60462868d836eb030eefe5d6759cb21..04bf2ef6a0824f3e806951939abd59f177224e85 100644 (file)
--- 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 1a4f441a9f09f8f7d2e01cd0665cbd97c9be83f9..7345a3f5e2571191ab94b525b9b83422f00d63e5 100644 (file)
--- 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"},