2014-08-25 12:54:17

by Jakub Tyszkowski

[permalink] [raw]
Subject: [PATCHv3 0/2] Fix accepting not encrypted connections from keyboards

v3 changes:
* extracted writing virtual unplug message to helper function
* send virtual unplug only for controll channel

v2 changes:
* checking subclass for the keyboard is done only once, when the SDP is
done. Then the default, minimum security level is stored inside
the device struct.
* fixed typo

Jakub Tyszkowski (2):
android/hid: Reject connections from unknown devices
android/hid: Force encryption for keyboards

android/hidhost.c | 59 ++++++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 47 insertions(+), 12 deletions(-)

--
1.9.1



2014-08-27 16:08:42

by Luiz Augusto von Dentz

[permalink] [raw]
Subject: Re: [PATCHv3 1/2] android/hid: Reject connections from unknown devices

Hi Jakub,

On Mon, Aug 25, 2014 at 3:54 PM, Jakub Tyszkowski
<[email protected]> wrote:
> Don't accept input from not bonded devices.
> ---
> android/hidhost.c | 33 ++++++++++++++++++++++++---------
> 1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/android/hidhost.c b/android/hidhost.c
> index da5f818..bb725c1 100644
> --- a/android/hidhost.c
> +++ b/android/hidhost.c
> @@ -950,6 +950,19 @@ failed:
> status);
> }
>
> +static bool bt_hid_write_virtual_unplug(GIOChannel *chan)
> +{
> + uint8_t hdr = HID_MSG_CONTROL | HID_VIRTUAL_CABLE_UNPLUG;
> + int fd = g_io_channel_unix_get_fd(chan);
> +
> + if (write(fd, &hdr, sizeof(hdr)) == sizeof(hdr))
> + return true;
> +
> + error("hidhost: Error writing virtual unplug command: %s (%d)",
> + strerror(errno), errno);
> + return false;
> +}
> +
> static void bt_hid_virtual_unplug(const void *buf, uint16_t len)
> {
> const struct hal_cmd_hidhost_virtual_unplug *cmd = buf;
> @@ -957,8 +970,6 @@ static void bt_hid_virtual_unplug(const void *buf, uint16_t len)
> GSList *l;
> uint8_t status;
> bdaddr_t dst;
> - uint8_t hdr;
> - int fd;
>
> DBG("");
>
> @@ -977,13 +988,7 @@ static void bt_hid_virtual_unplug(const void *buf, uint16_t len)
> goto failed;
> }
>
> - hdr = HID_MSG_CONTROL | HID_VIRTUAL_CABLE_UNPLUG;
> -
> - fd = g_io_channel_unix_get_fd(dev->ctrl_io);
> -
> - if (write(fd, &hdr, sizeof(hdr)) < 0) {
> - error("hidhost: Error writing virtual unplug command: %s (%d)",
> - strerror(errno), errno);
> + if (!bt_hid_write_virtual_unplug(dev->ctrl_io)) {
> status = HAL_STATUS_FAILED;
> goto failed;
> }
> @@ -1410,6 +1415,16 @@ static void connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
> ba2str(&dst, address);
> DBG("Incoming connection from %s on PSM %d", address, psm);
>
> + if (!bt_device_is_bonded(&dst)) {
> + warn("hidhost: Rejecting connection from unknown device %s",
> + address);
> + if (psm == L2CAP_PSM_HIDP_CTRL)
> + bt_hid_write_virtual_unplug(chan);
> +
> + g_io_channel_shutdown(chan, TRUE, NULL);
> + return;
> + }
> +
> switch (psm) {
> case L2CAP_PSM_HIDP_CTRL:
> l = g_slist_find_custom(devices, &dst, device_cmp);
> --
> 1.9.1
>
> --

Pushed, thanks.

--
Luiz Augusto von Dentz

2014-08-25 12:54:18

by Jakub Tyszkowski

[permalink] [raw]
Subject: [PATCHv3 1/2] android/hid: Reject connections from unknown devices

Don't accept input from not bonded devices.
---
android/hidhost.c | 33 ++++++++++++++++++++++++---------
1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/android/hidhost.c b/android/hidhost.c
index da5f818..bb725c1 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -950,6 +950,19 @@ failed:
status);
}

+static bool bt_hid_write_virtual_unplug(GIOChannel *chan)
+{
+ uint8_t hdr = HID_MSG_CONTROL | HID_VIRTUAL_CABLE_UNPLUG;
+ int fd = g_io_channel_unix_get_fd(chan);
+
+ if (write(fd, &hdr, sizeof(hdr)) == sizeof(hdr))
+ return true;
+
+ error("hidhost: Error writing virtual unplug command: %s (%d)",
+ strerror(errno), errno);
+ return false;
+}
+
static void bt_hid_virtual_unplug(const void *buf, uint16_t len)
{
const struct hal_cmd_hidhost_virtual_unplug *cmd = buf;
@@ -957,8 +970,6 @@ static void bt_hid_virtual_unplug(const void *buf, uint16_t len)
GSList *l;
uint8_t status;
bdaddr_t dst;
- uint8_t hdr;
- int fd;

DBG("");

@@ -977,13 +988,7 @@ static void bt_hid_virtual_unplug(const void *buf, uint16_t len)
goto failed;
}

