tls-crypto: Simplify handshake/application key derivation and rename methods

Also consistently change the ciphers outside of tls_crypto_t and
simplify key derivation in tls_peer_t and fix a memory leak.

diff --git a/src/libtls/tls_crypto.c b/src/libtls/tls_crypto.c
index 77fc153..0fbc51e 100644 (file)
--- a/src/libtls/tls_crypto.c
+++ b/src/libtls/tls_crypto.c
@@ -1244,10 +1244,6 @@ static bool create_aead(private_tls_crypto_t *this, suite_algs_t *algs)
        {
                this->aead_in = tls_aead_create_seq(algs->encr, algs->encr_size);
                this->aead_out = tls_aead_create_seq(algs->encr, algs->encr_size);
-
-               this->protection->set_cipher(this->protection, TRUE, this->aead_in);
-               this->protection->set_cipher(this->protection, FALSE, this->aead_out);
-
        }
        if (!this->aead_in || !this->aead_out)
        {
@@ -1982,148 +1978,79 @@ METHOD(tls_crypto_t, derive_secrets, bool,
                   expand_keys(this, client_random, server_random);
 }
 
-METHOD(tls_crypto_t, derive_handshake_secret, bool,
-       private_tls_crypto_t *this, chunk_t shared_secret)
+/**
+ * Derive and configure the keys/IVs using the given labels.
+ */
+static bool derive_labeled_keys(private_tls_crypto_t *this,
+                                                               tls_hkdf_label_t client_label,
+                                                               tls_hkdf_label_t server_label)
 {
-       chunk_t c_key, c_iv, s_key, s_iv;
+       chunk_t c_key = chunk_empty, c_iv = chunk_empty;
+       chunk_t s_key = chunk_empty, s_iv = chunk_empty;
+       tls_aead_t *aead_c = this->aead_out, *aead_s = this->aead_in;
+       bool success = FALSE;
 
-       this->hkdf->set_shared_secret(this->hkdf, shared_secret);
+       if (this->tls->is_server(this->tls))
+       {
+               aead_c = this->aead_in;
+               aead_s = this->aead_out;
+       }
 
-       /* client key material */
-       if (!this->hkdf->generate_secret(this->hkdf, TLS_HKDF_C_HS_TRAFFIC,
-                                                                        this->handshake, NULL) ||
+       if (!this->hkdf->generate_secret(this->hkdf, client_label, this->handshake,
+                                                                        NULL) ||
                !this->hkdf->derive_key(this->hkdf, FALSE,
-                                                               this->aead_out->
-                                                               get_encr_key_size(this->aead_out), &c_key) ||
+                                                               this->aead_out->get_encr_key_size(this->aead_out),
+                                                               &c_key) ||
                !this->hkdf->derive_iv(this->hkdf, FALSE,
-                                                          this->aead_out->
-                                                          get_iv_size(this->aead_out), &c_iv))
+                                                          this->aead_out->get_iv_size(this->aead_out),
+                                                          &c_iv))
        {
                DBG1(DBG_TLS, "deriving client key material failed");
-               chunk_clear(&c_key);
-               chunk_clear(&c_iv);
-               return FALSE;
+               goto out;
        }
 
-       /* server key material */
-       if (!this->hkdf->generate_secret(this->hkdf, TLS_HKDF_S_HS_TRAFFIC,
-          this->handshake, NULL) ||
-          !this->hkdf->derive_key(this->hkdf, TRUE, this->aead_in->
-          get_encr_key_size(this->aead_in), &s_key) ||
-          !this->hkdf->derive_iv(this->hkdf, TRUE, this->aead_in->
-          get_iv_size(this->aead_in), &s_iv))
+       if (!this->hkdf->generate_secret(this->hkdf, server_label, this->handshake,
+                                                                        NULL) ||
+               !this->hkdf->derive_key(this->hkdf, TRUE,
+                                                               this->aead_in->get_encr_key_size(this->aead_in),
+                                                               &s_key) ||
+               !this->hkdf->derive_iv(this->hkdf, TRUE,
+                                                          this->aead_in->get_iv_size(this->aead_in),
+                                                          &s_iv))
        {
                DBG1(DBG_TLS, "deriving server key material failed");
-               chunk_clear(&c_key);
-               chunk_clear(&c_iv);
-               chunk_clear(&s_key);
-               chunk_clear(&s_iv);
-               return FALSE;
+               goto out;
        }
 
