[PATCH BlueZ 0/1] Fixed a buffer overflow found by OSS-Fuzz.

[PATCH BlueZ 0/1] Fixed a buffer overflow found by OSS-Fuzz.

[Oliver Chang]

This fixes an old issue that OSS-Fuzz:
https://issues.oss-fuzz.com/issues/42516062

Oliver Chang (1):
  Fixed heap-buffer-overflow in `compute_seq_size`.

 src/sdp-xml.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

-- 
2.50.1.703.g449372360f-goog

[PATCH BlueZ 1/1] Fixed heap-buffer-overflow in `compute_seq_size`.

[Oliver Chang]

By adding checks for sequence/alternate types in element_end and
ensuring proper cleanup in sdp_xml_parse_record error path.

https://issues.oss-fuzz.com/issues/42516062
https://oss-fuzz.com/testcase-detail/5896441415729152
---
 src/sdp-xml.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/src/sdp-xml.c b/src/sdp-xml.c
index 5efa62ab8..b10882db2 100644
--- a/src/sdp-xml.c
+++ b/src/sdp-xml.c
@@ -545,6 +545,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,
+					"Unexpected data type %d for sequence",
+					ctx_data->stack_head->data->dtd);
+			return;
+		}
+
 		ctx_data->stack_head->data->unitSize = compute_seq_size(ctx_data->stack_head->data);
 
 		if (ctx_data->stack_head->data->unitSize > USHRT_MAX) {
@@ -557,6 +564,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,
+					"Unexpected data type %d for alternate",
+					ctx_data->stack_head->data->dtd);
+			return;
+		}
+
 		ctx_data->stack_head->data->unitSize = compute_seq_size(ctx_data->stack_head->data);
 
 		if (ctx_data->stack_head->data->unitSize > USHRT_MAX) {
@@ -621,6 +635,11 @@ sdp_record_t *sdp_xml_parse_record(const char *data, int size)
 	if (g_markup_parse_context_parse(ctx, data, size, NULL) == FALSE) {
 		error("XML parsing error");
 		g_markup_parse_context_free(ctx);
+		while (ctx_data->stack_head) {
+			struct sdp_xml_data *elem = ctx_data->stack_head;
+			ctx_data->stack_head = elem->next;
+			sdp_xml_data_free(elem);
+		}
 		sdp_record_free(record);
 		free(ctx_data);
 		return NULL;
-- 
2.50.1.703.g449372360f-goog

RE: Fixed a buffer overflow found by OSS-Fuzz.

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    PENDING   0.31 seconds
GitLint                       PENDING   0.27 seconds
BuildEll                      PASS      20.16 seconds
BluezMake                     PASS      2672.13 seconds
MakeCheck                     PASS      20.50 seconds
MakeDistcheck                 PASS      189.00 seconds
CheckValgrind                 PASS      235.94 seconds
CheckSmatch                   PASS      307.17 seconds
bluezmakeextell               PASS      128.65 seconds
IncrementalBuild              PENDING   0.34 seconds
ScanBuild                     PASS      906.31 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ 1/1] Fixed heap-buffer-overflow in `compute_seq_size`.

[Bastien Nocera]

On Fri, 2025-08-08 at 01:34 +0000, Oliver Chang wrote:
> By adding checks for sequence/alternate types in element_end and
> ensuring proper cleanup in sdp_xml_parse_record error path.
> 
> https://issues.oss-fuzz.com/issues/42516062
> https://oss-fuzz.com/testcase-detail/5896441415729152

Neither of those URLs show any details about the problem that was
detected.

It would be useful if there was a regression test in unit/ either in
unit/sdp.c or as a separate sdp-xml.c test that, for example, showed
the lost memory in a valgrind run, which the patch would fix.

Cheers

> ---
>  src/sdp-xml.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/src/sdp-xml.c b/src/sdp-xml.c
> index 5efa62ab8..b10882db2 100644
> --- a/src/sdp-xml.c
> +++ b/src/sdp-xml.c
> @@ -545,6 +545,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,
> +					"Unexpected data type %d for
> sequence",
> +					ctx_data->stack_head->data-
> >dtd);
> +			return;
> +		}
> +
>  		ctx_data->stack_head->data->unitSize =
> compute_seq_size(ctx_data->stack_head->data);
>  
>  		if (ctx_data->stack_head->data->unitSize >
> USHRT_MAX) {
> @@ -557,6 +564,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,
> +					"Unexpected data type %d for
> alternate",
> +					ctx_data->stack_head->data-
> >dtd);
> +			return;
> +		}
> +
>  		ctx_data->stack_head->data->unitSize =
> compute_seq_size(ctx_data->stack_head->data);
>  
>  		if (ctx_data->stack_head->data->unitSize >
> USHRT_MAX) {
> @@ -621,6 +635,11 @@ sdp_record_t *sdp_xml_parse_record(const char
> *data, int size)
>  	if (g_markup_parse_context_parse(ctx, data, size, NULL) ==
> FALSE) {
>  		error("XML parsing error");
>  		g_markup_parse_context_free(ctx);
> +		while (ctx_data->stack_head) {
> +			struct sdp_xml_data *elem = ctx_data-
> >stack_head;
> +			ctx_data->stack_head = elem->next;
> +			sdp_xml_data_free(elem);
> +		}
>  		sdp_record_free(record);
>  		free(ctx_data);
>  		return NULL;

Re: [PATCH BlueZ 1/1] Fixed heap-buffer-overflow in `compute_seq_size`.

[Oliver Chang]

(Apologies for the noise, I'm new to this. One more attempt to resend
this as text-only for those who have seen this email multiple times).

Thank you for the feedback. The problem here is that there is a heap
buffer overflow found by fuzzing with the following testcase:

`<sequence><foo/><text/></sequence>`

This causes the `compute_seq_size(ctx_data->stack_head->data);` to be
called on `ctx_data->stack_head->data` that isn't a sequence type.
This patch adds some type checks to guard against that.

I don't believe a regression test using valgrind would catch this --
we used AddressSanitizer to detect this.

While fixing this, we also discovered a memory leak in the error
handling path touched by the patch (` if
(g_markup_parse_context_parse(ctx, data, size, NULL) == FALSE) `),
which we included a fix for.
Would it be better if we separated out the heap buffer overflow fix
and the memory leak fix into 2 separate commits?

Best,
Oliver

Re: [PATCH BlueZ 1/1] Fixed heap-buffer-overflow in `compute_seq_size`.

[Bastien Nocera]

On Fri, 2025-08-08 at 21:45 +1000, Oliver Chang wrote:
> (Apologies for the noise, I'm new to this. One more attempt to resend
> this as text-only for those who have seen this email multiple times).
> 
> Thank you for the feedback. The problem here is that there is a heap
> buffer overflow found by fuzzing with the following testcase:
> 
> `<sequence><foo/><text/></sequence>`
> 
> This causes the `compute_seq_size(ctx_data->stack_head->data);` to be
> called on `ctx_data->stack_head->data` that isn't a sequence type.
> This patch adds some type checks to guard against that.
> 
> I don't believe a regression test using valgrind would catch this --
> we used AddressSanitizer to detect this.

I meant that there should be:
- a test for SDP XML
- valgrind run of that shows this specific memory being leaked (don't
need ASAN for that...)
- patch that fixes the leak with the section of the valgrind log 

We don't need the memory leak itself to be regression tested, don't
think it's something that's easy to put in place right now.

> While fixing this, we also discovered a memory leak in the error
> handling path touched by the patch (` if
> (g_markup_parse_context_parse(ctx, data, size, NULL) == FALSE) `),
> which we included a fix for.
> Would it be better if we separated out the heap buffer overflow fix
> and the memory leak fix into 2 separate commits?

Separate commits would be useful.