From 264d94dcdb4c6739355f7251b4e9e81256938dbf Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Thu, 14 May 2026 12:57:17 -0400 Subject: [PATCH 1/6] Med: libcrmcommon: Add sanity checks to localized_remote_header. These checks are present in the main and 3.0 branches, and they make sense here as well. --- lib/common/remote.c | 52 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 47 insertions(+), 5 deletions(-) diff --git a/lib/common/remote.c b/lib/common/remote.c index b25e5ce..a4c8b8a 100644 --- a/lib/common/remote.c +++ b/lib/common/remote.c @@ -1,5 +1,5 @@ /* - * Copyright 2008-2024 the Pacemaker project contributors + * Copyright 2008-2026 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -23,7 +23,7 @@ #include #include #include -#include // PRIx32 +#include // PRIu32, PRIx32 #include #include @@ -97,11 +97,18 @@ struct remote_header_v0 { static struct remote_header_v0 * localized_remote_header(pcmk__remote_t *remote) { - struct remote_header_v0 *header = (struct remote_header_v0 *)remote->buffer; - if(remote->buffer_offset < sizeof(struct remote_header_v0)) { + struct remote_header_v0 *header = NULL; + size_t expected_size = 0; + + if ((remote == NULL) || (remote->buffer == NULL) + || (remote->buffer_offset < sizeof(struct remote_header_v0))) { + + // Caller error or we haven't received the full header yet return NULL; + } - } else if(header->endian != ENDIAN_LOCAL) { + header = (struct remote_header_v0 *) remote->buffer; + if (header->endian != ENDIAN_LOCAL) { uint32_t endian = __swab32(header->endian); CRM_LOG_ASSERT(endian == ENDIAN_LOCAL); @@ -123,6 +130,41 @@ localized_remote_header(pcmk__remote_t *remote) header->payload_uncompressed = __swab32(header->payload_uncompressed); } + // Sanity checks + if (header->payload_offset != sizeof(struct remote_header_v0)) { + crm_err("Header payload offset %" PRIu32 " does not have expected " + "size %zu", header->payload_offset, + sizeof(struct remote_header_v0)); + return NULL; + } + + if (header->payload_compressed != 0) { + if (header->payload_compressed > (SIZE_MAX - header->payload_offset)) { + crm_err("Header compressed size %" PRIu32 " is too large", + header->payload_compressed); + return NULL; + } + + expected_size = (size_t) header->payload_offset + + header->payload_compressed; + + } else { + if (header->payload_uncompressed > (SIZE_MAX - header->payload_offset)) { + crm_err("Header uncompressed size %" PRIu32 " is too large", + header->payload_uncompressed); + return NULL; + } + + expected_size = (size_t) header->payload_offset + + header->payload_uncompressed; + } + + if (expected_size != header->size_total) { + crm_err("Header total size %" PRIu32 " does not match calculated " + "size %zu", header->size_total, expected_size); + return NULL; + } + return header; } -- 2.53.0 From 8d21d297629bcc8aebf71bb171c550bcc4532c3f Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Thu, 14 May 2026 13:06:57 -0400 Subject: [PATCH 2/6] High: libcrmcommon: Fix integer overflow in remote message code. It's possible for a client to send a specifically constructed header packet that will cause our size checks in pcmk__remote_message_xml to overflow, which could result in crashes (potentially leading to a node being fenced) or memory corruption problems. Such a header would have to indicate that the payload is compressed, which nothing in pacemaker is doing. In fact, we haven't used any of the remote message compression code since it was introduced in 2013 which explains why we haven't seen any problems with it. Thus, it would require a malicious client to cause this to happen. It would also require remote CIB administration to be enabled via remote-clear-port or remote-tls-port, which we do not do by default. Additionally, if remote-tls-port is in use, it requires the client to complete the gnutls handshake procedure (pcmk__remote_message_xml is only called in cib_remote_msg after the handshake but before the client has authenticated). Co-Authored-By: Reid Wahl --- lib/common/remote.c | 49 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 7 deletions(-) diff --git a/lib/common/remote.c b/lib/common/remote.c index a4c8b8a..aa8e762 100644 --- a/lib/common/remote.c +++ b/lib/common/remote.c @@ -619,16 +619,51 @@ pcmk__remote_message_xml(pcmk__remote_t *remote) } /* Support compression on the receiving end now, in case we ever want to add it later */ - if (header->payload_compressed) { + if (header->payload_compressed != 0) { int rc = 0; - unsigned int size_u = 1 + header->payload_uncompressed; - char *uncompressed = - pcmk__assert_alloc(1, header->payload_offset + size_u); + unsigned int size_u = 0; + char *uncompressed = NULL; + +#if (UINT32_MAX < UINT_MAX) + if (header->payload_uncompressed >= UINT_MAX) { + crm_err("Couldn't decompress message because uncompressed " + "payload size (%" PRIu32 ") is greater than UINT_MAX " + "(%u)", header->payload_uncompressed, UINT_MAX); + return NULL; + } +#endif + + /* @TODO Is the extra byte for the null terminator? + * pcmk__remote_send_xml() also adds one byte to the iov length. + * (However, we do need to account for the possibility of receiving a + * message from an untrusted sender.) + */ + size_u = 1 + header->payload_uncompressed; + + /* Header and uncompressed payload must fit in the destination buffer. + * We do not need to separately check the header size here since + * localized_remote_header will return NULL if it's incorrect. + */ +#if (UINT_MAX >= SIZE_MAX) + if ((size_u >= SIZE_MAX) + || (header->payload_offset > (SIZE_MAX - size_u))) { +#else + if (header->payload_offset > (SIZE_MAX - size_u)) { +#endif + crm_err("Couldn't decompress message because the required buffer " + "size (%" PRIu32 " + %u) is greater than SIZE_MAX (%zu)", + header->payload_offset, size_u, SIZE_MAX); + return NULL; + } + + crm_trace("Decompressing message data %" PRIu32 " bytes into %u " + "bytes", header->payload_compressed, size_u); - crm_trace("Decompressing message data %d bytes into %d bytes", - header->payload_compressed, size_u); + uncompressed = pcmk__assert_alloc(header->payload_offset + size_u, + sizeof(char)); - rc = BZ2_bzBuffToBuffDecompress(uncompressed + header->payload_offset, &size_u, + rc = BZ2_bzBuffToBuffDecompress(uncompressed + header->payload_offset, + &size_u, remote->buffer + header->payload_offset, header->payload_compressed, 1, 0); rc = pcmk__bzlib2rc(rc); -- 2.53.0 From 77a530d1bb867264fa0d42752da44d75ee063830 Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Thu, 14 May 2026 13:26:07 -0400 Subject: [PATCH 3/6] High: libcrmcommon: Limit the max size of a remote message. Previously, we were just taking the sizes straight from the message header and allocating however much memory was asked for without checking. It's possible for a malicious client to ask for a very large amount of memory (say, 4 GB since that'll fit in the uint32_t value for header->size_total). In that case, due to the use of pcmk__assert_alloc, we'll crash due to an out of memory error and the node could potentially be fenced. The fix is to simply check that the client isn't asking for too much memory. This does unfortunately impose a size restriction on the message. It's doubly unfortunate given how much work we've recently put into removing restrictions on IPC messages allowing for much larger clusters. I think the biggest thing these messages will ever contain is a CIB, so for now I'm going with a limit of 20 MB. It is not my intention that this become a tuneable parameter that is exposed to the user. --- include/crm/common/remote_internal.h | 5 ++++- lib/common/remote.c | 17 +++++++++++++++-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/include/crm/common/remote_internal.h b/include/crm/common/remote_internal.h index f8c0bea..00f5918 100644 --- a/include/crm/common/remote_internal.h +++ b/include/crm/common/remote_internal.h @@ -1,5 +1,5 @@ /* - * Copyright 2008-2024 the Pacemaker project contributors + * Copyright 2008-2026 the Pacemaker project contributors * * The version control history for this file may have further details. * @@ -15,6 +15,9 @@ #include // pcmk_node_variant_remote #include // pcmk_node_t +// The maximum payload size for a remote message (in bytes) +#define PCMK__REMOTE_MSG_MAX_SIZE (20 * 1024 * 1024) + // internal functions from remote.c typedef struct pcmk__remote_s pcmk__remote_t; diff --git a/lib/common/remote.c b/lib/common/remote.c index aa8e762..08b6988 100644 --- a/lib/common/remote.c +++ b/lib/common/remote.c @@ -623,6 +623,7 @@ pcmk__remote_message_xml(pcmk__remote_t *remote) int rc = 0; unsigned int size_u = 0; char *uncompressed = NULL; + size_t buffer_size = 0; #if (UINT32_MAX < UINT_MAX) if (header->payload_uncompressed >= UINT_MAX) { @@ -656,11 +657,17 @@ pcmk__remote_message_xml(pcmk__remote_t *remote) return NULL; } + buffer_size = (size_t) header->payload_offset + size_u; + if (buffer_size > PCMK__REMOTE_MSG_MAX_SIZE) { + crm_err("Message size %zu is larger than max allowed %u bytes", + buffer_size, PCMK__REMOTE_MSG_MAX_SIZE); + return NULL; + } + crm_trace("Decompressing message data %" PRIu32 " bytes into %u " "bytes", header->payload_compressed, size_u); - uncompressed = pcmk__assert_alloc(header->payload_offset + size_u, - sizeof(char)); + uncompressed = pcmk__assert_alloc(buffer_size, sizeof(char)); rc = BZ2_bzBuffToBuffDecompress(uncompressed + header->payload_offset, &size_u, @@ -805,6 +812,12 @@ pcmk__read_available_remote_data(pcmk__remote_t *remote) read_len = header->size_total; } + if (read_len > PCMK__REMOTE_MSG_MAX_SIZE) { + crm_err("Message size %zu is larger than max allowed %u bytes", + read_len, PCMK__REMOTE_MSG_MAX_SIZE); + return EINVAL; + } + /* automatically grow the buffer when needed */ if(remote->buffer_size < read_len) { remote->buffer_size = 2 * read_len; -- 2.53.0 From eaae2d00a2d29c0187832a55a81cf42b39d93a25 Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Fri, 15 May 2026 10:19:33 -0400 Subject: [PATCH 4/6] High: libcrmcommon: Fix an integer overflow in pcmk__remote_send_xml. iov[0].iov_len is a size_t. iov[1].iov_len is also a size_t (which comes from the length of a GString, which is a gsize, which is also a size_t). We're adding those together and storing them in a uint32_t, so make sure the result will fit. Fixes RHEL-181155 --- lib/common/remote.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/common/remote.c b/lib/common/remote.c index 08b6988..83f050d 100644 --- a/lib/common/remote.c +++ b/lib/common/remote.c @@ -586,6 +586,13 @@ pcmk__remote_send_xml(pcmk__remote_t *remote, const xmlNode *msg) header->version = REMOTE_MSG_VERSION; header->payload_offset = iov[0].iov_len; header->payload_uncompressed = iov[1].iov_len; + + if ((UINT32_MAX - iov[0].iov_len) < iov[1].iov_len) { + crm_err("Remote message size %zu + %zu exceeds maximum of %" PRIu32, + iov[0].iov_len, iov[1].iov_len, UINT32_MAX); + goto done; + } + header->size_total = iov[0].iov_len + iov[1].iov_len; rc = remote_send_iovs(remote, iov, 2); @@ -594,6 +601,7 @@ pcmk__remote_send_xml(pcmk__remote_t *remote, const xmlNode *msg) pcmk_rc_str(rc), rc); } +done: free(iov[0].iov_base); g_free((gchar *) iov[1].iov_base); return rc; -- 2.53.0 From 2e097d03f1d93f95162a37a5c21ec46d911af176 Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Thu, 28 May 2026 12:56:57 -0400 Subject: [PATCH 5/6] Refactor: libcib: Remove an unnecessary coverity suppression. With all the previous changes, this suppression is no longer necessary and can be removed. --- lib/cib/cib_remote.c | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/cib/cib_remote.c b/lib/cib/cib_remote.c index baed17b..a64ff95 100644 --- a/lib/cib/cib_remote.c +++ b/lib/cib/cib_remote.c @@ -243,7 +243,6 @@ cib_remote_callback_dispatch(gpointer user_data) return -1; } - // coverity[tainted_data] This can't easily be changed right now msg = pcmk__remote_message_xml(&private->callback); if (msg == NULL) { private->start_time = 0; -- 2.53.0 From 2a3c713c3ceab8f5aded9693d09203d18b33bf55 Mon Sep 17 00:00:00 2001 From: Chris Lumens Date: Wed, 17 Jun 2026 15:55:28 -0400 Subject: [PATCH 6/6] Med: libcrmcommon: Add additional checks to pcmk__remote_message_xml. coverity flags the existing code for using a tainted expression as an index to a pointer. While we're fixing other range errors and overflows in this code, we should fix this one as well by porting the existing code from the main branch. --- lib/common/remote.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/common/remote.c b/lib/common/remote.c index 83f050d..9ad4b17 100644 --- a/lib/common/remote.c +++ b/lib/common/remote.c @@ -620,6 +620,7 @@ xmlNode * pcmk__remote_message_xml(pcmk__remote_t *remote) { xmlNode *xml = NULL; + size_t data_size = 0; struct remote_header_v0 *header = localized_remote_header(remote); if (header == NULL) { @@ -709,7 +710,18 @@ pcmk__remote_message_xml(pcmk__remote_t *remote) /* take ownership of the buffer */ remote->buffer_offset = 0; - CRM_LOG_ASSERT(remote->buffer[sizeof(struct remote_header_v0) + header->payload_uncompressed - 1] == 0); + data_size = (size_t) header->payload_offset + header->payload_uncompressed; + + // Ensure the buffer is as big as it should be + CRM_CHECK(remote->buffer_size >= data_size, return NULL); + + /* Ensure the buffer is null-terminated (see + * pcmk__read_available_remote_data()). + * + * Note that payload_uncompressed contains the payload size including the + * null byte (see pcmk__remote_send_xml()). + */ + CRM_CHECK(remote->buffer[data_size] == '\0', return NULL); xml = pcmk__xml_parse(remote->buffer + header->payload_offset); if (xml == NULL && header->version > REMOTE_MSG_VERSION) { -- 2.53.0