-       if (this->tls->is_server(this->tls))
+       if (!aead_c->set_keys(aead_c, chunk_empty, c_key, c_iv) ||
+               !aead_s->set_keys(aead_s, chunk_empty, s_key, s_iv))
        {
-               if (!this->aead_in->set_keys(this->aead_in, chunk_empty, s_key, s_iv) ||
-                       !this->aead_out->set_keys(this->aead_out, chunk_empty, c_key, c_iv))
-               {
-                       DBG1(DBG_TLS, "setting aead server key material failed");
-                       chunk_clear(&c_key);
-                       chunk_clear(&c_iv);
-                       chunk_clear(&s_key);
-                       chunk_clear(&s_iv);
-                       return FALSE;
-               }
-       }
-       else
-       {
-               if (!this->aead_in->set_keys(this->aead_in, chunk_empty, s_key, s_iv) ||
-                       !this->aead_out->set_keys(this->aead_out, chunk_empty, c_key, c_iv))
-               {
-                       DBG1(DBG_TLS, "setting aead client key material failed");
-                       chunk_clear(&c_key);
-                       chunk_clear(&c_iv);
-                       chunk_clear(&s_key);
-                       chunk_clear(&s_iv);
-                       return FALSE;
-               }
+               DBG1(DBG_TLS, "setting AEAD key material failed");
+               goto out;
        }
+       success = TRUE;
 
+out:
        chunk_clear(&c_key);
        chunk_clear(&c_iv);
        chunk_clear(&s_key);
        chunk_clear(&s_iv);
-       return TRUE;
+       return success;
 }
 
-METHOD(tls_crypto_t, derive_app_secret, bool,
-       private_tls_crypto_t *this)
+METHOD(tls_crypto_t, derive_handshake_keys, bool,
+       private_tls_crypto_t *this, chunk_t shared_secret)
 {
-       chunk_t c_key, c_iv, s_key, s_iv;
-
-       /* client key material */
-       if (!this->hkdf->generate_secret(this->hkdf, TLS_HKDF_C_AP_TRAFFIC,
-                                                                        this->handshake, NULL) ||
-               !this->hkdf->derive_key(this->hkdf, FALSE,
-                                                               this->aead_out->
-                                                               get_encr_key_size(this->aead_out), &c_key) ||
-               !this->hkdf->derive_iv(this->hkdf, FALSE,
-                                                          this->aead_out->
-                                                          get_iv_size(this->aead_out), &c_iv))
-       {
-               DBG1(DBG_TLS, "deriving client key material failed");
-               chunk_clear(&c_key);
-               chunk_clear(&c_iv);
-               return FALSE;
-       }
-
-       /* server key material */
-       if (!this->hkdf->generate_secret(this->hkdf, TLS_HKDF_S_AP_TRAFFIC,
-          this->handshake, NULL) ||
-          !this->hkdf->derive_key(this->hkdf, TRUE, this->aead_in->
-          get_encr_key_size(this->aead_in), &s_key) ||
-          !this->hkdf->derive_iv(this->hkdf, TRUE, this->aead_in->
-          get_iv_size(this->aead_in), &s_iv))
-       {
-               DBG1(DBG_TLS, "deriving server key material failed");
-               chunk_clear(&c_key);
-               chunk_clear(&c_iv);
-               chunk_clear(&s_key);
-               chunk_clear(&s_iv);
-               return FALSE;
-       }
-
-       if (this->tls->is_server(this->tls))
-       {
-               if (!this->aead_in->set_keys(this->aead_in, chunk_empty, s_key, s_iv) ||
-                       !this->aead_out->set_keys(this->aead_out, chunk_empty, c_key, c_iv))
-               {
-                       DBG1(DBG_TLS, "setting aead server key material failed");
-                       chunk_clear(&c_key);
-                       chunk_clear(&c_iv);
-                       chunk_clear(&s_key);
-                       chunk_clear(&s_iv);
-                       return FALSE;
-               }
-       }
-       else
-       {
-               if (!this->aead_in->set_keys(this->aead_in, chunk_empty, s_key, s_iv) ||
-                       !this->aead_out->set_keys(this->aead_out, chunk_empty, c_key, c_iv))
-               {
-                       DBG1(DBG_TLS, "setting aead client key material failed");
-                       chunk_clear(&c_key);
-                       chunk_clear(&c_iv);
-                       chunk_clear(&s_key);
-                       chunk_clear(&s_iv);
-                       return FALSE;
-               }
-       }
+       this->hkdf->set_shared_secret(this->hkdf, shared_secret);
+       return derive_labeled_keys(this, TLS_HKDF_C_HS_TRAFFIC,
+                                                          TLS_HKDF_S_HS_TRAFFIC);
+}
 
