[BlueZ, v4 2/9] sdp-xml: Fix crash caused by type confusion when parsing crafted SDP XML

Bastien Nocera <[email protected]>
Newsgroups org.kernel.vger.linux-bluetooth
Message-ID <[email protected]>
When element_end() processes </attribute>, it frees ctx_data->stack_head
and clears the stack even if parsing is still nested inside a parent
container.

If a crafted ServiceRecord places a nested <attribute> inside <sequence>,
a later sibling scalar element such as <uint64> can become the new stack
head. When the closing </sequence> is then processed, compute_seq_size()
is reached without first validating that the current node is actually
a sequence.

sdp_data_t.val stores both scalar members such as uint64 and the
dataseq pointer in the same union. As a result, attacker-controlled
scalar data can be reinterpreted as a linked-list pointer and traversed
until bluetoothd crashes.

See https://github.com/bluez/bluez/security/advisories/GHSA-7mmr-gwqx-vc34

Reported-by: Aisle Research
Co-authored-by: Aisle Research
---
 src/sdp-xml.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/src/sdp-xml.c b/src/sdp-xml.c
index e5b30e88505f..c8f9ed013b29 100644
--- a/src/sdp-xml.c
+++ b/src/sdp-xml.c
@@ -529,7 +529,9 @@ static void element_end(GMarkupParseContext *context,
 		return;
 
 	if (!strcmp(element_name, "attribute")) {
-		if (ctx_data->stack_head && ctx_data->stack_head->data) {
+		/* Attributes are expected at top-level record scope. */
+		if (ctx_data->stack_head && ctx_data->stack_head->data &&
+		    ctx_data->stack_head->next == NULL) {
 			int ret = sdp_attr_add(ctx_data->record, ctx_data->attr_id,
 							ctx_data->stack_head->data);
 			if (ret == -1)
@@ -539,6 +541,11 @@ static void element_end(GMarkupParseContext *context,
 			ctx_data->stack_head->data = NULL;
 			sdp_xml_data_free(ctx_data->stack_head);
 			ctx_data->stack_head = NULL;
+		} else if (ctx_data->stack_head && ctx_data->stack_head->next) {
+			g_set_error(err, G_MARKUP_ERROR,
+				    G_MARKUP_ERROR_INVALID_CONTENT,
+				    "Nested <attribute> is invalid");
+			return;
 		} else {
 			DBG("No data for attribute 0x%04x", ctx_data->attr_id);
 		}
@@ -558,6 +565,13 @@ static void element_end(GMarkupParseContext *context,
 	}
 
 	if (!strcmp(element_name, "sequence")) {
+		if (!SDP_IS_SEQ(ctx_data->stack_head->data->dtd)) {
+			g_set_error(err, G_MARKUP_ERROR,
+				    G_MARKUP_ERROR_INVALID_CONTENT,
+				    "Mismatched </sequence> close");
+			return;
+		}
+
 		ctx_data->stack_head->data->unitSize = compute_seq_size(ctx_data->stack_head->data);
 
 		if (ctx_data->stack_head->data->unitSize > USHRT_MAX) {
@@ -570,6 +584,13 @@ static void element_end(GMarkupParseContext *context,
 			ctx_data->stack_head->data->unitSize += sizeof(uint8_t);
 		}
 	} else if (!strcmp(element_name, "alternate")) {
+		if (!SDP_IS_ALT(ctx_data->stack_head->data->dtd)) {
+			g_set_error(err, G_MARKUP_ERROR,
+				    G_MARKUP_ERROR_INVALID_CONTENT,
+				    "Mismatched </alternate> close");
+			return;
+		}
+
 		ctx_data->stack_head->data->unitSize = compute_seq_size(ctx_data->stack_head->data);
 
 		if (ctx_data->stack_head->data->unitSize > USHRT_MAX) {
-- 
2.55.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.