Refactor: libcrmcommon: Sanity-check remote message and buffer sizes

Coverity has been complaining about the payload_offset and
payload_uncompressed being tainted scalar values. There's only so much
validation we can do when we're reading from a socket. But apparently
this is enough, because it makes the Coverity errors go away.

There's a lot more room for improvement in the remote message
processing. I found a few bugs a while back that we need to fix
involving multiple messages received in rapid succession. This is an
improvement for now.

Note that I got rid of the CRM_LOG_ASSERT() line that subtracts 1 from
the index. As far as I can tell, that's an off-by-one error and we have
no reason to expect that position to contain a null byte. The commit
that added it doesn't have any information in the commit message or
comments.

Signed-off-by: Reid Wahl <[email protected]>

Author: nrwahl2

lib/common/remote.c

@@ -68,35 +68,47 @@ static struct remote_header_v0 *
 localized_remote_header(pcmk__remote_t *remote)
 {
     struct remote_header_v0 *header = NULL;
-    uint32_t endian_swapped = 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;
     }
 
     header = (struct remote_header_v0 *) remote->buffer;
-    if (header->endian == ENDIAN_LOCAL) {
-        return header;
-    }
-
-    endian_swapped = GUINT32_SWAP_LE_BE(header->endian);
-    CRM_CHECK(endian_swapped == ENDIAN_LOCAL,
-              crm_err("Invalid message detected (endian mismatch): local magic "
-                      "number %" PRIx32 " matches neither the header's magic "
-                      "number %" PRIx32 " nor the byte-swapped form %" PRIx32,
-                      ENDIAN_LOCAL, header->endian, endian_swapped);
-              return NULL);
-
-    header->endian = endian_swapped;
-    header->version = GUINT32_SWAP_LE_BE(header->version);
-    header->id = GUINT64_SWAP_LE_BE(header->id);
-    header->flags = GUINT64_SWAP_LE_BE(header->flags);
-    header->size_total = GUINT32_SWAP_LE_BE(header->size_total);
-    header->payload_offset = GUINT32_SWAP_LE_BE(header->payload_offset);
-    header->payload_compressed = GUINT32_SWAP_LE_BE(header->payload_compressed);
-    header->payload_uncompressed =
-        GUINT32_SWAP_LE_BE(header->payload_uncompressed);
+    if (header->endian != ENDIAN_LOCAL) {
+        uint32_t endian_swapped = GUINT32_SWAP_LE_BE(header->endian);
+
+        CRM_CHECK(endian_swapped == ENDIAN_LOCAL,
+                  crm_err("Invalid message detected (endian mismatch): local "
+                          "magic number %" PRIx32 " matches neither the "
+                          "header's magic number %" PRIx32 " nor the "
+                          "byte-swapped form %" PRIx32,
+                          ENDIAN_LOCAL, header->endian, endian_swapped);
+                  return NULL);
+
+        header->endian = endian_swapped;
+        header->version = GUINT32_SWAP_LE_BE(header->version);
+        header->id = GUINT64_SWAP_LE_BE(header->id);
+        header->flags = GUINT64_SWAP_LE_BE(header->flags);
+        header->size_total = GUINT32_SWAP_LE_BE(header->size_total);
+        header->payload_offset = GUINT32_SWAP_LE_BE(header->payload_offset);
+        header->payload_compressed =
+            GUINT32_SWAP_LE_BE(header->payload_compressed);
+        header->payload_uncompressed =
+            GUINT32_SWAP_LE_BE(header->payload_uncompressed);
+    }
+
+    // Sanity checks
+    if (header->payload_offset != sizeof(struct remote_header_v0)) {
+        return NULL;
+    }
+    if ((header->payload_offset
+         + header->payload_compressed
+         + header->payload_uncompressed) != header->size_total) {
+        return NULL;
+    }
 
     return header;
 }
@@ -268,6 +280,8 @@ xmlNode *
 pcmk__remote_message_xml(pcmk__remote_t *remote)
 {
     xmlNode *xml = NULL;
+    size_t data_size = 0;
+    char *payload = NULL;
     struct remote_header_v0 *header = localized_remote_header(remote);
 
     if (header == NULL) {
@@ -315,18 +329,33 @@ 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;
 
-    xml = pcmk__xml_parse(remote->buffer + header->payload_offset);
-    if (xml == NULL && header->version > REMOTE_MSG_VERSION) {
-        crm_warn("Couldn't parse v%d message, we only understand v%d",
-                 header->version, REMOTE_MSG_VERSION);
+    // Ensure the buffer is as big as it should be
+    CRM_CHECK(remote->buffer_size >= data_size, return NULL);
 
-    } else if (xml == NULL) {
-        crm_err("Couldn't parse: '%.120s'", remote->buffer + header->payload_offset);
-    }
+    /* 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);
+
+    payload = remote->buffer + header->payload_offset;
+
+    xml = pcmk__xml_parse(payload);
+    if (xml == NULL) {
+        if (header->version > REMOTE_MSG_VERSION) {
+            crm_warn("Couldn't parse v%d message, we only understand v%d",
+                     header->version, REMOTE_MSG_VERSION);
+        } else {
+            crm_err("Couldn't parse: '%.120s'", payload);
+        }
 
-    crm_log_xml_trace(xml, "[remote msg]");
+    } else {
+        crm_log_xml_trace(xml, "[remote msg]");
+    }
     return xml;
 }