Separate ca_names handling for client and server

SSL(_CTX)?_set_client_CA_list() was a server side only function in 1.1.0.
If it was called on the client side then it was ignored. In 1.1.1 it now
makes sense to have a CA list defined for both client and server (the
client now sends it the the TLSv1.3 certificate_authorities extension).
Unfortunately some applications were using the same SSL_CTX for both
clients and servers and this resulted in some client ClientHellos being
excessively large due to the number of certificate authorities being sent.

This commit seperates out the CA list updated by
SSL(_CTX)?_set_client_CA_list() and the more generic
SSL(_CTX)?_set0_CA_list(). This means that SSL(_CTX)?_set_client_CA_list()
still has no effect on the client side. If both CA lists are set then
SSL(_CTX)?_set_client_CA_list() takes priority.

Fixes #7411

Reviewed-by: Viktor Dukhovni <viktor@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/7503)

diff --git a/doc/man3/SSL_CTX_set0_CA_list.pod b/doc/man3/SSL_CTX_set0_CA_list.pod
index 618bd73e0420e4ebea370d61c082fc2d756441a3..37a4cee9ca3d678d8e122401d75f6c05d00e91a5 100644 (file)
--- a/doc/man3/SSL_CTX_set0_CA_list.pod
+++ b/doc/man3/SSL_CTX_set0_CA_list.pod
@@ -48,7 +48,10 @@ has sent.
 =head1 NOTES
 
 These functions are generalised versions of the client authentication
-CA list functions such as L<SSL_CTX_set_client_CA_list(3)>.
+CA list functions such as L<SSL_CTX_set_client_CA_list(3)>. If both these
+and L<SSL_CTX_set_client_CA_list(3)> or similar functions are used, then the
+latter functions take priority on the server side (they are ignored on the
+client side).
 
 For TLS versions before 1.3 the list of CA names is only sent from the server
 to client when requesting a client certificate. So any list of CA names set

diff --git a/doc/man3/SSL_CTX_set_client_CA_list.pod b/doc/man3/SSL_CTX_set_client_CA_list.pod
index 76fd65e6fcaa7c66276c3192253028d854d7985a..e23999aaaeba3c4942690cd0679ec2fc16aef6a0 100644 (file)
--- a/doc/man3/SSL_CTX_set_client_CA_list.pod
+++ b/doc/man3/SSL_CTX_set_client_CA_list.pod
@@ -34,6 +34,11 @@ the chosen B<ssl>, overriding the setting valid for B<ssl>'s SSL_CTX object.
 
 =head1 NOTES
 
+These functions are similar to L<SSL_CTX_set0_CA_list(3)> and similar functions
+but only have an effect on the server side. These functions are present for
+backwards compatibility. L<SSL_CTX_set0_CA_list(3)> and similar functions should
+be used in preference.
+
 When a TLS/SSL server requests a client certificate (see
 B<SSL_CTX_set_verify(3)>), it sends a list of CAs, for which
 it will accept certificates, to the client.

diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c
index 7d7357fb3a65f6d7bcb6078381be56863eb193d0..33145078963d950b2caa0fc001ff7ea2ce03e17c 100644 (file)
--- a/ssl/ssl_cert.c
+++ b/ssl/ssl_cert.c
@@ -501,17 +501,17 @@ const STACK_OF(X509_NAME) *SSL_get0_CA_list(const SSL *s)
 
 void SSL_CTX_set_client_CA_list(SSL_CTX *ctx, STACK_OF(X509_NAME) *name_list)
 {
-    SSL_CTX_set0_CA_list(ctx, name_list);
+    set0_CA_list(&ctx->client_ca_names, name_list);
 }
 
 STACK_OF(X509_NAME) *SSL_CTX_get_client_CA_list(const SSL_CTX *ctx)
 {
-    return ctx->ca_names;
+    return ctx->client_ca_names;
 }
 
 void SSL_set_client_CA_list(SSL *s, STACK_OF(X509_NAME) *name_list)
 {
-    SSL_set0_CA_list(s, name_list);
+    set0_CA_list(&s->client_ca_names, name_list);
 }
 
 const STACK_OF(X509_NAME) *SSL_get0_peer_CA_list(const SSL *s)
@@ -523,7 +523,8 @@ STACK_OF(X509_NAME) *SSL_get_client_CA_list(const SSL *s)
 {
     if (!s->server)
         return s->s3 != NULL ?  s->s3->tmp.peer_ca_names : NULL;
-    return s->ca_names != NULL ?  s->ca_names : s->ctx->ca_names;
+    return s->client_ca_names != NULL ?  s->client_ca_names
+                                      : s->ctx->client_ca_names;
 }
 
 static int add_ca_name(STACK_OF(X509_NAME) **sk, const X509 *x)
