[PATCH BlueZ 0/2] Optionally require security for notify/indicate

[PATCH BlueZ 0/2] Optionally require security for notify/indicate

[Dagan Martinez]

In some cases, it is desirable to require encryption and/or
authentication for server-initiated updates, as they may contain
sensitive data.

Currently, there is no way to do this with BlueZ.

Here is a query about this feature from 2019:
https://stackoverflow.com/questions/55884233

This patch implements this feature by introducing new `x-asynchronous`
flags that allow a user to restrict access to a characteristic's CCCD
(as well as documentation for those flags).

Dagan Martinez (2):
  gatt: allow GATT server to dicate CCC permissions
  doc/gatt-api: Add 'X-asynchronous` permissions

 doc/gatt-api.txt       | 11 ++++++++++-
 src/gatt-database.c    | 29 +++++++++++++++++++++++++----
 src/shared/att-types.h |  4 ++++
 3 files changed, 39 insertions(+), 5 deletions(-)

-- 
2.31.1

[PATCH BlueZ 1/2] gatt: allow GATT server to dicate CCC permissions

[Dagan Martinez]

Allow a GATT server to impose permissions/restrictions on a CCC by
setting additional `X-asynchronous` 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    | 29 +++++++++++++++++++++++++----
 src/shared/att-types.h |  4 ++++
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/src/gatt-database.c b/src/gatt-database.c
index 1f7ce5f02..bb65656bd 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -1044,6 +1044,7 @@ 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;
@@ -1052,9 +1053,23 @@ service_add_ccc(struct gatt_db_attribute *service,
 
 	ccc_cb = new0(struct ccc_cb_data, 1);
 
+	/*
+	 * Provide a way for the permissions on a characteristic to dictate
+	 * the permissions on the CCC
+	 */
+	uint32_t permissions = BT_ATT_PERM_READ | BT_ATT_PERM_WRITE;
+
+	if (parent_permissions & BT_ATT_PERM_ASYNCHRONOUS_ENCRYPT)
+		permissions |= BT_ATT_PERM_WRITE_ENCRYPT;
+
+	if (parent_permissions & BT_ATT_PERM_ASYNCHRONOUS_AUTHEN)
+		permissions |= BT_ATT_PERM_WRITE_AUTHEN;
+
+	if (parent_permissions & BT_ATT_PERM_ASYNCHRONOUS_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");
@@ -1211,7 +1226,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,
@@ -1674,6 +1689,12 @@ 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-asynchronous", flag)) {
+			*perm |= BT_ATT_PERM_ASYNCHRONOUS_ENCRYPT;
+		} else if (!strcmp("encrypt-authenticated-asynchronous", flag)) {
+			*perm |= BT_ATT_PERM_ASYNCHRONOUS_AUTHEN;
+		} else if (!strcmp("secure-asynchronous", flag)) {
+			*perm |= BT_ATT_PERM_ASYNCHRONOUS_SECURE;
 		} else {
 			error("Invalid characteristic flag: %s", flag);
 			return false;
@@ -2773,7 +2794,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..554441aca 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_ASYNCHRONOUS_ENCRYPT 0x0400
+#define BT_ATT_PERM_ASYNCHRONOUS_AUTHEN 0x0800
+#define BT_ATT_PERM_ASYNCHRONOUS_SECURE 0x1000
 
 /* GATT Characteristic Properties Bitfield values */
 #define BT_GATT_CHRC_PROP_BROADCAST			0x01
-- 
2.31.1

[PATCH BlueZ 2/2] doc/gatt-api: Add 'X-asynchronous` permissions

[Dagan Martinez]

Update docs to reflect the addition of `X-asynchronous` permissions,
which allow a GATT server to restrict CCC write permissions via
permissions set on its associated characteristic.
---
 doc/gatt-api.txt | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/doc/gatt-api.txt b/doc/gatt-api.txt
index 04789c6d3..2550510ba 100644
--- a/doc/gatt-api.txt
+++ b/doc/gatt-api.txt
@@ -253,7 +253,13 @@ Properties	string UUID [read-only]
 			Defines how the characteristic value can be used. See
 			Core spec "Table 3.5: Characteristic Properties bit
 			field", and "Table 3.8: Characteristic Extended
-			Properties bit field". Allowed values:
+			Properties bit field".
+
+			The "x-asynchronous" flags allow a characteristic to impose write
+			restrictions on its client characteristic configuration descriptor,
+			if applicable, restricting access to notifications and indications.
+
+			Allowed values:
 
 				"broadcast"
 				"read"
@@ -267,10 +273,13 @@ Properties	string UUID [read-only]
 				"writable-auxiliaries"
 				"encrypt-read"
 				"encrypt-write"
+				"encrypt-asynchronous" (Server only)
 				"encrypt-authenticated-read"
 				"encrypt-authenticated-write"
+				"encrypt-authenticated-asynchronous" (Server only)
 				"secure-read" (Server only)
 				"secure-write" (Server only)
+				"secure-asynchronous" (Server only)
 				"authorize"
 
 		uint16 Handle [read-write, optional] (Server Only)
-- 
2.31.1

RE: Optionally require security for notify/indicate

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 3663 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=555113

---Test result---

Test Summary:
CheckPatch                    FAIL      0.88 seconds
GitLint                       PASS      0.59 seconds
Prep - Setup ELL              PASS      44.86 seconds
Build - Prep                  PASS      0.18 seconds
Build - Configure             PASS      7.90 seconds
Build - Make                  FAIL      161.42 seconds
Make Check                    FAIL      1.37 seconds
Make Distcheck                PASS      223.29 seconds
Build w/ext ELL - Configure   PASS      7.79 seconds
Build w/ext ELL - Make        FAIL      143.08 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script with rule in .checkpatch.conf
Output:
[BlueZ,1/2] gatt: allow GATT server to dicate CCC permissions
WARNING:LONG_LINE: line length of 81 exceeds 80 columns
#144: FILE: src/gatt-database.c:1229:
+									0, NULL);

WARNING:LONG_LINE: line length of 81 exceeds 80 columns
#154: FILE: src/gatt-database.c:1694:
+		} else if (!strcmp("encrypt-authenticated-asynchronous", flag)) {

WARNING:LONG_LINE: line length of 86 exceeds 80 columns
#166: FILE: src/gatt-database.c:2797:
+						ccc_write_cb, chrc, chrc->perm, NULL);

