| Message ID | 20250423144020.358828-9-ludovico.denittis@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support Sixaxis gamepad with classic bonded only | expand |
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 >> >> >
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 > >> > >> > > >
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);