[PATCH BlueZ 0/2] audio: harden A2DP parser handling

[PATCH BlueZ 0/2] audio: harden A2DP parser handling

[Geraldo Netto]

This series hardens the A2DP persisted endpoint parsing path and adds
focused unit coverage around the parser behavior. It also splits the
parser helpers out of a2dp.c to keep the endpoint parsing code easier to
test and review.

These are my first BlueZ patches, so feedback on style, structure, or
submission format is welcome.

Geraldo Netto (2):
  audio: harden a2dp parsers
  audio: reduce a2dp parser complexity

 Makefile.am                   |   7 +
 Makefile.plugins              |   1 +
 profiles/audio/a2dp-helpers.c | 167 ++++++++++++++++++++
 profiles/audio/a2dp-helpers.h |  20 +++
 profiles/audio/a2dp.c         |  86 +++++-----
 unit/test-a2dp.c              | 288 ++++++++++++++++++++++++++++++++++
 6 files changed, 520 insertions(+), 49 deletions(-)
 create mode 100644 profiles/audio/a2dp-helpers.c
 create mode 100644 profiles/audio/a2dp-helpers.h
 create mode 100644 unit/test-a2dp.c

-- 
2.43.0

[PATCH BlueZ 1/2] audio: harden a2dp parsers

[Geraldo Netto]

---
 Makefile.am                   |   7 +
 Makefile.plugins              |   1 +
 profiles/audio/a2dp-helpers.c | 136 ++++++++++++++++
 profiles/audio/a2dp-helpers.h |  20 +++
 profiles/audio/a2dp.c         |  86 +++++-----
 unit/test-a2dp.c              | 288 ++++++++++++++++++++++++++++++++++
 6 files changed, 489 insertions(+), 49 deletions(-)
 create mode 100644 profiles/audio/a2dp-helpers.c
 create mode 100644 profiles/audio/a2dp-helpers.h
 create mode 100644 unit/test-a2dp.c

diff --git a/Makefile.am b/Makefile.am
index 76c4ab5d4..35871cc57 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -642,6 +642,13 @@ unit_test_avdtp_SOURCES = unit/test-avdtp.c \
 				unit/avdtp.c unit/avdtp.h
 unit_test_avdtp_LDADD = src/libshared-glib.la $(GLIB_LIBS)
 
+unit_tests += unit/test-a2dp
+
+unit_test_a2dp_SOURCES = unit/test-a2dp.c \
+				profiles/audio/a2dp-helpers.c \
+				profiles/audio/a2dp-helpers.h
+unit_test_a2dp_LDADD = src/libshared-glib.la $(GLIB_LIBS) $(DBUS_LIBS)
+
 unit_tests += unit/test-avctp
 
 unit_test_avctp_SOURCES = unit/test-avctp.c \
diff --git a/Makefile.plugins b/Makefile.plugins
index ac667beda..57400d877 100644
--- a/Makefile.plugins
+++ b/Makefile.plugins
@@ -27,6 +27,7 @@ builtin_modules += a2dp
 builtin_sources += profiles/audio/source.h profiles/audio/source.c \
 			profiles/audio/sink.h profiles/audio/sink.c \
 			profiles/audio/a2dp.h profiles/audio/a2dp.c \
+			profiles/audio/a2dp-helpers.h profiles/audio/a2dp-helpers.c \
 			profiles/audio/avdtp.h profiles/audio/avdtp.c \
 			profiles/audio/a2dp-codecs.h
 endif
