[PATCH v5 1/3] MGMT_OP_SET_BLOCKED_KEYS Api definitions.

[PATCH v5 1/3] MGMT_OP_SET_BLOCKED_KEYS Api definitions.

[Alain Michaud]

Adding the required definitions for the MGMT_OP_SET_BLOCKED_KEYS Api.
---

 lib/mgmt.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/lib/mgmt.h b/lib/mgmt.h
index 570dec997..276445d0a 100644
--- a/lib/mgmt.h
+++ b/lib/mgmt.h
@@ -583,6 +583,21 @@ 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_EV_CMD_COMPLETE		0x0001
 struct mgmt_ev_cmd_complete {
-- 
2.24.1.735.g03f4e72817-goog

[PATCH v5 2/3] Adding a shared ARRAY_SIZE macro.

[Alain Michaud]

This will allow other implementations within src/ to use a single
definition of the ARRAY_SIZE macro.
---

 src/shared/util.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/shared/util.h b/src/shared/util.h
index 604dc3be4..9193068d1 100644
--- a/src/shared/util.h
+++ b/src/shared/util.h
@@ -27,6 +27,8 @@
 #include <byteswap.h>
 #include <string.h>
 
+#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
+
 #if __BYTE_ORDER == __LITTLE_ENDIAN
 #define le16_to_cpu(val) (val)
 #define le32_to_cpu(val) (val)
-- 
2.24.1.735.g03f4e72817-goog

[PATCH v5 3/3] Loading keys that should be blocked by the kernel.

[Alain Michaud]

This change accomplishes 2 things:
 1. Drop device security data from previously paired devices
 using blocked keys.
 2. Send the list of known bad keys that should be blocked to the kernel
 if supported.

In particular keys from the Google Titan Security key are being
blocked.

For additional background information, please see
https://security.googleblog.com/2019/05/titan-keys-update.html
---

 src/adapter.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 134 insertions(+), 6 deletions(-)

diff --git a/src/adapter.c b/src/adapter.c
index cef25616f..9c41ebe86 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;
+
 static GList *adapter_list = NULL;
 static unsigned int adapter_remaining = 0;
 static bool powering_down = false;
@@ -124,6 +141,7 @@ struct link_key_info {
 	unsigned char key[16];
 	uint8_t type;
 	uint8_t pin_len;
+	bool is_blocked;
 };
 
 struct smp_ltk_info {
@@ -135,12 +153,14 @@ struct smp_ltk_info {
 	uint16_t ediv;
 	uint64_t rand;
 	uint8_t val[16];
+	bool is_blocked;
 };
 
 struct irk_info {
 	bdaddr_t bdaddr;
 	uint8_t bdaddr_type;
 	uint8_t val[16];
+	bool is_blocked;
 };
 
 struct conn_param {
@@ -3439,6 +3459,20 @@ static int str2buf(const char *str, uint8_t *buf, size_t blen)
 	return 0;
 }
 
+static bool is_blocked_key(uint8_t key_type, uint8_t *key_value)
+{
+	uint32_t i = 0;
+
+	for (i = 0; i < ARRAY_SIZE(blocked_keys); ++i) {
+		if (key_type == blocked_keys[i].type &&
+				!memcmp(blocked_keys[i].val, key_value,
+						sizeof(blocked_keys[i].val)))
+			return true;
+	}
+
+	return false;
+}
+
 static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
 {
 	struct link_key_info *info = NULL;
@@ -3461,6 +3495,9 @@ static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
 	info->pin_len = g_key_file_get_integer(key_file, "LinkKey", "PINLength",
 						NULL);
 
+	info->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LINKKEY,
+								info->key);
+
 failed:
 	g_free(str);
 
@@ -3534,6 +3571,9 @@ static struct smp_ltk_info *get_ltk(GKeyFile *key_file, const char *peer,
 	else
 		ltk->master = master;
 
+	ltk->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LTK,
+								ltk->val);
+
 failed:
 	g_free(key);
 	g_free(rand);
@@ -3584,6 +3624,9 @@ static struct irk_info *get_irk_info(GKeyFile *key_file, const char *peer,
 	else
 		str2buf(&str[0], irk->val, sizeof(irk->val));
 
+	irk->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LINKKEY,
+								irk->val);
+
 failed:
 	g_free(str);
 
@@ -4142,21 +4185,55 @@ static void load_devices(struct btd_adapter *adapter)
 		g_key_file_load_from_file(key_file, filename, 0, NULL);
 
 		key_info = get_key_info(key_file, entry->d_name);
-		if (key_info)
-			keys = g_slist_append(keys, key_info);
 
 		bdaddr_type = get_le_addr_type(key_file);
 
 		ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
-		if (ltk_info)
-			ltks = g_slist_append(ltks, ltk_info);
 
 		slave_ltk_info = get_slave_ltk_info(key_file, entry->d_name,
 								bdaddr_type);
+
+		irk_info = get_irk_info(key_file, entry->d_name, bdaddr_type);
+
+		// If any key for the device is blocked, we discard all.
+		if ((key_info && key_info->is_blocked) ||
+				(ltk_info && ltk_info->is_blocked) ||
+				(slave_ltk_info &&
+					slave_ltk_info->is_blocked) ||
+				(irk_info && irk_info->is_blocked)) {
+
+			if (key_info) {
+				g_free(key_info);
+				key_info = NULL;
+			}
+
+			if (ltk_info) {
+				g_free(ltk_info);
+				ltk_info = NULL;
+			}
+
+			if (slave_ltk_info) {
+				g_free(slave_ltk_info);
+				slave_ltk_info = NULL;
+			}
+
+			if (irk_info) {
+				g_free(irk_info);
+				irk_info = NULL;
+			}
+
+			goto free;
+		}
+
+		if (key_info)
+			keys = g_slist_append(keys, key_info);
+
+		if (ltk_info)
+			ltks = g_slist_append(ltks, ltk_info);
+
 		if (slave_ltk_info)
 			ltks = g_slist_append(ltks, slave_ltk_info);
 
-		irk_info = get_irk_info(key_file, entry->d_name, bdaddr_type);
 		if (irk_info)
 			irks = g_slist_append(irks, irk_info);
 
@@ -8568,6 +8645,42 @@ 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 = ARRAY_SIZE(blocked_keys);
+	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 +8908,13 @@ 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 +9092,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.1.735.g03f4e72817-goog

Re: [PATCH v5 3/3] Loading keys that should be blocked by the kernel.

[Luiz Augusto von Dentz]

Hi Alain,

On Mon, Jan 6, 2020 at 5:31 PM Alain Michaud <alainm@chromium.org> wrote:
>
> This change accomplishes 2 things:
>  1. Drop device security data from previously paired devices
>  using blocked keys.
>  2. Send the list of known bad keys that should be blocked to the kernel
>  if supported.
>
> In particular keys from the Google Titan Security key are being
> blocked.
>
> For additional background information, please see
> https://security.googleblog.com/2019/05/titan-keys-update.html
> ---
>
>  src/adapter.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 134 insertions(+), 6 deletions(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index cef25616f..9c41ebe86 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;
> +
>  static GList *adapter_list = NULL;
>  static unsigned int adapter_remaining = 0;
>  static bool powering_down = false;
> @@ -124,6 +141,7 @@ struct link_key_info {
>         unsigned char key[16];
>         uint8_t type;
>         uint8_t pin_len;
> +       bool is_blocked;
>  };
>
>  struct smp_ltk_info {
> @@ -135,12 +153,14 @@ struct smp_ltk_info {
>         uint16_t ediv;
>         uint64_t rand;
>         uint8_t val[16];
> +       bool is_blocked;
>  };
>
>  struct irk_info {
>         bdaddr_t bdaddr;
>         uint8_t bdaddr_type;
>         uint8_t val[16];
> +       bool is_blocked;
>  };
>
>  struct conn_param {
> @@ -3439,6 +3459,20 @@ static int str2buf(const char *str, uint8_t *buf, size_t blen)
>         return 0;
>  }
>
> +static bool is_blocked_key(uint8_t key_type, uint8_t *key_value)
> +{
> +       uint32_t i = 0;
> +
> +       for (i = 0; i < ARRAY_SIZE(blocked_keys); ++i) {
> +               if (key_type == blocked_keys[i].type &&
> +                               !memcmp(blocked_keys[i].val, key_value,
> +                                               sizeof(blocked_keys[i].val)))
> +                       return true;
> +       }
> +
> +       return false;
> +}
> +
>  static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
>  {
>         struct link_key_info *info = NULL;
> @@ -3461,6 +3495,9 @@ static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
>         info->pin_len = g_key_file_get_integer(key_file, "LinkKey", "PINLength",
>                                                 NULL);
>
> +       info->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LINKKEY,
> +                                                               info->key);
> +
>  failed:
>         g_free(str);
>
> @@ -3534,6 +3571,9 @@ static struct smp_ltk_info *get_ltk(GKeyFile *key_file, const char *peer,
>         else
>                 ltk->master = master;
>
> +       ltk->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LTK,
> +                                                               ltk->val);
> +
>  failed:
>         g_free(key);
>         g_free(rand);
> @@ -3584,6 +3624,9 @@ static struct irk_info *get_irk_info(GKeyFile *key_file, const char *peer,
>         else
>                 str2buf(&str[0], irk->val, sizeof(irk->val));
>
> +       irk->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LINKKEY,
> +                                                               irk->val);
> +
>  failed:
>         g_free(str);
>
> @@ -4142,21 +4185,55 @@ static void load_devices(struct btd_adapter *adapter)
>                 g_key_file_load_from_file(key_file, filename, 0, NULL);
>
>                 key_info = get_key_info(key_file, entry->d_name);
> -               if (key_info)
> -                       keys = g_slist_append(keys, key_info);
>
>                 bdaddr_type = get_le_addr_type(key_file);
>
>                 ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
> -               if (ltk_info)
> -                       ltks = g_slist_append(ltks, ltk_info);
>
>                 slave_ltk_info = get_slave_ltk_info(key_file, entry->d_name,
>                                                                 bdaddr_type);
> +
> +               irk_info = get_irk_info(key_file, entry->d_name, bdaddr_type);
> +
> +               // If any key for the device is blocked, we discard all.
> +               if ((key_info && key_info->is_blocked) ||
> +                               (ltk_info && ltk_info->is_blocked) ||
> +                               (slave_ltk_info &&
> +                                       slave_ltk_info->is_blocked) ||
> +                               (irk_info && irk_info->is_blocked)) {

Perhaps it would be more efficient if we don't add is_blocked as a
member of these keys and instead pass bool variable to get_*_info
which just set it in case the key is blocked, that way if is_blocked
is set for get_key_info you don't have to free anything. Also it might
be a good idea to move the handling of loading the keys to another
function i.e. load_keys to make it simpler to bail out, etc. Have you
though about the possibility of just using a the device block API when
you detect it is using compromised keys? That way the application can
detect the device is in fact blocked and can either remove the device,
which would clean up the storage as well (see my comments bellow), or
unblock it in order to continue using the device.

> +                       if (key_info) {
> +                               g_free(key_info);
> +                               key_info = NULL;
> +                       }
> +
> +                       if (ltk_info) {
> +                               g_free(ltk_info);
> +                               ltk_info = NULL;
> +                       }
> +
> +                       if (slave_ltk_info) {
> +                               g_free(slave_ltk_info);
> +                               slave_ltk_info = NULL;
> +                       }
> +
> +                       if (irk_info) {
> +                               g_free(irk_info);
> +                               irk_info = NULL;
> +                       }
> +
> +                       goto free;
> +               }
> +
> +               if (key_info)
> +                       keys = g_slist_append(keys, key_info);
> +
> +               if (ltk_info)
> +                       ltks = g_slist_append(ltks, ltk_info);
> +

Perhaps I missing something but don't we need to clear these keys form
the storage once we figure they are blocked or there is any reason to
leave them in there so we can unblock?

>                 if (slave_ltk_info)
>                         ltks = g_slist_append(ltks, slave_ltk_info);
>
> -               irk_info = get_irk_info(key_file, entry->d_name, bdaddr_type);
>                 if (irk_info)
>                         irks = g_slist_append(irks, irk_info);
>
> @@ -8568,6 +8645,42 @@ 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 = ARRAY_SIZE(blocked_keys);
> +       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 +8908,13 @@ 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 +9092,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.1.735.g03f4e72817-goog
>


-- 
Luiz Augusto von Dentz

Re: [PATCH v5 3/3] Loading keys that should be blocked by the kernel.

[Alain Michaud]

Hi Luiz,


On Tue, Jan 7, 2020 at 6:22 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Alain,
>
> On Mon, Jan 6, 2020 at 5:31 PM Alain Michaud <alainm@chromium.org> wrote:
> >
> > This change accomplishes 2 things:
> >  1. Drop device security data from previously paired devices
> >  using blocked keys.
> >  2. Send the list of known bad keys that should be blocked to the kernel
> >  if supported.
> >
> > In particular keys from the Google Titan Security key are being
> > blocked.
> >
> > For additional background information, please see
> > https://security.googleblog.com/2019/05/titan-keys-update.html
> > ---
> >
> >  src/adapter.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 134 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/adapter.c b/src/adapter.c
> > index cef25616f..9c41ebe86 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;
> > +
> >  static GList *adapter_list = NULL;
> >  static unsigned int adapter_remaining = 0;
> >  static bool powering_down = false;
> > @@ -124,6 +141,7 @@ struct link_key_info {
> >         unsigned char key[16];
> >         uint8_t type;
> >         uint8_t pin_len;
> > +       bool is_blocked;
> >  };
> >
> >  struct smp_ltk_info {
> > @@ -135,12 +153,14 @@ struct smp_ltk_info {
> >         uint16_t ediv;
> >         uint64_t rand;
> >         uint8_t val[16];
> > +       bool is_blocked;
> >  };
> >
> >  struct irk_info {
> >         bdaddr_t bdaddr;
> >         uint8_t bdaddr_type;
> >         uint8_t val[16];
> > +       bool is_blocked;
> >  };
> >
> >  struct conn_param {
> > @@ -3439,6 +3459,20 @@ static int str2buf(const char *str, uint8_t *buf, size_t blen)
> >         return 0;
> >  }
> >
> > +static bool is_blocked_key(uint8_t key_type, uint8_t *key_value)
> > +{
> > +       uint32_t i = 0;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(blocked_keys); ++i) {
> > +               if (key_type == blocked_keys[i].type &&
> > +                               !memcmp(blocked_keys[i].val, key_value,
> > +                                               sizeof(blocked_keys[i].val)))
> > +                       return true;
> > +       }
> > +
> > +       return false;
> > +}
> > +
> >  static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> >  {
> >         struct link_key_info *info = NULL;
> > @@ -3461,6 +3495,9 @@ static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> >         info->pin_len = g_key_file_get_integer(key_file, "LinkKey", "PINLength",
> >                                                 NULL);
> >
> > +       info->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LINKKEY,
> > +                                                               info->key);
> > +
> >  failed:
> >         g_free(str);
> >
> > @@ -3534,6 +3571,9 @@ static struct smp_ltk_info *get_ltk(GKeyFile *key_file, const char *peer,
> >         else
> >                 ltk->master = master;
> >
> > +       ltk->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LTK,
> > +                                                               ltk->val);
> > +
> >  failed:
> >         g_free(key);
> >         g_free(rand);
> > @@ -3584,6 +3624,9 @@ static struct irk_info *get_irk_info(GKeyFile *key_file, const char *peer,
> >         else
> >                 str2buf(&str[0], irk->val, sizeof(irk->val));
> >
> > +       irk->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LINKKEY,
> > +                                                               irk->val);
> > +
> >  failed:
> >         g_free(str);
> >
> > @@ -4142,21 +4185,55 @@ static void load_devices(struct btd_adapter *adapter)
> >                 g_key_file_load_from_file(key_file, filename, 0, NULL);
> >
> >                 key_info = get_key_info(key_file, entry->d_name);
> > -               if (key_info)
> > -                       keys = g_slist_append(keys, key_info);
> >
> >                 bdaddr_type = get_le_addr_type(key_file);
> >
> >                 ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
> > -               if (ltk_info)
> > -                       ltks = g_slist_append(ltks, ltk_info);
> >
> >                 slave_ltk_info = get_slave_ltk_info(key_file, entry->d_name,
> >                                                                 bdaddr_type);
> > +
> > +               irk_info = get_irk_info(key_file, entry->d_name, bdaddr_type);
> > +
> > +               // If any key for the device is blocked, we discard all.
> > +               if ((key_info && key_info->is_blocked) ||
> > +                               (ltk_info && ltk_info->is_blocked) ||
> > +                               (slave_ltk_info &&
> > +                                       slave_ltk_info->is_blocked) ||
> > +                               (irk_info && irk_info->is_blocked)) {
>
> Perhaps it would be more efficient if we don't add is_blocked as a
> member of these keys and instead pass bool variable to get_*_info
> which just set it in case the key is blocked, that way if is_blocked
> is set for get_key_info you don't have to free anything. Also it might
> be a good idea to move the handling of loading the keys to another
> function i.e. load_keys to make it simpler to bail out, etc. Have you
> though about the possibility of just using a the device block API when
> you detect it is using compromised keys? That way the application can
> detect the device is in fact blocked and can either remove the device,
> which would clean up the storage as well (see my comments bellow), or
> unblock it in order to continue using the device.

My thoughts was to keep the contract simple.  if the return is not
null then a key exists, it can be blocked or not blocked.  It feels a
bit weird to have a contract return null but a parameter tells you
that it's been blocked.  The extra allocation/free doesn't seem like a
lot of overhead for what is expected to be a rare case (blocked keys).
>
> > +                       if (key_info) {
> > +                               g_free(key_info);
> > +                               key_info = NULL;
> > +                       }
> > +
> > +                       if (ltk_info) {
> > +                               g_free(ltk_info);
> > +                               ltk_info = NULL;
> > +                       }
> > +
> > +                       if (slave_ltk_info) {
> > +                               g_free(slave_ltk_info);
> > +                               slave_ltk_info = NULL;
> > +                       }
> > +
> > +                       if (irk_info) {
> > +                               g_free(irk_info);
> > +                               irk_info = NULL;
> > +                       }
> > +
> > +                       goto free;
> > +               }
> > +
> > +               if (key_info)
> > +                       keys = g_slist_append(keys, key_info);
> > +
> > +               if (ltk_info)
> > +                       ltks = g_slist_append(ltks, ltk_info);
> > +
>
> Perhaps I missing something but don't we need to clear these keys form
> the storage once we figure they are blocked or there is any reason to
> leave them in there so we can unblock?
Since these are expected to be rare cases, I didn't go through the
extent to clear them from storage.  Having them in storage also can
help with diagnosability.

>
> >                 if (slave_ltk_info)
> >                         ltks = g_slist_append(ltks, slave_ltk_info);
> >
> > -               irk_info = get_irk_info(key_file, entry->d_name, bdaddr_type);
> >                 if (irk_info)
> >                         irks = g_slist_append(irks, irk_info);
> >
> > @@ -8568,6 +8645,42 @@ 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 = ARRAY_SIZE(blocked_keys);
> > +       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 +8908,13 @@ 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 +9092,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.1.735.g03f4e72817-goog
> >
>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH v5 3/3] Loading keys that should be blocked by the kernel.

[Luiz Augusto von Dentz]

Hi Alain,

On Wed, Jan 8, 2020 at 7:04 AM Alain Michaud <alainmichaud@google.com> wrote:
>
> Hi Luiz,
>
>
> On Tue, Jan 7, 2020 at 6:22 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Alain,
> >
> > On Mon, Jan 6, 2020 at 5:31 PM Alain Michaud <alainm@chromium.org> wrote:
> > >
> > > This change accomplishes 2 things:
> > >  1. Drop device security data from previously paired devices
> > >  using blocked keys.
> > >  2. Send the list of known bad keys that should be blocked to the kernel
> > >  if supported.
> > >
> > > In particular keys from the Google Titan Security key are being
> > > blocked.
> > >
> > > For additional background information, please see
> > > https://security.googleblog.com/2019/05/titan-keys-update.html
> > > ---
> > >
> > >  src/adapter.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 134 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/src/adapter.c b/src/adapter.c
> > > index cef25616f..9c41ebe86 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;
> > > +
> > >  static GList *adapter_list = NULL;
> > >  static unsigned int adapter_remaining = 0;
> > >  static bool powering_down = false;
> > > @@ -124,6 +141,7 @@ struct link_key_info {
> > >         unsigned char key[16];
> > >         uint8_t type;
> > >         uint8_t pin_len;
> > > +       bool is_blocked;
> > >  };
> > >
> > >  struct smp_ltk_info {
> > > @@ -135,12 +153,14 @@ struct smp_ltk_info {
> > >         uint16_t ediv;
> > >         uint64_t rand;
> > >         uint8_t val[16];
> > > +       bool is_blocked;
> > >  };
> > >
> > >  struct irk_info {
> > >         bdaddr_t bdaddr;
> > >         uint8_t bdaddr_type;
> > >         uint8_t val[16];
> > > +       bool is_blocked;
> > >  };
> > >
> > >  struct conn_param {
> > > @@ -3439,6 +3459,20 @@ static int str2buf(const char *str, uint8_t *buf, size_t blen)
> > >         return 0;
> > >  }
> > >
> > > +static bool is_blocked_key(uint8_t key_type, uint8_t *key_value)
> > > +{
> > > +       uint32_t i = 0;
> > > +
> > > +       for (i = 0; i < ARRAY_SIZE(blocked_keys); ++i) {
> > > +               if (key_type == blocked_keys[i].type &&
> > > +                               !memcmp(blocked_keys[i].val, key_value,
> > > +                                               sizeof(blocked_keys[i].val)))
> > > +                       return true;
> > > +       }
> > > +
> > > +       return false;
> > > +}
> > > +
> > >  static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> > >  {
> > >         struct link_key_info *info = NULL;
> > > @@ -3461,6 +3495,9 @@ static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> > >         info->pin_len = g_key_file_get_integer(key_file, "LinkKey", "PINLength",
> > >                                                 NULL);
> > >
> > > +       info->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LINKKEY,
> > > +                                                               info->key);
> > > +
> > >  failed:
> > >         g_free(str);
> > >
> > > @@ -3534,6 +3571,9 @@ static struct smp_ltk_info *get_ltk(GKeyFile *key_file, const char *peer,
> > >         else
> > >                 ltk->master = master;
> > >
> > > +       ltk->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LTK,
> > > +                                                               ltk->val);
> > > +
> > >  failed:
> > >         g_free(key);
> > >         g_free(rand);
> > > @@ -3584,6 +3624,9 @@ static struct irk_info *get_irk_info(GKeyFile *key_file, const char *peer,
> > >         else
> > >                 str2buf(&str[0], irk->val, sizeof(irk->val));
> > >
> > > +       irk->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LINKKEY,
> > > +                                                               irk->val);
> > > +
> > >  failed:
> > >         g_free(str);
> > >
> > > @@ -4142,21 +4185,55 @@ static void load_devices(struct btd_adapter *adapter)
> > >                 g_key_file_load_from_file(key_file, filename, 0, NULL);
> > >
> > >                 key_info = get_key_info(key_file, entry->d_name);
> > > -               if (key_info)
> > > -                       keys = g_slist_append(keys, key_info);
> > >
> > >                 bdaddr_type = get_le_addr_type(key_file);
> > >
> > >                 ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
> > > -               if (ltk_info)
> > > -                       ltks = g_slist_append(ltks, ltk_info);
> > >
> > >                 slave_ltk_info = get_slave_ltk_info(key_file, entry->d_name,
> > >                                                                 bdaddr_type);
> > > +
> > > +               irk_info = get_irk_info(key_file, entry->d_name, bdaddr_type);
> > > +
> > > +               // If any key for the device is blocked, we discard all.
> > > +               if ((key_info && key_info->is_blocked) ||
> > > +                               (ltk_info && ltk_info->is_blocked) ||
> > > +                               (slave_ltk_info &&
> > > +                                       slave_ltk_info->is_blocked) ||
> > > +                               (irk_info && irk_info->is_blocked)) {
> >
> > Perhaps it would be more efficient if we don't add is_blocked as a
> > member of these keys and instead pass bool variable to get_*_info
> > which just set it in case the key is blocked, that way if is_blocked
> > is set for get_key_info you don't have to free anything. Also it might
> > be a good idea to move the handling of loading the keys to another
> > function i.e. load_keys to make it simpler to bail out, etc. Have you
> > though about the possibility of just using a the device block API when
> > you detect it is using compromised keys? That way the application can
> > detect the device is in fact blocked and can either remove the device,
> > which would clean up the storage as well (see my comments bellow), or
> > unblock it in order to continue using the device.
>
> My thoughts was to keep the contract simple.  if the return is not
> null then a key exists, it can be blocked or not blocked.  It feels a
> bit weird to have a contract return null but a parameter tells you
> that it's been blocked.  The extra allocation/free doesn't seem like a
> lot of overhead for what is expected to be a rare case (blocked keys).

Fair enough, I suggested that because the is_block logic seems to
apply to all of the keys not just the key that was compromised, anyway
Im probably getting a little too paranoid regarding overhead but I
don't think we had loose anything by avoiding the extra
allocation/free

> >
> > > +                       if (key_info) {
> > > +                               g_free(key_info);
> > > +                               key_info = NULL;
> > > +                       }
> > > +
> > > +                       if (ltk_info) {
> > > +                               g_free(ltk_info);
> > > +                               ltk_info = NULL;
> > > +                       }
> > > +
> > > +                       if (slave_ltk_info) {
> > > +                               g_free(slave_ltk_info);
> > > +                               slave_ltk_info = NULL;
> > > +                       }
> > > +
> > > +                       if (irk_info) {
> > > +                               g_free(irk_info);
> > > +                               irk_info = NULL;
> > > +                       }
> > > +
> > > +                       goto free;
> > > +               }
> > > +
> > > +               if (key_info)
> > > +                       keys = g_slist_append(keys, key_info);
> > > +
> > > +               if (ltk_info)
> > > +                       ltks = g_slist_append(ltks, ltk_info);
> > > +
> >
> > Perhaps I missing something but don't we need to clear these keys form
> > the storage once we figure they are blocked or there is any reason to
> > leave them in there so we can unblock?
> Since these are expected to be rare cases, I didn't go through the
> extent to clear them from storage.  Having them in storage also can
> help with diagnosability.

That is an interesting point, but we don't allow new pairing with
compromised keys with these changes or do we? If we want to allow
diagnosing in all cases we could actually store the blocked flag into
the storage so something like a diagnostic tool can fetch the
information directly.

> >
> > >                 if (slave_ltk_info)
> > >                         ltks = g_slist_append(ltks, slave_ltk_info);
> > >
> > > -               irk_info = get_irk_info(key_file, entry->d_name, bdaddr_type);
> > >                 if (irk_info)
> > >                         irks = g_slist_append(irks, irk_info);
> > >
> > > @@ -8568,6 +8645,42 @@ 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 = ARRAY_SIZE(blocked_keys);
> > > +       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 +8908,13 @@ 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 +9092,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.1.735.g03f4e72817-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

Re: [PATCH v5 3/3] Loading keys that should be blocked by the kernel.

[Alain Michaud]

Hi Luiz,


On Wed, Jan 8, 2020 at 3:00 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Alain,
>
> On Wed, Jan 8, 2020 at 7:04 AM Alain Michaud <alainmichaud@google.com> wrote:
> >
> > Hi Luiz,
> >
> >
> > On Tue, Jan 7, 2020 at 6:22 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Alain,
> > >
> > > On Mon, Jan 6, 2020 at 5:31 PM Alain Michaud <alainm@chromium.org> wrote:
> > > >
> > > > This change accomplishes 2 things:
> > > >  1. Drop device security data from previously paired devices
> > > >  using blocked keys.
> > > >  2. Send the list of known bad keys that should be blocked to the kernel
> > > >  if supported.
> > > >
> > > > In particular keys from the Google Titan Security key are being
> > > > blocked.
> > > >
> > > > For additional background information, please see
> > > > https://security.googleblog.com/2019/05/titan-keys-update.html
> > > > ---
> > > >
> > > >  src/adapter.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 134 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/src/adapter.c b/src/adapter.c
> > > > index cef25616f..9c41ebe86 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;
> > > > +
> > > >  static GList *adapter_list = NULL;
> > > >  static unsigned int adapter_remaining = 0;
> > > >  static bool powering_down = false;
> > > > @@ -124,6 +141,7 @@ struct link_key_info {
> > > >         unsigned char key[16];
> > > >         uint8_t type;
> > > >         uint8_t pin_len;
> > > > +       bool is_blocked;
> > > >  };
> > > >
> > > >  struct smp_ltk_info {
> > > > @@ -135,12 +153,14 @@ struct smp_ltk_info {
> > > >         uint16_t ediv;
> > > >         uint64_t rand;
> > > >         uint8_t val[16];
> > > > +       bool is_blocked;
> > > >  };
> > > >
> > > >  struct irk_info {
> > > >         bdaddr_t bdaddr;
> > > >         uint8_t bdaddr_type;
> > > >         uint8_t val[16];
> > > > +       bool is_blocked;
> > > >  };
> > > >
> > > >  struct conn_param {
> > > > @@ -3439,6 +3459,20 @@ static int str2buf(const char *str, uint8_t *buf, size_t blen)
> > > >         return 0;
> > > >  }
> > > >
> > > > +static bool is_blocked_key(uint8_t key_type, uint8_t *key_value)
> > > > +{
> > > > +       uint32_t i = 0;
> > > > +
> > > > +       for (i = 0; i < ARRAY_SIZE(blocked_keys); ++i) {
> > > > +               if (key_type == blocked_keys[i].type &&
> > > > +                               !memcmp(blocked_keys[i].val, key_value,
> > > > +                                               sizeof(blocked_keys[i].val)))
> > > > +                       return true;
> > > > +       }
> > > > +
> > > > +       return false;
> > > > +}
> > > > +
> > > >  static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> > > >  {
> > > >         struct link_key_info *info = NULL;
> > > > @@ -3461,6 +3495,9 @@ static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> > > >         info->pin_len = g_key_file_get_integer(key_file, "LinkKey", "PINLength",
> > > >                                                 NULL);
> > > >
> > > > +       info->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LINKKEY,
> > > > +                                                               info->key);
> > > > +
> > > >  failed:
> > > >         g_free(str);
> > > >
> > > > @@ -3534,6 +3571,9 @@ static struct smp_ltk_info *get_ltk(GKeyFile *key_file, const char *peer,
> > > >         else
> > > >                 ltk->master = master;
> > > >
> > > > +       ltk->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LTK,
> > > > +                                                               ltk->val);
> > > > +
> > > >  failed:
> > > >         g_free(key);
> > > >         g_free(rand);
> > > > @@ -3584,6 +3624,9 @@ static struct irk_info *get_irk_info(GKeyFile *key_file, const char *peer,
> > > >         else
> > > >                 str2buf(&str[0], irk->val, sizeof(irk->val));
> > > >
> > > > +       irk->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LINKKEY,
> > > > +                                                               irk->val);
> > > > +
> > > >  failed:
> > > >         g_free(str);
> > > >
> > > > @@ -4142,21 +4185,55 @@ static void load_devices(struct btd_adapter *adapter)
> > > >                 g_key_file_load_from_file(key_file, filename, 0, NULL);
> > > >
> > > >                 key_info = get_key_info(key_file, entry->d_name);
> > > > -               if (key_info)
> > > > -                       keys = g_slist_append(keys, key_info);
> > > >
> > > >                 bdaddr_type = get_le_addr_type(key_file);
> > > >
> > > >                 ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
> > > > -               if (ltk_info)
> > > > -                       ltks = g_slist_append(ltks, ltk_info);
> > > >
> > > >                 slave_ltk_info = get_slave_ltk_info(key_file, entry->d_name,
> > > >                                                                 bdaddr_type);
> > > > +
> > > > +               irk_info = get_irk_info(key_file, entry->d_name, bdaddr_type);
> > > > +
> > > > +               // If any key for the device is blocked, we discard all.
> > > > +               if ((key_info && key_info->is_blocked) ||
> > > > +                               (ltk_info && ltk_info->is_blocked) ||
> > > > +                               (slave_ltk_info &&
> > > > +                                       slave_ltk_info->is_blocked) ||
> > > > +                               (irk_info && irk_info->is_blocked)) {
> > >
> > > Perhaps it would be more efficient if we don't add is_blocked as a
> > > member of these keys and instead pass bool variable to get_*_info
> > > which just set it in case the key is blocked, that way if is_blocked
> > > is set for get_key_info you don't have to free anything. Also it might
> > > be a good idea to move the handling of loading the keys to another
> > > function i.e. load_keys to make it simpler to bail out, etc. Have you
> > > though about the possibility of just using a the device block API when
> > > you detect it is using compromised keys? That way the application can
> > > detect the device is in fact blocked and can either remove the device,
> > > which would clean up the storage as well (see my comments bellow), or
> > > unblock it in order to continue using the device.
> >
> > My thoughts was to keep the contract simple.  if the return is not
> > null then a key exists, it can be blocked or not blocked.  It feels a
> > bit weird to have a contract return null but a parameter tells you
> > that it's been blocked.  The extra allocation/free doesn't seem like a
> > lot of overhead for what is expected to be a rare case (blocked keys).
>
> Fair enough, I suggested that because the is_block logic seems to
> apply to all of the keys not just the key that was compromised, anyway
> Im probably getting a little too paranoid regarding overhead but I
> don't think we had loose anything by avoiding the extra
> allocation/free
Correct.  The idea is that if any of the keys from the device is
blocked, the device
is simply ignored.

>
> > >
> > > > +                       if (key_info) {
> > > > +                               g_free(key_info);
> > > > +                               key_info = NULL;
> > > > +                       }
> > > > +
> > > > +                       if (ltk_info) {
> > > > +                               g_free(ltk_info);
> > > > +                               ltk_info = NULL;
> > > > +                       }
> > > > +
> > > > +                       if (slave_ltk_info) {
> > > > +                               g_free(slave_ltk_info);
> > > > +                               slave_ltk_info = NULL;
> > > > +                       }
> > > > +
> > > > +                       if (irk_info) {
> > > > +                               g_free(irk_info);
> > > > +                               irk_info = NULL;
> > > > +                       }
> > > > +
> > > > +                       goto free;
> > > > +               }
> > > > +
> > > > +               if (key_info)
> > > > +                       keys = g_slist_append(keys, key_info);
> > > > +
> > > > +               if (ltk_info)
> > > > +                       ltks = g_slist_append(ltks, ltk_info);
> > > > +
> > >
> > > Perhaps I missing something but don't we need to clear these keys form
> > > the storage once we figure they are blocked or there is any reason to
> > > leave them in there so we can unblock?
> > Since these are expected to be rare cases, I didn't go through the
> > extent to clear them from storage.  Having them in storage also can
> > help with diagnosability.
>
> That is an interesting point, but we don't allow new pairing with
> compromised keys with these changes or do we? If we want to allow
> diagnosing in all cases we could actually store the blocked flag into
> the storage so something like a diagnostic tool can fetch the
> information directly.
With these changes, the device is simply dropped from the list
entirely.  The user is free to attempt to pair it again.  If the
device is fixed, pairing will work and the key will be overwritten, if
the device is still broken, the pairing will fail and the user will
see the error.  Given that for now, the keys are hardcoded and a
relatively small list, it feels like overkill to store an additional
flag for it.  This may be something we'd want to do if the list of
blocked keys grows beyond something that is easily manageable.  If
that is the case, I also think we should read the blocked keys from a
configuration file rather than hardcoded in the code as it currently
is.

>
> > >
> > > >                 if (slave_ltk_info)
> > > >                         ltks = g_slist_append(ltks, slave_ltk_info);
> > > >
> > > > -               irk_info = get_irk_info(key_file, entry->d_name, bdaddr_type);
> > > >                 if (irk_info)
> > > >                         irks = g_slist_append(irks, irk_info);
> > > >
> > > > @@ -8568,6 +8645,42 @@ 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 = ARRAY_SIZE(blocked_keys);
> > > > +       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 +8908,13 @@ 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 +9092,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.1.735.g03f4e72817-goog
> > > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH v5 3/3] Loading keys that should be blocked by the kernel.

[Luiz Augusto von Dentz]

Hi Alain, Marcel,

On Wed, Jan 8, 2020 at 12:08 PM Alain Michaud <alainmichaud@google.com> wrote:
>
> Hi Luiz,
>
>
> On Wed, Jan 8, 2020 at 3:00 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Alain,
> >
> > On Wed, Jan 8, 2020 at 7:04 AM Alain Michaud <alainmichaud@google.com> wrote:
> > >
> > > Hi Luiz,
> > >
> > >
> > > On Tue, Jan 7, 2020 at 6:22 PM Luiz Augusto von Dentz
> > > <luiz.dentz@gmail.com> wrote:
> > > >
> > > > Hi Alain,
> > > >
> > > > On Mon, Jan 6, 2020 at 5:31 PM Alain Michaud <alainm@chromium.org> wrote:
> > > > >
> > > > > This change accomplishes 2 things:
> > > > >  1. Drop device security data from previously paired devices
> > > > >  using blocked keys.
> > > > >  2. Send the list of known bad keys that should be blocked to the kernel
> > > > >  if supported.
> > > > >
> > > > > In particular keys from the Google Titan Security key are being
> > > > > blocked.
> > > > >
> > > > > For additional background information, please see
> > > > > https://security.googleblog.com/2019/05/titan-keys-update.html
> > > > > ---
> > > > >
> > > > >  src/adapter.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++---
> > > > >  1 file changed, 134 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/src/adapter.c b/src/adapter.c
> > > > > index cef25616f..9c41ebe86 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;
> > > > > +
> > > > >  static GList *adapter_list = NULL;
> > > > >  static unsigned int adapter_remaining = 0;
> > > > >  static bool powering_down = false;
> > > > > @@ -124,6 +141,7 @@ struct link_key_info {
> > > > >         unsigned char key[16];
> > > > >         uint8_t type;
> > > > >         uint8_t pin_len;
> > > > > +       bool is_blocked;
> > > > >  };
> > > > >
> > > > >  struct smp_ltk_info {
> > > > > @@ -135,12 +153,14 @@ struct smp_ltk_info {
> > > > >         uint16_t ediv;
> > > > >         uint64_t rand;
> > > > >         uint8_t val[16];
> > > > > +       bool is_blocked;
> > > > >  };
> > > > >
> > > > >  struct irk_info {
> > > > >         bdaddr_t bdaddr;
> > > > >         uint8_t bdaddr_type;
> > > > >         uint8_t val[16];
> > > > > +       bool is_blocked;
> > > > >  };
> > > > >
> > > > >  struct conn_param {
> > > > > @@ -3439,6 +3459,20 @@ static int str2buf(const char *str, uint8_t *buf, size_t blen)
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > +static bool is_blocked_key(uint8_t key_type, uint8_t *key_value)
> > > > > +{
> > > > > +       uint32_t i = 0;
> > > > > +
> > > > > +       for (i = 0; i < ARRAY_SIZE(blocked_keys); ++i) {
> > > > > +               if (key_type == blocked_keys[i].type &&
> > > > > +                               !memcmp(blocked_keys[i].val, key_value,
> > > > > +                                               sizeof(blocked_keys[i].val)))
> > > > > +                       return true;
> > > > > +       }
> > > > > +
> > > > > +       return false;
> > > > > +}
> > > > > +
> > > > >  static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> > > > >  {
> > > > >         struct link_key_info *info = NULL;
> > > > > @@ -3461,6 +3495,9 @@ static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> > > > >         info->pin_len = g_key_file_get_integer(key_file, "LinkKey", "PINLength",
> > > > >                                                 NULL);
> > > > >
> > > > > +       info->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LINKKEY,
> > > > > +                                                               info->key);
> > > > > +
> > > > >  failed:
> > > > >         g_free(str);
> > > > >
> > > > > @@ -3534,6 +3571,9 @@ static struct smp_ltk_info *get_ltk(GKeyFile *key_file, const char *peer,
> > > > >         else
> > > > >                 ltk->master = master;
> > > > >
> > > > > +       ltk->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LTK,
> > > > > +                                                               ltk->val);
> > > > > +
> > > > >  failed:
> > > > >         g_free(key);
> > > > >         g_free(rand);
> > > > > @@ -3584,6 +3624,9 @@ static struct irk_info *get_irk_info(GKeyFile *key_file, const char *peer,
> > > > >         else
> > > > >                 str2buf(&str[0], irk->val, sizeof(irk->val));
> > > > >
> > > > > +       irk->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LINKKEY,
> > > > > +                                                               irk->val);
> > > > > +
> > > > >  failed:
> > > > >         g_free(str);
> > > > >
> > > > > @@ -4142,21 +4185,55 @@ static void load_devices(struct btd_adapter *adapter)
> > > > >                 g_key_file_load_from_file(key_file, filename, 0, NULL);
> > > > >
> > > > >                 key_info = get_key_info(key_file, entry->d_name);
> > > > > -               if (key_info)
> > > > > -                       keys = g_slist_append(keys, key_info);
> > > > >
> > > > >                 bdaddr_type = get_le_addr_type(key_file);
> > > > >
> > > > >                 ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
> > > > > -               if (ltk_info)
> > > > > -                       ltks = g_slist_append(ltks, ltk_info);
> > > > >
> > > > >                 slave_ltk_info = get_slave_ltk_info(key_file, entry->d_name,
> > > > >                                                                 bdaddr_type);
> > > > > +
> > > > > +               irk_info = get_irk_info(key_file, entry->d_name, bdaddr_type);
> > > > > +
> > > > > +               // If any key for the device is blocked, we discard all.
> > > > > +               if ((key_info && key_info->is_blocked) ||
> > > > > +                               (ltk_info && ltk_info->is_blocked) ||
> > > > > +                               (slave_ltk_info &&
> > > > > +                                       slave_ltk_info->is_blocked) ||
> > > > > +                               (irk_info && irk_info->is_blocked)) {
> > > >
> > > > Perhaps it would be more efficient if we don't add is_blocked as a
> > > > member of these keys and instead pass bool variable to get_*_info
> > > > which just set it in case the key is blocked, that way if is_blocked
> > > > is set for get_key_info you don't have to free anything. Also it might
> > > > be a good idea to move the handling of loading the keys to another
> > > > function i.e. load_keys to make it simpler to bail out, etc. Have you
> > > > though about the possibility of just using a the device block API when
> > > > you detect it is using compromised keys? That way the application can
> > > > detect the device is in fact blocked and can either remove the device,
> > > > which would clean up the storage as well (see my comments bellow), or
> > > > unblock it in order to continue using the device.
> > >
> > > My thoughts was to keep the contract simple.  if the return is not
> > > null then a key exists, it can be blocked or not blocked.  It feels a
> > > bit weird to have a contract return null but a parameter tells you
> > > that it's been blocked.  The extra allocation/free doesn't seem like a
> > > lot of overhead for what is expected to be a rare case (blocked keys).
> >
> > Fair enough, I suggested that because the is_block logic seems to
> > apply to all of the keys not just the key that was compromised, anyway
> > Im probably getting a little too paranoid regarding overhead but I
> > don't think we had loose anything by avoiding the extra
> > allocation/free
> Correct.  The idea is that if any of the keys from the device is
> blocked, the device
> is simply ignored.
>
> >
> > > >
> > > > > +                       if (key_info) {
> > > > > +                               g_free(key_info);
> > > > > +                               key_info = NULL;
> > > > > +                       }
> > > > > +
> > > > > +                       if (ltk_info) {
> > > > > +                               g_free(ltk_info);
> > > > > +                               ltk_info = NULL;
> > > > > +                       }
> > > > > +
> > > > > +                       if (slave_ltk_info) {
> > > > > +                               g_free(slave_ltk_info);
> > > > > +                               slave_ltk_info = NULL;
> > > > > +                       }
> > > > > +
> > > > > +                       if (irk_info) {
> > > > > +                               g_free(irk_info);
> > > > > +                               irk_info = NULL;
> > > > > +                       }
> > > > > +
> > > > > +                       goto free;
> > > > > +               }
> > > > > +
> > > > > +               if (key_info)
> > > > > +                       keys = g_slist_append(keys, key_info);
> > > > > +
> > > > > +               if (ltk_info)
> > > > > +                       ltks = g_slist_append(ltks, ltk_info);
> > > > > +
> > > >
> > > > Perhaps I missing something but don't we need to clear these keys form
> > > > the storage once we figure they are blocked or there is any reason to
> > > > leave them in there so we can unblock?
> > > Since these are expected to be rare cases, I didn't go through the
> > > extent to clear them from storage.  Having them in storage also can
> > > help with diagnosability.
> >
> > That is an interesting point, but we don't allow new pairing with
> > compromised keys with these changes or do we? If we want to allow
> > diagnosing in all cases we could actually store the blocked flag into
> > the storage so something like a diagnostic tool can fetch the
> > information directly.
> With these changes, the device is simply dropped from the list
> entirely.  The user is free to attempt to pair it again.  If the
> device is fixed, pairing will work and the key will be overwritten, if
> the device is still broken, the pairing will fail and the user will
> see the error.  Given that for now, the keys are hardcoded and a
> relatively small list, it feels like overkill to store an additional
> flag for it.  This may be something we'd want to do if the list of
> blocked keys grows beyond something that is easily manageable.  If
> that is the case, I also think we should read the blocked keys from a
> configuration file rather than hardcoded in the code as it currently
> is.

All good, just waiting the kernel changes to be applied then we can go
ahead with these changes as well, @Marcel are you planning to merge
the kernel changes?

> >
> > > >
> > > > >                 if (slave_ltk_info)
> > > > >                         ltks = g_slist_append(ltks, slave_ltk_info);
> > > > >
> > > > > -               irk_info = get_irk_info(key_file, entry->d_name, bdaddr_type);
> > > > >                 if (irk_info)
> > > > >                         irks = g_slist_append(irks, irk_info);
> > > > >
> > > > > @@ -8568,6 +8645,42 @@ 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 = ARRAY_SIZE(blocked_keys);
> > > > > +       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 +8908,13 @@ 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 +9092,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.1.735.g03f4e72817-goog
> > > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

Re: [PATCH v5 3/3] Loading keys that should be blocked by the kernel.

[Alain Michaud]

Looks like Marcel committed the kernel changes, are we good to commit these?

Thanks!
Alain

On Fri, Jan 10, 2020 at 4:08 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Alain, Marcel,
>
> On Wed, Jan 8, 2020 at 12:08 PM Alain Michaud <alainmichaud@google.com> wrote:
> >
> > Hi Luiz,
> >
> >
> > On Wed, Jan 8, 2020 at 3:00 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Alain,
> > >
> > > On Wed, Jan 8, 2020 at 7:04 AM Alain Michaud <alainmichaud@google.com> wrote:
> > > >
> > > > Hi Luiz,
> > > >
> > > >
> > > > On Tue, Jan 7, 2020 at 6:22 PM Luiz Augusto von Dentz
> > > > <luiz.dentz@gmail.com> wrote:
> > > > >
> > > > > Hi Alain,
> > > > >
> > > > > On Mon, Jan 6, 2020 at 5:31 PM Alain Michaud <alainm@chromium.org> wrote:
> > > > > >
> > > > > > This change accomplishes 2 things:
> > > > > >  1. Drop device security data from previously paired devices
> > > > > >  using blocked keys.
> > > > > >  2. Send the list of known bad keys that should be blocked to the kernel
> > > > > >  if supported.
> > > > > >
> > > > > > In particular keys from the Google Titan Security key are being
> > > > > > blocked.
> > > > > >
> > > > > > For additional background information, please see
> > > > > > https://security.googleblog.com/2019/05/titan-keys-update.html
> > > > > > ---
> > > > > >
> > > > > >  src/adapter.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++---
> > > > > >  1 file changed, 134 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/src/adapter.c b/src/adapter.c
> > > > > > index cef25616f..9c41ebe86 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;
> > > > > > +
> > > > > >  static GList *adapter_list = NULL;
> > > > > >  static unsigned int adapter_remaining = 0;
> > > > > >  static bool powering_down = false;
> > > > > > @@ -124,6 +141,7 @@ struct link_key_info {
> > > > > >         unsigned char key[16];
> > > > > >         uint8_t type;
> > > > > >         uint8_t pin_len;
> > > > > > +       bool is_blocked;
> > > > > >  };
> > > > > >
> > > > > >  struct smp_ltk_info {
> > > > > > @@ -135,12 +153,14 @@ struct smp_ltk_info {
> > > > > >         uint16_t ediv;
> > > > > >         uint64_t rand;
> > > > > >         uint8_t val[16];
> > > > > > +       bool is_blocked;
> > > > > >  };
> > > > > >
> > > > > >  struct irk_info {
> > > > > >         bdaddr_t bdaddr;
> > > > > >         uint8_t bdaddr_type;
> > > > > >         uint8_t val[16];
> > > > > > +       bool is_blocked;
> > > > > >  };
> > > > > >
> > > > > >  struct conn_param {
> > > > > > @@ -3439,6 +3459,20 @@ static int str2buf(const char *str, uint8_t *buf, size_t blen)
> > > > > >         return 0;
> > > > > >  }
> > > > > >
> > > > > > +static bool is_blocked_key(uint8_t key_type, uint8_t *key_value)
> > > > > > +{
> > > > > > +       uint32_t i = 0;
> > > > > > +
> > > > > > +       for (i = 0; i < ARRAY_SIZE(blocked_keys); ++i) {
> > > > > > +               if (key_type == blocked_keys[i].type &&
> > > > > > +                               !memcmp(blocked_keys[i].val, key_value,
> > > > > > +                                               sizeof(blocked_keys[i].val)))
> > > > > > +                       return true;
> > > > > > +       }
> > > > > > +
> > > > > > +       return false;
> > > > > > +}
> > > > > > +
> > > > > >  static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> > > > > >  {
> > > > > >         struct link_key_info *info = NULL;
> > > > > > @@ -3461,6 +3495,9 @@ static struct link_key_info *get_key_info(GKeyFile *key_file, const char *peer)
> > > > > >         info->pin_len = g_key_file_get_integer(key_file, "LinkKey", "PINLength",
> > > > > >                                                 NULL);
> > > > > >
> > > > > > +       info->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LINKKEY,
> > > > > > +                                                               info->key);
> > > > > > +
> > > > > >  failed:
> > > > > >         g_free(str);
> > > > > >
> > > > > > @@ -3534,6 +3571,9 @@ static struct smp_ltk_info *get_ltk(GKeyFile *key_file, const char *peer,
> > > > > >         else
> > > > > >                 ltk->master = master;
> > > > > >
> > > > > > +       ltk->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LTK,
> > > > > > +                                                               ltk->val);
> > > > > > +
> > > > > >  failed:
> > > > > >         g_free(key);
> > > > > >         g_free(rand);
> > > > > > @@ -3584,6 +3624,9 @@ static struct irk_info *get_irk_info(GKeyFile *key_file, const char *peer,
> > > > > >         else
> > > > > >                 str2buf(&str[0], irk->val, sizeof(irk->val));
> > > > > >
> > > > > > +       irk->is_blocked = is_blocked_key(HCI_BLOCKED_KEY_TYPE_LINKKEY,
> > > > > > +                                                               irk->val);
> > > > > > +
> > > > > >  failed:
> > > > > >         g_free(str);
> > > > > >
> > > > > > @@ -4142,21 +4185,55 @@ static void load_devices(struct btd_adapter *adapter)
> > > > > >                 g_key_file_load_from_file(key_file, filename, 0, NULL);
> > > > > >
> > > > > >                 key_info = get_key_info(key_file, entry->d_name);
> > > > > > -               if (key_info)
> > > > > > -                       keys = g_slist_append(keys, key_info);
> > > > > >
> > > > > >                 bdaddr_type = get_le_addr_type(key_file);
> > > > > >
> > > > > >                 ltk_info = get_ltk_info(key_file, entry->d_name, bdaddr_type);
> > > > > > -               if (ltk_info)
> > > > > > -                       ltks = g_slist_append(ltks, ltk_info);
> > > > > >
> > > > > >                 slave_ltk_info = get_slave_ltk_info(key_file, entry->d_name,
> > > > > >                                                                 bdaddr_type);
> > > > > > +
> > > > > > +               irk_info = get_irk_info(key_file, entry->d_name, bdaddr_type);
> > > > > > +
> > > > > > +               // If any key for the device is blocked, we discard all.
> > > > > > +               if ((key_info && key_info->is_blocked) ||
> > > > > > +                               (ltk_info && ltk_info->is_blocked) ||
> > > > > > +                               (slave_ltk_info &&
> > > > > > +                                       slave_ltk_info->is_blocked) ||
> > > > > > +                               (irk_info && irk_info->is_blocked)) {
> > > > >
> > > > > Perhaps it would be more efficient if we don't add is_blocked as a
> > > > > member of these keys and instead pass bool variable to get_*_info
> > > > > which just set it in case the key is blocked, that way if is_blocked
> > > > > is set for get_key_info you don't have to free anything. Also it might
> > > > > be a good idea to move the handling of loading the keys to another
> > > > > function i.e. load_keys to make it simpler to bail out, etc. Have you
> > > > > though about the possibility of just using a the device block API when
> > > > > you detect it is using compromised keys? That way the application can
> > > > > detect the device is in fact blocked and can either remove the device,
> > > > > which would clean up the storage as well (see my comments bellow), or
> > > > > unblock it in order to continue using the device.
> > > >
> > > > My thoughts was to keep the contract simple.  if the return is not
> > > > null then a key exists, it can be blocked or not blocked.  It feels a
> > > > bit weird to have a contract return null but a parameter tells you
> > > > that it's been blocked.  The extra allocation/free doesn't seem like a
> > > > lot of overhead for what is expected to be a rare case (blocked keys).
> > >
> > > Fair enough, I suggested that because the is_block logic seems to
> > > apply to all of the keys not just the key that was compromised, anyway
> > > Im probably getting a little too paranoid regarding overhead but I
> > > don't think we had loose anything by avoiding the extra
> > > allocation/free
> > Correct.  The idea is that if any of the keys from the device is
> > blocked, the device
> > is simply ignored.
> >
> > >
> > > > >
> > > > > > +                       if (key_info) {
> > > > > > +                               g_free(key_info);
> > > > > > +                               key_info = NULL;
> > > > > > +                       }
> > > > > > +
> > > > > > +                       if (ltk_info) {
> > > > > > +                               g_free(ltk_info);
> > > > > > +                               ltk_info = NULL;
> > > > > > +                       }
> > > > > > +
> > > > > > +                       if (slave_ltk_info) {
> > > > > > +                               g_free(slave_ltk_info);
> > > > > > +                               slave_ltk_info = NULL;
> > > > > > +                       }
> > > > > > +
> > > > > > +                       if (irk_info) {
> > > > > > +                               g_free(irk_info);
> > > > > > +                               irk_info = NULL;
> > > > > > +                       }
> > > > > > +
> > > > > > +                       goto free;
> > > > > > +               }
> > > > > > +
> > > > > > +               if (key_info)
> > > > > > +                       keys = g_slist_append(keys, key_info);
> > > > > > +
> > > > > > +               if (ltk_info)
> > > > > > +                       ltks = g_slist_append(ltks, ltk_info);
> > > > > > +
> > > > >
> > > > > Perhaps I missing something but don't we need to clear these keys form
> > > > > the storage once we figure they are blocked or there is any reason to
> > > > > leave them in there so we can unblock?
> > > > Since these are expected to be rare cases, I didn't go through the
> > > > extent to clear them from storage.  Having them in storage also can
> > > > help with diagnosability.
> > >
> > > That is an interesting point, but we don't allow new pairing with
> > > compromised keys with these changes or do we? If we want to allow
> > > diagnosing in all cases we could actually store the blocked flag into
> > > the storage so something like a diagnostic tool can fetch the
> > > information directly.
> > With these changes, the device is simply dropped from the list
> > entirely.  The user is free to attempt to pair it again.  If the
> > device is fixed, pairing will work and the key will be overwritten, if
> > the device is still broken, the pairing will fail and the user will
> > see the error.  Given that for now, the keys are hardcoded and a
> > relatively small list, it feels like overkill to store an additional
> > flag for it.  This may be something we'd want to do if the list of
> > blocked keys grows beyond something that is easily manageable.  If
> > that is the case, I also think we should read the blocked keys from a
> > configuration file rather than hardcoded in the code as it currently
> > is.
>
> All good, just waiting the kernel changes to be applied then we can go
> ahead with these changes as well, @Marcel are you planning to merge
> the kernel changes?
>
> > >
> > > > >
> > > > > >                 if (slave_ltk_info)
> > > > > >                         ltks = g_slist_append(ltks, slave_ltk_info);
> > > > > >
> > > > > > -               irk_info = get_irk_info(key_file, entry->d_name, bdaddr_type);
> > > > > >                 if (irk_info)
> > > > > >                         irks = g_slist_append(irks, irk_info);
> > > > > >
> > > > > > @@ -8568,6 +8645,42 @@ 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 = ARRAY_SIZE(blocked_keys);
> > > > > > +       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 +8908,13 @@ 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 +9092,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.1.735.g03f4e72817-goog
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Luiz Augusto von Dentz
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

Re: [PATCH v5 3/3] Loading keys that should be blocked by the kernel.

[Luiz Von Dentz]

Hi Alain,

On Thu, Jan 16, 2020 at 8:18 AM, Alain Michaud 
<alainmichaud@google.com> wrote:
> Looks like Marcel committed the kernel changes, are we good to commit 
> these?
> 
> Thanks!
> Alain
> 
> On Fri, Jan 10, 2020 at 4:08 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> 
>>  Hi Alain, Marcel,
>> 
>>  On Wed, Jan 8, 2020 at 12:08 PM Alain Michaud 
>> <alainmichaud@google.com> wrote:
>>  >
>>  > Hi Luiz,
>>  >
>>  >
>>  > On Wed, Jan 8, 2020 at 3:00 PM Luiz Augusto von Dentz
>>  > <luiz.dentz@gmail.com> wrote:
>>  > >
>>  > > Hi Alain,
>>  > >
>>  > > On Wed, Jan 8, 2020 at 7:04 AM Alain Michaud 
>> <alainmichaud@google.com> wrote:
>>  > > >
>>  > > > Hi Luiz,
>>  > > >
>>  > > >
>>  > > > On Tue, Jan 7, 2020 at 6:22 PM Luiz Augusto von Dentz
>>  > > > <luiz.dentz@gmail.com> wrote:
>>  > > > >
>>  > > > > Hi Alain,
>>  > > > >
>>  > > > > On Mon, Jan 6, 2020 at 5:31 PM Alain Michaud 
>> <alainm@chromium.org> wrote:
>>  > > > > >
>>  > > > > > This change accomplishes 2 things:
>>  > > > > >  1. Drop device security data from previously paired 
>> devices
>>  > > > > >  using blocked keys.
>>  > > > > >  2. Send the list of known bad keys that should be 
>> blocked to the kernel
>>  > > > > >  if supported.
>>  > > > > >
>>  > > > > > In particular keys from the Google Titan Security key are 
>> being
>>  > > > > > blocked.
>>  > > > > >
>>  > > > > > For additional background information, please see
>>  > > > > > 
>> https://security.googleblog.com/2019/05/titan-keys-update.html
>>  > > > > > ---
>>  > > > > >
>>  > > > > >  src/adapter.c | 140 
>> +++++++++++++++++++++++++++++++++++++++++++++++---
>>  > > > > >  1 file changed, 134 insertions(+), 6 deletions(-)
>>  > > > > >
>>  > > > > > diff --git a/src/adapter.c b/src/adapter.c
>>  > > > > > index cef25616f..9c41ebe86 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;
>>  > > > > > +
>>  > > > > >  static GList *adapter_list = NULL;
>>  > > > > >  static unsigned int adapter_remaining = 0;
>>  > > > > >  static bool powering_down = false;
>>  > > > > > @@ -124,6 +141,7 @@ struct link_key_info {
>>  > > > > >         unsigned char key[16];
>>  > > > > >         uint8_t type;
>>  > > > > >         uint8_t pin_len;
>>  > > > > > +       bool is_blocked;
>>  > > > > >  };
>>  > > > > >
>>  > > > > >  struct smp_ltk_info {
>>  > > > > > @@ -135,12 +153,14 @@ struct smp_ltk_info {
>>  > > > > >         uint16_t ediv;
>>  > > > > >         uint64_t rand;
>>  > > > > >         uint8_t val[16];
>>  > > > > > +       bool is_blocked;
>>  > > > > >  };
>>  > > > > >
>>  > > > > >  struct irk_info {
>>  > > > > >         bdaddr_t bdaddr;
>>  > > > > >         uint8_t bdaddr_type;
>>  > > > > >         uint8_t val[16];
>>  > > > > > +       bool is_blocked;
>>  > > > > >  };
>>  > > > > >
>>  > > > > >  struct conn_param {
>>  > > > > > @@ -3439,6 +3459,20 @@ static int str2buf(const char 
>> *str, uint8_t *buf, size_t blen)
>>  > > > > >         return 0;
>>  > > > > >  }
>>  > > > > >
>>  > > > > > +static bool is_blocked_key(uint8_t key_type, uint8_t 
>> *key_value)
>>  > > > > > +{
>>  > > > > > +       uint32_t i = 0;
>>  > > > > > +
>>  > > > > > +       for (i = 0; i < ARRAY_SIZE(blocked_keys); ++i) {
>>  > > > > > +               if (key_type == blocked_keys[i].type &&
>>  > > > > > +                               
>> !memcmp(blocked_keys[i].val, key_value,
>>  > > > > > +                                               
>> sizeof(blocked_keys[i].val)))
>>  > > > > > +                       return true;
>>  > > > > > +       }
>>  > > > > > +
>>  > > > > > +       return false;
>>  > > > > > +}
>>  > > > > > +
>>  > > > > >  static struct link_key_info *get_key_info(GKeyFile 
>> *key_file, const char *peer)
>>  > > > > >  {
>>  > > > > >         struct link_key_info *info = NULL;
>>  > > > > > @@ -3461,6 +3495,9 @@ static struct link_key_info 
>> *get_key_info(GKeyFile *key_file, const char *peer)
>>  > > > > >         info->pin_len = g_key_file_get_integer(key_file, 
>> "LinkKey", "PINLength",
>>  > > > > >                                                 NULL);
>>  > > > > >
>>  > > > > > +       info->is_blocked = 
>> is_blocked_key(HCI_BLOCKED_KEY_TYPE_LINKKEY,
>>  > > > > > +                                                         
>>       info->key);
>>  > > > > > +
>>  > > > > >  failed:
>>  > > > > >         g_free(str);
>>  > > > > >
>>  > > > > > @@ -3534,6 +3571,9 @@ static struct smp_ltk_info 
>> *get_ltk(GKeyFile *key_file, const char *peer,
>>  > > > > >         else
>>  > > > > >                 ltk->master = master;
>>  > > > > >
>>  > > > > > +       ltk->is_blocked = 
>> is_blocked_key(HCI_BLOCKED_KEY_TYPE_LTK,
>>  > > > > > +                                                         
>>       ltk->val);
>>  > > > > > +
>>  > > > > >  failed:
>>  > > > > >         g_free(key);
>>  > > > > >         g_free(rand);
>>  > > > > > @@ -3584,6 +3624,9 @@ static struct irk_info 
>> *get_irk_info(GKeyFile *key_file, const char *peer,
>>  > > > > >         else
>>  > > > > >                 str2buf(&str[0], irk->val, 
>> sizeof(irk->val));
>>  > > > > >
>>  > > > > > +       irk->is_blocked = 
>> is_blocked_key(HCI_BLOCKED_KEY_TYPE_LINKKEY,
>>  > > > > > +                                                         
>>       irk->val);
>>  > > > > > +
>>  > > > > >  failed:
>>  > > > > >         g_free(str);
>>  > > > > >
>>  > > > > > @@ -4142,21 +4185,55 @@ static void load_devices(struct 
>> btd_adapter *adapter)
>>  > > > > >                 g_key_file_load_from_file(key_file, 
>> filename, 0, NULL);
>>  > > > > >
>>  > > > > >                 key_info = get_key_info(key_file, 
>> entry->d_name);
>>  > > > > > -               if (key_info)
>>  > > > > > -                       keys = g_slist_append(keys, 
>> key_info);
>>  > > > > >
>>  > > > > >                 bdaddr_type = get_le_addr_type(key_file);
>>  > > > > >
>>  > > > > >                 ltk_info = get_ltk_info(key_file, 
>> entry->d_name, bdaddr_type);
>>  > > > > > -               if (ltk_info)
>>  > > > > > -                       ltks = g_slist_append(ltks, 
>> ltk_info);
>>  > > > > >
>>  > > > > >                 slave_ltk_info = 
>> get_slave_ltk_info(key_file, entry->d_name,
>>  > > > > >                                                           
>>       bdaddr_type);
>>  > > > > > +
>>  > > > > > +               irk_info = get_irk_info(key_file, 
>> entry->d_name, bdaddr_type);
>>  > > > > > +
>>  > > > > > +               // If any key for the device is blocked, 
>> we discard all.
>>  > > > > > +               if ((key_info && key_info->is_blocked) ||
>>  > > > > > +                               (ltk_info && 
>> ltk_info->is_blocked) ||
>>  > > > > > +                               (slave_ltk_info &&
>>  > > > > > +                                       
>> slave_ltk_info->is_blocked) ||
>>  > > > > > +                               (irk_info && 
>> irk_info->is_blocked)) {
>>  > > > >
>>  > > > > Perhaps it would be more efficient if we don't add 
>> is_blocked as a
>>  > > > > member of these keys and instead pass bool variable to 
>> get_*_info
>>  > > > > which just set it in case the key is blocked, that way if 
>> is_blocked
>>  > > > > is set for get_key_info you don't have to free anything. 
>> Also it might
>>  > > > > be a good idea to move the handling of loading the keys to 
>> another
>>  > > > > function i.e. load_keys to make it simpler to bail out, 
>> etc. Have you
>>  > > > > though about the possibility of just using a the device 
>> block API when
>>  > > > > you detect it is using compromised keys? That way the 
>> application can
>>  > > > > detect the device is in fact blocked and can either remove 
>> the device,
>>  > > > > which would clean up the storage as well (see my comments 
>> bellow), or
>>  > > > > unblock it in order to continue using the device.
>>  > > >
>>  > > > My thoughts was to keep the contract simple.  if the return 
>> is not
>>  > > > null then a key exists, it can be blocked or not blocked.  It 
>> feels a
>>  > > > bit weird to have a contract return null but a parameter 
>> tells you
>>  > > > that it's been blocked.  The extra allocation/free doesn't 
>> seem like a
>>  > > > lot of overhead for what is expected to be a rare case 
>> (blocked keys).
>>  > >
>>  > > Fair enough, I suggested that because the is_block logic seems 
>> to
>>  > > apply to all of the keys not just the key that was compromised, 
>> anyway
>>  > > Im probably getting a little too paranoid regarding overhead 
>> but I
>>  > > don't think we had loose anything by avoiding the extra
>>  > > allocation/free
>>  > Correct.  The idea is that if any of the keys from the device is
>>  > blocked, the device
>>  > is simply ignored.
>>  >
>>  > >
>>  > > > >
>>  > > > > > +                       if (key_info) {
>>  > > > > > +                               g_free(key_info);
>>  > > > > > +                               key_info = NULL;
>>  > > > > > +                       }
>>  > > > > > +
>>  > > > > > +                       if (ltk_info) {
>>  > > > > > +                               g_free(ltk_info);
>>  > > > > > +                               ltk_info = NULL;
>>  > > > > > +                       }
>>  > > > > > +
>>  > > > > > +                       if (slave_ltk_info) {
>>  > > > > > +                               g_free(slave_ltk_info);
>>  > > > > > +                               slave_ltk_info = NULL;
>>  > > > > > +                       }
>>  > > > > > +
>>  > > > > > +                       if (irk_info) {
>>  > > > > > +                               g_free(irk_info);
>>  > > > > > +                               irk_info = NULL;
>>  > > > > > +                       }
>>  > > > > > +
>>  > > > > > +                       goto free;
>>  > > > > > +               }
>>  > > > > > +
>>  > > > > > +               if (key_info)
>>  > > > > > +                       keys = g_slist_append(keys, 
>> key_info);
>>  > > > > > +
>>  > > > > > +               if (ltk_info)
>>  > > > > > +                       ltks = g_slist_append(ltks, 
>> ltk_info);
>>  > > > > > +
>>  > > > >
>>  > > > > Perhaps I missing something but don't we need to clear 
>> these keys form
>>  > > > > the storage once we figure they are blocked or there is any 
>> reason to
>>  > > > > leave them in there so we can unblock?
>>  > > > Since these are expected to be rare cases, I didn't go 
>> through the
>>  > > > extent to clear them from storage.  Having them in storage 
>> also can
>>  > > > help with diagnosability.
>>  > >
>>  > > That is an interesting point, but we don't allow new pairing 
>> with
>>  > > compromised keys with these changes or do we? If we want to 
>> allow
>>  > > diagnosing in all cases we could actually store the blocked 
>> flag into
>>  > > the storage so something like a diagnostic tool can fetch the
>>  > > information directly.
>>  > With these changes, the device is simply dropped from the list
>>  > entirely.  The user is free to attempt to pair it again.  If the
>>  > device is fixed, pairing will work and the key will be 
>> overwritten, if
>>  > the device is still broken, the pairing will fail and the user 
>> will
>>  > see the error.  Given that for now, the keys are hardcoded and a
>>  > relatively small list, it feels like overkill to store an 
>> additional
>>  > flag for it.  This may be something we'd want to do if the list of
>>  > blocked keys grows beyond something that is easily manageable.  If
>>  > that is the case, I also think we should read the blocked keys 
>> from a
>>  > configuration file rather than hardcoded in the code as it 
>> currently
>>  > is.
>> 
>>  All good, just waiting the kernel changes to be applied then we can 
>> go
>>  ahead with these changes as well, @Marcel are you planning to merge
>>  the kernel changes?
>> 
>>  > >
>>  > > > >
>>  > > > > >                 if (slave_ltk_info)
>>  > > > > >                         ltks = g_slist_append(ltks, 
>> slave_ltk_info);
>>  > > > > >
>>  > > > > > -               irk_info = get_irk_info(key_file, 
>> entry->d_name, bdaddr_type);
>>  > > > > >                 if (irk_info)
>>  > > > > >                         irks = g_slist_append(irks, 
>> irk_info);
>>  > > > > >
>>  > > > > > @@ -8568,6 +8645,42 @@ 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 = ARRAY_SIZE(blocked_keys);
>>  > > > > > +       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 +8908,13 @@ 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 +9092,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.1.735.g03f4e72817-goog
>>  > > > > >
>>  > > > >
>>  > > > >
>>  > > > > --
>>  > > > > Luiz Augusto von Dentz
>>  > >
>>  > >
>>  > >
>>  > > --
>>  > > Luiz Augusto von Dentz
>> 
>> 
>> 

Applied, thanks.

Note that I changed a little bit the commit messages since it was 
causing some warnings with gitlint:

gitlint: checking commit message...
1: T3 Title has trailing punctuation (.): "MGMT_OP_SET_BLOCKED_KEYS Api 
definitions."
-----------------------------------------------
gitlint: \033[31mYour commit message contains the above 
violations.\033[0m
Continue with commit anyways (this keeps the current commit message)? 
[y(es)/n(no)/e(dit)] y
Applying: MGMT_OP_SET_BLOCKED_KEYS Api definitions.
gitlint: checking commit message...
1: T3 Title has trailing punctuation (.): "Adding a shared ARRAY_SIZE 
macro."
-----------------------------------------------
gitlint: \033[31mYour commit message contains the above 
violations.\033[0m
Continue with commit anyways (this keeps the current commit message)? 
[y(es)/n(no)/e(dit)] y
Applying: Adding a shared ARRAY_SIZE macro.
gitlint: checking commit message...
1: T3 Title has trailing punctuation (.): "Loading keys that should be 
blocked by the kernel."
-----------------------------------------------
gitlint: \033[31mYour commit message contains the above 
violations.\033[0m
Continue with commit anyways (this keeps the current commit message)? 
[y(es)/n(no)/e(dit)] y
Applying: Loading keys that should be blocked by the kernel.


>> 
>>  --
>>  Luiz Augusto von Dentz

Re: [PATCH v5 3/3] Loading keys that should be blocked by the kernel.

[Alain Michaud]

Thanks Luiz!


On Thu, Jan 16, 2020 at 1:23 PM Luiz Von Dentz <luiz.dentz@gmail.com> wrote:
>
> Hi Alain,
>
> On Thu, Jan 16, 2020 at 8:18 AM, Alain Michaud
> <alainmichaud@google.com> wrote:
> > Looks like Marcel committed the kernel changes, are we good to commit
> > these?
> >
> > Thanks!
> > Alain
> >
> > On Fri, Jan 10, 2020 at 4:08 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> >>
> >>  Hi Alain, Marcel,
> >>
> >>  On Wed, Jan 8, 2020 at 12:08 PM Alain Michaud
> >> <alainmichaud@google.com> wrote:
> >>  >
> >>  > Hi Luiz,
> >>  >
> >>  >
> >>  > On Wed, Jan 8, 2020 at 3:00 PM Luiz Augusto von Dentz
> >>  > <luiz.dentz@gmail.com> wrote:
> >>  > >
> >>  > > Hi Alain,
> >>  > >
> >>  > > On Wed, Jan 8, 2020 at 7:04 AM Alain Michaud
> >> <alainmichaud@google.com> wrote:
> >>  > > >
> >>  > > > Hi Luiz,
> >>  > > >
> >>  > > >
> >>  > > > On Tue, Jan 7, 2020 at 6:22 PM Luiz Augusto von Dentz
> >>  > > > <luiz.dentz@gmail.com> wrote:
> >>  > > > >
> >>  > > > > Hi Alain,
> >>  > > > >
> >>  > > > > On Mon, Jan 6, 2020 at 5:31 PM Alain Michaud
> >> <alainm@chromium.org> wrote:
> >>  > > > > >
> >>  > > > > > This change accomplishes 2 things:
> >>  > > > > >  1. Drop device security data from previously paired
> >> devices
> >>  > > > > >  using blocked keys.
> >>  > > > > >  2. Send the list of known bad keys that should be
> >> blocked to the kernel
> >>  > > > > >  if supported.
> >>  > > > > >
> >>  > > > > > In particular keys from the Google Titan Security key are
> >> being
> >>  > > > > > blocked.
> >>  > > > > >
> >>  > > > > > For additional background information, please see
> >>  > > > > >
> >> https://security.googleblog.com/2019/05/titan-keys-update.html
> >>  > > > > > ---
> >>  > > > > >
> >>  > > > > >  src/adapter.c | 140
> >> +++++++++++++++++++++++++++++++++++++++++++++++---
> >>  > > > > >  1 file changed, 134 insertions(+), 6 deletions(-)
> >>  > > > > >
> >>  > > > > > diff --git a/src/adapter.c b/src/adapter.c
> >>  > > > > > index cef25616f..9c41ebe86 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;
> >>  > > > > > +
> >>  > > > > >  static GList *adapter_list = NULL;
> >>  > > > > >  static unsigned int adapter_remaining = 0;
> >>  > > > > >  static bool powering_down = false;
> >>  > > > > > @@ -124,6 +141,7 @@ struct link_key_info {
> >>  > > > > >         unsigned char key[16];
> >>  > > > > >         uint8_t type;
> >>  > > > > >         uint8_t pin_len;
> >>  > > > > > +       bool is_blocked;
> >>  > > > > >  };
> >>  > > > > >
> >>  > > > > >  struct smp_ltk_info {
> >>  > > > > > @@ -135,12 +153,14 @@ struct smp_ltk_info {
> >>  > > > > >         uint16_t ediv;
> >>  > > > > >         uint64_t rand;
> >>  > > > > >         uint8_t val[16];
> >>  > > > > > +       bool is_blocked;
> >>  > > > > >  };
> >>  > > > > >
> >>  > > > > >  struct irk_info {
> >>  > > > > >         bdaddr_t bdaddr;
> >>  > > > > >         uint8_t bdaddr_type;
> >>  > > > > >         uint8_t val[16];
> >>  > > > > > +       bool is_blocked;
> >>  > > > > >  };
> >>  > > > > >
> >>  > > > > >  struct conn_param {
> >>  > > > > > @@ -3439,6 +3459,20 @@ static int str2buf(const char
> >> *str, uint8_t *buf, size_t blen)
> >>  > > > > >         return 0;
> >>  > > > > >  }
> >>  > > > > >
> >>  > > > > > +static bool is_blocked_key(uint8_t key_type, uint8_t
> >> *key_value)
> >>  > > > > > +{
> >>  > > > > > +       uint32_t i = 0;
> >>  > > > > > +
> >>  > > > > > +       for (i = 0; i < ARRAY_SIZE(blocked_keys); ++i) {
> >>  > > > > > +               if (key_type == blocked_keys[i].type &&
> >>  > > > > > +
> >> !memcmp(blocked_keys[i].val, key_value,
> >>  > > > > > +
> >> sizeof(blocked_keys[i].val)))
> >>  > > > > > +                       return true;
> >>  > > > > > +       }
> >>  > > > > > +
> >>  > > > > > +       return false;
> >>  > > > > > +}
> >>  > > > > > +
> >>  > > > > >  static struct link_key_info *get_key_info(GKeyFile
> >> *key_file, const char *peer)
> >>  > > > > >  {
> >>  > > > > >         struct link_key_info *info = NULL;
> >>  > > > > > @@ -3461,6 +3495,9 @@ static struct link_key_info
> >> *get_key_info(GKeyFile *key_file, const char *peer)
> >>  > > > > >         info->pin_len = g_key_file_get_integer(key_file,
> >> "LinkKey", "PINLength",
> >>  > > > > >                                                 NULL);
> >>  > > > > >
> >>  > > > > > +       info->is_blocked =
> >> is_blocked_key(HCI_BLOCKED_KEY_TYPE_LINKKEY,
> >>  > > > > > +
> >>       info->key);
> >>  > > > > > +
> >>  > > > > >  failed:
> >>  > > > > >         g_free(str);
> >>  > > > > >
> >>  > > > > > @@ -3534,6 +3571,9 @@ static struct smp_ltk_info
> >> *get_ltk(GKeyFile *key_file, const char *peer,
> >>  > > > > >         else
> >>  > > > > >                 ltk->master = master;
> >>  > > > > >
> >>  > > > > > +       ltk->is_blocked =
> >> is_blocked_key(HCI_BLOCKED_KEY_TYPE_LTK,
> >>  > > > > > +
> >>       ltk->val);
> >>  > > > > > +
> >>  > > > > >  failed:
> >>  > > > > >         g_free(key);
> >>  > > > > >         g_free(rand);
> >>  > > > > > @@ -3584,6 +3624,9 @@ static struct irk_info
> >> *get_irk_info(GKeyFile *key_file, const char *peer,
> >>  > > > > >         else
> >>  > > > > >                 str2buf(&str[0], irk->val,
> >> sizeof(irk->val));
> >>  > > > > >
> >>  > > > > > +       irk->is_blocked =
> >> is_blocked_key(HCI_BLOCKED_KEY_TYPE_LINKKEY,
> >>  > > > > > +
> >>       irk->val);
> >>  > > > > > +
> >>  > > > > >  failed:
> >>  > > > > >         g_free(str);
> >>  > > > > >
> >>  > > > > > @@ -4142,21 +4185,55 @@ static void load_devices(struct
> >> btd_adapter *adapter)
> >>  > > > > >                 g_key_file_load_from_file(key_file,
> >> filename, 0, NULL);
> >>  > > > > >
> >>  > > > > >                 key_info = get_key_info(key_file,
> >> entry->d_name);
> >>  > > > > > -               if (key_info)
> >>  > > > > > -                       keys = g_slist_append(keys,
> >> key_info);
> >>  > > > > >
> >>  > > > > >                 bdaddr_type = get_le_addr_type(key_file);
> >>  > > > > >
> >>  > > > > >                 ltk_info = get_ltk_info(key_file,
> >> entry->d_name, bdaddr_type);
> >>  > > > > > -               if (ltk_info)
> >>  > > > > > -                       ltks = g_slist_append(ltks,
> >> ltk_info);
> >>  > > > > >
> >>  > > > > >                 slave_ltk_info =
> >> get_slave_ltk_info(key_file, entry->d_name,
> >>  > > > > >
> >>       bdaddr_type);
> >>  > > > > > +
> >>  > > > > > +               irk_info = get_irk_info(key_file,
> >> entry->d_name, bdaddr_type);
> >>  > > > > > +
> >>  > > > > > +               // If any key for the device is blocked,
> >> we discard all.
> >>  > > > > > +               if ((key_info && key_info->is_blocked) ||
> >>  > > > > > +                               (ltk_info &&
> >> ltk_info->is_blocked) ||
> >>  > > > > > +                               (slave_ltk_info &&
> >>  > > > > > +
> >> slave_ltk_info->is_blocked) ||
> >>  > > > > > +                               (irk_info &&
> >> irk_info->is_blocked)) {
> >>  > > > >
> >>  > > > > Perhaps it would be more efficient if we don't add
> >> is_blocked as a
> >>  > > > > member of these keys and instead pass bool variable to
> >> get_*_info
> >>  > > > > which just set it in case the key is blocked, that way if
> >> is_blocked
> >>  > > > > is set for get_key_info you don't have to free anything.
> >> Also it might
> >>  > > > > be a good idea to move the handling of loading the keys to
> >> another
> >>  > > > > function i.e. load_keys to make it simpler to bail out,
> >> etc. Have you
> >>  > > > > though about the possibility of just using a the device
> >> block API when
> >>  > > > > you detect it is using compromised keys? That way the
> >> application can
> >>  > > > > detect the device is in fact blocked and can either remove
> >> the device,
> >>  > > > > which would clean up the storage as well (see my comments
> >> bellow), or
> >>  > > > > unblock it in order to continue using the device.
> >>  > > >
> >>  > > > My thoughts was to keep the contract simple.  if the return
> >> is not
> >>  > > > null then a key exists, it can be blocked or not blocked.  It
> >> feels a
> >>  > > > bit weird to have a contract return null but a parameter
> >> tells you
> >>  > > > that it's been blocked.  The extra allocation/free doesn't
> >> seem like a
> >>  > > > lot of overhead for what is expected to be a rare case
> >> (blocked keys).
> >>  > >
> >>  > > Fair enough, I suggested that because the is_block logic seems
> >> to
> >>  > > apply to all of the keys not just the key that was compromised,
> >> anyway
> >>  > > Im probably getting a little too paranoid regarding overhead
> >> but I
> >>  > > don't think we had loose anything by avoiding the extra
> >>  > > allocation/free
> >>  > Correct.  The idea is that if any of the keys from the device is
> >>  > blocked, the device
> >>  > is simply ignored.
> >>  >
> >>  > >
> >>  > > > >
> >>  > > > > > +                       if (key_info) {
> >>  > > > > > +                               g_free(key_info);
> >>  > > > > > +                               key_info = NULL;
> >>  > > > > > +                       }
> >>  > > > > > +
> >>  > > > > > +                       if (ltk_info) {
> >>  > > > > > +                               g_free(ltk_info);
> >>  > > > > > +                               ltk_info = NULL;
> >>  > > > > > +                       }
> >>  > > > > > +
> >>  > > > > > +                       if (slave_ltk_info) {
> >>  > > > > > +                               g_free(slave_ltk_info);
> >>  > > > > > +                               slave_ltk_info = NULL;
> >>  > > > > > +                       }
> >>  > > > > > +
> >>  > > > > > +                       if (irk_info) {
> >>  > > > > > +                               g_free(irk_info);
> >>  > > > > > +                               irk_info = NULL;
> >>  > > > > > +                       }
> >>  > > > > > +
> >>  > > > > > +                       goto free;
> >>  > > > > > +               }
> >>  > > > > > +
> >>  > > > > > +               if (key_info)
> >>  > > > > > +                       keys = g_slist_append(keys,
> >> key_info);
> >>  > > > > > +
> >>  > > > > > +               if (ltk_info)
> >>  > > > > > +                       ltks = g_slist_append(ltks,
> >> ltk_info);
> >>  > > > > > +
> >>  > > > >
> >>  > > > > Perhaps I missing something but don't we need to clear
> >> these keys form
> >>  > > > > the storage once we figure they are blocked or there is any
> >> reason to
> >>  > > > > leave them in there so we can unblock?
> >>  > > > Since these are expected to be rare cases, I didn't go
> >> through the
> >>  > > > extent to clear them from storage.  Having them in storage
> >> also can
> >>  > > > help with diagnosability.
> >>  > >
> >>  > > That is an interesting point, but we don't allow new pairing
> >> with
> >>  > > compromised keys with these changes or do we? If we want to
> >> allow
> >>  > > diagnosing in all cases we could actually store the blocked
> >> flag into
> >>  > > the storage so something like a diagnostic tool can fetch the
> >>  > > information directly.
> >>  > With these changes, the device is simply dropped from the list
> >>  > entirely.  The user is free to attempt to pair it again.  If the
> >>  > device is fixed, pairing will work and the key will be
> >> overwritten, if
> >>  > the device is still broken, the pairing will fail and the user
> >> will
> >>  > see the error.  Given that for now, the keys are hardcoded and a
> >>  > relatively small list, it feels like overkill to store an
> >> additional
> >>  > flag for it.  This may be something we'd want to do if the list of
> >>  > blocked keys grows beyond something that is easily manageable.  If
> >>  > that is the case, I also think we should read the blocked keys
> >> from a
> >>  > configuration file rather than hardcoded in the code as it
> >> currently
> >>  > is.
> >>
> >>  All good, just waiting the kernel changes to be applied then we can
> >> go
> >>  ahead with these changes as well, @Marcel are you planning to merge
> >>  the kernel changes?
> >>
> >>  > >
> >>  > > > >
> >>  > > > > >                 if (slave_ltk_info)
> >>  > > > > >                         ltks = g_slist_append(ltks,
> >> slave_ltk_info);
> >>  > > > > >
> >>  > > > > > -               irk_info = get_irk_info(key_file,
> >> entry->d_name, bdaddr_type);
> >>  > > > > >                 if (irk_info)
> >>  > > > > >                         irks = g_slist_append(irks,
> >> irk_info);
> >>  > > > > >
> >>  > > > > > @@ -8568,6 +8645,42 @@ 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 = ARRAY_SIZE(blocked_keys);
> >>  > > > > > +       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 +8908,13 @@ 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 +9092,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.1.735.g03f4e72817-goog
> >>  > > > > >
> >>  > > > >
> >>  > > > >
> >>  > > > > --
> >>  > > > > Luiz Augusto von Dentz
> >>  > >
> >>  > >
> >>  > >
> >>  > > --
> >>  > > Luiz Augusto von Dentz
> >>
> >>
> >>
>
> Applied, thanks.
>
> Note that I changed a little bit the commit messages since it was
> causing some warnings with gitlint:
>
> gitlint: checking commit message...
> 1: T3 Title has trailing punctuation (.): "MGMT_OP_SET_BLOCKED_KEYS Api
> definitions."
> -----------------------------------------------
> gitlint: \033[31mYour commit message contains the above
> violations.\033[0m
> Continue with commit anyways (this keeps the current commit message)?
> [y(es)/n(no)/e(dit)] y
> Applying: MGMT_OP_SET_BLOCKED_KEYS Api definitions.
> gitlint: checking commit message...
> 1: T3 Title has trailing punctuation (.): "Adding a shared ARRAY_SIZE
> macro."
> -----------------------------------------------
> gitlint: \033[31mYour commit message contains the above
> violations.\033[0m
> Continue with commit anyways (this keeps the current commit message)?
> [y(es)/n(no)/e(dit)] y
> Applying: Adding a shared ARRAY_SIZE macro.
> gitlint: checking commit message...
> 1: T3 Title has trailing punctuation (.): "Loading keys that should be
> blocked by the kernel."
> -----------------------------------------------
> gitlint: \033[31mYour commit message contains the above
> violations.\033[0m
> Continue with commit anyways (this keeps the current commit message)?
> [y(es)/n(no)/e(dit)] y
> Applying: Loading keys that should be blocked by the kernel.
>
>
> >>
> >>  --
> >>  Luiz Augusto von Dentz
>
>