Add `for_comp` flag when retrieving certs for compression

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Hugo Landau <hlandau@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/18186)

diff --git a/include/openssl/ssl.h.in b/include/openssl/ssl.h.in
index 4ae96cd1ee1dc090bd429111c1e613459d75293a..2224b3269b60c39953e56dfc17956fdb6fcf0201 100644 (file)
--- a/include/openssl/ssl.h.in
+++ b/include/openssl/ssl.h.in
@@ -280,6 +280,7 @@ typedef int (*tls_session_secret_cb_fn)(SSL *s, void *secret, int *secret_len,
 #define SSL_EXT_TLS1_3_CERTIFICATE              0x1000
 #define SSL_EXT_TLS1_3_NEW_SESSION_TICKET       0x2000
 #define SSL_EXT_TLS1_3_CERTIFICATE_REQUEST      0x4000
+#define SSL_EXT_TLS1_3_CERTIFICATE_COMPRESSION  0x8000
 
 /* Typedefs for handling custom extensions */
 

diff --git a/ssl/ssl_cert_comp.c b/ssl/ssl_cert_comp.c
index a86282279c115c1a4c6f7c93ba9e8934e642f900..654da2dc03cf9758277c9a2433db8760ab9f5ebe 100644 (file)
--- a/ssl/ssl_cert_comp.c
+++ b/ssl/ssl_cert_comp.c
@@ -198,7 +198,6 @@ static size_t ssl_get_cert_to_compress(SSL *ssl, CERT_PKEY *cpk, unsigned char *
     WPACKET tmppkt;
     BUF_MEM buf = { 0 };
     size_t ret = 0;
-    const SSL_METHOD *method = NULL;
 
     if (sc == NULL
             || cpk == NULL
@@ -215,26 +214,14 @@ static size_t ssl_get_cert_to_compress(SSL *ssl, CERT_PKEY *cpk, unsigned char *
         goto out;
 
     /*
-     * ssl3_output_cert_chain() may generate an SSLfata() error,
-     * for this case, we want to ignore it
+     * ssl3_output_cert_chain() may generate an SSLfatal() error,
+     * for this case, we want to ignore it, argument for_comp = 1
      */
-    sc->statem.ignore_fatal = 1;
-    ERR_set_mark();
-    /* Must get the certificate as TLSv1.3, restore before returning */
-    method = SSL_get_ssl_method(ssl);
-    if (!SSL_set_ssl_method(ssl, tlsv1_3_server_method()))
-        goto out;
-
-    if (!ssl3_output_cert_chain(sc, &tmppkt, cpk))
+    if (!ssl3_output_cert_chain(sc, &tmppkt, cpk, 1))
         goto out;
     WPACKET_get_total_written(&tmppkt, &ret);
 
  out:
-    /* Don't leave errors in the queue */
-    ERR_pop_to_mark();
-    sc->statem.ignore_fatal = 0;
-    if (method != NULL && !SSL_set_ssl_method(ssl, method))
-        ret = 0;
     WPACKET_cleanup(&tmppkt);
     if (ret != 0 && data != NULL)
         *data = (unsigned char *)buf.data;

diff --git a/ssl/ssl_local.h b/ssl/ssl_local.h
index 2580064ebdd5ecaa94ffecd025440356cd3637ef..fd21d0be82ec3d62ed221a3db7d09bafaa02305f 100644 (file)
--- a/ssl/ssl_local.h
+++ b/ssl/ssl_local.h
@@ -2613,7 +2613,7 @@ __owur int ssl3_finish_mac(SSL_CONNECTION *s, const unsigned char *buf,
                            size_t len);
 void ssl3_free_digest_list(SSL_CONNECTION *s);
 __owur unsigned long ssl3_output_cert_chain(SSL_CONNECTION *s, WPACKET *pkt,
-                                            CERT_PKEY *cpk);
+                                            CERT_PKEY *cpk, int for_comp);
 __owur const SSL_CIPHER *ssl3_choose_cipher(SSL_CONNECTION *s,
                                             STACK_OF(SSL_CIPHER) *clnt,
                                             STACK_OF(SSL_CIPHER) *srvr);

diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index 2cfc9f2b7ded651d68d8a0e08f9f37410821df42..b879050c40ac2a36c602757f43e9aaed941ceba2 100644 (file)
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -832,6 +832,7 @@ int tls_construct_extensions(SSL_CONNECTION *s, WPACKET *pkt,
     size_t i;
     int min_version, max_version = 0, reason;
     const EXTENSION_DEFINITION *thisexd;
+    int for_comp = (context & SSL_EXT_TLS1_3_CERTIFICATE_COMPRESSION) != 0;
 
     if (!WPACKET_start_sub_packet_u16(pkt)
                /*
@@ -842,15 +843,17 @@ int tls_construct_extensions(SSL_CONNECTION *s, WPACKET *pkt,
             || ((context &
                  (SSL_EXT_CLIENT_HELLO | SSL_EXT_TLS1_2_SERVER_HELLO)) != 0
                 && !WPACKET_set_flags(pkt,
-                                     WPACKET_FLAGS_ABANDON_ON_ZERO_LENGTH))) {
-        SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
+                                      WPACKET_FLAGS_ABANDON_ON_ZERO_LENGTH))) {
+        if (!for_comp)
+            SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
         return 0;
     }
 
     if ((context & SSL_EXT_CLIENT_HELLO) != 0) {
         reason = ssl_get_min_max_version(s, &min_version, &max_version, NULL);
         if (reason != 0) {
-            SSLfatal(s, SSL_AD_INTERNAL_ERROR, reason);
+            if (!for_comp)
+                SSLfatal(s, SSL_AD_INTERNAL_ERROR, reason);
             return 0;
         }
     }
@@ -894,7 +897,8 @@ int tls_construct_extensions(SSL_CONNECTION *s, WPACKET *pkt,
     }
 
     if (!WPACKET_close(pkt)) {
-        SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
+        if (!for_comp)
+            SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
         return 0;
     }
 

diff --git a/ssl/statem/extensions_cust.c b/ssl/statem/extensions_cust.c
index 57713702f7f487d10fc039fdcabf13a593d087d6..ebfe7d16ee87cc5ed682a275be0ac9d39750c90e 100644 (file)
--- a/ssl/statem/extensions_cust.c
+++ b/ssl/statem/extensions_cust.c
@@ -178,6 +178,7 @@ int custom_ext_add(SSL_CONNECTION *s, int context, WPACKET *pkt, X509 *x,
     custom_ext_method *meth;
     size_t i;
     int al;
+    int for_comp = (context & SSL_EXT_TLS1_3_CERTIFICATE_COMPRESSION) != 0;
 
     for (i = 0; i < exts->meths_count; i++) {
         const unsigned char *out = NULL;
@@ -211,7 +212,8 @@ int custom_ext_add(SSL_CONNECTION *s, int context, WPACKET *pkt, X509 *x,
                                          meth->add_arg);
 
             if (cb_retval < 0) {
-                SSLfatal(s, al, SSL_R_CALLBACK_FAILED);
+                if (!for_comp)
+                    SSLfatal(s, al, SSL_R_CALLBACK_FAILED);
                 return 0;       /* error */
             }
             if (cb_retval == 0)
@@ -222,7 +224,8 @@ int custom_ext_add(SSL_CONNECTION *s, int context, WPACKET *pkt, X509 *x,
                 || !WPACKET_start_sub_packet_u16(pkt)
                 || (outlen > 0 && !WPACKET_memcpy(pkt, out, outlen))
                 || !WPACKET_close(pkt)) {
-            SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
+            if (!for_comp)
+                SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
             return 0;
         }
         if ((context & SSL_EXT_CLIENT_HELLO) != 0) {
@@ -230,7 +233,8 @@ int custom_ext_add(SSL_CONNECTION *s, int context, WPACKET *pkt, X509 *x,
              * We can't send duplicates: code logic should prevent this.
              */
             if (!ossl_assert((meth->ext_flags & SSL_EXT_FLAG_SENT) == 0)) {
-                SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
+                if (!for_comp)
+                    SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
                 return 0;
             }
             /*

diff --git a/ssl/statem/statem.c b/ssl/statem/statem.c
index ba3681285a3527aecff800df9546f614d5e7148e..448d655a17cf772b67b990e986d7d62a2d68daeb 100644 (file)
--- a/ssl/statem/statem.c
+++ b/ssl/statem/statem.c
@@ -130,7 +130,6 @@ void ossl_statem_clear(SSL_CONNECTION *s)
     s->statem.hand_state = TLS_ST_BEFORE;
     ossl_statem_set_in_init(s, 1);
     s->statem.no_cert_verify = 0;
-    s->statem.ignore_fatal = 0;
 }
 
 /*
@@ -144,15 +143,6 @@ void ossl_statem_set_renegotiate(SSL_CONNECTION *s)
 
 void ossl_statem_send_fatal(SSL_CONNECTION *s, int al)
 {
-    /*
-     * Some public APIs may call internal functions that fatal error,
-     * which doesn't make sense outside the state machine. Those APIs
-     * that can handle a failure set this flag to avoid errors sending
-     * alerts. Example: getting a wire-formatted certificate for
-     * compression.
-     */
-    if (s->statem.ignore_fatal)
-        return;
     /* We shouldn't call SSLfatal() twice. Once is enough */
     if (s->statem.in_init && s->statem.state == MSG_FLOW_ERROR)
       return;

diff --git a/ssl/statem/statem.h b/ssl/statem/statem.h
index b60103e6e511f948542230dcb294bdb4c19633c1..2b73eba6f68f598065765539a54746d4e22a01c1 100644 (file)
--- a/ssl/statem/statem.h
+++ b/ssl/statem/statem.h
@@ -96,7 +96,6 @@ struct ossl_statem_st {
     OSSL_HANDSHAKE_STATE request_state;
     int in_init;
     int read_state_first_init;
-    int ignore_fatal;
     /* true when we are actually in SSL_accept() or SSL_connect() */
     int in_handshake;
     /*

diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 303e87728259e106d39e41045b7b6447e7c38bd0..b3f4300434d88e3d38c488ef06460c8729b61989 100644 (file)
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -3628,7 +3628,7 @@ CON_FUNC_RETURN tls_construct_client_certificate(SSL_CONNECTION *s,
     }
     if (!ssl3_output_cert_chain(s, pkt,
                                 (s->s3.tmp.cert_req == 2) ? NULL
-                                                           : s->cert->key)) {
+                                                           : s->cert->key, 0)) {
         /* SSLfatal() already called */
         return CON_FUNC_ERROR;
     }
@@ -3676,7 +3676,7 @@ CON_FUNC_RETURN tls_construct_client_compressed_certificate(SSL_CONNECTION *sc,
     } else if (!WPACKET_sub_memcpy_u8(&tmppkt, sc->pha_context, sc->pha_context_len))
         goto err;
 
-    if (!ssl3_output_cert_chain(sc, &tmppkt, sc->cert->key)) {
+    if (!ssl3_output_cert_chain(sc, &tmppkt, sc->cert->key, 0)) {
         /* SSLfatal() already called */
         goto out;
     }

diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index 9712fb37355e4b219293ca81501269d1594bc66e..b37633e37f05e2267d9a4825a76341c33d33f8d9 100644 (file)
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -906,25 +906,30 @@ CON_FUNC_RETURN tls_construct_change_cipher_spec(SSL_CONNECTION *s, WPACKET *pkt
 
 /* Add a certificate to the WPACKET */
 static int ssl_add_cert_to_wpacket(SSL_CONNECTION *s, WPACKET *pkt,
-                                   X509 *x, int chain)
+                                   X509 *x, int chain, int for_comp)
 {
     int len;
     unsigned char *outbytes;
+    int context = SSL_EXT_TLS1_3_CERTIFICATE;
+
+    if (for_comp)
+        context |= SSL_EXT_TLS1_3_CERTIFICATE_COMPRESSION;
 
     len = i2d_X509(x, NULL);
     if (len < 0) {
-        SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_BUF_LIB);
+        if (!for_comp)
+            SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_BUF_LIB);
         return 0;
     }
     if (!WPACKET_sub_allocate_bytes_u24(pkt, len, &outbytes)
             || i2d_X509(x, &outbytes) != len) {
-        SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
+        if (!for_comp)
+            SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
         return 0;
     }
 
-    if (SSL_CONNECTION_IS_TLS13(s)
-            && !tls_construct_extensions(s, pkt, SSL_EXT_TLS1_3_CERTIFICATE, x,
-                                         chain)) {
+    if ((SSL_CONNECTION_IS_TLS13(s) || for_comp)
+            && !tls_construct_extensions(s, pkt, context, x, chain)) {
         /* SSLfatal() already called */
         return 0;
     }
@@ -933,7 +938,7 @@ static int ssl_add_cert_to_wpacket(SSL_CONNECTION *s, WPACKET *pkt,
 }
 
 /* Add certificate chain to provided WPACKET */
-static int ssl_add_cert_chain(SSL_CONNECTION *s, WPACKET *pkt, CERT_PKEY *cpk)
+static int ssl_add_cert_chain(SSL_CONNECTION *s, WPACKET *pkt, CERT_PKEY *cpk, int for_comp)
 {
     int i, chain_count;
     X509 *x;
@@ -967,12 +972,14 @@ static int ssl_add_cert_chain(SSL_CONNECTION *s, WPACKET *pkt, CERT_PKEY *cpk)
                                                        sctx->propq);
 
         if (xs_ctx == NULL) {
-            SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_X509_LIB);
+            if (!for_comp)
+                SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_X509_LIB);
             return 0;
         }
         if (!X509_STORE_CTX_init(xs_ctx, chain_store, x, NULL)) {
             X509_STORE_CTX_free(xs_ctx);
-            SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_X509_LIB);
+            if (!for_comp)
+                SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_X509_LIB);
             return 0;
         }
         /*
@@ -994,14 +1001,15 @@ static int ssl_add_cert_chain(SSL_CONNECTION *s, WPACKET *pkt, CERT_PKEY *cpk)
             ERR_raise(ERR_LIB_SSL, SSL_R_CA_MD_TOO_WEAK);
 #endif
             X509_STORE_CTX_free(xs_ctx);
-            SSLfatal(s, SSL_AD_INTERNAL_ERROR, i);
+            if (!for_comp)
+                SSLfatal(s, SSL_AD_INTERNAL_ERROR, i);
             return 0;
         }
         chain_count = sk_X509_num(chain);
         for (i = 0; i < chain_count; i++) {
             x = sk_X509_value(chain, i);
 
-            if (!ssl_add_cert_to_wpacket(s, pkt, x, i)) {
+            if (!ssl_add_cert_to_wpacket(s, pkt, x, i, for_comp)) {
                 /* SSLfatal() already called */
                 X509_STORE_CTX_free(xs_ctx);
                 return 0;
@@ -1011,16 +1019,17 @@ static int ssl_add_cert_chain(SSL_CONNECTION *s, WPACKET *pkt, CERT_PKEY *cpk)
     } else {
         i = ssl_security_cert_chain(s, extra_certs, x, 0);
         if (i != 1) {
-            SSLfatal(s, SSL_AD_INTERNAL_ERROR, i);
+            if (!for_comp)
+                SSLfatal(s, SSL_AD_INTERNAL_ERROR, i);
             return 0;
         }
-        if (!ssl_add_cert_to_wpacket(s, pkt, x, 0)) {
+        if (!ssl_add_cert_to_wpacket(s, pkt, x, 0, for_comp)) {
             /* SSLfatal() already called */
             return 0;
         }
         for (i = 0; i < sk_X509_num(extra_certs); i++) {
             x = sk_X509_value(extra_certs, i);
-            if (!ssl_add_cert_to_wpacket(s, pkt, x, i + 1)) {
+            if (!ssl_add_cert_to_wpacket(s, pkt, x, i + 1, for_comp)) {
                 /* SSLfatal() already called */
                 return 0;
             }
@@ -1030,18 +1039,20 @@ static int ssl_add_cert_chain(SSL_CONNECTION *s, WPACKET *pkt, CERT_PKEY *cpk)
 }
 
 unsigned long ssl3_output_cert_chain(SSL_CONNECTION *s, WPACKET *pkt,
-                                     CERT_PKEY *cpk)
+                                     CERT_PKEY *cpk, int for_comp)
 {
     if (!WPACKET_start_sub_packet_u24(pkt)) {
-        SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
+        if (!for_comp)
+            SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
         return 0;
     }
 
-    if (!ssl_add_cert_chain(s, pkt, cpk))
+    if (!ssl_add_cert_chain(s, pkt, cpk, for_comp))
         return 0;
 
     if (!WPACKET_close(pkt)) {
-        SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
+        if (!for_comp)
+            SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
         return 0;
     }
 

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index 0d68e4b1cf90961413fd39f4e8ce81bc31b97714..4f98eb12ad4c1c4615f3c5a61dad76766cdd8fde 100644 (file)
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -3733,7 +3733,7 @@ CON_FUNC_RETURN tls_construct_server_certificate(SSL_CONNECTION *s, WPACKET *pkt
         SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
         return CON_FUNC_ERROR;
     }
-    if (!ssl3_output_cert_chain(s, pkt, cpk)) {
+    if (!ssl3_output_cert_chain(s, pkt, cpk, 0)) {
         /* SSLfatal() already called */
         return CON_FUNC_ERROR;
     }