[PATCH BlueZ v1 5/5] unit/test-avrcp: Add robustness tests for response parsing

[Luiz Augusto von Dentz <luiz.dentz@gmail.com>]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

The existing tests drive avrcp-lib.c through the AVCTP harness and all
feed it well formed PDUs. Nothing covered what happens when a peer
sends a response that lies about its own length, which is what the
three preceding fixes were about.

Add tests under /robustness that call the parsers in avrcp-parse.c
directly, since a response is entirely peer controlled and the parser
is what has to survive it:

- headers that are short, that declare more parameter bytes than were
  received, and that declare fewer

- a ListPlayerApplicationSettingAttributes response declaring 255
  attributes, which used to be written into a four byte array, and one
  declaring more attributes than it carries

- attribute lists declaring a 0xFFFF byte value with none of it
  present, a truncated value, a truncated attribute header, and more
  attributes than were received

- media elements missing the attribute count that follows the name,
  carrying a name longer than NAME_MAX_LEN, or declaring a name that is
  not there, and the equivalent for media folders

Each PDU is copied into a buffer of exactly its size, so that reading
past the end of it is an out-of-bounds access rather than a read of
whatever the receive buffer happened to hold beforehand, and the
attribute array is surrounded by a guard so that a write past its end
is caught without a sanitizer.

Reverting the three fixes fails ten of these outright and trips
valgrind on six more.

Assisted-by: Claude:claude-opus-5 valgrind
---
 Makefile.am       |   4 +-
 unit/test-avrcp.c | 395 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 398 insertions(+), 1 deletion(-)

diff --git a/Makefile.am b/Makefile.am
index 475a344c713d..f0028cfb39f8 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -674,7 +674,9 @@ unit_tests += unit/test-avrcp
 unit_test_avrcp_SOURCES = unit/test-avrcp.c \
 				src/log.h src/log.c \
 				unit/avctp.c unit/avctp.h \
-				unit/avrcp-lib.c unit/avrcp-lib.h
+				unit/avrcp-lib.c unit/avrcp-lib.h \
+				profiles/audio/avrcp-parse.h \
+				profiles/audio/avrcp-parse.c
 unit_test_avrcp_LDADD = lib/libbluetooth-internal.la \
 				src/libshared-glib.la $(GLIB_LIBS)
 
diff --git a/unit/test-avrcp.c b/unit/test-avrcp.c
index 7bed8fbaf74a..e0c8971514ba 100644
--- a/unit/test-avrcp.c
+++ b/unit/test-avrcp.c
@@ -30,6 +30,7 @@
 
 #include "unit/avctp.h"
 #include "unit/avrcp-lib.h"
+#include "profiles/audio/avrcp-parse.h"
 
 struct test_pdu {
 	bool valid;
@@ -986,6 +987,398 @@ static void test_client(gconstpointer data)
 		avrcp_send_passthrough(context->session, 0, AVC_FAST_FORWARD);
 }
 
