Fix the TLSv1.3 key schedule implementation.

When the RFC refers to ("") for key derivation, it is referring to the
transcript hash of an empty string, not an empty string. Rename
tls13_secrets_new() to tls13_secrets_create(), make it take an EVP_MD *
and calculate the hash of an empty string so that we have it available
for the "derived" and other steps. Merge tls13_secrets_init() into
the same function, remove the EVP_MD * from other functions and use the
empty string hash at the appropriate places.

ok beck@ tb@

2 files changed, 95 insertions, 66 deletions

diff --git a/lib/libssl/tls13_internal.h b/lib/libssl/tls13_internal.h
index cad769a1bf9..83f99881406 100644
--- a/lib/libssl/tls13_internal.h
+++ b/lib/libssl/tls13_internal.h
@@ -1,7 +1,7 @@
-/* $OpenBSD: tls13_internal.h,v 1.4 2018/11/09 03:07:26 jsing Exp $ */
+/* $OpenBSD: tls13_internal.h,v 1.5 2018/11/09 23:56:20 jsing Exp $ */
 /*
- * Copyright (c) 2018, Bob Beck <beck@openbsd.org>
- * Copyright (c) 2018, Theo Buehler <tb@openbsd.org>
+ * Copyright (c) 2018 Bob Beck <beck@openbsd.org>
+ * Copyright (c) 2018 Theo Buehler <tb@openbsd.org>
  *
  * Permission to use, copy, modify, and/or distribute this software for any
  * purpose with or without fee is hereby granted, provided that the above
@@ -30,6 +30,7 @@ struct tls13_secret {
 
 /* RFC 8446 Section 7.1  Page 92 */
 struct tls13_secrets {
+	const EVP_MD *digest;
 	int resumption;
 	int init_done;
 	int early_done;
@@ -37,6 +38,7 @@ struct tls13_secrets {
 	int schedule_done;
 	int insecure; /* Set by tests */
 	struct tls13_secret zeros;
+	struct tls13_secret empty_hash;
 	struct tls13_secret extracted_early;
 	struct tls13_secret binder_key;
 	struct tls13_secret client_early_traffic;
@@ -53,18 +55,20 @@ struct tls13_secrets {
 	struct tls13_secret resumption_master;
 };
 
-struct tls13_secrets *tls13_secrets_new(size_t hash_length);
-void tls13_secrets_init(struct tls13_secrets *secrets, int resumption);
+struct tls13_secrets *tls13_secrets_create(const EVP_MD *digest,
+    int resumption);
 void tls13_secrets_destroy(struct tls13_secrets *secrets);
 
-int tls13_derive_early_secrets(struct tls13_secrets *secrets,
-    const EVP_MD *digest,uint8_t *psk, size_t psk_len,
+int tls13_hkdf_expand_label(struct tls13_secret *out, const EVP_MD *digest,
+    const struct tls13_secret *secret, const char *label,
     const struct tls13_secret *context);
+
+int tls13_derive_early_secrets(struct tls13_secrets *secrets, uint8_t *psk,
+    size_t psk_len, const struct tls13_secret *context);
 int tls13_derive_handshake_secrets(struct tls13_secrets *secrets,
-    const EVP_MD *digest, const uint8_t *ecdhe, size_t ecdhe_len,
-    const struct tls13_secret *context);
+    const uint8_t *ecdhe, size_t ecdhe_len, const struct tls13_secret *context);
 int tls13_derive_application_secrets(struct tls13_secrets *secrets,
-    const EVP_MD *digest, const struct tls13_secret *context);
+    const struct tls13_secret *context);
 
 struct tls13_ctx;
 
diff --git a/lib/libssl/tls13_key_schedule.c b/lib/libssl/tls13_key_schedule.c
index 6984d20730a..f20e9b741bf 100644
--- a/lib/libssl/tls13_key_schedule.c
+++ b/lib/libssl/tls13_key_schedule.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: tls13_key_schedule.c,v 1.3 2018/11/08 23:50:54 beck Exp $ */
+/* $OpenBSD: tls13_key_schedule.c,v 1.4 2018/11/09 23:56:20 jsing Exp $ */
 /* Copyright (c) 2018, Bob Beck <beck@openbsd.org>
  *
  * Permission to use, copy, modify, and/or distribute this software for any
@@ -30,6 +30,7 @@ tls13_secrets_destroy(struct tls13_secrets *secrets)
 
 	/* you can never be too sure :) */
 	freezero(secrets->zeros.data, secrets->zeros.len);
+	freezero(secrets->empty_hash.data, secrets->empty_hash.len);
 
 	freezero(secrets->extracted_early.data,
 	    secrets->extracted_early.len);
@@ -65,12 +66,17 @@ tls13_secrets_destroy(struct tls13_secrets *secrets)
 
 /*
  * Allocate a set of secrets for a key schedule using
- * a size of hash_length from RFC 8446 section 7.1
+ * a size of hash_length from RFC 8446 section 7.1.
  */
 struct tls13_secrets *
-tls13_secrets_new(size_t hash_length)
+tls13_secrets_create(const EVP_MD *digest, int resumption)
 {
 	struct tls13_secrets *secrets = NULL;
+	EVP_MD_CTX *mdctx = NULL;
+	unsigned int mdlen;
+	size_t hash_length;
+
+	hash_length = EVP_MD_size(digest);
 
 	if ((secrets = calloc(1, sizeof(struct tls13_secrets))) == NULL)
 		goto err;
@@ -79,6 +85,10 @@ tls13_secrets_new(size_t hash_length)
 		goto err;
 	secrets->zeros.len = hash_length;
 
+	if ((secrets->empty_hash.data = malloc(hash_length)) == NULL)
+		goto err;
+	secrets->empty_hash.len = hash_length;
+
 	if ((secrets->extracted_early.data = malloc(hash_length)) == NULL)
 		goto err;
 	secrets->extracted_early.len = hash_length;
@@ -130,20 +140,37 @@ tls13_secrets_new(size_t hash_length)
 		goto err;
 	secrets->resumption_master.len = hash_length;
 
+	/*
+	 * Calculate the hash of a zero-length string - this is needed during
+	 * the "derived" step for key extraction.
+	 */
+	if ((mdctx = EVP_MD_CTX_new()) == NULL)
+		goto err;
+	if (!EVP_DigestInit_ex(mdctx, digest, NULL))
+		goto err;
+	if (!EVP_DigestUpdate(mdctx, secrets->zeros.data, 0))
+		goto err;
+	if (!EVP_DigestFinal_ex(mdctx, secrets->empty_hash.data, &mdlen))
+		goto err;
+	EVP_MD_CTX_free(mdctx);
+
+	if (secrets->empty_hash.len != mdlen)
+		goto err;
+
+	secrets->digest = digest;
+	secrets->resumption = resumption;
+	secrets->init_done = 1;
+
 	return secrets;
+
  err:
 	tls13_secrets_destroy(secrets);
-	return NULL;
-}
+	EVP_MD_CTX_free(mdctx);
 
-void
-tls13_secrets_init(struct tls13_secrets *secrets, int resumption)
-{
-	secrets->resumption = resumption;
-	secrets->init_done = 1;
+	return NULL;
 }
 
-static int
+int
 tls13_hkdf_expand_label(struct tls13_secret *out, const EVP_MD *digest,
     const struct tls13_secret *secret, const char *label,
     const struct tls13_secret *context)
@@ -173,6 +200,7 @@ tls13_hkdf_expand_label(struct tls13_secret *out, const EVP_MD *digest,
 
 	ret = HKDF_expand(out->data, out->len, digest, secret->data,
 	    secret->len, hkdf_label, hkdf_label_len);
+
 	free(hkdf_label);
 	return(ret);
  err:
@@ -188,9 +216,8 @@ tls13_derive_secret(struct tls13_secret *out, const EVP_MD *digest,
 	return tls13_hkdf_expand_label(out, digest, secret, label, context);
 }
 
-
 int
-tls13_derive_early_secrets(struct tls13_secrets *secrets,  const EVP_MD *digest,
+tls13_derive_early_secrets(struct tls13_secrets *secrets,
     uint8_t *psk, size_t psk_len, const struct tls13_secret *context)
 {
 	struct tls13_secret binder_context;
@@ -204,7 +231,7 @@ tls13_derive_early_secrets(struct tls13_secrets *secrets,  const EVP_MD *digest,
 	if ((mdctx = EVP_MD_CTX_new()) == NULL)
 		return 0;
 
-	if (!EVP_DigestInit_ex(mdctx, digest, NULL) ||
+	if (!EVP_DigestInit_ex(mdctx, secrets->digest, NULL) ||
 	    !EVP_DigestUpdate(mdctx, secrets->zeros.data, secrets->zeros.len) ||
 	    !EVP_DigestFinal_ex(mdctx, binder_context_data,
 		&binder_context_len)) {
@@ -220,29 +247,29 @@ tls13_derive_early_secrets(struct tls13_secrets *secrets,  const EVP_MD *digest,
 		return 0;
 
 	if (!HKDF_extract(secrets->extracted_early.data,
-	    &secrets->extracted_early.len, digest, psk, psk_len,
+	    &secrets->extracted_early.len, secrets->digest, psk, psk_len,
 	    secrets->zeros.data, secrets->zeros.len))
 		return 0;
 
 	if (secrets->extracted_early.len != secrets->zeros.len)
 		return 0;
 
-	if (!tls13_derive_secret(&secrets->binder_key,
-	    digest, &secrets->extracted_early,
+	if (!tls13_derive_secret(&secrets->binder_key, secrets->digest,
+	    &secrets->extracted_early,
 	    secrets->resumption ? "res binder" : "ext binder",
 	    &binder_context))
 		return 0;
-
 	if (!tls13_derive_secret(&secrets->client_early_traffic,
-	    digest, &secrets->extracted_early, "c e traffic", context))
+	    secrets->digest, &secrets->extracted_early, "c e traffic",
+	    context))
 		return 0;
-
 	if (!tls13_derive_secret(&secrets->early_exporter_master,
-	    digest, &secrets->extracted_early, "e exp master", context))
+	    secrets->digest, &secrets->extracted_early, "e exp master",
+	    context))
 		return 0;
-
 	if (!tls13_derive_secret(&secrets->derived_early,
-	    digest, &secrets->extracted_early, "derived", context))
+	    secrets->digest, &secrets->extracted_early, "derived",
+	    &secrets->empty_hash))
 		return 0;
 
 	/* RFC 8446 recommends */
@@ -255,7 +282,7 @@ tls13_derive_early_secrets(struct tls13_secrets *secrets,  const EVP_MD *digest,
 
 int
 tls13_derive_handshake_secrets(struct tls13_secrets *secrets,
-    const EVP_MD *digest, const uint8_t *ecdhe, size_t ecdhe_len,
+    const uint8_t *ecdhe, size_t ecdhe_len,
     const struct tls13_secret *context)
 {
 	if (!secrets->init_done || !secrets->early_done ||
@@ -263,8 +290,8 @@ tls13_derive_handshake_secrets(struct tls13_secrets *secrets,
 		return 0;
 
 	if (!HKDF_extract(secrets->extracted_handshake.data,
-	    &secrets->extracted_handshake.len,
-	    digest, ecdhe, ecdhe_len, secrets->derived_early.data,
+	    &secrets->extracted_handshake.len, secrets->digest,
+	    ecdhe, ecdhe_len, secrets->derived_early.data,
 	    secrets->derived_early.len))
 		return 0;
 
@@ -277,36 +304,40 @@ tls13_derive_handshake_secrets(struct tls13_secrets *secrets,
 		    secrets->derived_early.len);
 
 	if (!tls13_derive_secret(&secrets->client_handshake_traffic,
-	    digest, &secrets->extracted_handshake, "c hs traffic", context))
+	    secrets->digest, &secrets->extracted_handshake, "c hs traffic",
+	    context))
 		return 0;
 	if (!tls13_derive_secret(&secrets->server_handshake_traffic,
-	    digest, &secrets->extracted_handshake, "s hs traffic", context))
+	    secrets->digest, &secrets->extracted_handshake, "s hs traffic",
+	    context))
 		return 0;
 	if (!tls13_derive_secret(&secrets->derived_handshake,
-	    digest, &secrets->extracted_handshake, "derived", context))
+	    secrets->digest, &secrets->extracted_handshake, "derived",
+	    context))
 		return 0;
 
 	/* RFC 8446 recommends */
 	if (!secrets->insecure)
 		explicit_bzero(secrets->extracted_handshake.data,
 		    secrets->extracted_handshake.len);
+
 	secrets->handshake_done = 1;
+
 	return 1;
 }
 
 int
 tls13_derive_application_secrets(struct tls13_secrets *secrets,
-    const EVP_MD *digest, const struct tls13_secret *context)
+    const struct tls13_secret *context)
 {
 	if (!secrets->init_done || !secrets->early_done ||
 	    !secrets->handshake_done || secrets->schedule_done)
 		return 0;
 
 	if (!HKDF_extract(secrets->extracted_master.data,
-		&secrets->extracted_master.len,
-		digest, secrets->zeros.data, secrets->zeros.len, // XXX ?
-		secrets->derived_handshake.data,
-		secrets->derived_handshake.len))
+	    &secrets->extracted_master.len, secrets->digest,
+	    secrets->zeros.data, secrets->zeros.len,
+	    secrets->derived_handshake.data, secrets->derived_handshake.len))
 		return 0;
 
 	if (secrets->extracted_master.len != secrets->zeros.len)
