Hi Luiz,
On Tuesday, 20 March 2018 09:01:09 CET Luiz Augusto von Dentz wrote:
> Hi Szymon,
>
> On Mon, Mar 19, 2018 at 5:06 PM, Szymon Janc <szymon.janc@xxxxxxxxxxx>
wrote:
> > This allows to configure bluetoothd to require minimum encryption key
> > size when accessing GATT server characteristics. It is a global
> > configuration option affecting whole GATT database.
>
> I guess this is just to pass some GATT testing, I really wonder why
> such a thing would be mandatory if there isn't any way to influence
> the key size on SMP, or is there? If you would have some extra time it
> would be great to have unit tests for the affected tests of the
> testing spec.
Yes, there are multiple tests affected by this. But I don't see a reason why
not to have option to enforce minimal key size as defined by spec. On SMP
level this is basically minimum of both sides. And I don't think we should
enforce this on pairing as this would have to result in Pairing Failed and
thus link disconnection.
>
> > ---
> >
> > peripheral/gatt.c | 2 +-
> > src/adapter.c | 44
> > +++++++++++++++++++++++++++++++-------------
> > src/device.c | 14 ++++++++++++--
> > src/device.h | 1 +
> > src/hcid.h | 2 ++
> > src/main.c | 10 ++++++++++
> > src/main.conf | 5 +++++
> > src/shared/gatt-server.c | 43 ++++++++++++++++++++++++++++++++++++-------
> > src/shared/gatt-server.h | 6 +++++-
> > tools/btgatt-server.c | 2 +-
> > unit/test-gatt.c | 2 +-
> > 11 files changed, 105 insertions(+), 26 deletions(-)
> >
> > diff --git a/peripheral/gatt.c b/peripheral/gatt.c
> > index 5ae19a8e5..08541c424 100644
> > --- a/peripheral/gatt.c
> > +++ b/peripheral/gatt.c
> > @@ -128,7 +128,7 @@ static struct gatt_conn *gatt_conn_new(int fd)
> >
> > bt_att_set_security(conn->att, BT_SECURITY_MEDIUM);
> >
> > - conn->gatt = bt_gatt_server_new(gatt_db, conn->att, mtu);
> > + conn->gatt = bt_gatt_server_new(gatt_db, conn->att, mtu, 0);
> >
> > if (!conn->gatt) {
> >
> > fprintf(stderr, "Failed to create GATT server\n");
> > bt_att_unref(conn->att);
> >
> > diff --git a/src/adapter.c b/src/adapter.c
> > index 6d7d61504..c8e365479 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> >
> > @@ -3441,25 +3441,26 @@ failed:
> > return ltk;
> >
> > }
> >
> > -static GSList *get_ltk_info(GKeyFile *key_file, const char *peer,
> > +struct smp_ltk_info *get_ltk_info(GKeyFile *key_file, const char *peer,
> > + uint8_t
> > bdaddr_type) +{
> > + DBG("%s", peer);
> > +
> > + return get_ltk(key_file, peer, bdaddr_type, "LongTermKey");
> > +}
> > +
> > +struct smp_ltk_info *get_slave_ltk_info(GKeyFile *key_file, const char
> > *peer,>
> > uint8_t
> > bdaddr_type)
> >
> > {
> >
> > struct smp_ltk_info *ltk;
> >
> > - GSList *l = NULL;
> >
> > DBG("%s", peer);
> >
> > - ltk = get_ltk(key_file, peer, bdaddr_type, "LongTermKey");
> > - if (ltk)
> > - l = g_slist_append(l, ltk);
> > -
> >
> > ltk = get_ltk(key_file, peer, bdaddr_type, "SlaveLongTermKey");
> >
> > - if (ltk) {
> > + if (ltk)
> >
> > ltk->master = false;
> >
> > - l = g_slist_append(l, ltk);
> > - }
> >
> > - return l;
> > + return ltk;
> >
> > }
> >
> > static struct irk_info *get_irk_info(GKeyFile *key_file, const char
> > *peer,
> >
> > @@ -4019,7 +4020,9 @@ static void load_devices(struct btd_adapter
> > *adapter)
> >
> > char filename[PATH_MAX];
> > GKeyFile *key_file;
> > struct link_key_info *key_info;
> >
> > - GSList *list, *ltk_info;
> > + struct smp_ltk_info *ltk_info;
> > + struct smp_ltk_info *slave_ltk_info;
> > + GSList *list;
> >
> > struct irk_info *irk_info;
> > struct conn_param *param;
> > uint8_t bdaddr_type;
> >
> > @@ -4044,7 +4047,13 @@ static void load_devices(struct btd_adapter
> > *adapter)>
> > bdaddr_type = get_le_addr_type(key_file);
> >
> > ltk_info = get_ltk_info(key_file, entry->d_name,
> > bdaddr_type);
> >
> > - ltks = g_slist_concat(ltks, ltk_info);
> > + if (ltk_info)
> > + ltks = g_slist_append(ltks, ltk_info);
> > +
> > + slave_ltk_info = get_slave_ltk_info(key_file,
> > entry->d_name, +
> > bdaddr_type); + 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)
> >
> > @@ -4079,9 +4088,16 @@ device_exist:
> > device_set_bonded(device, BDADDR_BREDR);
> >
> > }
> >
> > - if (ltk_info) {
> > + if (ltk_info || slave_ltk_info) {
> >
> > device_set_paired(device, bdaddr_type);
> > device_set_bonded(device, bdaddr_type);
> >
> > +
> > + if (ltk_info)
> > + device_set_ltk_enc_size(device,
> > +
> > ltk_info->enc_size); + else if (slave_ltk_info)
> > + device_set_ltk_enc_size(device,
> > + slave_ltk_info->enc_size);
> >
> > }
> >
> > free:
> > @@ -7464,6 +7480,8 @@ static void new_long_term_key_callback(uint16_t
> > index, uint16_t length,>
> > device_set_bonded(device, addr->type);
> >
> > }
> >
> > + device_set_ltk_enc_size(device, ev->key.enc_size);
> > +
> >
> > bonding_complete(adapter, &addr->bdaddr, addr->type, 0);
> >
> > }
> >
> > diff --git a/src/device.c b/src/device.c
> > index 7c7196ca1..64b05d2f4 100644
> > --- a/src/device.c
> > +++ b/src/device.c
> > @@ -243,6 +243,7 @@ struct btd_device {
> >
> > struct csrk_info *local_csrk;
> > struct csrk_info *remote_csrk;
> >
> > + uint8_t ltk_enc_size;
> >
> > sdp_list_t *tmp_records;
> >
> > @@ -1460,6 +1461,11 @@ bool device_is_disconnecting(struct btd_device
> > *device)>
> > return device->disconn_timer > 0;
> >
> > }
> >
> > +void device_set_ltk_enc_size(struct btd_device *device, uint8_t enc_size)
> > +{
> > + device->ltk_enc_size = enc_size;
> > +}
> > +
> >
> > static void device_set_auto_connect(struct btd_device *device, gboolean
> > enable) {
> >
> > char addr[18];
> >
> > @@ -4834,11 +4840,15 @@ static void gatt_server_init(struct btd_device
> > *device, struct gatt_db *db)>
> > gatt_server_cleanup(device);
> >
> > - device->server = bt_gatt_server_new(db, device->att,
> > device->att_mtu); - if (!device->server)
> > + device->server = bt_gatt_server_new(db, device->att,
> > device->att_mtu, +
> > main_opts.min_enc_key_size); + if (!device->server) {
> >
> > error("Failed to initialize bt_gatt_server");
> >
> > + return;
> > + }
> >
> > bt_gatt_server_set_debug(device->server, gatt_debug, NULL, NULL);
> >
> > + bt_gatt_server_set_enc_key_size(device->server,
> > device->ltk_enc_size);>
> > }
> >
> > static bool local_counter(uint32_t *sign_cnt, void *user_data)
> >
> > diff --git a/src/device.h b/src/device.h
> > index bc046f780..306c813fc 100644
> > --- a/src/device.h
> > +++ b/src/device.h
> > @@ -130,6 +130,7 @@ void device_add_connection(struct btd_device *dev,
> > uint8_t bdaddr_type);>
> > void device_remove_connection(struct btd_device *device, uint8_t
> > bdaddr_type); void device_request_disconnect(struct btd_device *device,
> > DBusMessage *msg); bool device_is_disconnecting(struct btd_device
> > *device);
> >
> > +void device_set_ltk_enc_size(struct btd_device *device, uint8_t
> > enc_size);
> >
> > typedef void (*disconnect_watch) (struct btd_device *device, gboolean
> > removal,>
> > void *user_data);
> >
> > diff --git a/src/hcid.h b/src/hcid.h
> > index 62e2bd606..2c2b89d9c 100644
> > --- a/src/hcid.h
> > +++ b/src/hcid.h
> > @@ -54,6 +54,8 @@ struct main_opts {
> >
> > bt_mode_t mode;
> > bt_gatt_cache_t gatt_cache;
> >
> > +
> > + uint8_t min_enc_key_size;
> >
> > };
> >
> > extern struct main_opts main_opts;
> >
> > diff --git a/src/main.c b/src/main.c
> > index 21f0b14a4..29a78e64c 100644
> > --- a/src/main.c
> > +++ b/src/main.c
> > @@ -407,6 +407,16 @@ static void parse_config(GKeyFile *config)
> >
> > main_opts.gatt_cache = parse_gatt_cache(str);
> >
> > + val = g_key_file_get_integer(config, "GATT",
> > + "MinEncKeySize", &err);
> > + if (err) {
> > + DBG("%s", err->message);
> > + g_clear_error(&err);
> > + } else {
> > + DBG("min_enc_key_size=%d", val);
> > + main_opts.min_enc_key_size = val;
> > + }
> > +
> >
> > g_free(str);
> >
> > }
> >
> > diff --git a/src/main.conf b/src/main.conf
> > index 21986b386..cbae32ec5 100644
> > --- a/src/main.conf
> > +++ b/src/main.conf
> > @@ -77,6 +77,11 @@
> >
> > # Default: always
> > #Cache = always
> >
> > +# Minimum required Encryption Key Size for accessing secured
> > characteristics. +# Possible values: 0 and 7-16. 0 means don't care.
> > +# Defaults to 0
> > +# MinEncKeySize = 0
> > +
> >
> > [Policy]
> > #
> > # The ReconnectUUIDs defines the set of remote services that should try
> >
> > diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> > index faa56abeb..cf88d72df 100644
> > --- a/src/shared/gatt-server.c
> > +++ b/src/shared/gatt-server.c
> > @@ -103,6 +103,9 @@ struct bt_gatt_server {
> >
> > unsigned int prep_write_id;
> > unsigned int exec_write_id;
> >
> > + uint8_t min_enc_size;
> > + uint8_t enc_size;
> > +
> >
> > struct queue *prep_queue;
> > unsigned int max_prep_queue_len;
> >
> > @@ -379,6 +382,12 @@ done:
> > process_read_by_type(op);
> >
> > }
> >
> > +void bt_gatt_server_set_enc_key_size(struct bt_gatt_server *server,
> > + uint8_t
> > size) +{
> > + server->enc_size = size;
> > +}
>
> I would prefer we would store the key size on bt_att if possible, then
> perhaps return it with bt_att_get_security.
hmm OK, I could do that.
>
> > static uint8_t check_permissions(struct bt_gatt_server *server,
> >
> > struct gatt_db_attribute *attr, uint32_t
> > mask)
> >
> > {
> >
> > @@ -398,14 +407,32 @@ static uint8_t check_permissions(struct
> > bt_gatt_server *server,>
> > return 0;
> >
> > security = bt_att_get_security(server->att);
> >
> > - if (perm & BT_ATT_PERM_SECURE && security < BT_ATT_SECURITY_FIPS)
> > - return BT_ATT_ERROR_AUTHENTICATION;
> > + if (perm & BT_ATT_PERM_SECURE) {
> > + if (security < BT_ATT_SECURITY_FIPS)
> > + return BT_ATT_ERROR_AUTHENTICATION;
> > +
> > + if (server->min_enc_size &&
> > + server->min_enc_size > server->enc_size)
> > + return
> > BT_ATT_ERROR_INSUFFICIENT_ENCRYPTION_KEY_SIZE; + }
> > +
> > + if (perm & BT_ATT_PERM_AUTHEN) {
> > + if (security < BT_ATT_SECURITY_HIGH)
> > + return BT_ATT_ERROR_AUTHENTICATION;
> >
> > - if (perm & BT_ATT_PERM_AUTHEN && security < BT_ATT_SECURITY_HIGH)
> > - return BT_ATT_ERROR_AUTHENTICATION;
> > + if (server->min_enc_size &&
> > + server->min_enc_size > server->enc_size)
> > + return
> > BT_ATT_ERROR_INSUFFICIENT_ENCRYPTION_KEY_SIZE; + }
> >
> > - if (perm & BT_ATT_PERM_ENCRYPT && security <
> > BT_ATT_SECURITY_MEDIUM) - return
> > BT_ATT_ERROR_INSUFFICIENT_ENCRYPTION;
> > + if (perm & BT_ATT_PERM_ENCRYPT) {
> > + if (security < BT_ATT_SECURITY_MEDIUM)
> > + return BT_ATT_ERROR_INSUFFICIENT_ENCRYPTION;
> > +
> > + if (server->min_enc_size &&
> > + server->min_enc_size > server->enc_size)
> > + return
> > BT_ATT_ERROR_INSUFFICIENT_ENCRYPTION_KEY_SIZE; + }
> >
> > return 0;
> >
> > }
> >
> > @@ -1481,7 +1508,8 @@ static bool gatt_server_register_att_handlers(struct
> > bt_gatt_server *server)>
> > }
> >
> > struct bt_gatt_server *bt_gatt_server_new(struct gatt_db *db,
> >
> > - struct bt_att *att, uint16_t mtu)
> > + struct bt_att *att, uint16_t mtu,
> > + uint8_t min_enc_size)
> >
> > {
> >
> > struct bt_gatt_server *server;
> >
> > @@ -1494,6 +1522,7 @@ struct bt_gatt_server *bt_gatt_server_new(struct
> > gatt_db *db,>
> > server->mtu = MAX(mtu, BT_ATT_DEFAULT_LE_MTU);
> > server->max_prep_queue_len = DEFAULT_MAX_PREP_QUEUE_LEN;
> > server->prep_queue = queue_new();
> >
> > + server->min_enc_size = min_enc_size;
> >
> > if (!gatt_server_register_att_handlers(server)) {
> >
> > bt_gatt_server_free(server);
> >
> > diff --git a/src/shared/gatt-server.h b/src/shared/gatt-server.h
> > index 74a6c721e..6bf178a9b 100644
> > --- a/src/shared/gatt-server.h
> > +++ b/src/shared/gatt-server.h
> > @@ -26,7 +26,8 @@
> >
> > struct bt_gatt_server;
> >
> > struct bt_gatt_server *bt_gatt_server_new(struct gatt_db *db,
> >
> > - struct bt_att *att, uint16_t mtu);
> > + struct bt_att *att, uint16_t mtu,
> > + uint8_t min_enc_size);
> >
> > uint16_t bt_gatt_server_get_mtu(struct bt_gatt_server *server);
> >
> > struct bt_gatt_server *bt_gatt_server_ref(struct bt_gatt_server *server);
> >
> > @@ -51,3 +52,6 @@ bool bt_gatt_server_send_indication(struct
> > bt_gatt_server *server,>
> > bt_gatt_server_conf_func_t
> > callback,
> > void *user_data,
> > bt_gatt_server_destroy_func_t
> > destroy);
> >
> > +
> > +void bt_gatt_server_set_enc_key_size(struct bt_gatt_server *server,
> > + uint8_t
> > size); diff --git a/tools/btgatt-server.c b/tools/btgatt-server.c
> > index fadaff2ad..89812bd75 100644
> > --- a/tools/btgatt-server.c
> > +++ b/tools/btgatt-server.c
> > @@ -583,7 +583,7 @@ static struct server *server_create(int fd, uint16_t
> > mtu, bool hr_visible)>
> > goto fail;
> >
> > }
> >
> > - server->gatt = bt_gatt_server_new(server->db, server->att, mtu);
> > + server->gatt = bt_gatt_server_new(server->db, server->att, mtu,
> > 0);
> >
> > if (!server->gatt) {
> >
> > fprintf(stderr, "Failed to create GATT server\n");
> > goto fail;
> >
> > diff --git a/unit/test-gatt.c b/unit/test-gatt.c
> > index 5d79e94c0..c7e28f865 100644
> > --- a/unit/test-gatt.c
> > +++ b/unit/test-gatt.c
> > @@ -667,7 +667,7 @@ static struct context *create_context(uint16_t mtu,
> > gconstpointer data)>
> > g_assert(context->server_db);
> >
> > context->server = bt_gatt_server_new(context->server_db,
> >
> > - context->att,
> > mtu);
> > + context->att, mtu,
> > 0);>
> > g_assert(context->server);
> >
> > bt_gatt_server_set_debug(context->server, print_debug,
> >
> > --
> > 2.14.3
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-bluetooth"
> > in the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
pozdrawiam
Szymon Janc
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
