diff --git a/SOURCES/0001-sdpd-Fix-leaking-buffers-stored-in-cstates-cache.patch b/SOURCES/0001-sdpd-Fix-leaking-buffers-stored-in-cstates-cache.patch new file mode 100644 index 0000000..11ce041 --- /dev/null +++ b/SOURCES/0001-sdpd-Fix-leaking-buffers-stored-in-cstates-cache.patch @@ -0,0 +1,468 @@ +From 4e6a2402ed4f46ea026ad0929fbc14faecf3a475 Mon Sep 17 00:00:00 2001 +From: Gopal Tiwari +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 +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 + diff --git a/SPECS/bluez.spec b/SPECS/bluez.spec index 6473e9b..49a9776 100644 --- a/SPECS/bluez.spec +++ b/SPECS/bluez.spec @@ -1,7 +1,7 @@ Name: bluez Summary: Bluetooth utilities Version: 5.56 -Release: 2%{?dist} +Release: 3%{?dist} License: GPLv2+ URL: http://www.bluez.org/ @@ -46,6 +46,8 @@ Patch24: 0001-adapter-Don-t-refresh-adv_manager-for-non-LE-devices.patch # fixing https://bugzilla.redhat.com/show_bug.cgi?id=1965057 Patch31: 0001-shared-gatt-server-Fix-not-properly-checking-for-sec.patch +Patch32: 0001-sdpd-Fix-leaking-buffers-stored-in-cstates-cache.patch + BuildRequires: git-core BuildRequires: dbus-devel >= 1.6 BuildRequires: glib2-devel @@ -279,6 +281,11 @@ make check %changelog +* Wed Dec 13 2021 Gopal Tiwari - 5.56-3 ++ bluez-5.56-3 +- Fixing (#2027434) +- Fixing CVE-2021-41229 + * Mon Jun 7 2021 Gopal Tiwari - 5.56-2 + bluez-5.56-2 - Fixing (#1968392)