Remove cipher from SSL_SESSION.
authorjsing <jsing@openbsd.org>
Sat, 20 Jul 2024 04:04:23 +0000 (04:04 +0000)
committerjsing <jsing@openbsd.org>
Sat, 20 Jul 2024 04:04:23 +0000 (04:04 +0000)
For a long time SSL_SESSION has had both a cipher ID and a pointer to
an SSL_CIPHER (and not both are guaranteed to be populated). There is also
a pointer to an SSL_CIPHER in the SSL_HANDSHAKE that denotes the cipher
being used for this connection. Some code has been using the cipher from
SSL_SESSION and some code has been using the cipher from SSL_HANDSHAKE.

Remove cipher from SSL_SESSION and use the version in SSL_HANDSHAKE
everywhere. If resuming from a session then we need to use the SSL_SESSION
cipher ID to set the SSL_HANDSHAKE cipher. And we still need to ensure that
we update the cipher ID in the SSL_SESSION whenever the SSL_HANDSHAKE
cipher changes (this only occurs in a few places).

ok tb@

13 files changed:
lib/libssl/d1_pkt.c
lib/libssl/ssl_asn1.c
lib/libssl/ssl_ciph.c
lib/libssl/ssl_clnt.c
lib/libssl/ssl_lib.c
lib/libssl/ssl_local.h
lib/libssl/ssl_pkt.c
lib/libssl/ssl_sess.c
lib/libssl/ssl_srvr.c
lib/libssl/ssl_txt.c
lib/libssl/t1_enc.c
lib/libssl/tls13_client.c
lib/libssl/tls13_server.c

index df9581a..cf32ca8 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: d1_pkt.c,v 1.128 2023/07/02 20:16:47 tb Exp $ */
+/* $OpenBSD: d1_pkt.c,v 1.129 2024/07/20 04:04:23 jsing Exp $ */
 /*
  * DTLS implementation written by Nagendra Modadugu
  * (nagendra@cs.stanford.edu) for the OpenSSL project 2005.
@@ -596,7 +596,7 @@ dtls1_read_handshake_unexpected(SSL *s)
                 * It should be impossible to hit this, but keep the safety
                 * harness for now...
                 */
-               if (s->session == NULL || s->session->cipher == NULL)
+               if (s->session == NULL || s->s3->hs.cipher == NULL)
                        return 1;
 
                /*
@@ -650,7 +650,7 @@ dtls1_read_handshake_unexpected(SSL *s)
                        return -1;
                }
 
-               if (s->session == NULL || s->session->cipher == NULL) {
+               if (s->session == NULL || s->s3->hs.cipher == NULL) {
                        SSLerror(s, ERR_R_INTERNAL_ERROR);
                        return -1;
                }
index f4552f1..ef34cbd 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_asn1.c,v 1.67 2023/07/08 16:40:13 beck Exp $ */
+/* $OpenBSD: ssl_asn1.c,v 1.68 2024/07/20 04:04:23 jsing Exp $ */
 /*
  * Copyright (c) 2016 Joel Sing <jsing@openbsd.org>
  *
@@ -70,10 +70,7 @@ SSL_SESSION_encode(SSL_SESSION *s, unsigned char **out, size_t *out_len,
                goto err;
 
        /* Cipher suite ID. */
-       /* XXX - require cipher to be non-NULL or always/only use cipher_id. */
        cid = (uint16_t)(s->cipher_id & SSL3_CK_VALUE_MASK);
-       if (s->cipher != NULL)
-               cid = ssl3_cipher_get_value(s->cipher);
        if (!CBB_add_asn1(&session, &cipher_suite, CBS_ASN1_OCTETSTRING))
                goto err;
        if (!CBB_add_u16(&cipher_suite, cid))
@@ -196,7 +193,7 @@ SSL_SESSION_ticket(SSL_SESSION *ss, unsigned char **out, size_t *out_len)
        if (ss == NULL)
                return 0;
 
-       if (ss->cipher == NULL && ss->cipher_id == 0)
+       if (ss->cipher_id == 0)
                return 0;
 
        return SSL_SESSION_encode(ss, out, out_len, 1);
@@ -212,7 +209,7 @@ i2d_SSL_SESSION(SSL_SESSION *ss, unsigned char **pp)
        if (ss == NULL)
                return 0;
 
-       if (ss->cipher == NULL && ss->cipher_id == 0)
+       if (ss->cipher_id == 0)
                return 0;
 
        if (!SSL_SESSION_encode(ss, &data, &data_len, 0))
