Tidying up the OpenSSL private key password logic

Message ID CAF8qwaCC3gEy10G+87OdM8=SBOr8sF74AQc6dhs66WD06g0zQQ@mail.gmail.com
State Accepted
Headers show
Series
  • Tidying up the OpenSSL private key password logic
Related show

Commit Message

David Benjamin Dec. 1, 2017, 11:08 p.m.
Hi folks,

I've attached two patches that I think tidy up the logic around
OpenSSL private key loading and passwords.

The first just removes an unnecessary strdup. That parameter isn't
mutated or anything, it's just a generic data argument to the same
callback that you pass in.

The second avoids using the SSL(_CTX) default password callback
altogether. Since you all use it for one-off calls anyway, it ends up
being a little cumbersome as you must set and unset them. Further, you
end up mutating the SSL_CTX after SSLs have been created, which isn't
generally safe. Rather, I think it's cleaner to just pass the password
into the PEM_read_bio_PrivateKey call yourself. The SSL-level
functions are merely convenience routines on top of this. This also
allows abstracting away the DER/PEM fallback code. (It also avoids a
mess of OpenSSL version variability.)

Note: you probably want to run tests on this. I wasn't sure how to do
that, but I have checked that they compile on my system.

Thoughts?

David

Comments

Jouni Malinen Dec. 9, 2017, 5:31 p.m. | #1
On Fri, Dec 01, 2017 at 06:08:21PM -0500, David Benjamin wrote:
> I've attached two patches that I think tidy up the logic around
> OpenSSL private key loading and passwords.

Thanks!

> The first just removes an unnecessary strdup. That parameter isn't
> mutated or anything, it's just a generic data argument to the same
> callback that you pass in.

I tried to figure out why the strdup() call was there in the first
place, but could not really find any good reason for this.. This came in
with one of the code cleanups in 2004, but the initial implementation
actually used the configuration value directly.

> The second avoids using the SSL(_CTX) default password callback
> altogether. Since you all use it for one-off calls anyway, it ends up
> being a little cumbersome as you must set and unset them. Further, you
> end up mutating the SSL_CTX after SSLs have been created, which isn't
> generally safe. Rather, I think it's cleaner to just pass the password
> into the PEM_read_bio_PrivateKey call yourself. The SSL-level
> functions are merely convenience routines on top of this. This also
> allows abstracting away the DER/PEM fallback code. (It also avoids a
> mess of OpenSSL version variability.)

Yes, this looks much cleaner. I really never liked the need to register
a callback for the simple task of reading a passphrase protected file. I
guess I tried to avoid using the lower layer routines for this, but
based on this patch, this looks so much better that it is justifiable to
use them.

> Note: you probably want to run tests on this. I wasn't sure how to do
> that, but I have checked that they compile on my system.

Seemed to work fine with the couple of hwsim test cases that ended up in
the callback function. I applied both (with some cleanup on the second
one to get rid of the #ifdef blocks in the callers).

Patch

From 821e2b131340a959bdb52a5079eac4932640509a Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben@google.com>
Date: Mon, 18 Sep 2017 11:47:47 -0400
Subject: [PATCH 2/2] OpenSSL: Avoid SSL*_use_default_passwd_cb.

These functions are a bit awkward to use for one-off file loads, as
suggested by the tls_clear_default_passwd_cb logic. There was also some
historical mess with OpenSSL versions and either not having per-SSL
settings, having per-SSL settings but ignoring them, and requiring the
per-SSL settings.

Instead, loading the key with the lower-level functions seems a bit
tidier and also allows abstracting away trying both formats, one after
another.

Signed-off-by: David Benjamin <davidben@google.com>
---
 src/crypto/tls_openssl.c | 121 ++++++++++++++++++++++-------------------------
 1 file changed, 56 insertions(+), 65 deletions(-)

diff --git a/src/crypto/tls_openssl.c b/src/crypto/tls_openssl.c
index df2ce07d2..3d32fc1f7 100644
--- a/src/crypto/tls_openssl.c
+++ b/src/crypto/tls_openssl.c
@@ -2686,16 +2686,6 @@  static int tls_global_client_cert(struct tls_data *data,
 }
 
 
-static int tls_passwd_cb(char *buf, int size, int rwflag, void *password)
-{
-	if (password == NULL) {
-		return 0;
-	}
-	os_strlcpy(buf, (char *) password, size);
-	return os_strlen(buf);
-}
-
-
 #ifdef PKCS12_FUNCS
 static int tls_parse_pkcs12(struct tls_data *data, SSL *ssl, PKCS12 *p12,
 			    const char *passwd)
@@ -3014,21 +3004,60 @@  static int tls_connection_engine_private_key(struct tls_connection *conn)
 }
 
 
