Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3608.60.0.2.5\))
Subject: Re: [PATCH v2] bluetooth: Enforce classic key size verification.
From:   Marcel Holtmann <marcel@holtmann.org>
In-Reply-To: <20200320133748.154926-1-alainm@chromium.org>
Date:   Sun, 22 Mar 2020 09:08:53 +0100
Cc:     linux-bluetooth@vger.kernel.org
Content-Transfer-Encoding: 8BIT
Message-Id: <6953D3BD-54D7-4453-992D-20B881B92AE4@holtmann.org>
References: <20200320133748.154926-1-alainm@chromium.org>
To:     Alain Michaud <alainm@chromium.org>
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

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