diff mbox series

[BlueZ,1/2] sdpd: Fix leaking buffers stored in cstates cache

Message ID 20211112220538.310085-1-luiz.dentz@gmail.com
State New
Headers show
Series [BlueZ,1/2] sdpd: Fix leaking buffers stored in cstates cache | expand

Commit Message

Luiz Augusto von Dentz Nov. 12, 2021, 10:05 p.m. UTC
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

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 mbox series

Patch

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);