diff mbox series

DPP: Allow loading bootstrap keys using an OpenSSL engine

Message ID CH2PR21MB14456ADF3FF22106E15325FCC2769@CH2PR21MB1445.namprd21.prod.outlook.com
State Changes Requested
Headers show
Series DPP: Allow loading bootstrap keys using an OpenSSL engine | expand

Commit Message

Andrew Beltrano April 6, 2021, 4:03 p.m. UTC 
Add ability to load a DPP bootstrap key-pair using an arbitrary OpenSSL
engine instead of requiring the private key to be specified explicitly.
The engine name, so path, and key identifier must be specified to
enable loading a key using an OpenSSL engine. The key identifier is
an engine-specific field used to identify the key to load.

Explicit private keys, if specified, take precedence over OpenSSL
engine-based keys.

Signed-off-by: Andrew Beltrano <anbeltra@microsoft.com>
---
Note that this patch depends on the patch-set titled 'Expose OpenSSL dynamic
engine loading globally'.

 hostapd/hostapd_cli.c    |   2 +-
 src/common/dpp.c         |  19 ++++++++
 src/common/dpp.h         |   9 ++++
 src/common/dpp_crypto.c  | 101 +++++++++++++++++++++++++++++++++++++++
 src/common/dpp_i.h       |   7 +++
 wpa_supplicant/wpa_cli.c |   2 +-
 6 files changed, 138 insertions(+), 2 deletions(-)

Comments

Jouni Malinen Nov. 8, 2021, 6:57 p.m. UTC | #1 
On Tue, Apr 06, 2021 at 04:03:48PM +0000, Andrew Beltrano wrote:
> Add ability to load a DPP bootstrap key-pair using an arbitrary OpenSSL
> engine instead of requiring the private key to be specified explicitly.
> The engine name, so path, and key identifier must be specified to
> enable loading a key using an OpenSSL engine. The key identifier is
> an engine-specific field used to identify the key to load.
> 
> Explicit private keys, if specified, take precedence over OpenSSL
> engine-based keys.

> diff --git a/src/common/dpp.c b/src/common/dpp.c
> @@ -180,6 +180,13 @@ void dpp_bootstrap_info_free(struct dpp_bootstrap_info *info)
> +#ifndef OPENSSL_NO_ENGINE
> +	os_free(info->key_id);
> +	os_free(info->engine_id);
> +	os_free(info->engine_path);
> +	if (info->engine)
> +		ENGINE_finish(info->engine);
> +#endif /* OPENSSL_NO_ENGINE */
>  	EVP_PKEY_free(info->pubkey);
>  	str_clear_free(info->configurator_params);
>  	os_free(info);

I'd prefer to move most of the OpenSSL specific items away from dpp.c
(as was actually done with another patch for the items shown here in the
earlier code snapshot). Maybe that info->engine could be a new context
struct from src/crypto/crypto.h with the key_id/engine_id/engine_path
being "hidden" within that struct just like the OpenSSL specific
reference within crypto_openssl.c.

> @@ -3893,6 +3900,11 @@ int dpp_bootstrap_gen(struct dpp_global *dpp, const char *cmd)
>  	info = get_param(cmd, " info=");
>  	curve = get_param(cmd, " curve=");
>  	key = get_param(cmd, " key=");
> +#ifndef OPENSSL_NO_ENGINE
> +	bi->key_id = get_param(cmd, " key_id=");
> +	bi->engine_id = get_param(cmd, " engine=");
> +	bi->engine_path = get_param(cmd, " engine_path=");
> +#endif /* OPENSSL_NO_ENGINE */

This could be done without that OPENSSL_NO_ENGINE.

> +#ifndef OPENSSL_NO_ENGINE
> +	else if (bi->key_id) {
> +		bi->engine = dpp_load_engine(bi->engine_id, bi->engine_path);
> +		if (!bi->engine)
> +			goto fail;
> +	}
> +#endif /* OPENSSL_NO_ENGINE */

And I guess this could also get rid of the OPENSSL_NO_ENGINE use and
replace dpp_load_engine() with something like crypto_load_engine(). It
might also be more convenient to provide key_id to that function so that
all such crypto implementation specific parameters would be within the
same context and dpp_load_keypair() would not need to pass the key_id
separately.

> diff --git a/src/common/dpp.h b/src/common/dpp.h
> +#ifndef OPENSSL_NO_ENGINE
> +#include <openssl/engine.h>
> +#endif /* OPENSSL_NO_ENGINE */ 

> @@ -166,6 +169,12 @@ struct dpp_bootstrap_info {
> +#ifndef OPENSSL_NO_ENGINE
> +	char *key_id;
> +	char *engine_id;
> +	char *engine_path;
> +	ENGINE *engine;
> +#endif /* OPENSSL_NO_ENGINE */
>  };