@@ -318,58 +349,52 @@ tls13_derive_application_secrets(struct tls13_secrets *secrets,
 		    secrets->derived_handshake.len);
 
 	if (!tls13_derive_secret(&secrets->client_application_traffic,
-	    digest, &secrets->extracted_master, "c ap traffic", context))
+	    secrets->digest, &secrets->extracted_master, "c ap traffic",
+	    context))
 		return 0;
 	if (!tls13_derive_secret(&secrets->server_application_traffic,
-	    digest, &secrets->extracted_master, "s ap traffic", context))
+	    secrets->digest, &secrets->extracted_master, "s ap traffic",
+	    context))
 		return 0;
 	if (!tls13_derive_secret(&secrets->exporter_master,
-	    digest, &secrets->extracted_master, "exp master", context))
+	    secrets->digest, &secrets->extracted_master, "exp master",
+	    context))
 		return 0;
 	if (!tls13_derive_secret(&secrets->resumption_master,
-	    digest, &secrets->extracted_master, "res master", context))
+	    secrets->digest, &secrets->extracted_master, "res master",
+	    context))
 		return 0;
 
 	/* RFC 8446 recommends */
 	if (!secrets->insecure)
 		explicit_bzero(secrets->extracted_master.data,
 		    secrets->extracted_master.len);
