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: <20191210175901.31161-1-alainm@chromium.org>
In-Reply-To: <20191210175901.31161-1-alainm@chromium.org>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Sun, 15 Dec 2019 23:34:23 +0200
Message-ID: <CABBYNZ+_4z9WNy_gWfcNhsYp4tzTXNWmAhPNE9S-fxPyPSAmWg@mail.gmail.com>
Subject: Re: [PATCH v2] Loading keys that should be blocked from bluetoothd.
To:     Alain Michaud <alainm@chromium.org>
Cc:     BlueZ <linux-bluetooth@vger.kernel.org>,
        Marcel Holtmann <marcel@holtmann.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Alain,

On Tue, Dec 10, 2019 at 8:00 PM Alain Michaud <alainm@chromium.org> wrote:
>
> This changes will send a list of known bad keys that should be blocked
> if supported by the kernel.
>
> In particular keys from the Google Titan Security key are being blocked.
> For additional information, please see
> https://security.googleblog.com/2019/05/titan-keys-update.html
>
> Signed-off-by: Alain Michaud <alainm@chromium.org>

Signed off by is not used for userspace, also in the future please
prefix the userspace patches with BlueZ to make it easier to identify
them:

https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/HACKING#n118
https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/HACKING#n136

> ---
>
>  lib/mgmt.h    | 16 ++++++++++++
>  src/adapter.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/lib/mgmt.h b/lib/mgmt.h
> index 570dec997..a99b18c88 100644
> --- a/lib/mgmt.h
> +++ b/lib/mgmt.h
> @@ -583,6 +583,22 @@ struct mgmt_cp_set_phy_confguration {
>         uint32_t        selected_phys;
>  } __packed;
>
> +#define MGMT_OP_SET_BLOCKED_KEYS       0x0046
> +
> +#define HCI_BLOCKED_KEY_TYPE_LINKKEY   0x00
> +#define HCI_BLOCKED_KEY_TYPE_LTK               0x01
> +#define HCI_BLOCKED_KEY_TYPE_IRK               0x02
> +
> +struct mgmt_blocked_key_info {
> +       uint8_t type;
> +       uint8_t val[16];
> +} __packed;
> +
> +struct mgmt_cp_set_blocked_keys {
> +       uint16_t key_count;
> +       struct mgmt_blocked_key_info keys[0];
> +} __packed;
> +#define MGMT_OP_SET_BLOCKED_KEYS_SIZE 0

lib/ changes should probably be in a separate patch.