-       chunk_clear(&c_key);
-       chunk_clear(&c_iv);
-       chunk_clear(&s_key);
-       chunk_clear(&s_iv);
-       return TRUE;
+METHOD(tls_crypto_t, derive_app_keys, bool,
+       private_tls_crypto_t *this)
+{
+       return derive_labeled_keys(this, TLS_HKDF_C_AP_TRAFFIC,
+                                                          TLS_HKDF_S_AP_TRAFFIC);
 }
 
 METHOD(tls_crypto_t, resume_session, tls_cipher_suite_t,
@@ -2223,8 +2150,8 @@ tls_crypto_t *tls_crypto_create(tls_t *tls, tls_cache_t *cache)
                        .calculate_finished = _calculate_finished,
                        .calculate_finished_tls13 = _calculate_finished_tls13,
                        .derive_secrets = _derive_secrets,
-                       .derive_handshake_secret = _derive_handshake_secret,
-                       .derive_app_secret = _derive_app_secret,
+                       .derive_handshake_keys = _derive_handshake_keys,
+                       .derive_app_keys = _derive_app_keys,
                        .resume_session = _resume_session,
                        .get_session = _get_session,
                        .change_cipher = _change_cipher,

diff --git a/src/libtls/tls_crypto.h b/src/libtls/tls_crypto.h
index 5cd0072..f980095 100644 (file)
--- a/src/libtls/tls_crypto.h
+++ b/src/libtls/tls_crypto.h
@@ -1,4 +1,9 @@
 /*
+ * Copyright (C) 2020 Tobias Brunner
+ * Copyright (C) 2020 Pascal Knecht
+ * Copyright (C) 2020 Méline Sieber
+ * HSR Hochschule fuer Technik Rapperswil
+ *
  * Copyright (C) 2010 Martin Willi
  * Copyright (C) 2010 revosec AG
  *
@@ -514,7 +519,7 @@ struct tls_crypto_t {
                                                         bio_reader_t *reader);
 
        /**
-        * Calculate the data of a legacyTLS finished message.
+        * Calculate the data of a legacy TLS finished message.
         *
         * @param label                 ASCII label to use for calculation
         * @param out                   buffer to write finished data to
@@ -551,14 +556,14 @@ struct tls_crypto_t {
         * @param shared_secret         input key material
         * @return                                      TRUE if secret derived successfully
         */
-       bool (*derive_handshake_secret)(tls_crypto_t *this, chunk_t shared_secret);
+       bool (*derive_handshake_keys)(tls_crypto_t *this, chunk_t shared_secret);
 
        /**
         * Derive the application keys.
         *
         * @return                                      TRUE if secret derived successfully
         */
-       bool (*derive_app_secret)(tls_crypto_t *this);
+       bool (*derive_app_keys)(tls_crypto_t *this);
 
        /**
         * Try to resume a TLS session, derive key material.

diff --git a/src/libtls/tls_peer.c b/src/libtls/tls_peer.c
index 870c755..96ce374 100644 (file)
--- a/src/libtls/tls_peer.c
+++ b/src/libtls/tls_peer.c
@@ -262,22 +262,21 @@ static status_t process_server_hello(private_tls_peer_t *this,
 
        if (this->tls->get_version_max(this->tls) == TLS_1_3)
        {
-               chunk_t shared_secret;
+               chunk_t shared_secret = chunk_empty;
 
-               if (key_type != CURVE_25519 &&
-                       !this->dh->set_other_public_value(this->dh, ext_key_share))
+               if (!this->dh->set_other_public_value(this->dh, ext_key_share) ||
+                       !this->dh->get_shared_secret(this->dh, &shared_secret) ||
+                       !this->crypto->derive_handshake_keys(this->crypto, shared_secret))
                {
-                       DBG2(DBG_TLS, "server key share unable to save");
-               }
-               if (!this->dh->get_shared_secret(this->dh, &shared_secret))
-               {
-                       DBG2(DBG_TLS, "No shared secret key found");
+                       DBG1(DBG_TLS, "DH key derivation failed");
+                       this->alert->add(this->alert, TLS_FATAL, TLS_HANDSHAKE_FAILURE);
+                       chunk_clear(&shared_secret);
+                       return NEED_MORE;
                }
+               chunk_clear(&shared_secret);
 
-               if (!this->crypto->derive_handshake_secret(this->crypto, shared_secret))
-               {
-                       DBG2(DBG_TLS, "derive handshake traffic secret failed");
-               }
+               this->crypto->change_cipher(this->crypto, TRUE);
+               this->crypto->change_cipher(this->crypto, FALSE);
        }
 
        this->state = STATE_HELLO_RECEIVED;
@@ -1537,14 +1536,15 @@ METHOD(tls_handshake_t, build, status_t,
                        case STATE_INIT:
                                return send_client_hello(this, type, writer);
                        case STATE_HELLO_DONE:
-                               /* otherwise fall through to next state */
                        case STATE_CIPHERSPEC_CHANGED_OUT:
                        case STATE_FINISHED_RECEIVED:
-                               /* fall through since legacy TLS and TLS 1.3
-                               * expect the same message */
                                return send_finished(this, type, writer);
                        case STATE_FINISHED_SENT:
-                               this->crypto->derive_app_secret(this->crypto);
+                               if (!this->crypto->derive_app_keys(this->crypto))
+                               {
+                                       this->alert->add(this->alert, TLS_FATAL, TLS_INTERNAL_ERROR);
+                                       return NEED_MORE;
+                               }
                                this->crypto->change_cipher(this->crypto, TRUE);
                                this->crypto->change_cipher(this->crypto, FALSE);
                                this->state = STATE_FINISHED_SENT_KEY_SWITCHED;