@@ -287,9 +284,6 @@ d2i_SSL_SESSION(SSL_SESSION **a, const unsigned char **pp, long length)
                goto err;
        if (CBS_len(&cipher_suite) != 0)
                goto err;
-
-       /* XXX - populate cipher instead? */
-       s->cipher = NULL;
        s->cipher_id = SSL3_CK_ID | cipher_value;
 
        /* Session ID. */
index 246d64e..7c32354 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_ciph.c,v 1.144 2024/07/16 14:38:04 jsing Exp $ */
+/* $OpenBSD: ssl_ciph.c,v 1.145 2024/07/20 04:04:23 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -408,25 +408,27 @@ static const SSL_CIPHER cipher_aliases[] = {
 };
 
 int
-ssl_cipher_get_evp(const SSL_SESSION *ss, const EVP_CIPHER **enc,
-    const EVP_MD **md, int *mac_pkey_type, int *mac_secret_size)
+ssl_cipher_get_evp(SSL *s, const EVP_CIPHER **enc, const EVP_MD **md,
+    int *mac_pkey_type, int *mac_secret_size)
 {
+       const SSL_CIPHER *cipher;
+
        *enc = NULL;
        *md = NULL;
        *mac_pkey_type = NID_undef;
        *mac_secret_size = 0;
 
-       if (ss->cipher == NULL)
+       if ((cipher = s->s3->hs.cipher) == NULL)
                return 0;
 
        /*
         * This function does not handle EVP_AEAD.
         * See ssl_cipher_get_evp_aead instead.
         */
-       if (ss->cipher->algorithm_mac & SSL_AEAD)
+       if (cipher->algorithm_mac & SSL_AEAD)
                return 0;
 
