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

[Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>]

Hi, Marcel,

On 09/28/2017 07:50 PM, Marcel Holtmann wrote:
> 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.

You want set_ecdh_privkey() to be static in ecdh_helper.c?
test_ecdh_sample() and test_debug_key() don't need to copy the public
key, so they need set_ecdh_privkey() as public.

I can make generate_ecdh_debug_keys() static to smp.c, but we will be
mixing helpers in smp.c and ecdh_helper.c.

> > 	} 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.

Copying of the public key is not necessary here,
generate_ecdh_debug_keys() shouldn't be used.


Cheers,
ta

--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html