| Message ID | 20200924140527.Bluez.v1.1.Iedecbb8c8ebb111b14206dddc5bea3c40dfa1771@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [Bluez,v1] device: Disable auto connect when pairing failed | expand |
Hi Yu Liu, On Thu, Sep 24, 2020 at 2:08 PM Yu Liu <yudiliu@google.com> 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 >
Hi Yu Liu, On Thu, Sep 24, 2020 at 2:50 PM Yu Liu <yudiliu@google.com> 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@gmail.com> wrote: >> >> Hi Yu Liu, >> >> On Thu, Sep 24, 2020 at 2:08 PM Yu Liu <yudiliu@google.com> 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
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;