+/*
+ * Robustness tests for the controller side response parsers.
+ *
+ * These call the parsers directly rather than going through the AVCTP
+ * harness, since responses are what the peer controls and the parsers are
+ * what has to survive them. Each PDU is copied into a buffer of exactly its
+ * size, so that reading past the end of it is an out-of-bounds access rather
+ * than a read of whatever the receive buffer happened to hold before.
+ */
+
+struct robustness_test {
+	char *test_name;
+	uint8_t *data;
+	size_t size;
+	/* Expected result, or -1 if the PDU must be rejected */
+	int expected;
+};
+
+#define define_robustness_test(name, function, exp, args...)		\
+	do {								\
+		static struct robustness_test rt;			\
+		rt.test_name = g_strdup(name);				\
+		rt.data = util_memdup(data(args), sizeof(data(args)));	\
+		rt.size = sizeof(data(args));				\
+		rt.expected = exp;					\
+		tester_add(name, &rt, NULL, function, NULL);		\
+	} while (0)
+
+/* AVRCP header: BT SIG company id, pdu id, packet type, parameters length */
+#define AVRCP_HDR(pdu_id, len)						\
+	0x00, 0x19, 0x58, pdu_id, 0x00, ((len) >> 8) & 0xff, (len) & 0xff
+
+#define X4	'x', 'x', 'x', 'x'
+#define X16	X4, X4, X4, X4
+#define X64	X16, X16, X16, X16
+#define X256	X64, X64, X64, X64
+#define LONG_NAME_260	X256, X4
+
+static void robustness_result(struct robustness_test *rt, void *buf,
+								int result)
+{
+	free(buf);
+
+	if (result != rt->expected) {
+		tester_warn("%s: expected %d, got %d", rt->test_name,
+							rt->expected, result);
+		tester_test_failed();
+		return;
+	}
+
+	tester_test_passed();
+}
+
+static void *robustness_iov(const struct robustness_test *rt,
+							struct iovec *iov)
+{
+	iov->iov_base = util_memdup(rt->data, rt->size);
+	iov->iov_len = rt->size;
+
+	return iov->iov_base;
+}
+
+/* Expected is the number of parameter bytes left, or -1 if rejected */
+static void test_pull_header(gconstpointer data)
+{
+	struct robustness_test *rt = (void *) data;
+	struct iovec iov;
+	void *buf = robustness_iov(rt, &iov);
+
+	if (!avrcp_pull_header(&iov)) {
+		robustness_result(rt, buf, -1);
+		return;
+	}
+
+	robustness_result(rt, buf, iov.iov_len);
+}
+
+static void test_pull_browsing_header(gconstpointer data)
+{
+	struct robustness_test *rt = (void *) data;
+	struct iovec iov;
+	void *buf = robustness_iov(rt, &iov);
+
+	if (!avrcp_pull_browsing_header(&iov)) {
+		robustness_result(rt, buf, -1);
+		return;
+	}
+
+	robustness_result(rt, buf, iov.iov_len);
+}
+
+/*
+ * The attribute count declared by the peer is what bounds the write into
+ * attrs, so surround it with a guard and check that nothing was written
+ * past its end. Expected is the number of attributes accepted.
+ */
+#define ATTRS_GUARD 8
+
+static void test_player_attributes(gconstpointer data)
+{
+	struct robustness_test *rt = (void *) data;
+	uint8_t attrs[AVRCP_ATTRIBUTE_LAST + ATTRS_GUARD];
+	struct iovec iov;
+	void *buf = robustness_iov(rt, &iov);
+	uint8_t count;
+	int i;
+
+	memset(attrs, 0xaa, sizeof(attrs));
+
+	if (!avrcp_pull_header(&iov)) {
+		robustness_result(rt, buf, -1);
+		return;
+	}
+
+	count = avrcp_parse_player_attributes(&iov, attrs,
+						AVRCP_ATTRIBUTE_LAST);
+
+	for (i = 0; i < ATTRS_GUARD; i++) {
+		if (attrs[AVRCP_ATTRIBUTE_LAST + i] == 0xaa)
+			continue;
+
+		tester_warn("%s: wrote %u bytes past the end of attrs",
+					rt->test_name, ATTRS_GUARD - i);
+		free(buf);
+		tester_test_failed();
+		return;
+	}
+
+	robustness_result(rt, buf, count);
+}
+
+static void count_attribute(const struct avrcp_attribute *attr,
+							void *user_data)
+{
+	unsigned int *count = user_data;
+	unsigned int sum = 0;
+	uint16_t i;
+
+	/* Read the whole value so that a bogus length is caught */
+	for (i = 0; i < attr->len; i++)
+		sum += attr->value[i];
+
+	(void) sum;
+
+	(*count)++;
+}
+
+/* Expected is the number of attributes reported */
+static void test_attribute_list(gconstpointer data)
+{
+	struct robustness_test *rt = (void *) data;
+	struct iovec iov;
+	void *buf = robustness_iov(rt, &iov);
+	unsigned int count = 0;
+	uint8_t number;
+
+	if (!avrcp_pull_header(&iov) || !util_iov_pull_u8(&iov, &number)) {
+		robustness_result(rt, buf, -1);
+		return;
+	}
+
+	avrcp_parse_attribute_list(&iov, number, count_attribute, &count);
+
+	robustness_result(rt, buf, count);
+}
+
+/* Expected is the declared attribute count, or -1 if rejected */
+static void test_media_element(gconstpointer data)
+{
+	struct robustness_test *rt = (void *) data;
+	struct iovec iov;
+	void *buf = robustness_iov(rt, &iov);
+	struct avrcp_media_element element;
+
+	if (!avrcp_parse_media_element(&iov, &element)) {
+		robustness_result(rt, buf, -1);
+		return;
+	}
+
+	/* The name must always be truncated to fit */
+	if (strlen(element.name) >= NAME_MAX_LEN) {
+		tester_warn("%s: name not truncated", rt->test_name);
+		free(buf);
+		tester_test_failed();
+		return;
+	}
+
+	robustness_result(rt, buf, element.count);
+}
+
+/* Expected is the playable flag, or -1 if rejected */
+static void test_media_folder(gconstpointer data)
+{
+	struct robustness_test *rt = (void *) data;
+	struct iovec iov;
+	void *buf = robustness_iov(rt, &iov);
+	struct avrcp_media_folder folder;
+
+	if (!avrcp_parse_media_folder(&iov, &folder)) {
+		robustness_result(rt, buf, -1);
+		return;
+	}
+
+	if (strlen(folder.name) >= NAME_MAX_LEN) {
+		tester_warn("%s: name not truncated", rt->test_name);
+		free(buf);
+		tester_test_failed();
+		return;
+	}
+
+	robustness_result(rt, buf, folder.playable);
+}
+
+static void define_robustness_tests(void)
+{
+	/*
+	 * Responses do not go through handle_vendordep_pdu(), so nothing
+	 * validated the declared parameters length against the number of
+	 * bytes actually received.
+	 */
+
+	/* One byte short of a complete header */
+	define_robustness_test("/robustness/header/short",
+			test_pull_header, -1,
+			0x00, 0x19, 0x58, 0x10, 0x00, 0x00);
+
+	/* Declares 16 parameter bytes but carries one */
+	define_robustness_test("/robustness/header/truncated",
+			test_pull_header, -1,
+			AVRCP_HDR(0x10, 16), 0x04);
+
+	/* Declares fewer parameter bytes than were received */
+	define_robustness_test("/robustness/header/overlong",
+			test_pull_header, -1,
+			AVRCP_HDR(0x10, 1), 0x04, 0x01, 0x02);
+
+	define_robustness_test("/robustness/header/valid",
+			test_pull_header, 2,
+			AVRCP_HDR(0x10, 2), 0x01, 0x04);
+
+	define_robustness_test("/robustness/browsing-header/short",
+			test_pull_browsing_header, -1,
+			0x71, 0x00);
+
+	/* Declares 32 parameter bytes but carries one */
+	define_robustness_test("/robustness/browsing-header/truncated",
+			test_pull_browsing_header, -1,
+			0x71, 0x00, 0x20, 0x04);
+
+	define_robustness_test("/robustness/browsing-header/valid",
+			test_pull_browsing_header, 2,
+			0x71, 0x00, 0x02, 0x04, 0x01);
+
+	/*
+	 * ListPlayerApplicationSettingAttributes response, see
+	 * GHSA-m2vx-pw5f-rc8v. The declared count is what bounds the write
+	 * into a four byte array.
+	 */
+
+	/* Declares and carries 255 valid attributes */
+	define_robustness_test("/robustness/player-attributes/overflow",
+			test_player_attributes, AVRCP_ATTRIBUTE_LAST,
+			AVRCP_HDR(0x11, 21), 0xff,
+			0x01, 0x02, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04,
+			0x01, 0x02, 0x03, 0x04, 0x01, 0x02, 0x03, 0x04,
+			0x01, 0x02, 0x03, 0x04);
+
+	/* Declares four attributes but carries two */
+	define_robustness_test("/robustness/player-attributes/truncated",
+			test_player_attributes, 2,
+			AVRCP_HDR(0x11, 3), 0x04, 0x01, 0x02);
+
+	/* Declares one attribute but carries none */
+	define_robustness_test("/robustness/player-attributes/empty",
+			test_player_attributes, 0,
+			AVRCP_HDR(0x11, 1), 0x01);
+
+	/* Illegal and out of range attributes must be skipped */
+	define_robustness_test("/robustness/player-attributes/illegal",
+			test_player_attributes, 1,
+			AVRCP_HDR(0x11, 5), 0x04,
+			AVRCP_ATTRIBUTE_ILLEGAL, 0x7f,
+			AVRCP_ATTRIBUTE_SHUFFLE, 0xff);
+
+	/*
+	 * GetElementAttributes and GetItemAttributes carry variable length
+	 * attribute values which were never bounds checked.
+	 */
+
+	/* Declares a 0xFFFF byte value with none of it present */
+	define_robustness_test("/robustness/attribute-list/huge-len",
+			test_attribute_list, 0,
+			AVRCP_HDR(0x20, 9), 0x01,
+			0x00, 0x00, 0x00, 0x01,		/* Title */
+			0x00, 0x6a,			/* UTF-8 */
+			0xff, 0xff);			/* value length */
+
+	/* Declares a four byte value but carries two */
+	define_robustness_test("/robustness/attribute-list/truncated-value",
+			test_attribute_list, 0,
+			AVRCP_HDR(0x20, 11), 0x01,
+			0x00, 0x00, 0x00, 0x01,
+			0x00, 0x6a,
+			0x00, 0x04,
+			'a', 'b');
+
+	/* Declares one attribute but carries a partial header for it */
+	define_robustness_test("/robustness/attribute-list/truncated-header",
+			test_attribute_list, 0,
+			AVRCP_HDR(0x20, 4), 0x01,
+			0x00, 0x00, 0x00);
+
+	/* Declares 255 attributes but carries one */
+	define_robustness_test("/robustness/attribute-list/count-overrun",
+			test_attribute_list, 1,
+			AVRCP_HDR(0x20, 12), 0xff,
+			0x00, 0x00, 0x00, 0x01,
+			0x00, 0x6a,
+			0x00, 0x03,
+			'a', 'b', 'c');
+
+	define_robustness_test("/robustness/attribute-list/valid",
+			test_attribute_list, 2,
+			AVRCP_HDR(0x20, 18), 0x02,
+			0x00, 0x00, 0x00, 0x01,
+			0x00, 0x6a,
+			0x00, 0x01, 'a',
+			0x00, 0x00, 0x00, 0x02,
+			0x00, 0x6a,
+			0x00, 0x00);
+
+	/* Media element and folder entries of a GetFolderItems response */
+
+	/* UID, media type and character set only, no name length */
+	define_robustness_test("/robustness/media-element/truncated",
+			test_media_element, -1,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
+			0x02, 0x00, 0x6a);
+
+	/* Declares a 0xFFFF byte name with none of it present */
+	define_robustness_test("/robustness/media-element/huge-name",
+			test_media_element, -1,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
+			0x02, 0x00, 0x6a,
+			0xff, 0xff);
+
+	/*
+	 * The name is complete but the attribute count byte that follows it
+	 * is not present. This is the off-by-one that used to read
+	 * operands[13 + namesize].
+	 */
+	define_robustness_test("/robustness/media-element/no-count",
+			test_media_element, -1,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
+			0x02, 0x00, 0x6a,
+			0x00, 0x03, 'a', 'b', 'c');
+
+	/* A name longer than NAME_MAX_LEN must be truncated, not overflow */
+	define_robustness_test("/robustness/media-element/long-name",
+			test_media_element, 0,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
+			0x02, 0x00, 0x6a,
+			0x01, 0x04,	/* 260 byte name */
+			LONG_NAME_260,
+			0x00);
+
+	define_robustness_test("/robustness/media-element/valid",
+			test_media_element, 1,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x2a,
+			0x02, 0x00, 0x6a,
+			0x00, 0x03, 'a', 'b', 'c',
+			0x01);
+
+	/* UID, folder type and playable flag only */
+	define_robustness_test("/robustness/media-folder/truncated",
+			test_media_folder, -1,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
+			0x01, 0x01);
+
+	define_robustness_test("/robustness/media-folder/huge-name",
+			test_media_folder, -1,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01,
+			0x01, 0x01, 0x00, 0x6a,
+			0xff, 0xff, 'a');
+
+	define_robustness_test("/robustness/media-folder/valid",
+			test_media_folder, 1,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x07,
+			0x01, 0x01, 0x00, 0x6a,
+			0x00, 0x03, 'a', 'b', 'c');
+}
+
 int main(int argc, char *argv[])
 {
 	tester_init(&argc, &argv);
@@ -2080,5 +2473,7 @@ int main(int argc, char *argv[])
 				0x00, 0x19, 0x58, AVRCP_ABORT_CONTINUING,
 				0x00, 0x00, 0x00));
 
+	define_robustness_tests();
+
 	return tester_run();
 }
-- 
2.54.0