Re: [PATCH BlueZ v3 1/2] gatt: Allow GATT server to dicate CCC permissions

[Luiz Augusto von Dentz <luiz.dentz@gmail.com>]

Hi Dagan,

On Thu, Sep 30, 2021 at 8:24 AM Dagan Martinez <dmartinez@starry.com> wrote:
>
> Allow a GATT server to impose permissions/restrictions on a CCC by
> setting additional `X-notify` and `X-indicate` permissions on its
> associated characteristic.
>
> This allows a developer to require encryption/authentication in order
> for a GATT client to subscribe to server-initiated updates.
>
> Test procedure:
> Attempt to read/write with a "low" security level on an unprotected CCC
> using gatttool, and succeed
> Attempt to READ with a "low" security level on an protected CCC
> using gatttool, and succeed
> Attempt to WRITE with a "low" security level on an protected CCC
> using gatttool, and fail
> Attempt to read/write while paired on a protected CCC using
> `bluetoothctl`, and succeed
> ---
>  src/gatt-database.c    | 42 ++++++++++++++++++++++++++++++++++++++----
>  src/shared/att-types.h |  4 ++++
>  2 files changed, 42 insertions(+), 4 deletions(-)
>
> diff --git a/src/gatt-database.c b/src/gatt-database.c
> index 68f411ba4..fd4a39166 100644
> --- a/src/gatt-database.c
> +++ b/src/gatt-database.c
> @@ -1060,17 +1060,33 @@ service_add_ccc(struct gatt_db_attribute *service,
>                                 struct btd_gatt_database *database,
>                                 btd_gatt_database_ccc_write_t write_callback,
>                                 void *user_data,
> +                               uint32_t parent_permissions,
>                                 btd_gatt_database_destroy_t destroy)
>  {
>         struct gatt_db_attribute *ccc;
>         struct ccc_cb_data *ccc_cb;
>         bt_uuid_t uuid;
> +       uint32_t permissions;
>
>         ccc_cb = new0(struct ccc_cb_data, 1);
>
> +       /*
> +        * Provide a way for the permissions on a characteristic to dictate
> +        * the permissions on the CCC
> +        */
> +       permissions = BT_ATT_PERM_READ | BT_ATT_PERM_WRITE;
> +
> +       if (parent_permissions & BT_ATT_PERM_SERVER_INITIATED_UPDATE_ENCRYPT)
> +               permissions |= BT_ATT_PERM_WRITE_ENCRYPT;
> +
> +       if (parent_permissions & BT_ATT_PERM_SERVER_INITIATED_UPDATE_AUTHEN)
> +               permissions |= BT_ATT_PERM_WRITE_AUTHEN;
> +
> +       if (parent_permissions & BT_ATT_PERM_SERVER_INITIATED_UPDATE_SECURE)
> +               permissions |= BT_ATT_PERM_WRITE_SECURE;
> +
>         bt_uuid16_create(&uuid, GATT_CLIENT_CHARAC_CFG_UUID);
> -       ccc = gatt_db_service_add_descriptor(service, &uuid,
> -                               BT_ATT_PERM_READ | BT_ATT_PERM_WRITE,
> +       ccc = gatt_db_service_add_descriptor(service, &uuid, permissions,
>                                 gatt_ccc_read_cb, gatt_ccc_write_cb, database);
>         if (!ccc) {
>                 error("Failed to create CCC entry in database");
> @@ -1227,7 +1243,7 @@ static void populate_gatt_service(struct btd_gatt_database *database)
>                                 NULL, NULL, database);
>
>         database->svc_chngd_ccc = service_add_ccc(service, database, NULL, NULL,
> -                                                                       NULL);
> +                                                                   0, NULL);
>
>         bt_uuid16_create(&uuid, GATT_CHARAC_CLI_FEAT);
>         database->cli_feat = gatt_db_service_add_characteristic(service,
> @@ -1690,6 +1706,24 @@ static bool parse_chrc_flags(DBusMessageIter *array, uint8_t *props,
>                         *perm |= BT_ATT_PERM_WRITE | BT_ATT_PERM_WRITE_SECURE;
>                 } else if (!strcmp("authorize", flag)) {
>                         *req_prep_authorization = true;
> +               } else if (!strcmp("encrypt-notify", flag)) {
> +                       *perm |= BT_ATT_PERM_SERVER_INITIATED_UPDATE_ENCRYPT;
> +                       *props |= BT_GATT_CHRC_PROP_NOTIFY;
> +               } else if (!strcmp("encrypt-authenticated-notify", flag)) {
> +                       *perm |= BT_ATT_PERM_SERVER_INITIATED_UPDATE_AUTHEN;
> +                       *props |= BT_GATT_CHRC_PROP_NOTIFY;
> +               } else if (!strcmp("secure-notify", flag)) {
> +                       *perm |= BT_ATT_PERM_SERVER_INITIATED_UPDATE_SECURE;
> +                       *props |= BT_GATT_CHRC_PROP_NOTIFY;
> +               } else if (!strcmp("encrypt-indicate", flag)) {
> +                       *perm |= BT_ATT_PERM_SERVER_INITIATED_UPDATE_ENCRYPT;
> +                       *props |= BT_GATT_CHRC_PROP_INDICATE;
> +               } else if (!strcmp("encrypt-authenticated-indicate", flag)) {
> +                       *perm |= BT_ATT_PERM_SERVER_INITIATED_UPDATE_AUTHEN;
> +                       *props |= BT_GATT_CHRC_PROP_INDICATE;
> +               } else if (!strcmp("secure-indicate", flag)) {
> +                       *perm |= BT_ATT_PERM_SERVER_INITIATED_UPDATE_SECURE;
> +                       *props |= BT_GATT_CHRC_PROP_INDICATE;
>                 } else {
>                         error("Invalid characteristic flag: %s", flag);
>                         return false;
> @@ -2796,7 +2830,7 @@ static bool database_add_ccc(struct external_service *service,
>                 return true;
>
>         chrc->ccc = service_add_ccc(service->attrib, service->app->database,
> -                                               ccc_write_cb, chrc, NULL);
> +                                       ccc_write_cb, chrc, chrc->perm, NULL);
>         if (!chrc->ccc) {
>                 error("Failed to create CCC entry for characteristic");
>                 return false;
> diff --git a/src/shared/att-types.h b/src/shared/att-types.h
> index a08b24155..eb5def503 100644
> --- a/src/shared/att-types.h
> +++ b/src/shared/att-types.h
> @@ -137,6 +137,10 @@ struct bt_att_pdu_error_rsp {
>                                         BT_ATT_PERM_WRITE_AUTHEN | \
>                                         BT_ATT_PERM_WRITE_ENCRYPT | \
>                                         BT_ATT_PERM_WRITE_SECURE)
> +/* Permissions to be applied to the CCC*/
> +#define BT_ATT_PERM_SERVER_INITIATED_UPDATE_ENCRYPT 0x0400
> +#define BT_ATT_PERM_SERVER_INITIATED_UPDATE_AUTHEN 0x0800
> +#define BT_ATT_PERM_SERVER_INITIATED_UPDATE_SECURE 0x1000

I don't think we really need these above, how about we do something
like the following:

https://gist.github.com/Vudentz/af6899625df3d83b62cfbc61bbd4b94b

That way we don't need to define more permissions since we can just
reuse the existing one which imo are easier to understand.

>  /* GATT Characteristic Properties Bitfield values */
>  #define BT_GATT_CHRC_PROP_BROADCAST                    0x01
> --
> 2.31.1

Btw, it would be great to have an example using the new flags in the
patch description, bluetoothctl> register-characteristic...


-- 
Luiz Augusto von Dentz