From 4e6a2402ed4f46ea026ad0929fbc14faecf3a475 Mon Sep 17 00:00:00 2001

From: Gopal Tiwari <gtiwari@redhat.com>

Date: Wed, 1 Dec 2021 12:18:24 +0530

Subject: [PATCH BlueZ] sdpd: Fix leaking buffers stored in cstates cache

commit e79417ed7185b150a056d4eb3a1ab528b91d2fc0

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

Date:   Thu Jul 15 11:01:20 2021 -0700

    sdpd: Fix leaking buffers stored in cstates cache

    These buffer shall only be keep in cache for as long as they are

    needed so this would cleanup any client cstates in the following

    conditions:

     - There is no cstate on the response

     - No continuation can be found for cstate

     - Different request opcode

     - Respond with an error

     - Client disconnect

    Fixes: https://github.com/bluez/bluez/security/advisories/GHSA-3fqg-r8j5-f5xq

---

 src/sdpd-request.c | 170 ++++++++++++++++++++++++++++++++-------------

 src/sdpd-server.c  |  20 +++---

 src/sdpd.h         |   3 +

 unit/test-sdp.c    |   2 +-

 4 files changed, 135 insertions(+), 60 deletions(-)

diff --git a/src/sdpd-request.c b/src/sdpd-request.c

index 033d1e5bf..c8f5a2c72 100644

--- a/src/sdpd-request.c

+++ b/src/sdpd-request.c