+
 	secrets->schedule_done = 1;
+
 	return 1;
 }
 
 int
-tls13_update_client_traffic_secret(struct tls13_secrets *secrets,
-    const EVP_MD *digest)
+tls13_update_client_traffic_secret(struct tls13_secrets *secrets)
 {
-	const struct tls13_secret empty = {
-		.data = "",
-		.len = 0,
-	};
-
 	if (!secrets->init_done || !secrets->early_done ||
 	    !secrets->handshake_done || !secrets->schedule_done)
 		return 0;
 
 	return tls13_hkdf_expand_label(&secrets->client_application_traffic,
-	    digest, &secrets->client_application_traffic, "traffic upd",
-	    &empty);
+	    secrets->digest, &secrets->client_application_traffic,
+	    "traffic upd", &secrets->empty_hash);
 }
 
 int
-tls13_update_server_traffic_secret(struct tls13_secrets *secrets,
-    const EVP_MD *digest)
+tls13_update_server_traffic_secret(struct tls13_secrets *secrets)
 {
-	const struct tls13_secret empty = {
-		.data = "",
-		.len = 0,
-	};
-
 	if (!secrets->init_done || !secrets->early_done ||
 	    !secrets->handshake_done || !secrets->schedule_done)
 		return 0;
 
 	return tls13_hkdf_expand_label(&secrets->server_application_traffic,
-	    digest, &secrets->server_application_traffic, "traffic upd",
-	    &empty);
+	    secrets->digest, &secrets->server_application_traffic,
+	    "traffic upd", &secrets->empty_hash);
 }