Hi Alain, On Tue, Dec 10, 2019 at 8:00 PM Alain Michaud <alainm@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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