@@ -561,12 +562,12 @@ int SSL_CTX_add1_to_CA_list(SSL_CTX *ctx, const X509 *x)
  */
 int SSL_add_client_CA(SSL *ssl, X509 *x)
 {
-    return add_ca_name(&ssl->ca_names, x);
+    return add_ca_name(&ssl->client_ca_names, x);
 }
 
 int SSL_CTX_add_client_CA(SSL_CTX *ctx, X509 *x)
 {
-    return add_ca_name(&ctx->ca_names, x);
+    return add_ca_name(&ctx->client_ca_names, x);
 }
 
 static int xname_cmp(const X509_NAME *a, const X509_NAME *b)

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index e7e8aa90f9768d873aad074fb57c54a58752764f..087f768b0b2a1822425367f4bec3aeb46e4471ab 100644 (file)
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -1194,6 +1194,7 @@ void SSL_free(SSL *s)
     EVP_MD_CTX_free(s->pha_dgst);
 
     sk_X509_NAME_pop_free(s->ca_names, X509_NAME_free);
+    sk_X509_NAME_pop_free(s->client_ca_names, X509_NAME_free);
 
     sk_X509_pop_free(s->verified_chain, X509_free);
 
@@ -2953,6 +2954,9 @@ SSL_CTX *SSL_CTX_new(const SSL_METHOD *meth)
     if ((ret->ca_names = sk_X509_NAME_new_null()) == NULL)
         goto err;
 
+    if ((ret->client_ca_names = sk_X509_NAME_new_null()) == NULL)
+        goto err;
+
     if (!CRYPTO_new_ex_data(CRYPTO_EX_INDEX_SSL_CTX, ret, &ret->ex_data))
         goto err;
 
@@ -3110,6 +3114,7 @@ void SSL_CTX_free(SSL_CTX *a)
     sk_SSL_CIPHER_free(a->tls13_ciphersuites);
     ssl_cert_free(a->cert);
     sk_X509_NAME_pop_free(a->ca_names, X509_NAME_free);
+    sk_X509_NAME_pop_free(a->client_ca_names, X509_NAME_free);
     sk_X509_pop_free(a->extra_certs, X509_free);
     a->comp_methods = NULL;
 #ifndef OPENSSL_NO_SRTP
@@ -3655,10 +3660,38 @@ const char *SSL_get_version(const SSL *s)
     return ssl_protocol_to_string(s->version);
 }
 
