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> <CALWDO_U+J1mCf1TzeWM=tSvy-NCsxnyoMeHppFsLKkaaYTiwug@mail.gmail.com>
In-Reply-To: <CALWDO_U+J1mCf1TzeWM=tSvy-NCsxnyoMeHppFsLKkaaYTiwug@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 10 Mar 2020 11:26:48 -0700
Message-ID: <CABBYNZ+6NDnuErRSiUkVacOViCBz1Bieda_KYD_0c3VrveER-A@mail.gmail.com>
Subject: Re: [PATCH BlueZ] input: hog: Attempt to set security level if not bonded
To:     Alain Michaud <alainmichaud@google.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 Alain,

On Tue, Mar 10, 2020 at 11:04 AM Alain Michaud <alainmichaud@google.com> wrote:
>
> 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.

There are dedicated APIs to connect specific profiles, so if
hog_accept is reached it means the user/application does want to
connect HoG and in that case it should trigger bonding, so this only
automate the process, like Ive commented for legacy HID we already
attempt to bump the security in a similar way. Having the user
deliberately pair may cause breakage since in most cases the GATT
services do that on demand, in fact HoG is possibly the only exception
to that since it appear to mandate encryption at connection level
rather than on attribute level, so if the user had a peripheral that
used to not require bonding it will suddenly stop working but if we do
have this change it would possible still work after the pairing
procedure is complete.

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


-- 
Luiz Augusto von Dentz