linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH BlueZ v2] Cleaning up gatt operations on a disconnect to fix possible use after free
@ 2021-09-28 23:00 Bernie Conrad
  2021-09-28 23:36 ` [RFC,BlueZ,v2] " bluez.test.bot
  2021-09-29 21:05 ` [RFC PATCH BlueZ v2] " Luiz Augusto von Dentz
  0 siblings, 2 replies; 4+ messages in thread
From: Bernie Conrad @ 2021-09-28 23:00 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Bernie Conrad

There is a current use after free possible on a gatt server if a client
disconnects while a WriteValue call is being processed with dbus.

This patch includes the addition of a pending disconnect callback to handle
cleanup better if a disconnect occurs during a write, an acquire write
or read operation using bt_att_register_disconnect with the cb.

v2: Fixed a bad call to pending_read_new

---
 src/gatt-database.c | 115 +++++++++++++++++++++++++-------------------
 1 file changed, 65 insertions(+), 50 deletions(-)

diff --git a/src/gatt-database.c b/src/gatt-database.c
index 1f7ce5f02..a03c579cf 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -933,6 +933,24 @@ static struct btd_device *att_get_device(struct bt_att *att)
 	return btd_adapter_find_device(adapter, &dst, dst_type);
 }
 
+
+static void pending_op_free(void *data)
+{
+	struct pending_op *op = data;
+
+	if (op->owner_queue)
+		queue_remove(op->owner_queue, op);
+
+	free(op);
+}
+
+static void pending_disconnect_cb(int err, void *user_data)
+{
+	struct pending_op *op = user_data;
+
+	op->owner_queue = NULL;
+}
+
 static struct pending_op *pending_ccc_new(struct bt_att *att,
 					struct gatt_db_attribute *attrib,
 					uint16_t value,
@@ -956,17 +974,12 @@ static struct pending_op *pending_ccc_new(struct bt_att *att,
 	op->attrib = attrib;
 	op->link_type = link_type;
 
-	return op;
-}
+	bt_att_register_disconnect(att,
+				   pending_disconnect_cb,
+				   op,
+				   NULL);
 
-static void pending_op_free(void *data)
-{
-	struct pending_op *op = data;
-
-	if (op->owner_queue)
-		queue_remove(op->owner_queue, op);
-
-	free(op);
+	return op;
 }
 
 static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib,
@@ -2169,24 +2182,26 @@ done:
 	gatt_db_attribute_read_result(op->attrib, op->id, ecode, value, len);
 }
 
-static struct pending_op *pending_read_new(struct btd_device *device,
+
+static struct pending_op *pending_read_new(struct bt_att *att,
 					struct queue *owner_queue,
 					struct gatt_db_attribute *attrib,
-					unsigned int id, uint16_t offset,
-					uint8_t link_type)
+					unsigned int id, uint16_t offset)
 {
 	struct pending_op *op;
 
 	op = new0(struct pending_op, 1);
 
 	op->owner_queue = owner_queue;
-	op->device = device;
+	op->device = att_get_device(att);
 	op->attrib = attrib;
 	op->id = id;
 	op->offset = offset;
-	op->link_type = link_type;
+	op->link_type = bt_att_get_link_type(att);
 	queue_push_tail(owner_queue, op);
 
+	bt_att_register_disconnect(att, pending_disconnect_cb, op, NULL);
+
 	return op;
 }
 
@@ -2243,18 +2258,16 @@ static void read_setup_cb(DBusMessageIter *iter, void *user_data)
 	dbus_message_iter_close_container(iter, &dict);
 }
 
-static struct pending_op *send_read(struct btd_device *device,
+static struct pending_op *send_read(struct bt_att *att,
 					struct gatt_db_attribute *attrib,
 					GDBusProxy *proxy,
 					struct queue *owner_queue,
 					unsigned int id,
-					uint16_t offset,
-					uint8_t link_type)
+					uint16_t offset)
 {
 	struct pending_op *op;
 
-	op = pending_read_new(device, owner_queue, attrib, id, offset,
-							link_type);
+	op = pending_read_new(att, owner_queue, attrib, id, offset);
 
 	if (g_dbus_proxy_method_call(proxy, "ReadValue", read_setup_cb,
 				read_reply_cb, op, pending_op_free) == TRUE)
@@ -2337,15 +2350,17 @@ static void write_reply_cb(DBusMessage *message, void *user_data)
 	}
 
 done:
-	gatt_db_attribute_write_result(op->attrib, op->id, ecode);
+	// Make sure that only reply if the device is connected
+	if (btd_device_is_connected(op->device))
+		gatt_db_attribute_write_result(op->attrib, op->id, ecode);
 }
 
