diff mbox

EAP-FAST PRF hooks and BoringSSL

Message ID CAF8qwaBwg-vC3KMEuyXMP2q4++zsbcrzoPWjQEoo2P4hhO6v4A@mail.gmail.com
State Accepted
Headers show

Commit Message

David Benjamin May 18, 2016, 5:27 p.m. UTC 
On Tue, May 10, 2016 at 1:28 PM, David Benjamin <davidben@google.com> wrote:
> 1. I can condition the EAP-FAST-only parts of the key derivation
> conditioned on EAP_FAST #defines. With
> df5bde83daaf3c820b6ac1f8d383b16a64ebd2db, we're finally no longer
> defining those symbols, so this would work. I would probably, to make
> this cleaner, split the tls_connection_prf entry-point into two
> functions, one for the reasonable RFC 5705 mechanisms and one for
> EAP-FAST's crazy thing. (Right now the API supports the RFC 5705
> style, but with skip_keyblock set, which is a little odd.)

I've attached the cleanup and #ifdefs I was thinking for this option.
Note: I've only compile-tested this against Android thus far. It's
mostly to clarify what I mean, and I'm sure I messed up somewhere.

I think the first of these two patches is worthwhile in itself. The
EAP-FAST key derivation is very different compared to how new
protocols are supposed to do it (RFC 5705). The split is also more
conducive to, say, OpenSSL maybe providing an API for extracting
EAP-FAST style keys, either by adding BoringSSL's
SSL_generate_key_block API to OpenSSL or perhaps they add an
EAP-FAST-specific key derivation function. I don't think it makes
sense that hostap needs to reimplement the TLS PRF.

You can also see awkwardness in the merged tls_connection_prf; the
skip_keyblock parameter makes no sense unless server_random_first and
label align with the native TLS key derivation. Whereas when
skip_keyblock and server_random_first is 0, this is just a call to
SSL_export_keying_material in OpenSSL. This suggests they should be
two distinct operations.

The second one I don't have strong opinions on. For my purposes, I
would be happy with it, with making BoringSSL use the OpenSSL 1.1.x
path (I'd add the missing functions), or with using the
BoringSSL-specific functions. I'm happy to put together whichever
version you all prefer.

Thoughts?

> 2. For OpenSSL 1.1.x, you all use SSL_CIPHER_get_cipher_nid and
> SSL_CIPHER_get_digest_nid. BoringSSL doesn't have these, but I have no
> problems adding them. (I would not have used NIDs for
> SSL_CIPHER_get_kx_nid and SSL_CIPHER_get_auth_nid but whatever.) It's
> a little odd because the computation is wrong for TLS 1.1+ and we
> don't use EVP_CIPHER in our SSL code anymore. But the code will never
> run anyway, and if it were to later, I could make it match OpenSSL.
>
> 3. For other reasons, we added a pair of functions,
> SSL_get_key_block_len and SSL_generate_key_block which are actually
> exactly what EAP-FAST wants here. I could make
> openssl_get_keyblock_size call SSL_get_key_block_len and even make
> openssl_tls_prf use SSL_generate_key_block. (Probably this would come
> with a similar tls_connection_prf split as in option #1.) But this
> would be BoringSSL-only code to support a feature that doesn't work in
> BoringSSL anyway, so this is of questionable use.
> https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#SSL_get_key_block_len
>
> Do you all have a preference? #2 would be the least invasive option,
> but splitting tls_connection_prf in two for #1 might be preferable, at
> which point you may as well condition on EAP-FAST. In doing so, we can
> probably do away with the fallback from SSL_export_keying_material to
> openssl_tls_prf if SSL_export_keying_material fails (looks like that's
> a remnant of how the #ifdefs worked out while 1.0.0 was supported) and
> not even ship a custom TLS PRF in the no-EAP-FAST config. I'm happy to
> do a bit of cleanup for you along the way here.
>
> David

Comments

Jouni Malinen May 23, 2016, 9:19 p.m. UTC | #1 
On Wed, May 18, 2016 at 01:27:17PM -0400, David Benjamin wrote:
> I've attached the cleanup and #ifdefs I was thinking for this option.
> Note: I've only compile-tested this against Android thus far. It's
> mostly to clarify what I mean, and I'm sure I messed up somewhere.

Thanks, applied with some fixes (GnuTLS one did not compile and Android
build with produced warnings about unused functions) and cleanup.
diff mbox

Patch

From 415c00bdbad6498137300230bfb9597c70ff288e Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben@google.com>
Date: Tue, 17 May 2016 13:24:43 -0400
Subject: [PATCH 2/2] OpenSSL: Don't implement tls_connection_get_eap_fast_key
 if EAP-FAST is disabled.

This avoids internal access of structs and also removes the dependency on the
reimplemented TLS PRF functions when EAP-FAST support is not enabled. Notably,
BoringSSL doesn't support EAP-FAST, so there is no need access its internals
with openssl_get_keyblock_size.

Signed-Off-By: David Benjamin <davidben@google.com>
---
 src/crypto/tls_openssl.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/crypto/tls_openssl.c b/src/crypto/tls_openssl.c
index e53cf9d..f880ea7 100644
--- a/src/crypto/tls_openssl.c
+++ b/src/crypto/tls_openssl.c
@@ -3087,8 +3087,9 @@  int tls_connection_get_random(void *ssl_ctx, struct tls_connection *conn,
 	return 0;
 }
 
-
-#ifndef CONFIG_FIPS
+#if !defined(CONFIG_FIPS) ||                             \
+    (!defined(EAP_FAST) && !defined(EAP_FAST_DYNAMIC) && \
+     !defined(EAP_SERVER_FAST))
 static int openssl_get_keyblock_size(SSL *ssl)
 {
 #if OPENSSL_VERSION_NUMBER < 0x10100000L || defined(LIBRESSL_VERSION_NUMBER)
@@ -3143,7 +3144,7 @@  static int openssl_get_keyblock_size(SSL *ssl)
 		    EVP_CIPHER_iv_length(c));
 #endif
 }
-#endif /* CONFIG_FIPS */
+#endif /* CONFIG_FIPS || !EAP_FAST */
 
 
 int tls_connection_export_key(void *tls_ctx, struct tls_connection *conn,
@@ -3162,11 +3163,13 @@  int tls_connection_export_key(void *tls_ctx, struct tls_connection *conn,
 int tls_connection_get_eap_fast_key(void *tls_ctx, struct tls_connection *conn,
 				    u8 *out, size_t out_len)
 {
-#ifdef CONFIG_FIPS
+#if !defined(EAP_FAST) && !defined(EAP_FAST_DYNAMIC) && !defined(EAP_SERVER_FAST)
+	return -1;
+#elif defined(CONFIG_FIPS)
 	wpa_printf(MSG_ERROR, "OpenSSL: TLS keys cannot be exported in FIPS "
 		   "mode");
 	return -1;
-#else /* CONFIG_FIPS */
+#else
 	SSL *ssl;
 	SSL_SESSION *sess;
 	u8 *rnd;
@@ -3235,7 +3238,7 @@  int tls_connection_get_eap_fast_key(void *tls_ctx, struct tls_connection *conn,
 	bin_clear_free(tmp_out, skip);
 
 	return ret;
-#endif /* CONFIG_FIPS */
+#endif
 }
 
 
-- 
2.8.0.rc3.226.g39d4020