[BlueZ PATCH 0/2] HID and HOGP connections from non-bonded devices.

[BlueZ PATCH 0/2] HID and HOGP connections from non-bonded devices.

[Alain Michaud]

It was discovered that BlueZ's HID and HOGP profiles implementations
don't specifically require bonding between the device and the host.

This creates an opportunity for an malicious device to connect to a
target host to either impersonate an existing HID device without
security or to cause an SDP or GATT service discovery to take place
which would allow HID reports to be injected to the input subsystem from
a non-bonded source.

This patch series addresses the issue by ensuring that only connections
from devices that are bonded are accepted by the HID and HOGP profile
implementation.

More information about the vulnerability is available here:
https://www.intel.com/content/www/us/en/security-center/advisory/intel-sa-00352.html

Alain Michaud (2):
  HOGP must only accept data from bonded devices.
  HID accepts bonded device connections only.

 profiles/input/device.c   | 23 ++++++++++++++++++++++-
 profiles/input/device.h   |  1 +
 profiles/input/hog.c      |  4 ++++
 profiles/input/input.conf |  8 ++++++++
 profiles/input/manager.c  | 13 ++++++++++++-
 5 files changed, 47 insertions(+), 2 deletions(-)

-- 
2.25.1.481.gfbce0eb801-goog

[BlueZ PATCH 1/2] HOGP must only accept data from bonded devices.

[Alain Michaud]

HOGP 1.0 Section 6.1 establishes that the HOGP must require bonding.

Reference:
https://www.intel.com/content/www/us/en/security-center/advisory/intel-sa-00352.htm
---

 profiles/input/hog.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/profiles/input/hog.c b/profiles/input/hog.c
index 83c017dcb..dfac68921 100644
--- a/profiles/input/hog.c
+++ b/profiles/input/hog.c
@@ -186,6 +186,10 @@ static int hog_accept(struct btd_service *service)
 			return -EINVAL;
 	}
 
+	/* HOGP 1.0 Section 6.1 requires bonding */
+	if (!device_is_bonded(device, btd_device_get_bdaddr_type(device)))
+		return -ECONNREFUSED;
+
 	/* TODO: Replace GAttrib with bt_gatt_client */
 	bt_hog_attach(dev->hog, attrib);
 
-- 
2.25.1.481.gfbce0eb801-goog

Re: [BlueZ PATCH 1/2] HOGP must only accept data from bonded devices.

[Luiz Augusto von Dentz]

Hi Alain,

On Mon, Mar 9, 2020 at 7:37 PM Alain Michaud <alainm@chromium.org> wrote:
>
> HOGP 1.0 Section 6.1 establishes that the HOGP must require bonding.
>
> Reference:
> https://www.intel.com/content/www/us/en/security-center/advisory/intel-sa-00352.htm
> ---
>
>  profiles/input/hog.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/profiles/input/hog.c b/profiles/input/hog.c
> index 83c017dcb..dfac68921 100644
> --- a/profiles/input/hog.c
> +++ b/profiles/input/hog.c
> @@ -186,6 +186,10 @@ static int hog_accept(struct btd_service *service)
>                         return -EINVAL;
>         }
>
> +       /* HOGP 1.0 Section 6.1 requires bonding */
> +       if (!device_is_bonded(device, btd_device_get_bdaddr_type(device)))
> +               return -ECONNREFUSED;

Perhaps attempting to elevate the security level would be better than
just refuse to attach the instance since otherwise we may end up with
connecting services like battery, etc, leaving the device half
working.

>         /* TODO: Replace GAttrib with bt_gatt_client */
>         bt_hog_attach(dev->hog, attrib);
>
> --
> 2.25.1.481.gfbce0eb801-goog
>


-- 
Luiz Augusto von Dentz

[BlueZ PATCH 2/2] HID accepts bonded device connections only.

[Alain Michaud]

This change adds a configuration for platforms to choose a more secure
posture for the HID profile.  While some older mice are known to not
support pairing or encryption, some platform may choose a more secure
posture by requiring the device to be bonded  and require the
connection to be encrypted when bonding is required.

