[PATCH v2] bluetooth: Enforce classic key size verification.

[PATCH v2] bluetooth: Enforce classic key size verification.

[Alain Michaud]

This change introduces a new configuration to strictly enforce key size
checks.  This ensures that systems are in a secured configuration by
default while allowing for a compatible posture via a Kconfig option to
support controllers who may not support the read encryption key size
command.

Signed-off-by: Alain Michaud <alainm@chromium.org>
---

 net/bluetooth/Kconfig     | 20 ++++++++++++++++++++
 net/bluetooth/hci_core.c  | 10 ++++++++++
 net/bluetooth/hci_event.c |  4 ++++
 3 files changed, 34 insertions(+)

diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
index 165148c7c4ce..8e177d4f3f02 100644
--- a/net/bluetooth/Kconfig
+++ b/net/bluetooth/Kconfig
@@ -128,4 +128,24 @@ config BT_DEBUGFS
 	  Provide extensive information about internal Bluetooth states
 	  in debugfs.
 
+config BT_EXPERT
+	bool "Expert Bluetooth options"
+	depends on BT
+	default n
+	help
+	  Provides a set of expert options and configurations that should
+	  only be used deliberately by BT experts.  This is considered a
+	  global switch to ensure these advanced features or options that
+	  depends on BT_EXPERT are only used in expert mode.
+
+config BT_ENFORCE_CLASSIC_KEY_SIZES
+	bool "Enforces security requirements for Bluetooth classic"
+	depends on BT && BT_EXPERT
+	default y
+	help
+	  Enforces Bluetooth classic security requirements by disallowing
+	  use of insecure Bluetooth controllers, i.e. that doesn't support
+	  Read Encryption Key Size command to prevent BT classic connection
+	  with very short encryption key.
+
 source "drivers/bluetooth/Kconfig"
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 4e6d61a95b20..142130d4b66b 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1540,6 +1540,16 @@ static int hci_dev_do_open(struct hci_dev *hdev)
 
 	clear_bit(HCI_INIT, &hdev->flags);
 