@@ -42,48 +42,78 @@ typedef struct {

 #define MIN(x, y) ((x) < (y)) ? (x): (y)

-typedef struct _sdp_cstate_list sdp_cstate_list_t;

+typedef struct sdp_cont_info sdp_cont_info_t;

-struct _sdp_cstate_list {

-	sdp_cstate_list_t *next;

+struct sdp_cont_info {

+	int sock;

+	uint8_t opcode;

 	uint32_t timestamp;

 	sdp_buf_t buf;

 };

-static sdp_cstate_list_t *cstates;

+static sdp_list_t *cstates;

-/* FIXME: should probably remove it when it's found */

-static sdp_buf_t *sdp_get_cached_rsp(sdp_cont_state_t *cstate)

+static int cstate_match(const void *data, const void *user_data)

 {

-	sdp_cstate_list_t *p;

+	const sdp_cont_info_t *cinfo = data;

+	const sdp_cont_state_t *cstate = user_data;

-	for (p = cstates; p; p = p->next) {

-		/* Check timestamp */

-		if (p->timestamp != cstate->timestamp)

-			continue;

+	/* Check timestamp */

+	return cinfo->timestamp - cstate->timestamp;

+}

+

+static void sdp_cont_info_free(sdp_cont_info_t *cinfo)

+{

+	if (!cinfo)

+		return;

+

+	cstates = sdp_list_remove(cstates, cinfo);

+	free(cinfo->buf.data);

+	free(cinfo);

+}

+

+static sdp_cont_info_t *sdp_get_cont_info(sdp_req_t *req,

+						sdp_cont_state_t *cstate)

+{

+	sdp_list_t *list;

+

+	list = sdp_list_find(cstates, cstate, cstate_match);

+	if (list) {

+		sdp_cont_info_t *cinfo = list->data;

-		/* Check if requesting more than available */

-		if (cstate->cStateValue.maxBytesSent < p->buf.data_size)

-			return &p->buf;

+		if (cinfo->opcode == req->opcode)

+			return cinfo;

+

+		/* Cleanup continuation if the opcode doesn't match since its

+		 * response buffer shall only be valid for the original requests

+		 */

+		sdp_cont_info_free(cinfo);

+		return NULL;

 	}

-	return 0;

+	/* Cleanup cstates if no continuation info could be found */

+	sdp_cstate_cleanup(req->sock);

+

+	return NULL;

 }

-static uint32_t sdp_cstate_alloc_buf(sdp_buf_t *buf)

+static uint32_t sdp_cstate_alloc_buf(sdp_req_t *req, sdp_buf_t *buf)

 {

-	sdp_cstate_list_t *cstate = malloc(sizeof(sdp_cstate_list_t));

+	sdp_cont_info_t *cinfo = malloc(sizeof(sdp_cont_info_t));

 	uint8_t *data = malloc(buf->data_size);

 	memcpy(data, buf->data, buf->data_size);

-	memset((char *)cstate, 0, sizeof(sdp_cstate_list_t));

-	cstate->buf.data = data;

-	cstate->buf.data_size = buf->data_size;

-	cstate->buf.buf_size = buf->data_size;

-	cstate->timestamp = sdp_get_time();

-	cstate->next = cstates;

-	cstates = cstate;

-	return cstate->timestamp;

+	memset(cinfo, 0, sizeof(sdp_cont_info_t));

+	cinfo->buf.data = data;

+	cinfo->buf.data_size = buf->data_size;

+	cinfo->buf.buf_size = buf->data_size;

+	cinfo->timestamp = sdp_get_time();

+	cinfo->sock = req->sock;

+	cinfo->opcode = req->opcode;

+

+	cstates = sdp_list_append(cstates, cinfo);

+

+	return cinfo->timestamp;

 }

 /* Additional values for checking datatype (not in spec) */

@@ -274,14 +304,16 @@ static int sdp_set_cstate_pdu(sdp_buf_t *buf, sdp_cont_state_t *cstate)

 	return length;

 }

-static int sdp_cstate_get(uint8_t *buffer, size_t len,

-						sdp_cont_state_t **cstate)

+static int sdp_cstate_get(sdp_req_t *req, uint8_t *buffer, size_t len,

+			sdp_cont_state_t **cstate, sdp_cont_info_t **cinfo)

 {

 	uint8_t cStateSize = *buffer;

 	SDPDBG("Continuation State size : %d", cStateSize);

 	if (cStateSize == 0) {

+		/* Cleanup cstates if request doesn't contain a cstate */

+		sdp_cstate_cleanup(req->sock);

 		*cstate = NULL;

 		return 0;

 	}

@@ -306,6 +338,8 @@ static int sdp_cstate_get(uint8_t *buffer, size_t len,

 	SDPDBG("Cstate TS : 0x%x", (*cstate)->timestamp);

 	SDPDBG("Bytes sent : %d", (*cstate)->cStateValue.maxBytesSent);

+	*cinfo = sdp_get_cont_info(req, *cstate);

+

 	return 0;

 }

@@ -360,6 +394,7 @@ static int service_search_req(sdp_req_t *req, sdp_buf_t *buf)

 	uint16_t expected, actual, rsp_count = 0;

 	uint8_t dtd;

 	sdp_cont_state_t *cstate = NULL;

+	sdp_cont_info_t *cinfo = NULL;

 	uint8_t *pCacheBuffer = NULL;

 	int handleSize = 0;

 	uint32_t cStateId = 0;

@@ -399,9 +434,9 @@ static int service_search_req(sdp_req_t *req, sdp_buf_t *buf)

 	/*

 	 * Check if continuation state exists, if yes attempt

-	 * to get rsp remainder from cache, else send error

+	 * to get rsp remainder from continuation info, else send error

 	 */

-	if (sdp_cstate_get(pdata, data_left, &cstate) < 0) {

+	if (sdp_cstate_get(req, pdata, data_left, &cstate, &cinfo) < 0) {

 		status = SDP_INVALID_SYNTAX;

 		goto done;

 	}

@@ -451,7 +486,7 @@ static int service_search_req(sdp_req_t *req, sdp_buf_t *buf)

 		if (rsp_count > actual) {

 			/* cache the rsp and generate a continuation state */

-			cStateId = sdp_cstate_alloc_buf(buf);

+			cStateId = sdp_cstate_alloc_buf(req, buf);

 			/*

 			 * subtract handleSize since we now send only

 			 * a subset of handles

@@ -459,6 +494,7 @@ static int service_search_req(sdp_req_t *req, sdp_buf_t *buf)

 			buf->data_size -= handleSize;

 		} else {

 			/* NULL continuation state */

+			sdp_cont_info_free(cinfo);

 			sdp_set_cstate_pdu(buf, NULL);

 		}

 	}

@@ -468,13 +504,15 @@ static int service_search_req(sdp_req_t *req, sdp_buf_t *buf)

 		short lastIndex = 0;

 		if (cstate) {

-			/*

-			 * Get the previous sdp_cont_state_t and obtain

-			 * the cached rsp

-			 */

-			sdp_buf_t *pCache = sdp_get_cached_rsp(cstate);

-			if (pCache) {

-				pCacheBuffer = pCache->data;

+			if (cinfo) {

+				/* Check if requesting more than available */

+				if (cstate->cStateValue.maxBytesSent >=

+						cinfo->buf.data_size) {

+					status = SDP_INVALID_CSTATE;

+					goto done;

+				}

+

+				pCacheBuffer = cinfo->buf.data;

 				/* get the rsp_count from the cached buffer */

 				rsp_count = get_be16(pCacheBuffer);

@@ -518,6 +556,7 @@ static int service_search_req(sdp_req_t *req, sdp_buf_t *buf)

 		if (i == rsp_count) {

 			/* set "null" continuationState */

 			sdp_set_cstate_pdu(buf, NULL);

+			sdp_cont_info_free(cinfo);

 		} else {

 			/*

 			 * there's more: set lastIndexSent to

@@ -540,6 +579,7 @@ static int service_search_req(sdp_req_t *req, sdp_buf_t *buf)

 done:

 	free(cstate);

+

 	if (pattern)

 		sdp_list_free(pattern, free);

@@ -619,15 +659,21 @@ static int extract_attrs(sdp_record_t *rec, sdp_list_t *seq, sdp_buf_t *buf)

 }

 /* Build cstate response */

-static int sdp_cstate_rsp(sdp_cont_state_t *cstate, sdp_buf_t *buf,

-							uint16_t max)

+static int sdp_cstate_rsp(sdp_cont_info_t *cinfo, sdp_cont_state_t *cstate,

+					sdp_buf_t *buf, uint16_t max)

 {

-	/* continuation State exists -> get from cache */

-	sdp_buf_t *cache = sdp_get_cached_rsp(cstate);

+	sdp_buf_t *cache;

 	uint16_t sent;

-	if (!cache)

+	if (!cinfo)

+		return 0;

+

+	if (cstate->cStateValue.maxBytesSent >= cinfo->buf.data_size) {

+		sdp_cont_info_free(cinfo);

 		return 0;

+	}

+

+	cache = &cinfo->buf;

 	sent = MIN(max, cache->data_size - cstate->cStateValue.maxBytesSent);

 	memcpy(buf->data, cache->data + cstate->cStateValue.maxBytesSent, sent);

@@ -637,8 +683,10 @@ static int sdp_cstate_rsp(sdp_cont_state_t *cstate, sdp_buf_t *buf,

 	SDPDBG("Response size : %d sending now : %d bytes sent so far : %d",

 		cache->data_size, sent, cstate->cStateValue.maxBytesSent);

-	if (cstate->cStateValue.maxBytesSent == cache->data_size)

+	if (cstate->cStateValue.maxBytesSent == cache->data_size) {

+		sdp_cont_info_free(cinfo);

 		return sdp_set_cstate_pdu(buf, NULL);

+	}

 	return sdp_set_cstate_pdu(buf, cstate);

 }

@@ -652,6 +700,7 @@ static int sdp_cstate_rsp(sdp_cont_state_t *cstate, sdp_buf_t *buf,

 static int service_attr_req(sdp_req_t *req, sdp_buf_t *buf)

 {

 	sdp_cont_state_t *cstate = NULL;

+	sdp_cont_info_t *cinfo = NULL;

 	short cstate_size = 0;

 	sdp_list_t *seq = NULL;

 	uint8_t dtd = 0;

@@ -708,7 +757,7 @@ static int service_attr_req(sdp_req_t *req, sdp_buf_t *buf)

 	 * if continuation state exists, attempt

 	 * to get rsp remainder from cache, else send error

 	 */

-	if (sdp_cstate_get(pdata, data_left, &cstate) < 0) {

+	if (sdp_cstate_get(req, pdata, data_left, &cstate, &cinfo) < 0) {

 		status = SDP_INVALID_SYNTAX;

 		goto done;

 	}

@@ -737,7 +786,7 @@ static int service_attr_req(sdp_req_t *req, sdp_buf_t *buf)

 	buf->buf_size -= sizeof(uint16_t);

 	if (cstate) {

-		cstate_size = sdp_cstate_rsp(cstate, buf, max_rsp_size);

+		cstate_size = sdp_cstate_rsp(cinfo, cstate, buf, max_rsp_size);

 		if (!cstate_size) {

 			status = SDP_INVALID_CSTATE;

 			error("NULL cache buffer and non-NULL continuation state");

@@ -749,7 +798,7 @@ static int service_attr_req(sdp_req_t *req, sdp_buf_t *buf)

 			sdp_cont_state_t newState;

 			memset((char *)&newState, 0, sizeof(sdp_cont_state_t));

-			newState.timestamp = sdp_cstate_alloc_buf(buf);

+			newState.timestamp = sdp_cstate_alloc_buf(req, buf);

 			/*

 			 * Reset the buffer size to the maximum expected and

 			 * set the sdp_cont_state_t

@@ -793,6 +842,7 @@ static int service_search_attr_req(sdp_req_t *req, sdp_buf_t *buf)

 	int scanned, rsp_count = 0;

 	sdp_list_t *pattern = NULL, *seq = NULL, *svcList;

 	sdp_cont_state_t *cstate = NULL;

+	sdp_cont_info_t *cinfo = NULL;

 	short cstate_size = 0;

 	uint8_t dtd = 0;

 	sdp_buf_t tmpbuf;

@@ -852,7 +902,7 @@ static int service_search_attr_req(sdp_req_t *req, sdp_buf_t *buf)

 	 * if continuation state exists attempt

 	 * to get rsp remainder from cache, else send error

 	 */

-	if (sdp_cstate_get(pdata, data_left, &cstate) < 0) {

+	if (sdp_cstate_get(req, pdata, data_left, &cstate, &cinfo) < 0) {

 		status = SDP_INVALID_SYNTAX;

 		goto done;

 	}

@@ -906,7 +956,7 @@ static int service_search_attr_req(sdp_req_t *req, sdp_buf_t *buf)

 			sdp_cont_state_t newState;

 			memset((char *)&newState, 0, sizeof(sdp_cont_state_t));

-			newState.timestamp = sdp_cstate_alloc_buf(buf);

+			newState.timestamp = sdp_cstate_alloc_buf(req, buf);

 			/*

 			 * Reset the buffer size to the maximum expected and

 			 * set the sdp_cont_state_t

@@ -917,7 +967,7 @@ static int service_search_attr_req(sdp_req_t *req, sdp_buf_t *buf)

 		} else

 			cstate_size = sdp_set_cstate_pdu(buf, NULL);

 	} else {

-		cstate_size = sdp_cstate_rsp(cstate, buf, max);

+		cstate_size = sdp_cstate_rsp(cinfo, cstate, buf, max);

 		if (!cstate_size) {

 			status = SDP_INVALID_CSTATE;

 			SDPDBG("Non-null continuation state, but null cache buffer");

@@ -974,6 +1024,9 @@ static void process_request(sdp_req_t *req)

 		status = SDP_INVALID_PDU_SIZE;

 		goto send_rsp;

 	}

+

+	req->opcode = reqhdr->pdu_id;

+

 	switch (reqhdr->pdu_id) {

 	case SDP_SVC_SEARCH_REQ:

 		SDPDBG("Got a svc srch req");

@@ -1020,6 +1073,8 @@ static void process_request(sdp_req_t *req)

 send_rsp:

 	if (status) {

+		/* Cleanup cstates on error */

+		sdp_cstate_cleanup(req->sock);

 		rsphdr->pdu_id = SDP_ERROR_RSP;

 		put_be16(status, rsp.data);

 		rsp.data_size = sizeof(uint16_t);

@@ -1108,3 +1163,20 @@ void handle_request(int sk, uint8_t *data, int len)

 	process_request(&req;;

 }

+

+void sdp_cstate_cleanup(int sock)

+{

+	sdp_list_t *list;

+

+	/* Remove any cinfo for the client */

+	for (list = cstates; list;) {

+		sdp_cont_info_t *cinfo = list->data;

+

+		list = list->next;

+

+		if (cinfo->sock != sock)

+			continue;

+

+		sdp_cont_info_free(cinfo);

+	}

+}

diff --git a/src/sdpd-server.c b/src/sdpd-server.c

index dfd8b1f00..66ee7ba14 100644

--- a/src/sdpd-server.c

+++ b/src/sdpd-server.c

@@ -146,16 +146,12 @@ static gboolean io_session_event(GIOChannel *chan, GIOCondition cond, gpointer d

 	sk = g_io_channel_unix_get_fd(chan);

-	if (cond & (G_IO_HUP | G_IO_ERR)) {

-		sdp_svcdb_collect_all(sk);

-		return FALSE;

-	}

+	if (cond & (G_IO_HUP | G_IO_ERR))

+		goto cleanup;

 	len = recv(sk, &hdr, sizeof(sdp_pdu_hdr_t), MSG_PEEK);

-	if (len < 0 || (unsigned int) len < sizeof(sdp_pdu_hdr_t)) {

-		sdp_svcdb_collect_all(sk);

-		return FALSE;

-	}

+	if (len < 0 || (unsigned int) len < sizeof(sdp_pdu_hdr_t))

+		goto cleanup;

 	size = sizeof(sdp_pdu_hdr_t) + ntohs(hdr.plen);

 	buf = malloc(size);

@@ -168,14 +164,18 @@ static gboolean io_session_event(GIOChannel *chan, GIOCondition cond, gpointer d

 	 * inside handle_request() in order to produce ErrorResponse.

 	 */

 	if (len <= 0) {

-		sdp_svcdb_collect_all(sk);

 		free(buf);

-		return FALSE;

+		goto cleanup;

 	}

 	handle_request(sk, buf, len);

 	return TRUE;

+

+cleanup:

+	sdp_svcdb_collect_all(sk);

+	sdp_cstate_cleanup(sk);

+	return FALSE;

 }

 static gboolean io_accept_event(GIOChannel *chan, GIOCondition cond, gpointer data)

diff --git a/src/sdpd.h b/src/sdpd.h

index 257411f03..4316aff67 100644

--- a/src/sdpd.h

+++ b/src/sdpd.h

@@ -27,8 +27,11 @@ typedef struct request {

 	int      flags;

 	uint8_t  *buf;

 	int      len;

+	uint8_t  opcode;

 } sdp_req_t;

+void sdp_cstate_cleanup(int sock);

+

 void handle_internal_request(int sk, int mtu, void *data, int len);

 void handle_request(int sk, uint8_t *data, int len);

diff --git a/unit/test-sdp.c b/unit/test-sdp.c

index d3a885f19..8f95fcb71 100644

--- a/unit/test-sdp.c

+++ b/unit/test-sdp.c

@@ -235,7 +235,7 @@ static gboolean client_handler(GIOChannel *channel, GIOCondition cond,

 	tester_monitor('>', 0x0000, 0x0001, buf, len);

 	g_assert(len > 0);

-	g_assert((size_t) len == rsp_pdu->raw_size + rsp_pdu->cont_len);

+	g_assert_cmpuint(len, ==, rsp_pdu->raw_size + rsp_pdu->cont_len);

 	g_assert(memcmp(buf, rsp_pdu->raw_data,	rsp_pdu->raw_size) == 0);

-- 

2.26.2