With HOG implementation, LE connection has been changed in the way
that we use security level medium to connect bonded devices.
This patch set changes this and makes sure that HOG has enable encryption as
required by spec.
However for all other connection we always start from security LOW and increase security only when characteristic permissions requires this.
This patch set is only compile-tested as I don't have HOG device here
Lukasz Rymanowski (4):
android/gatt: Rename set_security function
android/gatt: Add helper to set security level
android/gatt: Expose function to set security on GATT
android/hidhost: Start encryption for HOG when bonded
android/gatt.c | 61 ++++++++++++++++++++++++++++++++-----------------------
android/gatt.h | 2 ++
android/hidhost.c | 4 ++++
3 files changed, 42 insertions(+), 25 deletions(-)
--
1.8.4
Hi Luiz,
On 20 June 2014 01:18, Lukasz Rymanowski <[email protected]> wrote:
> HI Luiz,
>
> On 19 June 2014 15:25, Luiz Augusto von Dentz <[email protected]> wrote:
>> Hi Lukasz,
>>
>> On Thu, Jun 19, 2014 at 1:41 AM, Lukasz Rymanowski
>> <[email protected]> wrote:
>>> HOG requires encryption on connection, so make sure it is on.
>>>
>>> On the other hand we don't need medium security always when
>>> connecting LE device even device are bonded. It depends on permissions
>>> on characteristics. That's why we want security low in connect_le()
>>> ---
>>> android/gatt.c | 6 +-----
>>> android/hidhost.c | 4 ++++
>>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/android/gatt.c b/android/gatt.c
>>> index 9471eaf..746316d 100644
>>> --- a/android/gatt.c
>>> +++ b/android/gatt.c
>>> @@ -1419,7 +1419,6 @@ reply:
>>>
>>> static int connect_le(struct gatt_device *dev)
>>> {
>>> - BtIOSecLevel sec_level;
>>> GIOChannel *io;
>>> GError *gerr = NULL;
>>> char addr[18];
>>> @@ -1434,9 +1433,6 @@ static int connect_le(struct gatt_device *dev)
>>>
>>> DBG("Connection attempt to: %s", addr);
>>>
>>> - sec_level = bt_device_is_bonded(&dev->bdaddr) ? BT_IO_SEC_MEDIUM :
>>> - BT_IO_SEC_LOW;
>>> -
>>> /*
>>> * This connection will help us catch any PDUs that comes before
>>> * pairing finishes
>>> @@ -1448,7 +1444,7 @@ static int connect_le(struct gatt_device *dev)
>>> BT_IO_OPT_DEST_BDADDR, &dev->bdaddr,
>>> BT_IO_OPT_DEST_TYPE, dev->bdaddr_type,
>>> BT_IO_OPT_CID, ATT_CID,
>>> - BT_IO_OPT_SEC_LEVEL, sec_level,
>>> + BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW,
>>> BT_IO_OPT_INVALID);
>>> if (!io) {
>>> error("gatt: Failed bt_io_connect(%s): %s", addr,
>>> diff --git a/android/hidhost.c b/android/hidhost.c
>>> index 846dd57..1f66454 100644
>>> --- a/android/hidhost.c
>>> +++ b/android/hidhost.c
>>> @@ -777,6 +777,10 @@ static void hog_conn_cb(const bdaddr_t *addr, int err, void *attrib)
>>> bt_hid_notify_state(dev, HAL_HIDHOST_STATE_CONNECTING);
>>> }
>>>
>>> + /* If device is bonded lets enable encryption */
>>> + if (bt_device_is_bonded(addr))
>>> + bt_gatt_set_security(addr, BT_SECURITY_MEDIUM);
>>> +
>>
>> Don't we need to wait the encryption change to only then start sending
>> commands? Actually this gets more complicate since the specs says the
>> following:
>>
>> "If the HID Host receives the L2CAP Connection Parameter Update
>> request but has not
>> yet completed service discovery or has not completed encryption, the
>> HID Host may
>> send the L2CAP Connection Parameter Update Response with the Result field
>> indicating that the request has been rejected."
>>
> As far as I understand, socket will stop flow until the security level
> is updated.
>
>> So it seems HoG may have some requirements on L2CAP as well so it is
>> not per characteristics, so perhaps connect_le should take the initial
>> security level then we can add some security requirement to
>> bt_gatt_connect_app or something like that. Btw, Im not aware of any
>> drawback regarding encryption even if is not mandatory it will just
>> make the connection more secure, but perhaps this is fixing another
>> problem?
>
> One of the drawback is GATT PTS testcase TC_GAW_CL_BV_02_C which test
> signed wrtie. Scenario is that device do bonding and exchange CSRK,
> then disconnect and connect again. Once their are reconnected signed
> write should be performed. Note that signed write is no allowed on
> encrypted link.
>
Just to be clear here, LTK is also exchange during bonding. So you
might argue that PTS should request CSRK only, but would be not sure
about that. I saw devices which request LTK and CSRK in the same time,
even though don't know if CSRK is used.
>>
>>> if (!dev->hog) {
>>> /* TODO: Get device details and primary */
>>> dev->hog = bt_hog_new("bluez-input-device", dev->vendor,
>>> --
>>> 1.8.4
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
>
> BR
> Lukasz
\Lukasz
HI Luiz,
On 19 June 2014 15:25, Luiz Augusto von Dentz <[email protected]> wrote:
> Hi Lukasz,
>
> On Thu, Jun 19, 2014 at 1:41 AM, Lukasz Rymanowski
> <[email protected]> wrote:
>> HOG requires encryption on connection, so make sure it is on.
>>
>> On the other hand we don't need medium security always when
>> connecting LE device even device are bonded. It depends on permissions
>> on characteristics. That's why we want security low in connect_le()
>> ---
>> android/gatt.c | 6 +-----
>> android/hidhost.c | 4 ++++
>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/android/gatt.c b/android/gatt.c
>> index 9471eaf..746316d 100644
>> --- a/android/gatt.c
>> +++ b/android/gatt.c
>> @@ -1419,7 +1419,6 @@ reply:
>>
>> static int connect_le(struct gatt_device *dev)
>> {
>> - BtIOSecLevel sec_level;
>> GIOChannel *io;
>> GError *gerr = NULL;
>> char addr[18];
>> @@ -1434,9 +1433,6 @@ static int connect_le(struct gatt_device *dev)
>>
>> DBG("Connection attempt to: %s", addr);
>>
>> - sec_level = bt_device_is_bonded(&dev->bdaddr) ? BT_IO_SEC_MEDIUM :
>> - BT_IO_SEC_LOW;
>> -
>> /*
>> * This connection will help us catch any PDUs that comes before
>> * pairing finishes
>> @@ -1448,7 +1444,7 @@ static int connect_le(struct gatt_device *dev)
>> BT_IO_OPT_DEST_BDADDR, &dev->bdaddr,
>> BT_IO_OPT_DEST_TYPE, dev->bdaddr_type,
>> BT_IO_OPT_CID, ATT_CID,
>> - BT_IO_OPT_SEC_LEVEL, sec_level,
>> + BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW,
>> BT_IO_OPT_INVALID);
>> if (!io) {
>> error("gatt: Failed bt_io_connect(%s): %s", addr,
>> diff --git a/android/hidhost.c b/android/hidhost.c
>> index 846dd57..1f66454 100644
>> --- a/android/hidhost.c
>> +++ b/android/hidhost.c
>> @@ -777,6 +777,10 @@ static void hog_conn_cb(const bdaddr_t *addr, int err, void *attrib)
>> bt_hid_notify_state(dev, HAL_HIDHOST_STATE_CONNECTING);
>> }
>>
>> + /* If device is bonded lets enable encryption */
>> + if (bt_device_is_bonded(addr))
>> + bt_gatt_set_security(addr, BT_SECURITY_MEDIUM);
>> +
>
> Don't we need to wait the encryption change to only then start sending
> commands? Actually this gets more complicate since the specs says the
> following:
>
> "If the HID Host receives the L2CAP Connection Parameter Update
> request but has not
> yet completed service discovery or has not completed encryption, the
> HID Host may
> send the L2CAP Connection Parameter Update Response with the Result field
> indicating that the request has been rejected."
>
As far as I understand, socket will stop flow until the security level
is updated.
> So it seems HoG may have some requirements on L2CAP as well so it is
> not per characteristics, so perhaps connect_le should take the initial
> security level then we can add some security requirement to
> bt_gatt_connect_app or something like that. Btw, Im not aware of any
> drawback regarding encryption even if is not mandatory it will just
> make the connection more secure, but perhaps this is fixing another
> problem?
One of the drawback is GATT PTS testcase TC_GAW_CL_BV_02_C which test
signed wrtie. Scenario is that device do bonding and exchange CSRK,
then disconnect and connect again. Once their are reconnected signed
write should be performed. Note that signed write is no allowed on
encrypted link.
>
>> if (!dev->hog) {
>> /* TODO: Get device details and primary */
>> dev->hog = bt_hog_new("bluez-input-device", dev->vendor,
>> --
>> 1.8.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Luiz Augusto von Dentz
BR
Lukasz
Hi Lukasz,
On Thu, Jun 19, 2014 at 1:41 AM, Lukasz Rymanowski
<[email protected]> wrote:
> HOG requires encryption on connection, so make sure it is on.
>
> On the other hand we don't need medium security always when
> connecting LE device even device are bonded. It depends on permissions
> on characteristics. That's why we want security low in connect_le()
> ---
> android/gatt.c | 6 +-----
> android/hidhost.c | 4 ++++
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/android/gatt.c b/android/gatt.c
> index 9471eaf..746316d 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -1419,7 +1419,6 @@ reply:
>
> static int connect_le(struct gatt_device *dev)
> {
> - BtIOSecLevel sec_level;
> GIOChannel *io;
> GError *gerr = NULL;
> char addr[18];
> @@ -1434,9 +1433,6 @@ static int connect_le(struct gatt_device *dev)
>
> DBG("Connection attempt to: %s", addr);
>
> - sec_level = bt_device_is_bonded(&dev->bdaddr) ? BT_IO_SEC_MEDIUM :
> - BT_IO_SEC_LOW;
> -
> /*
> * This connection will help us catch any PDUs that comes before
> * pairing finishes
> @@ -1448,7 +1444,7 @@ static int connect_le(struct gatt_device *dev)
> BT_IO_OPT_DEST_BDADDR, &dev->bdaddr,
> BT_IO_OPT_DEST_TYPE, dev->bdaddr_type,
> BT_IO_OPT_CID, ATT_CID,
> - BT_IO_OPT_SEC_LEVEL, sec_level,
> + BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW,
> BT_IO_OPT_INVALID);
> if (!io) {
> error("gatt: Failed bt_io_connect(%s): %s", addr,
> diff --git a/android/hidhost.c b/android/hidhost.c
> index 846dd57..1f66454 100644
> --- a/android/hidhost.c
> +++ b/android/hidhost.c
> @@ -777,6 +777,10 @@ static void hog_conn_cb(const bdaddr_t *addr, int err, void *attrib)
> bt_hid_notify_state(dev, HAL_HIDHOST_STATE_CONNECTING);
> }
>
> + /* If device is bonded lets enable encryption */
> + if (bt_device_is_bonded(addr))
> + bt_gatt_set_security(addr, BT_SECURITY_MEDIUM);
> +
Don't we need to wait the encryption change to only then start sending
commands? Actually this gets more complicate since the specs says the
following:
"If the HID Host receives the L2CAP Connection Parameter Update
request but has not
yet completed service discovery or has not completed encryption, the
HID Host may
send the L2CAP Connection Parameter Update Response with the Result field
indicating that the request has been rejected."
So it seems HoG may have some requirements on L2CAP as well so it is
not per characteristics, so perhaps connect_le should take the initial
security level then we can add some security requirement to
bt_gatt_connect_app or something like that. Btw, Im not aware of any
drawback regarding encryption even if is not mandatory it will just
make the connection more secure, but perhaps this is fixing another
problem?
> if (!dev->hog) {
> /* TODO: Get device details and primary */
> dev->hog = bt_hog_new("bluez-input-device", dev->vendor,
> --
> 1.8.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Luiz Augusto von Dentz
HOG requires encryption on connection, so make sure it is on.
On the other hand we don't need medium security always when
connecting LE device even device are bonded. It depends on permissions
on characteristics. That's why we want security low in connect_le()
---
android/gatt.c | 6 +-----
android/hidhost.c | 4 ++++
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/android/gatt.c b/android/gatt.c
index 9471eaf..746316d 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -1419,7 +1419,6 @@ reply:
static int connect_le(struct gatt_device *dev)
{
- BtIOSecLevel sec_level;
GIOChannel *io;
GError *gerr = NULL;
char addr[18];
@@ -1434,9 +1433,6 @@ static int connect_le(struct gatt_device *dev)
DBG("Connection attempt to: %s", addr);
- sec_level = bt_device_is_bonded(&dev->bdaddr) ? BT_IO_SEC_MEDIUM :
- BT_IO_SEC_LOW;
-
/*
* This connection will help us catch any PDUs that comes before
* pairing finishes
@@ -1448,7 +1444,7 @@ static int connect_le(struct gatt_device *dev)
BT_IO_OPT_DEST_BDADDR, &dev->bdaddr,
BT_IO_OPT_DEST_TYPE, dev->bdaddr_type,
BT_IO_OPT_CID, ATT_CID,
- BT_IO_OPT_SEC_LEVEL, sec_level,
+ BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW,
BT_IO_OPT_INVALID);
if (!io) {
error("gatt: Failed bt_io_connect(%s): %s", addr,
diff --git a/android/hidhost.c b/android/hidhost.c
index 846dd57..1f66454 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -777,6 +777,10 @@ static void hog_conn_cb(const bdaddr_t *addr, int err, void *attrib)
bt_hid_notify_state(dev, HAL_HIDHOST_STATE_CONNECTING);
}
+ /* If device is bonded lets enable encryption */
+ if (bt_device_is_bonded(addr))
+ bt_gatt_set_security(addr, BT_SECURITY_MEDIUM);
+
if (!dev->hog) {
/* TODO: Get device details and primary */
dev->hog = bt_hog_new("bluez-input-device", dev->vendor,
--
1.8.4
---
android/gatt.c | 10 ++++++++++
android/gatt.h | 2 ++
2 files changed, 12 insertions(+)
diff --git a/android/gatt.c b/android/gatt.c
index 7061924..9471eaf 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -6286,3 +6286,13 @@ bool bt_gatt_disconnect_app(unsigned int id, const bdaddr_t *addr)
return true;
}
+
+void bt_gatt_set_security(const bdaddr_t *addr, int sec_level)
+{
+ struct gatt_device *dev = find_device_by_addr(addr);
+
+ if (!dev)
+ return;
+
+ set_security_level(dev, sec_level);
+}
diff --git a/android/gatt.h b/android/gatt.h
index 5ba9161..d4f8a8e 100644
--- a/android/gatt.h
+++ b/android/gatt.h
@@ -38,3 +38,5 @@ bool bt_gatt_unregister_app(unsigned int id);
bool bt_gatt_connect_app(unsigned int id, const bdaddr_t *addr);
bool bt_gatt_disconnect_app(unsigned int id, const bdaddr_t *addr);
+
+void bt_gatt_set_security(const bdaddr_t *addr, int sec_level);
--
1.8.4
---
android/gatt.c | 35 ++++++++++++++++++++---------------
1 file changed, 20 insertions(+), 15 deletions(-)
diff --git a/android/gatt.c b/android/gatt.c
index 004dead..7061924 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -2735,12 +2735,29 @@ static int get_sec_level(struct gatt_device *dev)
return sec_level;
}
-static bool set_security_by_auth(struct gatt_device *device, int auth_type)
+static bool set_security_level(struct gatt_device *dev, int sec_level)
{
- int req_sec_level, sec_level;
GError *gerr = NULL;
GIOChannel *io;
+ io = g_attrib_get_channel(dev->attrib);
+ if (!io)
+ return false;
+
+ bt_io_set(io, &gerr, BT_IO_OPT_SEC_LEVEL, sec_level, BT_IO_OPT_INVALID);
+ if (gerr) {
+ error("gatt: Failed to set security level: %s", gerr->message);
+ g_error_free(gerr);
+ return false;
+ }
+
+ return true;
+}
+
+static bool set_security_by_auth(struct gatt_device *device, int auth_type)
+{
+ int req_sec_level, sec_level;
+
switch (auth_type) {
case HAL_GATT_AUTHENTICATION_MITM:
req_sec_level = BT_SECURITY_HIGH;
@@ -2763,19 +2780,7 @@ static bool set_security_by_auth(struct gatt_device *device, int auth_type)
if (req_sec_level <= sec_level)
return true;
- io = g_attrib_get_channel(device->attrib);
- if (!io)
- return false;
-
- bt_io_set(io, &gerr, BT_IO_OPT_SEC_LEVEL, req_sec_level,
- BT_IO_OPT_INVALID);
- if (gerr) {
- error("gatt: Failed to set security level: %s", gerr->message);
- g_error_free(gerr);
- return false;
- }
-
- return true;
+ return set_security_level(device, req_sec_level);
}
static void handle_client_read_characteristic(const void *buf, uint16_t len)
--
1.8.4
New helper set_security_level will be introduced in next patch, so lets
give this function better name
---
android/gatt.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/android/gatt.c b/android/gatt.c
index 32853fa..004dead 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -2735,7 +2735,7 @@ static int get_sec_level(struct gatt_device *dev)
return sec_level;
}
-static bool set_security(struct gatt_device *device, int auth_type)
+static bool set_security_by_auth(struct gatt_device *device, int auth_type)
{
int req_sec_level, sec_level;
GError *gerr = NULL;
@@ -2818,7 +2818,7 @@ static void handle_client_read_characteristic(const void *buf, uint16_t len)
goto failed;
}
- if (!set_security(conn->device, cmd->auth_req)) {
+ if (!set_security_by_auth(conn->device, cmd->auth_req)) {
error("gatt: Failed to set security %d", cmd->auth_req);
status = HAL_STATUS_FAILED;
free(cb_data);
@@ -2952,7 +2952,7 @@ static void handle_client_write_characteristic(const void *buf, uint16_t len)
}
}
- if (!set_security(conn->device, cmd->auth_req)) {
+ if (!set_security_by_auth(conn->device, cmd->auth_req)) {
error("gatt: Failed to set security %d", cmd->auth_req);
status = HAL_STATUS_FAILED;
goto failed;
@@ -3159,7 +3159,7 @@ static void handle_client_read_descriptor(const void *buf, uint16_t len)
goto failed;
}
- if (!set_security(conn->device, cmd->auth_req)) {
+ if (!set_security_by_auth(conn->device, cmd->auth_req)) {
error("gatt: Failed to set security %d", cmd->auth_req);
status = HAL_STATUS_FAILED;
free(cb_data);
@@ -3291,7 +3291,7 @@ static void handle_client_write_descriptor(const void *buf, uint16_t len)
}
}
- if (!set_security(conn->device, cmd->auth_req)) {
+ if (!set_security_by_auth(conn->device, cmd->auth_req)) {
error("gatt: Failed to set security %d", cmd->auth_req);
status = HAL_STATUS_FAILED;
goto failed;
@@ -3774,7 +3774,7 @@ static uint8_t test_increase_security(bdaddr_t *bdaddr, uint16_t u1)
if (!device)
return HAL_STATUS_FAILED;
- if (!set_security(device, u1))
+ if (!set_security_by_auth(device, u1))
return HAL_STATUS_FAILED;
return HAL_STATUS_SUCCESS;
--
1.8.4