diff mbox series

[BlueZ] input: hog: Attempt to set security level if not bonded

Message ID 20200310173649.32722-1-luiz.dentz@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Luiz Von Dentz
Headers show
Series [BlueZ] input: hog: Attempt to set security level if not bonded | expand

Commit Message

Luiz Augusto von Dentz March 10, 2020, 5:36 p.m. UTC
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(-)

Comments

Alain Michaud March 10, 2020, 6:04 p.m. UTC | #1
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
>
Luiz Augusto von Dentz March 10, 2020, 6:26 p.m. UTC | #2
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
> >
Alain Michaud March 10, 2020, 6:37 p.m. UTC | #3
Hi Luiz,

On Tue, Mar 10, 2020 at 2:27 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> 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.
The outgoing contract where the user somehow asked for the profile to
be connected and would result in pairing, I'm ok with.  However, this
being in the accept path, it doesn't seem to always be client side
initiated, so that still seems like a concern to me.
>
> >
> > >
> > >         /* TODO: Replace GAttrib with bt_gatt_client */
> > >         bt_hog_attach(dev->hog, attrib);
> > > --
> > > 2.21.1
> > >
>
>
>
> --
> Luiz Augusto von Dentz
Luiz Augusto von Dentz March 10, 2020, 6:46 p.m. UTC | #4
Hi Alain,

On Tue, Mar 10, 2020 at 11:37 AM Alain Michaud <alainmichaud@google.com> wrote:
>
> Hi Luiz,
>
> On Tue, Mar 10, 2020 at 2:27 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > 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.
> The outgoing contract where the user somehow asked for the profile to
> be connected and would result in pairing, I'm ok with.  However, this
> being in the accept path, it doesn't seem to always be client side
> initiated, so that still seems like a concern to me.

Since this is a profile so we are always acting as GATT client here,
so it is either initiated by the client when setting up a new
peripheral or it has been previously setup with Add Device and is
marked to auto connect, the later is exactly the problem I described
that there could be existing peripheral not requiring bonding that
suddenly stop working.

> >
> > >
> > > >
> > > >         /* TODO: Replace GAttrib with bt_gatt_client */
> > > >         bt_hog_attach(dev->hog, attrib);
> > > > --
> > > > 2.21.1
> > > >
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
Alain Michaud March 10, 2020, 6:53 p.m. UTC | #5
Hi Luiz,

On Tue, Mar 10, 2020 at 2:46 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Alain,
>
> On Tue, Mar 10, 2020 at 11:37 AM Alain Michaud <alainmichaud@google.com> wrote:
> >
> > Hi Luiz,
> >
> > On Tue, Mar 10, 2020 at 2:27 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > 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.
> > The outgoing contract where the user somehow asked for the profile to
> > be connected and would result in pairing, I'm ok with.  However, this
> > being in the accept path, it doesn't seem to always be client side
> > initiated, so that still seems like a concern to me.
>
> Since this is a profile so we are always acting as GATT client here,
> so it is either initiated by the client when setting up a new
> peripheral or it has been previously setup with Add Device and is
> marked to auto connect, the later is exactly the problem I described
> that there could be existing peripheral not requiring bonding that
> suddenly stop working.
My understanding is that the HOG service can get added to any other
device through a service change notification or other means, so I
don't think it is a safe assumption that this code will only execute
if a user explicitly requested it.

You are correct that the change may cause a device to stop working if
it was using HOGP without bonding, but this would also be a non
compliant device and one that compromises the system's security.  I'm
ok if we make this a configuration in case you believe the
compatibility with these sorts of scenarios must be maintained.

>
> > >
> > > >
> > > > >
> > > > >         /* TODO: Replace GAttrib with bt_gatt_client */
> > > > >         bt_hog_attach(dev->hog, attrib);
> > > > > --
> > > > > 2.21.1
> > > > >
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz
Luiz Augusto von Dentz March 10, 2020, 8:39 p.m. UTC | #6
Hi Alain,

