Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200310173649.32722-1-luiz.dentz@gmail.com>
In-Reply-To: <20200310173649.32722-1-luiz.dentz@gmail.com>
From:   Alain Michaud <alainmichaud@google.com>
Date:   Tue, 10 Mar 2020 14:04:31 -0400
Message-ID: <CALWDO_U+J1mCf1TzeWM=tSvy-NCsxnyoMeHppFsLKkaaYTiwug@mail.gmail.com>
Subject: Re: [PATCH BlueZ] input: hog: Attempt to set security level if not bonded
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     BlueZ <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Luiz,

On Tue, Mar 10, 2020 at 1:36 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This attempts to set the security if the device is not bonded, the
> kernel will block any communication on the ATT socket while bumping
> the security and if that fails the device will be disconnected which
> is better than having the device dangling around without being able to
> communicate with it until it is properly bonded.
> ---
>  profiles/input/hog.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/profiles/input/hog.c b/profiles/input/hog.c
> index dfac68921..f0226ebbd 100644
> --- a/profiles/input/hog.c
> +++ b/profiles/input/hog.c
> @@ -49,6 +49,8 @@
>  #include "src/shared/util.h"
>  #include "src/shared/uhid.h"
>  #include "src/shared/queue.h"
> +#include "src/shared/att.h"
> +#include "src/shared/gatt-client.h"
>  #include "src/plugin.h"
>
>  #include "suspend.h"
> @@ -187,8 +189,15 @@ static int hog_accept(struct btd_service *service)
>         }
>
>         /* HOGP 1.0 Section 6.1 requires bonding */
> -       if (!device_is_bonded(device, btd_device_get_bdaddr_type(device)))
> -               return -ECONNREFUSED;
> +       if (!device_is_bonded(device, btd_device_get_bdaddr_type(device))) {
> +               struct bt_gatt_client *client;
> +
> +               client = btd_device_get_gatt_client(device);
> +               if (!bt_gatt_client_set_security(client,
> +                                               BT_ATT_SECURITY_MEDIUM)) {
> +                       return -ECONNREFUSED;
> +               }
> +       }
I wonder if this is really necessary.  For example, this may cause a
device the user has not deliberately bonded to suddenly expose a HOG
Service which will trigger the user to pair (most users are known to
blindly accept the pairing).  I believe the previous posture is more
secure by having the user deliberately pair HID devices as opposed to
on demand.

>
>         /* TODO: Replace GAttrib with bt_gatt_client */
>         bt_hog_attach(dev->hog, attrib);
> --
> 2.21.1
>