[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