Reference:
https://www.intel.com/content/www/us/en/security-center/advisory/intel-sa-00352.html
---

 profiles/input/device.c   | 23 ++++++++++++++++++++++-
 profiles/input/device.h   |  1 +
 profiles/input/input.conf |  8 ++++++++
 profiles/input/manager.c  | 13 ++++++++++++-
 4 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/profiles/input/device.c b/profiles/input/device.c
index 2cb3811c8..7fb22b18f 100644
--- a/profiles/input/device.c
+++ b/profiles/input/device.c
@@ -92,6 +92,7 @@ struct input_device {
 
 static int idle_timeout = 0;
 static bool uhid_enabled = false;
+static bool br_bonded_only = false;
 
 void input_set_idle_timeout(int timeout)
 {
@@ -103,6 +104,11 @@ void input_enable_userspace_hid(bool state)
 	uhid_enabled = state;
 }
 
+void input_set_br_bonded_only(bool state)
+{
+	br_bonded_only = state;
+}
+
 static void input_device_enter_reconnect_mode(struct input_device *idev);
 static int connection_disconnect(struct input_device *idev, uint32_t flags);
 
@@ -970,8 +976,18 @@ static int hidp_add_connection(struct input_device *idev)
 	if (device_name_known(idev->device))
 		device_get_name(idev->device, req->name, sizeof(req->name));
 
+	/* Make sure the device is bonded if required */
+	if (br_bonded_only && !device_is_bonded(idev->device,
+				btd_device_get_bdaddr_type(idev->device))) {
+		error("Rejected connection from !bonded device %s", dst_addr);
+		goto cleanup;
+	}
+
 	/* Encryption is mandatory for keyboards */
-	if (req->subclass & 0x40) {
+	/* Some platforms may choose to require encryption for all devices */
+	/* Note that this only matters for pre 2.1 devices as otherwise the */
+	/* device is encrypted by default by the lower layers */
+	if (br_bonded_only || req->subclass & 0x40) {
 		if (!bt_io_set(idev->intr_io, &gerr,
 					BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_MEDIUM,
 					BT_IO_OPT_INVALID)) {
@@ -1203,6 +1219,11 @@ static void input_device_enter_reconnect_mode(struct input_device *idev)
 	DBG("path=%s reconnect_mode=%s", idev->path,
 				reconnect_mode_to_string(idev->reconnect_mode));
 
+	/* Make sure the device is bonded if required */
+	if (br_bonded_only && !device_is_bonded(idev->device,
+				btd_device_get_bdaddr_type(idev->device)))
+		return;
+
 	/* Only attempt an auto-reconnect when the device is required to
 	 * accept reconnections from the host.
 	 */
diff --git a/profiles/input/device.h b/profiles/input/device.h
index 51a9aee18..aaf312f0e 100644
--- a/profiles/input/device.h
+++ b/profiles/input/device.h
@@ -29,6 +29,7 @@ struct input_conn;
 
 void input_set_idle_timeout(int timeout);
 void input_enable_userspace_hid(bool state);
+void input_set_br_bonded_only(bool state);
 
 int input_device_register(struct btd_service *service);
 void input_device_unregister(struct btd_service *service);
diff --git a/profiles/input/input.conf b/profiles/input/input.conf
index 3e1d65aae..58791b7e6 100644
--- a/profiles/input/input.conf
+++ b/profiles/input/input.conf
@@ -11,3 +11,11 @@
 # Enable HID protocol handling in userspace input profile
 # Defaults to false (HIDP handled in HIDP kernel module)
 #UserspaceHID=true
+
+# Limit HID connections to bonded devices
+# The HID Profile does not specify that devices must be bonded, however some
+# platforms may want to make sure that input connections only come from bonded
+# device connections. Several older mice have been known for not supporting
+# pairing/encryption.
+# Defaults to false to maximize device compatibility.
+#BrBondedOnly=true
diff --git a/profiles/input/manager.c b/profiles/input/manager.c
index 1d31b0652..ec45e1649 100644
--- a/profiles/input/manager.c
+++ b/profiles/input/manager.c
@@ -96,7 +96,7 @@ static int input_init(void)
 	config = load_config_file(CONFIGDIR "/input.conf");
 	if (config) {
 		int idle_timeout;
-		gboolean uhid_enabled;
+		gboolean uhid_enabled, br_bonded_only;
 
 		idle_timeout = g_key_file_get_integer(config, "General",
 							"IdleTimeout", &err);
@@ -114,6 +114,17 @@ static int input_init(void)
 			input_enable_userspace_hid(uhid_enabled);
 		} else
 			g_clear_error(&err);
+
+		br_bonded_only = g_key_file_get_boolean(config, "General",
+							"BrBondedOnly", &err);
+
+		if (!err) {
+			DBG("input.conf: BrBondedOnly=%s", br_bonded_only ?
+							"true" : "false");
+			input_set_br_bonded_only(br_bonded_only);
+		} else
+			g_clear_error(&err);
+
 	}
 
 	btd_profile_register(&input_profile);
-- 
2.25.1.481.gfbce0eb801-goog

Re: [BlueZ PATCH 2/2] HID accepts bonded device connections only.

[Luiz Augusto von Dentz]

Hi Alain,

On Mon, Mar 9, 2020 at 7:39 PM Alain Michaud <alainm@chromium.org> wrote:
>
> This change adds a configuration for platforms to choose a more secure
> posture for the HID profile.  While some older mice are known to not
> support pairing or encryption, some platform may choose a more secure
> posture by requiring the device to be bonded  and require the
> connection to be encrypted when bonding is required.
>
> Reference:
> https://www.intel.com/content/www/us/en/security-center/advisory/intel-sa-00352.html

Weird I cannot access this link.

> ---
>
>  profiles/input/device.c   | 23 ++++++++++++++++++++++-
>  profiles/input/device.h   |  1 +
>  profiles/input/input.conf |  8 ++++++++
>  profiles/input/manager.c  | 13 ++++++++++++-
>  4 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/profiles/input/device.c b/profiles/input/device.c
> index 2cb3811c8..7fb22b18f 100644
> --- a/profiles/input/device.c
> +++ b/profiles/input/device.c
> @@ -92,6 +92,7 @@ struct input_device {
>
>  static int idle_timeout = 0;
>  static bool uhid_enabled = false;
> +static bool br_bonded_only = false;
>
>  void input_set_idle_timeout(int timeout)
>  {
> @@ -103,6 +104,11 @@ void input_enable_userspace_hid(bool state)
>         uhid_enabled = state;
>  }
>
> +void input_set_br_bonded_only(bool state)
> +{
> +       br_bonded_only = state;
> +}
> +
>  static void input_device_enter_reconnect_mode(struct input_device *idev);
>  static int connection_disconnect(struct input_device *idev, uint32_t flags);
>
> @@ -970,8 +976,18 @@ static int hidp_add_connection(struct input_device *idev)
>         if (device_name_known(idev->device))
>                 device_get_name(idev->device, req->name, sizeof(req->name));
>
> +       /* Make sure the device is bonded if required */
> +       if (br_bonded_only && !device_is_bonded(idev->device,
> +                               btd_device_get_bdaddr_type(idev->device))) {
> +               error("Rejected connection from !bonded device %s", dst_addr);
> +               goto cleanup;
> +       }
> +
>         /* Encryption is mandatory for keyboards */
> -       if (req->subclass & 0x40) {
> +       /* Some platforms may choose to require encryption for all devices */
> +       /* Note that this only matters for pre 2.1 devices as otherwise the */
> +       /* device is encrypted by default by the lower layers */

You should use multiline comments above.

> +       if (br_bonded_only || req->subclass & 0x40) {
>                 if (!bt_io_set(idev->intr_io, &gerr,
>                                         BT_IO_OPT_SEC_LEVEL, BT_IO_SEC_MEDIUM,
>                                         BT_IO_OPT_INVALID)) {

This one seems to be doing what I suggested for HoG, it attempt to
bump the security so we might as well do the same for HoG.

> @@ -1203,6 +1219,11 @@ static void input_device_enter_reconnect_mode(struct input_device *idev)
>         DBG("path=%s reconnect_mode=%s", idev->path,
>                                 reconnect_mode_to_string(idev->reconnect_mode));
>
> +       /* Make sure the device is bonded if required */
> +       if (br_bonded_only && !device_is_bonded(idev->device,
> +                               btd_device_get_bdaddr_type(idev->device)))
> +               return;
> +
>         /* Only attempt an auto-reconnect when the device is required to
>          * accept reconnections from the host.
>          */
> diff --git a/profiles/input/device.h b/profiles/input/device.h
> index 51a9aee18..aaf312f0e 100644
> --- a/profiles/input/device.h
> +++ b/profiles/input/device.h
> @@ -29,6 +29,7 @@ struct input_conn;
>
>  void input_set_idle_timeout(int timeout);
>  void input_enable_userspace_hid(bool state);
> +void input_set_br_bonded_only(bool state);
>
>  int input_device_register(struct btd_service *service);
>  void input_device_unregister(struct btd_service *service);
> diff --git a/profiles/input/input.conf b/profiles/input/input.conf
> index 3e1d65aae..58791b7e6 100644
> --- a/profiles/input/input.conf
> +++ b/profiles/input/input.conf
> @@ -11,3 +11,11 @@
>  # Enable HID protocol handling in userspace input profile
>  # Defaults to false (HIDP handled in HIDP kernel module)
>  #UserspaceHID=true
> +
> +# Limit HID connections to bonded devices
> +# The HID Profile does not specify that devices must be bonded, however some
> +# platforms may want to make sure that input connections only come from bonded
> +# device connections. Several older mice have been known for not supporting
> +# pairing/encryption.
> +# Defaults to false to maximize device compatibility.
> +#BrBondedOnly=true

Id make this more generic e.g. RequireEncryption and use it for both
HoG and legacy HID, that way nobody will be caugh by surprise if we
starting doing this for HoG since there may be devices already
connecting that haven't been previously bonded, if we automatically
trigger bonding that should still work but it kind hard to know in
advance how broken peripherals are in this respect.

> diff --git a/profiles/input/manager.c b/profiles/input/manager.c
> index 1d31b0652..ec45e1649 100644
> --- a/profiles/input/manager.c
> +++ b/profiles/input/manager.c
> @@ -96,7 +96,7 @@ static int input_init(void)
>         config = load_config_file(CONFIGDIR "/input.conf");
>         if (config) {
>                 int idle_timeout;
> -               gboolean uhid_enabled;
> +               gboolean uhid_enabled, br_bonded_only;
>
>                 idle_timeout = g_key_file_get_integer(config, "General",
>                                                         "IdleTimeout", &err);
> @@ -114,6 +114,17 @@ static int input_init(void)
>                         input_enable_userspace_hid(uhid_enabled);
>                 } else
>                         g_clear_error(&err);
> +
> +               br_bonded_only = g_key_file_get_boolean(config, "General",
> +                                                       "BrBondedOnly", &err);
> +
> +               if (!err) {
> +                       DBG("input.conf: BrBondedOnly=%s", br_bonded_only ?
> +                                                       "true" : "false");
> +                       input_set_br_bonded_only(br_bonded_only);
> +               } else
> +                       g_clear_error(&err);
> +
>         }
>
>         btd_profile_register(&input_profile);
> --
> 2.25.1.481.gfbce0eb801-goog
>


-- 
Luiz Augusto von Dentz

Re: [BlueZ PATCH 0/2] HID and HOGP connections from non-bonded devices.

[Marcel Holtmann]

Hi Alain,

> It was discovered that BlueZ's HID and HOGP profiles implementations
> don't specifically require bonding between the device and the host.
> 
> This creates an opportunity for an malicious device to connect to a
> target host to either impersonate an existing HID device without
> security or to cause an SDP or GATT service discovery to take place
> which would allow HID reports to be injected to the input subsystem from
> a non-bonded source.
> 
> This patch series addresses the issue by ensuring that only connections
> from devices that are bonded are accepted by the HID and HOGP profile
> implementation.
> 
> More information about the vulnerability is available here:
> https://www.intel.com/content/www/us/en/security-center/advisory/intel-sa-00352.html
> 
> Alain Michaud (2):
>  HOGP must only accept data from bonded devices.
>  HID accepts bonded device connections only.
> 
> profiles/input/device.c   | 23 ++++++++++++++++++++++-
> profiles/input/device.h   |  1 +
> profiles/input/hog.c      |  4 ++++
> profiles/input/input.conf |  8 ++++++++
> profiles/input/manager.c  | 13 ++++++++++++-
> 5 files changed, 47 insertions(+), 2 deletions(-)

both patches have been applied.

However I changed BrBondedOnly configuration name into ClassicBondedOnly since that name seemed more obvious to me. The prefix Br has never been used and the Bluetooth SIG started calling it Classic a while back.

Regards

Marcel

Re: [BlueZ PATCH 0/2] HID and HOGP connections from non-bonded devices.

[Luiz Augusto von Dentz]

Hi Marcel,

On Mon, Mar 9, 2020 at 10:26 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Alain,
>
> > It was discovered that BlueZ's HID and HOGP profiles implementations
> > don't specifically require bonding between the device and the host.
> >
> > This creates an opportunity for an malicious device to connect to a
> > target host to either impersonate an existing HID device without
> > security or to cause an SDP or GATT service discovery to take place
> > which would allow HID reports to be injected to the input subsystem from
> > a non-bonded source.
> >
> > This patch series addresses the issue by ensuring that only connections
> > from devices that are bonded are accepted by the HID and HOGP profile
> > implementation.
> >
> > More information about the vulnerability is available here:
> > https://www.intel.com/content/www/us/en/security-center/advisory/intel-sa-00352.html
> >
> > Alain Michaud (2):
> >  HOGP must only accept data from bonded devices.
> >  HID accepts bonded device connections only.
> >
> > profiles/input/device.c   | 23 ++++++++++++++++++++++-
> > profiles/input/device.h   |  1 +
> > profiles/input/hog.c      |  4 ++++
> > profiles/input/input.conf |  8 ++++++++
> > profiles/input/manager.c  | 13 ++++++++++++-
> > 5 files changed, 47 insertions(+), 2 deletions(-)
>
> both patches have been applied.
>
> However I changed BrBondedOnly configuration name into ClassicBondedOnly since that name seemed more obvious to me. The prefix Br has never been used and the Bluetooth SIG started calling it Classic a while back.

Looks like you were quicker than me, anyway I do fill like we should
attempt to bump to security instead of just refuse to connection in
case of HoG since we are the central and the peripheral is not
mandated to started it either it may be just the client application is
attempting to connect to trigger pairing on demand, that would usually
kick latter when reading the characteristics but with this changes it
doesn't even get to that point if the devices was not bonded already.

-- 
Luiz Augusto von Dentz

Re: [BlueZ PATCH 0/2] HID and HOGP connections from non-bonded devices.

[Alain Michaud]

Hi Luiz,

On Tue, Mar 10, 2020 at 2:27 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Marcel,
>
> On Mon, Mar 9, 2020 at 10:26 PM Marcel Holtmann <marcel@holtmann.org> wrote:
> >
> > Hi Alain,
> >
> > > It was discovered that BlueZ's HID and HOGP profiles implementations
> > > don't specifically require bonding between the device and the host.
> > >
> > > This creates an opportunity for an malicious device to connect to a
> > > target host to either impersonate an existing HID device without
> > > security or to cause an SDP or GATT service discovery to take place
> > > which would allow HID reports to be injected to the input subsystem from
> > > a non-bonded source.
> > >
> > > This patch series addresses the issue by ensuring that only connections
> > > from devices that are bonded are accepted by the HID and HOGP profile
> > > implementation.
> > >
> > > More information about the vulnerability is available here:
> > > https://www.intel.com/content/www/us/en/security-center/advisory/intel-sa-00352.html
> > >
> > > Alain Michaud (2):
> > >  HOGP must only accept data from bonded devices.
> > >  HID accepts bonded device connections only.
> > >
> > > profiles/input/device.c   | 23 ++++++++++++++++++++++-
> > > profiles/input/device.h   |  1 +
> > > profiles/input/hog.c      |  4 ++++
> > > profiles/input/input.conf |  8 ++++++++
> > > profiles/input/manager.c  | 13 ++++++++++++-
> > > 5 files changed, 47 insertions(+), 2 deletions(-)
> >
> > both patches have been applied.
> >
> > However I changed BrBondedOnly configuration name into ClassicBondedOnly since that name seemed more obvious to me. The prefix Br has never been used and the Bluetooth SIG started calling it Classic a while back.
>
> Looks like you were quicker than me, anyway I do fill like we should
> attempt to bump to security instead of just refuse to connection in
> case of HoG since we are the central and the peripheral is not
> mandated to started it either it may be just the client application is
> attempting to connect to trigger pairing on demand, that would usually
> kick latter when reading the characteristics but with this changes it
> doesn't even get to that point if the devices was not bonded already.
The specification for HoG is that the device is bonded.  If client or
server attempts to access the service before it is bonded, it would
violate the specification.  For this reason, I believe it is both
safer and simpler to just reject any attempts to access the service
without first being bonded.
>
> --
> Luiz Augusto von Dentz

Re: [BlueZ PATCH 0/2] HID and HOGP connections from non-bonded devices.

[Luiz Augusto von Dentz]

Hi Alain,

On Tue, Mar 10, 2020 at 5:30 AM Alain Michaud <alainmichaud@google.com> wrote:
>
> Hi Luiz,
>
> On Tue, Mar 10, 2020 at 2:27 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Marcel,
> >
> > On Mon, Mar 9, 2020 at 10:26 PM Marcel Holtmann <marcel@holtmann.org> wrote:
> > >
> > > Hi Alain,
> > >
> > > > It was discovered that BlueZ's HID and HOGP profiles implementations
> > > > don't specifically require bonding between the device and the host.
> > > >
> > > > This creates an opportunity for an malicious device to connect to a
> > > > target host to either impersonate an existing HID device without
> > > > security or to cause an SDP or GATT service discovery to take place
> > > > which would allow HID reports to be injected to the input subsystem from
> > > > a non-bonded source.
> > > >
> > > > This patch series addresses the issue by ensuring that only connections
> > > > from devices that are bonded are accepted by the HID and HOGP profile
> > > > implementation.
> > > >
> > > > More information about the vulnerability is available here:
> > > > https://www.intel.com/content/www/us/en/security-center/advisory/intel-sa-00352.html
> > > >
> > > > Alain Michaud (2):
> > > >  HOGP must only accept data from bonded devices.
> > > >  HID accepts bonded device connections only.
> > > >
> > > > profiles/input/device.c   | 23 ++++++++++++++++++++++-
> > > > profiles/input/device.h   |  1 +
> > > > profiles/input/hog.c      |  4 ++++
> > > > profiles/input/input.conf |  8 ++++++++
> > > > profiles/input/manager.c  | 13 ++++++++++++-
> > > > 5 files changed, 47 insertions(+), 2 deletions(-)
> > >
> > > both patches have been applied.
> > >
> > > However I changed BrBondedOnly configuration name into ClassicBondedOnly since that name seemed more obvious to me. The prefix Br has never been used and the Bluetooth SIG started calling it Classic a while back.
> >
> > Looks like you were quicker than me, anyway I do fill like we should
> > attempt to bump to security instead of just refuse to connection in
> > case of HoG since we are the central and the peripheral is not
> > mandated to started it either it may be just the client application is
> > attempting to connect to trigger pairing on demand, that would usually
> > kick latter when reading the characteristics but with this changes it
> > doesn't even get to that point if the devices was not bonded already.
> The specification for HoG is that the device is bonded.  If client or
> server attempts to access the service before it is bonded, it would
> violate the specification.  For this reason, I believe it is both
> safer and simpler to just reject any attempts to access the service
> without first being bonded.

Ive sent a patch for it, we would not be accessing anything while the
security is being bump as the kernel would block that, so in practice
this would just initiate the pairing procedure if the device is not
bonded .


-- 
Luiz Augusto von Dentz