Call init and finalization functions per extension message

Previously, init and finalization function for extensions are called
per extension block, rather than per message.  This commit changes
that behaviour, and now they are called per message.  The parse
function is still called per extension block.

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3244)

diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index c8ed722ab157144b187091c16ac972041b0be50e..54075ae0e69bda34e69dab69d369799a05306637 100644 (file)
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -421,8 +421,9 @@ int extension_is_relevant(SSL *s, unsigned int extctx, unsigned int thisctx)
  * stored in |*res| on success. In the event of an error the alert type to use
  * is stored in |*al|. We don't actually process the content of the extensions
  * yet, except to check their types. This function also runs the initialiser
- * functions for all known extensions (whether we have collected them or not).
- * If successful the caller is responsible for freeing the contents of |*res|.
+ * functions for all known extensions if |init| is nonzero (whether we have
+ * collected them or not). If successful the caller is responsible for freeing
+ * the contents of |*res|.
  *
  * Per http://tools.ietf.org/html/rfc5246#section-7.4.1.4, there may not be
  * more than one extension of the same type in a ClientHello or ServerHello.
@@ -432,7 +433,8 @@ int extension_is_relevant(SSL *s, unsigned int extctx, unsigned int thisctx)
  * extensions that we know about. We ignore others.
  */
 int tls_collect_extensions(SSL *s, PACKET *packet, unsigned int context,
-                           RAW_EXTENSION **res, int *al, size_t *len)
+                           RAW_EXTENSION **res, int *al, size_t *len,
+                           int init)
 {
     PACKET extensions = *packet;
     size_t i = 0;
@@ -490,16 +492,19 @@ int tls_collect_extensions(SSL *s, PACKET *packet, unsigned int context,
         }
     }
 
-    /*
-     * Initialise all known extensions relevant to this context, whether we have
-     * found them or not
-     */
-    for (thisexd = ext_defs, i = 0; i < OSSL_NELEM(ext_defs); i++, thisexd++) {
-        if(thisexd->init != NULL && (thisexd->context & context) != 0
-                && extension_is_relevant(s, thisexd->context, context)
-                && !thisexd->init(s, context)) {
-            *al = SSL_AD_INTERNAL_ERROR;
-            goto err;
+    if (init) {
+        /*
+         * Initialise all known extensions relevant to this context,
+         * whether we have found them or not
+         */
+        for (thisexd = ext_defs, i = 0; i < OSSL_NELEM(ext_defs);
+             i++, thisexd++) {
+            if (thisexd->init != NULL && (thisexd->context & context) != 0 &&
+                extension_is_relevant(s, thisexd->context, context) &&
+                !thisexd->init(s, context)) {
+                *al = SSL_AD_INTERNAL_ERROR;
+                goto err;
+            }
         }
     }
 
@@ -578,14 +583,14 @@ int tls_parse_extension(SSL *s, TLSEXT_INDEX idx, int context,
 
 /*
  * Parse all remaining extensions that have not yet been parsed. Also calls the
- * finalisation for all extensions at the end, whether we collected them or not.
- * Returns 1 for success or 0 for failure. If we are working on a Certificate
- * message then we also pass the Certificate |x| and its position in the
- * |chainidx|, with 0 being the first certificate. On failure, |*al| is
- * populated with a suitable alert code.
+ * finalisation for all extensions at the end if |fin| is nonzero, whether we
+ * collected them or not. Returns 1 for success or 0 for failure. If we are
+ * working on a Certificate message then we also pass the Certificate |x| and
+ * its position in the |chainidx|, with 0 being the first certificate. On
+ * failure, |*al| is populated with a suitable alert code.
  */
 int tls_parse_all_extensions(SSL *s, int context, RAW_EXTENSION *exts, X509 *x,
-                             size_t chainidx, int *al)
+                             size_t chainidx, int *al, int fin)
 {
     size_t i, numexts = OSSL_NELEM(ext_defs);
     const EXTENSION_DEFINITION *thisexd;
@@ -599,15 +604,17 @@ int tls_parse_all_extensions(SSL *s, int context, RAW_EXTENSION *exts, X509 *x,
             return 0;
     }
 
-    /*
-     * Finalise all known extensions relevant to this context, whether we have
-     * found them or not
-     */
-    for (i = 0, thisexd = ext_defs; i < OSSL_NELEM(ext_defs); i++, thisexd++) {
-        if(thisexd->final != NULL
-                && (thisexd->context & context) != 0
-                && !thisexd->final(s, context, exts[i].present, al))
-            return 0;
+    if (fin) {
+        /*
+         * Finalise all known extensions relevant to this context,
+         * whether we have found them or not
+         */
+        for (i = 0, thisexd = ext_defs; i < OSSL_NELEM(ext_defs);
+             i++, thisexd++) {
+            if (thisexd->final != NULL && (thisexd->context & context) != 0 &&
+                !thisexd->final(s, context, exts[i].present, al))
+                return 0;
+        }
     }
 
     return 1;

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index ab77ba05e9ff4416e56f36f3a3c44bb1a84131ee..0b4931d6d0812d9644258851e0218f8d78206d40 100644 (file)
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -1373,7 +1373,7 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt)
 
     context = SSL_IS_TLS13(s) ? SSL_EXT_TLS1_3_SERVER_HELLO
                               : SSL_EXT_TLS1_2_SERVER_HELLO;
-    if (!tls_collect_extensions(s, &extpkt, context, &extensions, &al, NULL))
+    if (!tls_collect_extensions(s, &extpkt, context, &extensions, &al, NULL, 1))
         goto f_err;
 
     s->hit = 0;
@@ -1525,7 +1525,7 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt)
     }
 #endif
 
-    if (!tls_parse_all_extensions(s, context, extensions, NULL, 0, &al))
+    if (!tls_parse_all_extensions(s, context, extensions, NULL, 0, &al, 1))
         goto f_err;
 
 #ifndef OPENSSL_NO_SCTP
@@ -1616,9 +1616,9 @@ static MSG_PROCESS_RETURN tls_process_hello_retry_request(SSL *s, PACKET *pkt)
     }
 
     if (!tls_collect_extensions(s, &extpkt, SSL_EXT_TLS1_3_HELLO_RETRY_REQUEST,
-                                &extensions, &al, NULL)
+                                &extensions, &al, NULL, 1)
             || !tls_parse_all_extensions(s, SSL_EXT_TLS1_3_HELLO_RETRY_REQUEST,
-                                         extensions, NULL, 0, &al))
+                                         extensions, NULL, 0, &al, 1))
         goto f_err;
 
     OPENSSL_free(extensions);
