[PATCH BlueZ v2 0/8] Support Sixaxis gamepad with classic bonded only

[PATCH BlueZ v2 0/8] Support Sixaxis gamepad with classic bonded only

[Ludovico de Nittis]

This series adds a new "SixaxisCablePairing" property to allow us to
indentify sixaxis devices that have been paired using the canonical USB
cable method. With that information, we can dynamically enforce
encryption to drastically reduce the attack surface, compared to just
disabling the "ClassicBondedOnly" property.

The "SixaxisCablePairing" property is exposed via D-Bus to allow
clients to potentually show this information to end users.

As far as I can tell, starting the listening input server with
BT_IO_SEC_LOW and then bumping it in `hidp_add_connection()` should not
have any negative effect regarding the overall security. However,
please let me know if it turns out not being the case. 

The last commit "input: Validate the Sixaxis HID report descriptor" can
probably be reviewed and merged separately if needed.

Addresses https://github.com/bluez/bluez/issues/1165

Changes in v2:
 - Start the listening input server with BT_IO_SEC_LOW only if we
   actually have a known sixaxis device

Ludovico de Nittis (8):
  src: Add new SixaxisCablePairing property
  client: Print SixaxisCablePairing value if BlueZ was compiled with
    sixaxis
  plugins: Set SixaxisCablePairing property when pairing a sixaxis with
    USB
  adapter: Add btd_adapter_has_sixaxis_cable_pairing()
  input: Automatically use security level low when using a sixaxis
    device
  adapter: Set server security level in load_devices()
  sixaxis: Set security level when adding a sixaxis device
  input: Validate the Sixaxis HID report descriptor

 client/main.c            |  3 ++
 configure.ac             |  3 ++
 doc/org.bluez.Device.rst |  6 ++++
 plugins/sixaxis.c        |  8 ++++-
 profiles/input/device.c  | 77 ++++++++++++++++++++++++++++++++++++++--
 profiles/input/manager.c |  3 +-
 profiles/input/server.c  | 62 ++++++++++++++++++++++++++++++--
 profiles/input/server.h  |  3 +-
 src/adapter.c            | 24 +++++++++++++
 src/adapter.h            |  1 +
 src/device.c             | 44 +++++++++++++++++++++++
 src/device.h             |  3 ++
 12 files changed, 229 insertions(+), 8 deletions(-)

-- 
2.49.0

[PATCH BlueZ v2 1/8] src: Add new SixaxisCablePairing property

[Ludovico de Nittis]

This adds initial support for a new SixaxisCablePairing property.
The property can be used to set a device as being a "real" Sixaxis
gamepad and then handle it differently when needed.
---
 doc/org.bluez.Device.rst |  6 ++++++
 src/device.c             | 44 ++++++++++++++++++++++++++++++++++++++++
 src/device.h             |  3 +++
 3 files changed, 53 insertions(+)

diff --git a/doc/org.bluez.Device.rst b/doc/org.bluez.Device.rst
index 13328249b..2d85d0dc2 100644
--- a/doc/org.bluez.Device.rst
+++ b/doc/org.bluez.Device.rst
@@ -279,6 +279,12 @@ boolean LegacyPairing [readonly]
 	Bluetooth 2.1 (or newer) devices that have disabled Extended Inquiry
 	Response support.
 
+boolean SixaxisCablePairing [readonly]
+``````````````````````````````````````
+
+	Set to true if the device was paired using the Sixaxis USB custom protocol.
+	If true, the device connection will happen without enforcing encryption.
+
 string Modalias [readonly, optional]
 ````````````````````````````````````
 