So that could be replaced with struct crypto_engine (etc.) so that all
the OpenSSL specific stuff could be in openssl_engine.c (or
crypto_openssl.c if the Makefile conditions do not work easily for
adding that file based on OPENSSL_NO_ENGINE).

> diff --git a/src/common/dpp_crypto.c b/src/common/dpp_crypto.c
> +#ifndef OPENSSL_NO_ENGINE
> +static EVP_PKEY * dpp_load_keypair(const struct dpp_curve_params **curve,
> +				  ENGINE *engine, const char *key_id)
> +{

EVP_PKEY/ENGINE could be replace with struct crypto_ec_key/crypto_engine
now.

> +	EVP_PKEY *pkey;
> +	EC_KEY *eckey;
> +	const EC_GROUP *group;
> +	int nid;
> +
> +	pkey = ENGINE_load_private_key(engine, key_id, NULL, NULL);

And most of the fucntion contents moved into the OpenSSL specific file.
Or well, I guess the full function could go there since there would not
really be much need for an extra wrapper on top of that.

> +static int dpp_openssl_engine_load_dynamic(const char *engine_id,
> +			const char *engine_path)
> +{
> +	const char *pre_cmd[] = {
> +		"SO_PATH", engine_path,
> +		"ID", engine_id,
> +		"LIST_ADD", "1",
> +		"LOAD", NULL,
> +		NULL, NULL
> +	};
> +	const char *post_cmd[] = {
> +		NULL, NULL
> +	};
> +
> +	if (!engine_id || !engine_path)
> +		return 0;
> +
> +	wpa_printf(MSG_DEBUG, "ENGINE: Loading %s Engine from %s",
> +		   engine_id, engine_path);
> +
> +	return openssl_engine_load_dynamic_generic(pre_cmd, post_cmd, engine_id);
> +}

I'm not sure what would be the cleanest approach for this function.
Maybe this should all go into the OpenSSL specific files even though
this does not strictly speaking call any OpenSSL library specific
function, but that last function there is still a crypto library
specific one.

> +ENGINE * dpp_load_engine(const char *engine_id, const char *engine_path)
> +{
> +	if (dpp_openssl_engine_load_dynamic(engine_id, engine_path) < 0)
> +		return NULL;
> +
> +	ENGINE *engine = ENGINE_by_id(engine_id);
> +	if (!engine) {
> +		wpa_printf(MSG_ERROR, "ENGINE: engine %s not available [%s]",
> +			engine_id, ERR_error_string(ERR_get_error(), NULL));
> +		return NULL;
> +	}
> +
> +	if (ENGINE_init(engine) != 1) {
> +		wpa_printf(MSG_ERROR, "ENGINE: engine init failed "
> +			"(engine: %s) [%s]", engine_id,
> +			ERR_error_string(ERR_get_error(), NULL));
> +		ENGINE_free(engine);
> +		return NULL;
> +	}
> +
> +	return engine;
> +}

And this would move into openssl_engine/crypto_openssl.c as well.
diff mbox series

Patch

diff --git a/hostapd/hostapd_cli.c b/hostapd/hostapd_cli.c
index eaa628ad0..62d948680 100644
--- a/hostapd/hostapd_cli.c
+++ b/hostapd/hostapd_cli.c
@@ -1690,7 +1690,7 @@  static const struct hostapd_cli_cmd hostapd_cli_commands[] = {
 	{ "dpp_qr_code", hostapd_cli_cmd_dpp_qr_code, NULL,
 	  "report a scanned DPP URI from a QR Code" },
 	{ "dpp_bootstrap_gen", hostapd_cli_cmd_dpp_bootstrap_gen, NULL,
-	  "type=<qrcode> [chan=..] [mac=..] [info=..] [curve=..] [key=..] = generate DPP bootstrap information" },
+	  "type=<qrcode> [chan=..] [mac=..] [info=..] [curve=..] [key=..] [key_id=..] [engine=..] [engine_path=..] = generate DPP bootstrap information" },
 	{ "dpp_bootstrap_remove", hostapd_cli_cmd_dpp_bootstrap_remove, NULL,
 	  "*|<id> = remove DPP bootstrap information" },
 	{ "dpp_bootstrap_get_uri", hostapd_cli_cmd_dpp_bootstrap_get_uri, NULL,
diff --git a/src/common/dpp.c b/src/common/dpp.c
index 3c8c7682d..a7468ca91 100644
--- a/src/common/dpp.c
+++ b/src/common/dpp.c
@@ -180,6 +180,13 @@  void dpp_bootstrap_info_free(struct dpp_bootstrap_info *info)
 	os_free(info->info);
 	os_free(info->chan);
 	os_free(info->pk);
+#ifndef OPENSSL_NO_ENGINE
+	os_free(info->key_id);
+	os_free(info->engine_id);
+	os_free(info->engine_path);
+	if (info->engine)
+		ENGINE_finish(info->engine);
+#endif /* OPENSSL_NO_ENGINE */
 	EVP_PKEY_free(info->pubkey);
 	str_clear_free(info->configurator_params);
 	os_free(info);
@@ -3893,6 +3900,11 @@  int dpp_bootstrap_gen(struct dpp_global *dpp, const char *cmd)
 	info = get_param(cmd, " info=");
 	curve = get_param(cmd, " curve=");
 	key = get_param(cmd, " key=");
+#ifndef OPENSSL_NO_ENGINE
+	bi->key_id = get_param(cmd, " key_id=");
+	bi->engine_id = get_param(cmd, " engine=");
+	bi->engine_path = get_param(cmd, " engine_path=");
+#endif /* OPENSSL_NO_ENGINE */
 
 	if (key) {
 		privkey_len = os_strlen(key) / 2;
@@ -3901,6 +3913,13 @@  int dpp_bootstrap_gen(struct dpp_global *dpp, const char *cmd)
 		    hexstr2bin(key, privkey, privkey_len) < 0)
 			goto fail;
 	}
+#ifndef OPENSSL_NO_ENGINE
+	else if (bi->key_id) {
+		bi->engine = dpp_load_engine(bi->engine_id, bi->engine_path);
+		if (!bi->engine)
+			goto fail;
+	}
+#endif /* OPENSSL_NO_ENGINE */
 
 	if (dpp_keygen(bi, curve, privkey, privkey_len) < 0 ||
 	    dpp_parse_uri_chan_list(bi, bi->chan) < 0 ||
diff --git a/src/common/dpp.h b/src/common/dpp.h
index 65ee905a7..d5c062f82 100644
--- a/src/common/dpp.h
+++ b/src/common/dpp.h
@@ -12,6 +12,9 @@ 
 
 #ifdef CONFIG_DPP
 #include <openssl/x509.h>
+#ifndef OPENSSL_NO_ENGINE
+#include <openssl/engine.h>
+#endif /* OPENSSL_NO_ENGINE */ 
 
 #include "utils/list.h"
 #include "common/wpa_common.h"
@@ -166,6 +169,12 @@  struct dpp_bootstrap_info {
 	int nfc_negotiated; /* whether this has been used in NFC negotiated
 			     * connection handover */
 	char *configurator_params;
+#ifndef OPENSSL_NO_ENGINE
+	char *key_id;
+	char *engine_id;
+	char *engine_path;
+	ENGINE *engine;
+#endif /* OPENSSL_NO_ENGINE */
 };
 
 #define PKEX_COUNTER_T_LIMIT 5
diff --git a/src/common/dpp_crypto.c b/src/common/dpp_crypto.c
index c75fc7871..f04ee9e0e 100644
--- a/src/common/dpp_crypto.c
+++ b/src/common/dpp_crypto.c
@@ -19,6 +19,9 @@ 
 #include "utils/json.h"
 #include "common/ieee802_11_defs.h"
 #include "crypto/crypto.h"
+#ifndef OPENSSL_NO_ENGINE
+#include "crypto/openssl_engine.h"
+#endif
 #include "crypto/random.h"
 #include "crypto/sha384.h"
 #include "crypto/sha512.h"
@@ -363,6 +366,100 @@  int dpp_pbkdf2(size_t hash_len, const u8 *password, size_t password_len,
 #endif /* CONFIG_DPP2 */
 
 
+#ifndef OPENSSL_NO_ENGINE
+static EVP_PKEY * dpp_load_keypair(const struct dpp_curve_params **curve,
+				  ENGINE *engine, const char *key_id)
+{
+	EVP_PKEY *pkey;
+	EC_KEY *eckey;
+	const EC_GROUP *group;
+	int nid;
+
+	pkey = ENGINE_load_private_key(engine, key_id, NULL, NULL);
+	if (!pkey) {
+		wpa_printf(MSG_ERROR, "ENGINE: cannot load private key with id '%s' [%s]",
+			key_id, ERR_error_string(ERR_get_error(), NULL));
+		return NULL;
+	}
+
+	eckey = EVP_PKEY_get1_EC_KEY(pkey);
+	if (!eckey) {
+		EVP_PKEY_free(pkey);
+		return NULL;
+	}
+
+	group = EC_KEY_get0_group(eckey);
+	if (!group) {
+		EC_KEY_free(eckey);
+		EVP_PKEY_free(pkey);
+		return NULL;
+	}
+
+	nid = EC_GROUP_get_curve_name(group);
+	*curve = dpp_get_curve_nid(nid);
+	if (!*curve) {
+		wpa_printf(MSG_INFO,
+			   "DPP: Unsupported curve (nid=%d) in pre-assigned key",
+			   nid);
+		EC_KEY_free(eckey);
+		EVP_PKEY_free(pkey);
+		return NULL;
+	}
+
+	EC_KEY_free(eckey);
+	return pkey;
+}
+
+
+static int dpp_openssl_engine_load_dynamic(const char *engine_id,
+			const char *engine_path)
+{
+	const char *pre_cmd[] = {
+		"SO_PATH", engine_path,
+		"ID", engine_id,
+		"LIST_ADD", "1",
+		"LOAD", NULL,
+		NULL, NULL
+	};
+	const char *post_cmd[] = {
+		NULL, NULL
+	};
+
+	if (!engine_id || !engine_path)
+		return 0;
+
+	wpa_printf(MSG_DEBUG, "ENGINE: Loading %s Engine from %s",
+		   engine_id, engine_path);
+
+	return openssl_engine_load_dynamic_generic(pre_cmd, post_cmd, engine_id);
+}
+
+
+ENGINE * dpp_load_engine(const char *engine_id, const char *engine_path)
+{
+	if (dpp_openssl_engine_load_dynamic(engine_id, engine_path) < 0)
+		return NULL;
+
+	ENGINE *engine = ENGINE_by_id(engine_id);
+	if (!engine) {
+		wpa_printf(MSG_ERROR, "ENGINE: engine %s not available [%s]",
+			engine_id, ERR_error_string(ERR_get_error(), NULL));
+		return NULL;
+	}
+
+	if (ENGINE_init(engine) != 1) {
+		wpa_printf(MSG_ERROR, "ENGINE: engine init failed "
+			"(engine: %s) [%s]", engine_id,
+			ERR_error_string(ERR_get_error(), NULL));
+		ENGINE_free(engine);
+		return NULL;
+	}
+
+	return engine;
+}
+#endif /* OPENSSL_NO_ENGINE */
+
+
 int dpp_bn2bin_pad(const BIGNUM *bn, u8 *pos, size_t len)
 {
 	int num_bytes, offset;
@@ -730,6 +827,10 @@  int dpp_keygen(struct dpp_bootstrap_info *bi, const char *curve,
 
 	if (privkey)
 		bi->pubkey = dpp_set_keypair(&bi->curve, privkey, privkey_len);
+#ifndef OPENSSL_NO_ENGINE
+	else if (bi->engine)
+		bi->pubkey = dpp_load_keypair(&bi->curve, bi->engine, bi->key_id);
+#endif /* OPENSSL_NO_ENGINE */
 	else
 		bi->pubkey = dpp_gen_keypair(bi->curve);
 	if (!bi->pubkey)
diff --git a/src/common/dpp_i.h b/src/common/dpp_i.h
index af12467a5..3e42b1a11 100644
--- a/src/common/dpp_i.h
+++ b/src/common/dpp_i.h
@@ -12,6 +12,10 @@ 
 
 #ifdef CONFIG_DPP
 
+#ifndef OPENSSL_NO_ENGINE
+#include <openssl/engine.h>
+#endif /* OPENSSL_NO_ENGINE */
+
 struct dpp_global {
 	void *msg_ctx;
 	struct dl_list bootstrap; /* struct dpp_bootstrap_info */
@@ -139,6 +143,9 @@  char * dpp_sign_connector(struct dpp_configurator *conf,
 			  const struct wpabuf *dppcon);
 int dpp_test_gen_invalid_key(struct wpabuf *msg,
 			     const struct dpp_curve_params *curve);
+#ifndef OPENSSL_NO_ENGINE
+ENGINE * dpp_load_engine(const char *engine_id, const char *engine_path);
+#endif /* OPENSSL_NO_ENGINE */
 
 struct dpp_reconfig_id {
 	const EC_GROUP *group;
diff --git a/wpa_supplicant/wpa_cli.c b/wpa_supplicant/wpa_cli.c
index fea7b85e0..11bb63dc3 100644
--- a/wpa_supplicant/wpa_cli.c
+++ b/wpa_supplicant/wpa_cli.c
@@ -3855,7 +3855,7 @@  static const struct wpa_cli_cmd wpa_cli_commands[] = {
 	  "report a scanned DPP URI from a QR Code" },
 	{ "dpp_bootstrap_gen", wpa_cli_cmd_dpp_bootstrap_gen, NULL,
 	  cli_cmd_flag_sensitive,
-	  "type=<qrcode> [chan=..] [mac=..] [info=..] [curve=..] [key=..] = generate DPP bootstrap information" },
+	  "type=<qrcode> [chan=..] [mac=..] [info=..] [curve=..] [key=..] [key_id=..] [engine=..] [engine_path=..] = generate DPP bootstrap information" },
 	{ "dpp_bootstrap_remove", wpa_cli_cmd_dpp_bootstrap_remove, NULL,
 	  cli_cmd_flag_none,
 	  "*|<id> = remove DPP bootstrap information" },