Resending in plain text :)
On Tue, Mar 10, 2020 at 5:07 PM Alain Michaud <alainmichaud@xxxxxxxxxx> wrote:
>
> Hi Luiz
>
> On Tue, Mar 10, 2020 at 4:39 PM Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx> wrote:
>>
>> Hi Alain,
>>
>> On Tue, Mar 10, 2020 at 11:53 AM Alain Michaud <alainmichaud@xxxxxxxxxx> wrote:
>> >
>> > Hi Luiz,
>> >
>> > On Tue, Mar 10, 2020 at 2:46 PM Luiz Augusto von Dentz
>> > <luiz.dentz@xxxxxxxxx> wrote:
>> > >
>> > > Hi Alain,
>> > >
>> > > On Tue, Mar 10, 2020 at 11:37 AM Alain Michaud <alainmichaud@xxxxxxxxxx> wrote:
>> > > >
>> > > > Hi Luiz,
>> > > >
>> > > > On Tue, Mar 10, 2020 at 2:27 PM Luiz Augusto von Dentz
>> > > > <luiz.dentz@xxxxxxxxx> wrote:
>> > > > >
>> > > > > Hi Alain,
>> > > > >
>> > > > > On Tue, Mar 10, 2020 at 11:04 AM Alain Michaud <alainmichaud@xxxxxxxxxx> wrote:
>> > > > > >
>> > > > > > Hi Luiz,
>> > > > > >
>> > > > > > On Tue, Mar 10, 2020 at 1:36 PM Luiz Augusto von Dentz
>> > > > > > <luiz.dentz@xxxxxxxxx> wrote:
>> > > > > > >
>> > > > > > > From: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
>> > > > > > >
>> > > > > > > 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
