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;
MIME-Version: 1.0
References: <20200310151100.134881-1-alainm@chromium.org> <4C7539C9-2AEB-4B1A-93CD-23B468C49B2F@holtmann.org>
In-Reply-To: <4C7539C9-2AEB-4B1A-93CD-23B468C49B2F@holtmann.org>
From:   Alain Michaud <alainmichaud@google.com>
Date:   Wed, 11 Mar 2020 11:01:32 -0400
Message-ID: <CALWDO_Xpj8vD=9_KK=P=XWhVV7N5wb80duFw=tsP=0k58ct55A@mail.gmail.com>
Subject: Re: [PATCH v1] bluetooth: Enforce classic key size verification.
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     Alain Michaud <alainm@chromium.org>,
        Bluez mailing list <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Marcel,


On Wed, Mar 11, 2020 at 10:49 AM Marcel Holtmann <marcel@holtmann.org> wrot=
e:
>
> 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     | 10 ++++++++++
> > net/bluetooth/hci_core.c  | 10 ++++++++++
> > net/bluetooth/hci_event.c |  5 +++++
> > 3 files changed, 25 insertions(+)
> >
> > diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
> > index 165148c7c4ce..6a155b7b6fb2 100644
> > --- a/net/bluetooth/Kconfig
> > +++ b/net/bluetooth/Kconfig
> > @@ -128,4 +128,14 @@ config BT_DEBUGFS
> >         Provide extensive information about internal Bluetooth states
> >         in debugfs.
> >
> > +config BT_ENFORCE_CLASSIC_KEY_SIZES
> > +     bool "Enforces security requirements for Bluetooth classic"
> > +     depends on BT
> > +     default y
> > +     help
> > +       Enforces Bluetooth classic security requirements by disallowing=
 use of
> > +       insecure Bluetooth chips, i.e. that doesn't support Read Encryp=
tion
> > +       Key Size command to prevent BT classic connection with very sho=
rt
> > +       encryption key.
> > +
>
> I would drop the CLASSIC part here since it doesn=E2=80=99t really matter=
 that we can enforce this in the host for LE. In general we require the har=
dware to give us all basics. So I would just do
>
>         config BT_ENFORCE_ENC_KEY_SIZE
>
> I addition, I think that I want to put this behind BT_EXPERT option and a=
ctually have this default to n. Distributions or products should be conscio=
us about enabling this. Otherwise I am fine doing this.
ACK.

>
> > 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 !suppo=
rted");
> > +             ret =3D -EIO;
> > +     }
> > +#endif
> > +
>
> I do not need to check if this is best place. So actually I would like to=
 keep the controller in unconfigured state if a command is missing. It woul=
d stay in unconfigured state forever since there is no way to undo it.
I may need some more guidance on how to leave it in an unconfigured
state the right way.


>
> --- a/doc/mgmt-api.txt
> +++ b/doc/mgmt-api.txt
> @@ -2172,6 +2172,7 @@ Read Controller Configuration Information Command
>
>                 0       External configuration
>                 1       Bluetooth public address configuration
> +               2       Encryption Key Size enforcement
>
>         It is valid to call this command on controllers that do not
>         require any configuration. It is possible that a fully configured
>
> So if the Read Encryption Key Size command is supported, I would like to =
set the Supported_Options bit. And the Missing_Options bit would be set dep=
ending on this new Kconfig option.
>
> The real advantage with doing it like this is that when a controller is u=
nconfigured, it is easy to detect and can be skipped for systems that have =
multiple controllers attached.
>
> One fun part of course is that you could disable BR/EDR and enable LE and=
 this option could become allowed again. I would have to check if we can do=
 that with existing mgmt commands and it would flip the Missing_Options bit=
.
Ack.

>
> >       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..6605da7ec72e 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -2902,7 +2902,12 @@ static void read_enc_key_size_complete(struct hc=
i_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
> > +             hci_disconnect(conn, HCI_ERROR_REMOTE_USER_TERM);
> > +             hci_conn_drop(conn);
> > +#else
> >               conn->enc_key_size =3D HCI_LINK_KEY_SIZE;
> > +#endif
>
> >       } else {
> >               conn->enc_key_size =3D rp->key_size;
> >       }
>
> This change is not needed at all if you can not bring up a controller tha=
t missing the command.
This change is different as it deals with the command failing for any
reasons.  In that case it would be wrong to assume the key size is 16.

>
> Regards
>
> Marcel
>