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
From b80a2911d0a5d9719e3566f2a487012ddd5eaa45 Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben@xxxxxxxxxx>
Date: Mon, 18 Sep 2017 00:33:43 -0400
Subject: [PATCH 1/2] OpenSSL: Remove unnecessary os_strdup.
There's no need to make an extra copy of private_key_passwd.
Signed-Off-By: David Benjamin <davidben@xxxxxxxxxx>
---
src/crypto/tls_openssl.c | 38 +++++++++++---------------------------
1 file changed, 11 insertions(+), 27 deletions(-)
diff --git a/src/crypto/tls_openssl.c b/src/crypto/tls_openssl.c
index ada88a9d3..df2ce07d2 100644
--- a/src/crypto/tls_openssl.c
+++ b/src/crypto/tls_openssl.c
@@ -3039,19 +3039,11 @@ static int tls_connection_private_key(struct tls_data *data,
size_t private_key_blob_len)
{
SSL_CTX *ssl_ctx = data->ssl;
- char *passwd;
int ok;
if (private_key == NULL && private_key_blob == NULL)
return 0;
- if (private_key_passwd) {
- passwd = os_strdup(private_key_passwd);
- if (passwd == NULL)
- return -1;
- } else
- passwd = NULL;
-
#if OPENSSL_VERSION_NUMBER >= 0x10100000L
#ifndef LIBRESSL_VERSION_NUMBER
#ifndef OPENSSL_IS_BORINGSSL
@@ -3060,13 +3052,15 @@ static int tls_connection_private_key(struct tls_data *data,
* 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, passwd);
+ 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, passwd);
+ SSL_CTX_set_default_passwd_cb_userdata(ssl_ctx,
+ (void *)private_key_passwd);
ok = 0;
while (private_key_blob) {
@@ -3098,7 +3092,8 @@ static int tls_connection_private_key(struct tls_data *data,
}
if (tls_read_pkcs12_blob(data, conn->ssl, private_key_blob,
- private_key_blob_len, passwd) == 0) {
+ private_key_blob_len,
+ private_key_passwd) == 0) {
wpa_printf(MSG_DEBUG, "OpenSSL: PKCS#12 as blob --> "
"OK");
ok = 1;
@@ -3130,8 +3125,8 @@ static int tls_connection_private_key(struct tls_data *data,
__func__);
#endif /* OPENSSL_NO_STDIO */
- if (tls_read_pkcs12(data, conn->ssl, private_key, passwd)
- == 0) {
+ if (tls_read_pkcs12(data, conn->ssl, private_key,
+ private_key_passwd) == 0) {
wpa_printf(MSG_DEBUG, "OpenSSL: Reading PKCS#12 file "
"--> OK");
ok = 1;
@@ -3152,12 +3147,10 @@ static int tls_connection_private_key(struct tls_data *data,
tls_show_errors(MSG_INFO, __func__,
"Failed to load private key");
tls_clear_default_passwd_cb(ssl_ctx, conn->ssl);
- os_free(passwd);
return -1;
}
ERR_clear_error();
tls_clear_default_passwd_cb(ssl_ctx, conn->ssl);
- os_free(passwd);
if (!SSL_check_private_key(conn->ssl)) {
tls_show_errors(MSG_INFO, __func__, "Private key failed "
@@ -3175,20 +3168,13 @@ static int tls_global_private_key(struct tls_data *data,
const char *private_key_passwd)
{
SSL_CTX *ssl_ctx = data->ssl;
- char *passwd;
if (private_key == NULL)
return 0;
- if (private_key_passwd) {
- passwd = os_strdup(private_key_passwd);
- if (passwd == NULL)
- return -1;
- } else
- passwd = NULL;
-
SSL_CTX_set_default_passwd_cb(ssl_ctx, tls_passwd_cb);
- SSL_CTX_set_default_passwd_cb_userdata(ssl_ctx, passwd);
+ 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,
@@ -3196,16 +3182,14 @@ static int tls_global_private_key(struct tls_data *data,
SSL_CTX_use_PrivateKey_file(ssl_ctx, private_key,
SSL_FILETYPE_PEM) != 1 &&
#endif /* OPENSSL_NO_STDIO */
- tls_read_pkcs12(data, NULL, private_key, passwd)) {
+ 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);
- os_free(passwd);
ERR_clear_error();
return -1;
}
tls_clear_default_passwd_cb(ssl_ctx, NULL);
- os_free(passwd);
ERR_clear_error();
if (!SSL_CTX_check_private_key(ssl_ctx)) {
--
2.15.0.531.g2ccb3012c9-goog
From 821e2b131340a959bdb52a5079eac4932640509a Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