On Tue, Mar 10, 2020 at 11:53 AM Alain Michaud <alainmichaud@google.com> wrote:
>
> Hi Luiz,
>
> On Tue, Mar 10, 2020 at 2:46 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Alain,
> >
> > On Tue, Mar 10, 2020 at 11:37 AM Alain Michaud <alainmichaud@google.com> wrote:
> > >
> > > Hi Luiz,
> > >
> > > On Tue, Mar 10, 2020 at 2:27 PM Luiz Augusto von Dentz
> > > <luiz.dentz@gmail.com> wrote:
> > > >
> > > > 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.
> > > The outgoing contract where the user somehow asked for the profile to
> > > be connected and would result in pairing, I'm ok with.  However, this
> > > being in the accept path, it doesn't seem to always be client side
> > > initiated, so that still seems like a concern to me.
> >
> > Since this is a profile so we are always acting as GATT client here,
> > so it is either initiated by the client when setting up a new
> > peripheral or it has been previously setup with Add Device and is
> > marked to auto connect, the later is exactly the problem I described
> > that there could be existing peripheral not requiring bonding that
> > suddenly stop working.
> My understanding is that the HOG service can get added to any other
> device through a service change notification or other means, so I
> don't think it is a safe assumption that this code will only execute
> if a user explicitly requested it.

I would assume the users would expect that this would trigger pairing
procedure since silently ignoring the change would make this go
completely unnoticed.

> You are correct that the change may cause a device to stop working if
> it was using HOGP without bonding, but this would also be a non
> compliant device and one that compromises the system's security.  I'm
> ok if we make this a configuration in case you believe the
> compatibility with these sorts of scenarios must be maintained.

This gets a bit tricky since the HOGP mandates security but HIDS does not:

Security Permissions of “None” means that this service does not impose
any requirements.

So my understanding is that a peripheral implementing HIDS does _not_
require bonding and to make matters more confusing none of the
attributes requires security etiher which is perhaps the very reason
HOGP mandates bonding, also afaik peripherals are not mandate to
initiate pairing procedures so it looks like peripherals can in fact
not require bonding and still be compliant.

> >
> > > >
> > > > >
> > > > > >
> > > > > >         /* TODO: Replace GAttrib with bt_gatt_client */
> > > > > >         bt_hog_attach(dev->hog, attrib);
> > > > > > --
> > > > > > 2.21.1
> > > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
Alain Michaud March 10, 2020, 9:09 p.m. UTC | #7
Resending in plain text :)


On Tue, Mar 10, 2020 at 5:07 PM Alain Michaud <alainmichaud@google.com> wrote:
>
> Hi Luiz
>
> On Tue, Mar 10, 2020 at 4:39 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>
>> Hi Alain,
>>
>> On Tue, Mar 10, 2020 at 11:53 AM Alain Michaud <alainmichaud@google.com> wrote:
>> >
>> > Hi Luiz,
>> >
>> > On Tue, Mar 10, 2020 at 2:46 PM Luiz Augusto von Dentz
>> > <luiz.dentz@gmail.com> wrote:
>> > >
>> > > Hi Alain,
>> > >
>> > > On Tue, Mar 10, 2020 at 11:37 AM Alain Michaud <alainmichaud@google.com> wrote:
>> > > >
>> > > > Hi Luiz,
>> > > >
>> > > > On Tue, Mar 10, 2020 at 2:27 PM Luiz Augusto von Dentz
>> > > > <luiz.dentz@gmail.com> wrote:
>> > > > >
>> > > > > 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.
>> > > > The outgoing contract where the user somehow asked for the profile to
>> > > > be connected and would result in pairing, I'm ok with.  However, this
>> > > > being in the accept path, it doesn't seem to always be client side
>> > > > initiated, so that still seems like a concern to me.
>> > >
>> > > Since this is a profile so we are always acting as GATT client here,
>> > > so it is either initiated by the client when setting up a new
>> > > peripheral or it has been previously setup with Add Device and is
>> > > marked to auto connect, the later is exactly the problem I described
>> > > that there could be existing peripheral not requiring bonding that
>> > > suddenly stop working.
>> > My understanding is that the HOG service can get added to any other
>> > device through a service change notification or other means, so I
>> > don't think it is a safe assumption that this code will only execute
>> > if a user explicitly requested it.
>>
>> I would assume the users would expect that this would trigger pairing
>> procedure since silently ignoring the change would make this go
>> completely unnoticed.
>
> In this case it may be a device impersonating a device you were just communicating with without bonding, manages to connect and expose an HOG Service and all of the suddent requests a pairing confirmation.  Data suggests users pay little attention to these sorts of notification and tend to blindly accept them leading them to a compromised state.  HOGP is unique in the sense that the consequences are higher since it can lead to executing code in the user's context by injecting keystrokes.
>>
>>
>> > You are correct that the change may cause a device to stop working if
>> > it was using HOGP without bonding, but this would also be a non
>> > compliant device and one that compromises the system's security.  I'm
>> > ok if we make this a configuration in case you believe the
>> > compatibility with these sorts of scenarios must be maintained.
>>
>> This gets a bit tricky since the HOGP mandates security but HIDS does not:
>>
>> Security Permissions of “None” means that this service does not impose
>> any requirements.
>>
>> So my understanding is that a peripheral implementing HIDS does _not_
>> require bonding and to make matters more confusing none of the
>> attributes requires security etiher which is perhaps the very reason
>> HOGP mandates bonding, also afaik peripherals are not mandate to
>> initiate pairing procedures so it looks like peripherals can in fact
>> not require bonding and still be compliant.
>
>
> The peripheral yes, but HOGP would not so I'd assert if the device is required to work without bonding, it likely didn't pass the profile qualification.
>
> As stated before,  it seems acceptable to me if BlueZ would want a more compatible posture here, I would however like to request that a configuration be available so that HOGP can simply reject as the original patch did.
>
>>
>>
>> > >
>> > > > >
>> > > > > >
>> > > > > > >
>> > > > > > >         /* TODO: Replace GAttrib with bt_gatt_client */
>> > > > > > >         bt_hog_attach(dev->hog, attrib);
>> > > > > > > --
>> > > > > > > 2.21.1
>> > > > > > >
>> > > > >
>> > > > >
>> > > > >
>> > > > > --
>> > > > > Luiz Augusto von Dentz
>> > >
>> > >
>> > >
>> > > --
>> > > Luiz Augusto von Dentz
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
Luiz Augusto von Dentz March 10, 2020, 10:29 p.m. UTC | #8
Hi Alain,