diff --git a/profiles/audio/a2dp-helpers.c b/profiles/audio/a2dp-helpers.c
new file mode 100644
index 000000000..035236df6
--- /dev/null
+++ b/profiles/audio/a2dp-helpers.c
@@ -0,0 +1,136 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <stdbool.h>
+#include <stddef.h>
+#include <stdint.h>
+#include <string.h>
+
+#include <dbus/dbus.h>
+#include <glib.h>
+
+#include "a2dp-helpers.h"
+
+static bool parse_hex_byte(const char **value, uint8_t *byte)
+{
+	int high;
+	int low;
+
+	if ((*value)[0] == '\0' || (*value)[1] == '\0')
+		return false;
+
+	high = g_ascii_xdigit_value((*value)[0]);
+	low = g_ascii_xdigit_value((*value)[1]);
+	if (high < 0 || low < 0)
+		return false;
+
+	*byte = high << 4 | low;
+	*value += 2;
+
+	return true;
+}
+
+static bool parse_colon(const char **value)
+{
+	if (**value != ':')
+		return false;
+
+	(*value)++;
+
+	return true;
+}
+
+static bool parse_caps(const char *value, uint8_t *caps, size_t caps_len,
+								size_t *size)
+{
+	size_t len;
+	size_t i;
+
+	if (!value || !caps || !size)
+		return false;
+
+	*size = 0;
+
+	len = strlen(value);
+	if (!len || len % 2 || len / 2 > caps_len)
+		return false;
+
+	for (i = 0; i < len; i++) {
+		if (!g_ascii_isxdigit(value[i]))
+			return false;
+	}
+
+	for (i = 0; i < len; i += 2) {
+		const char *pos = value + i;
+
+		parse_hex_byte(&pos, &caps[i / 2]);
+	}
+
+	*size = len / 2;
+
+	return true;
+}
+
+bool a2dp_parse_capabilities_array(DBusMessageIter *value,
+					uint8_t **caps, int *size)
+{
+	DBusMessageIter array;
+
+	if (!value || !caps || !size)
+		return false;
+
+	*caps = NULL;
+	*size = 0;
+
+	if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_ARRAY)
+		return false;
+
+	if (dbus_message_iter_get_element_type(value) != DBUS_TYPE_BYTE)
+		return false;
+
+	dbus_message_iter_recurse(value, &array);
+	dbus_message_iter_get_fixed_array(&array, caps, size);
+
+	return *caps && *size > 0;
+}
+
+bool a2dp_parse_persisted_endpoint(const char *value, uint8_t *type,
+					uint8_t *codec,
+					bool *delay_reporting,
+					uint8_t *caps, size_t caps_len,
+					size_t *size)
+{
+	const char *pos;
+	uint8_t delay = 0;
+
+	if (!value || !type || !codec || !delay_reporting || !size)
+		return false;
+
+	*size = 0;
+
+	pos = value;
+	if (!parse_hex_byte(&pos, type) || !parse_colon(&pos))
+		return false;
+
+	if (!parse_hex_byte(&pos, codec) || !parse_colon(&pos))
+		return false;
+
+	if (pos[0] != '\0' && pos[1] != '\0' &&
+			g_ascii_isxdigit(pos[0]) && g_ascii_isxdigit(pos[1]) &&
+			pos[2] == ':') {
+		parse_hex_byte(&pos, &delay);
+		parse_colon(&pos);
+		if (delay > 1)
+			return false;
+	}
+
+	if (!parse_caps(pos, caps, caps_len, size))
+		return false;
+
+	*delay_reporting = delay;
+
+	return true;
+}
diff --git a/profiles/audio/a2dp-helpers.h b/profiles/audio/a2dp-helpers.h
new file mode 100644
index 000000000..a5c90c516
--- /dev/null
+++ b/profiles/audio/a2dp-helpers.h
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#ifndef BLUEZ_A2DP_HELPERS_H
+#define BLUEZ_A2DP_HELPERS_H
+
+#include <stdbool.h>
+#include <stddef.h>
+#include <stdint.h>
+
+#include <dbus/dbus.h>
+
+bool a2dp_parse_capabilities_array(DBusMessageIter *value,
+					uint8_t **caps, int *size);
+bool a2dp_parse_persisted_endpoint(const char *value, uint8_t *type,
+					uint8_t *codec,
+					bool *delay_reporting,
+					uint8_t *caps, size_t caps_len,
+					size_t *size);
+
+#endif /* BLUEZ_A2DP_HELPERS_H */
diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index a5e002784..0999436c5 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -52,6 +52,7 @@
 #include "sink.h"
 #include "source.h"
 #include "a2dp.h"
