File sdpd-Fix-leaking-buffers-stored-in-cstates-cach... of Package bluez

Overview Repositories Revisions Requests Users Attributes Meta

File sdpd-Fix-leaking-buffers-stored-in-cstates-cache.patch of Package bluez (Revision 42a3f14c75c468416e20dd685f09849a)

Currently displaying revision 42a3f14c75c468416e20dd685f09849a , Show latest

From e79417ed7185b150a056d4eb3a1ab528b91d2fc0 Mon Sep 17 00:00:00 2001
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Date: Thu, 15 Jul 2021 11:01:20 -0700
Subject: [PATCH] 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 9f4b51dac..748cbeb61 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 9488535d3..d4b8f2f5b 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.35.3

Places

File sdpd-Fix-leaking-buffers-stored-in-cstates-cache.patch of Package bluez (Revision 42a3f14c75c468416e20dd685f09849a)

Places