On Tue, Mar 10, 2020 at 2:07 PM Alain Michaud <alainmichaud@google.com> wrote:
>
> Hi Luiz
>
> On Tue, Mar 10, 2020 at 4:39 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>
>> Hi Alain,
>>
>> On Tue, Mar 10, 2020 at 11:53 AM Alain Michaud <alainmichaud@google.com> wrote:
>> >
>> > Hi Luiz,
>> >
>> > On Tue, Mar 10, 2020 at 2:46 PM Luiz Augusto von Dentz
>> > <luiz.dentz@gmail.com> wrote:
>> > >
>> > > Hi Alain,
>> > >
>> > > On Tue, Mar 10, 2020 at 11:37 AM Alain Michaud <alainmichaud@google.com> wrote:
>> > > >
>> > > > Hi Luiz,
>> > > >
>> > > > On Tue, Mar 10, 2020 at 2:27 PM Luiz Augusto von Dentz
>> > > > <luiz.dentz@gmail.com> wrote:
>> > > > >
>> > > > > 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.
>> > > > The outgoing contract where the user somehow asked for the profile to
>> > > > be connected and would result in pairing, I'm ok with.  However, this
>> > > > being in the accept path, it doesn't seem to always be client side
>> > > > initiated, so that still seems like a concern to me.
>> > >
>> > > Since this is a profile so we are always acting as GATT client here,
>> > > so it is either initiated by the client when setting up a new
>> > > peripheral or it has been previously setup with Add Device and is
>> > > marked to auto connect, the later is exactly the problem I described
>> > > that there could be existing peripheral not requiring bonding that
>> > > suddenly stop working.
>> > My understanding is that the HOG service can get added to any other
>> > device through a service change notification or other means, so I
>> > don't think it is a safe assumption that this code will only execute
>> > if a user explicitly requested it.
>>
>> I would assume the users would expect that this would trigger pairing
>> procedure since silently ignoring the change would make this go
>> completely unnoticed.
>
> In this case it may be a device impersonating a device you were just communicating with without bonding, manages to connect and expose an HOG Service and all of the suddent requests a pairing confirmation.  Data suggests users pay little attention to these sorts of notification and tend to blindly accept them leading them to a compromised state.  HOGP is unique in the sense that the consequences are higher since it can lead to executing code in the user's context by injecting keystrokes.

Sure but this is true regardless of doing pairing automatically or
requiring the user to pair it manually, or you are suggesting this is
safer because it would have the go over the setting to start the
pairing? Im afraid not all user interface would react the same in this
regard,  or at all,also at this state even if the user pair the device
it would have to reconnect before it start working again since the
driver would not be probed again.