+#include "a2dp-helpers.h"
 #include "a2dp-codecs.h"
 #include "media.h"
 
@@ -689,7 +690,7 @@ static void stream_state_changed(struct avdtp_stream *stream,
 		if (err < 0 && err != -EINPROGRESS) {
 			error("avdtp_start: %s (%d)", strerror(-err), -err);
 			finalize_setup_errno(setup, err, finalize_resume,
-									NULL);
+							(GSourceFunc) NULL);
 			return;
 		}
 
@@ -942,7 +943,8 @@ static void endpoint_open_cb(struct a2dp_setup *setup, uint8_t error_code)
 
 	if (error_code != 0) {
 		setup->stream = NULL;
-		finalize_setup_errno(setup, -EPERM, finalize_config, NULL);
+		finalize_setup_errno(setup, -EPERM, finalize_config,
+							(GSourceFunc) NULL);
 		goto done;
 	}
 
@@ -954,7 +956,7 @@ static void endpoint_open_cb(struct a2dp_setup *setup, uint8_t error_code)
 
 	error("avdtp_open %s (%d)", strerror(-err), -err);
 	setup->stream = NULL;
-	finalize_setup_errno(setup, err, finalize_config, NULL);
+	finalize_setup_errno(setup, err, finalize_config, (GSourceFunc) NULL);
 done:
 	setup_unref(setup);
 }
@@ -974,6 +976,7 @@ static void store_remote_sep(void *data, void *user_data)
 	char seid[4], value[256];
 	struct avdtp_service_capability *service = avdtp_get_codec(sep->sep);
 	struct avdtp_media_codec_capability *codec;
+	uint8_t delay_reporting;
 	unsigned int i;
 	ssize_t offset;
 
@@ -981,12 +984,13 @@ static void store_remote_sep(void *data, void *user_data)
 		return;
 
 	codec = (void *) service->data;
+	delay_reporting = avdtp_get_delay_reporting(sep->sep);
 
 	sprintf(seid, "%02hhx", avdtp_get_seid(sep->sep));
 
 	offset = sprintf(value, "%02hhx:%02hhx:%02hhx:",
 			avdtp_get_type(sep->sep), codec->media_codec_type,
-			avdtp_get_delay_reporting(sep->sep));
+			delay_reporting);
 
 	for (i = 0; i < service->length - sizeof(*codec); i++)
 		offset += sprintf(value + offset, "%02hhx", codec->data[i]);
@@ -1139,7 +1143,8 @@ static void setconf_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 			return;
 
 		setup->stream = NULL;
-		finalize_setup_errno(setup, -EPERM, finalize_config, NULL);
+		finalize_setup_errno(setup, -EPERM, finalize_config,
+							(GSourceFunc) NULL);
 		setup_unref(setup);
 		return;
 	}
@@ -1148,7 +1153,8 @@ static void setconf_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 	if (ret < 0) {
 		error("avdtp_open %s (%d)", strerror(-ret), -ret);
 		setup->stream = NULL;
-		finalize_setup_errno(setup, ret, finalize_config, NULL);
+		finalize_setup_errno(setup, ret, finalize_config,
+							(GSourceFunc) NULL);
 	}
 }
 
@@ -1431,7 +1437,8 @@ static gboolean suspend_ind(struct avdtp *session, struct avdtp_local_sep *sep,
 	if (start_err < 0 && start_err != -EINPROGRESS) {
 		error("avdtp_start: %s (%d)", strerror(-start_err),
 								-start_err);
-		finalize_setup_errno(setup, start_err, finalize_resume, NULL);
+		finalize_setup_errno(setup, start_err, finalize_resume,
+							(GSourceFunc) NULL);
 	}
 
 	return TRUE;
@@ -1483,7 +1490,8 @@ static void suspend_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
 	start_err = avdtp_start(session, a2dp_stream->stream);
 	if (start_err < 0 && start_err != -EINPROGRESS) {
 		error("avdtp_start: %s (%d)", strerror(-start_err), -start_err);
-		finalize_setup_errno(setup, start_err, finalize_suspend, NULL);
+		finalize_setup_errno(setup, start_err, finalize_suspend,
+							(GSourceFunc) NULL);
 	}
 }
 