-static void tls_clear_default_passwd_cb(SSL_CTX *ssl_ctx, SSL *ssl)
+#ifndef OPENSSL_NO_STDIO
+static int tls_passwd_cb(char *buf, int size, int rwflag, void *password)
 {
-#if OPENSSL_VERSION_NUMBER >= 0x10100000L
-#ifndef LIBRESSL_VERSION_NUMBER
-#ifndef OPENSSL_IS_BORINGSSL
-	if (ssl) {
-		SSL_set_default_passwd_cb(ssl, NULL);
-		SSL_set_default_passwd_cb_userdata(ssl, NULL);
-	}
-#endif /* !BoringSSL */
-#endif /* !LibreSSL */
-#endif /* >= 1.1.0f */
-	SSL_CTX_set_default_passwd_cb(ssl_ctx, NULL);
-	SSL_CTX_set_default_passwd_cb_userdata(ssl_ctx, NULL);
+	if (password == NULL) {
+		return 0;
+	}
+	os_strlcpy(buf, (const char *) password, size);
+	return os_strlen(buf);
+}
+
+
+static int tls_use_private_key_file(struct tls_data *data, SSL *ssl,
+				    const char *private_key,
+				    const char *private_key_passwd) {
+	BIO *bio;
+	EVP_PKEY *pkey;
+	int ret;
+
+	/* First try ASN.1. */
+	bio = BIO_new_file(private_key, "r");
+	if (bio == NULL)
+		return -1;
+	pkey = d2i_PrivateKey_bio(bio, NULL);
+	BIO_free(bio);
+
+	if (pkey != NULL) {
+		wpa_printf(MSG_DEBUG, "OpenSSL: "
+			   "tls_use_private_key_file (DER) --> loaded");
+	} else {
+		/* Try PEM with the provided password. */
+		bio = BIO_new_file(private_key, "r");
+		if (bio == NULL)
+			return -1;
+		pkey = PEM_read_bio_PrivateKey(bio, NULL, tls_passwd_cb,
+					       (void *)private_key_passwd);
+		BIO_free(bio);
+		if (pkey == NULL)
+			return -1;
+		wpa_printf(MSG_DEBUG, "OpenSSL: "
+			   "tls_use_private_key_file (PEM) --> loaded");
+		/* Clear errors from the previous failed load. */
+		ERR_clear_error();
+	}
+
+	if (ssl != NULL) {
+		ret = SSL_use_PrivateKey(ssl, pkey);
+	} else {
+		ret = SSL_CTX_use_PrivateKey(data->ssl, pkey);
+	}
+
+	EVP_PKEY_free(pkey);
+	return ret == 1 ? 0 : -1;
 }
