Fixup 15.1/16.0 backport: system/bt: Fix UAF in gatt_cl.cc

thanks to @syphyr for this!

Signed-off-by: Tad <tad@spotco.us>
This commit is contained in:
Tad 2023-09-12 16:55:46 -04:00
parent 3aa7e02455
commit 5eb6190931
No known key found for this signature in database
GPG Key ID: B286E9F57A07424B
6 changed files with 182 additions and 10 deletions

View File

@ -0,0 +1,80 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Qiyu Hu <qiyuh@google.com>
Date: Wed, 13 Jun 2018 08:08:17 -0700
Subject: [PATCH] Fix reliable write.
We cannot simply assume the write is terminated in reliable write. When
the reliable write value is longer than MTU allows, the current
implementation can only send whatever MTU allows and naively set the
status to GATT_SUCCESS, in the name of "application should verify handle
offset and value are matched or not". That's why MTU negotiation is a
workaround as people mention in b/37031096, which just fits all the write
value into a single request.
This also blocks our test on CtsVerifier.
Bug: 37031096
Test: Manual test and confirm that we don't simply send partial value
Change-Id: I907877608f4672f24c002e630e58bf9133937a5e
---
stack/gatt/gatt_cl.cc | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/stack/gatt/gatt_cl.cc b/stack/gatt/gatt_cl.cc
index dca49738c..2650a307a 100644
--- a/stack/gatt/gatt_cl.cc
+++ b/stack/gatt/gatt_cl.cc
@@ -297,7 +297,7 @@ void gatt_send_queue_write_cancel(tGATT_TCB& tcb, tGATT_CLCB* p_clcb,
bool gatt_check_write_long_terminate(tGATT_TCB& tcb, tGATT_CLCB* p_clcb,
tGATT_VALUE* p_rsp_value) {
tGATT_VALUE* p_attr = (tGATT_VALUE*)p_clcb->p_attr_buf;
- bool exec = false;
+ bool terminate = false;
tGATT_EXEC_FLAG flag = GATT_PREP_WRITE_EXEC;
VLOG(1) << __func__;
@@ -310,19 +310,18 @@ bool gatt_check_write_long_terminate(tGATT_TCB& tcb, tGATT_CLCB* p_clcb,
/* data does not match */
p_clcb->status = GATT_ERROR;
flag = GATT_PREP_WRITE_CANCEL;
- exec = true;
+ terminate = true;
} else /* response checking is good */
{
p_clcb->status = GATT_SUCCESS;
/* update write offset and check if end of attribute value */
- if ((p_attr->offset += p_rsp_value->len) >= p_attr->len) exec = true;
+ if ((p_attr->offset += p_rsp_value->len) >= p_attr->len) terminate = true;
}
}
- if (exec) {
+ if (terminate && p_clcb->op_subtype != GATT_WRITE_PREPARE) {
gatt_send_queue_write_cancel(tcb, p_clcb, flag);
- return true;
}
- return false;
+ return terminate;
}
/** Send prepare write */
@@ -586,15 +585,15 @@ void gatt_process_prep_write_rsp(tGATT_TCB& tcb, tGATT_CLCB* p_clcb,
memcpy(value.value, p, value.len);
+ if (!gatt_check_write_long_terminate(tcb, p_clcb, &value)) {
+ gatt_send_prepare_write(tcb, p_clcb);
+ return;
+ }
+
if (p_clcb->op_subtype == GATT_WRITE_PREPARE) {
- p_clcb->status = GATT_SUCCESS;
/* application should verify handle offset
and value are matched or not */
-
gatt_end_operation(p_clcb, p_clcb->status, &value);
- } else if (p_clcb->op_subtype == GATT_WRITE) {
- if (!gatt_check_write_long_terminate(tcb, p_clcb, &value))
- gatt_send_prepare_write(tcb, p_clcb);
}
}
/*******************************************************************************

View File

@ -20,20 +20,25 @@ Change-Id: I085ecfa1a9ba098ecbfecbd3cb3e263ae13f9724
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/stack/gatt/gatt_cl.cc b/stack/gatt/gatt_cl.cc
index 014240888..305a54660 100644
index db41c5f9f..f7f11b7a9 100644
--- a/stack/gatt/gatt_cl.cc
+++ b/stack/gatt/gatt_cl.cc
@@ -583,7 +583,12 @@ void gatt_process_prep_write_rsp(tGATT_TCB& tcb, tGATT_CLCB* p_clcb,
@@ -586,12 +586,17 @@ void gatt_process_prep_write_rsp(tGATT_TCB& tcb, tGATT_CLCB* p_clcb,
memcpy(value.value, p, value.len);
- if (p_clcb->op_subtype == GATT_WRITE_PREPARE) {
+ bool subtype_is_write_prepare = (p_clcb->op_subtype == GATT_WRITE_PREPARE);
+
if (!gatt_check_write_long_terminate(tcb, p_clcb, &value)) {
gatt_send_prepare_write(tcb, p_clcb);
return;
}
- if (p_clcb->op_subtype == GATT_WRITE_PREPARE) {
+ // We now know that we have not terminated, or else we would have returned
+ // early. We free the buffer only if the subtype is not equal to
+ // GATT_WRITE_PREPARE, so checking here is adequate to prevent UAF.
+ if (subtype_is_write_prepare) {
p_clcb->status = GATT_SUCCESS;
/* application should verify handle offset
and value are matched or not */
gatt_end_operation(p_clcb, p_clcb->status, &value);

View File

@ -0,0 +1,80 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Qiyu Hu <qiyuh@google.com>
Date: Wed, 13 Jun 2018 08:08:17 -0700
Subject: [PATCH] Fix reliable write.
We cannot simply assume the write is terminated in reliable write. When
the reliable write value is longer than MTU allows, the current
implementation can only send whatever MTU allows and naively set the
status to GATT_SUCCESS, in the name of "application should verify handle
offset and value are matched or not". That's why MTU negotiation is a
workaround as people mention in b/37031096, which just fits all the write
value into a single request.
This also blocks our test on CtsVerifier.
Bug: 37031096
Test: Manual test and confirm that we don't simply send partial value
Change-Id: I907877608f4672f24c002e630e58bf9133937a5e
---
stack/gatt/gatt_cl.cc | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/stack/gatt/gatt_cl.cc b/stack/gatt/gatt_cl.cc
index dca49738c..2650a307a 100644
--- a/stack/gatt/gatt_cl.cc
+++ b/stack/gatt/gatt_cl.cc
@@ -297,7 +297,7 @@ void gatt_send_queue_write_cancel(tGATT_TCB& tcb, tGATT_CLCB* p_clcb,
bool gatt_check_write_long_terminate(tGATT_TCB& tcb, tGATT_CLCB* p_clcb,
tGATT_VALUE* p_rsp_value) {
tGATT_VALUE* p_attr = (tGATT_VALUE*)p_clcb->p_attr_buf;
- bool exec = false;
+ bool terminate = false;
tGATT_EXEC_FLAG flag = GATT_PREP_WRITE_EXEC;
VLOG(1) << __func__;
@@ -310,19 +310,18 @@ bool gatt_check_write_long_terminate(tGATT_TCB& tcb, tGATT_CLCB* p_clcb,
/* data does not match */
p_clcb->status = GATT_ERROR;
flag = GATT_PREP_WRITE_CANCEL;
- exec = true;
+ terminate = true;
} else /* response checking is good */
{
p_clcb->status = GATT_SUCCESS;
/* update write offset and check if end of attribute value */
- if ((p_attr->offset += p_rsp_value->len) >= p_attr->len) exec = true;
+ if ((p_attr->offset += p_rsp_value->len) >= p_attr->len) terminate = true;
}
}
- if (exec) {
+ if (terminate && p_clcb->op_subtype != GATT_WRITE_PREPARE) {
gatt_send_queue_write_cancel(tcb, p_clcb, flag);
- return true;
}
- return false;
+ return terminate;
}
/** Send prepare write */
@@ -586,15 +585,15 @@ void gatt_process_prep_write_rsp(tGATT_TCB& tcb, tGATT_CLCB* p_clcb,
memcpy(value.value, p, value.len);
+ if (!gatt_check_write_long_terminate(tcb, p_clcb, &value)) {
+ gatt_send_prepare_write(tcb, p_clcb);
+ return;
+ }
+
if (p_clcb->op_subtype == GATT_WRITE_PREPARE) {
- p_clcb->status = GATT_SUCCESS;
/* application should verify handle offset
and value are matched or not */
-
gatt_end_operation(p_clcb, p_clcb->status, &value);
- } else if (p_clcb->op_subtype == GATT_WRITE) {
- if (!gatt_check_write_long_terminate(tcb, p_clcb, &value))
- gatt_send_prepare_write(tcb, p_clcb);
}
}
/*******************************************************************************

View File

@ -20,20 +20,25 @@ Change-Id: I085ecfa1a9ba098ecbfecbd3cb3e263ae13f9724
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/stack/gatt/gatt_cl.cc b/stack/gatt/gatt_cl.cc
index f8d5bab92..ef77f590d 100644
index db41c5f9f..f7f11b7a9 100644
--- a/stack/gatt/gatt_cl.cc
+++ b/stack/gatt/gatt_cl.cc
@@ -587,7 +587,12 @@ void gatt_process_prep_write_rsp(tGATT_TCB& tcb, tGATT_CLCB* p_clcb,
@@ -586,12 +586,17 @@ void gatt_process_prep_write_rsp(tGATT_TCB& tcb, tGATT_CLCB* p_clcb,
memcpy(value.value, p, value.len);
- if (p_clcb->op_subtype == GATT_WRITE_PREPARE) {
+ bool subtype_is_write_prepare = (p_clcb->op_subtype == GATT_WRITE_PREPARE);
+
if (!gatt_check_write_long_terminate(tcb, p_clcb, &value)) {
gatt_send_prepare_write(tcb, p_clcb);
return;
}
- if (p_clcb->op_subtype == GATT_WRITE_PREPARE) {
+ // We now know that we have not terminated, or else we would have returned
+ // early. We free the buffer only if the subtype is not equal to
+ // GATT_WRITE_PREPARE, so checking here is adequate to prevent UAF.
+ if (subtype_is_write_prepare) {
p_clcb->status = GATT_SUCCESS;
/* application should verify handle offset
and value are matched or not */
gatt_end_operation(p_clcb, p_clcb->status, &value);

View File

@ -449,7 +449,8 @@ applyPatch "$DOS_PATCHES/android_system_bt/360969.patch"; #R_asb_2023-07 Fix gat
applyPatch "$DOS_PATCHES/android_system_bt/365979.patch"; #R_asb_2023-09 Fix an integer overflow bug in avdt_msg_asmbl
applyPatch "$DOS_PATCHES/android_system_bt/365980.patch"; #R_asb_2023-09 Fix integer overflow in build_read_multi_rsp
applyPatch "$DOS_PATCHES/android_system_bt/365981.patch"; #R_asb_2023-09 Fix potential abort in btu_av_act.cc
applyPatch "$DOS_PATCHES/android_system_bt/365982-backport.patch"; #R_asb_2023-09 Fix UAF in gatt_cl.cc
applyPatch "$DOS_PATCHES/android_system_bt/365982-prereq.patch"; #Fix reliable write
applyPatch "$DOS_PATCHES/android_system_bt/365982.patch"; #R_asb_2023-09 Fix UAF in gatt_cl.cc
fi;
if enterAndClear "system/ca-certificates"; then

View File

@ -387,7 +387,8 @@ applyPatch "$DOS_PATCHES/android_system_bt/360969.patch"; #R_asb_2023-07 Fix gat
applyPatch "$DOS_PATCHES/android_system_bt/365979.patch"; #R_asb_2023-09 Fix an integer overflow bug in avdt_msg_asmbl
applyPatch "$DOS_PATCHES/android_system_bt/365980.patch"; #R_asb_2023-09 Fix integer overflow in build_read_multi_rsp
applyPatch "$DOS_PATCHES/android_system_bt/365981.patch"; #R_asb_2023-09 Fix potential abort in btu_av_act.cc
applyPatch "$DOS_PATCHES/android_system_bt/365982-backport.patch"; #R_asb_2023-09 Fix UAF in gatt_cl.cc
applyPatch "$DOS_PATCHES/android_system_bt/365982-prereq.patch"; #Fix reliable write
applyPatch "$DOS_PATCHES/android_system_bt/365982.patch"; #R_asb_2023-09 Fix UAF in gatt_cl.cc
#applyPatch "$DOS_PATCHES_COMMON/android_system_bt/0001-alloc_size.patch"; #Add alloc_size attributes to the allocator (GrapheneOS)
fi;