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

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

[Oliver Chang]

The change from the last patch I submitted is the removal of the memory leak fix.
This patch focuses on fixing the buffer overflow.

This fixes an ASan crash discovered by OSS-Fuzz:

```
==402==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x502000000218 at pc 0x564763e67877 bp 0x7ffc1a9f92b0 sp 0x7ffc1a9f92a8
READ of size 4 at 0x502000000218 thread T0
    #0 0x564763e67876 in compute_seq_size bluez/src/sdp-xml.c:62:21
    #1 0x564763e67876 in element_end bluez/src/sdp-xml.c:548:42
    #2 0x564763ea5416 in emit_end_element glib/glib/gmarkup.c:1045:5
    #3 0x564763ea4978 in g_markup_parse_context_parse glib/glib/gmarkup.c:1603:19
    #4 0x564763e65c55 in sdp_xml_parse_record bluez/src/sdp-xml.c:621:6
    #5 0x564763e6acf1 in LLVMFuzzerTestOneInput /src/fuzz_xml.c:30:9
    #6 0x564763d1a730 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614:13
    #7 0x564763d059a5 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:327:6
    #8 0x564763d0b43f in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:862:9
    #9 0x564763d366e2 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #10 0x79ed69692082 in __libc_start_main /build/glibc-LcI20x/glibc-2.31/csu/libc-start.c:308:16
    #11 0x564763cfdb8d in _start
```

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

 src/sdp-xml.c | 14 ++++++++++++++
 1 file changed, 14 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 to avoid a
type confusion.

This issue was found by OSS-Fuzz.

This can be triggered by using an input of
`<sequence><foo/><text/></sequence>` against the harness in
https://github.com/google/oss-fuzz/blob/master/projects/bluez/fuzz_xml.c

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

