Re: [v2 PATCH 5/5] Bluetooth: let the crypto subsystem generate the ecc privkey

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Tudor,

> That Bluetooth SMP knows about the private key is pointless, since the
> detection of debug key usage is actually via the public key portion.
> With this patch, the Bluetooth SMP will stop keeping a copy of the
> ecdh private key and will let the crypto subsystem to generate and
> handle the ecdh private key, potentially benefiting of hardware
> ecc private key generation and retention.
> 
> The loop that tries to generate a correct private key is now removed and
> we trust the crypto subsystem to generate a correct private key. This
> backup logic should be done in crypto, if really needed.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>
> ---
> net/bluetooth/ecdh_helper.c | 186 ++++++++++++++++++++++++--------------------
> net/bluetooth/ecdh_helper.h |   9 ++-
> net/bluetooth/selftest.c    |  14 +++-
> net/bluetooth/smp.c         |  66 +++++++---------
> 4 files changed, 147 insertions(+), 128 deletions(-)
> 
> diff --git a/net/bluetooth/ecdh_helper.c b/net/bluetooth/ecdh_helper.c
> index 16e022f..2155ce8 100644
> --- a/net/bluetooth/ecdh_helper.c
> +++ b/net/bluetooth/ecdh_helper.c
> @@ -49,15 +49,21 @@ static inline void swap_digits(u64 *in, u64 *out, unsigned int ndigits)
> 		out[i] = __swab64(in[ndigits - 1 - i]);
> }
> 
> +/* compute_ecdh_secret() - function assumes that the private key was
> + *                         already set.
> + * @tfm:          KPP tfm handle allocated with crypto_alloc_kpp().
> + * @public_key:   pair's ecc public key.
> + * secret:        memory where the ecdh computed shared secret will be saved.
> + *
> + * Return: zero on success; error code in case of error.
> + */
> int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 public_key[64],
> -			const u8 private_key[32], u8 secret[32])
> +			u8 secret[32])
> {
> 	struct kpp_request *req;
> -	struct ecdh p;
> +	u8 *tmp;
> 	struct ecdh_completion result;
> 	struct scatterlist src, dst;
> -	u8 *tmp, *buf;
> -	unsigned int buf_len;
> 	int err;
> 
> 	tmp = kmalloc(64, GFP_KERNEL);
> @@ -72,28 +78,6 @@ int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 public_key[64],
> 
> 	init_completion(&result.completion);
> 
> -	/* Security Manager Protocol holds digits in litte-endian order
> -	 * while ECC API expect big-endian data
> -	 */
> -	swap_digits((u64 *)private_key, (u64 *)tmp, 4);
> -	p.key = (char *)tmp;
> -	p.key_size = 32;
> -	/* Set curve_id */
> -	p.curve_id = ECC_CURVE_NIST_P256;
> -	buf_len = crypto_ecdh_key_len(&p);
> -	buf = kmalloc(buf_len, GFP_KERNEL);
> -	if (!buf) {
> -		err = -ENOMEM;
> -		goto free_req;
> -	}
> -
> -	crypto_ecdh_encode_key(buf, buf_len, &p);
> -
> -	/* Set A private Key */
> -	err = crypto_kpp_set_secret(tfm, (void *)buf, buf_len);
> -	if (err)
> -		goto free_all;
> -
> 	swap_digits((u64 *)public_key, (u64 *)tmp, 4); /* x */
> 	swap_digits((u64 *)&public_key[32], (u64 *)&tmp[32], 4); /* y */
> 
> @@ -118,26 +102,76 @@ int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 public_key[64],
> 	memcpy(secret, tmp, 32);
> 
> free_all:
> -	kzfree(buf);
> -free_req:
> 	kpp_request_free(req);
> free_tmp:
> 	kzfree(tmp);
> 	return err;
> }
> 
> -int generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
> -		       u8 private_key[32])
> +/* set_ecdh_privkey() - set or generate ecc private key.
> + *
> + * Function generates an ecc private key in the crypto subsystem when receiving
> + * a NULL private key or sets the received key when not NULL.
> + *
> + * @tfm:           KPP tfm handle allocated with crypto_alloc_kpp().
> + * @private_key:   user's ecc private key. When not NULL, the key is expected
> + *                 in little endian format.
> + *
> + * Return: zero on success; error code in case of error.
> + */
> +int set_ecdh_privkey(struct crypto_kpp *tfm, const u8 private_key[32])
> +{
> +	u8 *buf, *tmp = NULL;
> +	unsigned int buf_len;
> +	int err;
> +	struct ecdh p = {0};
> +
> +	p.curve_id = ECC_CURVE_NIST_P256;
> +
> +	if (private_key) {
> +		tmp = kmalloc(32, GFP_KERNEL);
> +		if (!tmp)
> +			return -ENOMEM;
> +		swap_digits((u64 *)private_key, (u64 *)tmp, 4);
> +		p.key = tmp;
> +		p.key_size = 32;
> +	}
> +
> +	buf_len = crypto_ecdh_key_len(&p);
> +	buf = kmalloc(buf_len, GFP_KERNEL);
> +	if (!buf) {
> +		err = -ENOMEM;
> +		goto free_tmp;
> +	}
> +
> +	err = crypto_ecdh_encode_key(buf, buf_len, &p);
> +	if (err)
> +		goto free_all;
> +
> +	err = crypto_kpp_set_secret(tfm, buf, buf_len);
> +	/* fall through */
> +free_all:
> +	kzfree(buf);
> +free_tmp:
> +	kzfree(tmp);
> +	return err;
> +}
> +
> +/* generate_ecdh_public_key() - function assumes that the private key was
> + *                              already set.
> + *
> + * @tfm:          KPP tfm handle allocated with crypto_alloc_kpp().
> + * @public_key:   memory where the computed ecc public key will be saved.
> + *
> + * Return: zero on success; error code in case of error.
> + */
> +int generate_ecdh_public_key(struct crypto_kpp *tfm, u8 public_key[64])
> {
> 	struct kpp_request *req;
> -	struct ecdh p;
> +	u8 *tmp;
> 	struct ecdh_completion result;
> 	struct scatterlist dst;
> -	u8 *tmp, *buf;
> -	unsigned int buf_len;
> 	int err;
> -	const unsigned short max_tries = 16;
> -	unsigned short tries = 0;
> 
> 	tmp = kmalloc(64, GFP_KERNEL);
> 	if (!tmp)
> @@ -150,63 +184,47 @@ int generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
> 	}
> 
> 	init_completion(&result.completion);
> +	sg_init_one(&dst, tmp, 64);
> +	kpp_request_set_input(req, NULL, 0);
> +	kpp_request_set_output(req, &dst, 64);
> +	kpp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
> +				 ecdh_complete, &result);
> 
> -	/* Set curve_id */
> -	p.curve_id = ECC_CURVE_NIST_P256;
> -	p.key_size = 32;
> -	buf_len = crypto_ecdh_key_len(&p);
> -	buf = kmalloc(buf_len, GFP_KERNEL);
> -	if (!buf)
> -		goto free_req;
> -
> -	do {
> -		if (tries++ >= max_tries)
> -			goto free_all;
> -
> -		/* Set private Key */
> -		p.key = (char *)private_key;
> -		crypto_ecdh_encode_key(buf, buf_len, &p);
> -		err = crypto_kpp_set_secret(tfm, buf, buf_len);
> -		if (err)
> -			goto free_all;
> -
> -		sg_init_one(&dst, tmp, 64);
> -		kpp_request_set_input(req, NULL, 0);
> -		kpp_request_set_output(req, &dst, 64);
> -		kpp_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
> -					 ecdh_complete, &result);
> -
> -		err = crypto_kpp_generate_public_key(req);
> -
> -		if (err == -EINPROGRESS) {
> -			wait_for_completion(&result.completion);
> -			err = result.err;
> -		}
> -
> -		/* Private key is not valid. Regenerate */
> -		if (err == -EINVAL)
> -			continue;
> -
> -		if (err < 0)
> -			goto free_all;
> -		else
> -			break;
> -
> -	} while (true);
> -
> -	/* Keys are handed back in little endian as expected by Security
> -	 * Manager Protocol
> +	err = crypto_kpp_generate_public_key(req);
> +	if (err == -EINPROGRESS) {
> +		wait_for_completion(&result.completion);
> +		err = result.err;
> +	}
> +	if (err < 0)
> +		goto free_all;
> +
> +	/* The public key is handed back in little endian as expected by
> +	 * the Security Manager Protocol.
> 	 */
> 	swap_digits((u64 *)tmp, (u64 *)public_key, 4); /* x */
> 	swap_digits((u64 *)&tmp[32], (u64 *)&public_key[32], 4); /* y */
> -	swap_digits((u64 *)private_key, (u64 *)tmp, 4);
> -	memcpy(private_key, tmp, 32);
> 
> free_all:
> -	kzfree(buf);
> -free_req:
> 	kpp_request_free(req);
> free_tmp:
> 	kfree(tmp);
> 	return err;
> }
> +
> +/* generate_ecdh_keys() - generate ecc key pair.
> + *
> + * @tfm:          KPP tfm handle allocated with crypto_alloc_kpp().
> + * @public_key:   memory where the computed ecc public key will be saved.
> + *
> + * Return: zero on success; error code in case of error.
> + */
> +int generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64])
> +{
> +	int err;
> +
> +	err = set_ecdh_privkey(tfm, NULL);
> +	if (err)
> +		return err;
> +
> +	return generate_ecdh_public_key(tfm, public_key);
> +}
> diff --git a/net/bluetooth/ecdh_helper.h b/net/bluetooth/ecdh_helper.h
> index 50e6676..a6f8d03 100644
> --- a/net/bluetooth/ecdh_helper.h
> +++ b/net/bluetooth/ecdh_helper.h
> @@ -23,7 +23,8 @@
> #include <crypto/kpp.h>
> #include <linux/types.h>
> 
> -int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 pub_a[64],
> -			const u8 priv_b[32], u8 secret[32]);
> -int generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64],
> -		       u8 private_key[32]);
> +int compute_ecdh_secret(struct crypto_kpp *tfm, const u8 pair_public_key[64],
> +			u8 secret[32]);
> +int set_ecdh_privkey(struct crypto_kpp *tfm, const u8 *private_key);
> +int generate_ecdh_public_key(struct crypto_kpp *tfm, u8 public_key[64]);
> +int generate_ecdh_keys(struct crypto_kpp *tfm, u8 public_key[64]);
> diff --git a/net/bluetooth/selftest.c b/net/bluetooth/selftest.c
> index ce99648..2d1519d 100644
> --- a/net/bluetooth/selftest.c
> +++ b/net/bluetooth/selftest.c
> @@ -152,11 +152,11 @@ static int __init test_ecdh_sample(struct crypto_kpp *tfm, const u8 priv_a[32],
> 	dhkey_a = &tmp[0];
> 	dhkey_b = &tmp[32];
> 
> -	ret = compute_ecdh_secret(tfm, pub_b, priv_a, dhkey_a);
> +	ret = set_ecdh_privkey(tfm, priv_a);
> 	if (ret)
> 		goto out;
> 
> -	ret = compute_ecdh_secret(tfm, pub_a, priv_b, dhkey_b);
> +	ret = compute_ecdh_secret(tfm, pub_b, dhkey_a);
> 	if (ret)
> 		goto out;
> 
> @@ -165,9 +165,17 @@ static int __init test_ecdh_sample(struct crypto_kpp *tfm, const u8 priv_a[32],
> 		goto out;
> 	}
> 
> +	ret = set_ecdh_privkey(tfm, priv_b);
> +	if (ret)
> +		goto out;
> +
> +	ret = compute_ecdh_secret(tfm, pub_a, dhkey_b);
> +	if (ret)
> +		goto out;
> +
> 	if (memcmp(dhkey_b, dhkey, 32))
> 		ret = -EINVAL;
> -
> +	/* fall through*/
> out:
> 	kfree(tmp);
> 	return ret;
> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
> index af7e610..d41449b 100644
> --- a/net/bluetooth/smp.c
> +++ b/net/bluetooth/smp.c
> @@ -84,7 +84,6 @@ enum {
> struct smp_dev {
> 	/* Secure Connections OOB data */
> 	u8			local_pk[64];
> -	u8			local_sk[32];
> 	u8			local_rand[16];
> 	bool			debug_key;
> 
> @@ -126,7 +125,6 @@ struct smp_chan {
> 
> 	/* Secure Connections variables */
> 	u8			local_pk[64];
> -	u8			local_sk[32];
> 	u8			remote_pk[64];
> 	u8			dhkey[32];
> 	u8			mackey[16];
> @@ -568,24 +566,22 @@ int smp_generate_oob(struct hci_dev *hdev, u8 hash[16], u8 rand[16])
> 
> 	if (hci_dev_test_flag(hdev, HCI_USE_DEBUG_KEYS)) {
> 		BT_DBG("Using debug keys");
> +		err = set_ecdh_privkey(smp->tfm_ecdh, debug_sk);
> +		if (err)
> +			return err;
> 		memcpy(smp->local_pk, debug_pk, 64);
> -		memcpy(smp->local_sk, debug_sk, 32);
> 		smp->debug_key = true;

this all looks good, but I do wonder why not have a helper function here.

	err = generate_ecdh_debug_keys(smp->tfm_ecdh, smp->local_pk);

And then have that function defined like this:

	int generate_ecdh_debug_keys(struct crypto_kpp *tfm, u8 public_key[64])
	{
		int err;

		err = set_ecdh_privkey(tfm, debug_sk);
		if (err)
			return err;

		memcpy(public_key, debug_pk, 64);
		return 0;
	}

I know this seems duplicate, but I wonder if that reduces the number of functions that have to be public. I prefer having most function static if possible.

> 	} else {
> 		while (true) {
> -			/* Seed private key with random number */
> -			get_random_bytes(smp->local_sk, 32);
> -
> -			/* Generate local key pair for Secure Connections */
> -			err = generate_ecdh_keys(smp->tfm_ecdh, smp->local_pk,
> -						 smp->local_sk);
> +			/* Generate key pair for Secure Connections */
> +			err = generate_ecdh_keys(smp->tfm_ecdh, smp->local_pk);
> 			if (err)
> 				return err;
> 
> 			/* This is unlikely, but we need to check that
> 			 * we didn't accidentially generate a debug key.
> 			 */
> -			if (crypto_memneq(smp->local_sk, debug_sk, 32))
> +			if (crypto_memneq(smp->local_pk, debug_pk, 64))
> 				break;
> 		}
> 		smp->debug_key = false;
> @@ -593,7 +589,6 @@ int smp_generate_oob(struct hci_dev *hdev, u8 hash[16], u8 rand[16])
> 
> 	SMP_DBG("OOB Public Key X: %32phN", smp->local_pk);
> 	SMP_DBG("OOB Public Key Y: %32phN", smp->local_pk + 32);
> -	SMP_DBG("OOB Private Key:  %32phN", smp->local_sk);
> 
> 	get_random_bytes(smp->local_rand, 16);
> 
> @@ -1900,7 +1895,6 @@ static u8 sc_send_public_key(struct smp_chan *smp)
> 		smp_dev = chan->data;
> 
> 		memcpy(smp->local_pk, smp_dev->local_pk, 64);
> -		memcpy(smp->local_sk, smp_dev->local_sk, 32);
> 		memcpy(smp->lr, smp_dev->local_rand, 16);
> 
> 		if (smp_dev->debug_key)
> @@ -1911,23 +1905,20 @@ static u8 sc_send_public_key(struct smp_chan *smp)
> 
> 	if (hci_dev_test_flag(hdev, HCI_USE_DEBUG_KEYS)) {
> 		BT_DBG("Using debug keys");
> +		if (set_ecdh_privkey(smp->tfm_ecdh, debug_sk))
> +			return SMP_UNSPECIFIED;
> 		memcpy(smp->local_pk, debug_pk, 64);
> -		memcpy(smp->local_sk, debug_sk, 32);
> 		set_bit(SMP_FLAG_DEBUG_KEY, &smp->flags);
> 	} else {
> 		while (true) {
> -			/* Seed private key with random number */
> -			get_random_bytes(smp->local_sk, 32);
> -
> -			/* Generate local key pair for Secure Connections */
> -			if (generate_ecdh_keys(smp->tfm_ecdh, smp->local_pk,
> -					       smp->local_sk))
> +			/* Generate key pair for Secure Connections */
> +			if (generate_ecdh_keys(smp->tfm_ecdh, smp->local_pk))
> 				return SMP_UNSPECIFIED;
> 
> 			/* This is unlikely, but we need to check that
> 			 * we didn't accidentially generate a debug key.
> 			 */
> -			if (crypto_memneq(smp->local_sk, debug_sk, 32))
> +			if (crypto_memneq(smp->local_pk, debug_pk, 64))
> 				break;
> 		}
> 	}
> @@ -1935,7 +1926,6 @@ static u8 sc_send_public_key(struct smp_chan *smp)
> done:
> 	SMP_DBG("Local Public Key X: %32phN", smp->local_pk);
> 	SMP_DBG("Local Public Key Y: %32phN", smp->local_pk + 32);
> -	SMP_DBG("Local Private Key:  %32phN", smp->local_sk);
> 
> 	smp_send_cmd(smp->conn, SMP_CMD_PUBLIC_KEY, 64, smp->local_pk);
> 
> @@ -2663,6 +2653,7 @@ static int smp_cmd_public_key(struct l2cap_conn *conn, struct sk_buff *skb)
> 	struct l2cap_chan *chan = conn->smp;
> 	struct smp_chan *smp = chan->data;
> 	struct hci_dev *hdev = hcon->hdev;
> +	struct crypto_kpp *tfm_ecdh;
> 	struct smp_cmd_pairing_confirm cfm;
> 	int err;
> 
> @@ -2695,8 +2686,18 @@ static int smp_cmd_public_key(struct l2cap_conn *conn, struct sk_buff *skb)
> 	SMP_DBG("Remote Public Key X: %32phN", smp->remote_pk);
> 	SMP_DBG("Remote Public Key Y: %32phN", smp->remote_pk + 32);
> 
> -	if (!compute_ecdh_secret(smp->tfm_ecdh, smp->remote_pk, smp->local_sk,
> -				 smp->dhkey))
> +	/* Compute the shared secret on the same crypto tfm on which the private
> +	 * key was set/generated.
> +	 */
> +	if (test_bit(SMP_FLAG_LOCAL_OOB, &smp->flags)) {
> +		struct smp_dev *smp_dev = chan->data;
> +
> +		tfm_ecdh = smp_dev->tfm_ecdh;
> +	} else {
> +		tfm_ecdh = smp->tfm_ecdh;
> +	}
> +
> +	if (compute_ecdh_secret(tfm_ecdh, smp->remote_pk, smp->dhkey))
> 		return SMP_UNSPECIFIED;
> 
> 	SMP_DBG("DHKey %32phN", smp->dhkey);
> @@ -3522,27 +3523,18 @@ void smp_unregister(struct hci_dev *hdev)
> 
> #if IS_ENABLED(CONFIG_BT_SELFTEST_SMP)
> 
> -static inline void swap_digits(u64 *in, u64 *out, unsigned int ndigits)
> -{
> -	int i;
> -
> -	for (i = 0; i < ndigits; i++)
> -		out[i] = __swab64(in[ndigits - 1 - i]);
> -}
> -
> static int __init test_debug_key(struct crypto_kpp *tfm_ecdh)
> {
> -	u8 pk[64], sk[32];
> +	u8 pk[64];
> 	int err;
> 
> -	swap_digits((u64 *)debug_sk, (u64 *)sk, 4);
> -
> -	err = generate_ecdh_keys(tfm_ecdh, pk, sk);
> +	err = set_ecdh_privkey(tfm_ecdh, debug_sk);
> 	if (err)
> 		return err;
> 
> -	if (crypto_memneq(sk, debug_sk, 32))
> -		return -EINVAL;
> +	err = generate_ecdh_public_key(tfm_ecdh, pk);
> +	if (err)
> +		return err;

And here using the mentioned generate_ecdh_debug_keys() would make things just simple as well.

Regards

Marcel




[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]

  Powered by Linux