+#endif /* OPENSSL_NO_STDIO */
 
 
 static int tls_connection_private_key(struct tls_data *data,
@@ -3038,30 +3067,11 @@  static int tls_connection_private_key(struct tls_data *data,
 				      const u8 *private_key_blob,
 				      size_t private_key_blob_len)
 {
-	SSL_CTX *ssl_ctx = data->ssl;
 	int ok;
 
 	if (private_key == NULL && private_key_blob == NULL)
 		return 0;
 
-#if OPENSSL_VERSION_NUMBER >= 0x10100000L
-#ifndef LIBRESSL_VERSION_NUMBER
-#ifndef OPENSSL_IS_BORINGSSL
-	/*
-	 * In OpenSSL >= 1.1.0f SSL_use_PrivateKey_file() uses the callback
-	 * from the SSL object. See OpenSSL commit d61461a75253.
-	 */
-	SSL_set_default_passwd_cb(conn->ssl, tls_passwd_cb);
-	SSL_set_default_passwd_cb_userdata(conn->ssl,
-					   (void *)private_key_passwd);
-#endif /* !BoringSSL */
-#endif /* !LibreSSL */
-#endif /* >= 1.1.0f && */
-	/* Keep these for OpenSSL < 1.1.0f */
-	SSL_CTX_set_default_passwd_cb(ssl_ctx, tls_passwd_cb);
-	SSL_CTX_set_default_passwd_cb_userdata(ssl_ctx,
-					       (void *)private_key_passwd);
-
 	ok = 0;
 	while (private_key_blob) {
 		if (SSL_use_PrivateKey_ASN1(EVP_PKEY_RSA, conn->ssl,
@@ -3105,18 +3115,8 @@  static int tls_connection_private_key(struct tls_data *data,
 
 	while (!ok && private_key) {
 #ifndef OPENSSL_NO_STDIO
-		if (SSL_use_PrivateKey_file(conn->ssl, private_key,
-					    SSL_FILETYPE_ASN1) == 1) {
-			wpa_printf(MSG_DEBUG, "OpenSSL: "
-				   "SSL_use_PrivateKey_File (DER) --> OK");
-			ok = 1;
-			break;
-		}
-
-		if (SSL_use_PrivateKey_file(conn->ssl, private_key,
-					    SSL_FILETYPE_PEM) == 1) {
-			wpa_printf(MSG_DEBUG, "OpenSSL: "
-				   "SSL_use_PrivateKey_File (PEM) --> OK");
+		if (tls_use_private_key_file(data, conn->ssl, private_key,
+					     private_key_passwd) == 0) {
 			ok = 1;
 			break;
 		}
@@ -3146,11 +3146,9 @@  static int tls_connection_private_key(struct tls_data *data,
 	if (!ok) {
 		tls_show_errors(MSG_INFO, __func__,
 				"Failed to load private key");
-		tls_clear_default_passwd_cb(ssl_ctx, conn->ssl);
 		return -1;
 	}
 	ERR_clear_error();
-	tls_clear_default_passwd_cb(ssl_ctx, conn->ssl);
 
 	if (!SSL_check_private_key(conn->ssl)) {
 		tls_show_errors(MSG_INFO, __func__, "Private key failed "
@@ -3172,24 +3170,17 @@  static int tls_global_private_key(struct tls_data *data,
 	if (private_key == NULL)
 		return 0;
 
-	SSL_CTX_set_default_passwd_cb(ssl_ctx, tls_passwd_cb);
-	SSL_CTX_set_default_passwd_cb_userdata(ssl_ctx,
-					       (void *)private_key_passwd);
 	if (
 #ifndef OPENSSL_NO_STDIO
-	    SSL_CTX_use_PrivateKey_file(ssl_ctx, private_key,
-					SSL_FILETYPE_ASN1) != 1 &&
-	    SSL_CTX_use_PrivateKey_file(ssl_ctx, private_key,
-					SSL_FILETYPE_PEM) != 1 &&
+	    tls_use_private_key_file(data, NULL, private_key,
+				     private_key_passwd) &&
 #endif /* OPENSSL_NO_STDIO */
 	    tls_read_pkcs12(data, NULL, private_key, private_key_passwd)) {
 		tls_show_errors(MSG_INFO, __func__,
 				"Failed to load private key");
-		tls_clear_default_passwd_cb(ssl_ctx, NULL);
 		ERR_clear_error();
 		return -1;
 	}
-	tls_clear_default_passwd_cb(ssl_ctx, NULL);
 	ERR_clear_error();
 
 	if (!SSL_CTX_check_private_key(ssl_ctx)) {
-- 
2.15.0.531.g2ccb3012c9-goog