-SSL *SSL_dup(SSL *s)
+static int dup_ca_names(STACK_OF(X509_NAME) **dst, STACK_OF(X509_NAME) *src)
 {
     STACK_OF(X509_NAME) *sk;
     X509_NAME *xn;
+    int i;
+
+    if (src == NULL) {
+        *dst = NULL;
+        return 1;
+    }
+
+    if ((sk = sk_X509_NAME_new_null()) == NULL)
+        return 0;
+    for (i = 0; i < sk_X509_NAME_num(src); i++) {
+        xn = X509_NAME_dup(sk_X509_NAME_value(src, i));
+        if (xn == NULL) {
+            sk_X509_NAME_pop_free(sk, X509_NAME_free);
+            return 0;
+        }
+        if (sk_X509_NAME_insert(sk, xn, i) == 0) {
+            X509_NAME_free(xn);
+            sk_X509_NAME_pop_free(sk, X509_NAME_free);
+            return 0;
+        }
+    }
+    *dst = sk;
+
+    return 1;
+}
+
+SSL *SSL_dup(SSL *s)
+{
     SSL *ret;
     int i;
 
@@ -3763,18 +3796,10 @@ SSL *SSL_dup(SSL *s)
             goto err;
 
     /* Dup the client_CA list */
-    if (s->ca_names != NULL) {
-        if ((sk = sk_X509_NAME_dup(s->ca_names)) == NULL)
-            goto err;
-        ret->ca_names = sk;
-        for (i = 0; i < sk_X509_NAME_num(sk); i++) {
-            xn = sk_X509_NAME_value(sk, i);
-            if (sk_X509_NAME_set(sk, i, X509_NAME_dup(xn)) == NULL) {
-                X509_NAME_free(xn);
-                goto err;
-            }
-        }
-    }
+    if (!dup_ca_names(&ret->ca_names, s->ca_names)
+            || !dup_ca_names(&ret->client_ca_names, s->client_ca_names))
+        goto err;
+
     return ret;
 
  err:

diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index 46719b00cab204a903b4c65ff21fbbbcfe11f471..e9c5c5cf80520f942d41062f7bde08401fe5f0f3 100644 (file)
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -854,9 +854,11 @@ struct ssl_ctx_st {
     /*
      * What we put in certificate_authorities extension for TLS 1.3
      * (ClientHello and CertificateRequest) or just client cert requests for
-     * earlier versions.
+     * earlier versions. If client_ca_names is populated then it is only used
+     * for client cert requests, and in preference to ca_names.
      */
     STACK_OF(X509_NAME) *ca_names;
+    STACK_OF(X509_NAME) *client_ca_names;
 
     /*
      * Default values to use in SSL structures follow (these are copied by
@@ -1233,8 +1235,14 @@ struct ssl_st {
     long verify_result;
     /* extra application data */
     CRYPTO_EX_DATA ex_data;
-    /* for server side, keep the list of CA_dn we can use */
+    /*
+     * What we put in certificate_authorities extension for TLS 1.3
+     * (ClientHello and CertificateRequest) or just client cert requests for
+     * earlier versions. If client_ca_names is populated then it is only used
+     * for client cert requests, and in preference to ca_names.
+     */
     STACK_OF(X509_NAME) *ca_names;
+    STACK_OF(X509_NAME) *client_ca_names;
     CRYPTO_REF_COUNT references;
     /* protocol behaviour */
     uint32_t options;

diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index ad4256d370e297ba3e62be9e0a3ef6ad989a9cb8..63e61c6184acfd478b516196c50a89d8635811d5 100644 (file)
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -1198,7 +1198,7 @@ static EXT_RETURN tls_construct_certificate_authorities(SSL *s, WPACKET *pkt,
                                                         X509 *x,
                                                         size_t chainidx)
 {
-    const STACK_OF(X509_NAME) *ca_sk = SSL_get0_CA_list(s);
+    const STACK_OF(X509_NAME) *ca_sk = get_ca_names(s);
 
     if (ca_sk == NULL || sk_X509_NAME_num(ca_sk) == 0)
         return EXT_RETURN_NOT_SENT;
@@ -1211,7 +1211,7 @@ static EXT_RETURN tls_construct_certificate_authorities(SSL *s, WPACKET *pkt,
         return EXT_RETURN_FAIL;
     }
 
-    if (!construct_ca_names(s, pkt)) {
+    if (!construct_ca_names(s, ca_sk, pkt)) {
         /* SSLfatal() already called */
         return EXT_RETURN_FAIL;
     }

diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c
index dc2bd20e936d22efbb4f0d2b329d0f57c1c30d98..95c22062ba68cbf8478408e1e3b3d939ada6a062 100644 (file)
--- a/ssl/statem/statem_lib.c
+++ b/ssl/statem/statem_lib.c
@@ -2287,10 +2287,24 @@ int parse_ca_names(SSL *s, PACKET *pkt)
     return 0;
 }
 
-int construct_ca_names(SSL *s, WPACKET *pkt)
+const STACK_OF(X509_NAME) *get_ca_names(SSL *s)
 {
-    const STACK_OF(X509_NAME) *ca_sk = SSL_get0_CA_list(s);
+    const STACK_OF(X509_NAME) *ca_sk = NULL;;
 
+    if (s->server) {
+        ca_sk = SSL_get_client_CA_list(s);
+        if (ca_sk != NULL && sk_X509_NAME_num(ca_sk) == 0)
+            ca_sk = NULL;
+    }
+
+    if (ca_sk == NULL)
+        ca_sk = SSL_get0_CA_list(s);
+
+    return ca_sk;
+}
+
+int construct_ca_names(SSL *s, const STACK_OF(X509_NAME) *ca_sk, WPACKET *pkt)
+{
     /* Start sub-packet for client CA list */
     if (!WPACKET_start_sub_packet_u16(pkt)) {
         SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_CONSTRUCT_CA_NAMES,

diff --git a/ssl/statem/statem_locl.h b/ssl/statem/statem_locl.h
index 25e56e4e8ddfcffaff32840da01f3ca400aec4b1..6b8cf37faa011644e6fedd43d41e45a9936d9b3c 100644 (file)
--- a/ssl/statem/statem_locl.h
+++ b/ssl/statem/statem_locl.h
@@ -61,7 +61,8 @@ int create_synthetic_message_hash(SSL *s, const unsigned char *hashval,
                                   size_t hashlen, const unsigned char *hrr,
                                   size_t hrrlen);
 int parse_ca_names(SSL *s, PACKET *pkt);
-int construct_ca_names(SSL *s, WPACKET *pkt);
+const STACK_OF(X509_NAME) *get_ca_names(SSL *s);
+int construct_ca_names(SSL *s, const STACK_OF(X509_NAME) *ca_sk, WPACKET *pkt);
 size_t construct_key_exchange_tbs(SSL *s, unsigned char **ptbs,
                                   const void *param, size_t paramlen);
 

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index 7d0e9d0ba8756466a562a2c14489adfc442ee9f3..e7c11c4bea4deebd7445cc8523f9c9e7d723846a 100644 (file)
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -2880,7 +2880,7 @@ int tls_construct_certificate_request(SSL *s, WPACKET *pkt)
         }
     }
 
-    if (!construct_ca_names(s, pkt)) {
+    if (!construct_ca_names(s, get_ca_names(s), pkt)) {
         /* SSLfatal() already called */
         return 0;
     }