diff --git a/src/device.c b/src/device.c
index b82a905f9..b11b5c8e4 100644
--- a/src/device.c
+++ b/src/device.c
@@ -239,6 +239,7 @@ struct btd_device {
 	GSList		*watches;		/* List of disconnect_data */
 	bool		temporary;
 	bool		connectable;
+	bool		sixaxis_cable_pairing;
 	unsigned int	disconn_timer;
 	unsigned int	discov_timer;
 	unsigned int	temporary_timer;	/* Temporary/disappear timer */
@@ -507,6 +508,9 @@ static gboolean store_device_info_cb(gpointer user_data)
 	g_key_file_set_boolean(key_file, "General", "Blocked",
 							device->blocked);
 
+	g_key_file_set_boolean(key_file, "General", "SixaxisCablePairing",
+							device->sixaxis_cable_pairing);
+
 	if (device->wake_override != WAKE_FLAG_DEFAULT) {
 		g_key_file_set_boolean(key_file, "General", "WakeAllowed",
 				       device->wake_override ==
@@ -908,6 +912,11 @@ bool btd_device_is_trusted(struct btd_device *device)
 	return device->trusted;
 }
 
+bool device_is_sixaxis_cable_pairing(struct btd_device *device)
+{
+	return device->sixaxis_cable_pairing;
+}
+
 static gboolean dev_property_get_address(const GDBusPropertyTable *property,
 					DBusMessageIter *iter, void *data)
 {
@@ -1153,6 +1162,17 @@ static gboolean dev_property_get_legacy(const GDBusPropertyTable *property,
 	return TRUE;
 }
 
+static gboolean dev_property_get_sixaxis_cable_pairing(const GDBusPropertyTable *property,
+					DBusMessageIter *iter, void *data)
+{
+	struct btd_device *device = data;
+	dbus_bool_t val = device->sixaxis_cable_pairing;
+
+	dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &val);
+
+	return TRUE;
+}
+
 static gboolean dev_property_get_rssi(const GDBusPropertyTable *property,
 					DBusMessageIter *iter, void *data)
 {
@@ -3483,6 +3503,7 @@ static const GDBusPropertyTable device_properties[] = {
 	{ "Trusted", "b", dev_property_get_trusted, dev_property_set_trusted },
 	{ "Blocked", "b", dev_property_get_blocked, dev_property_set_blocked },
 	{ "LegacyPairing", "b", dev_property_get_legacy },
+	{ "SixaxisCablePairing", "b", dev_property_get_sixaxis_cable_pairing },
 	{ "RSSI", "n", dev_property_get_rssi, NULL, dev_property_exists_rssi },
 	{ "Connected", "b", dev_property_get_connected },
 	{ "UUIDs", "as", dev_property_get_uuids },
@@ -4062,6 +4083,9 @@ next:
 	if (blocked)
 		device_block(device, FALSE);
 
+	device->sixaxis_cable_pairing = g_key_file_get_boolean(key_file, "General",
+							"SixaxisCablePairing", NULL);
+
 	/* Load device profile list */
 	uuids = g_key_file_get_string_list(key_file, "General", "Services",
 						NULL, NULL);
@@ -4857,6 +4881,9 @@ void device_merge_duplicate(struct btd_device *dev, struct btd_device *dup)
 	dev->trusted = dup->trusted;
 	dev->blocked = dup->blocked;
 
+	/* Skip sixaxis_cable_pairing property because it doesn't support LE, so we
+	 * never expect to have a duplicate device in that case */
+
 	for (l = dup->uuids; l; l = g_slist_next(l))
 		dev->uuids = g_slist_append(dev->uuids, g_strdup(l->data));
 
@@ -6416,6 +6443,23 @@ void device_set_legacy(struct btd_device *device, bool legacy)
 					DEVICE_INTERFACE, "LegacyPairing");
 }
 
+void device_set_sixaxis_cable_pairing(struct btd_device *device,
+							gboolean sixaxis_cable_pairing)
+{
+	if (!device)
+		return;
+
+	if (device->sixaxis_cable_pairing == sixaxis_cable_pairing)
+		return;
+
+	DBG("setting sixaxis cable pairing %d", sixaxis_cable_pairing);
+
+	device->sixaxis_cable_pairing = sixaxis_cable_pairing;
+
+	g_dbus_emit_property_changed(dbus_conn, device->path,
+					DEVICE_INTERFACE, "SixaxisCablePairing");
+}
+
 void device_store_svc_chng_ccc(struct btd_device *device, uint8_t bdaddr_type,
 								uint16_t value)
 {
diff --git a/src/device.h b/src/device.h
index 2e4a9771d..c0fa6ec3a 100644
--- a/src/device.h
+++ b/src/device.h
@@ -94,6 +94,7 @@ bool device_is_connectable(struct btd_device *device);
 bool device_is_paired(struct btd_device *device, uint8_t bdaddr_type);
 bool device_is_bonded(struct btd_device *device, uint8_t bdaddr_type);
 bool btd_device_is_trusted(struct btd_device *device);
+bool device_is_sixaxis_cable_pairing(struct btd_device *device);
 void device_set_paired(struct btd_device *dev, uint8_t bdaddr_type);
 void device_set_unpaired(struct btd_device *dev, uint8_t bdaddr_type);
 void btd_device_set_temporary(struct btd_device *device, bool temporary);
@@ -101,6 +102,8 @@ void btd_device_set_trusted(struct btd_device *device, gboolean trusted);
 void btd_device_set_connectable(struct btd_device *device, bool connectable);
 void device_set_bonded(struct btd_device *device, uint8_t bdaddr_type);
 void device_set_legacy(struct btd_device *device, bool legacy);
+void device_set_sixaxis_cable_pairing(struct btd_device *device,
+							gboolean sixaxis_cable_pairing);
 void device_set_rssi_with_delta(struct btd_device *device, int8_t rssi,
 							int8_t delta_threshold);
 void device_set_rssi(struct btd_device *device, int8_t rssi);
-- 
2.49.0

[PATCH BlueZ v2 2/8] client: Print SixaxisCablePairing value if BlueZ was compiled with sixaxis

[Ludovico de Nittis]

If BlueZ was compiled with sixaxis support enabled, print in output the
property value of SixaxisCablePairing when using `bluetoothctl info`.
---
 client/main.c | 3 +++
 configure.ac  | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/client/main.c b/client/main.c
index 6039aa50c..bda8983e4 100644
--- a/client/main.c
+++ b/client/main.c
@@ -1705,6 +1705,9 @@ static void cmd_info(int argc, char *argv[])
 	print_property(proxy, "Connected");
 	print_property(proxy, "WakeAllowed");
 	print_property(proxy, "LegacyPairing");
+#ifdef HAVE_SIXAXIS
+	print_property(proxy, "SixaxisCablePairing");
+#endif
 	print_uuids(proxy);
 	print_property(proxy, "Modalias");
 	print_property(proxy, "ManufacturerData");
diff --git a/configure.ac b/configure.ac
index 1e089aaa7..ec509c97e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -420,6 +420,9 @@ AC_ARG_ENABLE(sixaxis, AS_HELP_STRING([--enable-sixaxis],
 		[enable sixaxis plugin]), [enable_sixaxis=${enableval}])
 AM_CONDITIONAL(SIXAXIS, test "${enable_sixaxis}" = "yes" &&
 					 test "${enable_udev}" != "no")
+if (test "${enable_sixaxis}" = "yes" && test "${enable_udev}" != "no"); then
+	AC_DEFINE(HAVE_SIXAXIS, 1, [Define to 1 if sixaxis plugin is enabled])
+fi
 
 AC_ARG_ENABLE(hid2hci, AS_HELP_STRING([--enable-hid2hci],
 		[enable hid2hci tool]), [enable_hid2hci=${enableval}])
-- 
2.49.0

[PATCH BlueZ v2 3/8] plugins: Set SixaxisCablePairing property when pairing a sixaxis with USB

[Ludovico de Nittis]

If we are pairing a Sixaxis device using a USB cable, we set the
SixaxisCablePairing property to flag that event.
---
 plugins/sixaxis.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index 3e69f1dd2..84dd3891b 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -296,10 +296,13 @@ static void agent_auth_cb(DBusError *derr, void *user_data)
 	remove_device = false;
 	btd_device_set_temporary(closure->device, false);
 
-	if (closure->type == CABLE_PAIRING_SIXAXIS)
+	if (closure->type == CABLE_PAIRING_SIXAXIS) {
 		btd_device_set_record(closure->device, HID_UUID,
 						 SIXAXIS_HID_SDP_RECORD);
 
+		device_set_sixaxis_cable_pairing(closure->device, true);
+	}
+
 	ba2str(&closure->bdaddr, device_addr);
 	ba2str(&central_bdaddr, central_addr);
 	ba2str(adapter_bdaddr, adapter_addr);
-- 
2.49.0

[PATCH BlueZ v2 4/8] adapter: Add btd_adapter_has_sixaxis_cable_pairing()

[Ludovico de Nittis]

Add a function that can be used to know if any of the known devices have
the `SixaxisCablePairing` property set.
---
 src/adapter.c | 17 +++++++++++++++++
 src/adapter.h |  1 +
 2 files changed, 18 insertions(+)

diff --git a/src/adapter.c b/src/adapter.c
index c21b38095..75b962b5d 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -412,6 +412,23 @@ uint16_t btd_adapter_get_index(struct btd_adapter *adapter)
 	return adapter->dev_id;
 }
 
+bool btd_adapter_has_sixaxis_cable_pairing(struct btd_adapter *adapter)
+{
+	GSList *l;
+
+	if (!adapter)
+		return false;
+
+	for (l = adapter->devices; l; l = l->next) {
+		struct btd_device *device = l->data;
+
+		if (device_is_sixaxis_cable_pairing(device))
+			return true;
+	}
+
+	return false;
+}
+
 static gboolean process_auth_queue(gpointer user_data);
 
 static void dev_class_changed_callback(uint16_t index, uint16_t length,
diff --git a/src/adapter.h b/src/adapter.h
index 8dfbe762e..6caff5c84 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -30,6 +30,7 @@ struct queue;
 struct btd_adapter *btd_adapter_get_default(void);
 bool btd_adapter_is_default(struct btd_adapter *adapter);
 uint16_t btd_adapter_get_index(struct btd_adapter *adapter);
+bool btd_adapter_has_sixaxis_cable_pairing(struct btd_adapter *adapter);
 
 typedef void (*adapter_cb) (struct btd_adapter *adapter, gpointer user_data);
 
-- 
2.49.0

[PATCH BlueZ v2 5/8] input: Automatically use security level low when using a sixaxis device

[Ludovico de Nittis]

BT_IO_SEC_LOW is the only way to allow Sixaxis devices to establish
a connection.

This adds the ability to start the listening input server with
BT_IO_SEC_LOW to avoid breaking the Sixaxis support, and then,
in `hidp_add_connection()`, we check if either
`classic_bonded_only` was disabled or if this device is a Sixaxis. If
neither are true, we bump the security back to BT_IO_SEC_MEDIUM, i.e.
enforcing encryption.

This allows supporting the Sixaxis gamepad without having to change the
classic bonded only option.

This doesn't cover the case where a sixaxis device gets loaded from
storage, or when we are attempting to connect a new sixaxis. Both cases
will be handled with followup commits.
---
 profiles/input/device.c  |  6 ++--
 profiles/input/manager.c |  3 +-
 profiles/input/server.c  | 62 ++++++++++++++++++++++++++++++++++++++--
 profiles/input/server.h  |  3 +-
 4 files changed, 67 insertions(+), 7 deletions(-)

diff --git a/profiles/input/device.c b/profiles/input/device.c
index 3627573e7..9f05757a6 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -1088,8 +1088,10 @@ static int hidp_add_connection(struct input_device *idev)
 	if (device_name_known(idev->device))
 		device_get_name(idev->device, req->name, sizeof(req->name));
 
+	sixaxis_cable_pairing = device_is_sixaxis_cable_pairing(idev->device);
+
 	/* Make sure the device is bonded if required */
-	if (classic_bonded_only && !input_device_bonded(idev)) {
+	if (!sixaxis_cable_pairing && classic_bonded_only && !input_device_bonded(idev)) {
 		error("Rejected connection from !bonded device %s", idev->path);
 		goto cleanup;
 	}
@@ -1098,7 +1100,7 @@ static int hidp_add_connection(struct input_device *idev)
 	/* Some platforms may choose to require encryption for all devices */
 	/* Note that this only matters for pre 2.1 devices as otherwise the */
 	/* device is encrypted by default by the lower layers */
-	if (classic_bonded_only || idev->type == BT_UHID_KEYBOARD) {
+	if (!sixaxis_cable_pairing && (classic_bonded_only || idev->type == BT_UHID_KEYBOARD)) {
 		if (!bt_io_set(idev->intr_io, &gerr,
 					BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_MEDIUM,
 					BT_IO_OPT_INVALID)) {
diff --git a/profiles/input/manager.c b/profiles/input/manager.c
index d1accc24f..0a1d5c197 100644
--- a/profiles/input/manager.c
+++ b/profiles/input/manager.c
@@ -33,7 +33,8 @@
 
 static int hid_server_probe(struct btd_profile *p, struct btd_adapter *adapter)
 {
-	return server_start(btd_adapter_get_address(adapter));
+	return server_start(btd_adapter_get_address(adapter),
+				btd_adapter_has_sixaxis_cable_pairing(adapter));
 }
 
 static void hid_server_remove(struct btd_profile *p,
diff --git a/profiles/input/server.c b/profiles/input/server.c
index 79cf08a66..b4ea5daa9 100644
--- a/profiles/input/server.c
+++ b/profiles/input/server.c
@@ -266,12 +266,22 @@ drop:
 	g_io_channel_shutdown(chan, TRUE, NULL);
 }
 
-int server_start(const bdaddr_t *src)
+static BtIOSecLevel get_necessary_sec_level(bool device_sixaxis_cable_pairing)
+{
+	/* Use lower security to allow the Sixaxis gamepad to connect. */
+	/* Unless classic bonded only mode is disabled, the security level */
+	/* will be bumped again for non sixaxis devices in hidp_add_connection() */
+	if (device_sixaxis_cable_pairing)
+		return BT_IO_SEC_LOW;
+
+	return input_get_classic_bonded_only() ? BT_IO_SEC_MEDIUM : BT_IO_SEC_LOW;
+}
+
+int server_start(const bdaddr_t *src, bool device_sixaxis_cable_pairing)
 {
 	struct input_server *server;
 	GError *err = NULL;
-	BtIOSecLevel sec_level = input_get_classic_bonded_only() ?
-					BT_IO_SEC_MEDIUM : BT_IO_SEC_LOW;
+	const BtIOSecLevel sec_level = get_necessary_sec_level(device_sixaxis_cable_pairing);
 
 	server = g_new0(struct input_server, 1);
 	bacpy(&server->src, src);
@@ -308,6 +318,52 @@ int server_start(const bdaddr_t *src)
 	return 0;
 }
 
+int server_set_sixaxis_cable_pairing(const bdaddr_t *src, bool device_sixaxis_cable_pairing)
+{
+	struct input_server *server;
+	GSList *l;
+	BtIOSecLevel sec_level;
+	const BtIOSecLevel new_sec_level = get_necessary_sec_level(device_sixaxis_cable_pairing);
+	GError *err = NULL;
+
+	l = g_slist_find_custom(servers, src, server_cmp);
+	if (!l)
+		return -1;
+
+	server = l->data;
+
+	bt_io_get(server->ctrl, &err, BT_IO_OPT_SEC_LEVEL, &sec_level,
+				BT_IO_OPT_INVALID);
+	if (err) {
+		error("%s", err->message);
+		g_error_free(err);
+		return -1;
+	}
+
+	if (sec_level == new_sec_level) {
+		DBG("The listening input server is already using the expected security level");
+		return -1;
+	}
+
+	DBG("Applying the new security level to the listening input server");
+
+	if (!bt_io_set(server->ctrl, &err, BT_IO_OPT_SEC_LEVEL, new_sec_level,
+							BT_IO_OPT_INVALID)) {
+		error("bt_io_set(OPT_SEC_LEVEL): %s", err->message);
+		g_error_free(err);
+		return -1;
+	}
+
+	if (!bt_io_set(server->intr, &err, BT_IO_OPT_SEC_LEVEL, new_sec_level,
+						BT_IO_OPT_INVALID)) {
+		error("bt_io_set(OPT_SEC_LEVEL): %s", err->message);
+		g_error_free(err);
+		return -1;
+	}
+
+	return 0;
+}
+
 void server_stop(const bdaddr_t *src)
 {
 	struct input_server *server;
diff --git a/profiles/input/server.h b/profiles/input/server.h
index 50f4b6135..c8a1b7095 100644
--- a/profiles/input/server.h
+++ b/profiles/input/server.h
@@ -8,5 +8,6 @@
  *
  */
 
-int server_start(const bdaddr_t *src);
+int server_start(const bdaddr_t *src, bool device_sixaxis_cable_pairing);
+int server_set_sixaxis_cable_pairing(const bdaddr_t *src, bool device_sixaxis_cable_pairing);
 void server_stop(const bdaddr_t *src);
-- 
2.49.0

[PATCH BlueZ v2 6/8] adapter: Set server security level in load_devices()

[Ludovico de Nittis]

After loading known devices from storage, change the security level if
we have a sixaxis device.
This will allow it to successfully establish a connection.
---
 src/adapter.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/adapter.c b/src/adapter.c
index 75b962b5d..03d946444 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -68,6 +68,7 @@
 #include "adv_monitor.h"
 #include "eir.h"
 #include "battery.h"
+#include "profiles/input/server.h"
 
 #define MODE_OFF		0x00
 #define MODE_CONNECTABLE	0x01
@@ -5090,6 +5091,12 @@ free:
 		g_key_file_free(key_file);
 	}
 
+	if (btd_adapter_has_sixaxis_cable_pairing(adapter)) {
+		DBG("There is at least one known sixaxis device, setting the "
+			"listening input server security level accordingly");
+		server_set_sixaxis_cable_pairing(&adapter->bdaddr, true);
+	}
+
 	closedir(dir);
 
 	load_link_keys(adapter, keys, btd_opts.debug_keys);
-- 
2.49.0

[PATCH BlueZ v2 7/8] sixaxis: Set security level when adding a sixaxis device

[Ludovico de Nittis]

When doing the cable pairing for a sixaxis, we may need to change the
listening input server security level.
This is because sixaxis gamepads can only work with the level
BT_IO_SEC_LOW.
---
 plugins/sixaxis.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/plugins/sixaxis.c b/plugins/sixaxis.c
index 84dd3891b..0e5f1e420 100644
--- a/plugins/sixaxis.c
+++ b/plugins/sixaxis.c
@@ -36,6 +36,7 @@
 #include "src/plugin.h"
 #include "src/log.h"
 #include "src/shared/util.h"
+#include "profiles/input/server.h"
 #include "profiles/input/sixaxis.h"
 
 struct authentication_closure {
@@ -301,6 +302,8 @@ static void agent_auth_cb(DBusError *derr, void *user_data)
 						 SIXAXIS_HID_SDP_RECORD);
 
 		device_set_sixaxis_cable_pairing(closure->device, true);
+
+		server_set_sixaxis_cable_pairing(adapter_bdaddr, true);
 	}
 
 	ba2str(&closure->bdaddr, device_addr);
-- 
2.49.0

[PATCH BlueZ v2 8/8] input: Validate the Sixaxis HID report descriptor

[Ludovico de Nittis]

Given that the Sixaxis devices can't work with encryption, i.e. they
only work with BT_IO_SEC_LOW, this makes it harder to notice if the
device we are talking to is the expected Sixaxis gamepad or an impostor.

To reduce the possible attack surface, we ensure that the report
descriptor that the device provided resembles what a real Sixaxis
gamepad should have. E.g. it should only have Usages for `Joystick`,
`Pointer` etc... and nothing unexpected like `Keyboard`.
---
 profiles/input/device.c | 71 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/profiles/input/device.c b/profiles/input/device.c
index 9f05757a6..6f538759b 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -1062,9 +1062,72 @@ static gboolean encrypt_notify(GIOChannel *io, GIOCondition condition,
 	return FALSE;
 }
 
+static bool validate_sixaxis_rd_data(const uint8_t *rd_data, uint16_t rd_size)
+{
+	uint16_t i;
+	size_t data_size = 0;
+
+	for (i = 0; i < rd_size; i += 1 + data_size) {
+		uint8_t b = rd_data[i];
+
+		/* Long items are reserved for future use, HID 1.11 Section 6.2.2.3 */
+		if (b == 0xFE) {
+			DBG("The sixaxis HID report descriptor has an unexpected long item");
+			return false;
+		}
+
+		/* Extract data following the HID 1.11 Section 6.2.2.2 */
+		uint8_t bSize = b & 0x03;
+		uint8_t bType = (b >> 2) & 0x03;
+		uint8_t bTag = (b >> 4) & 0x0F;
+		data_size = bSize == 3 ? 4 : bSize;
+
+		if ((i + 1 + data_size) > rd_size)
+			break;
+
+		const uint8_t *data = &rd_data[i + 1];
+
+		if (bType == 1 && bTag == 0x0 && data_size >= 1) {
+			/* Usage Page (Generic Desktop) */
+			if (data_size == 1 && data[0] == 0x01)
+				continue;
+
+			/* Usage Page (Button) */
+			if (data_size == 1 && data[0] == 0x09)
+				continue;
+
+			/* Usage Page (Vendor Defined Page 1) */
+			if (data_size == 2 && data[0] == 0x00 && data[1] == 0xFF)
+				continue;
+
+			DBG("The sixaxis HID report descriptor has an unexpected Usage Page: 0x%02X", data[0]);
+			return false;
+		}
+
+		if (bType == 2 && bTag == 0x0 && data_size >= 1) {
+			/* Usage (Joystick) */
+			if (data_size == 1 && data[0] == 0x04)
+				continue;
+
+			/* Usage (Pointer) */
+			if (data_size == 1 && data[0] == 0x01)
+				continue;
+
+			/* Axis usages, e.g. Usage (X) */
+			if (data_size == 1 && data[0] >= 0x30 && data[0] <= 0x35)
+				continue;
+
+			DBG("The sixaxis HID report descriptor has an unexpected Usage: 0x%02X", data[0]);
+			return false;
+		}
+	}
+	return true;
+}
+
 static int hidp_add_connection(struct input_device *idev)
 {
 	struct hidp_connadd_req *req;
+	bool sixaxis_cable_pairing;
 	GError *gerr = NULL;
 	int err;
 
@@ -1090,6 +1153,14 @@ static int hidp_add_connection(struct input_device *idev)
 
 	sixaxis_cable_pairing = device_is_sixaxis_cable_pairing(idev->device);
 
+	/* The Sixaxis devices must use the security level BT_IO_SEC_LOW to work. */
+	/* We reduce the attack surface by ensuring that the report descriptor only */
+	/* contains the expected Usages that a real Sixaxis gamepad has */
+	if (sixaxis_cable_pairing && !validate_sixaxis_rd_data(req->rd_data, req->rd_size)) {
+		error("The sixaxis HID SDP record has unexpected entries, rejecting the connection to %s", idev->path);
+		goto cleanup;
+	}
+
 	/* Make sure the device is bonded if required */
 	if (!sixaxis_cable_pairing && classic_bonded_only && !input_device_bonded(idev)) {
 		error("Rejected connection from !bonded device %s", idev->path);
-- 
2.49.0

Re: [PATCH BlueZ v2 1/8] src: Add new SixaxisCablePairing property

[Luiz Augusto von Dentz]

Hi Ludovico,

On Wed, Apr 23, 2025 at 10:41 AM Ludovico de Nittis
<ludovico.denittis@collabora.com> wrote:
>
> This adds initial support for a new SixaxisCablePairing property.
> The property can be used to set a device as being a "real" Sixaxis
> gamepad and then handle it differently when needed.
> ---
>  doc/org.bluez.Device.rst |  6 ++++++
>  src/device.c             | 44 ++++++++++++++++++++++++++++++++++++++++
>  src/device.h             |  3 +++
>  3 files changed, 53 insertions(+)
>
> diff --git a/doc/org.bluez.Device.rst b/doc/org.bluez.Device.rst
> index 13328249b..2d85d0dc2 100644
> --- a/doc/org.bluez.Device.rst
> +++ b/doc/org.bluez.Device.rst
> @@ -279,6 +279,12 @@ boolean LegacyPairing [readonly]
>         Bluetooth 2.1 (or newer) devices that have disabled Extended Inquiry
>         Response support.
>
> +boolean SixaxisCablePairing [readonly]
> +``````````````````````````````````````
> +
> +       Set to true if the device was paired using the Sixaxis USB custom protocol.
> +       If true, the device connection will happen without enforcing encryption.

Can't we make this more generic and just call it CablePairing? For the
core I don't think it really matters what method of cable pairing it
was used.

>  string Modalias [readonly, optional]
>  ````````````````````````````````````
>
> diff --git a/src/device.c b/src/device.c
> index b82a905f9..b11b5c8e4 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -239,6 +239,7 @@ struct btd_device {
>         GSList          *watches;               /* List of disconnect_data */
>         bool            temporary;
>         bool            connectable;
> +       bool            sixaxis_cable_pairing;
>         unsigned int    disconn_timer;
>         unsigned int    discov_timer;
>         unsigned int    temporary_timer;        /* Temporary/disappear timer */
> @@ -507,6 +508,9 @@ static gboolean store_device_info_cb(gpointer user_data)
>         g_key_file_set_boolean(key_file, "General", "Blocked",
>                                                         device->blocked);
>
> +       g_key_file_set_boolean(key_file, "General", "SixaxisCablePairing",
> +                                                       device->sixaxis_cable_pairing);
> +
>         if (device->wake_override != WAKE_FLAG_DEFAULT) {
>                 g_key_file_set_boolean(key_file, "General", "WakeAllowed",
>                                        device->wake_override ==
> @@ -908,6 +912,11 @@ bool btd_device_is_trusted(struct btd_device *device)
>         return device->trusted;
>  }
>
> +bool device_is_sixaxis_cable_pairing(struct btd_device *device)
> +{
> +       return device->sixaxis_cable_pairing;
> +}
> +
>  static gboolean dev_property_get_address(const GDBusPropertyTable *property,
>                                         DBusMessageIter *iter, void *data)
>  {
> @@ -1153,6 +1162,17 @@ static gboolean dev_property_get_legacy(const GDBusPropertyTable *property,
>         return TRUE;
>  }
>
> +static gboolean dev_property_get_sixaxis_cable_pairing(const GDBusPropertyTable *property,
> +                                       DBusMessageIter *iter, void *data)
> +{
> +       struct btd_device *device = data;
> +       dbus_bool_t val = device->sixaxis_cable_pairing;
> +
> +       dbus_message_iter_append_basic(iter, DBUS_TYPE_BOOLEAN, &val);
> +
> +       return TRUE;
> +}
> +
>  static gboolean dev_property_get_rssi(const GDBusPropertyTable *property,
>                                         DBusMessageIter *iter, void *data)
>  {
> @@ -3483,6 +3503,7 @@ static const GDBusPropertyTable device_properties[] = {
>         { "Trusted", "b", dev_property_get_trusted, dev_property_set_trusted },
>         { "Blocked", "b", dev_property_get_blocked, dev_property_set_blocked },
>         { "LegacyPairing", "b", dev_property_get_legacy },
> +       { "SixaxisCablePairing", "b", dev_property_get_sixaxis_cable_pairing },
>         { "RSSI", "n", dev_property_get_rssi, NULL, dev_property_exists_rssi },
>         { "Connected", "b", dev_property_get_connected },
>         { "UUIDs", "as", dev_property_get_uuids },
> @@ -4062,6 +4083,9 @@ next:
>         if (blocked)
>                 device_block(device, FALSE);
>
> +       device->sixaxis_cable_pairing = g_key_file_get_boolean(key_file, "General",
> +                                                       "SixaxisCablePairing", NULL);
> +
>         /* Load device profile list */
>         uuids = g_key_file_get_string_list(key_file, "General", "Services",
>                                                 NULL, NULL);
> @@ -4857,6 +4881,9 @@ void device_merge_duplicate(struct btd_device *dev, struct btd_device *dup)
>         dev->trusted = dup->trusted;
>         dev->blocked = dup->blocked;
>
> +       /* Skip sixaxis_cable_pairing property because it doesn't support LE, so we
> +        * never expect to have a duplicate device in that case */
> +
>         for (l = dup->uuids; l; l = g_slist_next(l))
>                 dev->uuids = g_slist_append(dev->uuids, g_strdup(l->data));
>
> @@ -6416,6 +6443,23 @@ void device_set_legacy(struct btd_device *device, bool legacy)
>                                         DEVICE_INTERFACE, "LegacyPairing");
>  }
>
> +void device_set_sixaxis_cable_pairing(struct btd_device *device,
> +                                                       gboolean sixaxis_cable_pairing)
> +{
> +       if (!device)
> +               return;
> +
> +       if (device->sixaxis_cable_pairing == sixaxis_cable_pairing)
> +               return;
> +
> +       DBG("setting sixaxis cable pairing %d", sixaxis_cable_pairing);
> +
> +       device->sixaxis_cable_pairing = sixaxis_cable_pairing;
> +
> +       g_dbus_emit_property_changed(dbus_conn, device->path,
> +                                       DEVICE_INTERFACE, "SixaxisCablePairing");
> +}
> +
>  void device_store_svc_chng_ccc(struct btd_device *device, uint8_t bdaddr_type,
>                                                                 uint16_t value)
>  {
> diff --git a/src/device.h b/src/device.h
> index 2e4a9771d..c0fa6ec3a 100644
> --- a/src/device.h
> +++ b/src/device.h
> @@ -94,6 +94,7 @@ bool device_is_connectable(struct btd_device *device);
>  bool device_is_paired(struct btd_device *device, uint8_t bdaddr_type);
>  bool device_is_bonded(struct btd_device *device, uint8_t bdaddr_type);
>  bool btd_device_is_trusted(struct btd_device *device);
> +bool device_is_sixaxis_cable_pairing(struct btd_device *device);
>  void device_set_paired(struct btd_device *dev, uint8_t bdaddr_type);
>  void device_set_unpaired(struct btd_device *dev, uint8_t bdaddr_type);
>  void btd_device_set_temporary(struct btd_device *device, bool temporary);
> @@ -101,6 +102,8 @@ void btd_device_set_trusted(struct btd_device *device, gboolean trusted);
>  void btd_device_set_connectable(struct btd_device *device, bool connectable);
>  void device_set_bonded(struct btd_device *device, uint8_t bdaddr_type);
>  void device_set_legacy(struct btd_device *device, bool legacy);
> +void device_set_sixaxis_cable_pairing(struct btd_device *device,
> +                                                       gboolean sixaxis_cable_pairing);
>  void device_set_rssi_with_delta(struct btd_device *device, int8_t rssi,
>                                                         int8_t delta_threshold);
>  void device_set_rssi(struct btd_device *device, int8_t rssi);
> --
> 2.49.0
>
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ v2 8/8] input: Validate the Sixaxis HID report descriptor

[Luiz Augusto von Dentz]

Hi Ludovico,

On Wed, Apr 23, 2025 at 10:41 AM Ludovico de Nittis
<ludovico.denittis@collabora.com> wrote:
>
> Given that the Sixaxis devices can't work with encryption, i.e. they
> only work with BT_IO_SEC_LOW, this makes it harder to notice if the
> device we are talking to is the expected Sixaxis gamepad or an impostor.
>
> To reduce the possible attack surface, we ensure that the report
> descriptor that the device provided resembles what a real Sixaxis
> gamepad should have. E.g. it should only have Usages for `Joystick`,
> `Pointer` etc... and nothing unexpected like `Keyboard`.
> ---
>  profiles/input/device.c | 71 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)
>
> diff --git a/profiles/input/device.c b/profiles/input/device.c
> index 9f05757a6..6f538759b 100644
> --- a/profiles/input/device.c
> +++ b/profiles/input/device.c
> @@ -1062,9 +1062,72 @@ static gboolean encrypt_notify(GIOChannel *io, GIOCondition condition,
>         return FALSE;
>  }
>
> +static bool validate_sixaxis_rd_data(const uint8_t *rd_data, uint16_t rd_size)
> +{
> +       uint16_t i;
> +       size_t data_size = 0;
> +
> +       for (i = 0; i < rd_size; i += 1 + data_size) {
> +               uint8_t b = rd_data[i];
> +
> +               /* Long items are reserved for future use, HID 1.11 Section 6.2.2.3 */
> +               if (b == 0xFE) {
> +                       DBG("The sixaxis HID report descriptor has an unexpected long item");
> +                       return false;
> +               }
> +
> +               /* Extract data following the HID 1.11 Section 6.2.2.2 */
> +               uint8_t bSize = b & 0x03;
> +               uint8_t bType = (b >> 2) & 0x03;
> +               uint8_t bTag = (b >> 4) & 0x0F;
> +               data_size = bSize == 3 ? 4 : bSize;
> +
> +               if ((i + 1 + data_size) > rd_size)
> +                       break;
> +
> +               const uint8_t *data = &rd_data[i + 1];
> +
> +               if (bType == 1 && bTag == 0x0 && data_size >= 1) {
> +                       /* Usage Page (Generic Desktop) */
> +                       if (data_size == 1 && data[0] == 0x01)
> +                               continue;
> +
> +                       /* Usage Page (Button) */
> +                       if (data_size == 1 && data[0] == 0x09)
> +                               continue;
> +
> +                       /* Usage Page (Vendor Defined Page 1) */
> +                       if (data_size == 2 && data[0] == 0x00 && data[1] == 0xFF)
> +                               continue;
> +
> +                       DBG("The sixaxis HID report descriptor has an unexpected Usage Page: 0x%02X", data[0]);
> +                       return false;
> +               }
> +
> +               if (bType == 2 && bTag == 0x0 && data_size >= 1) {
> +                       /* Usage (Joystick) */
> +                       if (data_size == 1 && data[0] == 0x04)
> +                               continue;
> +
> +                       /* Usage (Pointer) */
> +                       if (data_size == 1 && data[0] == 0x01)
> +                               continue;
> +
> +                       /* Axis usages, e.g. Usage (X) */
> +                       if (data_size == 1 && data[0] >= 0x30 && data[0] <= 0x35)
> +                               continue;
> +
> +                       DBG("The sixaxis HID report descriptor has an unexpected Usage: 0x%02X", data[0]);
> +                       return false;
> +               }
> +       }
> +       return true;
> +}

The code above shall probably be placed in the sixaxis plugin, so it
checks if all the reports is proper and only then set cable pairing is
complete, so we don't have to check on every connection.

>  static int hidp_add_connection(struct input_device *idev)
>  {
>         struct hidp_connadd_req *req;
> +       bool sixaxis_cable_pairing;
>         GError *gerr = NULL;
>         int err;
>
> @@ -1090,6 +1153,14 @@ static int hidp_add_connection(struct input_device *idev)
>
>         sixaxis_cable_pairing = device_is_sixaxis_cable_pairing(idev->device);
>
> +       /* The Sixaxis devices must use the security level BT_IO_SEC_LOW to work. */
> +       /* We reduce the attack surface by ensuring that the report descriptor only */
> +       /* contains the expected Usages that a real Sixaxis gamepad has */
> +       if (sixaxis_cable_pairing && !validate_sixaxis_rd_data(req->rd_data, req->rd_size)) {
> +               error("The sixaxis HID SDP record has unexpected entries, rejecting the connection to %s", idev->path);
> +               goto cleanup;
> +       }
> +
>         /* Make sure the device is bonded if required */
>         if (!sixaxis_cable_pairing && classic_bonded_only && !input_device_bonded(idev)) {
>                 error("Rejected connection from !bonded device %s", idev->path);
> --
> 2.49.0
>
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ v2 8/8] input: Validate the Sixaxis HID report descriptor

[Ludovico de Nittis]

Hi Luiz,

On 4/23/25 4:56 PM, Luiz Augusto von Dentz wrote:
> Hi Ludovico,
>
> On Wed, Apr 23, 2025 at 10:41 AM Ludovico de Nittis
> <ludovico.denittis@collabora.com> wrote:
>> Given that the Sixaxis devices can't work with encryption, i.e. they
>> only work with BT_IO_SEC_LOW, this makes it harder to notice if the
>> device we are talking to is the expected Sixaxis gamepad or an impostor.
>>
>> To reduce the possible attack surface, we ensure that the report
>> descriptor that the device provided resembles what a real Sixaxis
>> gamepad should have. E.g. it should only have Usages for `Joystick`,
>> `Pointer` etc... and nothing unexpected like `Keyboard`.
>> ---
>>   profiles/input/device.c | 71 +++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 71 insertions(+)
>>
>> diff --git a/profiles/input/device.c b/profiles/input/device.c
>> index 9f05757a6..6f538759b 100644
>> --- a/profiles/input/device.c
>> +++ b/profiles/input/device.c
>> @@ -1062,9 +1062,72 @@ static gboolean encrypt_notify(GIOChannel *io, GIOCondition condition,
>>          return FALSE;
>>   }
>>
>> +static bool validate_sixaxis_rd_data(const uint8_t *rd_data, uint16_t rd_size)
>> +{
>> +       uint16_t i;
>> +       size_t data_size = 0;
>> +
>> +       for (i = 0; i < rd_size; i += 1 + data_size) {
>> +               uint8_t b = rd_data[i];
>> +
>> +               /* Long items are reserved for future use, HID 1.11 Section 6.2.2.3 */
>> +               if (b == 0xFE) {
>> +                       DBG("The sixaxis HID report descriptor has an unexpected long item");
>> +                       return false;
>> +               }
>> +
>> +               /* Extract data following the HID 1.11 Section 6.2.2.2 */
>> +               uint8_t bSize = b & 0x03;
>> +               uint8_t bType = (b >> 2) & 0x03;
>> +               uint8_t bTag = (b >> 4) & 0x0F;
>> +               data_size = bSize == 3 ? 4 : bSize;
>> +
>> +               if ((i + 1 + data_size) > rd_size)
>> +                       break;
>> +
>> +               const uint8_t *data = &rd_data[i + 1];
>> +
>> +               if (bType == 1 && bTag == 0x0 && data_size >= 1) {
>> +                       /* Usage Page (Generic Desktop) */
>> +                       if (data_size == 1 && data[0] == 0x01)
>> +                               continue;
>> +
>> +                       /* Usage Page (Button) */
>> +                       if (data_size == 1 && data[0] == 0x09)
>> +                               continue;
>> +
>> +                       /* Usage Page (Vendor Defined Page 1) */
>> +                       if (data_size == 2 && data[0] == 0x00 && data[1] == 0xFF)
>> +                               continue;
>> +
>> +                       DBG("The sixaxis HID report descriptor has an unexpected Usage Page: 0x%02X", data[0]);
>> +                       return false;
>> +               }
>> +
>> +               if (bType == 2 && bTag == 0x0 && data_size >= 1) {
>> +                       /* Usage (Joystick) */
>> +                       if (data_size == 1 && data[0] == 0x04)
>> +                               continue;
>> +
>> +                       /* Usage (Pointer) */
>> +                       if (data_size == 1 && data[0] == 0x01)
>> +                               continue;
>> +
>> +                       /* Axis usages, e.g. Usage (X) */
>> +                       if (data_size == 1 && data[0] >= 0x30 && data[0] <= 0x35)
>> +                               continue;
>> +
>> +                       DBG("The sixaxis HID report descriptor has an unexpected Usage: 0x%02X", data[0]);
>> +                       return false;
>> +               }
>> +       }
>> +       return true;
>> +}
> The code above shall probably be placed in the sixaxis plugin, so it
> checks if all the reports is proper and only then set cable pairing is
> complete, so we don't have to check on every connection.

I was under the wrong impression that a device could update its report 
at every connection.
If this only happens at pairing time, then doing the check there is 
definitely better.

And actually, in that case, it shouldn't be needed at all for the 
sixaxis because apparently
we manually replace it already with `btd_device_set_record()` if we are 
in the
`CABLE_PAIRING_SIXAXIS` situation.

I'm gonna follow up with a v3 in a few minutes.

>>   static int hidp_add_connection(struct input_device *idev)
>>   {
>>          struct hidp_connadd_req *req;
>> +       bool sixaxis_cable_pairing;
>>          GError *gerr = NULL;
>>          int err;
>>
>> @@ -1090,6 +1153,14 @@ static int hidp_add_connection(struct input_device *idev)
>>
>>          sixaxis_cable_pairing = device_is_sixaxis_cable_pairing(idev->device);
>>
>> +       /* The Sixaxis devices must use the security level BT_IO_SEC_LOW to work. */
>> +       /* We reduce the attack surface by ensuring that the report descriptor only */
>> +       /* contains the expected Usages that a real Sixaxis gamepad has */
>> +       if (sixaxis_cable_pairing && !validate_sixaxis_rd_data(req->rd_data, req->rd_size)) {
>> +               error("The sixaxis HID SDP record has unexpected entries, rejecting the connection to %s", idev->path);
>> +               goto cleanup;
>> +       }
>> +
>>          /* Make sure the device is bonded if required */
>>          if (!sixaxis_cable_pairing && classic_bonded_only && !input_device_bonded(idev)) {
>>                  error("Rejected connection from !bonded device %s", idev->path);
>> --
>> 2.49.0
>>
>>
>

Re: [PATCH BlueZ v2 8/8] input: Validate the Sixaxis HID report descriptor

[Luiz Augusto von Dentz]

Hi Ludovico,

On Thu, Apr 24, 2025 at 10:37 AM Ludovico de Nittis
<ludovico.denittis@collabora.com> wrote:
>
> Hi Luiz,
>
> On 4/23/25 4:56 PM, Luiz Augusto von Dentz wrote:
> > Hi Ludovico,
> >
> > On Wed, Apr 23, 2025 at 10:41 AM Ludovico de Nittis
> > <ludovico.denittis@collabora.com> wrote:
> >> Given that the Sixaxis devices can't work with encryption, i.e. they
> >> only work with BT_IO_SEC_LOW, this makes it harder to notice if the
> >> device we are talking to is the expected Sixaxis gamepad or an impostor.
> >>
> >> To reduce the possible attack surface, we ensure that the report
> >> descriptor that the device provided resembles what a real Sixaxis
> >> gamepad should have. E.g. it should only have Usages for `Joystick`,
> >> `Pointer` etc... and nothing unexpected like `Keyboard`.
> >> ---
> >>   profiles/input/device.c | 71 +++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 71 insertions(+)
> >>
> >> diff --git a/profiles/input/device.c b/profiles/input/device.c
> >> index 9f05757a6..6f538759b 100644
> >> --- a/profiles/input/device.c
> >> +++ b/profiles/input/device.c
> >> @@ -1062,9 +1062,72 @@ static gboolean encrypt_notify(GIOChannel *io, GIOCondition condition,
> >>          return FALSE;
> >>   }
> >>
> >> +static bool validate_sixaxis_rd_data(const uint8_t *rd_data, uint16_t rd_size)
> >> +{
> >> +       uint16_t i;
> >> +       size_t data_size = 0;
> >> +
> >> +       for (i = 0; i < rd_size; i += 1 + data_size) {
> >> +               uint8_t b = rd_data[i];
> >> +
> >> +               /* Long items are reserved for future use, HID 1.11 Section 6.2.2.3 */
> >> +               if (b == 0xFE) {
> >> +                       DBG("The sixaxis HID report descriptor has an unexpected long item");
> >> +                       return false;
> >> +               }
> >> +
> >> +               /* Extract data following the HID 1.11 Section 6.2.2.2 */
> >> +               uint8_t bSize = b & 0x03;
> >> +               uint8_t bType = (b >> 2) & 0x03;
> >> +               uint8_t bTag = (b >> 4) & 0x0F;
> >> +               data_size = bSize == 3 ? 4 : bSize;
> >> +
> >> +               if ((i + 1 + data_size) > rd_size)
> >> +                       break;
> >> +
> >> +               const uint8_t *data = &rd_data[i + 1];
> >> +
> >> +               if (bType == 1 && bTag == 0x0 && data_size >= 1) {
> >> +                       /* Usage Page (Generic Desktop) */
> >> +                       if (data_size == 1 && data[0] == 0x01)
> >> +                               continue;
> >> +
> >> +                       /* Usage Page (Button) */
> >> +                       if (data_size == 1 && data[0] == 0x09)
> >> +                               continue;
> >> +
> >> +                       /* Usage Page (Vendor Defined Page 1) */
> >> +                       if (data_size == 2 && data[0] == 0x00 && data[1] == 0xFF)
> >> +                               continue;
> >> +
> >> +                       DBG("The sixaxis HID report descriptor has an unexpected Usage Page: 0x%02X", data[0]);
> >> +                       return false;
> >> +               }
> >> +
> >> +               if (bType == 2 && bTag == 0x0 && data_size >= 1) {
> >> +                       /* Usage (Joystick) */
> >> +                       if (data_size == 1 && data[0] == 0x04)
> >> +                               continue;
> >> +
> >> +                       /* Usage (Pointer) */
> >> +                       if (data_size == 1 && data[0] == 0x01)
> >> +                               continue;
> >> +
> >> +                       /* Axis usages, e.g. Usage (X) */
> >> +                       if (data_size == 1 && data[0] >= 0x30 && data[0] <= 0x35)
> >> +                               continue;
> >> +
> >> +                       DBG("The sixaxis HID report descriptor has an unexpected Usage: 0x%02X", data[0]);
> >> +                       return false;
> >> +               }
> >> +       }
> >> +       return true;
> >> +}
> > The code above shall probably be placed in the sixaxis plugin, so it
> > checks if all the reports is proper and only then set cable pairing is
> > complete, so we don't have to check on every connection.
>
> I was under the wrong impression that a device could update its report
> at every connection.
> If this only happens at pairing time, then doing the check there is
> definitely better.
>
> And actually, in that case, it shouldn't be needed at all for the
> sixaxis because apparently
> we manually replace it already with `btd_device_set_record()` if we are
> in the
> `CABLE_PAIRING_SIXAXIS` situation.
>
> I'm gonna follow up with a v3 in a few minutes.

Yeah it looks like the record is created directly by sixaxis plugin,
that said Im not sure what is the actual record if we attempt to
discover it over SDP? In the other hand that means there is nothing
for us to validate the behavior of the device, well it will be limited
to what the kernel driver is capable of but Im not sure if the kernel
driver does limit the inputs, etc.

>
> >>   static int hidp_add_connection(struct input_device *idev)
> >>   {
> >>          struct hidp_connadd_req *req;
> >> +       bool sixaxis_cable_pairing;
> >>          GError *gerr = NULL;
> >>          int err;
> >>
> >> @@ -1090,6 +1153,14 @@ static int hidp_add_connection(struct input_device *idev)
> >>
> >>          sixaxis_cable_pairing = device_is_sixaxis_cable_pairing(idev->device);
> >>
> >> +       /* The Sixaxis devices must use the security level BT_IO_SEC_LOW to work. */
> >> +       /* We reduce the attack surface by ensuring that the report descriptor only */
> >> +       /* contains the expected Usages that a real Sixaxis gamepad has */
> >> +       if (sixaxis_cable_pairing && !validate_sixaxis_rd_data(req->rd_data, req->rd_size)) {
> >> +               error("The sixaxis HID SDP record has unexpected entries, rejecting the connection to %s", idev->path);
> >> +               goto cleanup;
> >> +       }
> >> +
> >>          /* Make sure the device is bonded if required */
> >>          if (!sixaxis_cable_pairing && classic_bonded_only && !input_device_bonded(idev)) {
> >>                  error("Rejected connection from !bonded device %s", idev->path);
> >> --
> >> 2.49.0
> >>
> >>
> >
>


-- 
Luiz Augusto von Dentz