/github/workspace/src/12526275.patch total: 0 errors, 3 warnings, 70 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/12526275.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: Build - Make - FAIL
Desc: Build the BlueZ source tree
Output:
src/gatt-database.c: In function ‘service_add_ccc’:
src/gatt-database.c:1060:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
 1060 |  uint32_t permissions = BT_ATT_PERM_READ | BT_ATT_PERM_WRITE;
      |  ^~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:9212: src/bluetoothd-gatt-database.o] Error 1
make: *** [Makefile:4151: all] Error 2


##############################
Test: Make Check - FAIL
Desc: Run 'make check'
Output:
src/gatt-database.c: In function ‘service_add_ccc’:
src/gatt-database.c:1060:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
 1060 |  uint32_t permissions = BT_ATT_PERM_READ | BT_ATT_PERM_WRITE;
      |  ^~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:9212: src/bluetoothd-gatt-database.o] Error 1
make: *** [Makefile:10443: check] Error 2


##############################
Test: Build w/ext ELL - Make - FAIL
Desc: Build BlueZ source with '--enable-external-ell' configuration
Output:
src/gatt-database.c: In function ‘service_add_ccc’:
src/gatt-database.c:1060:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
 1060 |  uint32_t permissions = BT_ATT_PERM_READ | BT_ATT_PERM_WRITE;
      |  ^~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:9212: src/bluetoothd-gatt-database.o] Error 1
make: *** [Makefile:4151: all] Error 2




---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ 2/2] doc/gatt-api: Add 'X-asynchronous` permissions

[Luiz Augusto von Dentz]

Hi Dagan,

On Wed, Sep 29, 2021 at 10:42 AM Dagan Martinez <dmartinez@starry.com> wrote:
>
> Update docs to reflect the addition of `X-asynchronous` permissions,
> which allow a GATT server to restrict CCC write permissions via
> permissions set on its associated characteristic.
> ---
>  doc/gatt-api.txt | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/doc/gatt-api.txt b/doc/gatt-api.txt
> index 04789c6d3..2550510ba 100644
> --- a/doc/gatt-api.txt
> +++ b/doc/gatt-api.txt
> @@ -253,7 +253,13 @@ Properties string UUID [read-only]
>                         Defines how the characteristic value can be used. See
>                         Core spec "Table 3.5: Characteristic Properties bit
>                         field", and "Table 3.8: Characteristic Extended
> -                       Properties bit field". Allowed values:
> +                       Properties bit field".
> +
> +                       The "x-asynchronous" flags allow a characteristic to impose write
> +                       restrictions on its client characteristic configuration descriptor,
> +                       if applicable, restricting access to notifications and indications.

Don't really like the asynchronous name, it doesn't really reflect
what we want to accomplish.

> +                       Allowed values:
>
>                                 "broadcast"
>                                 "read"
> @@ -267,10 +273,13 @@ Properties        string UUID [read-only]
>                                 "writable-auxiliaries"
>                                 "encrypt-read"
>                                 "encrypt-write"
> +                               "encrypt-asynchronous" (Server only)
>                                 "encrypt-authenticated-read"
>                                 "encrypt-authenticated-write"
> +                               "encrypt-authenticated-asynchronous" (Server only)

I think for CCC we may just inherit the read/write permissions, or
have it as encrypt-notify/encrypt-indicate, etc.

>                                 "secure-read" (Server only)
>                                 "secure-write" (Server only)
> +                               "secure-asynchronous" (Server only)
>                                 "authorize"
>
>                 uint16 Handle [read-write, optional] (Server Only)
> --
> 2.31.1
>


-- 
Luiz Augusto von Dentz