remove strcpy, and replace with an iio_strlcpy

We used strcpy a few places, which opens up classic buffer overflow
issues so check those, and fix them.

https://cwe.mitre.org/data/definitions/120.html

This adds some internal error checking, as we are building up xml
representations of things, we now keep track of the remaining space
in the buffer, and don't go over if we have no space. (which in theory
should never happen).

Tested on Pluto and M2k to see if this introduces issues, and couldn't
find anything.

Signed-off-by: Robin Getz <[email protected]>

channel.c

@@ -226,13 +226,16 @@ static char * get_scan_element(const struct iio_channel *chn, size_t *length)
 /* Returns a string containing the XML representation of this channel */
 char * iio_channel_get_xml(const struct iio_channel *chn, size_t *length)
 {
-	size_t len = sizeof("<channel id=\"\" name=\"\" "
-			"type=\"output\" ></channel>")
-		+ strlen(chn->id) + (chn->name ? strlen(chn->name) : 0);
-	char *ptr, *str, **attrs, *scan_element = NULL;
+	ssize_t len;
+	char *ptr, *eptr, *str, **attrs, *scan_element = NULL;
 	size_t *attrs_len, scan_element_len = 0;
 	unsigned int i;
 
+	len = sizeof("<channel id=\"\" type=\"\" ></channel>");
+	len += strnlen(chn->id, MAX_CHN_ID);
+	len += chn->is_output ? sizeof("output") : sizeof("input");
+	len += chn->name ? sizeof(" name= ") + strnlen(chn->name, MAX_CHN_NAME) : 0;
+
 	if (chn->is_scan_element) {
 		scan_element = get_scan_element(chn, &scan_element_len);
 		if (!scan_element)
@@ -260,35 +263,46 @@ char * iio_channel_get_xml(const struct iio_channel *chn, size_t *length)
 	str = malloc(len);
 	if (!str)
 		goto err_free_attrs;
+	eptr = str + len;
 
 	iio_snprintf(str, len, "<channel id=\"%s\"", chn->id);
 	ptr = strrchr(str, '\0');
+	len = eptr - ptr;
 
 	if (chn->name) {
-		sprintf(ptr, " name=\"%s\"", chn->name);
+		iio_snprintf(ptr, len, " name=\"%s\"", chn->name);
 		ptr = strrchr(ptr, '\0');
+		len = eptr - ptr;
 	}
 
-	sprintf(ptr, " type=\"%s\" >", chn->is_output ? "output" : "input");
+	iio_snprintf(ptr, len, " type=\"%s\" >", chn->is_output ? "output" : "input");
 	ptr = strrchr(ptr, '\0');
+	len = eptr - ptr;
 
 	if (chn->is_scan_element) {
-		strcpy(ptr, scan_element);
+		iio_strlcpy(ptr, scan_element, len);
 		ptr += scan_element_len;
+		len -= scan_element_len;
 	}
 
 	for (i = 0; i < chn->nb_attrs; i++) {
-		strcpy(ptr, attrs[i]);
+		iio_strlcpy(ptr, attrs[i], len);
 		ptr += attrs_len[i];
+		len -= attrs_len[i];
 		free(attrs[i]);
 	}
 
 	free(scan_element);
 	free(attrs);
 	free(attrs_len);
 
-	strcpy(ptr, "</channel>");
+	iio_strlcpy(ptr, "</channel>", len);
 	*length = ptr - str + sizeof("</channel>") - 1;
+	len -= sizeof("</channel>");
+
+	if (len < 0)
+		IIO_ERROR("Internal libIIO error: iio_channel_get_xml str length isssue\n");
+
 	return str;
 
 err_free_attrs:

context.c

@@ -53,8 +53,9 @@ static const char xml_header[] = "<?xml version=\"1.0\" encoding=\"utf-8\"?>"
 /* Returns a string containing the XML representation of this context */
 char * iio_context_create_xml(const struct iio_context *ctx)
 {
-	size_t len, *devices_len = NULL;
-	char *str, *ptr, **devices = NULL;
+	ssize_t len;
+	size_t *devices_len = NULL;
+	char *str, *ptr, *eptr, **devices = NULL;
 	unsigned int i;
 
 	len = strlen(ctx->name) + sizeof(xml_header) - 1 +
@@ -94,6 +95,7 @@ char * iio_context_create_xml(const struct iio_context *ctx)
 		errno = ENOMEM;
 		goto err_free_devices;
 	}
+	eptr = str + len;
 
 	if (ctx->description) {
 		iio_snprintf(str, len, "%s<context name=\"%s\" "
@@ -105,21 +107,30 @@ char * iio_context_create_xml(const struct iio_context *ctx)
 	}
 
 	ptr = strrchr(str, '\0');
+	len = eptr - ptr;
 
-	for (i = 0; i < ctx->nb_attrs; i++)
-		ptr += sprintf(ptr, "<context-attribute name=\"%s\" value=\"%s\" />",
+	for (i = 0; i < ctx->nb_attrs; i++) {
+		ptr += iio_snprintf(ptr, len, "<context-attribute name=\"%s\" value=\"%s\" />",
 				ctx->attrs[i], ctx->values[i]);
+		len = eptr - ptr;
+	}
 
 
 	for (i = 0; i < ctx->nb_devices; i++) {
-		strcpy(ptr, devices[i]);
+		iio_strlcpy(ptr, devices[i], len);
 		ptr += devices_len[i];
+		len -= devices_len[i];
 		free(devices[i]);
 	}
 
 	free(devices);
 	free(devices_len);
-	strcpy(ptr, "</context>");
+	iio_strlcpy(ptr, "</context>", len);
+	len -= sizeof("</context>");
+
+	if (len < 0)
+		IIO_ERROR("Internal libIIO error: iio_context_create_xml str length isssue\n");
+
 	return str;
 
 err_free_devices:

device.c

@@ -65,12 +65,15 @@ static char *get_attr_xml(const char *attr, size_t *length, enum iio_attr_type t
 /* Returns a string containing the XML representation of this device */
 char * iio_device_get_xml(const struct iio_device *dev, size_t *length)
 {
-	size_t len = sizeof("<device id=\"\" name=\"\" ></device>")
-		+ strlen(dev->id) + (dev->name ? strlen(dev->name) : 0);
-	char *ptr, *str, **attrs, **channels, **buffer_attrs, **debug_attrs;
+	ssize_t len;
+	char *ptr, *eptr, *str, **attrs, **channels, **buffer_attrs, **debug_attrs;
 	size_t *attrs_len, *channels_len, *buffer_attrs_len, *debug_attrs_len;
 	unsigned int i, j, k;
 
+	len = sizeof("<device id=\"\" ></device> ");
+	len += strnlen(dev->id, MAX_DEV_ID);
+	len += dev->name ? sizeof("name=\"\"") + strnlen(dev->name, MAX_DEV_NAME) : 0;
+
 	attrs_len = malloc(dev->nb_attrs * sizeof(*attrs_len));
 	if (!attrs_len)
 		return NULL;
@@ -143,56 +146,69 @@ char * iio_device_get_xml(const struct iio_device *dev, size_t *length)
 	str = malloc(len);
 	if (!str)
 		goto err_free_debug_attrs;
+	eptr = str + len;
 
 	iio_snprintf(str, len, "<device id=\"%s\"", dev->id);
 	ptr = strrchr(str, '\0');
+	len = eptr - ptr;
 
 	if (dev->name) {
-		sprintf(ptr, " name=\"%s\"", dev->name);
+		iio_snprintf(ptr, len, " name=\"%s\"", dev->name);
 		ptr = strrchr(ptr, '\0');
+		len = eptr - ptr;
 	}
 
-	strcpy(ptr, " >");
+	iio_strlcpy(ptr, " >", len);
 	ptr += 2;
+	len -= 2;
 
 	for (i = 0; i < dev->nb_channels; i++) {
-		strcpy(ptr, channels[i]);
+		iio_strlcpy(ptr, channels[i], len);
 		ptr += channels_len[i];
+		len -= channels_len[i];
 		free(channels[i]);
 	}
 
 	free(channels);
 	free(channels_len);
 
 	for (i = 0; i < dev->nb_attrs; i++) {
-		strcpy(ptr, attrs[i]);
+		iio_strlcpy(ptr, attrs[i], len);
 		ptr += attrs_len[i];
+		len -= attrs_len[i];
 		free(attrs[i]);
 	}
 
 	free(attrs);
 	free(attrs_len);
 
 	for (i = 0; i < dev->nb_buffer_attrs; i++) {
-		strcpy(ptr, buffer_attrs[i]);
+		iio_strlcpy(ptr, buffer_attrs[i], len);
 		ptr += buffer_attrs_len[i];
+		len -= buffer_attrs_len[i];
 		free(buffer_attrs[i]);
 	}
 
 	free(buffer_attrs);
 	free(buffer_attrs_len);
 
 	for (i = 0; i < dev->nb_debug_attrs; i++) {
-		strcpy(ptr, debug_attrs[i]);
+		iio_strlcpy(ptr, debug_attrs[i], len);
 		ptr += debug_attrs_len[i];
+		len -= debug_attrs_len[i];
 		free(debug_attrs[i]);
 	}
 
 	free(debug_attrs);
 	free(debug_attrs_len);
 
-	strcpy(ptr, "</device>");
+	iio_strlcpy(ptr, "</device>", len);
 	*length = ptr - str + sizeof("</device>") - 1;
+	len -= sizeof("</device>");
+
+	if (len < 0)
+		IIO_ERROR("Internal libIIO error: iio_device_get_xml str length isssue\n");
+
 	return str;
 
 err_free_debug_attrs:

iio-private.h

@@ -60,6 +60,11 @@
 #define CLEAR_BIT(addr, bit) \
 	*(((uint32_t *) addr) + BIT_WORD(bit)) &= ~BIT_MASK(bit)
 
+/* 256 is the MAX_NAME (file name) on Linux, but this might evolve over time */
+#define MAX_CHN_ID  256
+#define MAX_CHN_NAME 256
+#define MAX_DEV_ID 256
+#define MAX_DEV_NAME 256
 
 /* ntohl/htonl are a nightmare to use in cross-platform applications,
  * since they are defined in different headers on different platforms.
@@ -289,6 +294,7 @@ void iio_channel_init_finalize(struct iio_channel *chn);
 unsigned int find_channel_modifier(const char *s, size_t *len_p);
 
 char *iio_strdup(const char *str);
+void iio_strlcpy(char *dst, const char *src, ssize_t size);
 
 int iio_context_add_attr(struct iio_context *ctx,
 		const char *key, const char *value);

iiod-client.c

@@ -534,17 +534,21 @@ int iiod_client_open_unlocked(struct iiod_client *client, void *desc,
 {
 	char buf[1024], *ptr;
 	size_t i;
+	ssize_t len;
 
-	iio_snprintf(buf, sizeof(buf), "OPEN %s %lu ",
+	len = sizeof(buf);
+	iio_snprintf(buf, len, "OPEN %s %lu ",
 			iio_device_get_id(dev), (unsigned long) samples_count);
 	ptr = buf + strlen(buf);
+	len -= strnlen(buf, sizeof(buf));
 
 	for (i = dev->words; i > 0; i--, ptr += 8) {
-		iio_snprintf(ptr, (ptr - buf) + i * 8, "%08" PRIx32,
+		iio_snprintf(ptr, len, "%08" PRIx32,
 				dev->mask[i - 1]);
+		len -= 8; /* 8 characters */
 	}
 
-	strcpy(ptr, cyclic ? " CYCLIC\r\n" : "\r\n");
+	iio_strlcpy(ptr, cyclic ? " CYCLIC\r\n" : "\r\n", len);
 
 	return iiod_client_exec_command(client, desc, buf);
 }

utilities.c

@@ -214,3 +214,25 @@ char *iio_strdup(const char *str)
 	return buf;
 #endif
 }
+
+/* strlcpy is designed to be safer, more consistent, and less error prone
+ * replacements for strncpy, since it guarantees to NUL-terminate the result
+ * Note that a byte for the NUL should be included in size. This is a little
+ * different from the BSD verison, as we take a signed size, so we can
+ * call this with a negative number, (we don't check return values in libiio).
+ * We still assume src should be NUL-terminated, but it doesn't need to be.
+ */
+void iio_strlcpy(char *dst, const char *src, ssize_t size)
+{
+	size_t l = 0;
+
+	if (size <= 0)
+		return;
+
+	for (; size > 1 && *src; size--) {
+		*dst++ = *src++;
+		l++;
+	}
+	if (size)
+		*dst = '\0';
+}