>  #define MGMT_EV_CMD_COMPLETE           0x0001
>  struct mgmt_ev_cmd_complete {
> diff --git a/src/adapter.c b/src/adapter.c
> index cef25616f..2958870e6 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -99,10 +99,27 @@
>  #define DISTANCE_VAL_INVALID   0x7FFF
>  #define PATHLOSS_MAX           137
>
> +/*
> + * These are known security keys that have been compromised.
> + * If this grows or there are needs to be platform specific, it is
> + * conceivable that these could be read from a config file.
> + */
> +static const struct mgmt_blocked_key_info blocked_keys[] = {
> +       /* Google Titan Security Keys */
> +       { HCI_BLOCKED_KEY_TYPE_LTK,
> +               {0xbf, 0x01, 0xfb, 0x9d, 0x4e, 0xf3, 0xbc, 0x36,
> +                0xd8, 0x74, 0xf5, 0x39, 0x41, 0x38, 0x68, 0x4c}},
> +       { HCI_BLOCKED_KEY_TYPE_IRK,
> +               {0xa5, 0x99, 0xba, 0xe4, 0xe1, 0x7c, 0xa6, 0x18,
> +                0x22, 0x8e, 0x07, 0x56, 0xb4, 0xe8, 0x5f, 0x01}},
> +};
> +
>  static DBusConnection *dbus_conn = NULL;
>
>  static bool kernel_conn_control = false;
>
> +static bool kernel_blocked_keys_supported = false;
> +

I guess in the long run we would probably have a single
kernel_features and then use bits instead of a bool for each of the
kernel features.

>  static GList *adapter_list = NULL;
>  static unsigned int adapter_remaining = 0;
>  static bool powering_down = false;
> @@ -8568,6 +8585,40 @@ static bool set_static_addr(struct btd_adapter *adapter)
>         return false;
>  }
>
> +static void set_blocked_keys_complete(uint8_t status, uint16_t length,
> +                                       const void *param, void *user_data)
> +{
> +       struct btd_adapter *adapter = user_data;
> +
> +       if (status != MGMT_STATUS_SUCCESS) {
> +               btd_error(adapter->dev_id,
> +                               "Failed to set blocked keys: %s (0x%02x)",
> +                               mgmt_errstr(status), status);
> +               return;
> +       }
> +
> +       DBG("Successfully set blocked keys for index %u", adapter->dev_id);
> +}
> +
> +static bool set_blocked_keys(struct btd_adapter *adapter)
> +{
> +       uint8_t buffer[sizeof(struct mgmt_cp_set_blocked_keys) +
> +                                       sizeof(blocked_keys)] = { 0 };
> +       struct mgmt_cp_set_blocked_keys *cp =
> +                                       (struct mgmt_cp_set_blocked_keys *)buffer;
> +       int i;
> +
> +       cp->key_count = G_N_ELEMENTS(blocked_keys);

At some point we would be converting to use libell, might take a while
though, so if we could perhaps use something like:

#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))

Perhaps it could be added to src/shared/util.h that way we don't have
to keep adding it all the time.

> +       for (i = 0; i < cp->key_count; ++i) {
> +               cp->keys[i].type = blocked_keys[i].type;
> +               memcpy(cp->keys[i].val, blocked_keys[i].val, sizeof(cp->keys[i].val));
> +       }
> +
> +       return mgmt_send(mgmt_master, MGMT_OP_SET_BLOCKED_KEYS, adapter->dev_id,
> +                                       sizeof(buffer), buffer, set_blocked_keys_complete,
> +                                       adapter, NULL);
> +}
> +
>  static void read_info_complete(uint8_t status, uint16_t length,
>                                         const void *param, void *user_data)
>  {
> @@ -8795,6 +8846,12 @@ static void read_info_complete(uint8_t status, uint16_t length,
>
>         set_name(adapter, btd_adapter_get_name(adapter));
>
> +       if (kernel_blocked_keys_supported && !set_blocked_keys(adapter)) {
> +               btd_error(adapter->dev_id,
> +                       "Failed to set blocked keys for index %u", adapter->dev_id);
> +               goto failed;
> +       }
> +
>         if (main_opts.pairable &&
>                         !(adapter->current_settings & MGMT_SETTING_BONDABLE))
>                 set_mode(adapter, MGMT_OP_SET_BONDABLE, 0x01);
> @@ -8972,9 +9029,17 @@ static void read_commands_complete(uint8_t status, uint16_t length,
>         for (i = 0; i < num_commands; i++) {
>                 uint16_t op = get_le16(rp->opcodes + i);
>
> -               if (op == MGMT_OP_ADD_DEVICE) {
> +               switch (op) {
> +               case MGMT_OP_ADD_DEVICE:
>                         DBG("enabling kernel-side connection control");
>                         kernel_conn_control = true;
> +                       break;
> +               case MGMT_OP_SET_BLOCKED_KEYS:
> +                       DBG("kernel supports the set_blocked_keys op");
> +                       kernel_blocked_keys_supported = true;
> +                       break;
> +               default:
> +                       break;
>                 }
>         }
>  }
> --
> 2.24.0.525.g8f36a354ae-goog

Other than that it looks pretty good, Im just missing the changes to
the documentation that would be great to have in the same set.

-- 
Luiz Augusto von Dentz