diff --git a/src/sdp-xml.c b/src/sdp-xml.c
index 5efa62ab8..81bd11140 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) {
-- 
2.50.1.703.g449372360f-goog

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

[Paul Menzel]

Dear Oliver,


Thank you for the patch. For the summary, I’d use imperative mood and do 
not add a dot/period at the end:

Fix heap-buffer-overflow in `compute_seq_size`

Am 10.08.25 um 08:46 schrieb Oliver Chang:
> By adding checks for sequence/alternate types in element_end to avoid a
> type confusion.
> 
> This issue was found by OSS-Fuzz.
> 
> This can be triggered by using an input of
> `<sequence><foo/><text/></sequence>` against the harness in
> https://github.com/google/oss-fuzz/blob/master/projects/bluez/fuzz_xml.c
> 
> https://issues.oss-fuzz.com/issues/42516062

The last comment says:

> This issue was migrated from crbug.com/oss-fuzz/51480?no_tracker_redirect=1

But that URL gives *Page Not Found*.

> https://oss-fuzz.com/testcase-detail/5896441415729152

I am unable to access this without logging in.

With your patch and the test case, what error is logged now?

> ---
>   src/sdp-xml.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/src/sdp-xml.c b/src/sdp-xml.c
> index 5efa62ab8..81bd11140 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) {


Thank you again and kind regards,

Paul

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

[Oliver Chang]

Dear Paul,

> Thank you for the patch. For the summary, I’d use imperative mood and do
> not add a dot/period at the end:
>
> Fix heap-buffer-overflow in `compute_seq_size`

Thank you for the suggestion, I will make sure to do this for future
commit summaries. What do I need to do here in this instance to make
this change? Do I need to resend the patch with a new subject?

> The last comment says:
>
> > This issue was migrated from crbug.com/oss-fuzz/51480?no_tracker_redirect=1
>
> But that URL gives *Page Not Found*.

Apologies for that. We had a migration from a legacy issue tracking
system, and that legacy system has since been turned down.
>
> > https://oss-fuzz.com/testcase-detail/5896441415729152
>
> I am unable to access this without logging in.

The emails that can access this are the ones listed here:
https://github.com/google/oss-fuzz/blob/ef1c29822d62470fb6b0af7b73412d245d05f80c/projects/bluez/project.yaml#L3.
Are there any other emails we should add?

These emails also receive automatic email notifications whenever
OSS-Fuzz finds a new bug. Note though, to view the oss-fuzz.com
report, you'll need either a GitHub or Google account associated with
the email.

https://oss-fuzz.com/testcase-detail/5896441415729152 contains the
ASan stacktrace, which I've also included in my earlier email.

>
> With your patch and the test case, what error is logged now?

There is still a memory leak detected by ASan that's unrelated to this
patch/issue, but the buffer overflow report is gone.

I don't see any other log messages, including the ones I added in the
patch. This is likely because `sdp_xml_parse_record` calls
`g_markup_parse_context_parse` with a NULL `error`, so any parsing
errors encountered are not propagated.

```
if (g_markup_parse_context_parse(ctx, data, size, NULL) == FALSE) {
```

It seems useful to enable propagating of parsing errors to
`sdp_xml_parse_record` in the future. But if preferred, I can remove
the logging I added since they're not going anywhere right now.

Kind regards,
Oliver

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

[Paul Menzel]

Dear Oliver,


Am 10.08.25 um 10:20 schrieb Oliver Chang:

>> Thank you for the patch. For the summary, I’d use imperative mood and do
>> not add a dot/period at the end:
>>
>> Fix heap-buffer-overflow in `compute_seq_size`
> 
> Thank you for the suggestion, I will make sure to do this for future
> commit summaries. What do I need to do here in this instance to make
> this change? Do I need to resend the patch with a new subject?

If you want to, you could send v2. (`git format-patch -v2`)

>> The last comment says:
>>
>>> This issue was migrated from crbug.com/oss-fuzz/51480?no_tracker_redirect=1
>>
>> But that URL gives *Page Not Found*.
> 
> Apologies for that. We had a migration from a legacy issue tracking
> system, and that legacy system has since been turned down.

Understood. No problem.

>>> https://oss-fuzz.com/testcase-detail/5896441415729152
>>
>> I am unable to access this without logging in.
> 
> The emails that can access this are the ones listed here:
> https://github.com/google/oss-fuzz/blob/ef1c29822d62470fb6b0af7b73412d245d05f80c/projects/bluez/project.yaml#L3.
> Are there any other emails we should add?

I am not a maintainer, so, as these might security sensitive, I guess 
it’s fine, that I am unable to access it in general.

> These emails also receive automatic email notifications whenever
> OSS-Fuzz finds a new bug. Note though, to view the oss-fuzz.com
> report, you'll need either a GitHub or Google account associated with
> the email.
> 
> https://oss-fuzz.com/testcase-detail/5896441415729152 contains the
> ASan stacktrace, which I've also included in my earlier email.

Understood, in the cover letter [1]. Thank you! I personally prefer 
everything to be in the commit message, so it’s self-contained.

>> With your patch and the test case, what error is logged now?
> 
> There is still a memory leak detected by ASan that's unrelated to this
> patch/issue, but the buffer overflow report is gone.
> 
> I don't see any other log messages, including the ones I added in the
> patch. This is likely because `sdp_xml_parse_record` calls
> `g_markup_parse_context_parse` with a NULL `error`, so any parsing
> errors encountered are not propagated.
> 
> ```
> if (g_markup_parse_context_parse(ctx, data, size, NULL) == FALSE) {
> ```
> 
> It seems useful to enable propagating of parsing errors to
> `sdp_xml_parse_record` in the future. But if preferred, I can remove
> the logging I added since they're not going anywhere right now.

Thank you for analyzing this, and giving the reason. I’d keep it like 
this, but add a note to the commit message.


Thank you again and kind regards,

Paul


[1]: 
https://lore.kernel.org/all/20250810064656.1810093-2-ochang@google.com/#t

RE: *** Fixed heap-buffer-overflow in `compute_seq_size` ***

[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=989673

---Test result---

Test Summary:
CheckPatch                    PENDING   0.22 seconds
GitLint                       PENDING   0.23 seconds
BuildEll                      PASS      20.03 seconds
BluezMake                     PASS      2526.03 seconds
MakeCheck                     PASS      19.93 seconds
MakeDistcheck                 PASS      184.27 seconds
CheckValgrind                 PASS      235.46 seconds
CheckSmatch                   PASS      304.85 seconds
bluezmakeextell               PASS      126.86 seconds
IncrementalBuild              PENDING   0.27 seconds
ScanBuild                     PASS      906.18 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

[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: [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.