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

Currently displaying revision 7d77465cfd7b56cfe7d1aea236c6fae2 , 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(-)

Index: bluez-5.55/src/sdpd-request.c
===================================================================
--- bluez-5.55.orig/src/sdpd-request.c
+++ bluez-5.55/src/sdpd-request.c
@@ -55,48 +55,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) */
@@ -287,14 +317,16 @@ static int sdp_set_cstate_pdu(sdp_buf_t
 	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;
 	}
@@ -319,6 +351,8 @@ static int sdp_cstate_get(uint8_t *buffe
 	SDPDBG("Cstate TS : 0x%x", (*cstate)->timestamp);
 	SDPDBG("Bytes sent : %d", (*cstate)->cStateValue.maxBytesSent);
 
+	*cinfo = sdp_get_cont_info(req, *cstate);
+
 	return 0;
 }
 
@@ -373,6 +407,7 @@ static int service_search_req(sdp_req_t
 	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;
@@ -412,9 +447,9 @@ static int service_search_req(sdp_req_t
 
 	/*
 	 * 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;
 	}
@@ -464,7 +499,7 @@ static int service_search_req(sdp_req_t
 
 		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
@@ -472,6 +507,7 @@ static int service_search_req(sdp_req_t
 			buf->data_size -= handleSize;
 		} else {
 			/* NULL continuation state */
+			sdp_cont_info_free(cinfo);
 			sdp_set_cstate_pdu(buf, NULL);
 		}
 	}
@@ -481,13 +517,15 @@ static int service_search_req(sdp_req_t
 		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);
 
@@ -531,6 +569,7 @@ static int service_search_req(sdp_req_t
 		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
@@ -553,6 +592,7 @@ static int service_search_req(sdp_req_t
 
 done:
 	free(cstate);
+
 	if (pattern)
 		sdp_list_free(pattern, free);
 
@@ -632,15 +672,21 @@ static int extract_attrs(sdp_record_t *r
 }
 
 /* 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);
@@ -650,8 +696,10 @@ static int sdp_cstate_rsp(sdp_cont_state
 	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);
 }
@@ -665,6 +713,7 @@ static int sdp_cstate_rsp(sdp_cont_state
 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;
@@ -721,7 +770,7 @@ static int service_attr_req(sdp_req_t *r
 	 * 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;
 	}
@@ -750,7 +799,7 @@ static int service_attr_req(sdp_req_t *r
 	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");
@@ -762,7 +811,7 @@ static int service_attr_req(sdp_req_t *r
 			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
@@ -806,6 +855,7 @@ static int service_search_attr_req(sdp_r
 	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;
@@ -865,7 +915,7 @@ static int service_search_attr_req(sdp_r
 	 * 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;
 	}
@@ -919,7 +969,7 @@ static int service_search_attr_req(sdp_r
 			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
@@ -930,7 +980,7 @@ static int service_search_attr_req(sdp_r
 		} 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");
@@ -987,6 +1037,9 @@ static void process_request(sdp_req_t *r
 		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");
@@ -1033,6 +1086,8 @@ static void process_request(sdp_req_t *r
 
 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);
@@ -1121,3 +1176,20 @@ void handle_request(int sk, uint8_t *dat
 
 	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);
+	}
+}
Index: bluez-5.55/src/sdpd-server.c
===================================================================
--- bluez-5.55.orig/src/sdpd-server.c
+++ bluez-5.55/src/sdpd-server.c
@@ -159,16 +159,12 @@ static gboolean io_session_event(GIOChan
 
 	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);
@@ -181,14 +177,18 @@ static gboolean io_session_event(GIOChan
 	 * 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)
Index: bluez-5.55/src/sdpd.h
===================================================================
--- bluez-5.55.orig/src/sdpd.h
+++ bluez-5.55/src/sdpd.h
@@ -40,8 +40,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);
 
Index: bluez-5.55/unit/test-sdp.c
===================================================================
--- bluez-5.55.orig/unit/test-sdp.c
+++ bluez-5.55/unit/test-sdp.c
@@ -248,7 +248,7 @@ static gboolean client_handler(GIOChanne
 	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);

Places

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

Places