- hdr = HID_MSG_CONTROL | HID_VIRTUAL_CABLE_UNPLUG;
-
- fd = g_io_channel_unix_get_fd(dev->ctrl_io);
-
- if (write(fd, &hdr, sizeof(hdr)) < 0) {
- error("hidhost: Error writing virtual unplug command: %s (%d)",
- strerror(errno), errno);
+ if (!bt_hid_write_virtual_unplug(dev->ctrl_io)) {
status = HAL_STATUS_FAILED;
goto failed;
}
@@ -1410,6 +1415,16 @@ static void connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
ba2str(&dst, address);
DBG("Incoming connection from %s on PSM %d", address, psm);

+ if (!bt_device_is_bonded(&dst)) {
+ warn("hidhost: Rejecting connection from unknown device %s",
+ address);
+ if (psm == L2CAP_PSM_HIDP_CTRL)
+ bt_hid_write_virtual_unplug(chan);
+
+ g_io_channel_shutdown(chan, TRUE, NULL);
+ return;
+ }
+
switch (psm) {
case L2CAP_PSM_HIDP_CTRL:
l = g_slist_find_custom(devices, &dst, device_cmp);
--
1.9.1


2014-08-25 12:54:19

by Jakub Tyszkowski

[permalink] [raw]
Subject: [PATCHv3 2/2] android/hid: Force encryption for keyboards

Encryption is mandatory for keyboards. Instead of using hardcoded security
level it's now set per device (with LOW as default) and raised for
keyboards (after the SDP query is done). This level is then used to
establish outgoing connections and raising the security level of the
incomming ones.
---
android/hidhost.c | 26 +++++++++++++++++++++++---
1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/android/hidhost.c b/android/hidhost.c
index bb725c1..5b31c95 100644
--- a/android/hidhost.c
+++ b/android/hidhost.c
@@ -110,6 +110,7 @@ struct hid_device {
uint8_t last_hid_msg;
struct bt_hog *hog;
guint reconnect_id;
+ int sec_level;
};

static int device_cmp(gconstpointer s, gconstpointer user_data)
@@ -162,6 +163,8 @@ static struct hid_device *hid_device_new(const bdaddr_t *addr)
dev = g_new0(struct hid_device, 1);
bacpy(&dev->dst, addr);
dev->state = HAL_HIDHOST_STATE_DISCONNECTED;
+ dev->sec_level = BT_IO_SEC_LOW;
+
devices = g_slist_append(devices, dev);

return dev;
@@ -594,7 +597,7 @@ static void control_connect_cb(GIOChannel *chan, GError *conn_err,
BT_IO_OPT_SOURCE_BDADDR, &adapter_addr,
BT_IO_OPT_DEST_BDADDR, &dev->dst,
BT_IO_OPT_PSM, L2CAP_PSM_HIDP_INTR,
- BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW,
+ BT_IO_OPT_SEC_LEVEL, dev->sec_level,
BT_IO_OPT_INVALID);
if (!dev->intr_io) {
error("hidhost: Failed to connect interrupt channel (%s)",
@@ -640,9 +643,14 @@ static void hid_sdp_search_cb(sdp_list_t *recs, int err, gpointer data)
dev->country = data->val.uint8;

data = sdp_data_get(rec, SDP_ATTR_HID_DEVICE_SUBCLASS);
- if (data)
+ if (data) {
dev->subclass = data->val.uint8;

+ /* Encryption is mandatory for keyboards */
+ if (dev->subclass & 0x40)
+ dev->sec_level = BT_IO_SEC_MEDIUM;
+ }
+
data = sdp_data_get(rec, SDP_ATTR_HID_BOOT_DEVICE);
if (data)
dev->boot_dev = data->val.uint8;
@@ -673,6 +681,18 @@ static void hid_sdp_search_cb(sdp_list_t *recs, int err, gpointer data)
}

if (dev->ctrl_io) {
+ /* Raise the security level for this device if needed. */
+ if ((dev->sec_level > BT_IO_SEC_LOW) &&
+ !bt_io_set(dev->ctrl_io, &gerr,
+ BT_IO_OPT_SEC_LEVEL, dev->sec_level,
+ BT_IO_OPT_INVALID)) {
+ error("hidhost: Cannot raise security level: %s",
+ gerr->message);
+ g_error_free(gerr);
+
+ goto fail;
+ }
+
if (uhid_create(dev) < 0)
goto fail;
return;
@@ -682,7 +702,7 @@ static void hid_sdp_search_cb(sdp_list_t *recs, int err, gpointer data)
BT_IO_OPT_SOURCE_BDADDR, &adapter_addr,
BT_IO_OPT_DEST_BDADDR, &dev->dst,
BT_IO_OPT_PSM, L2CAP_PSM_HIDP_CTRL,
- BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_LOW,
+ BT_IO_OPT_SEC_LEVEL, dev->sec_level,
BT_IO_OPT_INVALID);
if (gerr) {
error("hidhost: Failed to connect control channel (%s)",
--
1.9.1