Fixed a buffer overflow found by OSS-Fuzz.

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

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(+)

index 5efa62ab8..b10882db2 100644
@@ -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

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

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

(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

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.

Separate commits would be useful.