[PATCH] Bluetooth: SMP: Fail if remote and local public keys are identical

[PATCH] Bluetooth: SMP: Fail if remote and local public keys are identical

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This fails the pairing procedure when both remote and local non-debug
public keys are identical.

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/smp.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index b0c1ee110eff..9e7e3655e4ec 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -2732,6 +2732,15 @@ static int smp_cmd_public_key(struct l2cap_conn *conn, struct sk_buff *skb)
 	if (skb->len < sizeof(*key))
 		return SMP_INVALID_PARAMS;
 
+	/* Check if remote and local public keys are the same and debug key is
+	 * not in use.
+	 */
+	if (!test_bit(SMP_FLAG_DEBUG_KEY, &smp->flags) &&
+	    !memcmp(key, smp->local_pk, 64)) {
+		BT_ERR("Remote and local public keys are identical");
+		return SMP_UNSPECIFIED;
+	}
+
 	memcpy(smp->remote_pk, key, 64);
 
 	if (test_bit(SMP_FLAG_REMOTE_OOB, &smp->flags)) {
-- 
2.29.2

RE: Bluetooth: SMP: Fail if remote and local public keys are identical

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1599 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=444839

---Test result---

##############################
    Test: CheckPatch - PASS
    

    ##############################
    Test: CheckGitLint - PASS
    

    ##############################
    Test: CheckBuildK - PASS
    

    ##############################
    Test: CheckTestRunner: Setup - PASS
    

    ##############################
    Test: CheckTestRunner: l2cap-tester - PASS
    Total: 40, Passed: 34 (85.0%), Failed: 0, Not Run: 6

    ##############################
    Test: CheckTestRunner: bnep-tester - PASS
    Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: mgmt-tester - FAIL
    Total: 416, Passed: 399 (95.9%), Failed: 3, Not Run: 14

    ##############################
    Test: CheckTestRunner: rfcomm-tester - PASS
    Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: sco-tester - PASS
    Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: smp-tester - PASS
    Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: userchan-tester - PASS
    Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0

    

---
Regards,
Linux Bluetooth


[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 43341 bytes --]

[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3532 bytes --]

[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 547121 bytes --]

[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 11652 bytes --]

[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 9887 bytes --]

[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11798 bytes --]

[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 5428 bytes --]

Re: [PATCH] Bluetooth: SMP: Fail if remote and local public keys are identical

[Matias Karhumaa]

Hi Luiz,

ti 9. maalisk. 2021 klo 20.42 Luiz Augusto von Dentz
(luiz.dentz@gmail.com) kirjoitti:
>
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This fails the pairing procedure when both remote and local non-debug
> public keys are identical.

Would you mind elaborating why we need to disallow identical public keys?
Do you have some specific attack scenario in your mind?

>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
>  net/bluetooth/smp.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
> index b0c1ee110eff..9e7e3655e4ec 100644
> --- a/net/bluetooth/smp.c
> +++ b/net/bluetooth/smp.c
> @@ -2732,6 +2732,15 @@ static int smp_cmd_public_key(struct l2cap_conn *conn, struct sk_buff *skb)
>         if (skb->len < sizeof(*key))
>                 return SMP_INVALID_PARAMS;
>
> +       /* Check if remote and local public keys are the same and debug key is
> +        * not in use.
> +        */
> +       if (!test_bit(SMP_FLAG_DEBUG_KEY, &smp->flags) &&
> +           !memcmp(key, smp->local_pk, 64)) {
> +               BT_ERR("Remote and local public keys are identical");
> +               return SMP_UNSPECIFIED;
> +       }
> +
>         memcpy(smp->remote_pk, key, 64);
>
>         if (test_bit(SMP_FLAG_REMOTE_OOB, &smp->flags)) {
> --
> 2.29.2
>

Best regards,
Matias Karhumaa

Re: [PATCH] Bluetooth: SMP: Fail if remote and local public keys are identical

[Marcel Holtmann]

Hi Luiz,

> This fails the pairing procedure when both remote and local non-debug
> public keys are identical.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> net/bluetooth/smp.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
> 
> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
> index b0c1ee110eff..9e7e3655e4ec 100644
> --- a/net/bluetooth/smp.c
> +++ b/net/bluetooth/smp.c
> @@ -2732,6 +2732,15 @@ static int smp_cmd_public_key(struct l2cap_conn *conn, struct sk_buff *skb)
> 	if (skb->len < sizeof(*key))
> 		return SMP_INVALID_PARAMS;
> 
> +	/* Check if remote and local public keys are the same and debug key is
> +	 * not in use.
> +	 */
> +	if (!test_bit(SMP_FLAG_DEBUG_KEY, &smp->flags) &&
> +	    !memcmp(key, smp->local_pk, 64)) {

lets use crypto_memneq here.

> +		BT_ERR("Remote and local public keys are identical");

And we need to start using bt_dev_err in SMP. I will apply v2, but then we need a cleanup patch to move this to bt_dev_{err,dbg,info}.

Regards

Marcel