@@ -1711,9 +1711,10 @@ MSG_PROCESS_RETURN tls_process_server_certificate(SSL *s, PACKET *pkt)
             }
             if (!tls_collect_extensions(s, &extensions,
                                         SSL_EXT_TLS1_3_CERTIFICATE, &rawexts,
-                                        &al, NULL)
+                                        &al, NULL, chainidx == 0)
                     || !tls_parse_all_extensions(s, SSL_EXT_TLS1_3_CERTIFICATE,
-                                                 rawexts, x, chainidx, &al)) {
+                                                 rawexts, x, chainidx, &al,
+                                                 !PACKET_remaining(pkt))) {
                 OPENSSL_free(rawexts);
                 goto f_err;
             }
@@ -2340,9 +2341,9 @@ MSG_PROCESS_RETURN tls_process_certificate_request(SSL *s, PACKET *pkt)
         }
         if (!tls_collect_extensions(s, &extensions,
                                     SSL_EXT_TLS1_3_CERTIFICATE_REQUEST,
-                                    &rawexts, &al, NULL)
+                                    &rawexts, &al, NULL, 1)
             || !tls_parse_all_extensions(s, SSL_EXT_TLS1_3_CERTIFICATE_REQUEST,
-                                         rawexts, NULL, 0, &al)) {
+                                         rawexts, NULL, 0, &al, 1)) {
             OPENSSL_free(rawexts);
             goto err;
         }
@@ -2501,10 +2502,10 @@ MSG_PROCESS_RETURN tls_process_new_session_ticket(SSL *s, PACKET *pkt)
         if (!PACKET_as_length_prefixed_2(pkt, &extpkt)
                 || !tls_collect_extensions(s, &extpkt,
                                            SSL_EXT_TLS1_3_NEW_SESSION_TICKET,
-                                           &exts, &al, NULL)
+                                           &exts, &al, NULL, 1)
                 || !tls_parse_all_extensions(s,
                                              SSL_EXT_TLS1_3_NEW_SESSION_TICKET,
-                                             exts, NULL, 0, &al)) {
+                                             exts, NULL, 0, &al, 1)) {
             SSLerr(SSL_F_TLS_PROCESS_NEW_SESSION_TICKET, SSL_R_BAD_EXTENSION);
             goto f_err;
         }
