[PATCH] Bluetooth: Link Keys should be stored if MITM is not required

[PATCH] Bluetooth: Link Keys should be stored if MITM is not required

[Vishal Agarwal]

If MITM protection is not required then except for Debug Keys, all
link keys should be persistent. And they should be stored for future
use.

Change-Id: Id438d424b999e9a30f29193d02ac266bee5f672b
Signed-off-by: Vishal Agarwal <vishal.agarwal@stericsson.com>
---
 net/bluetooth/hci_core.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index c5ee97c..bcb68dd 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1246,6 +1246,10 @@ static int hci_persistent_key(struct hci_dev *hdev, struct hci_conn *conn,
 	if (conn->remote_auth == 0x02 || conn->remote_auth == 0x03)
 		return 1;
 
+	/* If MITM is not required then store the Link Key */
+	if (!(conn->auth_type & 0x01))
+		return 1;
+
 	/* If none of the above criteria match, then don't store the key
 	 * persistently */
 	return 0;
-- 
1.7.0.4

Re: [PATCH] Bluetooth: Link Keys should be stored if MITM is not required

[Johan Hedberg]

Hi,

On Tue, Apr 03, 2012, Vishal Agarwal wrote:
> If MITM protection is not required then except for Debug Keys, all
> link keys should be persistent. And they should be stored for future
> use.
> 
> Change-Id: Id438d424b999e9a30f29193d02ac266bee5f672b
> Signed-off-by: Vishal Agarwal <vishal.agarwal@stericsson.com>
> ---
>  net/bluetooth/hci_core.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index c5ee97c..bcb68dd 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1246,6 +1246,10 @@ static int hci_persistent_key(struct hci_dev *hdev, struct hci_conn *conn,
>  	if (conn->remote_auth == 0x02 || conn->remote_auth == 0x03)
>  		return 1;
>  
> +	/* If MITM is not required then store the Link Key */
> +	if (!(conn->auth_type & 0x01))
> +		return 1;
> +
>  	/* If none of the above criteria match, then don't store the key
>  	 * persistently */
>  	return 0;

Nack.

This doesn't make much sense to me. Why should the MITM flag have
anything to do with the persistency of the key?

This looks more like a workaround for some device that is incorrectly
having a no-bonding requirement (which means that we should *not* store
the key). Please describe what kind of setup you've seen this with and
include a hcidump for it showing the local and remote authentication
requirement and IO capabilities.

Johan

RE: [PATCH] Bluetooth: Link Keys should be stored if MITM is not required

[Vishal AGARWAL]

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

Hi Johan,

I am testing with PTS. I have attached the HCI dump also for this case.

Also pls refer to function "link_key_request" in file hciops.c. It also has the same kind of implementation.

	/* Don't use unauthenticated combination keys if MITM is
	 * required */
	if (key_info->type == 0x04 && conn->loc_auth != 0xff &&
						(conn->loc_auth & 0x01))
		hci_send_cmd(dev->sk, OGF_LINK_CTL, OCF_LINK_KEY_NEG_REPLY,
								6, dba);
	else if (key_info->type == 0x00 &&
				sec_level == BT_SECURITY_HIGH &&
				key_info->pin_len <16) {
		hci_send_cmd(dev->sk, OGF_LINK_CTL, OCF_LINK_KEY_NEG_REPLY,
								6, dba);
	} else {
		link_key_reply_cp lr;

		memcpy(lr.link_key, key_info->key, 16);
		bacpy(&lr.bdaddr, dba);

		hci_send_cmd(dev->sk, OGF_LINK_CTL, OCF_LINK_KEY_REPLY,
						LINK_KEY_REPLY_CP_SIZE, &lr);
	}

Same PTS setup is passing if we use hci_ops instead of mgmt_ops because of the first check in which it checks if for MITM (conn->loc_auth & 0x01).
And if MITM is not required then key of type 04 (UNAUTHENTICATED_COMBINATION_KEY) will also work.

In case you are not able to open logs in this format, pls let me know. I will provide you raw HCI dump.

Thanks
Vishal

-----Original Message-----
From: Johan Hedberg [mailto:johan.hedberg@gmail.com] 
Sent: Tuesday, April 03, 2012 3:08 PM
To: Vishal AGARWAL
Cc: linux-bluetooth@vger.kernel.org; Naresh-kumar GUPTA
Subject: Re: [PATCH] Bluetooth: Link Keys should be stored if MITM is not required

Hi,

On Tue, Apr 03, 2012, Vishal Agarwal wrote:
> If MITM protection is not required then except for Debug Keys, all
> link keys should be persistent. And they should be stored for future
> use.
> 
> Change-Id: Id438d424b999e9a30f29193d02ac266bee5f672b
> Signed-off-by: Vishal Agarwal <vishal.agarwal@stericsson.com>
> ---
>  net/bluetooth/hci_core.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index c5ee97c..bcb68dd 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1246,6 +1246,10 @@ static int hci_persistent_key(struct hci_dev *hdev, struct hci_conn *conn,
>  	if (conn->remote_auth == 0x02 || conn->remote_auth == 0x03)
>  		return 1;
>  
> +	/* If MITM is not required then store the Link Key */
> +	if (!(conn->auth_type & 0x01))
> +		return 1;
> +
>  	/* If none of the above criteria match, then don't store the key
>  	 * persistently */
>  	return 0;

Nack.

This doesn't make much sense to me. Why should the MITM flag have
anything to do with the persistency of the key?

This looks more like a workaround for some device that is incorrectly
having a no-bonding requirement (which means that we should *not* store
the key). Please describe what kind of setup you've seen this with and
include a hcidump for it showing the local and remote authentication
requirement and IO capabilities.

Johan

[-- Attachment #2: cfa.cfa --]
[-- Type: application/octet-stream, Size: 2911 bytes --]

Re: [PATCH] Bluetooth: Link Keys should be stored if MITM is not required

[Johan Hedberg]

Hi Vishal,

On Tue, Apr 03, 2012, Vishal AGARWAL wrote:
> I am testing with PTS. I have attached the HCI dump also for this
> case.

First of all please stop top posting. It's not tolerated on this list.
Replying to an inline quoted mail makes it even doubly worse since it
completely messes up the history of the thread.

About the hcidump you attached we're getting the following from the
remote device:

 HCI Event: IO Capability Response (0x32) plen 9
    bdaddr 00:80:98:E7:32:4C capability 0x01 oob 0x00 auth 0x00
    Capability: DisplayYesNo (OOB data not present)
    Authentication: No Bonding (No MITM Protection)

I.e. the PTS is telling us to *not* store the key. Which test case is
this? To my understanding the PTS doesn't have BR/EDR GAP tests but you
need to use the BITE for them. Has something changed in that regard?

Looking at the full trace we're getting a link key request before
dropping the ACL. What we should probably do is to not immediately drop
the key from our list but instead keep it there as long as the
connection is up. I think that would still be in line with what the
specification expects us to do with no-bonding keys.

Now that I look at hciops it's more or less what's happening: it never
writes the key to the file system but does keep it in its list at
runtime.

So to conclude, the right fix is not what you've proposed but to modify
the code to hang on to the key until the ACL link goes down. I.e. please
add a "persistent" flag to struct link_key and add code to remove any
such keys from hdev->link_keys when the ACL goes away.

Johan

Re: [PATCH] Bluetooth: Link Keys should be stored if MITM is not required

[Johan Hedberg]

Hi Vishal,

On Tue, Apr 03, 2012, Johan Hedberg wrote:
> Hi Vishal,
> 
> On Tue, Apr 03, 2012, Vishal AGARWAL wrote:
> > I am testing with PTS. I have attached the HCI dump also for this
> > case.
> 
> First of all please stop top posting. It's not tolerated on this list.
> Replying to an inline quoted mail makes it even doubly worse since it
> completely messes up the history of the thread.
> 
> About the hcidump you attached we're getting the following from the
> remote device:
> 
>  HCI Event: IO Capability Response (0x32) plen 9
>     bdaddr 00:80:98:E7:32:4C capability 0x01 oob 0x00 auth 0x00
>     Capability: DisplayYesNo (OOB data not present)
>     Authentication: No Bonding (No MITM Protection)
> 
> I.e. the PTS is telling us to *not* store the key. Which test case is
> this? To my understanding the PTS doesn't have BR/EDR GAP tests but you
> need to use the BITE for them. Has something changed in that regard?
> 
> Looking at the full trace we're getting a link key request before
> dropping the ACL. What we should probably do is to not immediately drop
> the key from our list but instead keep it there as long as the
> connection is up. I think that would still be in line with what the
> specification expects us to do with no-bonding keys.
> 
> Now that I look at hciops it's more or less what's happening: it never
> writes the key to the file system but does keep it in its list at
> runtime.
> 
> So to conclude, the right fix is not what you've proposed but to modify
> the code to hang on to the key until the ACL link goes down. I.e. please
> add a "persistent" flag to struct link_key and add code to remove any
> such keys from hdev->link_keys when the ACL goes away.

Additionally, to avoid iterating hdev->link_keys unnecessarily it'd
probably make sense to add a flag to hci_conn to indicate that it has a
temporary key in hdev->link_keys, or maybe even add a direct reference
to the key to struct hci_conn.

Also, please let me know if you can do this by the end of this week
since it's something that should preferably be fixed before 3.4 goes
out.

Johan

Re: [PATCH] Bluetooth: Link Keys should be stored if MITM is not required

[vishal agarwal]

Hi Johan,

On Tue, Apr 3, 2012 at 5:11 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Vishal,
>
> On Tue, Apr 03, 2012, Johan Hedberg wrote:
>> Hi Vishal,
>>
>> On Tue, Apr 03, 2012, Vishal AGARWAL wrote:
>> > I am testing with PTS. I have attached the HCI dump also for this
>> > case.
>>
>> First of all please stop top posting. It's not tolerated on this list.
>> Replying to an inline quoted mail makes it even doubly worse since it
>> completely messes up the history of the thread.
>>
>> About the hcidump you attached we're getting the following from the
>> remote device:
>>
>>  HCI Event: IO Capability Response (0x32) plen 9
>>     bdaddr 00:80:98:E7:32:4C capability 0x01 oob 0x00 auth 0x00
>>     Capability: DisplayYesNo (OOB data not present)
>>     Authentication: No Bonding (No MITM Protection)
>>
>> I.e. the PTS is telling us to *not* store the key. Which test case is
>> this? To my understanding the PTS doesn't have BR/EDR GAP tests but you
>> need to use the BITE for them. Has something changed in that regard?
>>
>> Looking at the full trace we're getting a link key request before
>> dropping the ACL. What we should probably do is to not immediately drop
>> the key from our list but instead keep it there as long as the
>> connection is up. I think that would still be in line with what the
>> specification expects us to do with no-bonding keys.
>>
>> Now that I look at hciops it's more or less what's happening: it never
>> writes the key to the file system but does keep it in its list at
>> runtime.
>>
>> So to conclude, the right fix is not what you've proposed but to modify
>> the code to hang on to the key until the ACL link goes down. I.e. please
>> add a "persistent" flag to struct link_key and add code to remove any
>> such keys from hdev->link_keys when the ACL goes away.
>
> Additionally, to avoid iterating hdev->link_keys unnecessarily it'd
> probably make sense to add a flag to hci_conn to indicate that it has a
> temporary key in hdev->link_keys, or maybe even add a direct reference
> to the key to struct hci_conn.
>
> Also, please let me know if you can do this by the end of this week
> since it's something that should preferably be fixed before 3.4 goes
> out.
OK, I will add a new bool variable temporary_key in struct hci_conn.
I will do it in this week itself.

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

Thanks
Vishal