@@ -1504,7 +1512,7 @@ static gboolean close_ind(struct avdtp *session, struct avdtp_local_sep *sep,
 		return TRUE;
 
 	finalize_setup_errno(setup, -ECONNRESET, finalize_suspend,
-							finalize_resume, NULL);
+					finalize_resume, (GSourceFunc) NULL);
 
 	return TRUE;
 }
@@ -1572,7 +1580,8 @@ static gboolean a2dp_reconfigure(gpointer data)
 	return FALSE;
 
 failed:
-	finalize_setup_errno(setup, posix_err, finalize_config, NULL);
+	finalize_setup_errno(setup, posix_err, finalize_config,
+							(GSourceFunc) NULL);
 	return FALSE;
 }
 
@@ -1648,8 +1657,8 @@ static void abort_ind(struct avdtp *session, struct avdtp_local_sep *sep,
 		return;
 
 	finalize_setup_errno(setup, -ECONNRESET, finalize_suspend,
-							finalize_resume,
-							finalize_config, NULL);
+					finalize_resume, finalize_config,
+					(GSourceFunc) NULL);
 
 	return;
 }
@@ -1901,7 +1910,8 @@ static void channel_free(void *data)
 		setup->chan = NULL;
 		setup_ref(setup);
 		/* Finalize pending commands before we NULL setup->session */
-		finalize_setup_errno(setup, -ENOTCONN, finalize_all, NULL);
+		finalize_setup_errno(setup, -ENOTCONN, finalize_all,
+							(GSourceFunc) NULL);
 		avdtp_unref(setup->session);
 		setup->session = NULL;
 		setup_unref(setup);