>>
>>
>> > You are correct that the change may cause a device to stop working if
>> > it was using HOGP without bonding, but this would also be a non
>> > compliant device and one that compromises the system's security.  I'm
>> > ok if we make this a configuration in case you believe the
>> > compatibility with these sorts of scenarios must be maintained.
>>
>> This gets a bit tricky since the HOGP mandates security but HIDS does not:
>>
>> Security Permissions of “None” means that this service does not impose
>> any requirements.
>>
>> So my understanding is that a peripheral implementing HIDS does _not_
>> require bonding and to make matters more confusing none of the
>> attributes requires security etiher which is perhaps the very reason
>> HOGP mandates bonding, also afaik peripherals are not mandate to
>> initiate pairing procedures so it looks like peripherals can in fact
>> not require bonding and still be compliant.
>
>
> The peripheral yes, but HOGP would not so I'd assert if the device is required to work without bonding, it likely didn't pass the profile qualification.

It would have pass it alright, its the PTS side that would exercise
this requirement of HOGP not the peripheral, the peripheral just have
to respond to the pairing procedure but it may never inititate it by
itself. Like I said none of HIDS attributes require any security and
the TS don't seem to even test authentication errors as it only
mentions something like:

If the IUT requires a bonding procedure then perform a bonding procedure.

Take for example zephyr hids example:

https://github.com/zephyrproject-rtos/zephyr/blob/master/samples/bluetooth/peripheral_hids/src/hog.c

It never checks for bonding nor it requires any security, so if I attempt:

#bluetoothctl> connect

HoG no longer connects.

> As stated before,  it seems acceptable to me if BlueZ would want a more compatible posture here, I would however like to request that a configuration be available so that HOGP can simply reject as the original patch did.

Well in that case we would have to implement reprobing logic so that
if the device gets paired hog_accept should be called once again, imo
triggering bonding seems a better alternative in the sort term until
we verify all use cases are attended.

>>
>>
>> > >
>> > > > >
>> > > > > >
>> > > > > > >
>> > > > > > >         /* TODO: Replace GAttrib with bt_gatt_client */
>> > > > > > >         bt_hog_attach(dev->hog, attrib);
>> > > > > > > --
>> > > > > > > 2.21.1
>> > > > > > >
>> > > > >
>> > > > >
>> > > > >
>> > > > > --
>> > > > > Luiz Augusto von Dentz
>> > >
>> > >
>> > >
>> > > --
>> > > Luiz Augusto von Dentz
>>
>>
>>
>> --
>> Luiz Augusto von Dentz
Alain Michaud March 10, 2020, 11:35 p.m. UTC | #9
Hi Luiz,


On Tue, Mar 10, 2020 at 6:29 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Alain,
>
> On Tue, Mar 10, 2020 at 2:07 PM Alain Michaud <alainmichaud@google.com> wrote:
> >
> > Hi Luiz
> >
> > On Tue, Mar 10, 2020 at 4:39 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
> >>
> >> Hi Alain,
> >>
> >> On Tue, Mar 10, 2020 at 11:53 AM Alain Michaud <alainmichaud@google.com> wrote:
> >> >
> >> > Hi Luiz,
> >> >
> >> > On Tue, Mar 10, 2020 at 2:46 PM Luiz Augusto von Dentz
> >> > <luiz.dentz@gmail.com> wrote:
> >> > >
> >> > > Hi Alain,
> >> > >
> >> > > On Tue, Mar 10, 2020 at 11:37 AM Alain Michaud <alainmichaud@google.com> wrote:
> >> > > >
> >> > > > Hi Luiz,
> >> > > >
> >> > > > On Tue, Mar 10, 2020 at 2:27 PM Luiz Augusto von Dentz
> >> > > > <luiz.dentz@gmail.com> wrote:
> >> > > > >
> >> > > > > 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.
> >> > > > The outgoing contract where the user somehow asked for the profile to
> >> > > > be connected and would result in pairing, I'm ok with.  However, this
> >> > > > being in the accept path, it doesn't seem to always be client side
> >> > > > initiated, so that still seems like a concern to me.
> >> > >
> >> > > Since this is a profile so we are always acting as GATT client here,
> >> > > so it is either initiated by the client when setting up a new
> >> > > peripheral or it has been previously setup with Add Device and is
> >> > > marked to auto connect, the later is exactly the problem I described
> >> > > that there could be existing peripheral not requiring bonding that
> >> > > suddenly stop working.
> >> > My understanding is that the HOG service can get added to any other
> >> > device through a service change notification or other means, so I
> >> > don't think it is a safe assumption that this code will only execute
> >> > if a user explicitly requested it.
> >>
> >> I would assume the users would expect that this would trigger pairing
> >> procedure since silently ignoring the change would make this go
> >> completely unnoticed.
> >
> > In this case it may be a device impersonating a device you were just communicating with without bonding, manages to connect and expose an HOG Service and all of the suddent requests a pairing confirmation.  Data suggests users pay little attention to these sorts of notification and tend to blindly accept them leading them to a compromised state.  HOGP is unique in the sense that the consequences are higher since it can lead to executing code in the user's context by injecting keystrokes.
>
> Sure but this is true regardless of doing pairing automatically or
> requiring the user to pair it manually, or you are suggesting this is
> safer because it would have the go over the setting to start the
> pairing? Im afraid not all user interface would react the same in this
> regard,  or at all,also at this state even if the user pair the device
> it would have to reconnect before it start working again since the
> driver would not be probed again.
The difference is in the context.  In one case, the user is
deliberately trying to add a device and is cognitively within the
right context to be able to make a deliberate choice to choose a
device from the list and pair against it.  In the other, they are
receiving an unsolicited request to pair which they may or may not
understand sufficiently to make the right choice.