@@ -3464,9 +3465,9 @@ static MSG_PROCESS_RETURN tls_process_encrypted_extensions(SSL *s, PACKET *pkt)
 
     if (!tls_collect_extensions(s, &extensions,
                                 SSL_EXT_TLS1_3_ENCRYPTED_EXTENSIONS, &rawexts,
-                                &al, NULL)
+                                &al, NULL, 1)
             || !tls_parse_all_extensions(s, SSL_EXT_TLS1_3_ENCRYPTED_EXTENSIONS,
-                                         rawexts, NULL, 0, &al))
+                                         rawexts, NULL, 0, &al, 1))
         goto err;
 
     OPENSSL_free(rawexts);

diff --git a/ssl/statem/statem_locl.h b/ssl/statem/statem_locl.h
index 2352c6a11ea76585eade8364cb2ef8cd0ebe166e..3b9311e02f5b1e7065c7c51222c947a9b95f3f24 100644 (file)
--- a/ssl/statem/statem_locl.h
+++ b/ssl/statem/statem_locl.h
@@ -156,12 +156,13 @@ MSG_PROCESS_RETURN tls_process_end_of_early_data(SSL *s, PACKET *pkt);
 __owur int extension_is_relevant(SSL *s, unsigned int extctx,
                                  unsigned int thisctx);
 __owur int tls_collect_extensions(SSL *s, PACKET *packet, unsigned int context,
-                                  RAW_EXTENSION **res, int *al, size_t *len);
+                                  RAW_EXTENSION **res, int *al, size_t *len,
+                                  int init);
 __owur int tls_parse_extension(SSL *s, TLSEXT_INDEX idx, int context,
                                RAW_EXTENSION *exts,  X509 *x, size_t chainidx,
                                int *al);
 __owur int tls_parse_all_extensions(SSL *s, int context, RAW_EXTENSION *exts,
-                                    X509 *x, size_t chainidx, int *al);
+                                    X509 *x, size_t chainidx, int *al, int fin);
 __owur int should_add_extension(SSL *s, unsigned int extctx,
                                 unsigned int thisctx, int max_version);
 __owur int tls_construct_extensions(SSL *s, WPACKET *pkt, unsigned int context,

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index d751502aba726fa2647c637d7c6e5f52ae8bc1c8..f6ecbf700654643553600d27243882cda95919de 100644 (file)
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -1426,7 +1426,7 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt)
     extensions = clienthello->extensions;
     if (!tls_collect_extensions(s, &extensions, SSL_EXT_CLIENT_HELLO,
                                 &clienthello->pre_proc_exts, &al,
-                                &clienthello->pre_proc_exts_len)) {
+                                &clienthello->pre_proc_exts_len, 1)) {
         /* SSLerr already been called */
         goto f_err;
     }
@@ -1690,7 +1690,7 @@ static int tls_early_post_process_client_hello(SSL *s, int *pal)
 
     /* TLS extensions */
     if (!tls_parse_all_extensions(s, SSL_EXT_CLIENT_HELLO,
-                                  clienthello->pre_proc_exts, NULL, 0, &al)) {
+                                  clienthello->pre_proc_exts, NULL, 0, &al, 1)) {
         SSLerr(SSL_F_TLS_EARLY_POST_PROCESS_CLIENT_HELLO, SSL_R_PARSE_TLSEXT);
         goto err;
     }
@@ -3217,9 +3217,10 @@ MSG_PROCESS_RETURN tls_process_client_certificate(SSL *s, PACKET *pkt)
             }
             if (!tls_collect_extensions(s, &extensions,
                                         SSL_EXT_TLS1_3_CERTIFICATE, &rawexts,
-                                        &al, NULL)
+                                        &al, NULL, chainidx == 0)
                     || !tls_parse_all_extensions(s, SSL_EXT_TLS1_3_CERTIFICATE,
-                                                 rawexts, x, chainidx, &al)) {
+                                                 rawexts, x, chainidx, &al,
+                                                 !PACKET_remaining(&spkt))) {
                 OPENSSL_free(rawexts);
                 goto f_err;
             }