[core] Harden STUN attribute parsing bounds and USERNAME copy (#3112)

Bounds and termination fixes across the STUN attribute receive path
in `handle_ice` and `switch_stun_lookup`:

- `switch_stun_packet_next_attribute` and its `_hbo` variant now
  confirm the 4-byte attribute header is fully within `end` before
  dereferencing `type`/`length`, and include the header when checking
  that the value fits, so a truncated or overrunning attribute is not
  read past the buffer.
- Compute `end_buf` as the 20-byte STUN header plus the attribute
  section (`SWITCH_STUN_PACKET_MIN_LEN + header.length`) so the walk
  covers every attribute, including trailing ones.
- Make `switch_stun_packet_next_attribute` the sole loop terminator
  and drop the redundant `xlen` guard; its seed differed between the
  two functions and could skip a trailing attribute in
  `switch_stun_lookup`.
- `switch_stun_packet_attribute_get_username` reserves a byte for the
  terminator and always NUL-terminates, since callers use the result
  as a C string.

src/include/switch_stun.h

@@ -313,9 +313,9 @@ SWITCH_DECLARE(switch_status_t) switch_stun_ip_lookup(char **external_ip, const
   \return true or false depending on if there are any more attributes
 */
 
-#define switch_stun_packet_next_attribute(attribute, end) (attribute && (attribute = (switch_stun_packet_attribute_t *) (attribute->value +  switch_stun_attribute_padded_length(attribute))) && ((void *)attribute < end) && attribute->type && (((switch_byte_t *)attribute +  switch_stun_attribute_padded_length(attribute)) < (switch_byte_t *)end))
+#define switch_stun_packet_next_attribute(attribute, end) (attribute && (attribute = (switch_stun_packet_attribute_t *) (attribute->value +  switch_stun_attribute_padded_length(attribute))) && ((switch_byte_t *)(attribute + 1) <= (switch_byte_t *)(end)) && attribute->type && (((switch_byte_t *)attribute->value +  switch_stun_attribute_padded_length(attribute)) <= (switch_byte_t *)end))
 
-#define switch_stun_packet_next_attribute_hbo(attribute, end) (attribute && (attribute = (switch_stun_packet_attribute_t *) (attribute->value +  switch_stun_attribute_padded_length_hbo(attribute))) && ((void *)attribute < end) && attribute->type && (((switch_byte_t *)attribute +  switch_stun_attribute_padded_length_hbo(attribute)) < (switch_byte_t *)end))
+#define switch_stun_packet_next_attribute_hbo(attribute, end) (attribute && (attribute = (switch_stun_packet_attribute_t *) (attribute->value +  switch_stun_attribute_padded_length_hbo(attribute))) && ((switch_byte_t *)(attribute + 1) <= (switch_byte_t *)(end)) && attribute->type && (((switch_byte_t *)attribute->value +  switch_stun_attribute_padded_length_hbo(attribute)) <= (switch_byte_t *)end))
 
 /*!
   \brief Obtain the correct length in bytes of a stun packet

src/switch_rtp.c

@@ -1013,7 +1013,6 @@ static void handle_ice(switch_rtp_t *rtp_session, switch_rtp_ice_t *ice, void *d
 	char username[STUN_USERNAME_MAX_SIZE] = { 0 };
 	unsigned char buf[1500] = { 0 };
 	switch_size_t cpylen = len;
-	int xlen = 0;
 	int ok = 1;
 	uint32_t *pri = NULL;
 	int is_rtcp = ice == &rtp_session->rtcp_ice;
@@ -1100,7 +1099,7 @@ static void handle_ice(switch_rtp_t *rtp_session, switch_rtp_ice_t *ice, void *d
 
 	calc_elapsed(rtp_session, ice);
 
-	end_buf = buf + ((sizeof(buf) > packet->header.length) ? packet->header.length : sizeof(buf));
+	end_buf = buf + ((sizeof(buf) > SWITCH_STUN_PACKET_MIN_LEN + packet->header.length) ? SWITCH_STUN_PACKET_MIN_LEN + packet->header.length : sizeof(buf));
 
 	switch_stun_packet_first_attribute(packet, attr);
 	switch_log_printf(SWITCH_CHANNEL_SESSION_LOG(rtp_session->session), SWITCH_LOG_DEBUG8, "%s STUN PACKET TYPE: %s\n",
@@ -1179,12 +1178,7 @@ static void handle_ice(switch_rtp_t *rtp_session, switch_rtp_ice_t *ice, void *d
 			break;
 		}
 
-		if (!switch_stun_packet_next_attribute(attr, end_buf)) {
-			break;
-		}
-
-		xlen += 4 + switch_stun_attribute_padded_length(attr);
-	} while (xlen <= packet->header.length);
+	} while (switch_stun_packet_next_attribute(attr, end_buf));
 
 	if ((ice->type & ICE_GOOGLE_JINGLE) && ok) {
 		ok = !strcmp(ice->user_ice, username);

src/switch_stun.c

@@ -459,8 +459,16 @@ SWITCH_DECLARE(char *) switch_stun_packet_attribute_get_username(switch_stun_pac
 {
 	uint16_t cpylen;
 
-	cpylen = attribute->length < len ? attribute->length : len;
-	return memcpy(username, attribute->value, cpylen);
+	if (!len) {
+		return username;
+	}
+
+	/* Reserve one byte for the terminator and always NUL-terminate: callers treat the result as a C string. */
+	cpylen = attribute->length < len ? attribute->length : (uint16_t)(len - 1);
+	memcpy(username, attribute->value, cpylen);
+	username[cpylen] = '\0';
+
+	return username;
 }
 
 SWITCH_DECLARE(switch_stun_packet_t *) switch_stun_packet_build_header(switch_stun_message_t type, char *id, uint8_t *buf)
@@ -836,7 +844,6 @@ SWITCH_DECLARE(switch_status_t) switch_stun_lookup(char **ip,
 	switch_time_t started = 0;
 	int funny = 0;
 	int size = sizeof(buf);
-	int xlen = sizeof(switch_stun_packet_header_t);
 	switch_status_t res;
 
 	switch_assert(err);
@@ -931,7 +938,7 @@ SWITCH_DECLARE(switch_status_t) switch_stun_lookup(char **ip,
 		return SWITCH_STATUS_FALSE;
 	}
 
-	end_buf = buf + ((sizeof(buf) > packet->header.length) ? packet->header.length : sizeof(buf));
+	end_buf = buf + ((sizeof(buf) > SWITCH_STUN_PACKET_MIN_LEN + packet->header.length) ? SWITCH_STUN_PACKET_MIN_LEN + packet->header.length : sizeof(buf));
 
 	switch_stun_packet_first_attribute(packet, attr);
 	switch_assert(attr);
@@ -954,13 +961,7 @@ SWITCH_DECLARE(switch_status_t) switch_stun_lookup(char **ip,
 			break;
 		}
 
-		if (!switch_stun_packet_next_attribute(attr, end_buf)) {
-			break;
-		}
-
-		xlen += 4 + switch_stun_attribute_padded_length(attr);
-
-	} while (xlen <= packet->header.length);
+	} while (switch_stun_packet_next_attribute(attr, end_buf));
 
 	if (packet->header.type == SWITCH_STUN_BINDING_RESPONSE) {
 		*ip = switch_core_strdup(pool, rip);

tests/unit/switch_stun.c

@@ -312,6 +312,169 @@ FST_TEARDOWN_END()
 				   "a MESSAGE-INTEGRITY-SHA256 attribute after MESSAGE-INTEGRITY is tolerated");
 	}
 	FST_TEST_END()
