Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200924140527.Bluez.v1.1.Iedecbb8c8ebb111b14206dddc5bea3c40dfa1771@changeid>
 <CABBYNZKXSKL3jGByvAdbb77UQgrdtejQR92M00d=1d7simCBLA@mail.gmail.com> <CAHC-ybzWgkCjgmgQjxOAPFAvr_LX=YP8Fi4WJ7nvGDu1Tk5ZLQ@mail.gmail.com>
In-Reply-To: <CAHC-ybzWgkCjgmgQjxOAPFAvr_LX=YP8Fi4WJ7nvGDu1Tk5ZLQ@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 24 Sep 2020 15:12:00 -0700
Message-ID: <CABBYNZL6Q4w_UuAVXgb6RxN4eY7OqA1m64yab4JWc7Vkdfp7Dw@mail.gmail.com>
Subject: Re: [Bluez PATCH v1] device: Disable auto connect when pairing failed
To:     Yu Liu <yudiliu@google.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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


-- 
Luiz Augusto von Dentz