I agree, not all user interfaces (or frankly platforms) have the same
requirements.
>
> >>
> >>
> >> > You are correct that the change may cause a device to stop working if
> >> > it was using HOGP without bonding, but this would also be a non
> >> > compliant device and one that compromises the system's security.  I'm
> >> > ok if we make this a configuration in case you believe the
> >> > compatibility with these sorts of scenarios must be maintained.
> >>
> >> This gets a bit tricky since the HOGP mandates security but HIDS does not:
> >>
> >> Security Permissions of “None” means that this service does not impose
> >> any requirements.
> >>
> >> So my understanding is that a peripheral implementing HIDS does _not_
> >> require bonding and to make matters more confusing none of the
> >> attributes requires security etiher which is perhaps the very reason
> >> HOGP mandates bonding, also afaik peripherals are not mandate to
> >> initiate pairing procedures so it looks like peripherals can in fact
> >> not require bonding and still be compliant.
> >
> >
> > The peripheral yes, but HOGP would not so I'd assert if the device is required to work without bonding, it likely didn't pass the profile qualification.
>
> It would have pass it alright, its the PTS side that would exercise
> this requirement of HOGP not the peripheral, the peripheral just have
> to respond to the pairing procedure but it may never inititate it by
> itself. Like I said none of HIDS attributes require any security and
> the TS don't seem to even test authentication errors as it only
> mentions something like:
>
> If the IUT requires a bonding procedure then perform a bonding procedure.
>
> Take for example zephyr hids example:
>
> https://github.com/zephyrproject-rtos/zephyr/blob/master/samples/bluetooth/peripheral_hids/src/hog.c
>
> It never checks for bonding nor it requires any security, so if I attempt:
>
> #bluetoothctl> connect
>
> HoG no longer connects.
>
> > As stated before,  it seems acceptable to me if BlueZ would want a more compatible posture here, I would however like to request that a configuration be available so that HOGP can simply reject as the original patch did.
>
> Well in that case we would have to implement reprobing logic so that
> if the device gets paired hog_accept should be called once again, imo
> triggering bonding seems a better alternative in the sort term until
> we verify all use cases are attended.
I don't think re_probing is necessary in this case since we wouldn't
expect it to be "upgraded" to a bonded state.  For the golden path we
would expect to start bonded.

Again, I respect the different requirements where some platforms may
want to allow this upgrading, what I'm articulating is that some
platforms may also want to take a more secure posture and not allow
this upgrade to take place automatically and instead require that the
user deliberately initiate pairing with HID devices.  The later, more
secure posture, would be my preference for chromium platforms hence
the request to make this conditional to a configuration that can be
applied on some platforms.

>
> >>
> >>
> >> > >
> >> > > > >
> >> > > > > >
> >> > > > > > >
> >> > > > > > >         /* TODO: Replace GAttrib with bt_gatt_client */
> >> > > > > > >         bt_hog_attach(dev->hog, attrib);
> >> > > > > > > --
> >> > > > > > > 2.21.1
> >> > > > > > >
> >> > > > >
> >> > > > >
> >> > > > >
> >> > > > > --
> >> > > > > Luiz Augusto von Dentz
> >> > >
> >> > >
> >> > >
> >> > > --
> >> > > Luiz Augusto von Dentz
> >>
> >>
> >>
> >> --
> >> Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz
diff mbox series

Patch

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;
+		}
+	}
 
 	/* TODO: Replace GAttrib with bt_gatt_client */
 	bt_hog_attach(dev->hog, attrib);