From: Szymon Janc <szymon.janc@codecoup.pl>
To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Subject: Re: [PATCH] Add support for requiring min key size for access GATT characteristics
Date: Tue, 20 Mar 2018 10:10:31 +0100 [thread overview]
Message-ID: <3405591.sIV7NVuiC8@ix> (raw)
In-Reply-To: <CABBYNZLQGEEQujx49DhrBaoOGyUPQ_Pqv-MdFA9o3T0=5WzLRg@mail.gmail.com>
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@codecoup.pl>
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@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
pozdrawiam
Szymon Janc
prev parent reply other threads:[~2018-03-20 9:10 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-19 15:06 [PATCH] Add support for requiring min key size for access GATT characteristics Szymon Janc
2018-03-20 8:01 ` Luiz Augusto von Dentz
2018-03-20 9:10 ` Szymon Janc [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3405591.sIV7NVuiC8@ix \
--to=szymon.janc@codecoup.pl \
--cc=linux-bluetooth@vger.kernel.org \
--cc=luiz.dentz@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.