+#ifdef BT_ENFORCE_CLASSIC_KEY_SIZES
+	/* Don't allow usage of Bluetooth if the chip doesn't support */
+	/* Read Encryption Key Size command */
+	if (!ret && !(hdev->commands[20] & 0x10)) {
+		bt_dev_err(hdev,
+			   "Disabling BT, Read Encryption Key Size !supported");
+		ret = -EIO;
+	}
+#endif
+
 	if (!ret) {
 		hci_dev_hold(hdev);
 		hci_dev_set_flag(hdev, HCI_RPA_EXPIRED);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index a40ed31f6eb8..54f90799a088 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2902,7 +2902,11 @@ static void read_enc_key_size_complete(struct hci_dev *hdev, u8 status,
 	if (rp->status) {
 		bt_dev_err(hdev, "failed to read key size for handle %u",
 			   handle);
+#ifdef BT_ENFORCE_CLASSIC_KEY_SIZES
+		conn->enc_key_size = 0;
+#else
 		conn->enc_key_size = HCI_LINK_KEY_SIZE;
+#endif
 	} else {
 		conn->enc_key_size = rp->key_size;
 	}
-- 
2.25.1.696.g5e7596f4ac-goog

Re: [PATCH v2] bluetooth: Enforce classic key size verification.

[Alain Michaud]

Hi Marcel,


On Fri, Mar 20, 2020 at 9:37 AM Alain Michaud <alainm@chromium.org> wrote:
>
> This change introduces a new configuration to strictly enforce key size
> checks.  This ensures that systems are in a secured configuration by
> default while allowing for a compatible posture via a Kconfig option to
> support controllers who may not support the read encryption key size
> command.
>
> Signed-off-by: Alain Michaud <alainm@chromium.org>
> ---
>
>  net/bluetooth/Kconfig     | 20 ++++++++++++++++++++
>  net/bluetooth/hci_core.c  | 10 ++++++++++
>  net/bluetooth/hci_event.c |  4 ++++
>  3 files changed, 34 insertions(+)
>
> diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
> index 165148c7c4ce..8e177d4f3f02 100644
> --- a/net/bluetooth/Kconfig
> +++ b/net/bluetooth/Kconfig
> @@ -128,4 +128,24 @@ config BT_DEBUGFS
>           Provide extensive information about internal Bluetooth states
>           in debugfs.
>
> +config BT_EXPERT
> +       bool "Expert Bluetooth options"
> +       depends on BT
> +       default n
> +       help
> +         Provides a set of expert options and configurations that should
> +         only be used deliberately by BT experts.  This is considered a
> +         global switch to ensure these advanced features or options that
> +         depends on BT_EXPERT are only used in expert mode.
> +
> +config BT_ENFORCE_CLASSIC_KEY_SIZES
> +       bool "Enforces security requirements for Bluetooth classic"
> +       depends on BT && BT_EXPERT
> +       default y
> +       help
> +         Enforces Bluetooth classic security requirements by disallowing
> +         use of insecure Bluetooth controllers, i.e. that doesn't support
> +         Read Encryption Key Size command to prevent BT classic connection
> +         with very short encryption key.
> +
>  source "drivers/bluetooth/Kconfig"
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 4e6d61a95b20..142130d4b66b 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1540,6 +1540,16 @@ static int hci_dev_do_open(struct hci_dev *hdev)
>
>         clear_bit(HCI_INIT, &hdev->flags);
>
> +#ifdef BT_ENFORCE_CLASSIC_KEY_SIZES
> +       /* Don't allow usage of Bluetooth if the chip doesn't support */
> +       /* Read Encryption Key Size command */
> +       if (!ret && !(hdev->commands[20] & 0x10)) {
> +               bt_dev_err(hdev,
> +                          "Disabling BT, Read Encryption Key Size !supported");
> +               ret = -EIO;
> +       }
> +#endif
Just FYI, I haven't changed this bit yet.  I'll wait for your guidance
on where best to put this to leave the controller in the right state.
> +
>         if (!ret) {
>                 hci_dev_hold(hdev);
>                 hci_dev_set_flag(hdev, HCI_RPA_EXPIRED);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index a40ed31f6eb8..54f90799a088 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2902,7 +2902,11 @@ static void read_enc_key_size_complete(struct hci_dev *hdev, u8 status,
>         if (rp->status) {
>                 bt_dev_err(hdev, "failed to read key size for handle %u",
>                            handle);
> +#ifdef BT_ENFORCE_CLASSIC_KEY_SIZES
> +               conn->enc_key_size = 0;
> +#else
>                 conn->enc_key_size = HCI_LINK_KEY_SIZE;
> +#endif
>         } else {
>                 conn->enc_key_size = rp->key_size;
>         }
> --
> 2.25.1.696.g5e7596f4ac-goog
>

Re: [PATCH v2] bluetooth: Enforce classic key size verification.

[Marcel Holtmann]

Hi Alain,

>> This change introduces a new configuration to strictly enforce key size
>> checks.  This ensures that systems are in a secured configuration by
>> default while allowing for a compatible posture via a Kconfig option to
>> support controllers who may not support the read encryption key size
>> command.
>> 
>> Signed-off-by: Alain Michaud <alainm@chromium.org>
>> ---
>> 
>> net/bluetooth/Kconfig     | 20 ++++++++++++++++++++
>> net/bluetooth/hci_core.c  | 10 ++++++++++
>> net/bluetooth/hci_event.c |  4 ++++
>> 3 files changed, 34 insertions(+)
>> 
>> diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
>> index 165148c7c4ce..8e177d4f3f02 100644
>> --- a/net/bluetooth/Kconfig
>> +++ b/net/bluetooth/Kconfig
>> @@ -128,4 +128,24 @@ config BT_DEBUGFS
>>          Provide extensive information about internal Bluetooth states
>>          in debugfs.
>> 
>> +config BT_EXPERT
>> +       bool "Expert Bluetooth options"
>> +       depends on BT
>> +       default n
>> +       help
>> +         Provides a set of expert options and configurations that should
>> +         only be used deliberately by BT experts.  This is considered a
>> +         global switch to ensure these advanced features or options that
>> +         depends on BT_EXPERT are only used in expert mode.
>> +
>> +config BT_ENFORCE_CLASSIC_KEY_SIZES
>> +       bool "Enforces security requirements for Bluetooth classic"
>> +       depends on BT && BT_EXPERT
>> +       default y
>> +       help
>> +         Enforces Bluetooth classic security requirements by disallowing
>> +         use of insecure Bluetooth controllers, i.e. that doesn't support
>> +         Read Encryption Key Size command to prevent BT classic connection
>> +         with very short encryption key.
>> +
>> source "drivers/bluetooth/Kconfig"
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 4e6d61a95b20..142130d4b66b 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1540,6 +1540,16 @@ static int hci_dev_do_open(struct hci_dev *hdev)
>> 
>>        clear_bit(HCI_INIT, &hdev->flags);
>> 
>> +#ifdef BT_ENFORCE_CLASSIC_KEY_SIZES
>> +       /* Don't allow usage of Bluetooth if the chip doesn't support */
>> +       /* Read Encryption Key Size command */
>> +       if (!ret && !(hdev->commands[20] & 0x10)) {
>> +               bt_dev_err(hdev,
>> +                          "Disabling BT, Read Encryption Key Size !supported");
>> +               ret = -EIO;
>> +       }
>> +#endif
> Just FYI, I haven't changed this bit yet.  I'll wait for your guidance
> on where best to put this to leave the controller in the right state.

while I was writing a patch to show how to use unconfigured state for controllers that don’t support the Read Encryption Key Size command, I was wonder why put this into the kernel in the first place.

I was thinking that essentially userspace can just make the decision to use a controller, or use it in LE only mode or not use a controller at all. So all we need is to collect the security information of the controller and kernel and expose them to bluetoothd.

+Read Security Features Command
+==============================
+
+       Command Code:           0x0048
+       Controller Index:       <controller id>
+       Command Parameters:
+       Return Parameters:      Security_Features (4 Octets)
+
+       This command is used to retrieve the supported security features
+       by the controller or the kernel.
+
+       The Security_Features parameter is a bitmask with currently the
+       following available bits:
+
+               0       Encryption Key Size enforcement (BR/EDR)
+               1       Encryption Key Size enforcement (LE)
+
+       This command generates a Command Complete event on success or
+       a Command Status event on failure.
+
+       Possible errors:        Invalid Parameters
+                               Invalid Index
+
+

I was also considering that we additionally add the ECDH Public Key validation here as supported bits. And in the future even more security related information that we want to enforce. However the enforcement to power on or not use a controller is left to bluetoothd and its main.conf configuration. Thoughts?

Regards

Marcel

Re: [PATCH v2] bluetooth: Enforce classic key size verification.

[Alain Michaud]

Hi Marcel,

On Sun, Mar 22, 2020 at 4:17 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Alain,
>
> >> This change introduces a new configuration to strictly enforce key size
> >> checks.  This ensures that systems are in a secured configuration by
> >> default while allowing for a compatible posture via a Kconfig option to
> >> support controllers who may not support the read encryption key size
> >> command.
> >>
> >> Signed-off-by: Alain Michaud <alainm@chromium.org>
> >> ---
> >>
> >> net/bluetooth/Kconfig     | 20 ++++++++++++++++++++
> >> net/bluetooth/hci_core.c  | 10 ++++++++++
> >> net/bluetooth/hci_event.c |  4 ++++
> >> 3 files changed, 34 insertions(+)
> >>
> >> diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
> >> index 165148c7c4ce..8e177d4f3f02 100644
> >> --- a/net/bluetooth/Kconfig
> >> +++ b/net/bluetooth/Kconfig
> >> @@ -128,4 +128,24 @@ config BT_DEBUGFS
> >>          Provide extensive information about internal Bluetooth states
> >>          in debugfs.
> >>
> >> +config BT_EXPERT
> >> +       bool "Expert Bluetooth options"
> >> +       depends on BT
> >> +       default n
> >> +       help
> >> +         Provides a set of expert options and configurations that should
> >> +         only be used deliberately by BT experts.  This is considered a
> >> +         global switch to ensure these advanced features or options that
> >> +         depends on BT_EXPERT are only used in expert mode.
> >> +
> >> +config BT_ENFORCE_CLASSIC_KEY_SIZES
> >> +       bool "Enforces security requirements for Bluetooth classic"
> >> +       depends on BT && BT_EXPERT
> >> +       default y
> >> +       help
> >> +         Enforces Bluetooth classic security requirements by disallowing
> >> +         use of insecure Bluetooth controllers, i.e. that doesn't support
> >> +         Read Encryption Key Size command to prevent BT classic connection
> >> +         with very short encryption key.
> >> +
> >> source "drivers/bluetooth/Kconfig"
> >> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> >> index 4e6d61a95b20..142130d4b66b 100644
> >> --- a/net/bluetooth/hci_core.c
> >> +++ b/net/bluetooth/hci_core.c
> >> @@ -1540,6 +1540,16 @@ static int hci_dev_do_open(struct hci_dev *hdev)
> >>
> >>        clear_bit(HCI_INIT, &hdev->flags);
> >>
> >> +#ifdef BT_ENFORCE_CLASSIC_KEY_SIZES
> >> +       /* Don't allow usage of Bluetooth if the chip doesn't support */
> >> +       /* Read Encryption Key Size command */
> >> +       if (!ret && !(hdev->commands[20] & 0x10)) {
> >> +               bt_dev_err(hdev,
> >> +                          "Disabling BT, Read Encryption Key Size !supported");
> >> +               ret = -EIO;
> >> +       }
> >> +#endif
> > Just FYI, I haven't changed this bit yet.  I'll wait for your guidance
> > on where best to put this to leave the controller in the right state.
>
> while I was writing a patch to show how to use unconfigured state for controllers that don’t support the Read Encryption Key Size command, I was wonder why put this into the kernel in the first place.
>
> I was thinking that essentially userspace can just make the decision to use a controller, or use it in LE only mode or not use a controller at all. So all we need is to collect the security information of the controller and kernel and expose them to bluetoothd.
>
> +Read Security Features Command
> +==============================
> +
> +       Command Code:           0x0048
> +       Controller Index:       <controller id>
> +       Command Parameters:
> +       Return Parameters:      Security_Features (4 Octets)
> +
> +       This command is used to retrieve the supported security features
> +       by the controller or the kernel.
> +
> +       The Security_Features parameter is a bitmask with currently the
> +       following available bits:
> +
> +               0       Encryption Key Size enforcement (BR/EDR)
> +               1       Encryption Key Size enforcement (LE)
> +
> +       This command generates a Command Complete event on success or
> +       a Command Status event on failure.
> +
> +       Possible errors:        Invalid Parameters
> +                               Invalid Index
> +
> +
>
> I was also considering that we additionally add the ECDH Public Key validation here as supported bits. And in the future even more security related information that we want to enforce. However the enforcement to power on or not use a controller is left to bluetoothd and its main.conf configuration. Thoughts?
I like the idea.  However, I feel we will still need to guard against
the Read Encryption Key Size failing.  Perhaps we can just do this
unconditionally (where it is reported as supported but fails, we
simply set the encryption key size to 0 and move on).
>
> Regards
>
> Marcel
>

Re: [PATCH v2] bluetooth: Enforce classic key size verification.

[Marcel Holtmann]

Hi Alain,

>>>> This change introduces a new configuration to strictly enforce key size
>>>> checks.  This ensures that systems are in a secured configuration by
>>>> default while allowing for a compatible posture via a Kconfig option to
>>>> support controllers who may not support the read encryption key size
>>>> command.
>>>> 
>>>> Signed-off-by: Alain Michaud <alainm@chromium.org>
>>>> ---
>>>> 
>>>> net/bluetooth/Kconfig     | 20 ++++++++++++++++++++
>>>> net/bluetooth/hci_core.c  | 10 ++++++++++
>>>> net/bluetooth/hci_event.c |  4 ++++
>>>> 3 files changed, 34 insertions(+)
>>>> 
>>>> diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
>>>> index 165148c7c4ce..8e177d4f3f02 100644
>>>> --- a/net/bluetooth/Kconfig
>>>> +++ b/net/bluetooth/Kconfig
>>>> @@ -128,4 +128,24 @@ config BT_DEBUGFS
>>>>         Provide extensive information about internal Bluetooth states
>>>>         in debugfs.
>>>> 
>>>> +config BT_EXPERT
>>>> +       bool "Expert Bluetooth options"
>>>> +       depends on BT
>>>> +       default n
>>>> +       help
>>>> +         Provides a set of expert options and configurations that should
>>>> +         only be used deliberately by BT experts.  This is considered a
>>>> +         global switch to ensure these advanced features or options that
>>>> +         depends on BT_EXPERT are only used in expert mode.
>>>> +
>>>> +config BT_ENFORCE_CLASSIC_KEY_SIZES
>>>> +       bool "Enforces security requirements for Bluetooth classic"
>>>> +       depends on BT && BT_EXPERT
>>>> +       default y
>>>> +       help
>>>> +         Enforces Bluetooth classic security requirements by disallowing
>>>> +         use of insecure Bluetooth controllers, i.e. that doesn't support
>>>> +         Read Encryption Key Size command to prevent BT classic connection
>>>> +         with very short encryption key.
>>>> +
>>>> source "drivers/bluetooth/Kconfig"
>>>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>>>> index 4e6d61a95b20..142130d4b66b 100644
>>>> --- a/net/bluetooth/hci_core.c
>>>> +++ b/net/bluetooth/hci_core.c
>>>> @@ -1540,6 +1540,16 @@ static int hci_dev_do_open(struct hci_dev *hdev)
>>>> 
>>>>       clear_bit(HCI_INIT, &hdev->flags);
>>>> 
>>>> +#ifdef BT_ENFORCE_CLASSIC_KEY_SIZES
>>>> +       /* Don't allow usage of Bluetooth if the chip doesn't support */
>>>> +       /* Read Encryption Key Size command */
>>>> +       if (!ret && !(hdev->commands[20] & 0x10)) {
>>>> +               bt_dev_err(hdev,
>>>> +                          "Disabling BT, Read Encryption Key Size !supported");
>>>> +               ret = -EIO;
>>>> +       }
>>>> +#endif
>>> Just FYI, I haven't changed this bit yet.  I'll wait for your guidance
>>> on where best to put this to leave the controller in the right state.
>> 
>> while I was writing a patch to show how to use unconfigured state for controllers that don’t support the Read Encryption Key Size command, I was wonder why put this into the kernel in the first place.
>> 
>> I was thinking that essentially userspace can just make the decision to use a controller, or use it in LE only mode or not use a controller at all. So all we need is to collect the security information of the controller and kernel and expose them to bluetoothd.
>> 
>> +Read Security Features Command
>> +==============================
>> +
>> +       Command Code:           0x0048
>> +       Controller Index:       <controller id>
>> +       Command Parameters:
>> +       Return Parameters:      Security_Features (4 Octets)
>> +
>> +       This command is used to retrieve the supported security features
>> +       by the controller or the kernel.
>> +
>> +       The Security_Features parameter is a bitmask with currently the
>> +       following available bits:
>> +
>> +               0       Encryption Key Size enforcement (BR/EDR)
>> +               1       Encryption Key Size enforcement (LE)
>> +
>> +       This command generates a Command Complete event on success or
>> +       a Command Status event on failure.
>> +
>> +       Possible errors:        Invalid Parameters
>> +                               Invalid Index
>> +
>> +
>> 
>> I was also considering that we additionally add the ECDH Public Key validation here as supported bits. And in the future even more security related information that we want to enforce. However the enforcement to power on or not use a controller is left to bluetoothd and its main.conf configuration. Thoughts?
> I like the idea.  However, I feel we will still need to guard against
> the Read Encryption Key Size failing.  Perhaps we can just do this
> unconditionally (where it is reported as supported but fails, we
> simply set the encryption key size to 0 and move on).

I was thinking the same thing. Lets just set the encryption size to zero and report the error. Care to send a patch for it or should I send one?

Regards

Marcel

Re: [PATCH v2] bluetooth: Enforce classic key size verification.

[Alain Michaud]

On Tue, Mar 24, 2020 at 2:33 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Alain,
>
> >>>> This change introduces a new configuration to strictly enforce key size
> >>>> checks.  This ensures that systems are in a secured configuration by
> >>>> default while allowing for a compatible posture via a Kconfig option to
> >>>> support controllers who may not support the read encryption key size
> >>>> command.
> >>>>
> >>>> Signed-off-by: Alain Michaud <alainm@chromium.org>
> >>>> ---
> >>>>
> >>>> net/bluetooth/Kconfig     | 20 ++++++++++++++++++++
> >>>> net/bluetooth/hci_core.c  | 10 ++++++++++
> >>>> net/bluetooth/hci_event.c |  4 ++++
> >>>> 3 files changed, 34 insertions(+)
> >>>>
> >>>> diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
> >>>> index 165148c7c4ce..8e177d4f3f02 100644
> >>>> --- a/net/bluetooth/Kconfig
> >>>> +++ b/net/bluetooth/Kconfig
> >>>> @@ -128,4 +128,24 @@ config BT_DEBUGFS
> >>>>         Provide extensive information about internal Bluetooth states
> >>>>         in debugfs.
> >>>>
> >>>> +config BT_EXPERT
> >>>> +       bool "Expert Bluetooth options"
> >>>> +       depends on BT
> >>>> +       default n
> >>>> +       help
> >>>> +         Provides a set of expert options and configurations that should
> >>>> +         only be used deliberately by BT experts.  This is considered a
> >>>> +         global switch to ensure these advanced features or options that
> >>>> +         depends on BT_EXPERT are only used in expert mode.
> >>>> +
> >>>> +config BT_ENFORCE_CLASSIC_KEY_SIZES
> >>>> +       bool "Enforces security requirements for Bluetooth classic"
> >>>> +       depends on BT && BT_EXPERT
> >>>> +       default y
> >>>> +       help
> >>>> +         Enforces Bluetooth classic security requirements by disallowing
> >>>> +         use of insecure Bluetooth controllers, i.e. that doesn't support
> >>>> +         Read Encryption Key Size command to prevent BT classic connection
> >>>> +         with very short encryption key.
> >>>> +
> >>>> source "drivers/bluetooth/Kconfig"
> >>>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> >>>> index 4e6d61a95b20..142130d4b66b 100644
> >>>> --- a/net/bluetooth/hci_core.c
> >>>> +++ b/net/bluetooth/hci_core.c
> >>>> @@ -1540,6 +1540,16 @@ static int hci_dev_do_open(struct hci_dev *hdev)
> >>>>
> >>>>       clear_bit(HCI_INIT, &hdev->flags);
> >>>>
> >>>> +#ifdef BT_ENFORCE_CLASSIC_KEY_SIZES
> >>>> +       /* Don't allow usage of Bluetooth if the chip doesn't support */
> >>>> +       /* Read Encryption Key Size command */
> >>>> +       if (!ret && !(hdev->commands[20] & 0x10)) {
> >>>> +               bt_dev_err(hdev,
> >>>> +                          "Disabling BT, Read Encryption Key Size !supported");
> >>>> +               ret = -EIO;
> >>>> +       }
> >>>> +#endif
> >>> Just FYI, I haven't changed this bit yet.  I'll wait for your guidance
> >>> on where best to put this to leave the controller in the right state.
> >>
> >> while I was writing a patch to show how to use unconfigured state for controllers that don’t support the Read Encryption Key Size command, I was wonder why put this into the kernel in the first place.
> >>
> >> I was thinking that essentially userspace can just make the decision to use a controller, or use it in LE only mode or not use a controller at all. So all we need is to collect the security information of the controller and kernel and expose them to bluetoothd.
> >>
> >> +Read Security Features Command
> >> +==============================
> >> +
> >> +       Command Code:           0x0048
> >> +       Controller Index:       <controller id>
> >> +       Command Parameters:
> >> +       Return Parameters:      Security_Features (4 Octets)
> >> +
> >> +       This command is used to retrieve the supported security features
> >> +       by the controller or the kernel.
> >> +
> >> +       The Security_Features parameter is a bitmask with currently the
> >> +       following available bits:
> >> +
> >> +               0       Encryption Key Size enforcement (BR/EDR)
> >> +               1       Encryption Key Size enforcement (LE)
> >> +
> >> +       This command generates a Command Complete event on success or
> >> +       a Command Status event on failure.
> >> +
> >> +       Possible errors:        Invalid Parameters
> >> +                               Invalid Index
> >> +
> >> +
> >>
> >> I was also considering that we additionally add the ECDH Public Key validation here as supported bits. And in the future even more security related information that we want to enforce. However the enforcement to power on or not use a controller is left to bluetoothd and its main.conf configuration. Thoughts?
> > I like the idea.  However, I feel we will still need to guard against
> > the Read Encryption Key Size failing.  Perhaps we can just do this
> > unconditionally (where it is reported as supported but fails, we
> > simply set the encryption key size to 0 and move on).
>
> I was thinking the same thing. Lets just set the encryption size to zero and report the error. Care to send a patch for it or should I send one?
I'll do it shortly.

Would you want to send patch you suggested (eg: Read Security Features
Command) or should I?

>
> Regards
>
> Marcel
>

Re: [PATCH v2] bluetooth: Enforce classic key size verification.

[Marcel Holtmann]

Hi Alain,

>>>>>> This change introduces a new configuration to strictly enforce key size
>>>>>> checks.  This ensures that systems are in a secured configuration by
>>>>>> default while allowing for a compatible posture via a Kconfig option to
>>>>>> support controllers who may not support the read encryption key size
>>>>>> command.
>>>>>> 
>>>>>> Signed-off-by: Alain Michaud <alainm@chromium.org>
>>>>>> ---
>>>>>> 
>>>>>> net/bluetooth/Kconfig     | 20 ++++++++++++++++++++
>>>>>> net/bluetooth/hci_core.c  | 10 ++++++++++
>>>>>> net/bluetooth/hci_event.c |  4 ++++
>>>>>> 3 files changed, 34 insertions(+)
>>>>>> 
>>>>>> diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
>>>>>> index 165148c7c4ce..8e177d4f3f02 100644
>>>>>> --- a/net/bluetooth/Kconfig
>>>>>> +++ b/net/bluetooth/Kconfig
>>>>>> @@ -128,4 +128,24 @@ config BT_DEBUGFS
>>>>>>        Provide extensive information about internal Bluetooth states
>>>>>>        in debugfs.
>>>>>> 
>>>>>> +config BT_EXPERT
>>>>>> +       bool "Expert Bluetooth options"
>>>>>> +       depends on BT
>>>>>> +       default n
>>>>>> +       help
>>>>>> +         Provides a set of expert options and configurations that should
>>>>>> +         only be used deliberately by BT experts.  This is considered a
>>>>>> +         global switch to ensure these advanced features or options that
>>>>>> +         depends on BT_EXPERT are only used in expert mode.
>>>>>> +
>>>>>> +config BT_ENFORCE_CLASSIC_KEY_SIZES
>>>>>> +       bool "Enforces security requirements for Bluetooth classic"
>>>>>> +       depends on BT && BT_EXPERT
>>>>>> +       default y
>>>>>> +       help
>>>>>> +         Enforces Bluetooth classic security requirements by disallowing
>>>>>> +         use of insecure Bluetooth controllers, i.e. that doesn't support
>>>>>> +         Read Encryption Key Size command to prevent BT classic connection
>>>>>> +         with very short encryption key.
>>>>>> +
>>>>>> source "drivers/bluetooth/Kconfig"
>>>>>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>>>>>> index 4e6d61a95b20..142130d4b66b 100644
>>>>>> --- a/net/bluetooth/hci_core.c
>>>>>> +++ b/net/bluetooth/hci_core.c
>>>>>> @@ -1540,6 +1540,16 @@ static int hci_dev_do_open(struct hci_dev *hdev)
>>>>>> 
>>>>>>      clear_bit(HCI_INIT, &hdev->flags);
>>>>>> 
>>>>>> +#ifdef BT_ENFORCE_CLASSIC_KEY_SIZES
>>>>>> +       /* Don't allow usage of Bluetooth if the chip doesn't support */
>>>>>> +       /* Read Encryption Key Size command */
>>>>>> +       if (!ret && !(hdev->commands[20] & 0x10)) {
>>>>>> +               bt_dev_err(hdev,
>>>>>> +                          "Disabling BT, Read Encryption Key Size !supported");
>>>>>> +               ret = -EIO;
>>>>>> +       }
>>>>>> +#endif
>>>>> Just FYI, I haven't changed this bit yet.  I'll wait for your guidance
>>>>> on where best to put this to leave the controller in the right state.
>>>> 
>>>> while I was writing a patch to show how to use unconfigured state for controllers that don’t support the Read Encryption Key Size command, I was wonder why put this into the kernel in the first place.
>>>> 
>>>> I was thinking that essentially userspace can just make the decision to use a controller, or use it in LE only mode or not use a controller at all. So all we need is to collect the security information of the controller and kernel and expose them to bluetoothd.
>>>> 
>>>> +Read Security Features Command
>>>> +==============================
>>>> +
>>>> +       Command Code:           0x0048
>>>> +       Controller Index:       <controller id>
>>>> +       Command Parameters:
>>>> +       Return Parameters:      Security_Features (4 Octets)
>>>> +
>>>> +       This command is used to retrieve the supported security features
>>>> +       by the controller or the kernel.
>>>> +
>>>> +       The Security_Features parameter is a bitmask with currently the
>>>> +       following available bits:
>>>> +
>>>> +               0       Encryption Key Size enforcement (BR/EDR)
>>>> +               1       Encryption Key Size enforcement (LE)
>>>> +
>>>> +       This command generates a Command Complete event on success or
>>>> +       a Command Status event on failure.
>>>> +
>>>> +       Possible errors:        Invalid Parameters
>>>> +                               Invalid Index
>>>> +
>>>> +
>>>> 
>>>> I was also considering that we additionally add the ECDH Public Key validation here as supported bits. And in the future even more security related information that we want to enforce. However the enforcement to power on or not use a controller is left to bluetoothd and its main.conf configuration. Thoughts?
>>> I like the idea.  However, I feel we will still need to guard against
>>> the Read Encryption Key Size failing.  Perhaps we can just do this
>>> unconditionally (where it is reported as supported but fails, we
>>> simply set the encryption key size to 0 and move on).
>> 
>> I was thinking the same thing. Lets just set the encryption size to zero and report the error. Care to send a patch for it or should I send one?
> I'll do it shortly.
> 
> Would you want to send patch you suggested (eg: Read Security Features
> Command) or should I?

I was going to send one, but I was wondering if we go beyond a simple 4 octets bit list.

The encryption key size enforcement for BR/EDR is simple and depends on a supported command. And for LE we always do it. The public key validation is a bit funky since for some hardware we would need a negative feature since up to a certain time we assumed it was done the right way.

Additionally we added commands to return the max encryption key size (and some hardware has vendor commands to do so). Do we for example care if the max encryption key size supported is 56-bits or 128-bits.

I would like to spent a few brain cycles to think this through. The patch for returning “static” values is written within a few minutes once we figured out what bluetoothd needs from the kernel.

Regards

Marcel

Re: [PATCH v2] bluetooth: Enforce classic key size verification.

[Marcel Holtmann]

Hi Alain,

> I suspect we'd want bluetoothd to have a configuration that can enforce a more secure posture.
> 
> Unfortunately when the command isn't supported, the platform is left between a rock and hard place... There isn't much we can do but to block the use of Bluetooth if the platform requires a more secure posture.

so if the BR/EDR part is not up to the policy that the host requires, we could still configure the LE part. BlueZ is set up in this way that you can run a dual-mode controller as just a LE controller.

I would also opt for the kernel just tells us what options it have. Then at least we can provide some feedback to the end-user on why Bluetooth is not available or why only selected features are available.

Regards

Marcel

Re: [PATCH v2] bluetooth: Enforce classic key size verification.

[Marcel Holtmann]

Hi Alain,

>> I suspect we'd want bluetoothd to have a configuration that can enforce a more secure posture.
>> 
>> Unfortunately when the command isn't supported, the platform is left between a rock and hard place... There isn't much we can do but to block the use of Bluetooth if the platform requires a more secure posture.
> 
> so if the BR/EDR part is not up to the policy that the host requires, we could still configure the LE part. BlueZ is set up in this way that you can run a dual-mode controller as just a LE controller.
> 
> I would also opt for the kernel just tells us what options it have. Then at least we can provide some feedback to the end-user on why Bluetooth is not available or why only selected features are available.

what about something like this:

+Read Security Features Command
+==============================
+
+       Command Code:           0x0048
+       Controller Index:       <controller id>
+       Command Parameters:
+       Return Parameters:      Security_Data_Length (2 Octets)
+                               Security_Data (0-65535 Octets)
+
+       This command is used to retrieve the supported security features
+       by the controller or the kernel.
+
+       The Security_Data_Length and Security_Data parameters provide
+       a list of security settings, features and information. It uses
+       the same format as EIR_Data, but with the namespace defined here.
+
+               Data Type       Name
+               --------------------
+               0x01            Flags
+               0x02            Max Encryption Key Size (BR/EDR)
+               0x03            Max Encryption Key Size (LE)
+               0x04            Encryption Key Size enforcement (BR/EDR)
+               0x05            Encryption Key Size enforcement (LE)
+               0x06            ECDH Public Key validation (BR/EDR)
+               0x07            ECDH Public Key validation (LE)
+
+
+       Max Encryption Key Size (BR/EDR and LE)
+
+               When the field is present, then it provides 1 Octet value
+               indicating the max encryption key size. If the field is not
+               present, then it is unknown what the max encryption key
+               size of the controller or host is in use.
+
+       Encryption Key Size Enforcement (BR/EDR and LE)
+
+               When the field is present, then it provides 1 Octet value
+               indicating the min encryption key size that is enforced by
+               the controller or host. If the field is not present, then
+               it is unknown what the controller or host are enforcing.
+
+       ECDH Public Key validation (BR/EDR and LE)
+
+               When the field is present, then it provides 1 Octet value
+               indicating if public key validation is in use (0x01) or not
+               available (0x00). If the field is not present, then it is
+               unknown if the controller or host are validating public keys.
+
+       This command generates a Command Complete event on success or
+       a Command Status event on failure.
+
+       Possible errors:        Invalid Parameters
+                               Invalid Index

Maybe this is overkill, but it would give us some flexible way of having the kernel tell us what is supported. Then bluetoothd can decide to power a controller or parts of a controller.

Regards

Marcel

Re: [PATCH v2] bluetooth: Enforce classic key size verification.

[Alain Michaud]

Thanks Marcel, that looks good to me.  I agree it's more than the
customer asked for, but it's completely transparent :)

On Wed, Mar 25, 2020 at 2:19 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Alain,
>
> >> I suspect we'd want bluetoothd to have a configuration that can enforce a more secure posture.
> >>
> >> Unfortunately when the command isn't supported, the platform is left between a rock and hard place... There isn't much we can do but to block the use of Bluetooth if the platform requires a more secure posture.
> >
> > so if the BR/EDR part is not up to the policy that the host requires, we could still configure the LE part. BlueZ is set up in this way that you can run a dual-mode controller as just a LE controller.
> >
> > I would also opt for the kernel just tells us what options it have. Then at least we can provide some feedback to the end-user on why Bluetooth is not available or why only selected features are available.
>
> what about something like this:
>
> +Read Security Features Command
> +==============================
> +
> +       Command Code:           0x0048
> +       Controller Index:       <controller id>
> +       Command Parameters:
> +       Return Parameters:      Security_Data_Length (2 Octets)
> +                               Security_Data (0-65535 Octets)
> +
> +       This command is used to retrieve the supported security features
> +       by the controller or the kernel.
> +
> +       The Security_Data_Length and Security_Data parameters provide
> +       a list of security settings, features and information. It uses
> +       the same format as EIR_Data, but with the namespace defined here.
> +
> +               Data Type       Name
> +               --------------------
> +               0x01            Flags
> +               0x02            Max Encryption Key Size (BR/EDR)
> +               0x03            Max Encryption Key Size (LE)
> +               0x04            Encryption Key Size enforcement (BR/EDR)
> +               0x05            Encryption Key Size enforcement (LE)
> +               0x06            ECDH Public Key validation (BR/EDR)
> +               0x07            ECDH Public Key validation (LE)
> +
> +
> +       Max Encryption Key Size (BR/EDR and LE)
> +
> +               When the field is present, then it provides 1 Octet value
> +               indicating the max encryption key size. If the field is not
> +               present, then it is unknown what the max encryption key
> +               size of the controller or host is in use.
> +
> +       Encryption Key Size Enforcement (BR/EDR and LE)
> +
> +               When the field is present, then it provides 1 Octet value
> +               indicating the min encryption key size that is enforced by
> +               the controller or host. If the field is not present, then
> +               it is unknown what the controller or host are enforcing.
> +
> +       ECDH Public Key validation (BR/EDR and LE)
> +
> +               When the field is present, then it provides 1 Octet value
> +               indicating if public key validation is in use (0x01) or not
> +               available (0x00). If the field is not present, then it is
> +               unknown if the controller or host are validating public keys.
> +
> +       This command generates a Command Complete event on success or
> +       a Command Status event on failure.
> +
> +       Possible errors:        Invalid Parameters
> +                               Invalid Index
>
> Maybe this is overkill, but it would give us some flexible way of having the kernel tell us what is supported. Then bluetoothd can decide to power a controller or parts of a controller.
>
> Regards
>
> Marcel
>

Re: [PATCH v2] bluetooth: Enforce classic key size verification.

[Marcel Holtmann]

Hi Alain,

> This change introduces a new configuration to strictly enforce key size
> checks.  This ensures that systems are in a secured configuration by
> default while allowing for a compatible posture via a Kconfig option to
> support controllers who may not support the read encryption key size
> command.
> 
> Signed-off-by: Alain Michaud <alainm@chromium.org>
> ---
> 
> net/bluetooth/Kconfig     | 20 ++++++++++++++++++++
> net/bluetooth/hci_core.c  | 10 ++++++++++
> net/bluetooth/hci_event.c |  4 ++++
> 3 files changed, 34 insertions(+)
> 
> diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
> index 165148c7c4ce..8e177d4f3f02 100644
> --- a/net/bluetooth/Kconfig
> +++ b/net/bluetooth/Kconfig
> @@ -128,4 +128,24 @@ config BT_DEBUGFS
> 	  Provide extensive information about internal Bluetooth states
> 	  in debugfs.
> 
> +config BT_EXPERT
> +	bool "Expert Bluetooth options"
> +	depends on BT
> +	default n
> +	help
> +	  Provides a set of expert options and configurations that should
> +	  only be used deliberately by BT experts.  This is considered a
> +	  global switch to ensure these advanced features or options that
> +	  depends on BT_EXPERT are only used in expert mode.
> +
> +config BT_ENFORCE_CLASSIC_KEY_SIZES
> +	bool "Enforces security requirements for Bluetooth classic"
> +	depends on BT && BT_EXPERT
> +	default y
> +	help
> +	  Enforces Bluetooth classic security requirements by disallowing
> +	  use of insecure Bluetooth controllers, i.e. that doesn't support
> +	  Read Encryption Key Size command to prevent BT classic connection
> +	  with very short encryption key.
> +

actually using submenu might have been as good.

diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
index 165148c7c4ce..fc6b1cedbfc8 100644
--- a/net/bluetooth/Kconfig
+++ b/net/bluetooth/Kconfig
@@ -128,4 +128,35 @@ config BT_DEBUGFS
          Provide extensive information about internal Bluetooth states
          in debugfs.
 
+menu "Bluetooth security configuration"
+       depends on BT
+
+config BT_REQUIRE_ENC_KEY_SIZE
+       bool "Disable controllers without encryption key size support"
+       default n
+       help
+         For backwards compability reasons, the Read Encryption Key Size
+         command is not required if not available.  This options allows
+         to force the command to be support by a controller.  If it is
+         not supported, then the controller will stay in an unconfigured
+         state.
+
+         Changing this value increases the security, but might render
+         some Bluetooth controllers inactive.
+
+endmenu
+

> source "drivers/bluetooth/Kconfig"
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 4e6d61a95b20..142130d4b66b 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1540,6 +1540,16 @@ static int hci_dev_do_open(struct hci_dev *hdev)
> 
> 	clear_bit(HCI_INIT, &hdev->flags);
> 
> +#ifdef BT_ENFORCE_CLASSIC_KEY_SIZES

When used in an ifdef it is CONFIG_BT_..

> +	/* Don't allow usage of Bluetooth if the chip doesn't support */
> +	/* Read Encryption Key Size command */

This comment style is wrong.

> +	if (!ret && !(hdev->commands[20] & 0x10)) {
> +		bt_dev_err(hdev,
> +			   "Disabling BT, Read Encryption Key Size !supported");
> +		ret = -EIO;
> +	}
> +#endif
> +
> 	if (!ret) {
> 		hci_dev_hold(hdev);
> 		hci_dev_set_flag(hdev, HCI_RPA_EXPIRED);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index a40ed31f6eb8..54f90799a088 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2902,7 +2902,11 @@ static void read_enc_key_size_complete(struct hci_dev *hdev, u8 status,
> 	if (rp->status) {
> 		bt_dev_err(hdev, "failed to read key size for handle %u",
> 			   handle);
> +#ifdef BT_ENFORCE_CLASSIC_KEY_SIZES
> +		conn->enc_key_size = 0;
> +#else
> 		conn->enc_key_size = HCI_LINK_KEY_SIZE;
> +#endif
> 	} else {
> 		conn->enc_key_size = rp->key_size;
> 	}

So actually instead of putting an ifdef here, I would actually try to monitor if this can actually happen. I think this is a theoretical case. I wouldn’t know how this can actually fail since the key size is known by the controller and handing it out to the host is easy. Failure can only happen if the connection terminated in the meantime or if the handle is invalid. If all is valid, then this command will succeed.

Regards

Marcel