[PATCH] shared/att: change security as needed

[PATCH] shared/att: change security as needed

[josephsih]

From: Joseph Hwang <josephsih@chromium.org>

When pairing with a BLE keyboard, bluetoothd suffers from
authentication errors as follows:

ERR bluetoothd[1103]: Report Map read failed:
                    Attribute requires authentication before read/write
ERR bluetoothd[1103]: Protocol Mode characteristic read failed:
                    Attribute requires authentication before read/write
ERR bluetoothd[1103]: HID Information read failed:
                    Attribute requires authentication before read/write

This is because the original security level is BT_ATT_SECURITY_LOW,
while BT_ATT_SECURITY_HIGH is required for pairing. This patch enables
the security elevation so that handle_error_rsp() could push the
operation back to request queue properly.

---
 src/shared/att.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/shared/att.c b/src/shared/att.c
index 3a84783..331fae7 100644
--- a/src/shared/att.c
+++ b/src/shared/att.c
@@ -574,9 +574,6 @@ static bool change_security(struct bt_att *att, uint8_t ecode)
 	int security;
 
 	security = bt_att_get_security(att);
-	if (security != BT_ATT_SECURITY_AUTO)
-		return false;
-
 	if (ecode == BT_ATT_ERROR_INSUFFICIENT_ENCRYPTION &&
 					security < BT_ATT_SECURITY_MEDIUM)
 		security = BT_ATT_SECURITY_MEDIUM;
-- 
2.7.0.rc3.207.g0ac5344

Re: [PATCH] shared/att: change security as needed

[Luiz Augusto von Dentz]

Hi Joseph,

On Wed, Mar 2, 2016 at 3:28 PM,  <josephsih@chromium.org> wrote:
> From: Joseph Hwang <josephsih@chromium.org>
>
> When pairing with a BLE keyboard, bluetoothd suffers from
> authentication errors as follows:
>
> ERR bluetoothd[1103]: Report Map read failed:
>                     Attribute requires authentication before read/write
> ERR bluetoothd[1103]: Protocol Mode characteristic read failed:
>                     Attribute requires authentication before read/write
> ERR bluetoothd[1103]: HID Information read failed:
>                     Attribute requires authentication before read/write
>
> This is because the original security level is BT_ATT_SECURITY_LOW,
> while BT_ATT_SECURITY_HIGH is required for pairing. This patch enables
> the security elevation so that handle_error_rsp() could push the
> operation back to request queue properly.
>
> ---
>  src/shared/att.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/src/shared/att.c b/src/shared/att.c
> index 3a84783..331fae7 100644
> --- a/src/shared/att.c
> +++ b/src/shared/att.c
> @@ -574,9 +574,6 @@ static bool change_security(struct bt_att *att, uint8_t ecode)
>         int security;
>
>         security = bt_att_get_security(att);
> -       if (security != BT_ATT_SECURITY_AUTO)
> -               return false;
> -

While this perhaps works it seems to be just a work around the
problem, it seems to me that ATT security shall be set to
BT_ATT_SECURITY_AUTO not to BT_ATT_SECURITY_LOW which is preventing
security to be elevated.

-- 
Luiz Augusto von Dentz

Re: [PATCH] shared/att: change security as needed

[Luiz Augusto von Dentz]

Hi Joseph,

On Wed, Mar 2, 2016 at 3:55 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Joseph,
>
> On Wed, Mar 2, 2016 at 3:28 PM,  <josephsih@chromium.org> wrote:
>> From: Joseph Hwang <josephsih@chromium.org>
>>
>> When pairing with a BLE keyboard, bluetoothd suffers from
>> authentication errors as follows:
>>
>> ERR bluetoothd[1103]: Report Map read failed:
>>                     Attribute requires authentication before read/write
>> ERR bluetoothd[1103]: Protocol Mode characteristic read failed:
>>                     Attribute requires authentication before read/write
>> ERR bluetoothd[1103]: HID Information read failed:
>>                     Attribute requires authentication before read/write
>>
>> This is because the original security level is BT_ATT_SECURITY_LOW,
>> while BT_ATT_SECURITY_HIGH is required for pairing. This patch enables
>> the security elevation so that handle_error_rsp() could push the
>> operation back to request queue properly.
>>
>> ---
>>  src/shared/att.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/src/shared/att.c b/src/shared/att.c
>> index 3a84783..331fae7 100644
>> --- a/src/shared/att.c
>> +++ b/src/shared/att.c
>> @@ -574,9 +574,6 @@ static bool change_security(struct bt_att *att, uint8_t ecode)
>>         int security;
>>
>>         security = bt_att_get_security(att);
>> -       if (security != BT_ATT_SECURITY_AUTO)
>> -               return false;
>> -
>
> While this perhaps works it seems to be just a work around the
> problem, it seems to me that ATT security shall be set to
> BT_ATT_SECURITY_AUTO not to BT_ATT_SECURITY_LOW which is preventing
> security to be elevated.

Something like this should fix it:

index 14e850e..6bc44b6 100644
--- a/src/device.c
+++ b/src/device.c
@@ -4710,6 +4710,7 @@ bool device_attach_att(struct btd_device *dev,
GIOChannel *io)
        dev->att = g_attrib_get_att(attrib);

        bt_att_ref(dev->att);
+       bt_att_set_security(dev->att, BT_ATT_SECURITY_AUTO);

        dev->att_disconn_id = bt_att_register_disconnect(dev->att,
                                                att_disconnected_cb, dev, NULL);

-- 
Luiz Augusto von Dentz

Re: [PATCH] shared/att: change security as needed

[Luiz Augusto von Dentz]

Hi Joseph,

On Wed, Mar 2, 2016 at 4:17 PM, Joseph Hwang <josephsih@google.com> wrote:
> Hi Luiz:
>
>   Thank you for your comments. I will fix, verify, and re-submit the patch.

Ive sent a patch for this actually.

Re: [PATCH] shared/att: change security as needed

[Luiz Augusto von Dentz]

Hi Joseph,

On Thu, Mar 3, 2016 at 12:45 PM, Joseph Hwang <josephsih@google.com> wrote:
> Hi Luiz:
>
>   I try to apply your one-liner locally.
>
> +       bt_att_set_security(dev->att, BT_ATT_SECURITY_AUTO);
>
>   However, bt_att_set_security() failed  in device_attach_att() in
> src/device.c. It cannot set BT_ATT_SECURITY_AUTO successfully here.
>
>   But bt_att_set_security() would succeed in change_security() in
> src/shared/att.c as in my patch.

No top posting in the mailing list, respond inline. Btw, Ive sent a
new patch about this.

-- 
Luiz Augusto von Dentz