*** Fixed heap-buffer-overflow in `compute_seq_size` ***
cover *** Fixed heap-buffer-overflow in `compute_seq_size` ***
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(+)
Signature · 1 line
2.50.1.703.g449372360f-goog
PATCH 1/1 Fixed heap-buffer-overflow in `compute_seq_size`.
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(+)
src/sdp-xml.c +14 -0
@@ -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 ) {
Signature · 1 line
2.50.1.703.g449372360f-goog
Re: [PATCH BlueZ 1/1] Fixed heap-buffer-overflow in `compute_seq_size`.
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:
Quoted 10 lines
By adding checks for sequence/alternate types in
element_endto 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
The last comment says:
Quoted 1 line
This issue was migrated from crbug.com/oss-fuzz/51480?no_tracker_redirect=1
But that URL gives *Page Not Found*.
I am unable to access this without logging in.
With your patch and the test case, what error is logged now?
Quoted 26 lines
--- src/sdp-xml.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)src/sdp-xml.c +7
-0@@ -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" )) {
src/sdp-xml.c static void element_end(GMarkupParseContext *context, · line 576
Quoted 10 lines
src/sdp-xml.c +7
-0+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`.
Dear Paul,
Quoted 4 lines
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?
Quoted 5 lines
The last comment says:
Quoted 1 line
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.
Quoted 4 lines
Quoted 2 lines
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.
Quoted 2 lines
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`.
Dear Oliver,
Am 10.08.25 um 10:20 schrieb Oliver Chang:
Quoted 8 lines
Quoted 4 lines
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_sizeThank 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)
Quoted 8 lines
Quoted 5 lines
The last comment says:
Quoted 1 line
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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 7 lines
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.
Quoted 10 lines
Quoted 1 line
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_recordcallsg_markup_parse_context_parsewith a NULLerror, so any parsing errors encountered are not propagated.
Quoted 7 lines
``` if (g_markup_parse_context_parse(ctx, data, size, NULL) == FALSE) { ```It seems useful to enable propagating of parsing errors to
sdp_xml_parse_recordin 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
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