-       switch (ss->cipher->algorithm_enc) {
+       switch (cipher->algorithm_enc) {
        case SSL_3DES:
                *enc = EVP_des_ede3_cbc();
                break;
@@ -450,7 +452,7 @@ ssl_cipher_get_evp(const SSL_SESSION *ss, const EVP_CIPHER **enc,
                break;
        }
 
-       switch (ss->cipher->algorithm_mac) {
+       switch (cipher->algorithm_mac) {
        case SSL_MD5:
                *md = EVP_md5();
                break;
@@ -487,16 +489,18 @@ ssl_cipher_get_evp(const SSL_SESSION *ss, const EVP_CIPHER **enc,
  * for s->cipher. It returns 1 on success and 0 on error.
  */
 int
-ssl_cipher_get_evp_aead(const SSL_SESSION *ss, const EVP_AEAD **aead)
+ssl_cipher_get_evp_aead(SSL *s, const EVP_AEAD **aead)
 {
+       const SSL_CIPHER *cipher;
+
        *aead = NULL;
 
-       if (ss->cipher == NULL)
+       if ((cipher = s->s3->hs.cipher) == NULL)
                return 0;
-       if ((ss->cipher->algorithm_mac & SSL_AEAD) == 0)
+       if ((cipher->algorithm_mac & SSL_AEAD) == 0)
                return 0;
 
-       switch (ss->cipher->algorithm_enc) {
+       switch (cipher->algorithm_enc) {
        case SSL_AES128GCM:
                *aead = EVP_aead_aes_128_gcm();
                return 1;
@@ -515,12 +519,14 @@ ssl_cipher_get_evp_aead(const SSL_SESSION *ss, const EVP_AEAD **aead)
 int
 ssl_get_handshake_evp_md(SSL *s, const EVP_MD **md)
 {
+       const SSL_CIPHER *cipher;
+
        *md = NULL;
 
-       if (s->s3->hs.cipher == NULL)
+       if ((cipher = s->s3->hs.cipher) == NULL)
                return 0;
 
-       switch (s->s3->hs.cipher->algorithm2 & SSL_HANDSHAKE_MAC_MASK) {
+       switch (cipher->algorithm2 & SSL_HANDSHAKE_MAC_MASK) {
        case SSL_HANDSHAKE_MAC_SHA256:
                *md = EVP_sha256();
                return 1;
index 6cf0ee4..7b2e05d 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_clnt.c,v 1.166 2024/07/19 08:56:17 jsing Exp $ */
+/* $OpenBSD: ssl_clnt.c,v 1.167 2024/07/20 04:04:23 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -481,7 +481,7 @@ ssl3_connect(SSL *s)
 
                        s->s3->hs.state = SSL3_ST_CW_FINISHED_A;
                        s->init_num = 0;
-                       s->session->cipher = s->s3->hs.cipher;
+                       s->session->cipher_id = s->s3->hs.cipher->id;
 
                        if (!tls1_setup_key_block(s)) {
                                ret = -1;
@@ -946,8 +946,8 @@ ssl3_get_server_hello(SSL *s)
                 * client cannot change the cipher at this stage,
                 * as the server has already made a selection.
                 */
-               if ((s->session->cipher = pref_cipher) == NULL)
-                       s->session->cipher =
+               if ((s->s3->hs.cipher = pref_cipher) == NULL)
+                       s->s3->hs.cipher =
                            ssl3_get_cipher_by_value(cipher_suite);
                s->s3->flags |= SSL3_FLAGS_CCS_OK;
        }
@@ -1016,14 +1016,13 @@ ssl3_get_server_hello(SSL *s)
         * and/or cipher_id values may not be set. Make sure that
         * cipher_id is set and use it for comparison.
         */
-       if (s->session->cipher)
-               s->session->cipher_id = s->session->cipher->id;
        if (s->hit && (s->session->cipher_id != cipher->id)) {
                al = SSL_AD_ILLEGAL_PARAMETER;
                SSLerror(s, SSL_R_OLD_SESSION_CIPHER_NOT_RETURNED);
                goto fatal_err;
        }
        s->s3->hs.cipher = cipher;
+       s->session->cipher_id = cipher->id;
 
        if (!tls1_transcript_hash_init(s))
                goto err;
index 4b86b70..4cf5c46 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_lib.c,v 1.327 2024/07/19 08:54:31 jsing Exp $ */
+/* $OpenBSD: ssl_lib.c,v 1.328 2024/07/20 04:04:23 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -3073,11 +3073,10 @@ LSSL_ALIAS(SSL_get_privatekey);
 const SSL_CIPHER *
 SSL_get_current_cipher(const SSL *s)
 {
-       if ((s->session != NULL) && (s->session->cipher != NULL))
-               return (s->session->cipher);
-       return (NULL);
+       return s->s3->hs.cipher;
 }
 LSSL_ALIAS(SSL_get_current_cipher);
+
 const void *
 SSL_get_current_compression(SSL *s)
 {
index e9b6a62..79f41e6 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_local.h,v 1.20 2024/07/19 08:54:31 jsing Exp $ */
+/* $OpenBSD: ssl_local.h,v 1.21 2024/07/20 04:04:23 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -438,7 +438,6 @@ struct ssl_session_st {
        time_t time;
        int references;
 
-       const SSL_CIPHER *cipher;
        unsigned long cipher_id;        /* when ASN.1 loaded, this
                                         * needs to be used to load
                                         * the 'cipher' structure */
@@ -1267,9 +1266,9 @@ int ssl_merge_cipherlists(STACK_OF(SSL_CIPHER) *cipherlist,
     STACK_OF(SSL_CIPHER) *cipherlist_tls13,
     STACK_OF(SSL_CIPHER) **out_cipherlist);
 void ssl_update_cache(SSL *s, int mode);
-int ssl_cipher_get_evp(const SSL_SESSION *s, const EVP_CIPHER **enc,
+int ssl_cipher_get_evp(SSL *s, const EVP_CIPHER **enc,
     const EVP_MD **md, int *mac_pkey_type, int *mac_secret_size);
-int ssl_cipher_get_evp_aead(const SSL_SESSION *s, const EVP_AEAD **aead);
+int ssl_cipher_get_evp_aead(SSL *s, const EVP_AEAD **aead);
 int ssl_get_handshake_evp_md(SSL *s, const EVP_MD **md);
 
 int ssl_verify_cert_chain(SSL *s, STACK_OF(X509) *sk);
index 2c33c45..7d6785a 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_pkt.c,v 1.66 2023/07/11 17:02:47 tb Exp $ */
+/* $OpenBSD: ssl_pkt.c,v 1.67 2024/07/20 04:04:23 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -904,7 +904,7 @@ ssl3_read_handshake_unexpected(SSL *s)
                 * It should be impossible to hit this, but keep the safety
                 * harness for now...
                 */
-               if (s->session == NULL || s->session->cipher == NULL)
+               if (s->session == NULL || s->s3->hs.cipher == NULL)
                        return 1;
 
                /*
@@ -953,7 +953,7 @@ ssl3_read_handshake_unexpected(SSL *s)
                        return -1;
                }
 
-               if (s->session == NULL || s->session->cipher == NULL) {
+               if (s->session == NULL || s->s3->hs.cipher == NULL) {
                        SSLerror(s, ERR_R_INTERNAL_ERROR);
                        return -1;
                }
@@ -1235,7 +1235,8 @@ ssl3_do_change_cipher_spec(SSL *s)
                        return (0);
                }
 
-               s->session->cipher = s->s3->hs.cipher;
+               s->session->cipher_id = s->s3->hs.cipher->id;
+
                if (!tls1_setup_key_block(s))
                        return (0);
        }
index 76f194c..c2bd1bf 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_sess.c,v 1.126 2024/07/19 08:54:31 jsing Exp $ */
+/* $OpenBSD: ssl_sess.c,v 1.127 2024/07/20 04:04:23 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -287,7 +287,6 @@ ssl_session_dup(SSL_SESSION *sess, int include_ticket)
        copy->time = sess->time;
        copy->references = 1;
 
-       copy->cipher = sess->cipher;
        copy->cipher_id = sess->cipher_id;
 
        if (sess->tlsext_hostname != NULL) {
@@ -707,12 +706,6 @@ ssl_get_prev_session(SSL *s, CBS *session_id, CBS *ext_block, int *alert)
                goto err;
        }
 
-       if (sess->cipher == NULL) {
-               sess->cipher = ssl3_get_cipher_by_id(sess->cipher_id);
-               if (sess->cipher == NULL)
-                       goto err;
-       }
-
        if (sess->timeout < (time(NULL) - sess->time)) {
                s->session_ctx->stats.sess_timeout++;
                if (!ticket_decrypted) {
@@ -991,7 +984,7 @@ LSSL_ALIAS(SSL_SESSION_get_protocol_version);
 const SSL_CIPHER *
 SSL_SESSION_get0_cipher(const SSL_SESSION *s)
 {
-       return s->cipher;
+       return ssl3_get_cipher_by_id(s->cipher_id);
 }
 LSSL_ALIAS(SSL_SESSION_get0_cipher);
 
index 01155a8..be6bd74 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_srvr.c,v 1.163 2024/07/19 08:56:17 jsing Exp $ */
+/* $OpenBSD: ssl_srvr.c,v 1.164 2024/07/20 04:04:23 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -651,7 +651,7 @@ ssl3_accept(SSL *s)
                                goto end;
                        s->s3->hs.state = SSL3_ST_SW_FINISHED_A;
                        s->init_num = 0;
-                       s->session->cipher = s->s3->hs.cipher;
+                       s->session->cipher_id = s->s3->hs.cipher->id;
 
                        if (!tls1_setup_key_block(s)) {
                                ret = -1;
@@ -978,7 +978,7 @@ ssl3_get_client_hello(SSL *s)
        /* XXX - CBS_len(&cipher_suites) will always be zero here... */
        if (s->hit && CBS_len(&cipher_suites) > 0) {
                j = 0;
-               id = s->session->cipher->id;
+               id = s->session->cipher_id;
 
                for (i = 0; i < sk_SSL_CIPHER_num(ciphers); i++) {
                        c = sk_SSL_CIPHER_value(ciphers, i);
@@ -1098,7 +1098,7 @@ ssl3_get_client_hello(SSL *s)
                        SSLerror(s, SSL_R_NO_SHARED_CIPHER);
                        goto fatal_err;
                }
-               s->session->cipher = pref_cipher;
+               s->s3->hs.cipher = pref_cipher;
 
                /* XXX - why? */
                sk_SSL_CIPHER_free(s->cipher_list);
@@ -1127,8 +1127,11 @@ ssl3_get_client_hello(SSL *s)
                        goto fatal_err;
                }
                s->s3->hs.cipher = c;
+               s->session->cipher_id = s->s3->hs.cipher->id;
        } else {
-               s->s3->hs.cipher = s->session->cipher;
+               s->s3->hs.cipher = ssl3_get_cipher_by_id(s->session->cipher_id);
+               if (s->s3->hs.cipher == NULL)
+                       goto fatal_err;
        }
 
        if (!tls1_transcript_hash_init(s))
index ee3d218..26b631d 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_txt.c,v 1.37 2023/07/08 16:40:13 beck Exp $ */
+/* $OpenBSD: ssl_txt.c,v 1.38 2024/07/20 04:04:23 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -108,6 +108,7 @@ LSSL_ALIAS(SSL_SESSION_print_fp);
 int
 SSL_SESSION_print(BIO *bp, const SSL_SESSION *x)
 {
+       const SSL_CIPHER *cipher;
        size_t i;
        int ret = 0;
 
@@ -121,15 +122,15 @@ SSL_SESSION_print(BIO *bp, const SSL_SESSION *x)
            ssl_version_string(x->ssl_version)) <= 0)
                goto err;
 
-       if (x->cipher == NULL) {
+       if ((cipher = ssl3_get_cipher_by_id(x->cipher_id)) == NULL) {
                if (BIO_printf(bp, "    Cipher    : %04lX\n",
                    x->cipher_id & SSL3_CK_VALUE_MASK) <= 0)
                        goto err;
        } else {
                const char *cipher_name = "unknown";
 
-               if (x->cipher->name != NULL)
-                       cipher_name = x->cipher->name;
+               if (cipher->name != NULL)
+                       cipher_name = cipher->name;
 
                if (BIO_printf(bp, "    Cipher    : %s\n", cipher_name) <= 0)
                        goto err;
index c6140e9..64e1dd5 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: t1_enc.c,v 1.157 2022/11/26 16:08:56 tb Exp $ */
+/* $OpenBSD: t1_enc.c,v 1.158 2024/07/20 04:04:23 jsing Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -357,15 +357,17 @@ tls1_setup_key_block(SSL *s)
        if (s->s3->hs.tls12.key_block != NULL)
                return (1);
 
-       if (s->session->cipher &&
-           (s->session->cipher->algorithm_mac & SSL_AEAD)) {
-               if (!ssl_cipher_get_evp_aead(s->session, &aead)) {
+       if (s->s3->hs.cipher == NULL)
+               return (0);
+
+       if ((s->s3->hs.cipher->algorithm_mac & SSL_AEAD) != 0) {
+               if (!ssl_cipher_get_evp_aead(s, &aead)) {
                        SSLerror(s, SSL_R_CIPHER_OR_HASH_UNAVAILABLE);
                        return (0);
                }
        } else {
                /* XXX - mac_type and mac_secret_size are now unused. */
-               if (!ssl_cipher_get_evp(s->session, &cipher, &mac_hash,
+               if (!ssl_cipher_get_evp(s, &cipher, &mac_hash,
                    &mac_type, &mac_secret_size)) {
                        SSLerror(s, SSL_R_CIPHER_OR_HASH_UNAVAILABLE);
                        return (0);
@@ -395,12 +397,12 @@ tls1_setup_key_block(SSL *s)
                 */
                s->s3->need_empty_fragments = 1;
 
-               if (s->session->cipher != NULL) {
-                       if (s->session->cipher->algorithm_enc == SSL_eNULL)
+               if (s->s3->hs.cipher != NULL) {
+                       if (s->s3->hs.cipher->algorithm_enc == SSL_eNULL)
                                s->s3->need_empty_fragments = 0;
 
 #ifndef OPENSSL_NO_RC4
-                       if (s->session->cipher->algorithm_enc == SSL_RC4)
+                       if (s->s3->hs.cipher->algorithm_enc == SSL_RC4)
                                s->s3->need_empty_fragments = 0;
 #endif
                }
index 053cf16..8f6894f 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls13_client.c,v 1.102 2023/06/10 15:34:36 tb Exp $ */
+/* $OpenBSD: tls13_client.c,v 1.103 2024/07/20 04:04:23 jsing Exp $ */
 /*
  * Copyright (c) 2018, 2019 Joel Sing <jsing@openbsd.org>
  *
@@ -347,7 +347,7 @@ tls13_client_engage_record_protection(struct tls13_ctx *ctx)
            &shared_key_len))
                goto err;
 
-       s->session->cipher = ctx->hs->cipher;
+       s->session->cipher_id = ctx->hs->cipher->id;
        s->session->ssl_version = ctx->hs->tls13.server_version;
 
        if ((ctx->aead = tls13_cipher_aead(ctx->hs->cipher)) == NULL)
index f9cdbdd..6bd2993 100644 (file)
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls13_server.c,v 1.107 2024/07/19 08:54:31 jsing Exp $ */
+/* $OpenBSD: tls13_server.c,v 1.108 2024/07/20 04:04:23 jsing Exp $ */
 /*
  * Copyright (c) 2019, 2020 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2020 Bob Beck <beck@openbsd.org>
@@ -383,7 +383,7 @@ tls13_server_engage_record_protection(struct tls13_ctx *ctx)
            &shared_key_len))
                goto err;
 
-       s->session->cipher = ctx->hs->cipher;
+       s->session->cipher_id = ctx->hs->cipher->id;
 
        if ((ctx->aead = tls13_cipher_aead(ctx->hs->cipher)) == NULL)
                goto err;