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>
In-Reply-To: <20200924140527.Bluez.v1.1.Iedecbb8c8ebb111b14206dddc5bea3c40dfa1771@changeid>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 24 Sep 2020 14:40:43 -0700
Message-ID: <CABBYNZKXSKL3jGByvAdbb77UQgrdtejQR92M00d=1d7simCBLA@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: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