+
+	FST_TEST_BEGIN(test_stun_get_username_terminates)
+	{
+		/* get_username must always NUL-terminate within the caller's buffer, even when the
+		   attribute value is as long as or longer than the buffer: callers use the result as a C string. */
+		uint8_t abuf[128] = { 0 };
+		switch_stun_packet_attribute_t *attr = (switch_stun_packet_attribute_t *)abuf;
+		char dst[32];
+		char *ret;
+		int i;
+
+		attr->type = htons(SWITCH_STUN_ATTR_USERNAME);
+		attr->length = 64;	/* host order: the accessor reads attribute->length directly, as post-parse callers do */
+		for (i = 0; i < 64; i++) {
+			attr->value[i] = 'A';
+		}
+
+		memset(dst, 'x', sizeof(dst));
+		ret = switch_stun_packet_attribute_get_username(attr, dst, sizeof(dst));
+		fst_xcheck(ret == dst, "get_username returns the destination buffer");
+		fst_xcheck(dst[sizeof(dst) - 1] == '\0', "over-long USERNAME is NUL-terminated at the last byte");
+		fst_xcheck(strlen(dst) == sizeof(dst) - 1, "over-long USERNAME is truncated to len-1");
+
+		attr->length = 5;
+		memcpy(attr->value, "abcde", 5);
+		memset(dst, 'x', sizeof(dst));
+		switch_stun_packet_attribute_get_username(attr, dst, sizeof(dst));
+		fst_xcheck(strlen(dst) == 5, "short USERNAME is copied and terminated at its own length");
+		fst_check_string_equals(dst, "abcde");
+	}
+	FST_TEST_END()
+
+	FST_TEST_BEGIN(test_stun_next_attribute_walks_whole_attributes)
+	{
+		/* Positive test: the iterator visits each whole attribute and stops exactly at the end of the
+		   last one, including an attribute whose value ends on the buffer boundary. Guards against a
+		   future change breaking normal iteration; it does not distinguish the header/TLV bounds fix
+		   (a naive iterator passes it too). */
+		uint8_t buf[8] = { 0 };
+		switch_stun_packet_attribute_t *attr;
+		void *end = buf + sizeof(buf);
+
+		attr = (switch_stun_packet_attribute_t *)buf;
+		attr->type = htons(SWITCH_STUN_ATTR_USERNAME);
+		attr->length = 0;
+		attr = (switch_stun_packet_attribute_t *)(buf + 4);
+		attr->type = htons(SWITCH_STUN_ATTR_PRIORITY);
+		attr->length = 0;
+
+		attr = (switch_stun_packet_attribute_t *)buf;
+		fst_xcheck(switch_stun_packet_next_attribute(attr, end) != 0, "iterator advances to the second whole attribute");
+		fst_xcheck((uint8_t *)attr == buf + 4, "iterator lands exactly on the second attribute");
+		fst_xcheck(switch_stun_packet_next_attribute(attr, end) == 0, "iterator stops after the last whole attribute");
+	}
+	FST_TEST_END()
+
+	FST_TEST_BEGIN(test_stun_next_attribute_truncated_header)
+	{
+		/* After the first attribute only 2 bytes remain before end, so the next attribute's 4-byte
+		   header does not fit. The iterator must confirm the header is fully in-bounds before reading
+		   it and stop; reading attribute->length here would run past the buffer. The trailing bytes are
+		   non-zero so the type sentinel does not stop the walk first, so the out-of-bounds read (if the
+		   header check is missing) is exercised and caught under ASAN. */
+		uint8_t buf[6] = { 0 };
+		switch_stun_packet_attribute_t *attr = (switch_stun_packet_attribute_t *)buf;
+		void *end = buf + sizeof(buf);
+
+		attr->type = htons(SWITCH_STUN_ATTR_USERNAME);
+		attr->length = 0;
+		buf[4] = 0xff;	/* non-zero type for the truncated trailing header */
+		buf[5] = 0xff;
+
+		fst_xcheck(switch_stun_packet_next_attribute(attr, end) == 0, "iterator stops at a truncated trailing attribute header without reading past end");
+	}
+	FST_TEST_END()
+
+	FST_TEST_BEGIN(test_stun_next_attribute_hbo)
+	{
+		/* Positive test for the _hbo iterator, which reads attribute lengths in network byte order
+		   (ntohs) for callers walking raw on-the-wire packets that have not been through
+		   switch_stun_packet_parse, since parse byte-swaps type and length in place: it must advance by
+		   the network-order length and stop at the end. The plain macro would misread these
+		   network-order lengths; this confirms the _hbo variant walks normal attributes, not a bounds
+		   regression. */
+		uint8_t buf[16] = { 0 };
+		switch_stun_packet_attribute_t *attr;
+		void *end = buf + sizeof(buf);
+
+		attr = (switch_stun_packet_attribute_t *)buf;
+		attr->type = htons(SWITCH_STUN_ATTR_USERNAME);
+		attr->length = htons(4);	/* network order: _hbo applies ntohs, the plain macro would misread this */
+		attr = (switch_stun_packet_attribute_t *)(buf + 8);
+		attr->type = htons(SWITCH_STUN_ATTR_PRIORITY);
+		attr->length = htons(4);
+
+		attr = (switch_stun_packet_attribute_t *)buf;
+		fst_xcheck(switch_stun_packet_next_attribute_hbo(attr, end) != 0, "hbo iterator advances to the second attribute");
+		fst_xcheck((uint8_t *)attr == buf + 8, "hbo iterator lands on the second attribute using the network-order length");
+		fst_xcheck(switch_stun_packet_next_attribute_hbo(attr, end) == 0, "hbo iterator stops after the last attribute");
+	}
+	FST_TEST_END()
+
+	FST_TEST_BEGIN(test_stun_next_attribute_value_overruns)
+	{
+		/* An attribute whose 4-byte header fits before end but whose declared value extends past end
+		   (more value bytes than remain) must be rejected, so the caller never reads the value out of
+		   bounds. This exercises the value-length bound distinctly from a truncated header. */
+		uint8_t buf[12] = { 0 };
+		switch_stun_packet_attribute_t *attr;
+		void *end = buf + 10;	/* ends inside the second attribute's declared value */
+
+		attr = (switch_stun_packet_attribute_t *)buf;
+		attr->type = htons(SWITCH_STUN_ATTR_USERNAME);
+		attr->length = 0;	/* host order: empty leading attribute */
+		attr = (switch_stun_packet_attribute_t *)(buf + 4);
+		attr->type = htons(SWITCH_STUN_ATTR_PRIORITY);
+		attr->length = 4;	/* value would occupy buf[8..11], past end (buf+10) */
+
+		attr = (switch_stun_packet_attribute_t *)buf;
+		fst_xcheck(switch_stun_packet_next_attribute(attr, end) == 0, "attribute whose value overruns end is rejected");
+	}
+	FST_TEST_END()
+
+	FST_TEST_BEGIN(test_stun_walk_reaches_trailing_attribute)
+	{
+		/* Positive test: parse + walk over a well-formed multi-attribute packet reaches every attribute,
+		   including a trailing one whose value ends on the last byte of the message. end_buf is computed
+		   here rather than taken from switch_stun_lookup or handle_ice, so how those callers derive it is
+		   not covered. */
+		uint8_t buf[512] = { 0 };
+		switch_stun_packet_t *packet;
+		switch_stun_packet_attribute_t *attr;
+		void *end_buf;
+		int count;
+
+		packet = switch_stun_packet_build_header(SWITCH_STUN_BINDING_RESPONSE, NULL, buf);
+
+		attr = (switch_stun_packet_attribute_t *)packet->first_attribute;
+		attr->type = htons(SWITCH_STUN_ATTR_USERNAME);
+		attr->length = htons(4);
+		memcpy(attr->value, "abcd", 4);
+
+		attr = (switch_stun_packet_attribute_t *)(packet->first_attribute + 8);
+		attr->type = htons(SWITCH_STUN_ATTR_USERNAME);
+		attr->length = htons(4);
+		memcpy(attr->value, "efgh", 4);
+
+		packet->header.length = htons(8 + 8);
+
+		packet = switch_stun_packet_parse(buf, SWITCH_STUN_PACKET_MIN_LEN + 8 + 8);
+		fst_requires(packet != NULL);
+
+		/* Same end_buf the iterator's callers use: the 20-byte header plus the attribute section. */
+		end_buf = buf + SWITCH_STUN_PACKET_MIN_LEN + packet->header.length;
+
+		switch_stun_packet_first_attribute(packet, attr);
+		count = 1;
+		while (switch_stun_packet_next_attribute(attr, end_buf)) {
+			count++;
+		}
+		fst_xcheck(count == 2, "iterator reaches the trailing attribute in the last bytes of the message");
+	}
+	FST_TEST_END()
 }
 FST_SUITE_END()
 }