Hi Yu Liu,
On Thu, Sep 24, 2020 at 2:50 PM Yu Liu <yudiliu@xxxxxxxxxx> 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 <luiz.dentz@xxxxxxxxx> wrote:
>>
>> Hi Yu Liu,
>>
>> On Thu, Sep 24, 2020 at 2:08 PM Yu Liu <yudiliu@xxxxxxxxxx> 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
