When connecting a LE keyboard, if the user input the wrong passkey, the
stack would keep auto connect and thus allow the user to retry the
passkey indefinitely which is a security concern. This fix would
disallow the auto connect if the authentication failed.
---
Changes in v1:
- Initial change
src/device.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/device.c b/src/device.c
index a4b5968d4..764cca60e 100644
--- a/src/device.c
+++ b/src/device.c
@@ -6033,11 +6033,17 @@ void device_bonding_complete(struct btd_device *device, uint8_t bdaddr_type,
device_cancel_authentication(device, TRUE);
/* Put the device back to the temporary state so that it will be
- * treated as a newly discovered device.
+ * treated as a newly discovered device; also disable auto
+ * connect.
*/
if (!device_is_paired(device, bdaddr_type) &&
- !device_is_trusted(device))
+ !device_is_trusted(device)) {
btd_device_set_temporary(device, true);
+ if (device->auto_connect) {
+ device->disable_auto_connect = TRUE;
+ device_set_auto_connect(device, FALSE);
+ }
+ }
device_bonding_failed(device, status);
return;
--
2.28.0.681.g6f77f65b4e-goog
Hi Yu Liu,
On Thu, Sep 24, 2020 at 2:08 PM Yu Liu <[email protected]> wrote:
>
> When connecting a LE keyboard, if the user input the wrong passkey, the
> stack would keep auto connect and thus allow the user to retry the
> passkey indefinitely which is a security concern. This fix would
> disallow the auto connect if the authentication failed.
>
> ---
>
> Changes in v1:
> - Initial change
>
> src/device.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/src/device.c b/src/device.c
> index a4b5968d4..764cca60e 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -6033,11 +6033,17 @@ void device_bonding_complete(struct btd_device *device, uint8_t bdaddr_type,
> device_cancel_authentication(device, TRUE);
>
> /* Put the device back to the temporary state so that it will be
> - * treated as a newly discovered device.
> + * treated as a newly discovered device; also disable auto
> + * connect.
> */
> if (!device_is_paired(device, bdaddr_type) &&
> - !device_is_trusted(device))
> + !device_is_trusted(device)) {
> btd_device_set_temporary(device, true);
> + if (device->auto_connect) {
> + device->disable_auto_connect = TRUE;
> + device_set_auto_connect(device, FALSE);
> + }
> + }
While this looks correct we could perhaps design it in such a way that
only trusted device are allowed to set auto connect, though that may
need a lot more changes than this one so I would be fine applying this
so we can think about the implications, also perhaps this should be
incorporated to btd_device_set_temporary since a temporary device
should probably not be left in to auto-connect and only a user action
to attempt to use it again to restore its auto connect status.
To summarize we should answer 2 questions:
1. Should an untrusted device be allowed to be marked as auto-connect?
Maybe, though we didn't consider trusted property for auto-connect,
but I think using it might make auto-connect more predictable to the
upper layer.
2. Should a temporary device be allowed to be marked as auto-connect?
Most likely not, it should be removed if the user doesn't take any
action during the temporary grace period.
> device_bonding_failed(device, status);
> return;
> --
> 2.28.0.681.g6f77f65b4e-goog
>
--
Luiz Augusto von Dentz
Hi Yu Liu,
On Thu, Sep 24, 2020 at 2:50 PM Yu Liu <[email protected]> wrote:
>
> Hi Luiz,
>
> I was thinking about putting this into btd_device_set_temporary as well, so as the short term solution you think this should be the way to go while we look into only making trusted devices auto connectable? I can make the change then.
Yep, lets have it as part of btd_device_set_temporary first.
> On Thu, Sep 24, 2020 at 2:40 PM Luiz Augusto von Dentz <[email protected]> wrote:
>>
>> Hi Yu Liu,
>>
>> On Thu, Sep 24, 2020 at 2:08 PM Yu Liu <[email protected]> wrote:
>> >
>> > When connecting a LE keyboard, if the user input the wrong passkey, the
>> > stack would keep auto connect and thus allow the user to retry the
>> > passkey indefinitely which is a security concern. This fix would
>> > disallow the auto connect if the authentication failed.
>> >
>> > ---
>> >
>> > Changes in v1:
>> > - Initial change
>> >
>> > src/device.c | 10 ++++++++--
>> > 1 file changed, 8 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/src/device.c b/src/device.c
>> > index a4b5968d4..764cca60e 100644
>> > --- a/src/device.c
>> > +++ b/src/device.c
>> > @@ -6033,11 +6033,17 @@ void device_bonding_complete(struct btd_device *device, uint8_t bdaddr_type,
>> > device_cancel_authentication(device, TRUE);
>> >
>> > /* Put the device back to the temporary state so that it will be
>> > - * treated as a newly discovered device.
>> > + * treated as a newly discovered device; also disable auto
>> > + * connect.
>> > */
>> > if (!device_is_paired(device, bdaddr_type) &&
>> > - !device_is_trusted(device))
>> > + !device_is_trusted(device)) {
>> > btd_device_set_temporary(device, true);
>> > + if (device->auto_connect) {
>> > + device->disable_auto_connect = TRUE;
>> > + device_set_auto_connect(device, FALSE);
>> > + }
>> > + }
>>
>> While this looks correct we could perhaps design it in such a way that
>> only trusted device are allowed to set auto connect, though that may
>> need a lot more changes than this one so I would be fine applying this
>> so we can think about the implications, also perhaps this should be
>> incorporated to btd_device_set_temporary since a temporary device
>> should probably not be left in to auto-connect and only a user action
>> to attempt to use it again to restore its auto connect status.
>>
>> To summarize we should answer 2 questions:
>>
>> 1. Should an untrusted device be allowed to be marked as auto-connect?
>> Maybe, though we didn't consider trusted property for auto-connect,
>> but I think using it might make auto-connect more predictable to the
>> upper layer.
>> 2. Should a temporary device be allowed to be marked as auto-connect?
>> Most likely not, it should be removed if the user doesn't take any
>> action during the temporary grace period.
>>
>> > device_bonding_failed(device, status);
>> > return;
>> > --
>> > 2.28.0.681.g6f77f65b4e-goog
>> >
>>
>>
>> --
>> Luiz Augusto von Dentz
--
Luiz Augusto von Dentz