-static struct pending_op *pending_write_new(struct btd_device *device,
+static struct pending_op *pending_write_new(struct bt_att *att,
 					struct queue *owner_queue,
 					struct gatt_db_attribute *attrib,
 					unsigned int id,
 					const uint8_t *value, size_t len,
-					uint16_t offset, uint8_t link_type,
+					uint16_t offset,
 					bool is_characteristic,
 					bool prep_authorize)
 {
@@ -2356,33 +2371,37 @@ static struct pending_op *pending_write_new(struct btd_device *device,
 	op->data.iov_base = (uint8_t *) value;
 	op->data.iov_len = len;
 
-	op->device = device;
+	op->device = att_get_device(att);
 	op->owner_queue = owner_queue;
 	op->attrib = attrib;
 	op->id = id;
 	op->offset = offset;
-	op->link_type = link_type;
+	op->link_type = bt_att_get_link_type(att);
 	op->is_characteristic = is_characteristic;
 	op->prep_authorize = prep_authorize;
 	queue_push_tail(owner_queue, op);
 
+	bt_att_register_disconnect(att,
+			    pending_disconnect_cb,
+			    op, NULL);
+
 	return op;
 }
 
-static struct pending_op *send_write(struct btd_device *device,
+static struct pending_op *send_write(struct bt_att *att,
 					struct gatt_db_attribute *attrib,
 					GDBusProxy *proxy,
 					struct queue *owner_queue,
 					unsigned int id,
 					const uint8_t *value, size_t len,
-					uint16_t offset, uint8_t link_type,
+					uint16_t offset,
 					bool is_characteristic,
 					bool prep_authorize)
 {
 	struct pending_op *op;
 
-	op = pending_write_new(device, owner_queue, attrib, id, value, len,
-					offset, link_type, is_characteristic,
+	op = pending_write_new(att, owner_queue, attrib, id, value, len,
+					offset, is_characteristic,
 					prep_authorize);
 
 	if (g_dbus_proxy_method_call(proxy, "WriteValue", write_setup_cb,
@@ -2558,17 +2577,16 @@ static void acquire_write_setup(DBusMessageIter *iter, void *user_data)
 }
 
 static struct pending_op *acquire_write(struct external_chrc *chrc,
-					struct btd_device *device,
+					struct bt_att *att,
 					struct gatt_db_attribute *attrib,
 					unsigned int id,
-					const uint8_t *value, size_t len,
-					uint8_t link_type)
+					const uint8_t *value, size_t len)
 {
 	struct pending_op *op;
 	bool acquiring = !queue_isempty(chrc->pending_writes);
 
-	op = pending_write_new(device, chrc->pending_writes, attrib, id, value,
-				len, 0, link_type, false, false);
+	op = pending_write_new(att, chrc->pending_writes, attrib, id, value,
+				len, 0, false, false);
 
 	if (acquiring)
 		return op;
@@ -2851,8 +2869,8 @@ static void desc_read_cb(struct gatt_db_attribute *attrib,
 		goto fail;
 	}
 
-	if (send_read(device, attrib, desc->proxy, desc->pending_reads, id,
-					offset, bt_att_get_link_type(att)))
+	if (send_read(att, attrib, desc->proxy, desc->pending_reads, id,
+					offset))
 		return;
 
 fail:
@@ -2883,10 +2901,9 @@ static void desc_write_cb(struct gatt_db_attribute *attrib,
 	if (opcode == BT_ATT_OP_PREP_WRITE_REQ) {
 		if (!device_is_trusted(device) && !desc->prep_authorized &&
 						desc->req_prep_authorization)
-			send_write(device, attrib, desc->proxy,
+			send_write(att, attrib, desc->proxy,
 					desc->pending_writes, id, value, len,
-					offset, bt_att_get_link_type(att),
-					false, true);
+					offset, false, true);
 		else
 			gatt_db_attribute_write_result(attrib, id, 0);
 
@@ -2896,9 +2913,8 @@ static void desc_write_cb(struct gatt_db_attribute *attrib,
 	if (opcode == BT_ATT_OP_EXEC_WRITE_REQ)
 		desc->prep_authorized = false;
 
-	if (send_write(device, attrib, desc->proxy, desc->pending_writes, id,
-			value, len, offset, bt_att_get_link_type(att), false,
-			false))
+	if (send_write(att, attrib, desc->proxy, desc->pending_writes, id,
+			value, len, offset, false, false))
 		return;
 
 fail:
@@ -2977,8 +2993,8 @@ static void chrc_read_cb(struct gatt_db_attribute *attrib,
 		goto fail;
 	}
 
-	if (send_read(device, attrib, chrc->proxy, chrc->pending_reads, id,
-					offset, bt_att_get_link_type(att)))
+	if (send_read(att, attrib, chrc->proxy, chrc->pending_reads, id,
+	       offset))
 		return;
 
 fail:
@@ -3016,9 +3032,9 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
 	if (opcode == BT_ATT_OP_PREP_WRITE_REQ) {
 		if (!device_is_trusted(device) && !chrc->prep_authorized &&
 						chrc->req_prep_authorization)
-			send_write(device, attrib, chrc->proxy, queue,
+			send_write(att, attrib, chrc->proxy, queue,
 					id, value, len, offset,
-					bt_att_get_link_type(att), true, true);
+					true, true);
 		else
 			gatt_db_attribute_write_result(attrib, id, 0);
 
@@ -3039,13 +3055,12 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
 	}
 
 	if (g_dbus_proxy_get_property(chrc->proxy, "WriteAcquired", &iter)) {
-		if (acquire_write(chrc, device, attrib, id, value, len,
-						bt_att_get_link_type(att)))
+		if (acquire_write(chrc, att, attrib, id, value, len))
 			return;
 	}
 
-	if (send_write(device, attrib, chrc->proxy, queue, id, value, len,
-			offset, bt_att_get_link_type(att), false, false))
+	if (send_write(att, attrib, chrc->proxy, queue, id, value, len,
+			offset, false, false))
 		return;
 
 fail:
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* RE: [RFC,BlueZ,v2] Cleaning up gatt operations on a disconnect to fix possible use after free
  2021-09-28 23:00 [RFC PATCH BlueZ v2] Cleaning up gatt operations on a disconnect to fix possible use after free Bernie Conrad
@ 2021-09-28 23:36 ` bluez.test.bot
  2021-09-29 21:05 ` [RFC PATCH BlueZ v2] " Luiz Augusto von Dentz
  1 sibling, 0 replies; 4+ messages in thread
From: bluez.test.bot @ 2021-09-28 23:36 UTC (permalink / raw)
  To: linux-bluetooth, bernie

[-- Attachment #1: Type: text/plain, Size: 1216 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=554607

---Test result---

Test Summary:
CheckPatch                    PASS      1.56 seconds
GitLint                       FAIL      1.05 seconds
Prep - Setup ELL              PASS      48.61 seconds
Build - Prep                  PASS      0.51 seconds
Build - Configure             PASS      8.91 seconds
Build - Make                  PASS      207.06 seconds
Make Check                    PASS      9.64 seconds
Make Distcheck                PASS      246.35 seconds
Build w/ext ELL - Configure   PASS      9.02 seconds
Build w/ext ELL - Make        PASS      195.19 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint with rule in .gitlint
Output:
[RFC,BlueZ,v2] Cleaning up gatt operations on a disconnect to fix possible use after free
1: T1 Title exceeds max length (89>80): "[RFC,BlueZ,v2] Cleaning up gatt operations on a disconnect to fix possible use after free"




---
Regards,
Linux Bluetooth


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC PATCH BlueZ v2] Cleaning up gatt operations on a disconnect to fix possible use after free
  2021-09-28 23:00 [RFC PATCH BlueZ v2] Cleaning up gatt operations on a disconnect to fix possible use after free Bernie Conrad
  2021-09-28 23:36 ` [RFC,BlueZ,v2] " bluez.test.bot
@ 2021-09-29 21:05 ` Luiz Augusto von Dentz
  2021-09-29 22:47   ` Luiz Augusto von Dentz
  1 sibling, 1 reply; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2021-09-29 21:05 UTC (permalink / raw)
  To: Bernie Conrad; +Cc: linux-bluetooth

Hi Bernie,

On Tue, Sep 28, 2021 at 4:01 PM Bernie Conrad <bernie@allthenticate.net> wrote:
>
> There is a current use after free possible on a gatt server if a client
> disconnects while a WriteValue call is being processed with dbus.
>
> This patch includes the addition of a pending disconnect callback to handle
> cleanup better if a disconnect occurs during a write, an acquire write
> or read operation using bt_att_register_disconnect with the cb.
>
> v2: Fixed a bad call to pending_read_new
>
> ---
>  src/gatt-database.c | 115 +++++++++++++++++++++++++-------------------
>  1 file changed, 65 insertions(+), 50 deletions(-)
>
> diff --git a/src/gatt-database.c b/src/gatt-database.c
> index 1f7ce5f02..a03c579cf 100644
> --- a/src/gatt-database.c
> +++ b/src/gatt-database.c
> @@ -933,6 +933,24 @@ static struct btd_device *att_get_device(struct bt_att *att)
>         return btd_adapter_find_device(adapter, &dst, dst_type);
>  }
>
> +
> +static void pending_op_free(void *data)
> +{
> +       struct pending_op *op = data;
> +
> +       if (op->owner_queue)
> +               queue_remove(op->owner_queue, op);
> +
> +       free(op);

We might need to do something like the following:

https://gist.github.com/Vudentz/958d540591d1ae6a32d226ef8a0d6d54

Otherwise we risk the op being freeing when the op is completely
properly but when att is disconnected it still runs
pending_disconnect_cb causing a UAF.

> +}
> +
> +static void pending_disconnect_cb(int err, void *user_data)
> +{
> +       struct pending_op *op = user_data;
> +
> +       op->owner_queue = NULL;
> +}
> +
>  static struct pending_op *pending_ccc_new(struct bt_att *att,
>                                         struct gatt_db_attribute *attrib,
>                                         uint16_t value,
> @@ -956,17 +974,12 @@ static struct pending_op *pending_ccc_new(struct bt_att *att,
>         op->attrib = attrib;
>         op->link_type = link_type;
>
> -       return op;
> -}
> +       bt_att_register_disconnect(att,
> +                                  pending_disconnect_cb,
> +                                  op,
> +                                  NULL);
>
> -static void pending_op_free(void *data)
> -{
> -       struct pending_op *op = data;
> -
> -       if (op->owner_queue)
> -               queue_remove(op->owner_queue, op);
> -
> -       free(op);
> +       return op;
>  }
>
>  static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib,
> @@ -2169,24 +2182,26 @@ done:
>         gatt_db_attribute_read_result(op->attrib, op->id, ecode, value, len);
>  }
>
> -static struct pending_op *pending_read_new(struct btd_device *device,
> +
> +static struct pending_op *pending_read_new(struct bt_att *att,
>                                         struct queue *owner_queue,
>                                         struct gatt_db_attribute *attrib,
> -                                       unsigned int id, uint16_t offset,
> -                                       uint8_t link_type)
> +                                       unsigned int id, uint16_t offset)
>  {
>         struct pending_op *op;
>
>         op = new0(struct pending_op, 1);
>
>         op->owner_queue = owner_queue;
> -       op->device = device;
> +       op->device = att_get_device(att);
>         op->attrib = attrib;
>         op->id = id;
>         op->offset = offset;
> -       op->link_type = link_type;
> +       op->link_type = bt_att_get_link_type(att);
>         queue_push_tail(owner_queue, op);
>
> +       bt_att_register_disconnect(att, pending_disconnect_cb, op, NULL);
> +
>         return op;
>  }
>
> @@ -2243,18 +2258,16 @@ static void read_setup_cb(DBusMessageIter *iter, void *user_data)
>         dbus_message_iter_close_container(iter, &dict);
>  }
>
> -static struct pending_op *send_read(struct btd_device *device,
> +static struct pending_op *send_read(struct bt_att *att,
>                                         struct gatt_db_attribute *attrib,
>                                         GDBusProxy *proxy,
>                                         struct queue *owner_queue,
>                                         unsigned int id,
> -                                       uint16_t offset,
> -                                       uint8_t link_type)
> +                                       uint16_t offset)
>  {
>         struct pending_op *op;
>
> -       op = pending_read_new(device, owner_queue, attrib, id, offset,
> -                                                       link_type);
> +       op = pending_read_new(att, owner_queue, attrib, id, offset);
>
>         if (g_dbus_proxy_method_call(proxy, "ReadValue", read_setup_cb,
>                                 read_reply_cb, op, pending_op_free) == TRUE)
> @@ -2337,15 +2350,17 @@ static void write_reply_cb(DBusMessage *message, void *user_data)
>         }
>
>  done:
> -       gatt_db_attribute_write_result(op->attrib, op->id, ecode);
> +       // Make sure that only reply if the device is connected
> +       if (btd_device_is_connected(op->device))
> +               gatt_db_attribute_write_result(op->attrib, op->id, ecode);
>  }
>
> -static struct pending_op *pending_write_new(struct btd_device *device,
> +static struct pending_op *pending_write_new(struct bt_att *att,
>                                         struct queue *owner_queue,
>                                         struct gatt_db_attribute *attrib,
>                                         unsigned int id,
>                                         const uint8_t *value, size_t len,
> -                                       uint16_t offset, uint8_t link_type,
> +                                       uint16_t offset,
>                                         bool is_characteristic,
>                                         bool prep_authorize)
>  {
> @@ -2356,33 +2371,37 @@ static struct pending_op *pending_write_new(struct btd_device *device,
>         op->data.iov_base = (uint8_t *) value;
>         op->data.iov_len = len;
>
> -       op->device = device;
> +       op->device = att_get_device(att);
>         op->owner_queue = owner_queue;
>         op->attrib = attrib;
>         op->id = id;
>         op->offset = offset;
> -       op->link_type = link_type;
> +       op->link_type = bt_att_get_link_type(att);
>         op->is_characteristic = is_characteristic;
>         op->prep_authorize = prep_authorize;
>         queue_push_tail(owner_queue, op);
>
> +       bt_att_register_disconnect(att,
> +                           pending_disconnect_cb,
> +                           op, NULL);
> +
>         return op;
>  }
>
> -static struct pending_op *send_write(struct btd_device *device,
> +static struct pending_op *send_write(struct bt_att *att,
>                                         struct gatt_db_attribute *attrib,
>                                         GDBusProxy *proxy,
>                                         struct queue *owner_queue,
>                                         unsigned int id,
>                                         const uint8_t *value, size_t len,
> -                                       uint16_t offset, uint8_t link_type,
> +                                       uint16_t offset,
>                                         bool is_characteristic,
>                                         bool prep_authorize)
>  {
>         struct pending_op *op;
>
> -       op = pending_write_new(device, owner_queue, attrib, id, value, len,
> -                                       offset, link_type, is_characteristic,
> +       op = pending_write_new(att, owner_queue, attrib, id, value, len,
> +                                       offset, is_characteristic,
>                                         prep_authorize);
>
>         if (g_dbus_proxy_method_call(proxy, "WriteValue", write_setup_cb,
> @@ -2558,17 +2577,16 @@ static void acquire_write_setup(DBusMessageIter *iter, void *user_data)
>  }
>
>  static struct pending_op *acquire_write(struct external_chrc *chrc,
> -                                       struct btd_device *device,
> +                                       struct bt_att *att,
>                                         struct gatt_db_attribute *attrib,
>                                         unsigned int id,
> -                                       const uint8_t *value, size_t len,
> -                                       uint8_t link_type)
> +                                       const uint8_t *value, size_t len)
>  {
>         struct pending_op *op;
>         bool acquiring = !queue_isempty(chrc->pending_writes);
>
> -       op = pending_write_new(device, chrc->pending_writes, attrib, id, value,
> -                               len, 0, link_type, false, false);
> +       op = pending_write_new(att, chrc->pending_writes, attrib, id, value,
> +                               len, 0, false, false);
>
>         if (acquiring)
>                 return op;
> @@ -2851,8 +2869,8 @@ static void desc_read_cb(struct gatt_db_attribute *attrib,
>                 goto fail;
>         }
>
> -       if (send_read(device, attrib, desc->proxy, desc->pending_reads, id,
> -                                       offset, bt_att_get_link_type(att)))
> +       if (send_read(att, attrib, desc->proxy, desc->pending_reads, id,
> +                                       offset))
>                 return;
>
>  fail:
> @@ -2883,10 +2901,9 @@ static void desc_write_cb(struct gatt_db_attribute *attrib,
>         if (opcode == BT_ATT_OP_PREP_WRITE_REQ) {
>                 if (!device_is_trusted(device) && !desc->prep_authorized &&
>                                                 desc->req_prep_authorization)
> -                       send_write(device, attrib, desc->proxy,
> +                       send_write(att, attrib, desc->proxy,
>                                         desc->pending_writes, id, value, len,
> -                                       offset, bt_att_get_link_type(att),
> -                                       false, true);
> +                                       offset, false, true);
>                 else
>                         gatt_db_attribute_write_result(attrib, id, 0);
>
> @@ -2896,9 +2913,8 @@ static void desc_write_cb(struct gatt_db_attribute *attrib,
>         if (opcode == BT_ATT_OP_EXEC_WRITE_REQ)
>                 desc->prep_authorized = false;
>
> -       if (send_write(device, attrib, desc->proxy, desc->pending_writes, id,
> -                       value, len, offset, bt_att_get_link_type(att), false,
> -                       false))
> +       if (send_write(att, attrib, desc->proxy, desc->pending_writes, id,
> +                       value, len, offset, false, false))
>                 return;
>
>  fail:
> @@ -2977,8 +2993,8 @@ static void chrc_read_cb(struct gatt_db_attribute *attrib,
>                 goto fail;
>         }
>
> -       if (send_read(device, attrib, chrc->proxy, chrc->pending_reads, id,
> -                                       offset, bt_att_get_link_type(att)))
> +       if (send_read(att, attrib, chrc->proxy, chrc->pending_reads, id,
> +              offset))
>                 return;
>
>  fail:
> @@ -3016,9 +3032,9 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
>         if (opcode == BT_ATT_OP_PREP_WRITE_REQ) {
>                 if (!device_is_trusted(device) && !chrc->prep_authorized &&
>                                                 chrc->req_prep_authorization)
> -                       send_write(device, attrib, chrc->proxy, queue,
> +                       send_write(att, attrib, chrc->proxy, queue,
>                                         id, value, len, offset,
> -                                       bt_att_get_link_type(att), true, true);
> +                                       true, true);
>                 else
>                         gatt_db_attribute_write_result(attrib, id, 0);
>
> @@ -3039,13 +3055,12 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
>         }
>
>         if (g_dbus_proxy_get_property(chrc->proxy, "WriteAcquired", &iter)) {
> -               if (acquire_write(chrc, device, attrib, id, value, len,
> -                                               bt_att_get_link_type(att)))
> +               if (acquire_write(chrc, att, attrib, id, value, len))
>                         return;
>         }
>
> -       if (send_write(device, attrib, chrc->proxy, queue, id, value, len,
> -                       offset, bt_att_get_link_type(att), false, false))
> +       if (send_write(att, attrib, chrc->proxy, queue, id, value, len,
> +                       offset, false, false))
>                 return;
>
>  fail:
> --
> 2.17.1
>


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [RFC PATCH BlueZ v2] Cleaning up gatt operations on a disconnect to fix possible use after free
  2021-09-29 21:05 ` [RFC PATCH BlueZ v2] " Luiz Augusto von Dentz
@ 2021-09-29 22:47   ` Luiz Augusto von Dentz
  0 siblings, 0 replies; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2021-09-29 22:47 UTC (permalink / raw)
  To: Bernie Conrad; +Cc: linux-bluetooth

Hi Bernie,

On Wed, Sep 29, 2021 at 2:05 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Bernie,
>
> On Tue, Sep 28, 2021 at 4:01 PM Bernie Conrad <bernie@allthenticate.net> wrote:
> >
> > There is a current use after free possible on a gatt server if a client
> > disconnects while a WriteValue call is being processed with dbus.
> >
> > This patch includes the addition of a pending disconnect callback to handle
> > cleanup better if a disconnect occurs during a write, an acquire write
> > or read operation using bt_att_register_disconnect with the cb.
> >
> > v2: Fixed a bad call to pending_read_new
> >
> > ---
> >  src/gatt-database.c | 115 +++++++++++++++++++++++++-------------------
> >  1 file changed, 65 insertions(+), 50 deletions(-)
> >
> > diff --git a/src/gatt-database.c b/src/gatt-database.c
> > index 1f7ce5f02..a03c579cf 100644
> > --- a/src/gatt-database.c
> > +++ b/src/gatt-database.c
> > @@ -933,6 +933,24 @@ static struct btd_device *att_get_device(struct bt_att *att)
> >         return btd_adapter_find_device(adapter, &dst, dst_type);
> >  }
> >
> > +
> > +static void pending_op_free(void *data)
> > +{
> > +       struct pending_op *op = data;
> > +
> > +       if (op->owner_queue)
> > +               queue_remove(op->owner_queue, op);
> > +
> > +       free(op);
>
> We might need to do something like the following:
>
> https://gist.github.com/Vudentz/958d540591d1ae6a32d226ef8a0d6d54
>
> Otherwise we risk the op being freeing when the op is completely
> properly but when att is disconnected it still runs
> pending_disconnect_cb causing a UAF.
>
> > +}
> > +
> > +static void pending_disconnect_cb(int err, void *user_data)
> > +{
> > +       struct pending_op *op = user_data;
> > +
> > +       op->owner_queue = NULL;
> > +}
> > +
> >  static struct pending_op *pending_ccc_new(struct bt_att *att,
> >                                         struct gatt_db_attribute *attrib,
> >                                         uint16_t value,
> > @@ -956,17 +974,12 @@ static struct pending_op *pending_ccc_new(struct bt_att *att,
> >         op->attrib = attrib;
> >         op->link_type = link_type;
> >
> > -       return op;
> > -}
> > +       bt_att_register_disconnect(att,
> > +                                  pending_disconnect_cb,
> > +                                  op,
> > +                                  NULL);
> >
> > -static void pending_op_free(void *data)
> > -{
> > -       struct pending_op *op = data;
> > -
> > -       if (op->owner_queue)
> > -               queue_remove(op->owner_queue, op);
> > -
> > -       free(op);
> > +       return op;
> >  }
> >
> >  static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib,
> > @@ -2169,24 +2182,26 @@ done:
> >         gatt_db_attribute_read_result(op->attrib, op->id, ecode, value, len);
> >  }
> >
> > -static struct pending_op *pending_read_new(struct btd_device *device,
> > +
> > +static struct pending_op *pending_read_new(struct bt_att *att,
> >                                         struct queue *owner_queue,
> >                                         struct gatt_db_attribute *attrib,
> > -                                       unsigned int id, uint16_t offset,
> > -                                       uint8_t link_type)
> > +                                       unsigned int id, uint16_t offset)
> >  {
> >         struct pending_op *op;
> >
> >         op = new0(struct pending_op, 1);
> >
> >         op->owner_queue = owner_queue;
> > -       op->device = device;
> > +       op->device = att_get_device(att);
> >         op->attrib = attrib;
> >         op->id = id;
> >         op->offset = offset;
> > -       op->link_type = link_type;
> > +       op->link_type = bt_att_get_link_type(att);
> >         queue_push_tail(owner_queue, op);
> >
> > +       bt_att_register_disconnect(att, pending_disconnect_cb, op, NULL);
> > +
> >         return op;
> >  }
> >
> > @@ -2243,18 +2258,16 @@ static void read_setup_cb(DBusMessageIter *iter, void *user_data)
> >         dbus_message_iter_close_container(iter, &dict);
> >  }
> >
> > -static struct pending_op *send_read(struct btd_device *device,
> > +static struct pending_op *send_read(struct bt_att *att,
> >                                         struct gatt_db_attribute *attrib,
> >                                         GDBusProxy *proxy,
> >                                         struct queue *owner_queue,
> >                                         unsigned int id,
> > -                                       uint16_t offset,
> > -                                       uint8_t link_type)
> > +                                       uint16_t offset)
> >  {
> >         struct pending_op *op;
> >
> > -       op = pending_read_new(device, owner_queue, attrib, id, offset,
> > -                                                       link_type);
> > +       op = pending_read_new(att, owner_queue, attrib, id, offset);
> >
> >         if (g_dbus_proxy_method_call(proxy, "ReadValue", read_setup_cb,
> >                                 read_reply_cb, op, pending_op_free) == TRUE)
> > @@ -2337,15 +2350,17 @@ static void write_reply_cb(DBusMessage *message, void *user_data)
> >         }
> >
> >  done:
> > -       gatt_db_attribute_write_result(op->attrib, op->id, ecode);
> > +       // Make sure that only reply if the device is connected
> > +       if (btd_device_is_connected(op->device))
> > +               gatt_db_attribute_write_result(op->attrib, op->id, ecode);
> >  }
> >
> > -static struct pending_op *pending_write_new(struct btd_device *device,
> > +static struct pending_op *pending_write_new(struct bt_att *att,
> >                                         struct queue *owner_queue,
> >                                         struct gatt_db_attribute *attrib,
> >                                         unsigned int id,
> >                                         const uint8_t *value, size_t len,
> > -                                       uint16_t offset, uint8_t link_type,
> > +                                       uint16_t offset,
> >                                         bool is_characteristic,
> >                                         bool prep_authorize)
> >  {
> > @@ -2356,33 +2371,37 @@ static struct pending_op *pending_write_new(struct btd_device *device,
> >         op->data.iov_base = (uint8_t *) value;
> >         op->data.iov_len = len;
> >
> > -       op->device = device;
> > +       op->device = att_get_device(att);
> >         op->owner_queue = owner_queue;
> >         op->attrib = attrib;
> >         op->id = id;
> >         op->offset = offset;
> > -       op->link_type = link_type;
> > +       op->link_type = bt_att_get_link_type(att);
> >         op->is_characteristic = is_characteristic;
> >         op->prep_authorize = prep_authorize;
> >         queue_push_tail(owner_queue, op);
> >
> > +       bt_att_register_disconnect(att,
> > +                           pending_disconnect_cb,
> > +                           op, NULL);
> > +
> >         return op;
> >  }
> >
> > -static struct pending_op *send_write(struct btd_device *device,
> > +static struct pending_op *send_write(struct bt_att *att,
> >                                         struct gatt_db_attribute *attrib,
> >                                         GDBusProxy *proxy,
> >                                         struct queue *owner_queue,
> >                                         unsigned int id,
> >                                         const uint8_t *value, size_t len,
> > -                                       uint16_t offset, uint8_t link_type,
> > +                                       uint16_t offset,
> >                                         bool is_characteristic,
> >                                         bool prep_authorize)
> >  {
> >         struct pending_op *op;
> >
> > -       op = pending_write_new(device, owner_queue, attrib, id, value, len,
> > -                                       offset, link_type, is_characteristic,
> > +       op = pending_write_new(att, owner_queue, attrib, id, value, len,
> > +                                       offset, is_characteristic,
> >                                         prep_authorize);
> >
> >         if (g_dbus_proxy_method_call(proxy, "WriteValue", write_setup_cb,
> > @@ -2558,17 +2577,16 @@ static void acquire_write_setup(DBusMessageIter *iter, void *user_data)
> >  }
> >
> >  static struct pending_op *acquire_write(struct external_chrc *chrc,
> > -                                       struct btd_device *device,
> > +                                       struct bt_att *att,
> >                                         struct gatt_db_attribute *attrib,
> >                                         unsigned int id,
> > -                                       const uint8_t *value, size_t len,
> > -                                       uint8_t link_type)
> > +                                       const uint8_t *value, size_t len)
> >  {
> >         struct pending_op *op;
> >         bool acquiring = !queue_isempty(chrc->pending_writes);
> >
> > -       op = pending_write_new(device, chrc->pending_writes, attrib, id, value,
> > -                               len, 0, link_type, false, false);
> > +       op = pending_write_new(att, chrc->pending_writes, attrib, id, value,
> > +                               len, 0, false, false);
> >
> >         if (acquiring)
> >                 return op;
> > @@ -2851,8 +2869,8 @@ static void desc_read_cb(struct gatt_db_attribute *attrib,
> >                 goto fail;
> >         }
> >
> > -       if (send_read(device, attrib, desc->proxy, desc->pending_reads, id,
> > -                                       offset, bt_att_get_link_type(att)))
> > +       if (send_read(att, attrib, desc->proxy, desc->pending_reads, id,
> > +                                       offset))
> >                 return;
> >
> >  fail:
> > @@ -2883,10 +2901,9 @@ static void desc_write_cb(struct gatt_db_attribute *attrib,
> >         if (opcode == BT_ATT_OP_PREP_WRITE_REQ) {
> >                 if (!device_is_trusted(device) && !desc->prep_authorized &&
> >                                                 desc->req_prep_authorization)
> > -                       send_write(device, attrib, desc->proxy,
> > +                       send_write(att, attrib, desc->proxy,
> >                                         desc->pending_writes, id, value, len,
> > -                                       offset, bt_att_get_link_type(att),
> > -                                       false, true);
> > +                                       offset, false, true);
> >                 else
> >                         gatt_db_attribute_write_result(attrib, id, 0);
> >
> > @@ -2896,9 +2913,8 @@ static void desc_write_cb(struct gatt_db_attribute *attrib,
> >         if (opcode == BT_ATT_OP_EXEC_WRITE_REQ)
> >                 desc->prep_authorized = false;
> >
> > -       if (send_write(device, attrib, desc->proxy, desc->pending_writes, id,
> > -                       value, len, offset, bt_att_get_link_type(att), false,
> > -                       false))
> > +       if (send_write(att, attrib, desc->proxy, desc->pending_writes, id,
> > +                       value, len, offset, false, false))
> >                 return;
> >
> >  fail:
> > @@ -2977,8 +2993,8 @@ static void chrc_read_cb(struct gatt_db_attribute *attrib,
> >                 goto fail;
> >         }
> >
> > -       if (send_read(device, attrib, chrc->proxy, chrc->pending_reads, id,
> > -                                       offset, bt_att_get_link_type(att)))
> > +       if (send_read(att, attrib, chrc->proxy, chrc->pending_reads, id,
> > +              offset))
> >                 return;
> >
> >  fail:
> > @@ -3016,9 +3032,9 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
> >         if (opcode == BT_ATT_OP_PREP_WRITE_REQ) {
> >                 if (!device_is_trusted(device) && !chrc->prep_authorized &&
> >                                                 chrc->req_prep_authorization)
> > -                       send_write(device, attrib, chrc->proxy, queue,
> > +                       send_write(att, attrib, chrc->proxy, queue,
> >                                         id, value, len, offset,
> > -                                       bt_att_get_link_type(att), true, true);
> > +                                       true, true);
> >                 else
> >                         gatt_db_attribute_write_result(attrib, id, 0);
> >
> > @@ -3039,13 +3055,12 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
> >         }
> >
> >         if (g_dbus_proxy_get_property(chrc->proxy, "WriteAcquired", &iter)) {
> > -               if (acquire_write(chrc, device, attrib, id, value, len,
> > -                                               bt_att_get_link_type(att)))
> > +               if (acquire_write(chrc, att, attrib, id, value, len))
> >                         return;
> >         }
> >
> > -       if (send_write(device, attrib, chrc->proxy, queue, id, value, len,
> > -                       offset, bt_att_get_link_type(att), false, false))
> > +       if (send_write(att, attrib, chrc->proxy, queue, id, value, len,
> > +                       offset, false, false))
> >                 return;
> >
> >  fail:
> > --
> > 2.17.1
> >
>
>
> --
> Luiz Augusto von Dentz

I went ahead and applied this with my changes on top, thanks.

-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-09-29 22:48 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-28 23:00 [RFC PATCH BlueZ v2] Cleaning up gatt operations on a disconnect to fix possible use after free Bernie Conrad
2021-09-28 23:36 ` [RFC,BlueZ,v2] " bluez.test.bot
2021-09-29 21:05 ` [RFC PATCH BlueZ v2] " Luiz Augusto von Dentz
2021-09-29 22:47   ` Luiz Augusto von Dentz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).