@@ -1991,10 +2001,12 @@ static struct a2dp_sep *find_sep(struct a2dp_server *server, uint8_t type,
 
 static int parse_properties(DBusMessageIter *props, uint8_t **caps, int *size)
 {
+	*caps = NULL;
+	*size = 0;
+
 	while (dbus_message_iter_get_arg_type(props) == DBUS_TYPE_DICT_ENTRY) {
 		const char *key;
 		DBusMessageIter value, entry;
-		int var;
 
 		dbus_message_iter_recurse(props, &entry);
 		dbus_message_iter_get_basic(&entry, &key);
@@ -2002,15 +2014,10 @@ static int parse_properties(DBusMessageIter *props, uint8_t **caps, int *size)
 		dbus_message_iter_next(&entry);
 		dbus_message_iter_recurse(&entry, &value);
 
-		var = dbus_message_iter_get_arg_type(&value);
 		if (strcasecmp(key, "Capabilities") == 0) {
-			DBusMessageIter array;
-
-			if (var != DBUS_TYPE_ARRAY)
+			if (!a2dp_parse_capabilities_array(&value, caps, size))
 				return -EINVAL;
 
-			dbus_message_iter_recurse(&value, &array);
-			dbus_message_iter_get_fixed_array(&array, caps, size);
 			return 0;
 		}
 
@@ -2130,7 +2137,7 @@ static DBusMessage *set_configuration(DBusConnection *conn, DBusMessage *msg,
 	struct avdtp_media_codec_capability *codec;
 	DBusMessageIter args, props;
 	const char *sender, *path;
-	uint8_t *caps;
+	uint8_t *caps = NULL;
 	int err, size = 0;
 
 	sender = dbus_message_get_sender(msg);
@@ -2371,11 +2378,10 @@ static void load_remote_sep(struct a2dp_channel *chan, GKeyFile *key_file,
 	for (; *seids; seids++) {
 		uint8_t type;
 		uint8_t codec;
-		uint8_t delay_reporting;
+		bool delay_reporting;
 		GSList *l = NULL;
-		char caps[256];
 		uint8_t data[128];
-		int i, size;
+		size_t size;
 
 		if (sscanf(*seids, "%02hhx", &rseid) != 1)
 			continue;
@@ -2385,34 +2391,16 @@ static void load_remote_sep(struct a2dp_channel *chan, GKeyFile *key_file,
 		if (!value)
 			continue;
 
-		/* Try loading with delay_reporting first */
-		if (sscanf(value, "%02hhx:%02hhx:%02hhx:%s", &type, &codec,
-					&delay_reporting, caps) != 4) {
-			/* Try old format */
-			if (sscanf(value, "%02hhx:%02hhx:%s", &type, &codec,
-								caps) != 3) {
-				warn("Unable to load Endpoint: seid %u", rseid);
-				g_free(value);
-				continue;
-			}
-			delay_reporting = false;
-		}
-
-		for (i = 0, size = strlen(caps); i < size; i += 2) {
-			uint8_t *tmp = data + i / 2;
-
-			if (sscanf(caps + i, "%02hhx", tmp) != 1) {
-				warn("Unable to load Endpoint: seid %u", rseid);
-				break;
-			}
+		if (!a2dp_parse_persisted_endpoint(value, &type, &codec,
+						&delay_reporting, data,
+						sizeof(data), &size)) {
+			warn("Unable to load Endpoint: seid %u", rseid);
+			g_free(value);
+			continue;
 		}
-
 		g_free(value);
 
-		if (i != size)
-			continue;
-
-		caps_add_codec(&l, codec, data, size / 2);
+		caps_add_codec(&l, codec, data, size);
 
 		rsep = avdtp_register_remote_sep(chan->session, rseid, type, l,
 							delay_reporting);
diff --git a/unit/test-a2dp.c b/unit/test-a2dp.c
new file mode 100644
index 000000000..2f7bd7bbc
--- /dev/null
+++ b/unit/test-a2dp.c
@@ -0,0 +1,288 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <stdbool.h>
+#include <stdint.h>
+#include <string.h>
+
+#include <dbus/dbus.h>
+#include <glib.h>
+
+#include "profiles/audio/a2dp-helpers.h"
+
+static DBusMessage *new_method_call(void)
+{
+	return dbus_message_new_method_call("org.bluez.test",
+						"/org/bluez/test",
+						"org.bluez.test",
+						"Test");
+}
+
+static void append_byte_array(DBusMessage *msg, const uint8_t *data, int size)
+{
+	DBusMessageIter iter;
+	DBusMessageIter array;
+
+	dbus_message_iter_init_append(msg, &iter);
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
+						DBUS_TYPE_BYTE_AS_STRING,
+						&array);
+	dbus_message_iter_append_fixed_array(&array, DBUS_TYPE_BYTE,
+						&data, size);
+	dbus_message_iter_close_container(&iter, &array);
+}
+
+static void append_string_array(DBusMessage *msg)
+{
+	DBusMessageIter iter;
+	DBusMessageIter array;
+	const char *value = "not-bytes";
+
+	dbus_message_iter_init_append(msg, &iter);
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
+						DBUS_TYPE_STRING_AS_STRING,
+						&array);
+	dbus_message_iter_append_basic(&array, DBUS_TYPE_STRING, &value);
+	dbus_message_iter_close_container(&iter, &array);
+}
+
+static void test_capabilities_array_accepts_byte_array(void)
+{
+	DBusMessage *msg = new_method_call();
+	DBusMessageIter iter;
+	const uint8_t bytes[] = { 0x11, 0x22, 0x33 };
+	uint8_t *caps = NULL;
+	int size = 0;
+
+	g_assert_nonnull(msg);
+
+	append_byte_array(msg, bytes, sizeof(bytes));
+	g_assert_true(dbus_message_iter_init(msg, &iter));
+	g_assert_true(a2dp_parse_capabilities_array(&iter, &caps, &size));
+	g_assert_cmpint(size, ==, 3);
+	g_assert_nonnull(caps);
+	g_assert_cmpint(memcmp(caps, bytes, sizeof(bytes)), ==, 0);
+
+	dbus_message_unref(msg);
+}
+
+static void test_capabilities_array_rejects_wrong_element_type(void)
+{
+	DBusMessage *msg = new_method_call();
+	DBusMessageIter iter;
+	uint8_t *caps = (void *) 0x01;
+	int size = 1;
+
+	g_assert_nonnull(msg);
+
+	append_string_array(msg);
+	g_assert_true(dbus_message_iter_init(msg, &iter));
+	g_assert_false(a2dp_parse_capabilities_array(&iter, &caps, &size));
+	g_assert_null(caps);
+	g_assert_cmpint(size, ==, 0);
+
+	dbus_message_unref(msg);
+}
+
+static void test_capabilities_array_rejects_empty_array(void)
+{
+	DBusMessage *msg = new_method_call();
+	DBusMessageIter iter;
+	uint8_t *caps = (void *) 0x01;
+	int size = 1;
+
+	g_assert_nonnull(msg);
+
+	append_byte_array(msg, NULL, 0);
+	g_assert_true(dbus_message_iter_init(msg, &iter));
+	g_assert_false(a2dp_parse_capabilities_array(&iter, &caps, &size));
+	g_assert_cmpint(size, ==, 0);
+
+	dbus_message_unref(msg);
+}
+
+static void test_capabilities_array_rejects_missing_iter(void)
+{
+	uint8_t *caps = (void *) 0x01;
+	int size = 1;
+
+	g_assert_false(a2dp_parse_capabilities_array(NULL, &caps, &size));
+}
+
+static void test_capabilities_array_rejects_non_array(void)
+{
+	DBusMessage *msg = new_method_call();
+	DBusMessageIter iter;
+	const char *value = "not-array";
+	uint8_t *caps = (void *) 0x01;
+	int size = 1;
+
+	g_assert_nonnull(msg);
+
+	dbus_message_iter_init_append(msg, &iter);
+	dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING, &value);
+	g_assert_true(dbus_message_iter_init(msg, &iter));
+	g_assert_false(a2dp_parse_capabilities_array(&iter, &caps, &size));
+	g_assert_null(caps);
+	g_assert_cmpint(size, ==, 0);
+
+	dbus_message_unref(msg);
+}
+
+static void assert_endpoint(const char *value, uint8_t expected_type,
+				uint8_t expected_codec,
+				bool expected_delay,
+				const uint8_t *expected_caps,
+				size_t expected_size)
+{
+	uint8_t type = 0xff;
+	uint8_t codec = 0xff;
+	bool delay_reporting = true;
+	uint8_t caps[128];
+	size_t size = 0;
+
+	memset(caps, 0xa5, sizeof(caps));
+
+	g_assert_true(a2dp_parse_persisted_endpoint(value, &type, &codec,
+							&delay_reporting,
+							caps, sizeof(caps),
+							&size));
+	g_assert_cmpint(type, ==, expected_type);
+	g_assert_cmpint(codec, ==, expected_codec);
+	g_assert_cmpint(delay_reporting, ==, expected_delay);
+	g_assert_cmpuint(size, ==, expected_size);
+	g_assert_cmpint(memcmp(caps, expected_caps, expected_size), ==, 0);
+}
+
+static void test_endpoint_parser_accepts_current_format(void)
+{
+	const uint8_t caps[] = { 0x11, 0x22, 0x33 };
+
+	assert_endpoint("00:40:01:112233", 0x00, 0x40, true, caps,
+							sizeof(caps));
+}
+
+static void test_endpoint_parser_accepts_old_format(void)
+{
+	const uint8_t caps[] = { 0xaa, 0xbb };
+
+	assert_endpoint("01:02:aabb", 0x01, 0x02, false, caps, sizeof(caps));
+}
+
+static void assert_endpoint_rejected(const char *value)
+{
+	uint8_t type = 0xff;
+	uint8_t codec = 0xff;
+	bool delay_reporting = true;
+	uint8_t caps[4] = { 0xa5, 0xa5, 0xa5, 0xa5 };
+	size_t size = 7;
+
+	g_assert_false(a2dp_parse_persisted_endpoint(value, &type, &codec,
+							&delay_reporting,
+							caps, sizeof(caps),
+							&size));
+	g_assert_cmpint(caps[0], ==, 0xa5);
+	g_assert_cmpint(caps[1], ==, 0xa5);
+	g_assert_cmpint(caps[2], ==, 0xa5);
+	g_assert_cmpint(caps[3], ==, 0xa5);
+}
+
+static void test_endpoint_parser_rejects_invalid_fields(void)
+{
+	assert_endpoint_rejected(NULL);
+	assert_endpoint_rejected("");
+	assert_endpoint_rejected("00:40");
+	assert_endpoint_rejected("00:40:");
+	assert_endpoint_rejected("00:40:01:");
+	assert_endpoint_rejected("00:40:02:aabb");
+	assert_endpoint_rejected("00:40:01:aab");
+	assert_endpoint_rejected("00:40:01:aazz");
+	assert_endpoint_rejected("00:40:01:aa:bb");
+	assert_endpoint_rejected("00:40:aabb:");
+	assert_endpoint_rejected("xx:40:01:aabb");
+}
+
+static void test_endpoint_parser_rejects_missing_output_buffer(void)
+{
+	uint8_t type;
+	uint8_t codec;
+	bool delay_reporting;
+	size_t size;
+
+	g_assert_false(a2dp_parse_persisted_endpoint("00:40:01:aabb",
+							&type, &codec,
+							&delay_reporting,
+							NULL, 0, &size));
+}
+
+static void test_endpoint_parser_rejects_oversized_caps(void)
+{
+	char value[sizeof("00:40:01:") + 16];
+
+	memset(value, 'a', sizeof(value));
+	memcpy(value, "00:40:01:", strlen("00:40:01:"));
+	value[sizeof(value) - 1] = '\0';
+
+	assert_endpoint_rejected(value);
+}
+
+static void test_endpoint_parser_fuzz_cases_keep_bounds(void)
+{
+	static const char alphabet[] = "0123456789abcdefABCDEF:gZ";
+	unsigned int i;
+
+	for (i = 0; i < 4096; i++) {
+		char value[16];
+		uint8_t type;
+		uint8_t codec;
+		bool delay_reporting;
+		uint8_t caps[6] = { 0, 0, 0, 0, 0xcc, 0xdd };
+		size_t size;
+		size_t len = i % (sizeof(value) - 1);
+		size_t j;
+
+		for (j = 0; j < len; j++)
+			value[j] = alphabet[(i + j * 7) %
+						(sizeof(alphabet) - 1)];
+		value[len] = '\0';
+
+		a2dp_parse_persisted_endpoint(value, &type, &codec,
+						&delay_reporting, caps, 4,
+						&size);
+		g_assert_cmpint(caps[4], ==, 0xcc);
+		g_assert_cmpint(caps[5], ==, 0xdd);
+	}
+}
+
+int main(int argc, char *argv[])
+{
+	g_test_init(&argc, &argv, NULL);
+
+	g_test_add_func("/a2dp/capabilities/byte-array",
+			test_capabilities_array_accepts_byte_array);
+	g_test_add_func("/a2dp/capabilities/wrong-element-type",
+			test_capabilities_array_rejects_wrong_element_type);
+	g_test_add_func("/a2dp/capabilities/empty-array",
+			test_capabilities_array_rejects_empty_array);
+	g_test_add_func("/a2dp/capabilities/missing-iter",
+			test_capabilities_array_rejects_missing_iter);
+	g_test_add_func("/a2dp/capabilities/non-array",
+			test_capabilities_array_rejects_non_array);
+	g_test_add_func("/a2dp/endpoint/current-format",
+			test_endpoint_parser_accepts_current_format);
+	g_test_add_func("/a2dp/endpoint/old-format",
+			test_endpoint_parser_accepts_old_format);
+	g_test_add_func("/a2dp/endpoint/invalid-fields",
+			test_endpoint_parser_rejects_invalid_fields);
+	g_test_add_func("/a2dp/endpoint/missing-output-buffer",
+			test_endpoint_parser_rejects_missing_output_buffer);
+	g_test_add_func("/a2dp/endpoint/oversized-caps",
+			test_endpoint_parser_rejects_oversized_caps);
+	g_test_add_func("/a2dp/endpoint/fuzz-bounds",
+			test_endpoint_parser_fuzz_cases_keep_bounds);
+
+	return g_test_run();
+}
-- 
2.43.0

[PATCH BlueZ 2/2] audio: reduce a2dp parser complexity

[Geraldo Netto]

---
 profiles/audio/a2dp-helpers.c | 55 +++++++++++++++++++++++++++--------
 1 file changed, 43 insertions(+), 12 deletions(-)

diff --git a/profiles/audio/a2dp-helpers.c b/profiles/audio/a2dp-helpers.c
index 035236df6..09dc6db91 100644
--- a/profiles/audio/a2dp-helpers.c
+++ b/profiles/audio/a2dp-helpers.c
@@ -74,6 +74,46 @@ static bool parse_caps(const char *value, uint8_t *caps, size_t caps_len,
 	return true;
 }
 
+static bool has_delay_field(const char *value)
+{
+	return value[0] != '\0' && value[1] != '\0' &&
+			g_ascii_isxdigit(value[0]) &&
+			g_ascii_isxdigit(value[1]) &&
+			value[2] == ':';
+}
+
+static bool parse_endpoint_header(const char **value, uint8_t *type,
+							uint8_t *codec)
+{
+	if (!parse_hex_byte(value, type) || !parse_colon(value))
+		return false;
+
+	if (!parse_hex_byte(value, codec) || !parse_colon(value))
+		return false;
+
+	return true;
+}
+
+static bool parse_delay_field(const char **value, uint8_t *delay)
+{
+	*delay = 0;
+
+	if (!has_delay_field(*value))
+		return true;
+
+	parse_hex_byte(value, delay);
+	parse_colon(value);
+
+	return *delay <= 1;
+}
+
+static bool valid_endpoint_args(const char *value,
+				const uint8_t *type, const uint8_t *codec,
+				const bool *delay_reporting, const size_t *size)
+{
+	return value && type && codec && delay_reporting && size;
+}
+
 bool a2dp_parse_capabilities_array(DBusMessageIter *value,
 					uint8_t **caps, int *size)
 {
@@ -106,27 +146,18 @@ bool a2dp_parse_persisted_endpoint(const char *value, uint8_t *type,
 	const char *pos;
 	uint8_t delay = 0;
 
-	if (!value || !type || !codec || !delay_reporting || !size)
+	if (!valid_endpoint_args(value, type, codec, delay_reporting, size))
 		return false;
 
 	*size = 0;
 
 	pos = value;
-	if (!parse_hex_byte(&pos, type) || !parse_colon(&pos))
+	if (!parse_endpoint_header(&pos, type, codec))
 		return false;
 
-	if (!parse_hex_byte(&pos, codec) || !parse_colon(&pos))
+	if (!parse_delay_field(&pos, &delay))
 		return false;
 
-	if (pos[0] != '\0' && pos[1] != '\0' &&
-			g_ascii_isxdigit(pos[0]) && g_ascii_isxdigit(pos[1]) &&
-			pos[2] == ':') {
-		parse_hex_byte(&pos, &delay);
-		parse_colon(&pos);
-		if (delay > 1)
-			return false;
-	}
-
 	if (!parse_caps(pos, caps, caps_len, size))
 		return false;
 
-- 
2.43.0

RE: audio: harden A2DP parser handling

[bluez.test.bot]

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

---Test result---

Test Summary:
CheckPatch                    PASS      1.10 seconds
GitLint                       PASS      0.66 seconds
BuildEll                      PASS      20.28 seconds
BluezMake                     PASS      655.58 seconds
MakeCheck                     PASS      18.46 seconds
MakeDistcheck                 PASS      246.91 seconds
CheckValgrind                 PASS      297.01 seconds
CheckSmatch                   PASS      350.72 seconds
bluezmakeextell               PASS      181.74 seconds
IncrementalBuild              PASS      662.39 seconds
ScanBuild                     PASS      1035.90 seconds



https://github.com/bluez/bluez/pull/2244

---
Regards,
Linux Bluetooth